Skip to content

Make server teardown safe against pid reuse and refuse to erase unreadable stores - #21

Merged
quantizor merged 3 commits into
harden/trust-and-input-validationfrom
fix/process-safety-teardown
Aug 9, 2026
Merged

Make server teardown safe against pid reuse and refuse to erase unreadable stores#21
quantizor merged 3 commits into
harden/trust-and-input-validationfrom
fix/process-safety-teardown

Conversation

@quantizor

Copy link
Copy Markdown
Owner

Settles the process-safety cluster from the backlog: teardown that could signal the wrong process, a deliberate stop that left an escaped descendant running, a crash sweep that signaled an already-reaped pid, and a store read failure that erased real data. Stacked on #19; review that first.

Teardown never signals a recycled pid

Every signal devctl sends while tearing a server down is now checked against the identity it recorded for that process while it was alive. A pid the kernel has since handed to something else is never signaled. The deliberate-stop and crash-cleanup paths share one revalidated sweep (signalRun over ProcessTree.liveDescendants), which unions the three ways a descendant can be found: the startup snapshot, a fresh parent-chain sweep, and the root's session members. The root's process group is signaled only while its pid still names the recorded process; the crash path, whose root is already reaped, never signals the group at all.

This closes three gaps that were separate backlog entries:

  • A stop racing a start could kill the wrong process. The teardown read the mutable pid and signaled without revalidation, so a restart that recycled the pid meant the signal could land anywhere, including the caller. recordOutcome now captures the run's pid, session, and snapshot at entry, before a concurrent start can replace them, and revalidates before every signal.
  • A deliberate stop left escaped descendants running. Stop swept only the parent chain, so a helper that backgrounded a grandchild and then exited left the grandchild reparented and alive. Stop now sweeps the whole session too.
  • The crash sweep signaled a reaped pid. It sent kill(-pid) after waitpid had already reaped the root; a recycled group would take the signal. The crash path now passes no root identity, so the group is never signaled, and only the descendants that still match are.

signalTree also no longer skips a group member when the group itself was not signaled, and the now-redundant escalateIndividuals folds into it.

The daemon refuses to start rather than erase its records

AtomicFile.load separates three outcomes a single nil used to blur together: a missing file (start empty), a file that exists but cannot be READ (throws), and a file that reads but will not parse (quarantined, start empty). The daemon probes its registry, run state, and lock stores this way before serving and exits on a read failure (EMFILE as it nears its fd limit, an I/O error), so launchd retries instead of the next write overwriting real data with an empty store. loadDefensively keeps the collapse-to-nil behavior, now used only for the rebuildable update cache and secondary hint reads.

Verification

  • make test passes (7 new tests). deliberateStopKillsAnOrphanedSessionGrandchild uses a new --orphan-grandchild fixture mode to create a session-only descendant and asserts stop reaps it; concurrentStopAndStartNeverSignalTheWrongProcess races stop against starts across rounds and confirms the host process survives and the server settles; AtomicFileTests cover missing, valid, corrupt, and unreadable.
  • scripts/smoke.sh passes (whole-group death on stop and child survival across a daemon kill are unchanged); each commit builds in isolation.

Not claimed here

This addresses the teardown signalling safety. A separate question of continuation timing under heavy concurrent stop/start churn is not part of this change; the tests here bound child lifetime so a regression slows a case rather than hanging the suite.

…sion

Both the deliberate-stop and crash-cleanup paths now run one revalidated pass,
signalRun over ProcessTree.liveDescendants, which unions the startup snapshot, a
fresh parent-chain sweep, and the root's session members. The root's process
group is signaled only while the pid still names the process whose identity was
captured while it was alive, so a recycled pid is never hit; the crash path,
whose root is already reaped, passes rootIdentity nil and never signals the
group at all. recordOutcome captures the run's pid, session, and snapshot at
entry, since a concurrent start can replace them while it is suspended.

This closes three gaps: a stop racing a start could signal a pid that a restart
had recycled (the "stop kills the caller" report); a deliberate stop swept only
the parent chain, so a descendant that backgrounded itself and reparented away
survived; and the crash sweep signaled -pid after waitpid had reaped it.
signalTree also no longer skips a group member when the group itself was not
signaled, and escalateIndividuals folds into that. A new fixture mode,
--orphan-grandchild, backgrounds a sleep through a shell that exits so a
teardown test can exercise the session-only case.
…g it

AtomicFile.load separates three outcomes a single nil used to blur: a missing
file returns nil (start empty), a file that exists but cannot be READ throws
(EMFILE as the daemon nears its fd limit, an I/O error), and a file that reads
but will not parse is quarantined and returns nil. The daemon's main probes the
registry, run state, and lock stores this way before serving and exits on a read
failure, so launchd retries rather than the next write overwriting real data
with an empty store. loadDefensively keeps the collapse-to-nil behavior for the
rebuildable update cache and secondary hint reads.
Records that stop() and the crash path share one revalidated sweep over
liveDescendants, that the crash path never signals a reaped group, and that
AtomicFile.load distinguishes missing, unreadable, and corrupt so the daemon
refuses to start rather than erase. Adds the changeset.
Copilot AI lite review requested due to automatic review settings August 9, 2026 20:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@quantizor
quantizor merged commit 327fc60 into main Aug 9, 2026
4 checks passed
@quantizor
quantizor deleted the fix/process-safety-teardown branch August 9, 2026 20:53
@github-actions github-actions Bot mentioned this pull request Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants