fix(serve): remove ephemeral.port on SIGTERM/SIGINT via graceful signal shutdown - #112
Open
tachyon-beep wants to merge 1 commit into
Open
fix(serve): remove ephemeral.port on SIGTERM/SIGINT via graceful signal shutdown#112tachyon-beep wants to merge 1 commit into
tachyon-beep wants to merge 1 commit into
Conversation
…ding it clarion-7ad374bac4 (serve half; the doctor /health half landed in 7eb79a5): serve publishes its actually-bound HTTP port to <store>/ephemeral.port (ADR-044) and relied solely on PublishedPortGuard's Drop to compare-and- delete it. That covers graceful shutdown, error return, and panic-unwind — but SIGTERM (a Codex-owned serve being reaped) and Ctrl-C/SIGINT terminate the process without running destructors, stranding the marker; consumers then classify HTTP as configured from a dead port. Fix: an async-signal-safe SIGINT/SIGTERM latch (signal-hook flag:: register_usize storing the signal number in a static AtomicUsize — the safe alternative to sigaction under the workspace's unsafe_code=deny), installed once at the real serve entry (run()), never from library/test paths. The supervision loop's existing 100ms tick polls the flag: on signal it shuts the HTTP read API down (joining the HTTP thread drops PublishedPortGuard, whose compare-and-delete stays the SINGLE deletion mechanism — the two-serve overwrite ownership rule is untouched), then exits promptly with the conventional 128+signo code (130/143) without waiting on the stdin-blocked stdio thread. Non-unix stays compiling as a no-op stub. The supervisor is split into a testable core (SupervisedOutcome + supervise_stdio_http_and_signals taking the flag) so the signal step is unit-asserted without real signals, plus real-binary integration coverage for BOTH SIGTERM and SIGINT (targeted pid kill, never the process group): - serve::tests::termination_exit_code_uses_the_128_plus_signo_convention - serve::tests::supervisor_signal_shuts_down_http_and_reports_signal_exit - serve::tests::supervisor_without_signal_returns_the_stdio_result - tests/serve.rs::sigterm_removes_published_ephemeral_port - tests/serve.rs::sigint_removes_published_ephemeral_port Gates: fmt, clippy -D warnings, build --bins, nextest (loomweave-cli + loomweave-federation, 886 passed — loomweave_port compare-and-delete tests unregressed), cargo deny (new dep signal-hook), doc -D warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR ensures loomweave serve cleans up its published <store>/ephemeral.port marker when the process receives SIGINT/SIGTERM, by latching the signal in an async-signal-safe way and having the existing supervision loop perform HTTP shutdown (dropping PublishedPortGuard) before exiting with the conventional 128 + signo status.
Changes:
- Add a Unix-only SIGINT/SIGTERM latch in
serve.rsusingsignal-hook, installed once from the realserveentrypoint. - Extend the stdio/HTTP supervisor loop to poll the signal flag and, on signal, shut down the HTTP read server and exit with
128 + signo(without waiting on the stdin-blocked stdio thread). - Add unit tests for exit-code mapping and supervisor behavior, plus integration tests that
kill(pid, SIGINT/SIGTERM)and assert both exit code and marker removal; addsignal-hookandnixdependencies.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/loomweave-cli/src/serve.rs | Introduces a signal latch and integrates it into the serve supervisor to ensure HTTP shutdown + marker cleanup on SIGINT/SIGTERM. |
| crates/loomweave-cli/tests/serve.rs | Adds integration tests that deliver SIGINT/SIGTERM to the serve process and assert exit code + ephemeral.port cleanup. |
| crates/loomweave-cli/Cargo.toml | Adds Unix-only signal-hook dependency and Unix-only nix dev-dependency for signal delivery in tests. |
| Cargo.toml | Adds workspace dependency entry for signal-hook. |
| Cargo.lock | Locks signal-hook and signal-hook-registry additions, and updates loomweave-cli deps accordingly. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+3790
to
+3802
| #[test] | ||
| fn sigterm_removes_published_ephemeral_port() { | ||
| signal_removes_published_ephemeral_port(nix::sys::signal::Signal::SIGTERM, 143); | ||
| } | ||
|
|
||
| /// Same contract for Ctrl-C on a foreground serve. | ||
| #[test] | ||
| fn sigint_removes_published_ephemeral_port() { | ||
| signal_removes_published_ephemeral_port(nix::sys::signal::Signal::SIGINT, 130); | ||
| } | ||
|
|
||
| fn signal_removes_published_ephemeral_port(signal: nix::sys::signal::Signal, expected_exit: i32) { | ||
| let dir = tempfile::tempdir().expect("temp project"); |
Comment on lines
+917
to
+921
| tracing::info!( | ||
| signal = signo, | ||
| "termination signal observed; HTTP read API stopped and ephemeral.port released" | ||
| ); | ||
| SupervisedOutcome::SignalExit(termination_exit_code(signo)) |
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.
Closes the remaining half of clarion-7ad374bac4 (the doctor
/healthprobe half landed in 7eb79a5).loomweave servecleaned up its published.weft/loomweave/ephemeral.portvia thePublishedPortGuardcompare-and-delete on every drop path — but SIGTERM (Codex-owned serves) and Ctrl-C (foreground serves) terminated the process without running destructors, stranding the marker so consumers classified HTTP as configured from a dead port.termination_signalmodule in serve.rs: SIGINT/SIGTERM handlers store the raw signo into a staticAtomicUsizeviasignal_hook::flag::register_usize(fully safe — the workspace deniesunsafe_code); installed viaOnceonly from the real serve entry, so library/test paths never mutate signal dispositions.HttpReadServer(joining the HTTP thread drops the guard — compare-and-delete stays the single deletion mechanism, so another live server's marker is never touched), then exits128+signo(130/143) without waiting on the stdin-blocked stdio thread. No-signal behavior unchanged.sigterm_removes_published_ephemeral_portandsigint_removes_published_ephemeral_port(targetedkillon the pid, exit code + marker asserted). Ownership-rule tests in loomweave-federation unregressed.Gates: fmt, clippy
-D warnings, workspace build, nextest (cli+federation 886 passed), cargo-deny (newsignal-hookdep), rustdoc-D warnings.clarion-7ad374bac4
🤖 Generated with Claude Code