Uh oh!
There was an error while loading. Please reload this page.
feat(devx): gate any Plugin implementation whose teardown the kernel never calls - #10774
Merged
Conversation
…utdown()/close()/dispose() with no destroy() Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
…ratchet-remedy control Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DdCnBGcHeufjrq7drTD3wt
os-zhuang
marked this pull request as ready for review
August 21, 2026 11:11
os-zhuang
enabled auto-merge
August 21, 2026 11:11
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#10619
Plugin(packages/core/src/types.ts) declares exactly one teardown hook —destroy?()— and both kernels call only that one:ObjectKernel.performShutdown()andLiteKernel.destroy()walk the plugins in reverse and callplugin.destroy(). Nothing anywhere callsstop(),shutdown(),close()ordispose()on a plugin. So a plugin whose teardown is spelledstop()is never torn down —await kernel.shutdown()resolves with its timers still armed.The trap is an asymmetry, which is why review does not catch it:
start?()is on the interface and does fire, so astart/stoppair reads symmetric while only one half is ever called. It survived review in two packages and was found only by merge-queue evictions (#9371: two green PRs evicted). Fixing the known instances leaves the class open — the next one arrives spelledshutdown()orclose(). This gate makes the class unreachable. Ruled FOR by thedomain:devxPM on the #9371 thread.The landing hazard, and how it was resolved
Route 2 of the card's two: land with a baseline.#10371 has not landed — verified by reading the code at
origin/main=78ac958552, not by reading the card: all of its named instances still declare an alias and nodestroy(). So the guard is landing with the pre-existing instances baselined inKNOWN_TEARDOWN_UNREACHED, ⛔ SHRINK-ONLY, each entry pointing at #10371 which owns the repair. The list refuses new members outright — the failure text turns the "just baseline it" path down rather than marking it, which is howcheck:ratchet-remedy-authorityclassifies this gate (refused, its fourth such instance). Repairing the six here was not an option: that is the services lane's work and it would have made this PR unreviewable against its card.The population was re-derived mechanically rather than adopted from #10371, and the class is larger than that card enumerates. Five instances it does not name:
MetadataPlugin,AppPlugin,ExternalValidationPlugin— spell the alias as an arrow property (stop = async (ctx) => …), which a method-only reading of the class misses entirely;EmailServicePlugin,WebhookOutboxPlugin— spell itdispose(), i.e. the "seventh arrives spelled differently" case the card predicts is already here.ExternalValidationPluginis the one that raises the stakes: besidesplugin-reportsit is the only instance owningsetIntervaltimers, and on shutdown those intervals are never cleared. Filed as #10772, unassigned, as a sub-issue of #10371 so the repair lane does not under-repair. Not fixed here.The teardown roster —
stop/shutdown/close/disposeMeasured over this tree's 54
Pluginimplementations:stopx10,disposex2,closex0 methods,shutdownx0. Two of the four have no instance today and that is the point — the class closes for the seventh instance, not the sixth.disposeis on the roster because it is more live than two of the card's three suggestions; excluding a name with two live instances while including two with none would not be defensible.Deliberately off the roster, argued in the gate header and pinned as green cases by the self-test rather than asserted in prose:
end,release,disconnect,abort,cancel,drain,flush,unbind— each names one resource, request or period, not the object's own end of life.disconnect()on a connector plausibly drops a single connection;release()releases a lock. Each has a live domain reading in this product and none has an instance here.cleanup,teardown— test-harness vocabulary; a helper namedcleanup()in a plugin is ordinary.deactivate,unload,unmount,finalize,quit,halt,terminate,kill— no instance, no precedent in this runtime, and each reads more plausibly as a domain verb.The one roster member carrying a live domain reading is
close(an approval or period is "closed"). It stays, because the shape rule below already removes the specimen in this tree that would otherwise have been the false positive.What counts as a declared teardown
A non-static class member on the roster that is a function the author wrote on the class — a method with a body, or a property initialised to an arrow/function expression. Both spellings are required: 3 of the 10
stopinstances are arrow properties, so a method-only scan reports a smaller, confident, wrong number.A property that merely holds a callback is not a declared teardown, and this is measured rather than assumed:
ConnectorMcpPluginkeepsprivate close?: () => Promise<void>— a handle taken from a bundle, which it calls from a realdestroy(). Correct code, the exact shape a name-only scan would flag, pinned green.Population is
packages/**, tests included (the kernel drives fixture plugins too, and a test process is where #9371's bill actually landed). ThePluginsymbol is not required to come from@objectstack/core:CloudConnectionPlugindeclares its own structuralPluginon purpose, runs on the same kernel and is torn down by the same loop, so an import-anchored population would exclude exactly that plugin.Verification
Positive control is the real fixture, not a synthetic class.
MessagingServicePluginas it stood onmainimmediately before PR #10375 (621a487607881c66b2899b7e3477115229a156b4, the squash merge's first parent) —async stop(), nodestroy(). It is extracted withgit showat self-test time and pinned to a commit, so nobody can quietly edit the fixture into passing; an unreachable revision refuses and prints the deepen command rather than skipping.Negative controls. The same file after the #9371 repair stays green, which is the delegating alias in real life (
stop()callingthis.destroy()); both delegating directions are pinned synthetically as well; a plugin withdestroy()and no alias stays green.Refusals, each paired with a readable tree that still returns a verdict — so "refuses unconditionally" cannot satisfy the battery:
Pluginimplementations foundp.ts, in an otherwise readable treeOne deliberate deviation from the card's wording: an unparseable source exits
EXIT_UNPARSEABLE(3) fromscripts/ts-parse.mjsrather than 1. Routing every parse through that module is whatcheck:parse-guardenforces, and it ends the process itself; it is a refusal that names the file either way, which is the property that matters. The self-test asserts non-zero and that the file is named.Ablation. Predicted first, then run. Blinding the red condition (
hasKernelHook: declared.has(KERNEL_HOOK)→true) reddens exactly the 10 of 47 cases predicted: the real pre-#10375 fixture, all four roster names, the arrow-property spelling, the structural-Plugincase, the test-source case, the paired tree that must still red, and the live-tree ratchet agreement. The mutation was confirmed on disk by anchor count (original1 -> 0, injected0 -> 1), and the restore leg confirmed the same way (1,0) with 47/47 green again.Wiring
lintjob (Lint & Repo Gates, already a required context — no new job, so no ruleset change and no aggregator roster to update). Self-test leg first, then the live check, so the leg actually executes in CI.node scripts/…rather than apnpm check:*alias: that alias is a line in rootpackage.json, inside the Migrate the release toolchain to @changesets/cli v3 — one atomic PR carrying the bump, the pre-mode restructure, and the gates that model v2's semantics #9465 fence.isEntrypoint(import.meta.url)from the start, socheck:entry-guard's shrink-only ledger gains no member.packages/**, never the bare word:One bounded edit outside the new file
scripts/check-ratchet-remedy-authority.mjsgains aCONTROLentry for this gate. That corpus is audited for set equality both ways, so a new gate the sweep reaches and the control does not cover fails asUNCLASSIFIED; the entry is the sanctioned fix (that file states its control is a declaration registry, not a ratchet, and needs nobody's leave). Verdict recorded as measured —refused— not chosen.Gates
No changeset: a new gate publishes nothing.
skip-changesetlabel applied.Derived with
node scripts/pm/dispatch-gates.mjs(no paths) and run atb97bab513d, all green:check-plugin-teardown-shape --self-test(47 cases) ·check-plugin-teardown-shape·check:entry-guard·check:parse-guard·check:ratchet-remedy-authority(+--self-test) ·check:required-contexts·check:aggregator-roster·check:workflow-status-functions·check:cross-package-test-inputs·check:node-version·check:shard-attestation·check:type-check-coverage·check:nul-bytesDeclared narrowing:
check:type-check-debt(check-type-check-coverage.mjs --re-measure) was not run. It refuses without a built dependency closure, and this diff adds no package source and changes no tsconfig, so the frozen raw-error counts cannot move. The leg of that gate which actually reads thelint.ymledited here —check-type-check-coverage.mjs— is green above. CI runs the re-measure on the built closure regardless.Generated by Claude Code