Bound PTY hard-stop cleanup latency

[steipete]

Summary

Bound PTY idle/deadline hard-stop latency without abandoning detached processes that still retain the PTY.

The original PR routed a live root through the scoped abort path, but that could miss true double-forked/session-escaped holders. This reconstruction keeps the immediate bounded abort while moving holder discovery and cleanup into a small, deadline-bounded lease.

Design

• launchPTY reserves one CLOEXEC duplicate of the PTY master before posix_spawn. Reservation failure prevents child creation, so cleanup never needs to allocate under EMFILE/ENFILE pressure.
• A lock-protected one-shot owner transfers that descriptor to cleanup, discards it before output-overflow close-master-first aborts, or closes it during ordinary teardown.
• Early-stop cleanup always uses the bounded path, even if the root exits during the settle window. Normal exited-before-deadline fix: drain PTY output after process exit #2807 pre-drain cleanup remains unchanged.
• The owned root/process group is synchronously aborted first. Holder discovery starts afterward, so processes created by root TERM handlers are visible.
• The deferred worker captures no SpawnedProcessGroup: only immutable output identities, exclusions, grace values, and the reserved master descriptor.
• The reserved descriptor prevents PTY identity reuse. A production 15-second lease closes/disarms it exactly once; expired scan results cannot signal.
• Holder cleanup sends TERM, then repeatedly discovers current PID-safe holders, sends SIGKILL, waits, and rescans until the PTY is clear or the lease expires.

Deliberately unchanged:

• the /exit write for ordinary non-overflow cleanup;
• output-overflow ordering: discard reserve, close master, scoped abort;
• normal exited-before-deadline synchronous cleanup and fix: drain PTY output after process exit #2807 EOF/EIO drain bookkeeping;
• non-PTY subprocess behavior.

Regression proof

The composite deterministic fixture covers four escaped generations:

1. a double-forked holder whose intermediate parent exits and whose grandchild calls setsid();
2. a holder forked by that process's TERM handler;
3. a third-generation holder created only after cleanup records its first kill snapshot;
4. a separate session-escaped holder created by the root's TERM handler during the synchronous abort.

A second fixture proves an early-stopped root can exit during settle while a detached holder remains, without returning to synchronous system-wide discovery.

Wall-clock assertions use the repository standard: 3 seconds locally and 9 seconds on loaded CI. Synthetic discovery begins one second beyond that budget in either environment, so an implementation that waits for discovery still fails. A DEBUG-only lease allowance keeps the artificial delay separate from production's fixed 15-second cleanup window.

Validation

• Composite four-generation regression repeatedly returned in 1.36–1.52 seconds locally and removed every PID.
• Settle-exit regression repeatedly returned in 1.04–1.05 seconds locally and removed the detached holder.
• KiroStatusProbeTests: 57/57 passed.
• SpawnedProcessGroupTests: 18/18 passed, including descriptor ownership, reservation failure, and lease expiry.
• TTYCommandRunnerTests: 29/29 passed.
• BoundedChildProcessProofTests: 3/3 across repeated runs.
• Release CodexBarCore build passed with DEBUG test APIs excluded.
• make check passed.
• Full local matrix: 841 selections across 71 groups passed with zero retries or timeouts.
• Structured Codex autoreview through P2 and TruffleHog passed with no accepted/actionable finding.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5778321424

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Keep sweeping detached PTY holders

When a provider CLI daemonizes a helper that keeps the PTY slave open while the root TUI is still alive (for example, a double-fork/setsid helper), this new live-root branch only signals the tracked descendants/process group via abortSynchronously() and skips the terminateSynchronously() output-holder sweep. That detached holder is no longer a descendant or group member, so cleanup can return after killing the root and leave the helper process running; the previous path found it by scanning PTY descriptor holders. Please retain a holder sweep for this case, or run it after the bounded root abort.

Useful? React with 👍 / 👎.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed August 13, 2026, 3:12 AM ET / 07:12 UTC.

ClawSweeper review

What this changes

The PR makes PTY hard-stop cleanup return promptly while a deferred, lease-bounded worker terminates detached processes that still retain the terminal.

Regression provenance

Possible regression — probable (reviewed change; known regression link). No predecessor PR is attributed.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

The final head addresses the prior escaped-holder concern with a bounded caller path and deterministic coverage; no discrete correctness finding remains. The remaining choice is whether a utility-queue holder scan that can outlive its disarmed 15-second lease is acceptable operationally.

Priority: P2
Reviewed head: b01946c9d37097f55c40a967f934b3f8f5508196
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	The reconstructed patch is focused and well-covered by deterministic fixtures, with one explicit operational availability decision remaining.
Proof confidence	🌊 off-meta tidepool	Not applicable: This is a maintainer-authored PR, so the external-contributor proof gate does not apply; the body nevertheless provides detailed repeated local fixture results.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: This is a maintainer-authored PR, so the external-contributor proof gate does not apply; the body nevertheless provides detailed repeated local fixture results.
Evidence reviewed	5 items	Current-main gap: Current main sends ordinary early-stop cleanup through synchronous termination, whose residual-process collection includes system-wide output-holder discovery; this is the latency path the PR replaces. Final bounded implementation: The final head reserves a CLOEXEC PTY-master duplicate before spawning, aborts the owned root first, then dispatches holder cleanup without making the caller wait beyond the short completion wait. Regression coverage: The final head adds deterministic fixtures for double-fork/session escape, TERM-created holders, post-snapshot descendants, and root exit during early-stop settle.
Findings	None	None.
Security	None	None.

How this fits together

CodexBar runs provider CLIs through a pseudo-terminal and consumes their output for status probes. When a probe stops early, its cleanup must release the caller quickly while preventing detached helpers from keeping the PTY alive.

flowchart LR
A[Provider CLI] --> B[PTY command runner]
B --> C[Early-stop cleanup]
C --> D[Abort root process group]
D --> E[Deferred holder scan]
E --> F[Lease expiry guard]
E --> G[Detached holders terminated]
B --> H[Probe result returned]

Loading

Decision needed

Question	Recommendation
Is it acceptable for an already-started utility-queue system-wide PTY-holder scan to continue after the 15-second lease has disarmed it, provided it can no longer signal processes or retain the reserved descriptor?	Accept the disarmed worker: Merge with the current design, relying on the lease to bound caller latency, descriptor ownership, and all late signaling.

Why: Source confirms the lease bounds caller latency and late cleanup effects, but cannot cancel a scan already inside process enumeration; the operational resource tradeoff needs maintainer intent.

Before merge

• Resolve merge risk (P1) - The lease prevents late signals and closes the reserved descriptor after 15 seconds, but an already-started system-wide holder scan is not cancellable and may continue on a utility queue after lease expiry.
• Complete next step (P2) - A maintainer must accept or reject the remaining bounded-lease versus uncancellable-scan availability tradeoff; no mechanical repair is currently indicated.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production versus test delta	production +354/-12, tests +402 across 5 files	The patch is accompanied by more deterministic regression coverage than production code, including escaped-holder and lease-lifetime cases.

Merge-risk options

Maintainer options:

1. Accept the bounded caller path (recommended)
   Land the lease-based design after maintainer acceptance that expiry disarms cleanup but does not cancel a scan already in progress.
2. Require cancellable discovery
   Keep the PR open for a redesign that can stop or time-slice holder enumeration itself at lease expiry.

Technical review

Best possible solution:

Keep the deferred post-abort holder cleanup and accept it only with explicit ownership of the disarmed-but-not-cancellable scan tradeoff; future hard bounding would require a cancellable discovery mechanism.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: current main synchronously performs output-holder discovery during ordinary early-stop cleanup, and the final head supplies deterministic fixtures for the escaped-holder cases. The fixtures were not executed in this read-only review.

Is this the best way to solve the issue?

Yes, conditionally: aborting the owned root before deferred holder discovery preserves prompt return and catches TERM-created detached holders; only the uncancellable-scan operational tradeoff remains for maintainer acceptance.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 208016687098.

Labels

Label changes:

• add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
• remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

• P2: This repairs provider-probe subprocess cleanup with a bounded but non-emergency availability impact.
• merge-risk: 🚨 availability: PTY abort and deferred process cleanup can affect probe completion and background resource use under timeout conditions.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This is a maintainer-authored PR, so the external-contributor proof gate does not apply; the body nevertheless provides detailed repeated local fixture results.

Evidence

What I checked:

• Current-main gap: Current main sends ordinary early-stop cleanup through synchronous termination, whose residual-process collection includes system-wide output-holder discovery; this is the latency path the PR replaces. (Sources/CodexBarCore/Host/PTY/TTYCommandRunner.swift:680, 208016687098)
• Final bounded implementation: The final head reserves a CLOEXEC PTY-master duplicate before spawning, aborts the owned root first, then dispatches holder cleanup without making the caller wait beyond the short completion wait. (Sources/CodexBarCore/Host/Process/SpawnedProcessGroup.swift:1037, b01946c9d370)
• Regression coverage: The final head adds deterministic fixtures for double-fork/session escape, TERM-created holders, post-snapshot descendants, and root exit during early-stop settle. (Tests/CodexBarTests/KiroStatusProbeTTYHardStopTests.swift:13, b01946c9d370)
• Feature history: The related normal-exit PTY drain change was merged as commit 171c2dc, and the final PR sequence culminates in the reconstructed head after targeted fixes for reservation, post-abort discovery, and settle exit. (Sources/CodexBarCore/Host/Process/SpawnedProcessGroup.swift:1039, b01946c9d370)
• Release status: No checked-out release tag contains the PR head; this remains unshipped proposed work rather than behavior already implemented on main. (b01946c9d370)

Likely related people:

• steipete: The PTY cleanup history, the related merged drain work, and the full reconstructed PR sequence are authored by steipete. (role: feature owner; confidence: high; commits: 171c2dce44d1, b01946c9d370; files: Sources/CodexBarCore/Host/PTY/TTYCommandRunner.swift, Sources/CodexBarCore/Host/Process/SpawnedProcessGroup.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Decide whether the lease-disarmed but uncancellable utility scan is acceptable before merge.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (8 earlier review cycles)

• reviewed 2026-08-09T10:28:56.803Z sha 5778321 :: found issues before merge. :: [P2] Retain cleanup for escaped PTY holders
• reviewed 2026-08-09T11:12:54.354Z sha 5778321 :: needs changes before merge. :: [P2] Retain cleanup for escaped PTY holders
• reviewed 2026-08-09T12:42:25.129Z sha 5778321 :: found issues before merge. :: [P2] Retain cleanup for escaped PTY holders
• reviewed 2026-08-09T14:16:05.296Z sha 5778321 :: found issues before merge. :: [P2] Retain cleanup for escaped PTY holders
• reviewed 2026-08-12T15:07:07.908Z sha 5778321 :: needs changes before merge. :: [P2] Retain cleanup for escaped PTY holders
• reviewed 2026-08-13T05:03:18.195Z sha 33034b4 :: needs maintainer review before merge. :: none
• reviewed 2026-08-13T05:30:26.128Z sha da3b7a4 :: needs maintainer review before merge. :: none
• reviewed 2026-08-13T05:33:21.713Z sha da3b7a4 :: needs maintainer review before merge. :: none

[steipete]

@clawsweeper re-review

The branch is reconstructed on current main at 33034b46daf4c924965859c50d13cd4c2ee5fbac. The prior escaped-holder finding is addressed with a bounded PTY lease and a convergent holder-cleanup loop. The regression now covers a true double-fork/session escape, a TERM-spawned holder, and a third-generation holder created after the first kill snapshot.

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

[steipete]

@clawsweeper re-review

Final head is da3b7a45d52328995f988773ead3b705770d0ba0. The first CI attempt exposed a timing-fixture issue: the 2.5-second synthetic scan woke before the three-second assertion and competed on a loaded runner. The injected discovery now starts at four seconds, beyond the unchanged <3s caller deadline. Five local runs returned in 1.346–1.491 seconds and still removed all three escaped holder generations.

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Complete
• Detail: A newer review tuple already exists; this stale result was superseded.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/31670452897
• Updated: 2026-08-13T05:29:06.742Z

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

[steipete]

@clawsweeper re-review

Final reconstructed head is b01946c9d37097f55c40a967f934b3f8f5508196. The prior review findings are covered by deterministic tests: post-abort discovery catches a holder created by the root TERM handler; a DEBUG snapshot barrier forces a third-generation holder to appear after the first kill snapshot; early-stop settle exit remains on bounded cleanup; descriptor reservation happens before spawn.

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.