Register broker ownership and safely reclaim abandoned helper trees - #563
Register broker ownership and safely reclaim abandoned helper trees#563ccheng555 wants to merge 27 commits into
Conversation
Attributed-To: Codex interactive
Attributed-To: Codex interactive
Attributed-To: Codex interactive
Attributed-To: Codex interactive
Attributed-To: Codex interactive
… non-fatal cleanup verification, async non-blocking process cleanup, ps portability with degraded fallback, de-flaked idle-release tests Attributed-To: Codex executor lane (implementation), Claude interactive (verification, test-anchor fix) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MrRkob3GtGnbYqq6aeE5co
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38529f436e
ℹ️ 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".
…s, keep stream ownership on socket close, reap descendants before their parent Attributed-To: Claude interactive Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MrRkob3GtGnbYqq6aeE5co
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16547a0cc8
ℹ️ 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".
…kill phase, reclaim orphaned process group on unexpected child exit Attributed-To: Claude interactive Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MrRkob3GtGnbYqq6aeE5co
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58d84b1801
ℹ️ 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".
A TERM-resistant helper whose parent already exited is reparented out of the root's tree, so it was signaled by the group kill but never tracked: polling saw no live records, skipped SIGKILL, and reported verified while the helper was still alive. Group members observed at scan time now enter the shared tracked map, so they escalate and count as survivors. Attributed-To: Codex executor lane Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
connect() rejected before returning the client, so a detached app-server that answered initialize with an RPC error but stayed alive was never closed, and every later request could spawn another live helper tree. connect() now closes its own client on a failed initialize and relays an unverified cleanup outcome to the broker's existing refusal gate. Attributed-To: Codex executor lane Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 386411c99f
ℹ️ 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".
An adversarial audit found this branch's cleanup layer repeatedly claiming success it could not prove: an empty process-group selection, a missing root, and a dead root all produced verified: true while a detached helper could still be alive. This replaces those claims with one contract. Ownership is captured while the root is alive, so cleanup consults what it recorded owning rather than rediscovering a tree whose root has since exited. Absence of evidence is no longer proof of death: an unreadable process table, an unaccounted-for owned descendant, or a missing root all report verified: false. A root positively observed absent from a readable table, with nothing owned unaccounted for, is still a clean success, so cancelling an already-exited job does not surface a spurious error. Survivors are tracked as pid@lstart identities rather than bare pids, so pid reuse can neither mask a survivor nor wedge the broker permanently. Also: identity-capture failure no longer pre-kills the child in a way that made close() skip tree cleanup and suppress the unverified-cleanup gate; the broker closes its listener before awaiting child cleanup during shutdown, so a late client cannot start a replacement app-server it would then abandon; and callers no longer discard an unverified cleanup outcome and delete the ownership records needed to retry it. Attributed-To: Codex executor lane Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
|
Status note for anyone reading the review threads: the automated passes above all ran against commits that predate the current head ( Resolving the ones the current head addresses:
Left open deliberately: the two findings about reaping helpers before their app-server parent and keeping the root out of the descendant phase. Both are the same gap on systems where nothing reaps orphans, and both are named as out of scope in the PR description rather than silently dropped. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c663c57f41
ℹ️ 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".
|
Correction to my previous note: I resolved the "Reclaim independently grouped helpers after a child crash" thread on faulty reasoning and have reopened it. I had read The two findings from the review of
Working on both. |
Cleanup could adopt the identity of whatever process currently held a recorded pid, which makes identity verification a no-op and can signal an unrelated process tree after pid reuse. Background task records persisted only a pid, so that was the live path for cancel and session-end cleanup. - Remove the current-identity fallback; a record with no ownership evidence refuses to signal and reports unverified rather than killing. - Capture ownership when spawning background workers and brokers so the normal path stays fully verified. - Allow best-effort cleanup after an identity-capture failure only when the caller still holds a live child handle, which is what makes pid reuse impossible; persisted records never qualify and are left alone with a message saying so. Leaking a process is preferred over killing a stranger. - Include a live member of an owned process group even when it postdates the ownership snapshot, so a helper spawned after initialize is reclaimed instead of wedging the broker against ever spawning a replacement. Attributed-To: Codex executor lane (implementation), Claude interactive (review, verification) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed Refuse to adopt an unverified current PID. The fallback that took the current holder of a recorded pid as the expected identity is gone. A record with no ownership evidence now refuses to signal and reports unverified, rather than killing whatever inherited that pid. Background workers and broker sessions capture ownership at spawn time so the normal path stays fully verified instead of merely permitted. The one case that still needs best-effort cleanup is an identity-capture failure. That is now allowed only when the caller still holds a live, unreaped child handle, since that is the specific condition that makes pid reuse impossible. Persisted records never qualify: they are left alone with a message saying the job could not be verified as owned. Leaking a process is the intended outcome there. Worth stating plainly, because I got it wrong first: an earlier version of this fix persisted the capture-failure flag into the job record and honoured it on cancel and session end. That reintroduced the same defect by a different route, since a record on disk outlives its process. Caught on review, not by the tests, and there is now a test that fails specifically on it. Reclaim helpers created after the ownership snapshot. A live member of the owned process group is included even when it postdates the snapshot, so a helper spawned after Verification. Full suite: 118 tests, 114 pass, 4 fail — the same 4 environmental failures present on the base commit, by name. Four new tests. Reverting the source while keeping the tests fails exactly those four; reverting only the live-handle gating fails exactly the one test written for it. Still deliberately out of scope, as described in the PR body: reaping helpers before their app-server parent, and keeping the root out of the descendant phase, both on systems where nothing reaps orphans. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce3a15e9ae
ℹ️ 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".
|
Acknowledging the startup-race finding: it is real and it is mine. Ownership capture is a synchronous process-table scan sitting between the worker spawn and the job record being written, so a worker that starts quickly can read a record that does not exist yet, exit with The obvious fix — persist before spawning, then patch ownership onto the stored record — is not sufficient, so I am not pushing it. It moves the race rather than removing it: the worker loads the job record at startup and writes back a record derived from that load, so the parent's ownership patch and the worker's first write contend, and whichever lands second wins. Measured on a full unsandboxed run, that reordering takes the suite from 118 tests / 114 pass / 4 fail to 120 / 115 / 5, with The shape that should work is keeping ownership out of the record the worker rewrites — a sidecar the parent owns and the worker never touches — so there is no contention to order. I would rather land that than a reordering that trades one race for another. Head stays at |
Ownership data had no single author. The parent wrote a job record at enqueue time and the worker rewrote the same file as it ran, both through unlocked whole-record writes, so correctness depended on an ordering that neither side controlled. Successive fixes kept trading one race for another. The invariant now enforced by data ownership rather than by conditionals: a process identity may be recorded only by the process it describes, or by a holder of a live unreaped handle to it, and every persisted file has exactly one writer at a time. - The enqueuer writes the queued record before spawning and never writes it again. It no longer scans the process table, which removes the window where a fast worker read a record that did not exist yet and exited. - The worker records its own identity. A process scanning for itself cannot be confused by pid reuse, because it is alive while scanning. - Task records no longer carry an ownership snapshot. Descendants and process-group members are still discovered at termination time, and the worker leads its own group, so late-spawned helpers are still reclaimed. - Cancellation of a job whose worker has not started yet uses a create-only flag file whose existence is the whole message, so it cannot be contended. This closes a case where cancelling a queued job reported success while the worker went on to run it to completion. - Spawn sites that hold a live child handle keep capturing ownership directly; that is the other half of the invariant, and it is unchanged. Attributed-To: Codex executor lane (implementation), Claude interactive (design review, verification) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed The startup race. Ownership capture is a process-table scan, and it sat between spawning the background worker and writing its job record. The enqueuer no longer captures ownership at all; it writes the record before spawning and never writes it again. The worker records its own identity instead. A process scanning for itself cannot be confused by pid reuse, because it is alive while scanning — which is the property every earlier version of this code was missing. I tried the obvious fix first (persist, spawn, then patch ownership onto the record) and did not push it, because it moves the race rather than removing it: the worker loads that record at startup and writes back a record derived from it, so the parent's patch and the worker's first write contend. Measured, that reordering took the suite from 4 failures to 5, with A case that was not reported and is worth flagging. On The change shrinks the surface. Ownership capture drops from three call sites to two, both of which hold a live unreaped child handle. Task records no longer carry an ownership snapshot; descendants and process-group members are still discovered at termination time, and the worker leads its own group, so late-spawned helpers are still reclaimed — there is a test for that specifically. The invariant this all rests on is now stated in the PR description, so future changes have something to be checked against rather than being re-derived per call site. Verification. 124 tests, 120 pass, 4 fail — the same 4 pre-existing environmental failures, by name. Reverting only the cancel-flag check fails exactly the two flag-protocol tests and nothing else, which isolates them to the behaviour rather than to the feature's general absence. One flaky test is disclosed in the description rather than left to be discovered. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f27c1a2f0
ℹ️ 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".
|
Pushed The cancel tombstone did not survive the pass that wrote it. Session end wrote a flag for a queued job and then pruned that job, and pruning sweeps the flag. A worker caught between reading its queued record and publishing its pid would find nothing and run the job after the session ended — which is the exact case the flag exists to prevent. The job is now retained until the worker acknowledges the cancellation with its own terminal write. Pruning still sweeps flags for genuinely finished jobs; the ordering changed, not the sweep. Cleanup could not converge for a worker that exited abnormally. Dropping the ownership snapshot from task records in the previous commit left the "root absent from a readable table" branch still keyed on that snapshot, so it reported unverified forever and the job sat permanently The failure cases are unchanged and now have their own guard: an unreadable table stays unverified, a live tracked descendant stays unverified, a present root whose identity does not match stays a refusal, and an absent pid with no ownership evidence at all is still not a success. Both are consequences of the previous commit's scope reduction rather than new territory, so they were in scope under the invariant rather than requests to widen it. Verification. 127 tests, 123 pass, 4 fail — the same 4 pre-existing environmental failures, by name. Isolating red arm run per fix: reverting only the retention change fails only the tombstone test, reverting only the convergence change fails only the abnormal-exit test. Neither disarm disturbs the other. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03eac6e0da
ℹ️ 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".
Admitting live group members that postdate the ownership snapshot made reclaiming a late-spawned helper possible, but it also admitted the members of an unrelated group when the group id itself had been reused. Those members are absent from the snapshot, so the per-record identity check could not exclude them, and the leader identity check ran only after signals had already been delivered. The result was signalling a stranger's process tree and reporting it as unverified afterwards. Check the leader before admitting anything: if a live process holds the group id and is not the root we recorded, nothing in that group is ours, so refuse without signalling. A missing leader is still not a mismatch, which keeps the crashed-root case working where surviving helpers do need reclaiming. Attributed-To: Claude interactive Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed Group leader identity — fixed, and it was the more serious of the two. Admitting live group members that postdate the ownership snapshot is what makes reclaiming a late-spawned helper possible, but it also admitted members of an unrelated group once the group id itself had been reused. Those members are absent from the snapshot, so the per-record check could not exclude them, and the leader check ran only after signals had gone out. That is signalling a stranger's process tree and then reporting it unverified afterwards, which is the exact failure class this PR exists to prevent. The leader is now checked before anything is admitted. A missing leader is still not a mismatch, so the crashed-root case where surviving helpers do need reclaiming keeps working. Red-armed: removing the guard makes the new test fail by signalling the stranger. Identity resolution — real, not fixed, now documented. You are right that I am not fixing it here, for three reasons. It is on the disclosed limits list in the description alongside the other known gaps, so whoever picks this area up inherits the problem stated rather than hidden. Verification. 128 tests, 124 pass, 4 fail — the same 4 pre-existing environmental failures, by name. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f95424ce1
ℹ️ 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".
Retaining stream lifetime after a client disconnects left one piece still keyed on the socket. A client that disconnected before a detached review/start response arrived cleared the active stream socket, which suppressed recording the review thread id from that result. The completion notification then carried a thread the broker had never recorded, so it never matched, the stream stayed marked running, and the broker remained busy indefinitely. Each streaming request now takes a turn number and the result-derived update is gated on that turn still being the active one. The owning socket may come and go; the turn is what the stream actually belongs to. Known gap, stated rather than implied: unlike the other fixes on this branch, this one has no dedicated regression test. Covering it needs a broker integration test around detached review streams, which is the least stable part of the suite. The full suite passes unchanged. Attributed-To: Claude interactive Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed This finding was real and worth fixing. Retaining stream lifetime after a client disconnects left one piece still keyed on the socket, so a client that dropped before a detached Stated plainly: unlike every other fix here, this one has no dedicated regression test. Covering it needs a broker integration test around detached review streams, which is the least stable part of this suite, and I would rather ship a small reasoned fix with the gap named than either skip it or destabilise the suite chasing it. Full suite passes unchanged: 128 tests, 124 pass, 4 pre-existing environmental failures. Closing position. Every finding in this review series has been real, and several were introduced by earlier fixes in the series — that history is in the description rather than tidied away. The branch is now at the state I intend to defend: the invariant governing process ownership is written into the description, the known limits are listed there including the one-second identity resolution window and the zombie-ordering gaps, and each remaining open thread is deliberate rather than overlooked. Further automated findings will not be triaged into this branch unless they identify a violation of the stated invariant. Anything that requires widening the guarantee belongs in follow-up work against #543 and #526, where the surrounding design questions can be settled by someone who owns this area. Continuing to expand scope in an unreviewed branch trades a defensible change for a larger one nobody has read. Happy to rebase, split, or revise this if a maintainer picks it up. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19ac644d82
ℹ️ 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".
Attributed-To: Codex
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e8659be6c
ℹ️ 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".
Attributed-To: Codex executor lane
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d0b378d42
ℹ️ 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".
Attributed-To: Codex
|
@codex review Please review exact head
Please treat any confirmed violation of the registered-only ownership invariant as actionable. Suggestions that widen cleanup to unregistered or heuristic targets are intentionally out of scope. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cec19f8fc
ℹ️ 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".
Attributed-To: Codex
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fbd95872f
ℹ️ 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".
Attributed-To: Codex
Attributed-To: Codex
|
@codex review Please review exact head 58c1ea3 as one bounded final architecture pass. Focus only on violations of these invariants:
Please flag only confirmed executable violations on this head. Do not suggest heuristic cleanup of unregistered processes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58c1ea3158
ℹ️ 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".
Attributed-To: Codex
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 328bdd56f9
ℹ️ 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".
Attributed-To: Codex
|
@codex review Please review exact head 4e8884b as one bounded follow-up pass. Focus only on confirmed executable violations of these invariants:
Please flag only a concrete path that violates those invariants on this head. Suggestions for heuristic cleanup of unregistered processes remain out of scope. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e8884be6e
ℹ️ 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".
Attributed-To: Codex
Attributed-To: Codex
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f6a87f181
ℹ️ 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".
| fs.chmodSync(ownerPath, 0o600); | ||
| return { registered: true, ownerKey: owner.ownerKey, path: ownerPath, owner: existing }; | ||
| } | ||
| createImmutableJson(ownerPath, payload); |
There was a problem hiding this comment.
Revalidate reused owners at publication time
When an existing broker is reused, the session owner is checked before the awaited endpoint probe in ensureBrokerSessionLocked, but this write publishes the owner without rechecking its exact identity. If that process-group leader exits during the probe or just before this call, the command proceeds with an owner row that was dead at creation; after the registry lock is released, a reaper can immediately classify the broker as abandoned and terminate it during the caller's request. Revalidate the owner identity while holding the lock immediately before creating or accepting the owner row.
Useful? React with 👍 / 👎.
| live = getLiveProcessPidsImpl([existingOwner.pid]); | ||
| } catch { | ||
| return { acquired: false, reason: "registry-lock-liveness-unavailable", path: lockPath }; | ||
| } | ||
| if (!Array.isArray(live) || live.includes(existingOwner.pid)) { |
There was a problem hiding this comment.
Bind registry locks to a process identity
When a registry-lock holder crashes and its PID is reused before another contender arrives, this PID-only liveness check treats the unrelated replacement as the lock owner. A long-lived replacement can therefore leave the registry permanently registry-busy, blocking owner release, child observations, and registered reaping. Persist the lock creator's process identity and pass it to the liveness check so PID reuse allows the abandoned lock to be quarantined.
Useful? React with 👍 / 👎.
Adds durable ownership evidence and a registered lifecycle so Codex Companion broker, app-server, and helper trees do not depend on manual process-name cleanup. This expands the earlier app-server work into the ownership architecture tracked by CC-5219 and CC-5589, and relates to #543 and #526.
Root cause
A detached shared broker could outlive its session, and its app-server could be spawned or initialized before durable child ownership existed. SessionEnd could not run after a hard crash and was scoped to the session cwd, while pid files were not a durable identity handle. An external reaper therefore could not safely distinguish abandoned trees from live shared ownership. Process names, PPID 1, command text, age, and socket presence are observations, not authority to signal.
Ownership invariant
A process identity may be recorded only by that process or by a parent holding its live unreaped child handle. Automated cleanup is eligible only when the immutable broker registry is valid, at least one valid owner row exists, every unreleased owner is freshly proven absent or reused, and every target still matches the recorded identity immediately before signalling.
One live owner blocks cleanup. Missing, malformed, unreadable, overly permissive, symlinked, incomplete, locked, identity-mismatched, caller-supplied, manual, and unregistered state remains report-only.
What changed
codex app-server; broker death or publication failure before activation closes the control pipe and starts no Codex child.ps sessis an audit session and is explicitly never used as a containment boundary.Honest scope
This PR never infers ownership for manual broker starts or caller-supplied endpoints, and it does not add broad process-name cleanup.
No user-space harness can run cleanup at the instant the entire session is killed. The contract is immediate cleanup on normal SessionEnd and automatic registered-only convergence on the next SessionStart. If the harness never starts again, no future cleanup code can execute.
Unix process identity remains bounded by the platform surface. On systems where
ps lstarthas only whole-second resolution, this PR does not claim a kernel start-tick identity. Cleanup still requires immutable registered ownership plus fresh PID, start-time, process-group, and group-leader agreement, and fails closed on every observable mismatch, but a same-PID same-second replacement is an explicitly accepted portable boundary.Checks
npm test: 194 passed, 0 failed, 0 skipped.git diff --check, and the staged public-repo leak sweep are clean.Review closure
Current head:
4f6a87f1810b84a851b50dac85a5dc9c05674215.The prior exact-head Codex review found two remaining gaps: post-activation helper ownership and repeated cleanup of already-verified registries. Both are fixed with red-to-green regressions, and both review threads have implementation receipts and are resolved. The single bounded follow-up review found a P1 cleanup-authority containment gap and a P2 lock-contention gap;
4f6a87fandec40de8fix them with report-only structural validation and synchronous fail-closed teardown. Both threads are resolved.The bounded automated-review round is closed. No further automated review has been requested; new maintainer feedback can be handled as a new bounded change.