Skip to content

fix(metadata): a failing history cleanup run is no longer silent, at both of start()’s triggers - #16177

Merged
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-16061-history-cleanup-silent-errors
Sep 6, 2026
Merged

fix(metadata): a failing history cleanup run is no longer silent, at both of start()’s triggers#16177
zhuangjianguo merged 3 commits into
mainfrom
claude/issue-16061-history-cleanup-silent-errors

Conversation

@claude

@claude claude Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Fixes #16061

A HistoryCleanupManager run that loses deletes now says so — at both of start()'s triggers.

The closed loop, and where it was actually broken

Three things composed to make a failing cleanup completely silent:

  1. every inner catch on the delete path is a bare catch { — unbound, so the error object is gone;
  2. the only relevant console.error sits in runCleanup()'s OUTER catch, which those inner catches prevent execution from reaching;
  3. start() invoked the run as void this.runCleanup(), discarding the { deleted, errors } the run returns — at BOTH call sites, the immediate run and every interval tick.

A driver whose deletes failed on every scheduled run therefore produced zero output and no reachable error count, while the history table grew past its retention policy with nothing to find.

This is a bug, not an enhancement. Adding a report here is not an improvement to a working path — it turns an error path that already exists from unobservable into observable.

The declared convention — read first, and it decides the remedy

Both the card and triage flagged that runCleanup()'s error channel might have a declared convention neither had read. It does, in two places that agree:

  • AGENTS.md → "Degradation log levels" names three legal answers for a durability seam's catch: rethrow, log at error, or hand the failure to the CALLER. On the third it is explicit: "Do not bolt a logger.error onto such a site", and an error that is warranted owes two things in its first line — the consequence and the fix — said once, at the first degradation, not once per failed write.

  • scripts/check-durability-degradation-log-level.mjs has already classified these exact seams. Its triage table records metadata history-cleanup.ts x3 (runCleanup) as correct-but-undeclarederrors++ into the returned { deleted, errors }, and a later note states the defect in its own words:

    The three history-cleanup seams deliver errors into a returned envelope, and BOTH production call sites are void this.runCleanup() — so the count is correct by contract and read by nobody in-process.

So the inner catches were never the defect: the envelope is the sanctioned channel. What was missing was a reader. start() is where the chain ends — it returns void, and an interval tick has no caller at all — so that is where the report belongs, once per run.

The repair therefore reads the envelope rather than replacing it. runCleanup()'s signature, its inner catches and its counting are untouched. Both call sites now go through one module-level pass that reads the counts and, when a run lost deletes, prints a single error line naming the consequence (rows past the retention policy are still in the table, nothing retries them, and the system keeps reporting healthy) and where to look.

The shape follows the repo's own reference implementation of this pattern — the counter-guarded summary report in packages/plugins/plugin-email/src/outbox-sweep.ts, which check:durability-log-level's summary limb already recognises.

Counts, re-located by text on my own base

main had moved since triage, so everything was re-derived. Measured on origin/main @ 94ecb7e5c, packages/metadata/src/utils/history-cleanup.ts, 317 lines — reproducing triage exactly:

site count lines
bare catch { (unbound) 4 :116, :162, :166, :224
bound catch (error) 2 :170, :304
console.error 2 :171, :305
void this.runCleanup() 2 :55, :59
errors incremented 7 :115, :117, :160, :163, :167, :172, :225

One correction to the record, measured: the brief said main "has moved a long way" since triage's a4816a7 and that the numbers could not be trusted for that reason. Re-deriving was right, but the stated reason does not hold for this filegit diff a4816a7 94ecb7e5c on it is empty, and a4816a7 is an ancestor of origin/main. The file was byte-identical to what triage measured. The card's own numbers, by contrast, really were low on two rows (three bare catches, one void call site).

The serial, measured before a branch was cut

The hard serial on this file is released by MERGE, not by "armed" or "claimed". PR #16060 (card #15824): merged: true, merged_at 2026-09-06T00:21:18Z, merge commit 281bf0d35, an ancestor of origin/main. Confirmed independently by content — setUTCDate(/getUTCDate() = 2/2 and setDate(/getDate() = 0/0, with the control toISOString = 2. No open PR touches this file. The calendar arithmetic is not touched here; that is a wrong value, this is a missing signal.

The pin, and two ablations

packages/metadata/src/utils/history-cleanup-failure-signal.test.ts asserts the signal, never the cleanup — no case asserts a row was deleted. Four cases: the immediate run reports; every scheduled run reports; a control run that loses nothing stays silent; and runCleanup()'s envelope is unchanged for a direct caller.

Both triggers are asserted separately on purpose, and two ablations prove why. Each mutated the committed tree, proved the mutation on disk by line-anchored counts plus a git hash-object mismatch against the HEAD blob, and restored via git checkout HEAD -- <absolute path> under an EXIT/INT/TERM trap, proving the restore by blob equality and an empty git diff HEAD:

ablation code-level call sites reverted result
A — restore the swallow at both sites runCleanupAndReport 0, runCleanup 2 2 failed / 2 passed, vitest exit 1
B — half-fix: immediate repaired, interval reverted runCleanupAndReport 1, runCleanup 1 1 failed / 3 passed — exactly the scheduled-run case

Ablation B is the load-bearing one: repairing only the immediate run leaves every scheduled run silent, which is the defect for the trigger that runs forever, and the pin catches precisely that.

A first attempt at ablation A did not land and was caught by its own proof step rather than by its result: the 4-space immediate call site is a substring of the 6-space interval one, so a substring anchor matched both and the edit no-opped, leaving the blob equal to HEAD. The instrument was switched to whole-line matching and re-run. Recording it because a silent no-op ablation reads exactly like a passing one.

Clause-② — no, both limbs, measured

@objectstack/metadata publishes files: ["dist", ...], so the surface is every declaration file under dist/, not the root barrel. Built at head, swapped this file back to the merge base, rebuilt, and diffed all 10 published declaration files (5 entry points x 2 module formats):

  • hash-manifest diff: empty. All 10 byte-identical, dist/node.d.ts and the subpath barrels included.
  • The rebuild is proven to have re-run rather than been skipped: dist/index.js mtime 1788668782 -> 1788668792.
  • The instrument is proven live by a firing control from the same script one commit earlier: with the reporter written as a private method it reported 2 of the 10 differing (index.d.ts, index.d.cts), the delta being private runCleanupAndReport; plus its JSDoc. That is why the reporter is a module-level function here — the idiom this file already uses for executionPinnedTypes(). A private member is emitted into the published .d.ts and joins the class's nominal identity, so keeping it out means an observability repair moves no published declaration at all.

Limb 1 — nothing published moves: no exported symbol added, removed or renamed; the declaration bytes are identical.

Limb 2, argued explicitly since it is the one that can fire without a contract path: runCleanup()'s signature is unchanged (Promise<{ deleted: number; errors: number }>, byte-identical in the built .d.ts), and start() was not made to propagate — it still returns void. So no consumer's accept/reject can move in either direction: the type surface a consumer compiles against is byte-identical, and no request that compiled before fails now. At runtime nothing accepted or rejected moves either — the delete filter, the rows deleted and the returned counts are unchanged, which the control case and the envelope case pin from opposite sides. The only new behaviour is output on a path that previously produced none.

Changeset

patch on @objectstack/metadata. This diff does publish from a package (packages/metadata/src/** ships in dist/), so the skip-changeset label does not apply — that label is for a diff that publishes nothing.

Verification

Run on a5ef26c7e (origin/main @ 07f40e546 merged in; no conflicts). Every command through scripts/pm/os-verify-lock.sh, exit codes captured by redirect-then-read, never through a pipe.

  • pnpm --filter @objectstack/metadata typecheck — clean. Proven to cover the new files rather than exclude them: tsc --noEmit --listFiles lists both the edited source and the new test (551 files in the program).
  • pnpm --filter @objectstack/metadata exec vitest run on the new pin plus the sibling history-cleanup-dst.test.ts31 passed (2 files).
  • The derived gate family — node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, 54 commands, change set of 3 paths against merge base 07f40e546. The family was not narrowed for a small diff: this change alters the file's line count, so the anchor/census members (check:system-context-census, check:platform-object-tenancy-census, check:comment-mask-corpus) were run regardless, alongside check:durability-log-level, check:logger-receiver-detach and check:refd-timer-probe, which this change's subject matter reaches directly.

Generated by Claude Code

…ither start() trigger

Every inner catch on `runCleanup()`'s delete path is a bare `catch {`, so the
error object is discarded; the only `console.error` in the method sits in its
OUTER catch, which those inner catches prevent execution from reaching; and
`start()` invoked the run as `void this.runCleanup()` at BOTH call sites — the
immediate run and every interval tick — discarding the `{ deleted, errors }`
the run returns. A driver whose deletes failed on every scheduled run produced
zero output and no reachable error count.

Read the envelope rather than replace it. `runCleanup()`'s contract, its inner
catches and its counting are untouched: handing a failure to the CALLER is the
third answer AGENTS.md "Degradation log levels" allows a durability seam, and
that section names a log per failed write as the mirror-image failure. What was
missing was a reader, and `start()` is where the chain ends — it returns void
and an interval tick has no caller. Both call sites now go through one private
pass that reads the counts and prints a single `error` line, naming the
consequence and the fix, when a run lost deletes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ARYe3yQTQCUFm5qPYNgKaJ
… published declaration does not move

`private` members are emitted into the published `dist/*.d.ts` and join the
class's nominal identity, so a private method would have made an observability
repair change `@objectstack/metadata`'s declaration surface. As a module-level
function — the idiom this file already uses for `executionPinnedTypes()` — it
changes none of it. Behaviour, call sites and the pin are unchanged.

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

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

1 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 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 — 13 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 7d7ca6c0cc1ed46a6e236de3b0749aff1a69008bpackageMentionDocs.

Which tree this was computed on

This run read content/docs from add4849a9af5e1942b3d10f6f4e5d3c2c0f85ef0 — the merge of head a5ef26c7eba5e290700c7e9d1743da4801bb750e into base 7d7ca6c0cc1ed46a6e236de3b0749aff1a69008b, 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 add4849a9af5e1942b3d10f6f4e5d3c2c0f85ef0 && git checkout add4849a9af5e1942b3d10f6f4e5d3c2c0f85ef0
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 7d7ca6c0cc1ed46a6e236de3b0749aff1a69008b a5ef26c7eba5e290700c7e9d1743da4801bb750e && git checkout -B drift-repro 7d7ca6c0cc1ed46a6e236de3b0749aff1a69008b && git merge --no-ff a5ef26c7eba5e290700c7e9d1743da4801bb750e

node scripts/docs-audit/affected-docs.mjs --json 7d7ca6c0cc1ed46a6e236de3b0749aff1a69008b

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

@zhuangjianguo
zhuangjianguo marked this pull request as ready for review September 6, 2026 05:07
@zhuangjianguo
zhuangjianguo added this pull request to the merge queue Sep 6, 2026
Merged via the queue into main with commit 0c5d035 Sep 6, 2026
35 checks passed
@zhuangjianguo
zhuangjianguo deleted the claude/issue-16061-history-cleanup-silent-errors branch September 6, 2026 05:48
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/m tests tooling

Projects

None yet

2 participants