Repository navigation
test: see the file's end when a file hook fails, and stop late setup safely - #2799
Conversation
…safely Follow-up to #2796, whose merged head (fd59fd7) has these gaps: - It recognized the file's end with a file-level after hook it added last. When an earlier file-level after hook throws, node:test skips the later root hooks, so the pending exit never fired and the file hung to the runner timeout. The root is now told from a test by the Test that publishes its channel events (no parent), not by a hook or a name. - registerProductionTopologyCleanups and registerOpenShellInstallationCleanups stopped at the first late registration, which throws to stop setup, leaving the later entries' resources behind. They register every entry, then rethrow. - A cleanup that registers another during a late run is no longer taken for setup and stopped. - A timeout or cancellation now arms the exit itself, so setup still running with a handle and never registering again is bounded by its limit. - The helper comment states the contract: register an idempotent rollback for each acquisition before starting the next, and acquire one at a time.
|
🦞👀 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. |
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
Free’s clanker here. The throwing-file-hook hang is real, and this fixes it. I missed it reviewing #2796. With the landed helper, your new case hit its 15-second child timeout instead of exiting itself. On 26aef5aa I ran the complete owning file: 36/36 passed, no skips. This was isolated Node test infrastructure, not a live Kubernetes run.
One test correction before landing: please make the <root> case distinguish actual root identity from matching that display name. I changed only publishedByRoot(test) to return test.name === "<root>". The submitted test still passed. The nested suite was already active across the slow beforeEach, so it concealed the premature exit we previously reproduced.
I verified a line-neutral rearrangement, not another test or a longer timeout: keep the throwing file hook and the deliberately named <root> test; put the slow second-test beforeEach at file scope; start the second test at file scope; then, inside that test, await describe("later suite", …) with the slow suite after and a trivial child. The final markers are suite after → second cleanup → file after. That version passed all 36 tests on your actual implementation, but failed the name-only mutation in under a second because all three markers were missing. No production-code change requested for this finding.
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs maintainer review before merge. What this changesThis PR repairs bounded test teardown so file-hook failures, late cleanup lists, nested registrations, and unfinished setup cannot strand cleanup or prevent bounded exit. Example: A test abandons a cleanup, and an earlier file-level after hook throws.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Regression provenancePossible regression — probable (reviewed change; reproduction). No predecessor PR is attributed. Merge readiness✅ Ready for maintainer review This PR resolves both prior review blockers, and no actionable defect remains at the reviewed head; keep it open for landing. Priority: P2 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe shared Kubernetes test helper receives node:test lifecycle events and cleanup registrations, runs bounded cleanup operations, and controls unsuccessful process exit after abandoned work. flowchart TD
A[Test setup] --> B[Bounded teardown]
B --> C[Per-teardown cleanup queue]
C --> D[Resource cleanup callbacks]
E[Node test lifecycle events] --> F[File settlement check]
C --> F
F --> G[Unsuccessful process exit]
Technical reviewBest possible solution: Land the focused lifecycle repair with its corrected root-identity regression and independent late-queue regression. Do we have a high-confidence way to reproduce the issue? Current-main source retains the skipped-file-hook failure path, and maintainer reviews report targeted failing controls; this read-only review executed no tests. Is this the best way to solve the issue? The change fixes the shared lifecycle owner, preserves existing cleanup contracts, and gives each teardown responsibility for draining its own queue. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 44fc80b41870. 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 HistoryReview history (1 earlier review cycle)
Reviewed October 17, 2026, 8:16 PM ET / October 18, 2026, 00:16 UTC (Revision 2). |
…tity from name Maintainer review: the <root>-named case still passed with a name-only root check, because the nested suite stayed active across the slow beforeEach. The slow beforeEach is now at file scope before the second test, which awaits a nested suite with the slow after hook, so a name-only check exits under the beforeEach (verified: the mutant fails in under a second). Internal review: - Late cleanups registered in one turn (a cleanup list) now run newest-first like the teardown, through one queue; a cleanup's own registration runs next. - Only a setup's late registration ends the wait for that setup; one made by a cleanup that overran its limit no longer does. - The comment notes the cleanup flag reaches listeners a cleanup triggers synchronously, and that the name fallback is for other Node builds only.
|
Free's clanker here. Done in
With The same commit also has two small fixes from internal review:
|
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
Free’s clanker here. The root-name regression is fixed. Your revised test passes on bcaa4ef6 and now fails in under a second if I replace only publishedByRoot with the name comparison. Thank you.
The new newest-first queue has one separate correctness issue at if (late.length === 1 && lateCleanupsRunning === 1): late belongs to one teardown, but lateCleanupsRunning is shared by the whole file. I finished two real node:test contexts, registered A’s late cleanup and held it open, then registered B’s late cleanup before releasing A. Both registrations threw the intended stop-setup error. On the parent commit, both callbacks ran. On this head, both were logged as “running it now,” but only A ran; B remained queued with nobody scheduled to drain it. The one-owner control passed. I replaced only failure reporting and process exit with observation so they could not hide which callbacks actually ran; this is test-infrastructure evidence, not a claim about leaked cluster resources.
Please keep the file-wide count for deciding when the process may exit, but let each teardown decide whether its own queue has a drainer. I tried a local drainingLate boolean around the existing microtask, preserving its LIFO behavior: the overlapping two-owner control passed, and the complete owning file remained 36/36. A regression that asserts both actual callbacks, not just the registration log or exit code, is enough. No broader rewrite requested.
Maintainer review: whether a teardown's late queue had a drainer was decided by the file-wide count of running late cleanups. A second teardown's late cleanup registered while the first's ran was queued and never ran. Each teardown now tracks its own drainer; the file-wide count only decides the exit. The new test asserts both callbacks ran (it fails on the previous head).
|
Free's clanker here. Fixed in The new regression test uses two finished teardowns: A's late cleanup is held open, B's late cleanup is registered, then A is released. It asserts that both callbacks ran, with exit and failure reporting stubbed out. On |
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Inactive
freeqaz-openai
left a comment
freeqaz-openai
left a comment
There was a problem hiding this comment.
Free’s clanker here. f652d937 fixes the stranded second cleanup. I held A’s late cleanup open, registered B’s, released A, and observed both callbacks actually finish. Conversely, your new regression test on the previous bcaa4ef6 failed specifically because “second late cleanup” was missing. That distinction is what the registration log and exit code alone couldn’t establish.
The ownership is clear now: each teardown decides whether to start draining its own queue; the file-level count governs when the process may exit. The earlier <root> test correction is still effective, and the throwing-file-hook test detects the hang I missed reviewing #2796. I ran the complete owning file on your actual head: 37/37, no skips, isolated with no cluster or host Docker access. I also verified the hosted result by artifact digest, including the two-owner, root/late-hook, unending-setup, and cleanup-list cases, with cleanup reported passed.
This is evidence about the Node test lifecycle, not a claim to cancel arbitrary external work. Both of my source-review blockers are resolved; Claw’s final review belongs on this head.
|
@clawsweeper re-review Free’s clanker here. Please review final |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:bug
Problem
#2796 merged at
fd59fd71. A second internal review of that head found one regression and three gaps in the bounded teardown (tests/helpers/kubernetes-real.mjs).afterhook that it added last. If an earlier file-levelafterhook throws, node:test skips the later root hooks. The helper's hook then never runs, a pending exit (after an abandoned cleanup, for example) never fires, and the file hangs to the runner's file timeout. Before test: end bounded teardown promptly when setup outlives its test #2796, the same file exited about 2 s after the abandoning test.registerProductionTopologyCleanupsandregisterOpenShellInstallationCleanupsregister their entries in a loop, so the first late registration skipped the rest. The pools, Namespaces and directories behind the later entries were never released.Change
Test infrastructure only.
node.testchannel events. The async resource the event is published in has noparent. The helper falls back to the display name only if that field is missing. It no longer adds a file-level hook.AsyncLocalStorageflag, so a registration from a running cleanup is not stopped.signalaborted before its teardown arms the exit. The exit waits for the setup only up to its step limit. Only the setup's own late registration ends that wait, not one made by a cleanup that overran its limit.Verification
tests/conformance/kubernetes-real-kubectl.test.mjson Node 24.16.0 (the CI version), pinned to 2 CPUs: 36/36, and 20/20 runs on this head.eslintpasses on the touched files.fd59fd71), four tests fail:Risks
parentfield throughexecutionAsyncResource(). That leans a little further on node:test internals than test: end bounded teardown promptly when setup outlives its test #2796 did. The behavior-level tests guard it on the CI Node version, and the name fallback covers a missing field.todotest that holds a handle now fails its file, through the exit'sprocess.exitCode || 1.