NATS Testing Framework as a Library

[ripienaar]

This pulls in the pure nats-micro components of the closed source ntf-server so that they can be used directly in a go routine.

The outcome is that for a go project the side car is not needed anymore, the test infra manager can run in-process.

Behaviours related to traces is injectible at build time and the ntf-server docker container is built with those injected

[synadia-claude-reviewer]

Claude finished @ripienaar's task in 5m 18s —— View job

---

PR Review: NATS Testing Framework as a Library

• Gather context and read PR diff
• Review ntf/service.go — goroutine lifecycle, context cancellation
• Review ntf/handlers.go — correctness, security, error handling
• Review ntf/api/api.go — API design, types
• Review ntf/tls.go — TLS/security correctness
• Review test files
• Post inline comments on specific issues
• Post overall summary

---

Summary

The overall design is solid: the unwind pattern in New, the snapshot-then-release pattern in the instance handlers, the per-node listener ownership in createCluster, the explicit cfgMu serialisation of update/reload/start, and the documented security model are all well-thought-out. Four concrete issues found:

1. Port-reservation lifetime bug in createSuperCluster (real bug)

createCluster was fixed to use per-node nodeLns slices so each server's route port listeners stay held until that specific server starts. That fix was not applied to createSuperCluster. A single heldListeners slice accumulates all gateway and route listeners upfront; when the first server calls runServerWithConfig(... heldListeners), every listener is closed — including the route and gateway ports reserved for servers 2..N. Those ports are immediately available for other processes to grab, making subsequent server starts potentially fail with "address already in use" on loaded machines. See inline comments on lines 1224–1225.

2. Data race on ms.srv in status handler

status snapshots *managedServer pointers under s.mu, releases the lock, then reads ms.srv.Name(), ms.srv.ClusterAddr(), ms.srv.Running() on those pointers. startServer and startInstance write ms.srv = newServer while holding s.mu. Concurrent execution is a data race under Go's memory model. Fix: snapshot the scalar fields you need before releasing the lock, or promote s.mu to sync.RWMutex and hold a read lock across the read path. See inline on line 1691.

3. Data race on ms.srv in stopServer

ms.srv.Running(), Shutdown(), and WaitForShutdown() are called on the pointer returned by findServerByName without holding s.mu or cfgMu. A concurrent startServer can write ms.srv under those locks. The fix is to acquire ms.cfgMu before the Running() check, consistent with how updateServer and reloadServer approach the same pointer. See inline on line 1310.

4. TOCTOU + data race in reloadServer

ms.srv.Running() at line 1630 is read before cfgMu is acquired. startServer/startInstance write ms.srv while holding cfgMu, so this is both an unsynchronised read and a TOCTOU: the server could stop between the check and the Reload() call. Moving cfgMu.Lock() above the Running() check closes both issues. See inline on line 1630.

[ripienaar]

@claude review

[synadia-claude-reviewer]

Claude finished @ripienaar's task in 3m 37s —— View job

---

PR Review: NATS Testing Framework as a Library

• Read key files (ntf/service.go, ntf/handlers.go, ntf/api/api.go, ntf/tls.go)
• Review goroutine lifecycle, concurrency, and locking
• Review API design and error handling
• Review TLS/security correctness
• Review test coverage
• Post findings

---

Overall Assessment

The design is solid: the unwind-on-failure pattern in New, per-node listener ownership in both createCluster and createSuperCluster, snapshot-then-release in status, and cfgMu serialisation of config ops are all thoughtfully implemented. The security model is honestly documented in doc.go. I checked the four bugs raised in the prior automated review — all four are fixed in the code as it stands. Two new concurrency issues remain.

---

Previously Reported Bugs — Status

Issue	Status
Port-reservation lifetime in createSuperCluster	✅ Fixed — per-node nodeLns with nil-on-transfer (lines 1176–1178)
Data race on ms.srv in status	✅ Fixed — snapshotInstance called under s.mu; server fields read before lock release
Data race on ms.srv in stopServer	✅ Fixed — cfgMu.Lock() at line 1344 is before the ms.srv.Running() check at line 1347
TOCTOU + data race in reloadServer	✅ Fixed — cfgMu.Lock() at line 1671 is before the ms.srv.Running() check at line 1674

---

New Bug 1: startServer reads ms.srv.Running() before acquiring any lock

File: ntf/handlers.go, lines 1377–1386

ms := s.findServerByName(creq.Name)  // returns after releasing s.mu
if ms == nil { ... }

if ms.srv.Running() {        // ← no lock held here
    req.Error("003", "Server already running", nil)
    return
}

// ...

ms.cfgMu.Lock()              // lock acquired too late
srv, err := startFromConfig(...)

ms.srv is written by concurrent startInstance under s.mu (line 1534). Reading it here without holding s.mu or cfgMu is a data race under Go's memory model.

Beyond the race, there is a TOCTOU: the server could be stopped between this check and the cfgMu.Lock() at line 1395, or a concurrent startInstance could start the server in that window, causing two callers to both proceed past the Running() guard.

Fix: acquire cfgMu before the Running() check, mirroring stopServer:

ms.cfgMu.Lock()
if ms.srv.Running() {
    ms.cfgMu.Unlock()
    req.Error("003", "Server already running", nil)
    return
}
// ...startFromConfig, ms.srv = srv, cfgMu.Unlock()

Fix this →

---

New Bug 2: startServer leaks a freshly started server on concurrent destroy/Reset

File: ntf/handlers.go, lines 1402–1407

startInstance correctly guards against a concurrent destroy with an alive-check before writing ms.srv:

// startInstance — correct
s.mu.Lock()
_, alive := s.instances[creq.InstanceID]
if alive {
    ms.srv = srv
}
s.mu.Unlock()
ms.cfgMu.Unlock()

if !alive {
    srv.Shutdown(); srv.WaitForShutdown()
    req.Error("404", "Instance destroyed during start", nil)
    return
}

startServer omits this check entirely:

// startServer — missing alive check
s.mu.Lock()
ms.srv = srv   // written unconditionally
s.mu.Unlock()
ms.cfgMu.Unlock()

Race window: findServerByName returns ms, then destroy/Reset removes the instance from s.instances and calls tearDownInstance (which shuts down the old ms.srv). Then startServer starts a fresh server and writes it into ms.srv — but the instance is gone, so tearDownInstance already ran and nothing will ever call Shutdown on the new server. The port it holds stays occupied for the life of the process.

Fix: add the same alive-check as startInstance after acquiring s.mu:

s.mu.Lock()
_, alive := s.instances[inst.ID]   // inst found via findServerByName
if alive {
    ms.srv = srv
}
s.mu.Unlock()
ms.cfgMu.Unlock()

if !alive {
    srv.Shutdown(); srv.WaitForShutdown()
    req.Error("404", "Instance destroyed during start", nil)
    return
}

Note: findServerByName currently doesn't return the instance ID, so you'd need to either walk s.instances to find which instance owns ms, or store the instance ID on managedServer. startInstance avoids this because it receives creq.InstanceID in the request.

Fix this →

---

Minor Issues

Error code "001" is overloaded. In the per-server handlers (stopServer, startServer, updateServer, reloadServer), error code "001" is used for both "invalid request" and "server not found". The instance-level handlers correctly use "404" for "not found". A client trying to distinguish a bad request from a missing server in the single-server handlers cannot do so from the code alone. Consider a dedicated "404" or "404" for not-found cases to be consistent.

No upper bound on cluster/server counts. CreateSuperClusterRequest validates Clusters >= 2 and Servers >= 2 but has no ceiling. A caller that passes Servers: 1000, Clusters: 1000 will attempt to start 1,000,000 servers and exhaust the machine. For a test harness this is unlikely to be a practical concern, but a sanity cap (e.g., 20 nodes per cluster, 10 clusters) would be a reasonable guard.

TLS cert validity is 24 hours. tlsCertValidity = 24 * time.Hour can cause failures in test environments where the same instance is created near midnight, or in long-running integration environments. Consider 7 days, which is still short enough to be clearly test-only.

---

Test Coverage Observations

• updateServer and reloadServer handlers have no dedicated test cases in handlers_test.go.
• validateListenerPortSet is untested.
• No test for the concurrent destroy-during-startInstance path (though the code handles it correctly).

---

Positive Notes

• The snapshotInstance/serverSnapshot split is a clean solution to the previous status-handler race.
• The cloneTemplateData copy-on-write in updateServer is a correct and minimal approach to avoiding partial-state mutation on failure.
• doc.go's security model section is unusually honest and clear about what exposing this service implies.
• The reservePort/closeListeners/runServerWithConfig handover protocol is well-documented.

[ripienaar]

@claude brief review, concurrent destroy-during-startInstance test was deliberately not done

[synadia-claude-reviewer]

Claude finished @ripienaar's task in 1m 24s —— View job

---

Brief Review

• Read trigger comment
• Read ntf/handlers.go — check status of previously reported bugs
• Post findings

---

All six bugs raised across both prior automated reviews are fixed in the current code. Here's the status:

Issue	Status
Port-reservation lifetime in createSuperCluster	✅ Fixed — per-node nodeLns with nil-on-transfer
Data race on ms.srv in status	✅ Fixed — snapshotInstance called under s.mu, reading all server fields before lock release
Data race on ms.srv in stopServer	✅ Fixed — cfgMu.Lock() at line 1370 before Running() check at line 1373
TOCTOU + data race in reloadServer	✅ Fixed — cfgMu.Lock() at line 1718 before Running() check at line 1721
startServer reads ms.srv.Running() before acquiring any lock	✅ Fixed — cfgMu.Lock() at line 1414 covers the Running() check at line 1417
startServer leaks server on concurrent destroy/Reset	✅ Fixed — alive-check via ms.instanceID at line 1438, with shutdown + 404 if dead

The startInstance alive-check (lines 1574–1587) is also consistent with the startServer pattern — both acquire s.mu, check s.instances, conditionally write ms.srv, release, then shut down and error if the instance was gone. The ms.instanceID field stored at create time (line 711) makes the key lookup clean without having to walk the map.

The deliberately-omitted concurrent-destroy test is noted; the three paths that handle it (startServer, startInstance, startInstance's per-node loop) are all covered by code inspection and the pattern is consistent across them.

The minor issues from the first review (error code "001" overloaded for both bad-request and not-found in single-server handlers; no ceiling on cluster/server counts; 24 h TLS cert validity) are still present but none are correctness or safety issues — they're fine to leave as-is or address in follow-up.

Code looks ready to merge.