SIGTERM and SIGHUP run the SIGINT drain - #348
Conversation
`kill <dl>` mid-`devpod up` left the plaintext GH_TOKEN file on disk and orphaned the `up` child — the exact pair the SIGINT handler exists to prevent — because only SIGINT was handled. The disposition is now a set of signals sharing one handler, and the exit code is derived from the signal (128 + signo), which is what 130 already was.
Closing the terminal window is the same leak as a kill, and the least watched of the three: the window any complaint would have shown up in is the one that went away. 129 is 128 + SIGHUP, by the same rule.
Draining on SIGHUP took `nohup` away: its whole purpose is outliving the terminal, and it says so by handing the child a SIG_IGN it expects to survive the exec. The classic POSIX idiom restores it, applied to all three signals rather than to SIGHUP alone, because a per-signal exception is the thing that drifts. README said SIGHUP was not handled at all; it now says what each signal does clean up, what it still does not (the --autorm removal, which a handler may not run), and what code each exits with.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideExtend the shared signal handling for dl/aid so SIGINT, SIGTERM, and SIGHUP all run the same async-signal-safe drain, derive exit codes as 128+signo, and respect inherited ignored dispositions, with tests and docs updated accordingly. Sequence diagram for signal-driven dl cleanupsequenceDiagram
participant Signal as Signal source
participant DL as dl or aid
participant Handler as drain(signal)
participant Runner as cleanup_and_exit
participant Child as devpod up process group
participant Files as Staged token files
Signal->>DL: SIGINT, SIGTERM, or SIGHUP
DL->>Handler: invoke handler with signal
Handler->>Runner: cleanup_and_exit(signalled(signal))
Runner->>Child: kill foreground process group
Runner->>Files: unlink registered temporary files
Runner-->>DL: _exit(128 + signal number)
Flow diagram for inherited signal dispositionsflowchart TD
A[install_signal_handlers] --> B{For SIGINT, SIGTERM, and SIGHUP}
B --> C[Read inherited disposition]
C --> D{Already SIG_IGN?}
D -->|Yes| E[Leave signal ignored]
D -->|No| F[Install drain handler]
E --> G[Process remains protected from that signal]
F --> H[Signal runs cleanup drain]
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Two independent axes, fresh context, fixed point a1c6fe8...0d0e59a. Verified locally: cargo test --workspace, cargo clippy --locked --all-targets -- -D warnings and cargo fmt --check are all clean (two failures appeared only under parallel contention with other agents on this host — dl/tests/read_side.rs and aid/tests/interactive.rs, both untouched by this diff, both green run alone).
The #343 claim is verified, not accepted. #343 touches rust/devlaunch-runner/src/lib.rs and rust/devlaunch-runner/src/tests.rs and nothing else; this diff touches neither, and note_foreground_child, clear_foreground_child, own_group and the killpg in drain() are byte-identical to main. No process-group membership moves here. Independently confirmed that note_foreground_child is called only on the own_group arm (devlaunch-runner/src/lib.rs:470-476), so the new SIGTERM/SIGHUP paths cannot killpg dl's own foreground group during an interactive devpod ssh.
Signal-safety probes (run against a binary linking the real install_signal_handlers + cleanup_and_exit). All pass:
- The drain is genuinely async-signal-safe:
killpg/unlink/rmdir/_exitonly, overAtomicPtr/AtomicI32slots, no allocation and no lock — so it cannot deadlock against a lock the main thread holds (devlaunch-runner/src/interrupt.rs:160-190). - Exit codes are observed as exit codes, not signals:
waitpidreportsWIFEXITEDwith 130 / 143 / 129 for INT / TERM / HUP, so a shell's$?is those values. The_exitreally does beat the default disposition, as documented. - Re-entrancy is safe. Read back from the installed disposition:
sa_maskblocks only the delivered signal andSA_NODEFERis clear, so a secondDRAINEDsignal can nest inside the handler.drain()is idempotent under it (killpgagain,unlink→ENOENT, atomics only); the only observable effect is that the inner_exitwins, so a racing pair may report the second signal's code. 4/4 triple-signal runs exited 143 with the token gone and the child dead — no hang, no double-free, no half-clean state. - Inherited
SIG_IGNreally does survive for all three signals, and anohup-style run stays reachable by whatever was not disarmed.
Standards
Judged against CLAUDE.md, docs/rust-rewrite-plan.md, the CHANGELOG's Keep-a-Changelog header plus publish.yml's release-notes extraction, and ci.yml's public-api job. The doc prose in rust/dl/src/lib.rs:80-147 is up to this repo's culture — it names SIGQUIT as the alternative that lost and argues the all-three rule over a SIGHUP-only exception. Findings are all in the surrounding docs and surface.
- non-blocking — the live divergence table was not updated.
docs/rust-rewrite-plan.md:12-16says the divergence table "is the exception and is still live… the only written record of every deliberate behavioural difference from the Python build… Keep it." This PR creates exactly such a difference (Python died by SIGTERM/SIGHUP with no cleanup;dlnow drains and exits 143/129), and row 27 atdocs/rust-rewrite-plan.md:190still reads "On SIGINT dl prints no timing summary… the handler is_exit(130)". Row 30 shows post-port rows do get added. - non-blocking — "stays ignored" hides a real change to shipped SIGINT behaviour.
CHANGELOG.md:20-22andREADME.md:248-250present the inherited-SIG_IGNrule as preserving behaviour, illustrated only withnohup/SIGHUP. For SIGINT it is a change, not a preservation (see Spec 1).publish.yml:72-83turns this section verbatim into the GitHub release notes, so this is the text users get. (Theinstall_interrupt_handler→install_signal_handlersrename correctly needs noChangedentry —dl's lib is unpublished and the CHANGELOG is user-facing.) - non-blocking — two premises in
devlaunch-runnerwere widened without being revisited.devlaunch-runner/src/interrupt.rs:3still asserts "dl's SIGINT disposition is_exit(130)", and the stale-pgid SAFETY argument at:171-174closes its window partly "by SIGINT delivery being to this process" — which no longer characterises delivery, since a group-wide or cgroup-wide SIGTERM reaches the child too. The conclusion still holds; the stated reason no longer does. - non-blocking — dead public surface.
INTERRUPTED(rust/dl/src/lib.rs:100) now has zero call sites workspace-wide — the only remaining mention is the doc comment at:87.signalled(:93) ispubwith no caller outside the crate; the tests assert literal 130/143/129. Givenci.yml:380-388's stated dislike of an accidentalpub, both wantpub(crate)or deletion.
Refuted rather than reported: [libc::c_int; 3] (the length is compile-checked); the unguarded dl lib surface (ci.yml:379-395 scopes the snapshot to devlaunch-core deliberately); Ending::code's -signal at rust/dl/src/commands.rs:75 (documented Python parity for a child's status); Aftermath/MidUp (the World + harness-struct idiom every rust/dl/tests/*.rs already uses).
Axis: pass. No blocking findings; four documentation/surface items worth fixing before merge.
Spec
Anchors: #304 — "Install a SIGTERM handler running the same drain"; "Settle inside this ticket: whether SIGHUP joins (README:233 — closing the terminal today skips both --autorm and the cleanup), and the exit-code convention"; "Test at the binary boundary in the style of dl/tests/interrupt.rs".
- blocking — the SIGINT half of the inherited-ignore rule is an undeclared change to the pre-existing path.
rust/dl/src/lib.rs:161-170applies theSIG_IGNprobe to all ofDRAINED. Measured on this host: a background job of a non-interactive shell (cmd &in a script or CI step) starts withSigIgncontaining SIGINT|SIGQUIT — POSIX job control, nobody typedtrap. Reproduced directly against the real disposition: a run launched that way survivedSIGINTwith the staged token still on disk, where ona1c6fe8it drained and exited 130. In that case the ticket's own leak gets longer-lived rather than shorter: after a Ctrl-C kills the script,dlis an orphan still holding the launch lock, still runningdevpod up, with the plaintextGH_TOKENstill staged. Declared nowhere. It also sits against the builder's own stated bar for rejecting the re-raise alternative — "reversing that would be a silent behaviour change for everyone scriptingdl" — which applies with equal force here. The uniformity argument does not carry across the two signals: inheritedSIG_IGNon SIGHUP means "I rannohup"; on SIGINT it usually means "my shell backgrounded me", which is no statement about interruption at all. Either narrow the rule to SIGHUP, or keep it uniform and say so in CHANGELOG/README as a behaviour change. - blocking — the changed half is the untested half.
rust/dl/tests/interrupt.rs:299-311:reached_with_signal_ignoredis parametrised by signal and called only with"HUP". The spec's "Test at the binary boundary in the style ofdl/tests/interrupt.rs" is otherwise well met — all three signals covered at:278,:286,:295, correct boundary, correct style, andAftermathmakes a half-clean exit unpassable. But the one rule that alters existing behaviour has no test; a one-line("INT")case would have surfaced finding 1. - non-blocking — README overclaims.
README.md:243-245: "no signal leaves a credential on disk or a build running behind you" is false as written —rust/dl/src/lib.rs:118-120says "SIGQUIT is deliberately absent", and a probe confirms Ctrl-\ leaves the staged token on disk. Scope the sentence to "none of these three". - non-blocking — wrong mechanism named.
README.md:249andrust/dl/src/lib.rs:135credit the inherited disposition for "a job disowned by a script";disownsets noSIG_IGN(measured), it works by the shell not sending SIGHUP, andsetsidlikewise sets none — it is covered by session detachment. So the sub-decision's "the ambiguity is entirely carried by the inherited disposition" is false for two of the three cases it names. The outcome is still right for both; the reasoning and the doc are what need correcting. - non-blocking — the exit-code justification overreaches.
devlaunch-core/src/flows/launch.rs:1396-1398reads "A signal is Python's negativereturncode, kept here so the binary renders the same exit code rather than inventing 128+n" — the codebase had explicitly declined 128+n. It is a different question (a child's status vsdl's own death) so the decision stands, but "generalises what the codebase does rather than adding a convention" is not accurate, and the repo now holds two opposite conventions with no cross-reference between them. - non-blocking — stale neighbours:
rust/devlaunch-runner/src/interrupt.rs:3andrust/dl/tests/interrupt.rs:1still describe a SIGINT-only world.
Checked and clean: the README:233 edit satisfies both halves of the cited line (the cleanup now runs; --autorm is still correctly listed under "These do not fire", since a handler may not run it); scope is otherwise tight (devlaunch-runner untouched, rename fully propagated); the delegation the ticket made was honoured — both sub-decisions are breadcrumbed with what nearly won, as #304, not the map, directed.
Axis: fail. Findings 1 and 2 need resolving before merge.
Verdict
Request changes (posted as a comment: GitHub refuses --request-changes on a PR authored by the same account).
Blocking:
- Spec 1 — the inherited-
SIG_IGNrule applied to SIGINT silently changes the pre-existing interrupt path: adlbackgrounded from a non-interactive shell no longer drains on SIGINT, and in that case the staged credential and thedevpod upchild outlive the run that was cancelled. Narrow the rule to SIGHUP, or keep it uniform and declare it in CHANGELOG and README. - Spec 2 — add the
("INT")case toa_signal_already_ignored_when_dl_started_stays_ignored, so whichever way 1 is settled is pinned at the binary boundary.
Non-blocking but worth landing in the same pass: docs/rust-rewrite-plan.md row 27, the two stale premises in devlaunch-runner/src/interrupt.rs, the README.md:243-245 "no signal" overclaim, the disown/setsid mechanism wording, and the now-unused INTERRUPTED / over-pub signalled.
The core of the change is sound: SIGTERM and SIGHUP genuinely close the leak the ticket names, the handler is async-signal-safe and re-entrant-safe, the exit codes are real exit codes, and the process-group claim about #343 holds.
Review found the SIGINT half was a regression, not a preservation. A non-interactive shell backgrounding a job hands its child an ignored SIGINT and SIGQUIT under POSIX job control — measured, SigIgn 0x6 — so every `dl … &` in a script or a CI step stopped draining on Ctrl-C. The staged plaintext GH_TOKEN and the `devpod up` child then outlive a cancelled run, which is the leak this ticket exists to close, reopened through the door that was already shut. The rule now says which signals it speaks for, as a table pairing each signal with what an inherited ignore means coming from it: a statement for SIGTERM and SIGHUP (nohup disarms SIGHUP precisely so the run outlives the terminal), an accident of job control for SIGINT. Ctrl-C is byte-for-byte the behaviour it had before this branch. The changed half was also the untested half. The test is now parametrised over all three signals from one table, so a signal cannot be left uncovered by omission — which is how this got through. Proved red against the previous commit's lib.rs (SIGINT ignored, run continued to its natural end, exit 1 not 130) and green against this one. One row expects the build to outlive the finishing Ctrl-C, and that is inherent rather than a defect: `trap '' TERM` reaches everything dl spawns, and the drain fells the build with killpg(SIGTERM), so disarming SIGTERM disarms the drain's own reach into the child. Recorded where the row is, since it reads as an inconsistency otherwise. Also drops now-dead public surface: INTERRUPTED had no call sites left, and `signalled` had no caller outside the crate.
Four review items, all documentation or premise rather than behaviour. The divergence table is declared live and cited by row number, and this change created exactly the kind of difference it records. Row 27 still described a SIGINT-only `_exit(130)`; it now covers all three signals, the 128+signo codes, and the inherited-ignore asymmetry — including the cross-reference the exit-code decision was missing, since Session:: exit_status keeps Python's negative returncode for a *child* killed by a signal and explicitly declines 128+n. Two precedents, each followed in its own place, neither generalising to the other. The CHANGELOG and README framed the inherited-ignore rule as preserving behaviour, illustrated only with nohup. After the scoping that is true of SIGTERM and SIGHUP and needs saying out loud for SIGINT, which is where a reader would otherwise assume symmetry. This text becomes the release notes verbatim, so it says which two signals can be switched off, which one cannot, and why that is deliberate. README also claimed no signal leaves a credential behind. SIGQUIT does, by the same design decision that keeps it out of the drain, so the claim is now scoped to the three and names the exception. Two premises in devlaunch-runner were widened without being revisited. The module still opened on a SIGINT-only disposition. The stale-pgid SAFETY argument rested on delivery being to this process, which no longer characterises delivery — a group-wide SIGTERM reaches the child too — and on clear_foreground_child running before the reap, which it does not: it runs just after the wait returns. Restated on the reap window itself, which is what actually bounds it and holds whatever woke the handler. Finally, disown and setsid were credited to the inherited disposition. Neither sets SIG_IGN (measured); they work by the shell not sending SIGHUP and by leaving the session. Corrected in both places.
|
Addressed in BlockingSpec 1 — the SIGINT half of the inherited-ignore rule. Reproduced independently before touching anything: Scoped as decided on #304: the inherited-ignore rule now applies to SIGTERM and SIGHUP only, and Ctrl-C behaves byte-for-byte as it did before this branch. Rather than a uniform loop with an Spec 2 — the changed half was the untested half. Fixed at the root rather than by adding one case: Proved both ways:
One thing the parametrised test surfaced that I had not predicted, and it is not a defect. With SIGTERM disarmed, the finishing Ctrl-C drains and unlinks the token but the build survives. Non-blocking
Gates green locally: Thanks for the probe binary — the |
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Follow-up: blocking findings resolved
Re-reviewed at 4b6539c (fast-forward from 0d0e59a, nothing rebased). Scoped to my own two blocking findings, the non-blocking list, and what the two new commits introduce. cargo test -p dl --test interrupt, -p aid --test interrupt, clippy --locked --all-targets -- -D warnings and fmt --check all clean here.
Spec 1 — resolved, and verified byte-for-byte rather than by inspection. DRAINED now pairs each signal with an InheritedIgnore (rust/dl/src/lib.rs:153-157), and SIGINT's Loses arm is a bare libc::signal(SIGINT, drain) with no SIG_IGN probe (:206-209) — the same single call pre-branch main made, and signalled(SIGINT) is 130, which is what INTERRUPTED was. Measured against the real disposition on this head: a run started with SIGINT already SIG_IGN drains and exits 130 with the token gone, where at 0d0e59a it survived with the token on disk. Ctrl-C is back to exactly what it was. Declared in three places now — CHANGELOG.md:26-31 ("unchanged from previous releases"), README.md:252-260, and divergence-table row 27.
Spec 2 — resolved. INHERITED_IGNORE (rust/dl/tests/interrupt.rs:318-329) is iterated by one test covering all three signals, and the ("INT", StillDrains(130)) row is the assertion whose absence let this through.
Non-blocking list — all six landed. Row 27 rewritten with the SIGINT exception and the two-conventions note; devlaunch-runner/src/interrupt.rs:3-9 and dl/tests/interrupt.rs:1-16 no longer describe a SIGINT-only world; README.md:243-247 scopes the claim to "none of these three" and names Ctrl-\ explicitly; the disown/setsid mis-credit is gone from the README and corrected at rust/dl/src/lib.rs:182-185; signalled's doc and row 27 both record that Session::exit_status declines 128+n and why neither generalises; INTERRUPTED is deleted and signalled is no longer pub.
The correction to my Standards item — you are right and I understated it. Confirmed at rust/devlaunch-runner/src/lib.rs:473-476: note_foreground_child → wait(...) → clear_foreground_child, so the clear runs after the reap, and the pre-branch claim that the window was "closed by clear_foreground_child running before the reap is observable" was already false independently of which signal delivered. The rewritten argument at interrupt.rs:173-185 — ESRCH on an empty group, and a live group needing full pid-space wraparound within a few instructions — is the correct bound, and stating it on the reap rather than the sender is what makes it hold for all three signals. Accepted; the 128+n narrowing likewise.
The trap '' TERM row — I agree it is not a defect, and it is not even new. Three things settle it:
- Pre-existing, provably. With comments stripped,
rust/devlaunch-runner/src/interrupt.rsis identical to pre-branchmain—killpg(pgid, SIGTERM)and everything around it. So atrap '' TERM; exec dlplus a Ctrl-C left the build alive ona1c6fe8too. The new table surfaced a pre-existing property; it did not introduce one. nohupis genuinely unaffected — measured, not reasoned.nohupleavesSigIgn: 0000000000000001: SIGHUP alone, no SIGTERM, so the drain'skillpg(…, SIGTERM)still fells the build. Same for the case that started all this — a non-interactive shell's background job isSigIgn: 0x6(INT|QUIT), also no SIGTERM. Demonstrated the mechanism directly, too: a parent that disarms SIGTERM and thenkillpgs its own child group with SIGTERM leaves the child alive; with SIGHUP disarmed instead, the child dies. Exactly the two rows as pinned.- So the caveat bites only a caller who explicitly disarmed SIGTERM for the whole subtree — someone who has said, in as many words, that no SIGTERM should end anything in here. Escalating to SIGKILL to defeat that would override an explicit instruction, deny
devpod upits own teardown, and mean editing the one file this PR deliberately kept clear of #343.
Pinning it as that row's expected outcome with the reason beside it is the right resolution, and better than weakening the assertion. If the drain's kill signal is ever worth revisiting it is its own ticket, after #343 lands.
One non-blocking item, newly introduced. The table is not quite the single source: DRAINED (rust/dl/src/lib.rs:153) is private, and INHERITED_IGNORE (rust/dl/tests/interrupt.rs:318) is a second, independent list in a separate crate. Nothing machine-checks that they agree, so a fourth signal added to DRAINED would still be untested by omission — the precise failure mode the comment at :316-317 claims to have closed ("so no signal is left untested by omission"), and that row 27 calls "Pinned by the INHERITED_IGNORE table". True for the three that exist; not true structurally. A unit test inside rust/dl/src/lib.rs — which can see the private const — asserting DRAINED's exact contents and pointing at the integration table would make the claim real for about four lines. Worth doing, not worth blocking on.
Verdict
Approve (posted as a comment: GitHub refuses --approve on a PR authored by the same account). Both blocking findings are resolved and independently verified; the whole non-blocking list landed; the one new item above is a four-line follow-up, not a gate. Merging stays with the human.
The two lists were independent: the handled set is private to dl's library and the boundary table lives in the integration crate, which is a separate crate and cannot see it. Confirmed by compiling the reference rather than reasoning about it — `constant DRAINED is private`, E0603. So a fourth signal added to the set would still be untested by omission, which is exactly what the table's own comment and divergence row 27 claimed to have closed. Closed as far as it can be closed without widening surface: a unit test beside the set, in the crate that can see it, pinning its exact contents. A signal added there now fails a test whose message names the file whose table has to grow with it. Proved by adding a fourth signal to the set alone — the guard fired with that message — then reverting. The alternative was making the set `pub` so the integration test could iterate it, which would be a genuine cross-check. It loses because this review has just finished trimming dead `pub` off this crate, and paying for a test with permanent API surface on the library `aid` links is the wrong trade for a three-element list. So the claim is narrowed to what is now true — the set cannot grow in silence — rather than left at "cannot disagree", and both the comment and row 27 say which one they mean. What still slips through is stated in the guard: an author who edits the guard and the set together and still leaves the boundary table alone, which is a deliberate act rather than an oversight, and the class of thing review catches and a tripwire cannot.
|
Addressed in The diagnosis, confirmed by compiling it rather than reasoning about itI checked the visibility claim directly before acting, because the fix depends on it. Adding So the report is exactly right: an integration test is its own crate, the handled-signal set is private to the library, and no assertion in What I choseA guard beside the set, in the crate that can see it — Proved it fires, not just that it passes. Added Then reverted; The alternative I did not take was And the claim narrowed, since the guard does not make the original one trueYou were right that a vaguer claim would not be an improvement, so both places now say which guarantee they mean and what escapes it.
Gates green locally: Good catch — this was the same defect class the PR had just been sent back for, one level up, and shipping it in the PR whose review found it would have been the worse outcome. |
main released 0.10.0 while this branch was in review, which left the PR unmergeable and — because GitHub cannot compute a merge commit for a conflicting PR — stopped CI from running at all on the last push. One conflict, in CHANGELOG.md: both sides added a Fixed entry under [Unreleased] and the release moved the section underneath them. Both entries kept, nothing reworded. Everything else merged clean, including all four files this branch touches.
|
Also merged The last push ran no CI at all, and that was not a fluke. main released 0.10.0 while this was in review, which left the PR One conflict, in Re-ran the gates on the merged tree: One note on the suite, since you hit the same thing: |
Six releases went out on main while this branch was open, and the `### Fixed` heading this entry was written under became 0.11.0's. Git matched it textually and merged without a conflict, which recorded an unreleased fix as having shipped two releases ago. It belongs under [Unreleased].
…as one reach limit Three corrections to the prose this branch adds. `--autorm` was renamed to `--rm` in 0.9.0 and now only prints the refusal that says so, so "the `--autorm` removal" names a flag that removes nothing. The README section this text points at already says `--rm`. `nohup` sets SIG_IGN for SIGHUP alone -- it does not touch SIGTERM -- so it cannot be cited as what disarms the pair. The code comment had this right and the README generalised it. And the claim that none of the three leaves a build running behind you has one exception, which this branch's own INHERITED_IGNORE table already records: the drain fells the child with killpg(..., SIGTERM), so a run whose SIGTERM was disarmed before it started disarms that reach too.
1 participant
Closes #304.
Only SIGINT was handled, so
kill <dl>and closing the terminal window endeddlwhere it stood — leaving the staged plaintextGH_TOKENfile on disk and thedevpod upchild orphaned. That is the exact pairdl/tests/interrupt.rsexists to prevent, reached by two doors it did not cover.The shared disposition both binaries install is now a small set of signals over one handler, and
install_interrupt_handleris renamedinstall_signal_handlersto match.Red → green, one slice each
MidUp::reached().signalled("TERM")returnedAftermath { code: None, token_left: true, up_alive: true }— died by the signal, token on disk, build still running. Green:DRAINED = [SIGINT, SIGTERM]over one handler that derives its exit code from the signal it was passed.DRAINED.nohup— adlstarted behindtrap '' HUP; exec …was killed by the SIGHUP it had been told to ignore. Green: the classic POSIX read-the-inherited-disposition idiom, applied to all three signals rather than to SIGHUP alone.The two sub-decisions the ticket delegated
SIGHUP joins. It is the same leak as SIGTERM through a door people walk through far more often, and the least watched of the three — the window any complaint would have appeared in is the one that just went away. What nearly won was leaving it out on the grounds that a closed terminal is ambiguous (
nohupandsetsidmean it deliberately), but that reading turned out to argue for slice 3 rather than against joining: the ambiguity is entirely carried by the inherited disposition, and once that is respected, a baredlin a closed terminal is unambiguous and anohup dlis untouched. Excluding SIGHUP would also have meant one signal inDRAINEDneeding a paragraph of exception, which is what rots.Exit code is 128 + signo — 130 SIGINT, 143 SIGTERM, 129 SIGHUP. Not invented here:
INTERRUPTED = 130was already 128 + SIGINT written out long-hand (inherited from Python'ssys.exit(130)), so this generalises what the codebase does rather than adding a convention, and it is derived in the handler from the signal the kernel passes, so no signal can be given the wrong code. Two alternatives were live. Re-raising with the default disposition so the parent seesWIFSIGNALEDis the more correct Unix citizenship, but the existing doc comment rejects it explicitly and for a real reason — a caller reading a child's status sees no exit code at all, where it used to see 130 — and reversing that would be a silent behaviour change for everyone scriptingdl. Keeping one code (130) for every signal was simpler by one function but throws away the only information distinguishing "someone cancelled this" from "the terminal went away", which is exactly what a CI log needs.On PR #343 and process groups
This change touches neither process-group membership nor the interrupt handler's foreground-pgid bookkeeping.
devlaunch-runneris not in the diff at all:note_foreground_child,clear_foreground_childand thekillpginsidedrain()are untouched, and nothing here changes which process ends up in which group. The whole diff is which signals reach the existing drain and what code the process exits with — the drain itself is called exactly as SIGINT already called it. So the SIGTTIN hangs and orphaned children under revision on #343 are independent of this, and the two should merge cleanly. Files that PR is rewriting were deliberately avoided.Testing
Five tests at the binary boundary in
dl/tests/interrupt.rs, all against the fakedevpodwhoseupblocks with the token staged and the child live. Each asserts oneAftermath— exit code, whether the token survived, whether the build survived — so a signal that half-cleans up cannot pass. The pre-existing SIGINT test now goes through the same harness and asserts the same three facts it always did.cargo test --workspace,cargo clippy --locked --all-targets -- -D warnings,cargo fmt --checkandpixi run styleall green.Summary by Sourcery
Extend shared signal handling so SIGTERM and SIGHUP perform the same safe cleanup as SIGINT without breaking intentional signal ignores.
Bug Fixes:
nohupbehavior intact while retaining the existing SIGINT handling semantics.Enhancements:
dlandaid.Documentation:
Tests: