fix(test): give each spawned worker its own metrics port - #415
Merged
Merged
Conversation
main went red on the hooks two-worker gate: both spawned workers died at
startup with
Error: binding metrics listener 127.0.0.1:9091
Caused by: Address already in use (os error 98)
so nothing claimed the seeded attempts and the wait timed out after 30s.
sms-worker binds its metrics listener unconditionally before any role
runs and main propagates the error, so a worker that cannot bind never
claims anything. The flag defaults to a fixed 127.0.0.1:9091 and all
three worker-spawning suites took that default, while cargo test runs
test binaries in parallel.
The part worth internalising is not the red build. Every assertion in
hooks_two_workers_live is satisfied by ONE worker draining everything:
all rows succeeded, attempts == 1, one HTTP request per row. The two
workers in that test always contended for the same port, so one always
died, and the test passed anyway. A gate whose whole purpose is "two
workers never double-deliver" has been running with one worker, and its
green history is not evidence of the property. It only went red now
because a concurrent binary took 9091 first and killed both.
Two changes, because the port alone would leave the silent-degradation
mode in place:
- all three suites pass --metrics-listen 127.0.0.1:0, so the kernel
assigns a free port and there is no shared global to contend over
- the two-worker gate asserts both children are still running before it
asserts anything else, so a dead worker fails loudly instead of
quietly halving the test
Verified against a real Postgres, both directions:
with the fix both workers start, both bind, 1 passed in 3.92s
(the property holds with two real workers, which had
never actually been exercised)
guard-failure port pinned back to 9091: fails in 3.38s naming the
dead worker, not a 30s timeout and not a false pass
cargo fmt --check and clippy -D warnings clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mainwent red on the hooks two-worker gate. Both spawned workers died at startup:Nothing claimed the seeded attempts, and the wait timed out after 30s.
The red build is the less interesting half
sms-workerbinds its metrics listener unconditionally, before any role runs, andmainpropagates the error — so a worker that cannot bind never claims anything, it exits.--metrics-listendefaults to a fixed127.0.0.1:9091, all three worker-spawning suites took that default, andcargo testruns test binaries in parallel.Every assertion in
hooks_two_workers_liveis satisfied by one worker draining everything: all rowssucceeded,attempts == 1on each, exactly one HTTP request per row. The two workers in that test have always contended for the same port, so one has always died — and the test passed anyway.So the gate whose entire purpose is "two workers never double-deliver" has been running with one worker. Its green history is not evidence of the property. It only went red now because a concurrently running binary took 9091 first and killed both.
Two changes, because the port alone is not enough
Fixing only the port would leave the silent-degradation mode intact — the next thing to kill a worker would quietly halve the test again.
hooks_two_workers_live,hooks_node_receiver_live,kill9_reclaim_live) pass--metrics-listen 127.0.0.1:0. The kernel assigns a free port; no probe-then-bind race, no shared global.leaseOwner— that is cleared the moment an attempt goes terminal, so observing both owners depends on catching them mid-flight.This is the same class of defect
sms-test-supportalready solves for the database (one per test binary). The metrics port was the global nobody had noticed.Verification — against a real Postgres, both directions
9091: fails in 3.38s naming the dead worker — not a 30s timeout, and not a false passThe guard-failure output, verbatim:
Worth stating plainly: the exactly-once property does hold with two real workers. That had never actually been exercised, so this could have surfaced a genuine bug. It did not.
cargo fmt --checkandcargo clippy -p sms-worker-bin --all-targets -- -D warningsclean.Note on how this was run
The harness's own container could not start on this machine — it binds a fixed host port
127.0.0.1:55432, which a localvaam-postgres-1already held. I usedVSMS_TEST_DATABASE_URLagainst a scratch Postgres on another port rather than stopping anyone's stack. That fixed harness port is a third instance of the same shared-global-port pattern; not fixed here.🤖 Generated with Claude Code