jnats-server-runner 4.0.2 across the repo, chaos-runner fixes

[scottf]

All eleven jnats-server-runner declarations move from 3.1.1 to 4.0.2. The ten test-only modules needed the version string and nothing else.

4.0.2 rather than 4.0.1 because 4.0.1 did not escape a store dir it was handed. That is a startup failure rather than a wrong directory — the nats-server conf parser treats \ as an escape even in an unquoted value, so store_dir=C:\temp\x fails to parse on \t. It was fixed upstream in nats-io/java-nats-server-runner#47 rather than worked around here, so chaos-runner carries nothing for it.

chaos-runner, adapting to 4.x

• getPort() is gone. The three call sites use getNatsPort(), the resolved client port, rather than getReadyPort(), which is only the port that was asked for or generated — they differ when there is no top level port and the server falls back to 4222, or when the ready port turns out to be a non-nats port. getConnectionUrls() already reported node.port, so the resolved port is the consistent one. getNatsPort() existed in 3.1.1 too, so this is a correction rather than a rename.
• The single server branch no longer hand rolls its jetstream block and escapes the store dir inline. It calls JsConfig, the same as the cluster branch gets by way of createClusterInserts, so neither branch escapes anything itself.

chaos-runner bugs found while verifying

None of these are version related; they were all reachable before.

• upTask leaked a server on shutdown. It never took INSTANCE_LOCK, and executor.shutdown() does not interrupt a task already running, so an in-flight upTask finished createRunner after shutdownServers() had closed out the list and the jvm hook was removed. The new server went into a list nobody reads again and nothing ever closed it — it outlived the jvm still holding its port, which is what made a later run fail to start. Reproduced 5 of 5 on Windows by timing the shutdown to land while a server was starting, each time leaving a nats-server.exe listening on 4222. Now takes the lock after createRunner returns and closes the runner instead of adding it when the instance is gone; verified across ten shutdown offsets with no leftovers.
• INSTANCE_ARGUMENTS was declared and read but never assigned, so a second start() threw NullPointerException from INSTANCE_ARGUMENTS.equals(a) instead of the intended "Instance already started with different arguments."
• The jvm shutdown hook threw an NPE on exit, calling shutdownServers() (which nulls the instance) then shutdownExecutor() (which dereferences it). Order now matches the public shutdown().
• deleteDirContents was called on ClusterNode.jsStoreDir, which is @Nullable. Nothing reaches it with a null today, but the guarantee is spread across three places.
• --nojs --servers 1 died on a null jsStoreDirBase, and --nojs did not actually disable JetStream because createRunner passed jetstream(true). Confirmed off the process command line: --nojs now runs nats-server --config <conf> and the default runs it with -js.

pcgroups-cli

It tracked gradle/wrapper/gradle-wrapper.jar and .properties but no gradlew, so the wrapper could not be used, and it cannot build standalone anyway — implementation project(':') makes it its own root project, giving a circular :jar → :classes → :compileJava dependency. Both CI workflows build it from ./pcgroups as :pcgroups-cli:dist. The half wrapper is removed; :pcgroups-cli:dist still builds.

Verification

• Twelve projects compile against 4.0.2.
• 131 tests, 0 failures against nats-server v2.15.0-dev: counters 6, direct-batch 4, encoded-kv 19, js-publish-extensions 2, pcgroups 67, request-many 18, retrier 2, schedule-message 13.
• chaos-runner has no test suite, so it was run directly: cluster and single server, on both Linux and Windows, with JetStream state landing in the configured per-port directories and surviving DOWN/UP cycles. On Windows the config carried store_dir=C:\\temp\\...\\4222, escaped exactly once by the library.
• --nojs starts in both modes with no store dir written.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YRbGPiFEXpwnU9BEy8mXb7

[synadia-claude-reviewer]

Claude finished @scottf's task in 4m 31s —— View job

---

PR Review: jnats-server-runner 4.0.2 upgrade + chaos-runner fixes

• Gather context and read changed files
• Review ChaosRunner.java changes (thread safety, shutdown logic, correctness)
• Review build.gradle version bumps
• Review pcgroups-cli wrapper removal
• Post inline comments for specific issues
• Write summary

---

Summary

The version bumps across all eleven modules are clean — version string only, no API changes needed.

The pcgroups-cli wrapper removal is correct: a half-wrapper with no gradlew script was unusable, and the module can't build standalone anyway due to the circular :jar dependency, so there's nothing lost.

The chaos-runner fixes are generally well-reasoned and the PR description is unusually thorough about reproducing each bug. Two issues remain:

1. shutdownExecutor() has no null guard on INSTANCE (line 368–376)
shutdownServers() checks if (INSTANCE != null) before dereferencing; shutdownExecutor() does not. A race where the JVM shutdown hook and an explicit shutdown() call overlap can result in NPE at INSTANCE.executor.shutdown(). Inline comment with suggested fix posted.

2. downTask accesses natsServerRunners without INSTANCE_LOCK (line 128)
executor.shutdown() does not interrupt a task already running, so downTask can still be mid-execution when shutdownServers() iterates natsServerRunners under the lock. The concurrent remove() vs. for-each on an ArrayList can cause ConcurrentModificationException. This is the symmetric counterpart of the upTask bug that the PR already fixes — the same lock-then-null-check pattern applies.

Everything else looks correct: the INSTANCE_ARGUMENTS null assignment fix, the shutdown hook ordering fix, the JsConfig delegation for path escaping, the js flag threading through createRunner, and the jsStoreDir nullable guard are all sound.

[synadia-claude-reviewer]

downTask accesses natsServerRunners without INSTANCE_LOCK, creating the same class of race that was fixed for upTask.

executor.shutdown() (called in shutdownExecutor()) prevents new tasks from being scheduled but does not interrupt a task already executing. If downTask is mid-execution when shutdown fires:

• shutdownServers() acquires INSTANCE_LOCK and iterates INSTANCE.natsServerRunners
• downTask concurrently calls natsServerRunners.remove(downIx) with no lock held

ArrayList is not thread-safe; the concurrent structural modification can cause ConcurrentModificationException (or index corruption). This is the symmetric counterpart of the upTask bug described in the PR. The upTask fix (acquire lock → null-check INSTANCE → operate → release) is the right template to apply here too.

[scottf]

Fixed in 741cd1e.

Correct, and worse than I had assessed. When I looked at this earlier I concluded downTask "leaks nothing, a late run just throws IndexOutOfBounds into its own catch(Throwable)" and deferred it as tidiness. That was wrong: ArrayList is not thread safe, so a structural change racing the iteration in shutdownServers() is corruption or ConcurrentModificationException, not a bounded exception.

Applied the same template as upTask: acquire INSTANCE_LOCK, return if INSTANCE is null, operate, release in a finally. The lock is held across runner.close(), which matches what shutdownServers() already does.

Verified with the shutdown race harness across ten offsets on Windows, including 1900-2100ms, which brackets the first DOWN at initialDelay=2000 and so exercises this path specifically. No orphaned servers at any offset.

[synadia-claude-reviewer]

shutdownExecutor() can NPE if INSTANCE is already null.

shutdownServers() guards with if (INSTANCE != null) before dereferencing, but shutdownExecutor() (the public sibling) does not. A narrow concurrent path:

1. The JVM shutdown hook fires while an explicit shutdown() call is also in flight.
2. The hook wins INSTANCE_LOCK first, runs shutdownExecutor() + shutdownServers() (nulling INSTANCE).
3. The explicit shutdown() then re-enters shutdownExecutor() → NPE at INSTANCE.executor.shutdown().

The same issue is reachable if shutdownExecutor() is ever called as a standalone public API after shutdown() has completed. A guard matching shutdownServers() is all that's needed:

Suggested change

	shutdownExecutor();
	INSTANCE_LOCK.lock();
	try {
	if (INSTANCE != null) {
	INSTANCE.executor.shutdown();
	}
	}
	finally {
	INSTANCE_LOCK.unlock();
	}

[scottf]

Fixed in 741cd1e, using the suggested guard.

I noticed this while reading the file earlier and left it alone as out of scope for the version upgrade. That was the wrong call given the shutdown hook order change in this same PR touches the adjacent code.

Two reachable paths, not one: the concurrent one you describe, and the simpler case of shutdownExecutor() being called a second time after shutdown() has completed, since it is public API.

The guard now matches shutdownServers() exactly.