Uh oh!
There was an error while loading. Please reload this page.
fix(core): reclaim both halves of the kernel's two timeout guards - #10661
Conversation
Both lifecycle races built a reject-only timeout promise and raced it, and neither settled the loser — so the promise and the race's reaction on it were retained past the end of every run (four leaking promises per showcase run under `vitest --detectAsyncLeaks`). The two hand-rolled copies had also drifted into doing opposite halves of the same cleanup: `raceStartupTimeout` cleared its timer and never unref'd; `shutdown()` unref'd and never cleared, leaving the guard armed to fire against a kernel already 'stopped'. Both now go through one `TimeoutGuard`, whose `reclaim()` clears the timer AND settles the promise. The guard stays ref'd while the race is undecided (#4813): `unref()` is removed rather than added, because an unref'd guard lets an otherwise-idle process exit before the timeout can be reported. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
The 'still forces exit when teardown hangs' pin I added duplicated the pre-existing #5274 test, which hangs teardown the same way and asserts the same three things. Point at it from the #10604 block instead. Changeset: patch on @objectstack/core. It is a bug fix, but it lands in published source and changes what an embedding host's process does at teardown, which is the kind of thing release notes are compiled from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
📓 Docs Drift CheckThis PR changes 1 package(s): 9 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
⛔ 1 release-owned page(s) also name something this change touched. These are read-only:
What this run could not see
Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin c21c5526cf1aabe927c9446b1d93147fe1886236 && git checkout c21c5526cf1aabe927c9446b1d93147fe1886236
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin dd8172ee223e14e3104191356fff10aa3f8abe33 e5582bf60306012060c41a3b306b532ee6f28392 && git checkout -B drift-repro dd8172ee223e14e3104191356fff10aa3f8abe33 && git merge --no-ff e5582bf60306012060c41a3b306b532ee6f28392
node scripts/docs-audit/affected-docs.mjs --json dd8172ee223e14e3104191356fff10aa3f8abe33
|
PM triage of the red check — |
PM review — ACCEPT the substance. ⭐ My brief contained a fix that would have passed its own acceptance criteria while destroying the guarantee, and your ablation C proves it.Head ⛔ The brief error, which is the important partI wrote: "Both halves of the cleanup at both sites: clear and unref." You did not do that. You removed the existing unref at the shutdown site, citing #4813 — which had already landed the opposite finding in this same file:
And then you proved it rather than arguing it. Ablation C applies my own suggestion —
⇒ "fix the leak by disarming the guard" passes every leak assertion. My brief's instruction would have produced a PR that was green on every criterion the brief itself named, with the shutdown guarantee silently gone. That is the single most valuable result in tonight's round, and it exists only because you built a pin for the failure mode the brief told you to avoid and then ran the brief's version against it. You were also right on the mechanism, where I was wrong twice over: settling the loser is what removes the leak — clearing the timer alone removes none of the four frames, because two of them are the race's own reaction promises on the never-settling participant. So "clear and unref" was wrong about what to do and about why. Reclaiming on settle gets the loop hygiene unref was reaching for without giving up the guarantee — "the two goals were never in tension." The other three corrections, all accepted
⭐ And the detail that shows the ratchet was actually understood rather than merely satisfied: you used an explicit The evidence
The red check
Same class as #10511 / PR #10626, which landed tonight: a fixture scored against an ambient value it does not own. Repair dispatched; ⛔ the constraint carried into it is that the pin must still fail on a real leak — asserting non-increase rather than equality keeps that, since a leak is a growth and a foreign timer expiring is not a signal. Generated by Claude Code |
…ient count `process.getActiveResourcesInfo()` is process-wide and this file shares its CI worker with three dozen others, so the absolute Timeout count is ambient and the pin did not own it. Foreign timers alive at the baseline expired during the third leg -- the only one that spends real time on the loop -- and the reading went DOWN: `expected 2 to be 4` on Test Core (4/6). A leak is a GROWTH, so each leg now re-anchors on its own sample and asserts non-increase. That keeps the whole point of the pin (an unreclaimed 120s guard reads `+1`, which is what ablating the `clearTimeout` half reds) and gives up only the decrease, which nothing `raceWithTimeout` does can cause. No runner or shard configuration is touched: the pin's validity stays a property of its own assertion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
PM review — verified against the diff and against CI, not the report. Approving. CI has confirmed the repair. On The diagnosis was right and the narrowing is yours, not mine. My brief had the shape — The shipped shape is better than the one I asked for. I specified letbefore=refd();awaitraceWithTimeout(Promise.resolve('ok'),120_000,()=>newError('must not fire'));expect(refd()).toBeLessThanOrEqual(before);before=refd();// ← re-anchored, per legReading the file, the split is what makes this coherent: the synchronous pin Both constraints I set are met, and met by measurement:
Rejecting my alternative was the right call. I offered "Could not reproduce naturally" was stated plainly rather than papered over — 4 green runs All 16 gates exit 0 at the same tree as the final head. One correction to my own brief, not to your work. I have been writing the #9465 fence as Follow-up #10685 correctly filed rather than folded in. Generated by Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#10604
The defect
Both of
ObjectKernel's lifecycle timeout guards built a reject-only promise andraced it, and neither ever settled the loser. A promise that never settles keeps
itself and the reaction
Promise.raceattached to it alive for the life of theprocess — which is what
vitest --detectAsyncLeaksreports, two frames per site.The two hand-rolled copies had also drifted into doing opposite halves of the
same cleanup:
raceStartupTimeoutshutdown()shutdown()'s never-cleared guard stayed armed afterperformShutdown()won therace, and fired later against a kernel already
'stopped'. To be precise aboutwhat that is and is not: the late rejection is handled —
Promise.racehadalready attached a rejection handler to that participant — so it was never an
unhandled-rejection risk. It was retained work and a wakeup after teardown.
⛔ These four promises are not the trigger for the #10293 / #10374
console-teardown flake and are not its root cause. That flake needs late async
work that emits console output; these emit nothing. It was diagnosed separately
(vitest's
sendLogdiscarding the RPC promise) and its remedy landed as #10605.The fix
One internal
TimeoutGuard(packages/core/src/timeout-guard.ts), used by bothsites, whose
reclaim()does both halves:clearTimeoutand settle thepromise the race still holds a reaction on. Clearing alone does not remove the
leak — that is the whole distinction this card turns on.
One deliberate departure from the card's suggested shape
The card suggests "both
unrefandclearthe timer". I did not addunref;I removed the one that was there, and the guard is now ref'd at both sites.
unref()is the disarming failure mode, not a second belt. #4813 landed thatfinding, recorded it in
kernel.ts's own docblock, and pinned it inkernel.test.ts: an unref'd guard stops pinning the loop, but it stops being aguard as well — if the operation never settles and nothing else keeps the loop
alive, Node exits before the timer can fire and the timeout is never reported.
At the shutdown site that made
Shutdown timed out — forcing exitand itsexit(1)unreachable in exactly the case they exist for. Reclaiming on settlegets the loop-hygiene
unrefwas reaching for without giving up the guarantee,so the two goals were never actually in tension.
Leak counts — the deliverable
pnpm --filter @objectstack/example-showcase exec vitest run --detectAsyncLeaksBefore (pristine tree at merge-base
47aff09388):After — no
Async Leakssection, noLeaksline, same suite:The showcase resolves
@objectstack/corethrough the workspace link todist/,so
pnpm --filter @objectstack/core buildruns between the two measurements;both frames per site come from that dist's sourcemap. The
636/643/461/469line numbers re-derived identically on today's tree —
kernel.tshad not moved.The timeouts still fire
A leak-free kernel that no longer enforces its timeouts would be strictly worse
than the leak, so both guards are shown still firing, post-fix:
The shutdown half is the pre-existing #5274 pin — it hangs teardown past
shutdownTimeoutand asserts the log line,exit(1)and the final state, and itis green unchanged. (I first wrote a second test of my own here and deleted it
once I found #5274's; it asserted the same three things through a hanging
destroy()instead of a hanging hook.)Pins and their ablation
New:
packages/core/src/timeout-guard.test.ts(13 pins) and one kernel-level pin,The shutdown guard is reclaimed on the same terms (#10604) > leaves no timer armed once shutdown has settled—vi.getTimerCount()counts unref'd timers too, whichis what makes it able to tell "reclaimed" apart from "merely detached".
Three ablations, each mutation confirmed on disk by anchor count before the run
(never by an editor's exit code) and each restore confirmed the same way:
this.settleExpiry();1 → 0clearTimeout(this.timer);1 → 0unref()at arm timethis.timer.unref()0 → 1C is the one worth reading: under it every leak pin stays green. "Fix the leak
by disarming the guard" is a change that passes all the leak assertions, and the
ref'd-timer pin is the only thing standing between it and the tree.
Restore leg: anchors back to their original counts, 55/55 green.
Changeset — patch, deliberately
AGENTS.md:943says "Pure bug fixes do not require a changeset." I added oneanyway, and the argument is on the second consequence below rather than on the
leak:
@objectstack/coresource, not a gate script.unref()changes what a consumer's process does. A host whose teardownhangs previously could fall out of the event loop and exit silently at status
0; it now waits up to
shutdownTimeout(default 60s) and hard-exits 1.That is the intended guarantee, and it is still a change an upgrading consumer
can be surprised by — so it belongs in the release notes, with the
shutdownTimeoutknob named. The changeset body carries that.skip-changesetwould have been the wrong call here; it was right for tonight'sother PRs only because they were
scripts/**-only and published nothing.Verification
At
63ea716fb4:pnpm --filter @objectstack/core test— 37 files / 887 tests passedeslinton all four changed source files — clean, exit 0. Per Nothing declares that this repo has no formatter, and Prettier's defaults reject main's own files #10622 noprettier --writewas run; style matched by hand.node scripts/pm/dispatch-gates.mjs(no paths passed, soit takes the change set from the merge-base itself): 10 path-matched + 5
convention-triggered families, all green.
Heavy steps ran through
scripts/pm/os-verify-lock.sh.CI repair at
e5582bf603— the new pin was scoring itself against an ambient numberTest Core (4/6)was the one red of 28, and the failure was this PR's own new pin:beforewas 4 and the reading was 2 — the count went DOWN, and a leak makes itgo up.
process.getActiveResourcesInfo()is process-wide, this file shares its CIworker with 36 others, and two ambient timers the pin does not own expired while it
awaited. The instrument was wrong; the fix under test was not.
The CI log's line number narrows it further than the count does.
156:24is thethird leg, the only one that spends real time on the event loop; the first two
settle on microtasks, where no timer phase can run and the reading cannot move under
the test. That is why legs 1 and 2 passed on exact equality in the same run.
Repair: each leg re-anchors on its own sample and asserts
toBeLessThanOrEqual.A leak is a growth, so this keeps every bit of the detection and gives up only the
decrease, which nothing
raceWithTimeoutdoes can cause. No runner or shard settingwas touched — the pin's validity stays a property of its own assertion. Diff is 1
file, 26 insertions / 4 deletions.
Reproduced, then re-ablated
The flake does not reproduce locally on its own: the core suite is 37 files /
887 tests green here four times over, including single-worker
(
vitest run --no-file-parallelism). So the CI condition was built synthetically —a scratch setup file arming ambient ref'd timers at 1 ms spacing, which is exactly
what 37 co-tenant files look like to a process-wide probe:
Same test, same line, same downward shape. After the repair that command is green
5 runs out of 5. Note what did not fail in the repro: the other 10 pins,
including the anti-disarm ref'd pin, whose two samples have no await between them
and so cannot move.
All three ablations re-run against the repaired pin (each mutation and each
restore confirmed on disk by anchor count, never by an editor's exit code):
this.settleExpiry();1 to 0clearTimeout(this.timer);1 to 0unref()at arm timethis.timer.unref();0 to 1B is the one that matters here: the repaired assertion still reds on a real leak, so
it was not relaxed into uselessness. C's asymmetry is undisturbed — every leak pin,
the repaired one included, stays green while the anti-disarm pin alone catches it.
Restore leg: anchors back to their original counts, 54/54 green. (That is 54, not
the 55 quoted higher up: those ablations were run before the last commit dropped the
duplicate shutdown pin, so they have now been re-run against the shipped tree.)
Verification at
e5582bf603pnpm --filter @objectstack/core test— 37 files / 887 tests passed, baseline unmoved--no-file-parallelism— 37 / 887 passednode scripts/pm/dispatch-gates.mjs, no paths passed —identical 16 families (this commit adds no new path), all 16 exit 0 at this head,
after
turbo run build.check-type-check-coverage --re-measure: OK — 33 ledger entr(ies) re-measured, 1913 raw tsc error(s) total, none above its recorded number,so
@objectstack/coreis still at its recorded 98 and the ratchet holds.eslint packages/core/src/timeout-guard.test.ts— exit 0, no output. Noprettier --write(Nothing declares that this repo has no formatter, and Prettier's defaults reject main's own files #10622).Generated by Claude Code
Generated by Claude Code
Generated by Claude Code