This is an automated email from the ASF dual-hosted git repository. spmallette pushed a commit to branch tinkergraph-storage in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 1d9198f03fa6345317ba9aa3f0154aa1ee49237e Author: Stephen Mallette <[email protected]> AuthorDate: Tue Aug 4 15:15:22 2026 +0000 Lock TinkerStorageGraph storage directory to a single writer A persistent TinkerStorageGraph is a single-writer embedded store, but nothing stopped a second graph — in the same JVM or another process — from opening the same location and corrupting its log and snapshot. open() now takes an exclusive OS advisory lock on a LOCK file in the storage directory, held for the graph's lifetime and released on close(); a second open fails fast with a clear error. An OS lock is used so the kernel releases it if the JVM dies. Assisted-by: Claude Code:claude-opus-4-8 --- CHANGELOG.asciidoc | 2 +- .../tinkergraph/structure/AbstractTinkerGraph.java | 13 +++ .../tinkergraph/structure/TinkerStorageGraph.java | 25 ++++- .../structure/storage/DirectoryLock.java | 115 +++++++++++++++++++++ .../structure/storage/DirectoryLockTest.java | 101 ++++++++++++++++++ .../structure/storage/GraphBinaryStorageTest.java | 15 ++- 6 files changed, 261 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 414863edeb..40c46500c6 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -28,7 +28,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima * Fixed `gremlin-go` to report a malformed or truncated GraphBinary response as a deserialization error rather than a bare decoder message. * Made `TinkerGraph` an interface and renamed the in-memory implementation to `TinkerMemoryGraph`; `TinkerGraph.open()` and `gremlin.graph=...TinkerGraph` behave as before. *(breaking)* * Renamed `TinkerTransactionGraph` to `TinkerStorageGraph`. *(breaking)* -* Added a pluggable storage layer to `TinkerStorageGraph` that durably persists each committed transaction to disk, selected with the `gremlin.tinkergraph.storage` config key and shipping a GraphBinary engine, with a `gremlin.tinkergraph.storage.sync` key to choose `commit` (fsync per commit) or `os` durability. +* Added a pluggable storage layer to `TinkerStorageGraph` that durably persists each committed transaction to disk, selected with the `gremlin.tinkergraph.storage` config key and shipping a GraphBinary engine, with a `gremlin.tinkergraph.storage.sync` key to choose `commit` (fsync per commit) or `os` durability; a storage location is locked to a single writer, so opening one already in use fails fast. * Removed automatic persistence from `TinkerMemoryGraph`, which is now purely in-memory and ignores `gremlin.tinkergraph.graphLocation`/`graphFormat`. Use `TinkerStorageGraph` for durability or `g.io()` for interchange. *(breaking)* [[release-4-0-0-beta-3]] diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerGraph.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerGraph.java index bb663ad0bd..17e359b94f 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerGraph.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/AbstractTinkerGraph.java @@ -36,6 +36,7 @@ import org.apache.tinkerpop.gremlin.tinkergraph.process.computer.TinkerGraphComp import org.apache.tinkerpop.gremlin.gql.GqlDeclarativeMatchStrategy; import org.apache.tinkerpop.gremlin.tinkergraph.services.TinkerServiceRegistry; import org.apache.tinkerpop.gremlin.tinkergraph.structure.storage.DefaultStorage; +import org.apache.tinkerpop.gremlin.tinkergraph.structure.storage.DirectoryLock; import org.apache.tinkerpop.gremlin.tinkergraph.structure.storage.TinkerStorage; import java.lang.reflect.InvocationTargetException; @@ -85,6 +86,12 @@ public abstract class AbstractTinkerGraph implements TinkerGraph { */ protected TinkerStorage storage; + /** + * Exclusive lock on the storage directory, held for the graph's lifetime so no second graph — in this or another + * process — can open the same location and corrupt its files. {@code null} when the graph is purely in-memory. + */ + protected DirectoryLock directoryLock; + /** * Serializes the durable write of a committing transaction. TinkerGraph transactions lock only their own changed * elements, so two commits touching disjoint elements run their commit paths concurrently; without this lock they @@ -327,6 +334,12 @@ public abstract class AbstractTinkerGraph implements TinkerGraph { storage.close(); } finally { storageCommitLock.unlock(); + // release the exclusive directory lock last, so the location is only reopenable once the engine has + // fully released its files + if (directoryLock != null) { + directoryLock.close(); + directoryLock = null; + } } } serviceRegistry.close(); diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerStorageGraph.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerStorageGraph.java index 409f618f25..0eced39af1 100644 --- a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerStorageGraph.java +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/TinkerStorageGraph.java @@ -35,8 +35,10 @@ import org.apache.tinkerpop.gremlin.gql.GqlDeclarativeMatchStrategy; import org.apache.tinkerpop.gremlin.tinkergraph.process.traversal.strategy.optimization.TinkerGraphCountStrategy; import org.apache.tinkerpop.gremlin.tinkergraph.process.traversal.strategy.optimization.TinkerGraphStepStrategy; import org.apache.tinkerpop.gremlin.tinkergraph.services.TinkerServiceRegistry; +import org.apache.tinkerpop.gremlin.tinkergraph.structure.storage.DirectoryLock; import org.apache.tinkerpop.gremlin.util.iterator.IteratorUtils; +import java.io.File; import java.util.Arrays; import java.util.Collections; import java.util.Iterator; @@ -107,12 +109,25 @@ public final class TinkerStorageGraph extends AbstractTinkerGraph { serviceRegistry.registerService(instantiate(serviceClass))); if (storage != null) { - storage.open(this, configuration); - loading = true; + // take an exclusive lock on the storage directory before the engine touches any files, so a second graph + // on the same location fails fast rather than corrupting it. The directory must exist to hold the lock. + final File dir = new File(graphLocation); + if (!dir.isDirectory() && !dir.mkdirs()) + throw new IllegalStateException(String.format("Could not create storage directory %s", dir)); + directoryLock = DirectoryLock.acquire(dir); try { - storage.replay(this); - } finally { - loading = false; + storage.open(this, configuration); + loading = true; + try { + storage.replay(this); + } finally { + loading = false; + } + } catch (RuntimeException | Error ex) { + // don't leak the lock if the engine fails to open or replay + directoryLock.close(); + directoryLock = null; + throw ex; } } } diff --git a/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/DirectoryLock.java b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/DirectoryLock.java new file mode 100644 index 0000000000..061e4e3372 --- /dev/null +++ b/tinkergraph-gremlin/src/main/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/DirectoryLock.java @@ -0,0 +1,115 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.tinkergraph.structure.storage; + +import java.io.File; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.channels.FileChannel; +import java.nio.channels.FileLock; +import java.nio.channels.OverlappingFileLockException; +import java.nio.file.StandardOpenOption; + +/** + * An exclusive, whole-process lock on a {@code TinkerStorageGraph} storage directory. A persistent, transactional + * TinkerGraph is a single-writer embedded store: two graphs opened on the same directory — whether in one JVM or + * across processes — would both append to and compact the same files, corrupting them. This holds an OS advisory lock + * ({@link FileLock}) on a {@code LOCK} file in the directory for the lifetime of the graph, so a second open fails + * fast rather than silently corrupting data. + * <p/> + * An OS lock (rather than a mere marker file) is used so the kernel releases it automatically if the JVM dies, + * avoiding a stale lock that would wedge the store after a crash. + * <p/> + * Note: {@link FileLock} semantics are unreliable on some network filesystems (notably NFS); this guarantee holds on + * local filesystems. + */ +public final class DirectoryLock implements AutoCloseable { + + static final String LOCK_FILE = "LOCK"; + + private final FileChannel channel; + private final FileLock lock; + private final File lockFile; + + private DirectoryLock(final FileChannel channel, final FileLock lock, final File lockFile) { + this.channel = channel; + this.lock = lock; + this.lockFile = lockFile; + } + + /** + * Acquire an exclusive lock on the {@code LOCK} file within {@code directory}. + * + * @param directory the storage directory, which must already exist + * @return the held lock, released by {@link #close()} + * @throws IllegalStateException if another graph (in this or another process) already holds the lock + */ + public static DirectoryLock acquire(final File directory) { + final File lockFile = new File(directory, LOCK_FILE); + FileChannel channel = null; + try { + channel = FileChannel.open(lockFile.toPath(), + StandardOpenOption.CREATE, StandardOpenOption.WRITE); + final FileLock lock = channel.tryLock(); + if (lock == null) { + channel.close(); + throw lockedByAnother(directory, null); + } + return new DirectoryLock(channel, lock, lockFile); + } catch (OverlappingFileLockException ofle) { + // another graph in *this* JVM already holds (or is acquiring) the lock on this file + closeQuietly(channel); + throw lockedByAnother(directory, ofle); + } catch (IOException ex) { + closeQuietly(channel); + throw new UncheckedIOException(String.format("Could not acquire storage lock for %s", directory), ex); + } + } + + private static IllegalStateException lockedByAnother(final File directory, final Throwable cause) { + return new IllegalStateException(String.format( + "Storage location %s is already in use by another TinkerStorageGraph (in this or another process); " + + "a persistent TinkerStorageGraph allows only a single writer", directory), cause); + } + + private static void closeQuietly(final FileChannel channel) { + if (channel != null) { + try { + channel.close(); + } catch (IOException ignored) { + // best effort on the failure path + } + } + } + + /** + * Release the lock and close the channel. Idempotent-friendly: safe to call once per acquired lock. + */ + @Override + public void close() { + try { + if (lock.isValid()) + lock.release(); + } catch (IOException ex) { + throw new UncheckedIOException(String.format("Could not release storage lock %s", lockFile), ex); + } finally { + closeQuietly(channel); + } + } +} diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/DirectoryLockTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/DirectoryLockTest.java new file mode 100644 index 0000000000..6856dddbf2 --- /dev/null +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/DirectoryLockTest.java @@ -0,0 +1,101 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.tinkergraph.structure.storage; + +import org.apache.commons.configuration2.BaseConfiguration; +import org.apache.commons.configuration2.Configuration; +import org.apache.tinkerpop.gremlin.structure.Graph; +import org.apache.tinkerpop.gremlin.structure.T; +import org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerGraph; +import org.apache.tinkerpop.gremlin.tinkergraph.structure.TinkerStorageGraph; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.File; +import java.nio.channels.FileChannel; +import java.nio.channels.FileLock; +import java.nio.file.StandardOpenOption; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +/** + * Verifies that a {@link TinkerStorageGraph} takes an exclusive lock on its storage directory so a second opener on + * the same location fails fast rather than corrupting the store. + */ +public class DirectoryLockTest { + + @Rule + public TemporaryFolder tempFolder = new TemporaryFolder(); + + private String location; + + @Before + public void setUp() throws Exception { + location = tempFolder.newFolder("storage").getAbsolutePath(); + } + + private Configuration config() { + final Configuration conf = new BaseConfiguration(); + conf.setProperty(Graph.GRAPH, TinkerStorageGraph.class.getName()); + conf.setProperty(TinkerGraph.GREMLIN_TINKERGRAPH_STORAGE, "graphbinary"); + conf.setProperty(TinkerGraph.GREMLIN_TINKERGRAPH_GRAPH_LOCATION, location); + return conf; + } + + @Test + public void shouldReleaseLockOnCloseAndAllowReopen() { + // sequential open/close must not leave the directory wedged + TinkerStorageGraph graph = TinkerStorageGraph.open(config()); + graph.addVertex(T.id, 1); + graph.tx().commit(); + graph.close(); + + graph = TinkerStorageGraph.open(config()); + assertNotNull(graph.vertices(1).next()); + graph.close(); + } + + @Test + public void shouldRejectSecondOpenWhileLocationIsLocked() throws Exception { + // simulate another process holding the directory lock by taking the OS lock on the LOCK file directly + final File dir = new File(location); + dir.mkdirs(); + final File lockFile = new File(dir, DirectoryLock.LOCK_FILE); + try (final FileChannel channel = FileChannel.open(lockFile.toPath(), + StandardOpenOption.CREATE, StandardOpenOption.WRITE); + final FileLock held = channel.lock()) { + assertNotNull(held); + try { + TinkerStorageGraph.open(config()); + fail("expected open to fail while the storage location is locked"); + } catch (IllegalStateException expected) { + assertTrue("message should name the location: " + expected.getMessage(), + expected.getMessage().contains(location)); + } + } + + // once the simulated holder releases (try-with-resources above), the location opens normally + final TinkerStorageGraph graph = TinkerStorageGraph.open(config()); + graph.close(); + } +} diff --git a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorageTest.java b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorageTest.java index 4bdf5b8888..05e9970504 100644 --- a/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorageTest.java +++ b/tinkergraph-gremlin/src/test/java/org/apache/tinkerpop/gremlin/tinkergraph/structure/storage/GraphBinaryStorageTest.java @@ -26,6 +26,7 @@ import org.junit.Test; import java.io.File; import java.io.RandomAccessFile; +import java.nio.file.Files; import java.util.Iterator; import static org.junit.Assert.assertEquals; @@ -112,14 +113,20 @@ public class GraphBinaryStorageTest extends AbstractTinkerStorageConformanceTest graph.tx().commit(); graph.addVertex(T.id, 2, "value", 2); graph.tx().commit(); - // do NOT close (avoid compaction) so the raw log is preserved graph.tx().close(); - // simulate a crash mid-append by appending a partial (garbage) trailing frame to the log + // capture the raw log (two good frames) while the graph holds it, then close to release the directory lock. + // close() compacts, folding the log into a snapshot, so we reconstruct a "crashed" on-disk state below. final File logFile = new File(location, GraphBinaryStorage.LOG_FILE); + final byte[] goodLog = Files.readAllBytes(logFile.toPath()); + graph.close(); + + // recreate the pre-crash layout: no snapshot, a log of the two good frames plus a torn trailing frame (a + // length prefix promising 100 bytes with only a couple following), as an interrupted append would leave. + Files.deleteIfExists(new File(location, GraphBinaryStorage.SNAPSHOT_FILE).toPath()); try (final RandomAccessFile raf = new RandomAccessFile(logFile, "rw")) { - raf.seek(raf.length()); - // a length prefix promising 100 bytes, but only a couple follow — a torn write + raf.setLength(0); + raf.write(goodLog); raf.writeInt(100); raf.write(new byte[]{ 0x01, 0x02 }); }
