[adapters] NATS: isolate and verify test server startup - #6741
Conversation
Every NATS test spawns its own nats-server, and three defects in the spawn helpers let one test's server break an unrelated test: - All servers shared the default JetStream store directory. Servers starting at the same moment race on `mkdir $G/streams` and the loser dies with `Can't start JetStream`. The window is open only while that directory does not yet exist, so the race hit fresh CI containers and almost never a developer machine. Reproduced: 1 of 20 concurrent servers died on a fresh store directory. - All servers shared one ports file directory, and the file is named `nats-server_<pid>.ports`. ProcessKillGuard uses SIGKILL, so servers never delete their file; a later server assigned the same pid found the stale file and reported the dead server's port. Waiting only for the file to exist also meant a half-written file parsed as an error. Reproduced: with a stale file present, 40 of 40 spawns read the dead port. - start_nats_on_port returned Ok without checking that the server came up. `Command::spawn` succeeds even when nats-server exits immediately with `address already in use`, so a test whose port was taken while its server was down ran on against whichever process held the port, then timed out several actions later somewhere unrelated. Each server now gets a private scratch directory for its JetStream store, its ports file, and its log, and startup waits for the server to report the port it bound. The ports file proves the server owns that port; a TCP probe would also succeed against a foreign server. Startup failures now report the server's own log instead of discarding it. Fixes the `test_nats_replay_server_killed_retries_non_fatal` failure in https://github.com/feldera/feldera/actions/runs/30363748070, where a restarted server was gone by the time the test published to it. At RUST_TEST_THREADS=20 the suite failed 6 of 12 runs before, across seven different tests; after, 57 of 57 runs pass. Both regression tests fail against the previous implementation.
blp
left a comment
There was a problem hiding this comment.
This looks OK to me.
I waited to review it because it's a draft.
mythical-fred
left a comment
There was a problem hiding this comment.
High-level feedback only since this is still marked draft.
Root-causing is sharp: three real, independent bugs, each with a plausible mechanism (mkdir race on the shared JetStream store, stale nats-server_<pid>.ports from SIGKILLed servers colliding on pid reuse, Command::spawn masking immediate exit). The reproduction numbers in the PR body (1/20 for the mkdir race, 40/40 for stale ports, 6/12 → 57/57 on the whole suite) are exactly the evidence I want to see for "flaky test" fixes — no hand-waving.
Design of the fix looks right:
- Per-server
TempDirscratch for JetStream store, ports dir, and log — the guard owns the tempdir afterprocessin the struct soDroporder kills the child before wiping the tree. Good. wait_until_readyparses the ports file before checkingtry_wait, so a server that came up and then died still counts as started, and partial writes fail parse silently — that's the correct polling shape.- Startup failures surface the server's own log instead of
/dev/null. This alone is worth the PR. start_nats_on_portverifying the bound address actually ends with the requested port catches the "address already in use → some other process holds it" case that used to time out downstream.
Tests are unusually good for infrastructure code: start_nats_on_port_fails_when_port_is_taken pins down the specific defect and even asserts the log substring; servers_do_not_share_directories correctly notes it's structural (racing on a race is a bad test); concurrent_servers_start_independently at N=16 exercises the exact scenario. parse_ports_file_rejects_incomplete_content locks the partial-write behavior.
One thing worth thinking about before un-drafting: MAX_ATTEMPTS = 2 with a fixed 250ms backoff is inherited from the previous implementation, but the failure modes it was papering over (mkdir race, stale ports) are the ones this PR eliminates. If startup is now expected to succeed on the first try in the normal case, is the retry still earning its keep? Either drop it (and let a real spawn failure fail immediately with the server's log), or add a comment naming the residual transient it exists for. Leaving it as an unexplained retry loop invites the next flake investigator down the wrong path.
No architectural blockers. Ready to look properly once you mark it non-draft.
|
Merging, so the PR doesn't bit rot. |
Every NATS test spawns its own nats-server, and three defects in the spawn helpers let one test's server break an unrelated test:
All servers shared the default JetStream store directory. Servers starting at the same moment race on
mkdir $G/streamsand the loser dies withCan't start JetStream. The window is open only while that directory does not yet exist, so the race hit fresh CI containers and almost never a developer machine. Reproduced: 1 of 20 concurrent servers died on a fresh store directory.All servers shared one ports file directory, and the file is named
nats-server_<pid>.ports. ProcessKillGuard uses SIGKILL, so servers never delete their file; a later server assigned the same pid found the stale file and reported the dead server's port. Waiting only for the file to exist also meant a half-written file parsed as an error. Reproduced: with a stale file present, 40 of 40 spawns read the dead port.start_nats_on_port returned Ok without checking that the server came up.
Command::spawnsucceeds even when nats-server exits immediately withaddress already in use, so a test whose port was taken while its server was down ran on against whichever process held the port, then timed out several actions later somewhere unrelated.Each server now gets a private scratch directory for its JetStream store, its ports file, and its log, and startup waits for the server to report the port it bound. The ports file proves the server owns that port; a TCP probe would also succeed against a foreign server. Startup failures now report the server's own log instead of discarding it.
Fixes the
test_nats_replay_server_killed_retries_non_fatalfailure in https://github.com/feldera/feldera/actions/runs/30363748070, where a restarted server was gone by the time the test published to it.At RUST_TEST_THREADS=20 the suite failed 6 of 12 runs before, across seven different tests; after, 57 of 57 runs pass. Both regression tests fail against the previous implementation.
Describe Manual Test Plan
Checklist
Breaking Changes?
Mark if you think the answer is yes for any of these components:
Describe Incompatible Changes