fix: time out hung capture and profile child processes - #59
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review in progressClawSweeper is reviewing this revision. This supersedes any previous blocked status. |
|
Codex review: needs real behavior proof before merge. Reviewed September 7, 2026, 3:16 PM ET / 19:16 UTC. ClawSweeper reviewWhat this changesAdds configurable capture and profiling deadlines, escalates child termination, caps profiling output, and flushes capture JSON before exiting. Merge readiness⛔ Blocked before merge - 10 items remain This PR addresses a remaining subprocess hang on main, but all five prior correctness concerns remain supported by the rebased source. The related timeout PRs cover distinct execution paths and do not replace this work. Priority: P2 Review scores
Verification
How this fits togetherPlugin Inspector executes opted-in plugin captures and profiling commands in subprocesses. Their output and exit status feed compatibility captures and performance reports. flowchart TD
A[Plugin entrypoint or profile command] --> B[Capture and profile helpers]
B --> C[Child process]
C --> D[Output collection]
B --> E[Deadline and termination]
E --> C
D --> F[Capture result or profile report]
Decision needed
Why: Previously valid long-running work will now be terminated, and the supplied discussion does not establish approval or upgrade proof for that default change. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Provide complete-output capture, bounded diagnostics, reliable process-tree cleanup, and unambiguous timeout failures while preserving slow-workload compatibility under an agreed deadline policy. Do we have a high-confidence way to reproduce the issue? Yes, source establishes that main waits indefinitely for a live capture or profile child that never exits. No target code or artifact-producing tests were executed during this read-only review. Is this the best way to solve the issue? No, the deadline approach is useful but incomplete: termination must cover owned descendants, output must finish before parsing, and expired profiles must fail regardless of graceful exit status. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 92db8c57e1d5. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (36 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review
Pushed 0c98869: |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review
Pushed 694d9ec: mock-SDK capture now SIGTERM then SIGKILL after a short grace, matching profiling. CLI capture on this head prints complete JSON ( |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Kill mock-SDK capture after 30s and classify the failure as capture-timeout so a register() that never settles cannot hang plugin-inspector capture. Profiled subprocesses now share that budget, get killed on expiry, cap stdout/stderr, and clear poll timers on every path. Flush stdout before process.exit. Escalate from SIGTERM to SIGKILL after the grace window. Replayed onto upstream/main 92db8c5. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
694d9ec to
cb9ccfe
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
What Problem This Solves
Fixes an issue where users capturing or profiling plugins could hang indefinitely, retain ordinary child processes after a timeout, or receive a successful result after cancellation or a timeout handler exited zero. Unbounded output and process sampling could also keep the inspection alive or grow its memory use.
Why This Change Was Made
Builds on @SebTardif's original timeout and JSON-flush repair. One owned-process lifecycle now handles capture, profiles, and their bounded sampler: finite deadlines, bounded output, TERM-to-KILL escalation, and completion through stdio close. POSIX cleanup targets the owned process group even after its leader exits.
Default import-loop samples and their baseline launch the existing mock capture runner directly, so the profile budget owns the actual plugin process. The runner finishes its JSON artifact before exiting; only a fresh, valid, bounded artifact from a successful, noncancelled capture is accepted. Capture artifacts retain the shipped 10 MiB limit, independently of the profile's 1 MiB stdout/stderr caps. Explicit overrides remain supported.
Custom capture commands/scripts retain their launch and artifact contracts. The public in-process capture API retains handler identity; this patch does not pretend to cancel synchronous in-process code or retained callbacks.
User Impact
Evidence
Candidate:
6499f33ff204a2705eaaf0f3d8289d12343f88c0. Contributor headcb9ccfe708e97867ac84ea842743f8efeba0f572remains an ancestor; current main98751f9333a840f8d2978975910feed397ff9f63was merged without rebasing. The only conflict was the changelog, where every entry from both sides was retained.Sanitized direct AWS proof for repair commit
e8e81a4b6d12e82dd2f460408d8af9abcd60ed8bused Node 22.19.0, a fresh PR checkout plus the exact candidate patch, no instance role, no Tailscale, no hydrated credentials, and an isolated environment:98751f9333a840f8d2978975910feed397ff9f63: no actionable P0-P2 findings. Diff whitespace and privacy checks pass.Final-head secretless GitHub CI: Check run 34369845453, bound to
6499f33ff204a2705eaaf0f3d8289d12343f88c0, passed on Node 22.23.2: 312/312 tests, zero failures/skips, package contents pass.After maintainer review, exact head
6499f33ff204a2705eaaf0f3d8289d12343f88c0was squash-merged asf5e66bb6fb3b6168186a9b9fba45ad2a18a0c5a1. The merged tree exactly matches the reviewed candidate. The approved policy is a finite 30-second default with validated finite overrides, not an unlimited zero-value mode.Windows descendant cleanup and downstream Crabpot integration/release are not claimed by the Linux proof and remain outside this explicitly bounded unit. No release is included.