Repository navigation
🐛 Prevent passive supervision cleanup from hiding test failures - #1094
Conversation
PR #1094: 🐛 Prevent passive supervision cleanup from hiding test failures1 files, +235 / -131 Scope✅ PR scope looks good. Structural✅ No structural bloat detected. Slop✅ Slop indicators look low. Static Analysis✅ Oxlint found no issues. CorrectnessNo extraneous code patterns detected. |
taras
left a comment
There was a problem hiding this comment.
Independent Fable review: PASS at a3bdd03ecfeea9f604f0adf3538ad760007345cd.
All recorded clauses were reviewed:
- C1: The synchronous finalizer breaks the teardown dependency cycle; every original supervision assertion is preserved.
- C2: The deterministic real-child failure distinguishes the original ordering from the repair, preserves the initiating error, and proves joined entry cleanup.
- C3: Focused Node/Bun and the Deno suite pass; ordinary unfiltered Node passes 52/52. Deadlines and runtime coverage are preserved.
No blocking findings. Nonblocking observations:
-
The local baseline stall (baseline-node-permitted.log: two children, no progress, killed at 30 s) is consistent with an early failure hidden by the teardown deadlock, but its initiating event was never identified and the run immediately followed a sandbox EPERM run, so baseline-vs-ordering-node is not a controlled pair. The controlled discriminator is the original-order-regression patch versus the finalizer, which is sufficient for the accepted scope. Record that the historical Linux CI initiating event remains unknown; under the fix a recurrence will surface as a visible assertion failure instead of a stall, which is the settled contract.
-
Regression rescue hard-codes sleep(5000), duplicating deadline()'s default. Acceptable since it must yield a boolean rather than throw, but a named constant shared with deadline() would keep the two from drifting.
-
In the regression, the spawned prompt task is halted by scope destruction rather than awaited; the held entry still runs to completion because releaseBody is resolved first, and cleaned===true proves the entry cleanup joined. No change needed, noted for readers interpreting 'all owned work joined'.
Disposition: the PR description already preserves the historical-trigger limitation. The shared-timeout-constant suggestion is optional and deferred; the cleanup observation requires no change.
Workflow reconciliation: Fable's original summary is 1394 characters against a 1200-character presentation limit. An invocation-local length-only exception accepts that original summary. The raw response, actual joined execution, verdict, findings, clauses, reviewer and commit are unchanged; no second review or formatting call ran. Required CI and exact-current-main health remain delivery gates.
An early supervisor failure can leave the passive-supervision test joining an entry whose cleanup waits on barriers that teardown has not released. The failure then produces no assertion verdict and stalls the CI shard. Fixes #1091.
The held-entry fixture now releases those barriers synchronously before scope destruction joins its owned work. The completed/failed cases share that fixture with a deterministic missing-supervisor regression, which checks that the initiating error returns only after entry cleanup finishes. All existing observation, progress, request-count, canonical-result and cleanup assertions remain. The change is confined to the test file.
Validation at signed feedback commit
a3bdd03ecfeea9f604f0adf3538ad760007345cd:ensureapproach also fails.The original externally terminated baseline and every first failure are retained as non-pass evidence. Two Deno filters selected zero tests and supply no evidence; the enclosing-suite run above supplies the Deno result. Local execution used macOS Node 26.5.1, Deno 2.9.7 and Bun 1.3.14. The exact initiating event in historical Linux CI remains unknown; the cleanup deadlock is reproduced and corrected independently.
Planner acceptance and independent Fable review both pass the exact signed feedback head above. Fable's three observations are nonblocking: preserve the historical-trigger qualification, optionally share the rescue timeout constant, and clarify scope-owned prompt teardown. The qualification is recorded here; the optional cleanup is deferred and no source correction is required.
Required CI2396 passes the same head. Current-main CI2394 passes exact main
98cc6f173a36e4bfb532c9f6067f1ed284989e49. Native delivery validation reports all three verification gates passed. The PR is ready for review and awaits authorized merge.The completed Fable reply exceeded the workflow's 1200-character summary limit (1394 characters). Its original reply and joined execution are preserved. A documented invocation-local length-only exception accepts that summary with the verdict, findings, clauses, reviewer, source and other gates unchanged. No additional model call or source correction ran.