This is an automated email from the ASF dual-hosted git repository. Cole-Greer pushed a commit to branch docs-3.7 in repository https://gitbox.apache.org/repos/asf/tinkerpop.git
commit 460c27d1db1e44edde9a8e1180c827340ddaab07 Author: Cole Greer <[email protected]> AuthorDate: Fri Jun 5 08:59:46 2026 -0700 Harden plugin directory toggling against stale ext-disabled state (tinkerpop-6jq.7) PluginDirectoryRestartHandler moved ext/<plugin> to ext-disabled/<plugin> with Files.move(REPLACE_EXISTING), which throws DirectoryNotEmptyException when ext-disabled/<plugin> already exists as a non-empty directory. An interrupted docs build leaves such a directory, poisoning the next run with "Failed to restart console with excluded plugins". Make the toggle idempotent and source-authoritative: clear any stale destination before moving when disabling, and when enabling drop a leftover disabled duplicate if the plugin is already present in ext/. Also clear ext-disabled/ at the start of bin/process-docs.sh so each build begins from a known state. Adds PluginDirectoryRestartHandlerTest covering the round-trip, double-exclude, and stale-state scenarios. Assisted-by: Kiro:claude-opus-4.8 [kiro-cli] --- bin/process-docs.sh | 6 + .../docs/PluginDirectoryRestartHandler.java | 35 +++++- .../docs/PluginDirectoryRestartHandlerTest.java | 138 +++++++++++++++++++++ 3 files changed, 175 insertions(+), 4 deletions(-) diff --git a/bin/process-docs.sh b/bin/process-docs.sh index 90dee94934..10bd27c8a4 100755 --- a/bin/process-docs.sh +++ b/bin/process-docs.sh @@ -103,6 +103,12 @@ if [ -z "${SERVER_DIR}" ] || [ ! -d "${SERVER_DIR}" ]; then fi SERVER_HOME="$(cd "${SERVER_DIR}" && pwd)" +# Clear any plugin directories parked aside by a previous (possibly interrupted) run. The +# extension moves ext/<plugin> to ext-disabled/<plugin> when excluding a plugin per-book; a +# leftover ext-disabled/ from a crashed build would otherwise shadow the freshly-installed +# plugins and break the next run's console restarts. +rm -rf "${CONSOLE_HOME}/ext-disabled" + # 3. Install plugins into console echo "Installing plugins into console..." diff --git a/tools/tinkerpop-docs/src/main/java/org/apache/tinkerpop/gremlin/docs/PluginDirectoryRestartHandler.java b/tools/tinkerpop-docs/src/main/java/org/apache/tinkerpop/gremlin/docs/PluginDirectoryRestartHandler.java index ea1763e906..078cf193b0 100644 --- a/tools/tinkerpop-docs/src/main/java/org/apache/tinkerpop/gremlin/docs/PluginDirectoryRestartHandler.java +++ b/tools/tinkerpop-docs/src/main/java/org/apache/tinkerpop/gremlin/docs/PluginDirectoryRestartHandler.java @@ -21,7 +21,6 @@ package org.apache.tinkerpop.gremlin.docs; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.StandardCopyOption; import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; @@ -74,23 +73,51 @@ final class PluginDirectoryRestartHandler implements ConsoleRestartHandler { private void disable(final String plugin) throws IOException { final Path active = extDir.resolve(plugin); + final Path disabled = disabledDir.resolve(plugin); if (Files.isDirectory(active)) { + // The active copy is authoritative. Clear any stale disabled copy left by an + // interrupted run before moving, since Files.move(REPLACE_EXISTING) cannot replace + // a non-empty directory. Files.createDirectories(disabledDir); - Files.move(active, disabledDir.resolve(plugin), StandardCopyOption.REPLACE_EXISTING); + deleteRecursively(disabled); + Files.move(active, disabled); LOG.info("Excluded plugin: " + plugin); } setPluginEnabled(TOGGLEABLE.get(plugin), false); } private void enable(final String plugin) throws IOException { + final Path active = extDir.resolve(plugin); final Path disabled = disabledDir.resolve(plugin); if (Files.isDirectory(disabled)) { - Files.move(disabled, extDir.resolve(plugin), StandardCopyOption.REPLACE_EXISTING); - LOG.info("Restored plugin: " + plugin); + if (Files.isDirectory(active)) { + // Plugin is already present in ext/ (e.g. freshly installed); the active copy + // wins. Just drop the leftover disabled duplicate. + deleteRecursively(disabled); + } else { + Files.move(disabled, active); + LOG.info("Restored plugin: " + plugin); + } } setPluginEnabled(TOGGLEABLE.get(plugin), true); } + /** Recursively deletes a directory tree if it exists; a no-op otherwise. */ + private static void deleteRecursively(final Path path) throws IOException { + if (!Files.exists(path)) return; + try (java.util.stream.Stream<Path> walk = Files.walk(path)) { + walk.sorted(java.util.Comparator.reverseOrder()).forEach(p -> { + try { + Files.delete(p); + } catch (final IOException e) { + throw new java.io.UncheckedIOException(e); + } + }); + } catch (final java.io.UncheckedIOException e) { + throw e.getCause(); + } + } + /** Adds or removes a single activation class line in ext/plugins.txt, preserving the rest. */ private void setPluginEnabled(final String pluginClass, final boolean enabled) throws IOException { if (!Files.exists(pluginsTxt)) return; diff --git a/tools/tinkerpop-docs/src/test/java/org/apache/tinkerpop/gremlin/docs/PluginDirectoryRestartHandlerTest.java b/tools/tinkerpop-docs/src/test/java/org/apache/tinkerpop/gremlin/docs/PluginDirectoryRestartHandlerTest.java new file mode 100644 index 0000000000..d42367cb6a --- /dev/null +++ b/tools/tinkerpop-docs/src/test/java/org/apache/tinkerpop/gremlin/docs/PluginDirectoryRestartHandlerTest.java @@ -0,0 +1,138 @@ +/* + * 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.docs; + +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +/** + * Unit tests for {@link PluginDirectoryRestartHandler} verifying that toggling plugin + * directories is idempotent and resilient to stale {@code ext-disabled/} state left by an + * interrupted build. + */ +public class PluginDirectoryRestartHandlerTest { + + @Rule + public final TemporaryFolder tmp = new TemporaryFolder(); + + private static final String NEO4J = "neo4j-gremlin"; + private static final String SPARK = "spark-gremlin"; + private static final String NEO4J_CLASS = "org.apache.tinkerpop.gremlin.neo4j.jsr223.Neo4jGremlinPlugin"; + private static final String SPARK_CLASS = "org.apache.tinkerpop.gremlin.spark.jsr223.SparkGremlinPlugin"; + + private Path consoleHome; + private Path ext; + private Path disabled; + private Path pluginsTxt; + private PluginDirectoryRestartHandler handler; + + @Before + public void setUp() throws IOException { + consoleHome = tmp.getRoot().toPath(); + ext = Files.createDirectories(consoleHome.resolve("ext")); + disabled = consoleHome.resolve("ext-disabled"); + pluginsTxt = ext.resolve("plugins.txt"); + // Seed a populated plugin layout with a non-empty plugin dir for each toggleable plugin. + for (final String p : Arrays.asList(NEO4J, SPARK, "hadoop-gremlin")) { + installPlugin(p); + } + Files.write(pluginsTxt, Arrays.asList( + "org.apache.tinkerpop.gremlin.tinkergraph.jsr223.TinkerGraphGremlinPlugin", + NEO4J_CLASS, SPARK_CLASS, + "org.apache.tinkerpop.gremlin.hadoop.jsr223.HadoopGremlinPlugin")); + handler = new PluginDirectoryRestartHandler(consoleHome); + } + + private void installPlugin(final String plugin) throws IOException { + final Path dir = Files.createDirectories(ext.resolve(plugin).resolve("plugin")); + Files.write(dir.resolve(plugin + ".jar"), "jar-bytes".getBytes(StandardCharsets.UTF_8)); + } + + @Test + public void shouldDisableAndReEnablePlugin() throws IOException { + handler.onRestart(Collections.singletonList(SPARK)); + assertThat(Files.isDirectory(ext.resolve(SPARK)), is(false)); + assertThat(Files.isDirectory(disabled.resolve(SPARK)), is(true)); + assertThat(pluginClasses().contains(SPARK_CLASS), is(false)); + + // A later book with no exclusions restores everything. + handler.onRestart(Collections.emptyList()); + assertThat(Files.isDirectory(ext.resolve(SPARK)), is(true)); + assertThat(Files.isDirectory(disabled.resolve(SPARK)), is(false)); + assertThat(pluginClasses().contains(SPARK_CLASS), is(true)); + // The plugin jar survived the round trip. + assertThat(Files.exists(ext.resolve(SPARK).resolve("plugin").resolve(SPARK + ".jar")), is(true)); + } + + @Test + public void shouldBeIdempotentWhenExcludingTwice() throws IOException { + handler.onRestart(Collections.singletonList(NEO4J)); + // Excluding the same plugin again must not throw, even though ext-disabled/neo4j already exists. + handler.onRestart(Collections.singletonList(NEO4J)); + assertThat(Files.isDirectory(ext.resolve(NEO4J)), is(false)); + assertThat(Files.isDirectory(disabled.resolve(NEO4J)), is(true)); + } + + @Test + public void shouldRecoverFromStaleDisabledDirectoryLeftByInterruptedRun() throws IOException { + // Simulate a crashed prior run: a non-empty ext-disabled/neo4j exists AND ext/neo4j was + // re-installed by process-docs.sh, so the plugin is present in BOTH locations. + Files.createDirectories(disabled.resolve(NEO4J).resolve("plugin")); + Files.write(disabled.resolve(NEO4J).resolve("plugin").resolve("stale.jar"), + "stale".getBytes(StandardCharsets.UTF_8)); + + // Disabling must not throw (the move target already exists and is non-empty). + handler.onRestart(Collections.singletonList(NEO4J)); + assertThat(Files.isDirectory(ext.resolve(NEO4J)), is(false)); + assertThat(Files.isDirectory(disabled.resolve(NEO4J)), is(true)); + // The authoritative active copy replaced the stale one (no stale.jar remains). + assertThat(Files.exists(disabled.resolve(NEO4J).resolve("plugin").resolve("stale.jar")), is(false)); + assertThat(Files.exists(disabled.resolve(NEO4J).resolve("plugin").resolve(NEO4J + ".jar")), is(true)); + } + + @Test + public void shouldEnableCleanlyWhenPluginPresentInBothLocations() throws IOException { + // ext-disabled/spark left over AND ext/spark freshly installed: enabling drops the duplicate. + Files.createDirectories(disabled.resolve(SPARK).resolve("plugin")); + Files.write(disabled.resolve(SPARK).resolve("plugin").resolve("stale.jar"), + "stale".getBytes(StandardCharsets.UTF_8)); + + handler.onRestart(Collections.emptyList()); + assertThat(Files.isDirectory(ext.resolve(SPARK)), is(true)); + assertThat(Files.isDirectory(disabled.resolve(SPARK)), is(false)); + assertThat(pluginClasses().contains(SPARK_CLASS), is(true)); + } + + private List<String> pluginClasses() throws IOException { + return Files.readAllLines(pluginsTxt); + } +}
