Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
scottf merged 2 commits into from
Sep 10, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bba9da
jnats-server-runner 4.0.2 across the repo, chaos-runner fixes
scottf Sep 10, 2026
741cd1e
chaos-runner: lock downTask, guard shutdownExecutor
scottf Sep 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion batch-publish/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ dependencies {
implementation 'io.nats:jnats:2.26.3-SNAPSHOT'
implementation 'org.jspecify:jspecify:1.0.0'

testImplementation 'io.nats:jnats-server-runner:3.1.1'
testImplementation 'io.nats:jnats-server-runner:4.0.2'
testImplementation 'com.github.stefanbirkner:system-lambda:1.2.1'
testImplementation 'nl.jqno.equalsverifier:equalsverifier:4.2.3'

Expand Down
2 changes: 1 addition & 1 deletion chaos-runner/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ repositories {
}

dependencies {
implementation 'io.nats:jnats-server-runner:3.1.1'
implementation 'io.nats:jnats-server-runner:4.0.2'

// this is only for the example and the uber jar won't include it
implementation 'io.nats:jnats:2.26.2'
Expand Down
80 changes: 54 additions & 26 deletions chaos-runner/src/main/java/io/synadia/chaos/ChaosRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import io.nats.ClusterDefaults;
import io.nats.ClusterInsert;
import io.nats.ClusterNode;
import io.nats.JsConfig;
import io.nats.NatsRunnerUtils;
import io.nats.NatsServerRunner;

Expand Down Expand Up @@ -93,11 +94,10 @@ private NatsServerRunner createRunner(int index) throws Exception {
ClusterInsert ci = clusterInserts.get(index);
NatsServerRunner.Builder b = NatsServerRunner.builder()
.debug(false)
.jetstream(true)
.jetstream(js)
.configInserts(ci.configInserts)
.port(ci.node.port)
.skipConnectValidate()
;
.skipConnectValidate();
return b.build();
}

Expand All @@ -110,11 +110,19 @@ private void scheduleUp() {
}

private void downTask() {
// natsServerRunners is an ArrayList and shutdownServers() iterates it under this
// lock. executor.shutdown() does not interrupt a task already running, so without
// the lock a structural change here can race that iteration.
INSTANCE_LOCK.lock();
try {
if (INSTANCE == null) {
// shut down before this task got to run
return;
}
if (specificPort != -1) {
for (int i = 0; i < natsServerRunners.size(); i++) {
NatsServerRunner nsr = natsServerRunners.get(i);
if (nsr.getPort() == specificPort) {
if (nsr.getNatsPort() == specificPort) {
downIx = i;
break;
}
Expand All @@ -125,30 +133,47 @@ else if (random) {
}

NatsServerRunner runner = natsServerRunners.remove(downIx);
printer.out(CR_LABEL, "DOWN", runner.getPort());
printer.out(CR_LABEL, "DOWN", runner.getNatsPort());

@synadia-claude-reviewer synadia-claude-reviewer Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 scottf Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

clusterInserts.add(clusterInserts.remove(downIx));
runner.close();
scheduleUp();
}
catch (Throwable e) {
printer.out(CR_LABEL, "DOWN/EX", e);
printer.out(CR_LABEL, "DOWN/EX", e);
}
finally {
INSTANCE_LOCK.unlock();
}
}

private void upTask() {
try {
NatsServerRunner runner = createRunner(servers - 1);
printer.out(CR_LABEL, "UP", runner.getPort());
natsServerRunners.add(runner);
scheduleDown(delay);
INSTANCE_LOCK.lock();
try {
if (INSTANCE == null) {
// Shut down while this server was starting. executor.shutdown() does not
// interrupt a task already running, so we got here after shutdownServers()
// had already closed out the list and the jvm hook was removed. Close it
// here, or it outlives the jvm still holding its port.
try { runner.close(); } catch (Exception ignore) {}
return;
}
printer.out(CR_LABEL, "UP", runner.getNatsPort());
natsServerRunners.add(runner);
scheduleDown(delay);
}
finally {
INSTANCE_LOCK.unlock();
}
}
catch (Throwable e) {
printer.out(CR_LABEL, "UP/EX: ", e);
scheduleUp();
}
}

private static void deleteDirContents(Path dir, boolean alsoDeleteSelf) throws IOException {
private static void deleteDirContents(Path dir, boolean alsoDeleteSelf) {
File fDir = dir.toFile();
if (fDir.exists()) {
File[] items = fDir.listFiles();
Expand Down Expand Up @@ -208,9 +233,10 @@ else if (!a.workDirectory.toFile().exists()) {
throw new IllegalArgumentException("Invalid specific port");
}
List<String> inserts = new ArrayList<>();
ClusterNode cn;
Path jsStorePath = Paths.get(jsStoreDirBase.toString(), "" + port);
cn = ClusterNode.builder()
// jsStoreDirBase is only set when js is on, and ClusterNode takes a null
// jsStoreDir, which is what the cluster branch ends up with in that case too
Path jsStorePath = js ? Paths.get(jsStoreDirBase.toString(), "" + port) : null;
ClusterNode cn = ClusterNode.builder()
.port(port)
.listen(listen)
.monitor(monitor < 1 ? null : monitor)
Expand All @@ -221,16 +247,9 @@ else if (!a.workDirectory.toFile().exists()) {
inserts.add("http: " + monitor);
}
if (js) {
String storeDir = jsStorePath.toString();
if (File.separatorChar == '\\') {
storeDir = storeDir.replace("\\", "\\\\").replace("/", "\\\\");
}
else {
storeDir = storeDir.replace("\\", "/");
}
inserts.add("jetstream {");
inserts.add(" store_dir=" + storeDir);
inserts.add("}");
// as of jnats-server-runner 4.0.2 JsConfig cleans and escapes the dir it is
// given, the same as the cluster branch gets by way of createClusterInserts
inserts.addAll(new JsConfig(jsStorePath).configInserts);
}
inserts.add("server_name=" + serverNamePrefix);

Expand Down Expand Up @@ -266,7 +285,10 @@ else if (!a.workDirectory.toFile().exists()) {
// delete jsStoreDirs for clean start
if (js) {
for (ClusterInsert ci : clusterInserts) {
deleteDirContents(ci.node.jsStoreDir, false);
// jsStoreDir is nullable on ClusterNode, so it might not have been given one
if (ci.node.jsStoreDir != null) {
deleteDirContents(ci.node.jsStoreDir, false);
}
}
}

Expand Down Expand Up @@ -307,12 +329,13 @@ public static ChaosRunner start(ChaosArguments a, ChaosPrinter printer) throws E
}

INSTANCE = new ChaosRunner(a, finalPrinter);
INSTANCE_ARGUMENTS = a;

APP_SHUTDOWN_HOOK_THREAD = new Thread("app-shutdown-hook") {
@Override
public void run() {
shutdownServers();
shutdownExecutor();

@synadia-claude-reviewer synadia-claude-reviewer Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 scottf Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

shutdownServers();
finalPrinter.out(CR_LABEL, "EXIT");
}
};
Expand Down Expand Up @@ -356,7 +379,11 @@ public static void shutdown() {
public static void shutdownExecutor() {
INSTANCE_LOCK.lock();
try {
INSTANCE.executor.shutdown();
// guard matches shutdownServers(). This is public and is also reachable a
// second time when the jvm hook and an explicit shutdown() overlap.
if (INSTANCE != null) {
INSTANCE.executor.shutdown();
}
}
finally {
INSTANCE_LOCK.unlock();
Expand Down Expand Up @@ -384,6 +411,7 @@ private static void shutdownServers() {
try { runner.close(); } catch (Exception ignore) {}
}
INSTANCE = null;
INSTANCE_ARGUMENTS = null;
}
}
finally {
Expand Down
2 changes: 1 addition & 1 deletion counters/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ dependencies {
implementation 'io.synadia:direct-batch:0.1.4'
implementation 'org.jspecify:jspecify:1.0.0'

testImplementation 'io.nats:jnats-server-runner:3.1.1'
testImplementation 'io.nats:jnats-server-runner:4.0.2'
testImplementation 'commons-codec:commons-codec:1.20.0'
testImplementation 'com.github.stefanbirkner:system-lambda:1.2.1'
testImplementation 'nl.jqno.equalsverifier:equalsverifier:4.2.3'
Expand Down
2 changes: 1 addition & 1 deletion direct-batch/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ dependencies {
implementation 'io.nats:jnats:2.26.2'
implementation 'org.jspecify:jspecify:1.0.0'

testImplementation 'io.nats:jnats-server-runner:3.1.1'
testImplementation 'io.nats:jnats-server-runner:4.0.2'
testImplementation 'org.junit.jupiter:junit-jupiter:5.14.1'
testImplementation 'org.junit.platform:junit-platform-launcher:1.14.3'
testImplementation 'com.github.stefanbirkner:system-lambda:1.2.1'
Expand Down
2 changes: 1 addition & 1 deletion encoded-kv/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ dependencies {
implementation 'io.nats:jnats:2.26.2'

testImplementation 'commons-codec:commons-codec:1.18.0'
testImplementation 'io.nats:jnats-server-runner:3.1.1'
testImplementation 'io.nats:jnats-server-runner:4.0.2'
testImplementation 'org.junit.jupiter:junit-jupiter:5.14.1'
testImplementation 'org.junit.platform:junit-platform-launcher:1.14.3'
testImplementation 'com.github.stefanbirkner:system-lambda:1.2.1'
Expand Down
2 changes: 1 addition & 1 deletion js-publish-extensions/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ dependencies {
implementation 'io.nats:jnats:2.26.2'
implementation 'io.synadia:retrier:0.2.1'

testImplementation 'io.nats:jnats-server-runner:3.1.1'
testImplementation 'io.nats:jnats-server-runner:4.0.2'
testImplementation 'org.junit.jupiter:junit-jupiter:5.14.1'
testImplementation 'org.junit.platform:junit-platform-launcher:1.14.3'
testImplementation 'com.github.stefanbirkner:system-lambda:1.2.1'
Expand Down
2 changes: 1 addition & 1 deletion pcgroups-cli/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ dependencies {
implementation project(':') // ':' means root project which is 'pcgroups'
implementation 'info.picocli:picocli:4.7.5'

testImplementation 'io.nats:jnats-server-runner:3.1.1'
testImplementation 'io.nats:jnats-server-runner:4.0.2'
testImplementation 'org.junit.jupiter:junit-jupiter:5.14.1'
testImplementation 'org.junit.platform:junit-platform-launcher:1.14.3'
}
Expand Down
Binary file removed pcgroups-cli/gradle/wrapper/gradle-wrapper.jar
View file
Open in desktop
Binary file not shown.
7 changes: 0 additions & 7 deletions pcgroups-cli/gradle/wrapper/gradle-wrapper.properties
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion pcgroups/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ dependencies {
api 'io.nats:jnats:2.26.2'
api 'org.jspecify:jspecify:1.0.0'

testImplementation 'io.nats:jnats-server-runner:3.1.1'
testImplementation 'io.nats:jnats-server-runner:4.0.2'
testImplementation 'org.junit.jupiter:junit-jupiter:5.14.1'
testImplementation 'org.junit.platform:junit-platform-launcher:1.14.3'
}
Expand Down
2 changes: 1 addition & 1 deletion request-many/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ repositories {
dependencies {
implementation 'io.nats:jnats:2.26.2'

testImplementation 'io.nats:jnats-server-runner:3.1.1'
testImplementation 'io.nats:jnats-server-runner:4.0.2'
testImplementation 'org.junit.jupiter:junit-jupiter:5.14.1'
testImplementation 'org.junit.platform:junit-platform-launcher:1.14.3'
testImplementation 'com.github.stefanbirkner:system-lambda:1.2.1'
Expand Down
2 changes: 1 addition & 1 deletion retrier/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ repositories {
dependencies {
implementation 'io.nats:jnats:2.26.2'

testImplementation 'io.nats:jnats-server-runner:3.1.1'
testImplementation 'io.nats:jnats-server-runner:4.0.2'
testImplementation 'org.junit.jupiter:junit-jupiter:5.14.1'
testImplementation 'org.junit.platform:junit-platform-launcher:1.14.3'
testImplementation 'com.github.stefanbirkner:system-lambda:1.2.1'
Expand Down
2 changes: 1 addition & 1 deletion schedule-message/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ dependencies {
implementation 'org.jspecify:jspecify:1.0.0'
implementation 'io.synadia:counters:0.2.2'

testImplementation 'io.nats:jnats-server-runner:3.1.1'
testImplementation 'io.nats:jnats-server-runner:4.0.2'
testImplementation 'org.junit.jupiter:junit-jupiter:5.14.1'
testImplementation 'org.junit.platform:junit-platform-launcher:1.14.3'
}
Expand Down
Loading