Skip to content

fix(spec,core): PluginHealthMonitor stops claiming a restart it never performed; the three PluginHealthCheck restart keys retired (#12032, ADR-0049) - #12589

Merged
os-warren merged 2 commits into
mainfrom
claude/issue-12032-autorestart-never-reinits
Aug 26, 2026
Merged

fix(spec,core): PluginHealthMonitor stops claiming a restart it never performed; the three PluginHealthCheck restart keys retired (#12032, ADR-0049)#12589
os-warren merged 2 commits into
mainfrom
claude/issue-12032-autorestart-never-reinits

Conversation

@os-warren

Copy link
Copy Markdown
Collaborator

Fixes #12032

PluginHealthMonitor.attemptRestart called plugin.destroy() and stopped there. The comment above the call read "Call destroy and init to restart", and init appeared in health-monitor.ts only inside that comment. So a plugin that crossed failureThreshold with autoRestart: true got: destroy(), a log line reading Plugin restarted, status recovering, and periodic health checks that carried on running against the destroyed instance.

The reproduction — the terminal state, not the missing call

The default check when no checkMethod resolves is { name: 'plugin-loaded', status: 'passed' }, which a destroyed object passes indefinitely. Measured at ee3595cefd before anything was changed, with successThreshold: 3:

round 1 (failing): status=failed     destroyed=0 alive=true
after backoff:     status=recovering destroyed=1 alive=false
recovery round 1:  status=recovering destroyed=1 alive=false
recovery round 2:  status=recovering destroyed=1 alive=false
recovery round 3:  status=healthy    destroyed=1 alive=false

The terminal report on a torn-down, never-re-initialised plugin was healthy. #11955 made that more convincing rather than less: reaching healthy now costs successThreshold consecutive passing rounds, so the plugin has to earn a declared number of passes before it is misreported.

The route, decided by measurement

Route 2 — stop claiming a restart — taken to its coherent end: the declaration goes, and the monitor stops destroying.

ENFORCE was not available. Plugin.init(ctx) needs a PluginContext. The only two plugin.init(...) call sites in the tree are the kernel's own boot loops (kernel-base.ts:202, kernel.ts:607), both over the full plugin list, with a context that is private on ObjectKernel and protected on KernelBasecreateContext() is protected too, and there is no public accessor. A host-provided re-init hook would have had nothing to call. Positive control for that scan: the same pass resolves five real non-test plugin.destroy() call sites, so it does see lifecycle drivers. Building a per-plugin re-init API for a caller that does not exist — no runtime constructs PluginHealthMonitor (#11825) — is the speculation ADR-0049's staged decision names as the wrong default at this milestone, where "the real, shippable liability is the false promise, not the missing feature".

EXPERIMENTAL requires a roadmap. A scan of the whole docs/ planning + ADR corpus returned zero mentions of plugin auto-restart, against 118 control hits for "health" and 13 for "hot reload" in the same corpus. PluginHealthMonitor itself: 0 hits.

So: REMOVE. autoRestart, maxRestartAttempts and restartBackoff are tombstoned in @objectstack/spec 18. The latter two leave with the first rather than as a tidy-up — with no restart, "Maximum restart attempts before giving up" and "Backoff strategy for restart delays" have nothing left to be the vocabulary of, exactly the test that took distributedConfig out with the stateStrategy value it was documented as requiring (#12340).

This follows #12340 (PR #12425) and #12428 (PR #12571) one class over, with one difference worth naming: those keys had no reader. This one had a reader that acted — and what it did was not what the key declared.

Citations re-taken on the post-#12336 ref

  • packages/spec/src/kernel/plugin-lifecycle-advanced.zod.ts:73 (pre-change) — autoRestart: z.boolean().default(false).describe('Automatically restart plugin on health check failure'). Still present, text unchanged; the card cited it without a line number and it holds.
  • content/docs/references/kernel/plugin-lifecycle-advanced.mdx:114moved. The autoRestart row is at line 66 on current origin/main, and the page is now auto-generated from the zod describe strings (⚠️ AUTO-GENERATED — DO NOT EDIT). Same text, different line and different provenance.
  • content/docs/protocol/kernel/lifecycle.mdx:737 — a citation the card did not have, and the one that mattered most: the hand-written protocol page taught the false contract — "A successful auto-restart lands the plugin in recovering with both counters cleared, so a restarted plugin still owes a full successThreshold of passing rounds before it reads healthy." Corrected here.

Clause ② — accept/reject behaviour on a shipped surface: YES, on two doors

Not "only what is logged and what status is recorded". Evidence, git diff --stat against the merge base ee3595cefd:

 packages/spec/src/kernel/plugin-lifecycle-advanced.zod.ts  | 152 +++++++-
 packages/spec/authorable-surface/kernel.json               |   6 +-
 packages/spec/authorable-defaults/kernel.json              |   3 -
 packages/core/src/health-monitor.ts                        | 250 +++++++------
 15 files changed, 1257 insertions(+), 260 deletions(-)
  1. The parse. An accept-set narrowing: three keys that parsed now reject.

    -  autoRestart: z.boolean().default(false)
    +  autoRestart: retiredKey(AUTO_RESTART_RETIRED),
    -  maxRestartAttempts: z.number().int().min(0).default(3)
    +  maxRestartAttempts: retiredKey(MAX_RESTART_ATTEMPTS_RETIRED),
    -  restartBackoff: z.enum(['fixed', 'linear', 'exponential']).default('exponential')
    +  restartBackoff: retiredKey(RESTART_BACKOFF_RETIRED),
    

    The ratchets moved with it: three [RETIRED] markers in authorable-surface/kernel.json, three defaults dropped from authorable-defaults/kernel.json.

  2. registerPlugin now throws. A method that previously always returned refuses a config carrying any retired key, with an ADR-0112 envelope (code: 'VALIDATION_ERROR', status: 400), before any state is stored so a refused config leaves no half-registered plugin behind. This is the door for the audience that does not parse — which is every host there is, since nothing in the tree parses PluginHealthCheckSchema outside its own unit test.

Tombstones rather than deletions, for #12428's reason: PluginHealthCheckSchema is not .strict(), so a bare deletion would be a silent strip (#3733, ADR-0104) — a milder form of the very defect being retired.

The pin fails on the terminal state, not on the missing call

Two pins, deliberately. The first walks the sequence and checks each step; because it does, an ablation trips one of its early assertions first. So there is a second that asserts nothing in between — it drives the failure round, the former backoff window, and successThreshold consecutive passes, then reads only what an operator reads:

a plugin reported 'healthy' is a plugin that is alive

It reads the plugin's own liveness (destroy() flips it), so — unlike a pin asserting "init was called" — it cannot be satisfied by adding a no-op init.

Assertions changed, declared here, in the report, and on the line in the test files

Four #11852 tests and one #11955 test pinned the false behaviour as the contract — each drove a plugin past failureThreshold and then asserted destroyed.count === 1 and status recovering, i.e. asserted that a restart had happened. They were true readings of a false behaviour. Every one is rewritten in place with an inline [#12032] note saying what it used to assert and why that assertion was wrong:

  • restarts a plugin whose check THROWS…leaves a plugin whose check THROWS alone…
  • restarts a plugin whose check exceeds 'timeout'…leaves a plugin that exceeds 'timeout' alone… (the timeout-report half kept verbatim)
  • still restarts a plugin whose check RETURNS a failureleaves a plugin whose check RETURNS a failure alone
  • leaves a throwing plugin alone when 'autoRestart' is false → replaced by the refusal block (the control was vacuous once the key was gone)
  • requires all three successes to leave the 'recovering' a restart wrote…a 'recovering' it did not just write. Its whole point was to reach recovering from the restart path as a genuine starting state; that path is gone, so the same property is re-pinned by suspending and resuming monitoring.
  • Spec side: should validate custom health check configuration had autoRestart: true in a toEqual fixture — the strongest single pin of the false contract.

Verification

pnpm --filter @objectstack/core exec vitest run39 files, 987 tests passed.
pnpm --filter @objectstack/spec exec vitest run432 files, 11480 tests passed.
pnpm --filter @objectstack/spec run typecheck — green (check:test-typecheck: OK).
pnpm --filter @objectstack/core build — green.
pnpm lint (eslint . --no-inline-config, whole repo) — exit 0, no narrowing claimed.

Gate union derived, not recalled: node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack over the real changeset → 47 gates, re-derived unchanged after the final commit. 46/47 green at ea9a6c90c1, each exit code captured before any pipe.

The 47th, check:dev-prereqs, is not a finding: its scan half refuses on an unbuilt workspace ("The workspace is not built — 1 unmet precondition, not a list of problems", naming 34 app packages this diff never touches) and says so itself — "Nothing was measured". CI runs only the other half (lint.yml:3304, "self-test half only, never the scan"), which is green here: ✓ check:dev-prereqs --self-test — every verdict reachable … (16 cases).

Three gates first read red for the same unbuilt-prerequisite reason and were converted into real readings by building the closures rather than reported as failures:

  • check:doc-formula-expressions → exit 0
  • check:doc-security-posture✅ 26 ObjectSchema.create example(s) in 230 marked block(s) across 237 prose file(s) … carry an os validate-clean security posture
  • check:skill-examples✅ 260 prose examples type-check across 3 surface(s)

Ablation

Mutation: re-introduce the defect class — await plugin.destroy() plus healthStatus.set(…, 'recovering') on the threshold-reached branch of recordFailedRound, marker OS_ABLATION_12032_DESTROY.

No rebuild needed, justified by import form: health-monitor.test.ts imports ./health-monitor.js — a relative source import inside the same package, resolved by vitest to src/health-monitor.ts, never through the package exports map to dist/.

Proof on disk, before any result was read: INJECTED_MARKER_COUNT=1, INJECTED_DESTROY_CALL=1, REMOVED_SYNC_SIG_COUNT=0 (the sync signature really left), git diff --numstat = 14 3 packages/core/src/health-monitor.ts.

Predicted in writing first, then observed:

leg predicted observed
#1 direction RED RED
#1 count 5 6 — missed
#2 direction RED RED
#2 count 7 7
#2 failing assertion on the isolated pin state.alive the monitor reported 'healthy' for a plugin that had been destroyed: expected false to be true

The #1 miss is recorded rather than smoothed over: I wrote off requires all three successes to leave 'unhealthy' on the grounds that its failureThreshold: 2 needed more failing rounds than the test drives, but it drives exactly two before switching to pass, so round 2 does reach the threshold. Miscounted the test's own drive, not the mechanism.

Restored under trap … EXIT INT TERM, verified: git diff on health-monitor.ts empty after both legs.

Changeset

.changeset/plugin-auto-restart-never-reinitialised.md@objectstack/spec: minor, @objectstack/core: minor. A BREAKING accept-set narrowing landing after the v17.0.0 cut; the lockstep launch-window convention ships it as minor and registers the prescription under protocol major 18, exactly as #12340 and #12428 did the day before in this same module. check:changeset-no-major, check:adr-0087-registration and check:empty-changeset all green.

Not done

content/docs/releases/ untouched. No governed surface touched (docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md).

Generated by Claude Code


Generated by Claude Code

os-warren and others added 2 commits August 26, 2026 12:46
… performed; the three PluginHealthCheck restart keys retired (#12032, ADR-0049)

`attemptRestart` called `plugin.destroy()` and stopped there. The comment above
the call read "Call destroy and init to restart", and `init` appeared in
`health-monitor.ts` ONLY inside that comment. A plugin that crossed
`failureThreshold` with `autoRestart: true` got destroy, a log line reading
'Plugin restarted', status `recovering`, and periodic checks that carried on
against the destroyed instance — which the default `plugin-loaded` check passes
forever, so the terminal report on a torn-down plugin was `healthy`.

ENFORCE was unavailable: `Plugin.init(ctx)` needs a `PluginContext`, and the
only two `plugin.init(...)` call sites are the kernel's own boot loops with a
context that is private on ObjectKernel and protected on KernelBase, so a
host-provided re-init hook would have had nothing to call. EXPERIMENTAL needs a
roadmap and the `docs/` corpus has zero mentions of plugin auto-restart against
118 control hits. So the declaration goes: `autoRestart`, `maxRestartAttempts`
and `restartBackoff` are tombstoned, and the monitor no longer destroys
anything.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o
…n the contract

The richer walk-the-sequence pin trips one of its early status assertions
first, so under ablation it reports 'expected recovering to be failed' rather
than the contract. This one asserts nothing in between: it drives the failure
round, the former backoff window and successThreshold consecutive passes, then
reads what an operator reads. Under the re-introduced destroy it fails on
exactly 'the monitor reported `healthy` for a plugin that had been destroyed'.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 2 package(s): @objectstack/core, @objectstack/spec, touching 22 documentable anchor(s). ⚠️ 5 changed file(s) yielded no anchor (packages/spec/authorable-defaults/kernel.json, packages/spec/authorable-surface/kernel.json, packages/spec/src/migrations/entries/retired-keys/18.kernel__PluginHealthCheck__autoRestart.ts, …), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/protocol/kernel/lifecycle.mdx (via PluginHealthCheckSchema (symbol), PluginHealthMonitor (symbol), registerPlugin (symbol), autoRestart (literal), maxRestartAttempts (literal), restartBackoff (literal))
What this run could not see
  • 5 changed file(s) yielded no anchor (packages/spec/authorable-defaults/kernel.json, packages/spec/authorable-surface/kernel.json, packages/spec/src/migrations/entries/retired-keys/18.kernel__PluginHealthCheck__autoRestart.ts, …) — pages documenting those are invisible to this run
  • 6 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 132 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json da1126a032b175f39606fc1ddb86642d3cdad64fpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 590b84844f39752798d137cd8862ea85a90223a1 — the merge of head ea9a6c90c1785de2a0d4ae4a17bd3578fde3ac04 into base da1126a032b175f39606fc1ddb86642d3cdad64f, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 590b84844f39752798d137cd8862ea85a90223a1 && git checkout 590b84844f39752798d137cd8862ea85a90223a1
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin da1126a032b175f39606fc1ddb86642d3cdad64f ea9a6c90c1785de2a0d4ae4a17bd3578fde3ac04 && git checkout -B drift-repro da1126a032b175f39606fc1ddb86642d3cdad64f && git merge --no-ff ea9a6c90c1785de2a0d4ae4a17bd3578fde3ac04

node scripts/docs-audit/affected-docs.mjs --json da1126a032b175f39606fc1ddb86642d3cdad64f

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs da1126a032b175f39606fc1ddb86642d3cdad64f → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actions github-actions Bot added size/xl documentation Improvements or additions to documentation tests tooling labels Aug 26, 2026
@os-warren
os-warren marked this pull request as ready for review August 26, 2026 13:27
@os-warren
os-warren enabled auto-merge August 26, 2026 13:27

Copy link
Copy Markdown
Collaborator Author

PM review — accepted, flipped ready, auto-merge armed. needs:contract-review added to #12032, because clause ② came back yes and I had not pre-applied it.

Measured: 15 files, +1257/−260. Both doors verified independently of the report.

Door 1 — the parse narrows. Three keys converted from parsing schemas to refusals:

-  autoRestart: z.boolean().default(false)                              → retiredKey(AUTO_RESTART_RETIRED)
-  maxRestartAttempts: z.number().int().min(0).default(3)               → retiredKey(MAX_RESTART_ATTEMPTS_RETIRED)
-  restartBackoff: z.enum(['fixed','linear','exponential']).default(…)  → retiredKey(RESTART_BACKOFF_RETIRED)

Door 2 — registerPlugin throws where it always returned, and the placement is the load-bearing detail. assertNoRetiredKeys is genuinely the first statement, with the reason on the line:

"Deliberately FIRST, so a config that would otherwise register cleanly cannot smuggle the false declaration past the door."

That is the same reasoning #12425 used to put its refusal before the enabled check. Precedent applied, not coincidence.

The route went past triage's floor — and the argument for that is the best thing in the report

Triage pre-blessed "a loud destroy-only, will not re-init posture is acceptable". This PR rejects that floor from above: destroy-only "turns a degraded plugin into a dead one the host still owns, and it leaves maxRestartAttempts provably inert." That is correct and it is the kind of push-back a floor exists to invite rather than to cap. A plugin that is merely failing its checks is still serving; destroying it is a strictly worse outcome than reporting it.

The two closed forks were measured, not presumed:

  • ENFORCE is unavailable. Plugin.init(ctx) needs a PluginContext; the only two plugin.init(...) call sites are the kernel's own boot loops, over the full plugin list, with a context that is private on ObjectKernel and protected on KernelBasecreateContext() is protected and no public accessor exists. So a "host-provided re-init hook" would have had nothing to call, and route 1 meant building a per-plugin re-init kernel API. Positive control fired in the same scan: five real non-test plugin.destroy() call sites resolve.
  • EXPERIMENTAL is unavailable. Zero mentions of plugin auto-restart across the whole docs/ planning+ADR corpus, against 118 control hits for health and 13 for hot reload in the same corpus. That control is what makes the zero a reading.

And maxRestartAttempts / restartBackoff leave with autoRestart rather than as tidy-up — with no restart they have nothing left to be the vocabulary of. Same per-key test #12340 applied when distributedConfig left with the stateStrategy value its own doc comment named it required for. Keeping them would have created two fresh declared-but-unenforced keys inside a PR retiring one.

The ablation caught its own first pin being wrong

Leg 1: predicted 5 red, observed 6 — recorded, not smoothed, with the cause named ("miscounted the test's own drive, not the mechanism"). But the useful finding is what leg 1 exposed: the richer walk-the-sequence pin tripped an early status assertion first (expected recovering to be failed), so the contract itself was not the thing that broke. A pin that fails for the wrong reason is a pin that will pass for the wrong reason later.

So they added an isolated terminal-state pin and re-ran: leg 2 predicted 7, observed 7, and the predicted assertion identity held — it failed on "the monitor reported healthy for a plugin that had been destroyed". That is the terminal state I asked for in the dispatch, and they discovered their own first attempt had not actually pinned it.

The reproduction is preserved as a comment in the pin file, which is where it will be read:

after backoff:    status=recovering  destroyed=1  alive=false
recovery round 3: status=healthy     destroyed=1  alive=false

Five tests pinned the defect as the contract — the third occurrence today

Four #11852 tests and one #11955 test each asserted destroyed.count === 1 and status recovering after crossing failureThreshold. All rewritten, all declared in the report, in the PR body, and inline on the line — and the renames are honest about what changed ("restarts a plugin whose check THROWS…""leaves a plugin whose check THROWS alone…"). One is worth singling out: "leaves a throwing plugin alone when autoRestart is false" went vacuous once the key was gone, and was replaced by the refusal block rather than deleted quietly — a control that no longer controls anything is dead weight, and noticing that is harder than fixing it.

Three landed invariants confirmed intact: #4875's refd-timer guard, #11852's shared counters, #11955's successThreshold gate. recovering is now written only by the success branch.

Not-measured, correctly

check-dev-prereqs.mjs's scan half refused on an unbuilt workspace (naming 34 app packages outside this diff) and prints its own "Nothing was measured … It is NOT a finding." CI runs only the --self-test half, which is green here. Three further gates first read red for the same unbuilt-prerequisite reason and were converted into real green readings by building their dependency closures rather than reported as failures — which is the right direction of travel for a refusal.

My correction

I dispatched this without needs:contract-review, reasoning that the fix might only change what is logged and what status is recorded. It narrows an accept set on two doors and retires three published keys. Label added to #12032 now. The dispatch did ask for the clause-② judgement with evidence rather than asserting the answer, which is why the gap closed on measurement instead of shipping unlabelled.

CI is the remaining gate.


Generated by Claude Code

@os-warren
os-warren added this pull request to the merge queue Aug 26, 2026
Merged via the queue into main with commit b72db01 Aug 26, 2026
35 checks passed
@os-warren
os-warren deleted the claude/issue-12032-autorestart-never-reinits branch August 26, 2026 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/xl tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

autoRestart destroys the plugin and never re-initialises it, then reports it recovering — and, once successThreshold binds, healthy

1 participant