fix(core): autoRestart fires for a health check that throws or times out, not only for one that returns a failure

[os-warren]

Fixes#11852

PluginHealthMonitor.performHealthCheck reaches its failure handling by two disjoint
routes, and only one of them could ever restart the plugin.

A check that returned a failure (false or { status: 'unhealthy' }) incremented
failureCounters, cleared successCounters, and — once failureThreshold consecutive
failures accumulated — consulted autoRestart and restarted the plugin. A check that
threw took a separate catch block that incremented failureCounters and stopped
there: it never cleared successCounters and never read autoRestart. Because
raceCheckTimeout builds its guard as new Promise((_, reject) => …) and so rejects
rather than resolving, every timeout overrun lands in that catch. A plugin that hung
was therefore marked failed and never restarted, no matter how many rounds passed or
what autoRestart said — the severer of the two failure modes was the one that could not
trigger recovery.

config.autoRestart appeared exactly once in the file, inside the returned-failure
branch. It now appears once inside the shared path both routes take.

What changed

Both routes funnel into one recordFailedRound step that owns the failure counter, the
successCounters reset, the failureThreshold comparison and the autoRestart decision.
A thrown or timed-out check is restart-eligible on exactly the same terms as a returned
failure.

The step sits outside the try, deliberately: it may await a restart, and a fault
raised by restart handling is not a health-check exception. Under the old shape
attemptRestart ran inside the try, so a throw from it would have been relabelled as a
failed check and pushed a second health-check entry for a check that had actually run.

What deliberately did NOT change

The per-route status label. A throw stays the separate failed status applied
immediately with no threshold. That is not incidental — it is the documented contract, and
it predates this card (the text is already present at a1c804bc9, the commit the issue
measured):

A check that throws — including one that exceeds timeout — is the separate
failed status, applied immediately with no threshold.

— content/docs/protocol/kernel/lifecycle.mdx, "Custom Health Checks"

It is also pinned by the pre-existing still reports the timeout when the check never answers, which asserts failed on round 1 against a failureThreshold of 3.

So the fully-unified shape floated in triage — route both failures through the same
threshold handling end to end — would have turned a below-threshold throw into degraded,
contradicting a documented sentence and reddening an existing test. Only the counters and
the restart decision are shared here, because those are what failureThreshold and
autoRestart declare, and neither of them names a route. Mirroring was the smaller honest
change; a new pin (keeps a throw at 'failed' immediately, with no threshold) guards that
boundary against a future re-unification.

The successCounters half

The issue's second asymmetry — the catch path not clearing successCounters — is fixed
(the shared step clears it structurally, on both routes). It has no behavioural pin,
and that is a measurement rather than an omission: the counter's only read site is
unreachable with a stale non-zero value, because the returned-failure route always zeroes
it before unhealthy/degraded can be entered, and the throw route sets failed, which
the recovery branch short-circuits past. Filed separately as #11955, which is the reason
the read site is unreachable. Writing a test here would have meant one that passes for the
wrong reason.

Tests

Five new pins in packages/core/src/health-monitor.test.ts, all asserting the observable
consequence — attemptRestart is the only caller of plugin.destroy() and the only
writer of recovering, so those two readings together mean a restart happened and nothing
else can produce them:

• restarts a plugin whose check THROWS, once failureThreshold accumulates
• restarts a plugin whose check exceeds timeout — the severest route, and the one no
  test covered
• still restarts a plugin whose check RETURNS a failure (the route that already
  worked, so unifying cannot close the throw gap by opening one here)
• leaves a throwing plugin alone when autoRestart is false — the control, without
  which the pins above would also pass if every failure restarted unconditionally
• keeps a throw at failed immediately, with no threshold — the documented rule above

Non-vacuity

The before-state is a silent absence of a restart, so the new pins were proven to go red
against the unfixed code. Direction predicted in advance: reverting health-monitor.ts to
origin/main with the tests untouched should turn exactly 2 of 12 red — the two
restart-on-throw pins — while the returned-failure pin, the autoRestart: false control
and the failed-immediately pin stay green, since they describe behaviour the unfixed code
already had.

Mutation confirmed on disk by anchored count before any result was read
(recordFailedRound 3 → 0), restore under trap … EXIT INT TERM, tree verified clean
after (git status --porcelain empty, recordFailedRound back to 3):

BEFORE: recordFailedRound=3 config.autoRestart=1
AFTER: recordFailedRound=0 config.autoRestart=1
MUTATION CONFIRMED ON DISK
× restarts a plugin whose check THROWS, once failureThreshold accumulates
× restarts a plugin whose check exceeds `timeout` — the severest route
AssertionError: expected +0 to be 1 // destroy() never called
Tests 2 failed | 10 passed (12)

Observed direction matched the prediction exactly. No rebuild leg applies: the test imports
the subject by relative path (./health-monitor.js) and packages/core/vitest.config.ts
declares no alias, so vitest resolves it to source — the stale-dist hazard only bites when
the subject resolves through a package's exports.

Restored tree: Tests 12 passed (12).

Gates

Union derived from the actual diff with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (never recalled),
re-derived after merging origin/main because that merge changed three of the gate scripts
themselves. All 21 green at f540920189 — 14 path-matched, 6 convention-triggered by
editing a test file, plus check:nul-bytes. Exit codes captured before any pipe.

The ratchet half is the load-bearing one: pnpm check:type-check-debt re-measured all 32
ledger entries against the built closure —

check-type-check-coverage --re-measure: OK — 32 ledger entr(ies) re-measured in 332.8s,
1898 raw tsc error(s) total, none above its recorded number.

@objectstack/core carries a DEBT entry of 98, so this diff is inside that gate's
population rather than beside it.

---

Generated by Claude Code

[github-actions]

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/core, touching 3 documentable anchor(s).

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 PluginHealthMonitor (symbol))

What this run could not see

• the SDK route bridge reached 45 of 222 client-bound route-ledger rows — the other 177 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: 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 — 23 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 0cbe79a36d750423381542b49c44ab720f2e0384 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 436fcab008049402ed6868f6e4528d696bf0f810 — the merge of head f540920189e3eb61d7deac70864406f54f1d1468 into base 0cbe79a36d750423381542b49c44ab720f2e0384, 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 436fcab008049402ed6868f6e4528d696bf0f810 && git checkout 436fcab008049402ed6868f6e4528d696bf0f810
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 0cbe79a36d750423381542b49c44ab720f2e0384 f540920189e3eb61d7deac70864406f54f1d1468 && git checkout -B drift-repro 0cbe79a36d750423381542b49c44ab720f2e0384 && git merge --no-ff f540920189e3eb61d7deac70864406f54f1d1468
node scripts/docs-audit/affected-docs.mjs --json 0cbe79a36d750423381542b49c44ab720f2e0384

⚠️ 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 0cbe79a36d750423381542b49c44ab720f2e0384 → pass the list as
args.docs, on the commit named under Which tree this was computed on.