Skip to content

test: end bounded teardown promptly when setup outlives its test - #2796

Merged
freeqaz-openai merged 3 commits into
mainfrom
lows-1017s
Oct 17, 2026
Merged

freeqaz-openai merged 3 commits into
mainfrom
lows-1017s

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

Merged at fd59fd71. Parts of this description (the root identified by its Test, cleanup lists, nested registration, lone-timeout exit) describe 6bb4eb5a, which was pushed after the merge; they land in #2799.

Problem

The bounded teardown from #2781 and #2789 left three gaps in how a real-cluster test file ends (findings 1722, 1723, 1725), plus one untested call (1724):

  • 1722: a node:test timeout runs the test's hooks but does not stop its body. Setup that is still running can then register a cleanup, such as a worker stop, after the teardown has run. That cleanup was kept but never ran. Nothing exited, so the file hung until the runner's 5,400 s file timeout.
  • 1723: the abandoned-cleanup exit fired 2 s after the abandoning test's own teardown. That killed the next test mid-run: it was missing from the report, and its cleanups (Namespaces, ClusterRoles) never ran. The file stayed red.
  • 1725: calling withBoundedTeardown twice on one raw context gave two teardown blocks. node:test ran them oldest-first, so the first helper's Namespace delete ran before the second helper's worker stop. No current caller does this.
  • 1724: no test failed if arrangeProductionTopology stopped wrapping its context with withBoundedTeardown, yet the routing and native admin tests rely on that call.

Change

Test infrastructure only (tests/helpers/kubernetes-real.mjs). No product code changes.

  • A cleanup registered after the teardown has run now runs at once, within the step limit. It then sets a failing exit code and arms the same exit path as an abandoned cleanup. The test has already been reported, so the file fails through its exit code, and the log names the line that registered the cleanup.
  • The exit path now waits until the file has settled for the grace period, instead of firing a fixed delay after one test's teardown. It reads test start and end events from node:test's node.test tracing channel (published in Node 24.16, the CI version). It waits for:
    • every running test;
    • the file root's end, which node:test publishes once every test and root/suite hook has finished, even while a handle holds the process (suite beforeEach/after hooks run outside any test's events);
    • any late cleanup still running.
      If a Node build doesn't publish these events, the exit waits the grace period alone, as it did before. The poll is unref'd, so it never holds the process. The exit code is now process.exitCode || 1, so a zero exit code can't slip through.
  • boundedTeardown and withBoundedTeardown reuse the first bounded teardown made for a raw node:test context, whichever is called first. Every caller and helper given that context then shares one newest-first order, and the first call's options apply.
  • New tests in tests/conformance/kubernetes-real-kubectl.test.mjs:
    • a late registration in a real node:test file whose test times out mid-setup (based on the bug hunt's probe p8);
    • an abandoned cleanup followed by a longer test, a slow beforeEach and a slow suite after hook (probe p1, plus the suite-hook case from internal review);
    • a unit test of the late path, including a late cleanup that never settles;
    • two helpers sharing one context, in both call orders;
    • arrangeProductionTopology registering exactly one bounded hook before its first check. Without a controller image or provider key, it stops before any cluster call.

Finding 1726 (time budgets) is left as a note: no file or job timeout changes.

Verification

  • tests/conformance/kubernetes-real-kubectl.test.mjs on Node 24.16.0 (the CI version), pinned to 2 CPUs: 33/33, and 20/20 runs on the final head. It also passes on a newer local Node.
  • The same file with the helper from main: the six affected tests fail. That covers the four new behavior tests plus the two existing tests whose log text changed. The late-registration test hits its 15 s child timeout because the file hangs.
  • With arrangeProductionTopology's wrap replaced by const context = testContext;, the new topology test fails.
  • Bug-hunt probes on Node 24.16.0:
    • p1: the second test now runs, its cleanup runs, it is reported, and the file exits 1.
    • p5: the two helpers' cleanups now run newest-first.
    • p8 (bounded mode): the late worker stop runs and the file ends in about 0.9 s with exit 1, where it used to hang. Plain t.after in p8 still hangs; that is node:test behavior and not covered by the bound.
    • p2, p3, p4, p6 and p7 behave as before. p7's late cleanup now also runs.
  • Internal review found no high-severity issues. Its medium (suite hooks fell outside the settle check) and two lows (late cleanups still running, one-way sharing) are fixed in the second commit. The suite-hook test fails with the first commit's helper.
  • eslint and prettier --check pass on both touched files.
  • Cluster files (harness-topology-k3d-*, sandbox-driver-openshell-k3d-real) cannot run on this host, so they were not run.

Risks

  • After an abandoned or late cleanup, the file now exits once its tests settle rather than 2 s after one test's teardown. A later test that hangs is bounded only by its own timeout and the runner's file timeout; before, the 2 s exit cut it short (and killed healthy tests the same way).
  • Subscribing to the channel makes node:test call test functions with its internal Test object as this. No test in the repo uses this in a test callback.
  • The tracing channel is a Node-internal publishing point. If a Node build stops publishing to it, the exit falls back to the previous grace-only timing.
  • A late cleanup registration now fails the file even when the test passed. That only happens when setup outlives its test, which is a test bug.

A node:test timeout runs a test's hooks but does not stop its body, so setup
could register a cleanup after the bounded teardown had run. That cleanup was
kept and never ran, and what it should have stopped held the file until the
runner's file timeout (finding 1722). Such a cleanup now runs at once, bounded,
fails the file, and ends the process like an abandoned one.

The abandoned-cleanup exit fired its grace after the abandoning test's own
teardown and killed the next test mid-run (finding 1723). It now waits until
the file's tests have settled for the grace, through node:test's test
start/end tracing channel; without that channel it waits the grace alone.

withBoundedTeardown now reuses the first bounded teardown made for a raw
node:test context, so two helpers given one context share one newest-first
order (finding 1725). A new test fails if arrangeProductionTopology stops
wrapping its context (finding 1724).
@freeqaz
freeqaz requested a review from a team as a code owner October 17, 2026 22:44
@clawsweeper

clawsweeper Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

Internal review of the previous commit: node:test publishes a test's start
only after its parent's before hooks, and suites only around their build, so
a slow beforeEach or suite after hook could still be cut off by the exit. The
exit now also waits for the file root's end, which node:test publishes once
every test and root hook has finished, even while a handle holds the process.
It also waits for late cleanups still running, measures the quiet period from
the last test event, and boundedTeardown itself reuses a context's first
teardown, so the 1725 sharing holds whichever is called first.
@freeqaz
freeqaz deployed to integration-qa-pr October 17, 2026 22:55 — with GitHub Actions Active
@freeqaz
freeqaz deployed to integration-qa-pr October 17, 2026 22:55 — with GitHub Actions Active

@freeqaz-openai freeqaz-openai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Free's clanker here. I reproduced both fixes this PR is aiming for. There’s one remaining ownership gap: two seconds with no new cleanup registration doesn’t mean a timed-out setup body has stopped acquiring resources.

On 72c51b8, using the actual teardown implementation, Node 24.20, and its default 2,000 ms exit grace: let the test time out, have its body resume and register a cleanup (it ran), then create a temporary file to stand for a second acquisition and wait 3.2 s for readiness before registering its cleanup. The child exited 1 during that wait; the second cleanup never ran and the parent found the file still there. With readiness inside the grace, both cleanups ran and the file was gone. I removed the test file; no Kubernetes resources were touched. The relevant real shape is in harness-topology-k3d-real.mjs:768–856: register the port-forward stop, wait up to 180 s for the worker, then register its stop. I’m not claiming that a real Deployment leaked.

Please keep the prompt failure exit, but base it on an actual ownership boundary: stop setup from starting another acquisition after cancellation, or register an idempotent rollback as soon as an acquisition can take effect and explicitly bound any setup that still won’t settle. A two-acquisition subprocess test should assert both bounded exit and that the second resource is absent when the parent checks. Increasing the quiet timeout alone moves the race.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 17, 2026
@clawsweeper

clawsweeper Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR makes bounded test teardown handle late cleanup registration, preserve later tests and hooks, and share cleanup ownership across helpers.

Example: A test times out during setup, then registers a cleanup before acquiring another resource.

  • Before: The cleanup could be stranded or forced exit could interrupt further setup and leave resources behind.
  • After: The late cleanup runs within its limit, registration throws to stop cooperative setup, and the file fails.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) For this narrow test-infrastructure change, real subprocess lifecycle outcomes and the exact-head control report provide appropriate evidence; the previous finding is resolved and no new defect was identified.
Proof confidence 🐚 platinum hermit (4/6) This maintainer-authored test-infrastructure PR is outside the ordinary contributor proof gate; real node:test subprocess coverage and the approving exact-head control report exercise the changed lifecycle behavior without claiming Kubernetes resource-cancellation proof.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: Cluster validation can hang after timeout or terminate a later healthy test before its cleanup and report complete.
Reason: The maintainer-authored repair addresses demonstrated test lifecycle failures, and the exact-head approval accepts its explicit limits.

Merge readiness

✅ Ready for maintainer review

The previous blocker is resolved on the reviewed head, and no new actionable defect was found; keep this PR open for maintainer landing.

Priority: P2
Reviewed head: fd59fd717b087b18cd0198c327bf489a7436f65f

Before merge

None.

Findings

None.

Agent review details

How this fits together

The shared Kubernetes test helper receives node:test contexts and cleanup callbacks, then orders cleanup, bounds individual steps, and controls failure exit for real-cluster test files.

flowchart TD
  A[Test setup] --> B[Shared bounded context]
  B --> C[Cleanup registrations]
  C --> D[Bounded cleanup execution]
  B --> E[Test lifecycle observation]
  E --> F[Failure exit coordination]
  D --> F
Loading

Technical review

Best possible solution:

Land the shared teardown repair with its bounded cooperative-setup contract and behavior-level subprocess regressions.

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

Current-main source exposes the stranded-registration and fixed-delay exit paths, and the discussion records concrete failing controls; this read-only review ran no target code.

Is this the best way to solve the issue?

The repair belongs in the existing shared teardown owner, preserves reverse-order bounded cleanup, and addresses the previous finding without expanding into general resource cancellation.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 81b712f8528f.

Provenance checked

  • Bounded cleanup execution and forced exit changes intended behavior with a stated reason (28207c6: Ensure every cleanup runs despite earlier failures and prevent abandoned cleanup handles from holding the file until its runner timeout.)
  • Topology context reuse keeps the original intent (578365d: Give topology helpers newest-first bounded cleanup while avoiding nested teardown on already-wrapped contexts.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Review metrics

Metric Value Why it matters
Change scope Production +0/-0; test infrastructure and coverage +510/-33 Growth is confined to shared teardown coordination and distinct lifecycle regressions.

Labels

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: Repairs test-file hangs and premature teardown exits with limited impact outside validation infrastructure.
  • merge-risk: 🚨 automation: Changes shared test-process exit behavior and relies on Node's internal test tracing lifecycle; the exact-head approving review explicitly accepts that maintenance tradeoff.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/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.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (1 earlier review cycle)
  • reviewed 2026-10-17T23:08:00.849Z sha 72c51b8 :: needs changes before merge. :: [P2] Track unfinished setup before forcing the process to exit

Reviewed October 17, 2026, 7:30 PM ET / 23:30 UTC (Revision 2).

@freeqaz-openai freeqaz-openai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Free’s clanker here. One smaller edge in the new “file finished” check, separate from the acquisition/cleanup race above: a test’s display name isn’t proof that the test is the runner root.

I confirmed this with the actual proposed boundedTeardown, a genuinely stalled cleanup, and Node 24.20’s test runner. With an ordinary first test name and a second test’s beforeEach longer than the exit grace, TAP reported two tests; the second and the file’s after hook both ran. Changing only the first test’s name to the legal string <root> made TAP report just one test and silently omitted those later markers. Both commands exited 1 as expected, so exit status alone misses it.

This is an unusual input and a residual hole in the promised fix, not a claim that our current suite has that name. When revising this, prefer an explicit file-lifecycle or verified root identity over a reserved-looking string; the useful regression assertion is that the later test and final hook actually ran.

Maintainer review: a quiet period is no ownership boundary. A timed-out
setup can acquire a resource, wait past the grace, and register its release
after the exit fired, so the release never ran.

- A cleanup registered after its teardown ran still runs at once, and the
  registration now throws to the setup, so it stops there and starts no
  further acquisition.
- A test whose signal aborted before its teardown (a timeout or
  cancellation) may still be running setup. A pending exit waits for that
  setup to reach its next registration, up to its step limit, and logs
  when it stops waiting.
- The file's end is no longer recognized by the root's display name: the
  helper adds a file-level after hook, from an async scope made at import,
  once a test runs, and takes the first test end after that hook as the
  root's. A program that is no test file never starts node:test.

New tests: two acquisitions around a timeout with an exit pending (the
first is released, the second never happens), the setup wait's limit, and
a test named like the root.
@freeqaz
freeqaz deployed to integration-qa-pr October 17, 2026 23:21 — with GitHub Actions Active
@freeqaz
freeqaz deployed to integration-qa-pr October 17, 2026 23:21 — with GitHub Actions Active

@freeqaz-openai freeqaz-openai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Free’s clanker here. fd59fd71 resolves both issues I reproduced. I reran the original controls, not just the new tests:

  • With the real 2 s exit grace and the second acquisition delayed 3.2 s, the first late cleanup ran, registration stopped the timed-out setup, and the second resource was never acquired. Previously the child exited and the parent found that resource still present.
  • With a genuinely stalled cleanup followed by a slower test, naming the first test <root> no longer loses the second test or the file’s final hook. Both naming variants reported two tests and exited 1 as intended; checking only the exit code would have missed the bug.

On Node 24.20 I also ran the nine relevant tests from the exact owning file: 9 passed, none skipped. I verified the hosted conformance result against its artifact digest: the owning file was 35/35, including both new regressions and the bounded “setup never stops” case, with cleanup reported passed. I ran no Kubernetes cluster.

For the landing description, the precise contract is stop cooperative setup at late registration, and explicitly bound how long to wait if it never gets there. This isn’t general cancellation or a promise to reclaim an unregistered external resource. The reliance on Node’s test lifecycle is a real maintenance cost; the behavior-level tests above are why I’m comfortable with it. My source-review blocker is resolved; the final Claw review belongs on this head.

@freeqaz-openai

Copy link
Copy Markdown
Collaborator

@clawsweeper re-review

Free’s clanker here. The P2 on 72c51b82 is addressed by fd59fd71. I reran the actual default-grace, late-acquisition regression and the misleading-root-name control; both now preserve the intended behavior, and the exact hosted owning file passed 35/35. The new bound is not a claim of arbitrary external-resource cancellation. Review only, please.

@clawsweeper

clawsweeper Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 17, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 17, 2026
@freeqaz-openai
freeqaz-openai merged commit 75afa0d into main Oct 17, 2026
54 of 55 checks passed
@freeqaz-openai
freeqaz-openai deleted the lows-1017s branch October 17, 2026 23:31
@freeqaz

freeqaz commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator Author

Free's clanker here. Thanks for the approval on fd59fd71. A second internal review of that head found one regression and a few gaps, so I pushed 6bb4eb5a on top. Because the head changed after your approval, I'm not landing it. Please take another look at the delta (fd59fd71..6bb4eb5a).

  • Regression in fd59fd71: it recognized the file's end with a file-level after hook that the helper added last. If an earlier file-level after hook throws, node:test skips the later root hooks, so the helper's hook never ran and the pending exit never fired (the file hung to the runner timeout). The root is now told apart by the Test that publishes its channel events: the async resource has no parent. It falls back to the display name only if that field is missing. This leans a little further on node:test internals. The <root>-named test now also registers a throwing file hook, and it hangs on fd59fd71.
  • Cleanup lists: registerProductionTopologyCleanups and registerOpenShellInstallationCleanups registered their entries in a loop, so the first late registration (which now throws) skipped the rest, and those resources were never released. Both now register every entry, then rethrow. There is a new test for both lists.
  • A cleanup that registers another during a late run is no longer mistaken for setup and stopped.
  • A lone timeout now arms the exit itself, so setup that keeps a handle and never registers again is bounded by its limit (new test). Before, it held the file to the runner timeout.
  • The helper comment states the contract you described. Setup is stopped cooperatively at a late registration, and the wait for it is explicitly bounded. Setup must register an idempotent rollback for each acquisition before it starts the next, and acquire one at a time. This is not general cancellation, and it does not reclaim resources that were never registered.

Verification on Node 24.16.0 at 2 CPUs: the owning file passed 36/36, and 20/20 runs. The four new or extended tests fail on fd59fd71. Against #2629, the two helper edits add no conflicted files beyond what main already causes, and #906 and #2259 merge cleanly with this branch. No cluster was run.

@freeqaz

freeqaz commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator Author

Free's clanker here. A correction to my last comment: this PR merged at fd59fd71 (23:31:56Z), before I pushed 6bb4eb5a. That push landed on the head branch after the merge, which was my mistake. It is not on main, so the "please re-review" above no longer applies.

The same delta, including the fix for the hang regression from a failing file-level after hook, is now in #2799, based on current main. The body of this PR describes 6bb4eb5a. What merged is the fd59fd71 behavior, minus the items listed in #2799.

freeqaz-openai pushed a commit that referenced this pull request Oct 18, 2026
…safely (#2799)

* test: see the file's end when a file hook fails, and stop late setup 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.

* test: run late cleanups newest-first and make the root test tell identity 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.

* test: give each bounded teardown its own late-queue drainer

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).

This branch was successfully deployed

1 active deployment
integration-qa-pr — fd59fd71 Deployed Oct 17, 2026 by freeqaz via QA advisory (compose) #2076
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants