fix(lifecycle): install signal handlers before startup work - #4
Merged
Conversation
A SIGTERM arriving early in startup killed the process outright, bypassing graceful shutdown and dropping any in-flight request. Reproduced directly: SIGTERM at 50ms exited 143 with no shutdown log line; at 1s it exited 0. Root cause: `tokio::signal::ctrl_c()` is an `async fn` that registers its handler on first *poll*, and the shutdown future was not polled until `axum::serve(..).with_graceful_shutdown(..)` ran — after CLI parsing, tracing init, adapter logging and `TcpListener::bind`. Until then SIGTERM kept its default disposition. By contrast `tokio::signal::unix::signal(kind)` is a plain function that registers synchronously inside its body. So both binaries now install SIGTERM and SIGINT via `signal()` as the first statement in `main()`, through a shared `vpay_config::ShutdownSignals` (the crate already hosts the shared `CommonArgs`). Installation failure is now a hard startup error rather than a logged warning: it means the process has no graceful shutdown for its entire lifetime, not for a brief window. `--shutdown-grace-seconds` and the bounded-drain logic are untouched. Non-unix keeps the previous `ctrl_c()` fallback. Verified: SIGTERM at 50ms now exits 0 with the graceful-shutdown line, 6/6 for both binaries. Caveat, recorded in the test's own doc comment rather than glossed: the regression test found a genuine signal at 2ms (~90-98% pass fixed vs ~68% unfixed over 250+ trials per side), but that delay is not survivable under full-suite contention, which widens the window for fixed code too. It settled at 50ms, which is stable but mainly guards against total removal of signal handling rather than narrow reintroduction of this exact race. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`just ci` failed roughly one run in several with testcontainers' "container port" error. Not a logic failure: each integration test starts its own postgres:16-alpine container, so nextest's default parallelism raced 13+ container starts at once. The same test passed 3/3 in isolation. Pre-existing since the migrations landed. Bounds the `vpay-tests-integration` package to one concurrent test via a nextest test group, leaving each test's own container and its existing Drop-based cleanup exactly as they were. A shared container behind a `static` was tried first and rejected on evidence: Rust does not drop statics at process exit, so `ContainerAsync`'s cleanup never runs and every invocation leaks the container. That was not theoretical — it orphaned hundreds of containers and drove the host's Docker VM into memory pressure. `max-threads = 3` was also measured and still failed ~1 run in 14; the binding constraint is the Docker VM's 4 vCPUs, not the host's 12 cores. Cost is wall-clock only: ~7s to ~15s for the full workspace run. Verified 5 consecutive clean runs (80 passed / 3 skipped) with no leaked containers, and `just ci` exiting 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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
Two fixes, both found by running the software rather than reading it.
vpay-serveroutright, bypassing graceful shutdown and dropping any in-flight request.just cifailed roughly one run in several on a testcontainers concurrency flake — pre-existing onmaster, unrelated to (1), but a gate that fails intermittently is not a gate.Source of truth: found while running the stack locally at the maintainer's request after #3 merged. Bears on docs/flows/crash-safety.md, whose whole subject is not losing work at process boundaries.
Intent
The signal race
Root cause.
tokio::signal::ctrl_c()is anasync fn— it registers its handler on first poll. The shutdown future was not polled untilaxum::serve(..).with_graceful_shutdown(..)ran, i.e. after CLI parsing, tracing init, adapter-registry logging andTcpListener::bind. Until that moment SIGTERM retained its default disposition: immediate termination.tokio::signal::unix::signal(kind), by contrast, is a plain function that registers synchronously inside its body.Under CPU contention the window widens, which is why
vpay-worker-binshowed it too when both binaries were started together.The flake
Each integration test starts its own
postgres:16-alpinetestcontainer, so nextest's default parallelism raced 13+ container starts simultaneously, intermittently failing withError: container port. The same test passed 3/3 in isolation.Scope
Signal handling. Both binaries now install SIGTERM and SIGINT via
signal()as the first statement inmain(), before tracing init, through a sharedvpay_config::ShutdownSignals(that crate already hosts the sharedCommonArgs, so there was precedent). Installation failure is now a hard startup error rather than a logged warning — it means the process has no graceful shutdown for its entire lifetime, not for a brief window, which is a misconfiguration rather than a degradation.--shutdown-grace-secondsand the bounded-drain logic are untouched; non-unix keeps the previousctrl_c()fallback.Test concurrency. A nextest test group bounds
vpay-tests-integrationto one concurrent test. Each test keeps its own container and its existingDrop-based cleanup — only the rate of container starts changes.Verification
vpay-servervpay-worker-bincargo nextest run --workspace×5just ciScreenshots / Evidence
Risk Assessment
Low, with two disclosures I want visible rather than buried.
The regression test is weaker than its name suggests. A genuine signal exists at
DELAY = 2ms(~90-98% pass fixed vs ~68% unfixed, 250+ trials per side, macOS and a Linux container). That delay does not survive full-suite contention, which widens the window for fixed code too — no delay was found that is simultaneously safe under load and sensitive to this exact bug. It settled at 50ms, which is stable across repeated full runs but primarily guards against total removal of signal handling rather than narrow reintroduction of this race. This is stated in the test's own doc comment.A shared-container approach was tried first and rejected on evidence, destructively. Rust does not drop
statics at process exit, so aContainerAsyncheld in aOnceCellnever runsDropand leaks its container on every invocation. This was not theoretical — it orphaned hundreds of containers, drove the host's Docker VM into memory pressure, and OOM-killed an unrelated container on the development machine. All orphans were cleaned up and the affected container restarted. The finding is real and worth keeping: do not hold a testcontainer in astatic.max-threads = 3was also measured and still failed ~1 run in 14; the binding constraint is the Docker VM's 4 vCPUs, not the host's 12 cores.max-threads = 1may be more conservative than a well-resourced Linux CI runner needs — it is a fixed bound, not an auto-detect, so it leaves some speed on the table there. Cost is wall-clock only: ~7s → ~15s for the full workspace run.AI Usage Declaration
AI (Claude Opus 5 via Claude Code) performed this work, delegating to Sonnet sub-agents. The orchestrating session re-ran every claim rather than accepting agent reports — which mattered here: my own first reproduction script reported 0/6 and was wrong (it reused one log file across iterations and mangled the exit-code check); re-measured properly it is 6/6. The machine state after the container leak was independently verified clean before committing.
Reviewer Focus
ShutdownSignals::install()— a process that cannot install a handler now refuses to start. Agree?max-threads = 1— right bound for your CI, or should it be relaxed there?