Repository navigation
test(ci): release the escaped cleanup descendant and add two browser duration rows - #2954
Conversation
…duration rows The escaped-group cleanup case waited out its descendant's 4 s lifetime in teardown; release it once ps has observed it alive (4.1 s -> 0.9 s locally). Add console-namespace-recheck's row and re-measure console-status-block's with the sweep's slowdown-normalised median.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs maintainer review before merge. What this changesThis PR releases a test descendant after observing it alive and refreshes two browser-test scheduling estimates. Example: A contributor runs the escaped-group cleanup integration case.
Review scores
ProductKind: Test only · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open for maintainer triage; the focused test optimization preserves the cleanup assertions, and no actionable defect was found. Priority: P3 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe CI runner consumes measured file durations to order shared test files, while the cleanup integration fixture exercises real subprocess ownership and inherited-pipe deadlines. flowchart TD
A[Duration table] --> B[CI runner]
B --> C[Ordered test files]
C --> D[Cleanup integration fixture]
D --> E[Cleanup executor]
E --> F[Command process group]
D --> G[Escaped descendant observation and release]
Technical reviewBest possible solution: Retain the fixture release after observation and the measured scheduling updates while preserving all existing cleanup deadlines and ownership checks. Do we have a high-confidence way to reproduce the issue? The base fixture leaves the escaped descendant alive and teardown waits for its four-second expiry; source establishes the unnecessary wait, while timing measurements remain contributor-reported. Is this the best way to solve the issue? The change addresses the unnecessary wait through the existing fixture heartbeat without weakening the assertions that cleanup returns promptly and leaves the escaped process alive. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against d331d24fdddc. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 19, 2026, 2:48 PM ET / 18:48 UTC. |
1 participant
Problem
The 10-19 CI flake sweep flagged two timing gaps in the test suites:
escaped-group: the whole test reached 5.34 s against the 6 stimeoutin its block (89%), unchanged across sweeps, on runners still 1.2-1.5x slow. That 6 s bounds only thespawnSynccleanup driver, which takes about 0.8 s (tree ready about 100 ms, cleanup done about 710 ms after its deadline starts). The other ~3.3 s was a fixed wait: cleanup must leave the escaped descendant running, so the fixture teardown waited out the descendant's own 4 s lifetime before it could observe it gone.file-durations.json:console-namespace-recheck(new in fix(console): the Namespace recheck survives a failed read and stops on terminal states #2944) has no row, so it starts first in its lane;console-status-block's row is 12.2 s while its CI median is 25.9 s raw (about 21 s normalised). test(console): consolidate the console browser tests added since #2903 #2947 set that row by scaling with a local before/after ratio (0.687) that did not hold on CI.Change
tests/integration/ci-cleanup.test.mjs: the descendant also exits when areleasefile exists (checked in its existing 50 ms heartbeat). Theescaped-groupcase writes that file only afterpshas observed the descendant alive, so teardown sees it end within one tick instead of waiting 4 s. The 4 s lifetime stays as the bound for a failed run, and the 6 s driver limit is unchanged (it bounds a ~0.8 s step). The other four scenarios are unchanged: their descendant dies with the owned group before the driver exits.scripts/ci/test-suites/file-durations.json:console-status-block12200 -> 20900; newconsole-namespace-recheck11900; thesourcenote says how. Method (the sweep's): per main run and lane, factor = median(file wall / row) over the lane's passed files that have a row; a file's normalised time = its wall / factor; the row = median over the main runs since the file last changed.console-status-block, 8 runs 22283f6..d331d24: normalised 19.4, 20.0, 20.8, 20.8, 20.9, 21.1, 22.2, 23.0 s -> 20.9 s (raw 21.7-37.0 s).console-namespace-recheck, 3 runs since test(console): pin the kept form's refocus, the refusal's description and the note's resize reset #2952 (7144ace, d80e9a8, d331d24): 10.7, 11.9, 13.0 s -> 11.9 s. The two runs on fix(console): the Namespace recheck survives a failed read and stops on terminal states #2944's version gave 11.1 s.Watched, no change: QA provider-delivery worst 1,746 s of 2,400 s (73%) and QA shipped startup 678 s of 1,100 s (62%).
Verification
escaped-grouplocally at 2 CPUs: 4.11 s / 4.12 s before, 0.87 s after; cleanup still completes 709 ms after its deadline starts.escaped-groupcase (each reverted):detached: false, the same as cleanup killing it): fails "escaped descendant stopped during cleanup".scripts/ci/cleanup.mjs): fails "cleanup exceeded the command and pipe deadlines" at 4011 ms, so the release cannot shorten a hang.tests/integration/ci-cleanup.test.mjsfull file 20/20 at 2 CPUs (CPUs 2-3): 30/30 tests each run,escaped-group861-885 ms, file 16.0-16.4 s.node scripts/ci/run-tests.mjs audit: no issues.Risks
Test and CI-scheduling data only. The release file is written after the liveness check, so it cannot hide a cleanup that killed or waited on the descendant. The rows only change start order in the browser lanes.
ci-cleanup's own row (23.2 s) will be about 3 s high until the next refresh; left as is rather than set from a local estimate.