Skip to content

fix(lifecycle): close supervisor review follow-ups from rounds 4 to 6 - #859

Open
roodboi wants to merge 63 commits into
nextfrom
feat/lifecycle-supervisor-stop-followups
Open

roodboi wants to merge 63 commits into
nextfrom
feat/lifecycle-supervisor-stop-followups

Conversation

@roodboi

@roodboi roodboi commented Nov 15, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #802 (merged): the open non-blocking findings from the lead's review rounds 4, 5 and 6. Base next. Each finding maps to a change below.

Round 4

Finding Change Evidence
1. A descendant forked after the recovery capture gave unknown immediately Recovery::live returns Captured, Unobserved or Gone. Unobserved members are never signalled; stop waits until the target's own deadline for them, and reports unknown only if they are still alive orphan_recovery_waits_for_unobserved_descendant_to_exit and orphan_recovery_never_signals_unobserved_descendant; both fail with an immediate error on Unobserved
3. guard_lost_mid_stop_keeps_admission_snapshot did not force the fallback The helper freezes the stop request, kills the guard, and resumes only after the guard has no process record (zombie-aware lookup that matches production on Darwin) All three guard-loss tests fail with the admission fallback removed
4. One unverifiable receipt aborted the whole stop before any signal Admission is per target. An unverifiable receipt is never signalled and records the failure; the owner's other targets are still stopped; the reply is unknown / stop-incomplete unverifiable_receipt_does_not_block_healthy_targets fails with the old abort

Round 5

Finding Change
1. Linux thread scan could read a live process as a zombie during complete thread turnover "No live thread" needs two consecutive /proc/<pid>/task listings to agree. inspect re-reads the leader's start time after assembling an identity from stat, status and the task list, and repeats the reads if the PID was reused. No deterministic test: the window is complete thread turnover inside microseconds
2. "Every stop and status request ends with one framed receipt" was too strong The doc now says this holds for requests whose frame is read, and lists the exits without a frame and what they mean
3. The stop lock is released before the reply's receipts are read The doc says processes, liveProcessNames and unknownProcessNames are a snapshot at reply time; outcome, reason and the exit status describe the request
4. Most reason codes were not pinned foreign-live-process, stop-incomplete and retired-owner-live are asserted. A refused_framed helper requires the frame for stop and status refusals
Nits New reason foreign-unknown-process (was io-error), with a test. The receipt-read-failed comment is fixed. The doc notes the retire-failed marker may remain. #[track_caller] on Fixture::request and Fixture::state

Round 6

Finding Change
1. The concurrent append test did not force overlap or catch a split in the timestamp Both bursts start from one start file. The test requires an alpha record after the first beta record and checks every field, including a 20-byte timestamp. Mutation: writing each record in two pieces fails it (3 of 3)
2. The short-write sentence described only the good case The doc covers: readers hold back an unterminated last line; a full disk can leave a damaged head followed by a whole record; a failed log write ends that guard's command; a file-size limit's SIGXFSZ ends the guard itself, and stop recovers its command as after any lost guard
3. A guard frozen before it detaches The lock table says it keeps run and stop of the whole root failing, each within its bound, for as long as it is frozen. It also covers the failure-path hold, a guard with no receipt yet, and the recovery step (find the stopped guard, then SIGCONT or kill it)
Nits The lock table names the plan-write deadline, the rollback time, and the signal-triggered stop as a waiter

Decisions for the lead

  • Q2, failed log write: today a failed log write ends that guard's command (TERM, grace, KILL). I kept that contract and documented it. The alternative is to keep the command running and drop output. If you keep the current contract, I recommend also ignoring SIGXFSZ in the guard, so a file-size limit takes the same orderly path instead of ending the guard. That is a behaviour change, so it is not in this PR.
  • Q1, APFS append atomicity above one page: not verified in source. The tightened two-guard test runs in the hosted macOS jobs.
  • Codex P2 on feat(lifecycle): ship supervisor with CLI installations #821 (numeric group signalling race): still open. I recommend keeping it documented for this release.

Validation (Linux 6.8 x86_64)

  • fmt; Clippy with warnings denied on Linux, aarch64-apple-darwin and x86_64-apple-darwin.
  • 13 unit and 57 fake-caller tests.
  • tests/machine-output-contract.test.ts, tests/sync-spawn-bound.test.ts and tests/tla-runtime-models.test.ts: 267 pass.
  • Typecheck; all 8 check:native-plan-cli acceptance scripts.
  • A recovery-group loop at this head is in progress; its count goes in the Ready note.

🤖 Generated with Claude Code

roodboi and others added 30 commits November 13, 2026 19:03
…isor

# Conflicts:
#	scripts/check-native-env-plan-cli.ts
…isor

# Conflicts:
#	scripts/check-native-env-plan-cli.ts
Freeze the guard before removing it and prove it has not begun its own
escalation and that the TERM-ignoring child is still live, so only the
stop's admission snapshot can KILL the child. Require a fresh five-second
recovery grace after guard loss. Align the documented model summary with
the maintained 426-state coverage.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@roodboi

roodboi commented Nov 15, 2026

Copy link
Copy Markdown
Contributor Author

Status at bf6787000da21f464854b38fe5232d3adef75805: the red docker-e2e and test summary gates are the CI draft rule ("skipped: draft — full pull-request evidence is required before merge"), not a test failure. This PR stays a draft until #802 merges and GitHub retargets it to next; I'll then merge next, mark it ready, let the full suite run, and post a Ready note. It now carries only your round-4 findings 1 and 3 (finding 2, the stop-lock bound and framed receipts, moved into #802). Persistent reviewer: CLEAN at this head; Codex bot: no major issues at this head.

roodboi and others added 6 commits November 15, 2026 06:03
…isor

# Conflicts:
#	docs/reference/v5-model-coverage.md
#	scripts/lib/tla-runtime-models.ts
…on a sibling

Every guard took a blocking flock on the shared lifecycle.log around each
record. A guard stopped (SIGSTOP, a debugger, a hung write) while holding
it stalled every sibling guard's loop in flock: no output, no exit
detection, no stop handling. Captured on macOS as the 9/100 failure of
expired_target_does_not_abandon_other_recovery (worker guard blocked in
Lines::flush -> flock while the frozen blocked guard held the lock).

Each record is now one write on the O_APPEND descriptor with no lock. The
kernel places each append atomically, so whole records from different
guards never interleave on a local file system; only a short write (for
example a full disk) can split one. The unused flock retry helper and its
unit test are removed.

Regressions: held_log_lock_never_stalls_a_guard holds the log flock from
outside for the whole run and requires output to reach the log and stop
to answer stopped within the normal bound (fails with the old lock);
concurrent_guards_append_whole_records_without_a_lock checks two guards
bursting 64 KiB records leave only whole, unmixed records. Docs state the
log's ordering and integrity and list every lock, who can hold it across
a blocking call, and the bound.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hort-write causes

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Base automatically changed from feat/lifecycle-supervisor to next November 15, 2026 09:02
roodboi and others added 4 commits November 15, 2026 09:15
…isor-stop-followups

# Conflicts:
#	docs/lifecycle-supervisor.md
#	packages/lifecycle-supervisor/src/os.rs
#	packages/lifecycle-supervisor/src/supervisor.rs
#	packages/lifecycle-supervisor/tests/fake_caller.rs
…echeck Linux identities

Round-4 finding 4: one receipt of the owner that could not be verified
(guard gone, no witness for its session) returned from stop admission
before any target was signalled, so healthy commands of the same owner
kept running. Admission now records that failure and skips only that
receipt, which is never signalled; the other targets get their normal
stop, and the reply is still unknown with stop-incomplete. Regression:
unverifiable_receipt_does_not_block_healthy_targets (fails with the old
abort).

Round-5 finding 1 (Linux): a single listing of /proc/<pid>/task could read
a process whose main thread exited as a zombie if every listed thread was
replaced during the scan. "No live thread" now needs two consecutive
listings to agree. inspect also re-reads the leader's start time after
assembling an identity from stat, status and the task list, and repeats
the reads if the PID was reused in between, instead of combining two
processes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ument reply and log limits

Round-5 and round-6 follow-ups (all non-blocking):
- A foreign receipt whose live members no witness ties to it now answers
  foreign-unknown-process instead of io-error. Tests pin it and the
  foreign-live-process, stop-incomplete and retired-owner-live reasons; a
  refused_framed helper requires the frame for stop and status refusals.
- concurrent_guards_append_whole_records_without_a_lock now starts both
  bursts from one start file, requires them to overlap, and checks every
  field including a 20-byte timestamp; splitting each record into two
  writes fails it.
- Docs: the framed-reply guarantee covers requests whose frame is read, and
  lists the exits without a frame; reply process lists are a snapshot
  taken after the stop lock is released; readers hold back an unterminated
  last line; a failed log write can split a record and ends that guard's
  command (a file-size limit's SIGXFSZ ends the guard itself); a guard
  frozen before it detaches keeps run and stop failing within their bounds,
  with the recovery step; the retire-failed marker may remain; the lock
  table names the plan-write deadline, the rollback time and the signal
  stop as a waiter.
- Comment fix (receipt-read-failed) and #[track_caller] on Fixture::request
  and Fixture::state.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@roodboi roodboi changed the title fix(lifecycle): finish supervisor recovery stops on Linux 6.8 and wait out late descendants fix(lifecycle): close supervisor review follow-ups from rounds 4 to 6 Nov 15, 2026
@roodboi
roodboi marked this pull request as ready for review November 15, 2026 09:25
…count

Reviewer P1: two listings of /proc/<pid>/task still cannot prove a thread
group dead under repeated thread turnover, because a listing never sees a
thread created after it. A leader that shows Z now counts as dead only when
the thread count on its own stat line (field 20, read in the same read as
the state) is at most one. An exiting thread stays counted until released,
which only errs toward live. The task scan is removed.

Regression: main_thread_exit_with_live_worker_is_still_stopped also runs a
pthread-churn fixture whose only worker keeps replacing itself, with and
without guard loss; stop must signal it and the process must be gone.

Reviewer P2: concurrent_guards_append_whole_records_without_a_lock now
forces overlap with a barrier on the shared log (each writer waits until
both halves are logged) and asserts overlap in both directions; splitting
records into two writes still fails it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@roodboi

roodboi commented Nov 15, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 87b9477729

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 87b9477729

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

roodboi and others added 3 commits November 15, 2026 09:47
…at a fixed time

Hosted Intel macOS failed orphan_recovery_waits_for_unobserved_descendant_to_exit:
the fixture's late child lived a fixed six seconds from its own fork, which
can land after the stop's seven-second deadline on a slow runner. The child
now waits until its parent (the captured command) has been killed by
recovery at five seconds, then exits 0.5 s later, well inside the deadline.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reopen bound from the earlier fix could still run out under heavy
contention: a reader that kept opening an inode the writer replaced before
its fstat refused after 64 tries (4 of 24 runs of
receipt_reads_survive_concurrent_atomic_replacement under parallel load).
A read-only open now accepts an inode that was unlinked after the open: it
is a complete receipt, the version current at the open, and an unlinked
inode has no name a hard link could alias. Opens that may write still retry
the name for its current inode. Extra hard links are still refused.
Stress: 0 of 40 parallel runs fail (was 4 of 24).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…before fork

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@roodboi

roodboi commented Nov 15, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 4002381f2e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 4002381f2e

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@roodboi

roodboi commented Nov 15, 2026

Copy link
Copy Markdown
Contributor Author

Ready for lead: 4002381

Follow-up to #802: every open non-blocking finding from your rounds 4, 5 and 6, mapped one by one in the PR description. Base next 34b5053. The merge of next resolved #802's squash to this branch's side; the delta against next was proven identical to the branch's own work before the new commits.

Since the description was written (all reviewer-checked):

  • Linux thread-group liveness (reviewer P1 on my first attempt): a Z leader now counts as dead only when the thread count on its own stat line (field 20, the same read as its state) is at most one. No /proc/<pid>/task listing is used. Regression: main_thread_exit_with_live_worker_is_still_stopped also runs a pthread-churn fixture whose only worker keeps replacing itself, with and without guard loss.
  • Receipt reads under contention: feat(lifecycle): add standalone Rust supervisor with crash recovery #802's 64-reopen bound could still run out (4 of 24 parallel stress runs of receipt_reads_survive_concurrent_atomic_replacement; diagnostics showed the retry exhaustion). A read-only open now reads the complete version it opened. Write-capable opens still retry for the current inode, and nlink > 1 is still refused. Result: 0 of 40 under the same stress.
  • Hosted Intel macOS failure of orphan_recovery_waits_for_unobserved_descendant_to_exit: test timing. The late child lived a fixed 6 s from its own fork and could pass the 7 s deadline. It now exits 0.5 s after its parent dies, comparing against the parent PID saved before the fork.
  • Append-overlap test: overlap is now forced by a barrier on the shared log (each writer waits until both halves are logged) and asserted in both directions. Writing each record in two pieces fails it (3 of 3).

Loop counts (recovery group orphan guard_lost expired_target reused dead_guard, default threads, Linux 6.8 x86_64): 100/100 at this head; 100/100 at 87b9477 and 100/100 at b4c27af (earlier heads of this PR). The group includes the new late-descendant tests.

What I ran at this head: fmt; Clippy with warnings denied on Linux, aarch64-apple-darwin and x86_64-apple-darwin; 13 unit and 57 fake-caller tests. At 87b9477 (repo files unchanged since): tests/machine-output-contract.test.ts, tests/sync-spawn-bound.test.ts and tests/tla-runtime-models.test.ts (267 pass), typecheck, and all 8 acceptance scripts. Mutation checks: round-4 item 4 (old abort), late descendant (immediate error), split appends, and the held log lock. computer pr review-status: CLEAR at 4002381. Hosted CI at note time: 15 pass, 2 pending, 0 failed.

Review: the persistent Codex reviewer is CLEAN at this head, after findings on the task scan, the overlap test and the fixture's parent PID. I re-requested the Codex GitHub bot.

Decisions for you:

  • Q2: a failed log write ends that guard's command. I kept and documented this. If you keep it, I recommend ignoring SIGXFSZ in the guard as a follow-up, so a file-size limit no longer ends the guard itself.
  • Q1: APFS append atomicity above one page is not verified in source; the hosted macOS jobs run the two-guard test.
  • feat(lifecycle): ship supervisor with CLI installations #821's group-signal race: I recommend keeping it documented for this release.

For your Mac rerun: the same 100-run recovery-group loop. The crate changes are in os.rs (Linux only), storage.rs (both platforms) and supervisor.rs (stop admission, reason).

#811 and #821 do not contain this PR's receipt-read fix; it reaches them through next once this merges.

…isor-stop-followups

# Conflicts:
#	docs/lifecycle-supervisor.md
@roodboi

roodboi commented Nov 15, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@roodboi

roodboi commented Nov 15, 2026

Copy link
Copy Markdown
Contributor Author

Ready for lead: daa096f

This head is a normal merge of next at 081a526 (#811 squashed, #863, #808) into 4002381, the previous Ready head. Against next, this PR changes 5 files: packages/lifecycle-supervisor/src/os.rs, src/supervisor.rs, src/storage.rs, tests/fake_caller.rs and docs/lifecycle-supervisor.md. The Rust delta for your review is git diff origin/next daa096f35 -- packages/lifecycle-supervisor.

Merge: #811 and #859 both changed the crate, so this was a real two-sided merge.

What I ran (Linux 6.8 x86_64):

  • fmt; all-feature Clippy with warnings denied on Linux, aarch64-apple-darwin and x86_64-apple-darwin.
  • Crate tests: 13 unit, 58 fake-caller, 18 incremental and 2 crash-checkpoint.
  • tests/machine-output-contract.test.ts, tests/sync-spawn-bound.test.ts and tests/tla-runtime-models.test.ts: 269 pass.
  • Typecheck; all 8 acceptance scripts; generators (no diff).
  • feat(lifecycle): opt into built-in host process supervision #811's 306 lifecycle Bun tests against the merged release binary.
  • lifecycle-session-recovery e2e under the supervisor runner.

Recovery-group loop at this head: 100/100 (orphan guard_lost expired_target reused dead_guard, default threads). Earlier heads of this PR: 100/100 three times.

Status: computer pr review-status is CLEAR at daa096f. Hosted CI at note time: 15 pass, 2 pending, 0 failed. The persistent Codex reviewer is CLEAN on the merge commit. I re-requested the Codex GitHub bot.

The decisions (Q1 APFS, Q2 failed log write and SIGXFSZ, the group-signal race) are unchanged from the previous note.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: daa096f35e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: daa096f35e

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

This branch has not been deployed

No deployments
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.

1 participant