fix(shutdown): handle SIGTERM at tracker boundary - #2133
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2133 +/- ##
===========================================
+ Coverage 84.04% 84.26% +0.21%
===========================================
Files 351 351
Lines 27988 28008 +20
Branches 27988 28008 +20
===========================================
+ Hits 23523 23601 +78
+ Misses 4070 4006 -64
- Partials 395 401 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
src/main.rs currently ignores the Result from tokio::signal::ctrl_c() inside tokio::select!, which can cause an error to be treated as a SIGINT and trigger an unintended shutdown.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds Unix SIGTERM handling at the tracker executable boundary so the existing coordinated shutdown sequence runs for both SIGINT and SIGTERM, with distinct observable logs, and updates the related issue spec/evidence docs accordingly.
Changes:
File summaries
| File | Description |
|---|---|
| src/main.rs | Adds SIGTERM handling (Unix) and shutdown-source logging before running the existing JobManager cancellation/wait sequence. |
| docs/issues/open/2132-add-sigterm-to-main/verification.md | Records post-implementation manual verification evidence and checklist updates. |
| docs/issues/open/2132-add-sigterm-to-main/ISSUE.md | Promotes draft to open issue #2132 and updates acceptance criteria/status narrative. |
| docs/issues/open/1488-overhaul-tracker-shutdown/ISSUE.md | Updates the EPIC table link to reference the new SI-1 issue spec path. |
Review details
Suppressed comments (3)
docs/issues/open/2132-add-sigterm-to-main/ISSUE.md:10
- Frontmatter
last-updated-utcis date-only, but the repo template requires UTC timestamp precision (YYYY-MM-DD HH:MM). Update this field to include the time in UTC to match the documented convention.
docs/issues/open/2132-add-sigterm-to-main/verification.md:161 - The verification environment captures a personal machine hostname (
josecelano-desktop) in the OS line. To avoid leaking identifiable workstation details in repo docs, redact the hostname while keeping kernel/version info.
docs/issues/open/2132-add-sigterm-to-main/verification.md:156 - Verification notes say the SIGTERM fix was "uncommitted" and the commit hash line includes a "working tree contains" qualifier, which reduces reproducibility for anyone trying to re-run the evidence later. Prefer re-running verification on a clean working tree and recording the exact commit SHA that contains the change.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
src/main.rs currently treats sigterm.recv() returning None as a SIGTERM, which can misreport the shutdown source and trigger shutdown unexpectedly if the signal stream closes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
src/main.rs:59
- Same as the outer
select!:sigterm.recv()yieldsOption<()>. If it ever returnsNone, the current arm would still select and report SIGTERM. HandleNoneexplicitly to avoid misreporting the shutdown source.
_ = sigterm.recv() => "SIGTERM",
docs/issues/open/2132-add-sigterm-to-main/ISSUE.md:10
last-updated-utcshould include a UTC time (YYYY-MM-DD HH:MM) per the issue spec template; this entry is date-only, which makes updates harder to track consistently.
- Files reviewed: 12/13 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new tests/lifecycle/native_tracker.rs introduces const fn methods that mutate or use non-const operations and are likely to break compilation under the project MSRV.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
docs/issues/open/AGENTS.md:114
- The summary table mixes the legacy standalone EPIC example (
1978-configuration-overhaul-epic.mdabove) with a folder-style EPIC example here, but the table labelEPIC specdoesn’t clarify which format it refers to. This can mislead readers into thinking EPIC specs are always folder-based even when documenting legacy single-file EPICs.
tests/lifecycle/native_tracker.rs:349
child_refis declared asconst fnbut calls runtime-only methods (as_ref()+expect(...)). This is very likely to be rejected by const-eval rules and can break compilation; use a normalfn.
const fn child_ref(&self) -> &Child {
tests/lifecycle/native_tracker.rs:353
child_mutis declared asconst fnbut usesOption::as_mut()+expect(...), which is unlikely to be const-evaluable and can break compilation. Use a normalfn.
const fn child_mut(&mut self) -> &mut Child {
tests/lifecycle/native_tracker.rs:357
output_refis declared asconst fnbut usesOption::as_ref()+expect(...), which is unlikely to be const-evaluable and can break compilation. Use a normalfn.
const fn output_ref(&self) -> &TrackerOutputCapture {
- Files reviewed: 29/30 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of concrete documentation/maintainability issues in changed files (notably a contradictory naming “Summary Table” and an unnecessary const fn) that should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
tests/lifecycle/native_tracker.rs:257
take_drop_cleanup_observeris declared as aconst fn, but it performs normal runtime state mutation (Option::take()) and returns a non-const type. Marking itconstdoesn't provide a benefit here and can unnecessarily constrain future changes to this helper.
/// Returns an observer for the signal that terminated the reaped drop-path child.
pub const fn take_drop_cleanup_observer(&mut self) -> oneshot::Receiver<Result<i32, String>> {
self.drop_cleanup_observer
.take()
.expect("drop cleanup observer must be taken at most once")
}
- Files reviewed: 29/30 changed files
- Comments generated: 1
- Review effort level: Lite
4543c53 to
b0810e3
Compare
|
ACK b0810e3 |
Summary
Add Unix
SIGTERMhandling at the tracker executable boundary. SIGINT and SIGTERM select the shutdown source, emit distinct observable logs, and execute the existing JobManager cancellation and waiting sequence once.Details
wait_for_shutdown_signal()so platform-specific signal registration and source selection are isolated from the executable lifecycle.ReadinessProbe.docs/issues/open/2132-add-sigterm-to-main/.Closes #2132
Validation
cargo build --releasecargo test --test lifecycle-signalscargo testlinter allKnown transitional behavior
Legacy periodic jobs retain their existing sequential per-job timeout behavior. The documented aggregate shutdown-result and deadline policy remain owned by SI-20. The native fixture's output assertions intentionally require message presence only; stdout and stderr are drained concurrently into one retained buffer and their cross-stream ordering is not asserted.