From 050c5ba6613df539956e9f9e5086a99646efdda2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 08:22:26 +0000 Subject: [PATCH 1/2] fix(objectql,cli): the integrity audit stops when the engine does (#4747) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every `os migrate` subcommand ended a SUCCESSFUL run with two `ERROR Find operation failed` lines and a #4551 report naming `sys_metadata` / `sys_view_definition` as `unreadableObjects`. That bucket exists to separate "I could not check" from "I checked and it was fine"; non-empty on every healthy run, it separated nothing. Two silent no-ops stacked up: - `ObjectQLPlugin` put its teardown in `stop()`, but the kernel's Plugin contract is `init`/`start`/`destroy` — `stop()` is never called by anyone, so the ADR-0057 sweep timers were never disarmed on any host. - `bootSchemaStack().shutdown()` called `(runtime as any).stop?.()` and `Runtime` has no `stop`. The optional call made "no teardown" look exactly like "teardown performed": the kernel stayed running with every timer armed while the command closed its driver. 60s after boot the sweep woke inside the still-alive process and read a pool its own host had already disconnected. - ObjectQLPlugin: `stop` -> `destroy`, the hook the kernel actually calls. - bootSchemaStack: tear down via `kernel.shutdown()` — the same path `os serve` takes on SIGTERM, so one-shot and server exit alike. - LifecycleService.stop(): raise an abort bit the in-flight sweep holds, not just clear timers; refuse to start a sweep once stopped. - The audit takes that bit as `signal` and stops issuing reads. A read that fails *because* the run was called off is dropped rather than filed — it is not evidence about the datasource. The new `DanglingReferenceReport.aborted` keeps the incompleteness loud without spending the finding bucket on it. A genuine datasource fault still lands in `unreadableObjects`, and the audit still runs on CLI hosts: this is a liveness boundary, not the "one-shot commands skip the audit" switch the issue rejected. Fixes #4747 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny --- .changeset/hungry-donkeys-repeat.md | 28 +++ ...chema-migrate.teardown.integration.test.ts | 120 +++++++++++++ packages/cli/src/utils/schema-migrate.ts | 22 ++- packages/objectql/src/index.ts | 3 + .../dangling-reference-audit.test.ts | 159 ++++++++++++++++++ .../src/integrity/dangling-reference-audit.ts | 95 ++++++++++- .../src/lifecycle/lifecycle-service.test.ts | 116 ++++++++++++- .../src/lifecycle/lifecycle-service.ts | 61 ++++++- packages/objectql/src/plugin.ts | 31 +++- 9 files changed, 620 insertions(+), 15 deletions(-) create mode 100644 .changeset/hungry-donkeys-repeat.md create mode 100644 packages/cli/src/utils/schema-migrate.teardown.integration.test.ts diff --git a/.changeset/hungry-donkeys-repeat.md b/.changeset/hungry-donkeys-repeat.md new file mode 100644 index 0000000000..586bd8d3b9 --- /dev/null +++ b/.changeset/hungry-donkeys-repeat.md @@ -0,0 +1,28 @@ +--- +'@objectstack/objectql': patch +'@objectstack/cli': patch +--- + +修复:每个 `os migrate` 子命令关停后,#4551 悬空引用巡检都会把 `sys_metadata` / `sys_view_definition` 报成 `unreadableObjects`(#4747) + +一条**成功**的命令过去会在返回 JSON 之后打出两行 `ERROR Find operation failed` 和一份 +`unreadableObjects` 非空的巡检报告 —— 对抓 ERROR 的 CI 流水线是直接误报源,更要命的是它把 +`unreadableObjects` 变成了恒为真的告警:那个桶存在的意义正是区分「我没能检查」和「我检查了, +没问题」,一个每次健康运行都非空的桶不再携带任何信息。 + +两处静默空转叠出了这个结果: + +- `ObjectQLPlugin` 的关停逻辑写在 `stop()` 里,而内核的插件契约是 `init`/`start`/`destroy` —— + `stop()` 从来没有被任何人调用过,ADR-0057 巡检定时器因此在任何宿主上都不会被解除。改为 + `destroy()`(与 `DefaultDatasourcePlugin` 一致)。 +- `bootSchemaStack().shutdown()` 调的是 `(runtime as any).stop?.()`,而 `Runtime` 根本没有 + `stop` —— 可选调用把「没有关停」伪装成了「关停过了」。改为走内核自己的 `kernel.shutdown()`, + 与 `os serve` 收到 SIGTERM 时同一条路径。 + +同时 `LifecycleService.stop()` 不再只是清定时器:它还会把「引擎正在拆」这一位交给正在飞行中的 +sweep,巡检据此在读之前停手。因关停而失败的读**不再进入** `unreadableObjects` —— 那不是关于 +数据源的证据;报告改用新增的 `DanglingReferenceReport.aborted` 记录「这次没跑完」,所以不完整 +依然是响的,只是不再占用发现桶。 + +**真正读不出来的对象(数据源故障)照旧进 `unreadableObjects`**,巡检在 CLI 场景也照旧运行 —— +这里没有「一次性命令不跑巡检」的开关,只有「引擎活着才读」的生命周期边界。 diff --git a/packages/cli/src/utils/schema-migrate.teardown.integration.test.ts b/packages/cli/src/utils/schema-migrate.teardown.integration.test.ts new file mode 100644 index 0000000000..86178b84fb --- /dev/null +++ b/packages/cli/src/utils/schema-migrate.teardown.integration.test.ts @@ -0,0 +1,120 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4747] A one-shot CLI stack tears down through the KERNEL, and the ADR-0057 + * sweep stops with it. + * + * The bug this pins, end to end on the real `bootSchemaStack` path: + * + * $ os migrate recorded-by --json # exits 0, prints valid JSON … + * ERROR Find operation failed {"object":"sys_metadata", …} + * WARN [integrity] dangling-reference audit could not list an object … + * WARN [integrity] stored references that resolve to nothing (#4551) + * {"unreadableObjects":["sys_metadata","sys_view_definition"], …} + * + * Two silent no-ops stacked up to produce it. `shutdown()` called + * `(runtime as any).stop?.()` and `Runtime` has no `stop`; the one thing that + * would have disarmed the sweep was `ObjectQLPlugin.stop()`, a hook the kernel + * never calls (the Plugin contract is `init`/`start`/`destroy`). So the kernel + * stayed "running" with every timer armed while the command closed its driver, + * and 60s later the sweep read a pool that was gone — filing both objects as + * `unreadableObjects` on a completely healthy run. + * + * Two assertions matter here and they pull in opposite directions on purpose: + * + * - while the engine is LIVE, a CLI-booted stack really does audit (this is + * not #4747's rejected option C — "one-shot commands skip the audit" would + * make it permanently blind exactly where an operator has no other tool); + * - once the stack is torn down, the sweep issues no reads at all. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { bootSchemaStack } from './schema-migrate.js'; + +interface LifecycleServiceLike { + stopped: boolean; + sweep(): Promise<{ danglingReferences?: { unreadableObjects: string[]; aborted?: boolean } }>; +} + +describe('[#4747] bootSchemaStack teardown disarms the ADR-0057 sweep', () => { + let dir: string; + let dbFile: string; + const savedEnv: Record = {}; + + beforeAll(() => { + dir = mkdtempSync(join(tmpdir(), 'os-teardown-')); + mkdirSync(join(dir, 'dist'), { recursive: true }); + mkdirSync(join(dir, 'data'), { recursive: true }); + dbFile = join(dir, 'data', 'app.db'); + writeFileSync( + join(dir, 'dist', 'objectstack.json'), + JSON.stringify({ + id: 'teardown_smoke', + name: 'Teardown Smoke', + objects: [ + { + name: 'td_note', + fields: { + title: { type: 'text', required: true }, + // A real reference field, so the audit has something to read + // rather than skipping the object outright. + owner: { type: 'lookup', reference: 'td_person' }, + }, + }, + { name: 'td_person', fields: { name: { type: 'text' } } }, + ], + }), + ); + savedEnv.OS_ARTIFACT_PATH = process.env.OS_ARTIFACT_PATH; + process.env.OS_ARTIFACT_PATH = join(dir, 'dist', 'objectstack.json'); + }); + + afterAll(() => { + process.env.OS_ARTIFACT_PATH = savedEnv.OS_ARTIFACT_PATH; + try { rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ } + }); + + it('audits while the engine is live, and reads nothing once the stack is down', async () => { + const stack = await bootSchemaStack({ databaseUrl: `file:${dbFile}`, projectRoot: dir }); + // Resolved BEFORE teardown — the point is what this same instance does + // afterwards, and service resolution post-shutdown is not the subject. + const lifecycle = stack.kernel.getService('lifecycle') as LifecycleServiceLike; + expect(lifecycle).toBeTruthy(); + + // ── While the engine is live: the audit runs for real ───────────────── + // Not "the CLI skips the audit" — it reads, and reports a clean, COMPLETE + // run. An empty `unreadableObjects` here is a fact about the database, + // which is precisely what it stopped being before this fix. + expect(lifecycle.stopped).toBe(false); + const live = await lifecycle.sweep(); + expect(live.danglingReferences).toBeDefined(); + expect(live.danglingReferences!.unreadableObjects).toEqual([]); + expect(live.danglingReferences!.aborted).toBe(false); + + // ── Teardown ────────────────────────────────────────────────────────── + await stack.shutdown(); + + // The kernel really shut down. `(runtime as any).stop?.()` left it running + // and every plugin undestroyed, which is how a missing teardown managed to + // look exactly like a performed one. + expect(stack.kernel.isRunning()).toBe(false); + expect(lifecycle.stopped).toBe(true); + + // The sweep the timer would have fired 60s later: no engine reads, so no + // `ERROR Find operation failed` on a successful command, and no object + // filed as unreadable for the crime of being asked after closing time. + const afterDown = await lifecycle.sweep(); + expect(afterDown.danglingReferences).toBeUndefined(); + + // …and this is not vacuous: the pool really is closed, so a read issued + // here really would fail. The silence above is the fix, not an absence of + // anything to read. + const engine = stack.kernel.getService('objectql') as { + find(object: string, options: Record): Promise; + }; + await expect(engine.find('td_note', { limit: 1, context: { isSystem: true } })).rejects.toThrow(); + }, 120_000); +}); diff --git a/packages/cli/src/utils/schema-migrate.ts b/packages/cli/src/utils/schema-migrate.ts index 4c65c8090b..3e6e4f40c7 100644 --- a/packages/cli/src/utils/schema-migrate.ts +++ b/packages/cli/src/utils/schema-migrate.ts @@ -209,8 +209,28 @@ export async function bootSchemaStack( flushSchemaDdl: async () => (defer && driver?.flushDeferredSchemaDdl ? await driver.flushDeferredSchemaDdl() : []), + /** + * Tear the one-shot stack down through the kernel's own teardown — the + * same `kernel.shutdown()` `os serve` runs on SIGTERM, so a one-shot + * command and a server take ONE path out (#4747). + * + * It used to call `(runtime as any).stop?.()`. `Runtime` has no `stop` — + * the optional-call swallowed that fact, so every `os migrate` subcommand + * closed its driver while leaving the kernel fully "running": no plugin + * ever got `destroy()`, and the ADR-0057 lifecycle sweep stayed armed. 60s + * later it woke inside the still-alive process and read through the pool + * this line had already closed, which is why a successful command ended in + * `ERROR Find operation failed` and a #4551 report naming `sys_metadata` / + * `sys_view_definition` as unreadable. A cast plus `?.` is how a missing + * teardown looks exactly like a performed one; there is no version of that + * call that could ever have worked. + * + * The explicit `disconnect()` stays as the backstop for a driver this + * kernel did not register through `DefaultDatasourcePlugin` (whose own + * `destroy()` closes the ones it owns); a second disconnect is a no-op. + */ shutdown: async () => { - try { await (runtime as any).stop?.(); } catch { /* ignore */ } + try { await kernel.shutdown(); } catch { /* teardown is best-effort */ } try { await driver?.disconnect?.(); } catch { /* ignore */ } }, }; diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index ba0862ebea..b7b56c5619 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -138,6 +138,9 @@ export type { DanglingReferenceAuditOptions, DanglingReferenceAuditPort, AuditableObject, + // [#4747] The teardown bit the audit reads — exported alongside the options + // type that names it, so a caller can spell the parameter it passes. + AuditAbortSignal, } from './integrity/dangling-reference-audit.js'; // Export MetadataFacade diff --git a/packages/objectql/src/integrity/dangling-reference-audit.test.ts b/packages/objectql/src/integrity/dangling-reference-audit.test.ts index 7e8ec1ef90..54e9ae3889 100644 --- a/packages/objectql/src/integrity/dangling-reference-audit.test.ts +++ b/packages/objectql/src/integrity/dangling-reference-audit.test.ts @@ -372,3 +372,162 @@ describe('[#4551] dangling stored references are reported, never rewritten', () ]); }); }); + +/** + * [#4747] "The datasource refused" and "nobody asked" are different facts. + * + * The audit reads through a live engine, and the engine outlives it only until + * the host closes the pool. Before this, every `os migrate` invocation ended + * with `unreadableObjects: ['sys_metadata', 'sys_view_definition']` — the sweep + * fired 60s after boot, inside a process whose datasource had already been + * disconnected. An `unreadableObjects` that is non-empty on every healthy run + * is not a cautious report; it is a broken alarm, and it costs exactly the + * signal #4551 built the bucket for. + * + * The pair of tests that opens this block is the whole point: BEFORE the fix + * the two runs were indistinguishable, and after it they must never again be + * confusable. Deleting the abort handling makes the second one fail; deleting + * the `unreadableObjects` push makes the first one fail. + */ +describe('[#4747] a run that was called off is not a finding about the data', () => { + /** A port whose listing always fails — the two runs below differ ONLY in why. */ + const unreadablePort = () => makePort({ + objects: [binding], + rows: {}, + unreadable: new Set(['sys_position_permission_set']), + }); + + it('a REAL datasource fault still lands in `unreadableObjects`, loudly', async () => { + // The case the bucket exists for, and the one that must survive the fix: + // the audit tried, the store would not answer, and the report must not read + // as a clean bill of health on an object nothing could look at. + const port = unreadablePort(); + + const out = await auditDanglingReferences(port); + + expect(out.unreadableObjects).toEqual(['sys_position_permission_set']); + expect(out.aborted).toBe(false); + expect(port.warnings.map((w) => w[0])).toContain( + '[integrity] dangling-reference audit could not list an object', + ); + }); + + it('the SAME failure, raced by a teardown, is dropped instead of filed', async () => { + // Identical throw from identical code — the only difference is that the + // caller had called the run off, which is what closing a connection pool on + // purpose looks like from in here. It is not evidence about the datasource, + // so it must not spend the bucket that only holds evidence. + const signal = { aborted: false }; + const port = unreadablePort(); + const failing = port.find.bind(port); + port.find = async (o, opts) => { + signal.aborted = true; // the pool closes mid-query + return failing(o, opts); // …and the query fails because of it + }; + + const out = await auditDanglingReferences(port, { signal }); + + expect(out.unreadableObjects).toEqual([]); + // Not silence either: the run says it did not finish, so nothing can read + // `dangling: []` as "everything is fine". + expect(out.aborted).toBe(true); + expect(port.warnings).toEqual([]); + }); + + it('called off BEFORE a read: no query is issued at all', async () => { + // This is what removes the `ERROR Find operation failed` line from a + // SUCCESSFUL command — the engine never gets asked, so it never logs. + const reads: string[] = []; + const port = makePort({ + objects: [binding, task], + rows: { sys_position_permission_set: [{ id: 'ppr_1', permission_set_id: 'ps_gone' }] }, + }); + const findSpy = port.find.bind(port); + port.find = async (o, opts) => { reads.push(o); return findSpy(o, opts); }; + + const out = await auditDanglingReferences(port, { signal: { aborted: true } }); + + expect(reads).toEqual([]); + expect(port.probes).toEqual([]); + expect(out.aborted).toBe(true); + expect(out.unreadableObjects).toEqual([]); + }); + + it('called off MID-run keeps what it already proved and stops there', async () => { + // A finding is a finding whenever it was made; only the SILENCE about the + // rest of the run becomes unreliable, which is what `aborted` records. So + // an abort must not discard the first object's verdict — and must not turn + // the second object into a report about the datasource. + const signal = { aborted: false }; + const reads: string[] = []; + const port = makePort({ + objects: [binding, task], // binding is scanned first (security surface) + rows: { + sys_position_permission_set: [{ id: 'ppr_1', permission_set_id: 'ps_gone' }], + showcase_task: [{ id: 't1', title: 'T', project: 'proj_gone' }], + }, + unreadable: new Set(['showcase_task']), + }); + const findSpy = port.find.bind(port); + port.find = async (o, opts) => { + reads.push(o); + // Teardown lands as the second listing is issued — so that listing fails + // BECAUSE the run was called off, which is the production race exactly. + if (o === 'showcase_task') signal.aborted = true; + return findSpy(o, opts); + }; + + const out = await auditDanglingReferences(port, { signal }); + + expect(reads).toEqual(['sys_position_permission_set', 'showcase_task']); + expect(out.dangling).toHaveLength(1); + expect(out.dangling[0]).toMatchObject({ recordId: 'ppr_1', value: 'ps_gone' }); + expect(out.aborted).toBe(true); + // The object the teardown cut short is NOT reported as unreadable. + expect(out.unreadableObjects).toEqual([]); + // The real finding is still reported, and the summary line carries the + // incompleteness so the log cannot read as a finished run either. + const summary = port.warnings.find((w) => w[0].includes('#4551')); + expect(summary).toBeDefined(); + expect((summary![1] as any).aborted).toBe(true); + }); + + it('a probe that fails under a teardown is not `undetermined` either', async () => { + // `undetermined` means "the probe RAN and could not tell". A withdrawn + // question did not run, so counting it there would repeat the same category + // error one bucket over. + const signal = { aborted: false }; + const port = makePort({ + objects: [binding], + rows: { sys_position_permission_set: [{ id: 'ppr_1', permission_set_id: 'ps_x' }] }, + throwingTargets: new Set(['sys_permission_set']), + }); + const probeSpy = port.probe.bind(port); + port.probe = async (target, id) => { + signal.aborted = true; + return probeSpy(target, id); + }; + + const out = await auditDanglingReferences(port, { signal }); + + expect(out.undetermined).toBe(0); + expect(out.dangling).toEqual([]); + expect(out.aborted).toBe(true); + }); + + it('a run nobody called off says so explicitly — `aborted: false`, not absent', async () => { + // The flag is a positive statement about completeness, so a consumer never + // has to guess whether `undefined` meant "finished" or "old report shape". + const port = makePort({ + objects: [binding], + rows: { sys_position_permission_set: [{ id: 'ppr_1', permission_set_id: 'ps_real' }] }, + existing: new Set(['sys_permission_set ps_real']), + }); + + const out = await auditDanglingReferences(port, { signal: { aborted: false } }); + expect(out.aborted).toBe(false); + + const noSignal = await auditDanglingReferences(port); + expect(noSignal.aborted).toBe(false); + }); +}); diff --git a/packages/objectql/src/integrity/dangling-reference-audit.ts b/packages/objectql/src/integrity/dangling-reference-audit.ts index 37a666b0e5..2cd4e931df 100644 --- a/packages/objectql/src/integrity/dangling-reference-audit.ts +++ b/packages/objectql/src/integrity/dangling-reference-audit.ts @@ -46,6 +46,30 @@ import { PLATFORM_OBJECTS_BY_PACKAGE } from '@objectstack/spec/system'; * nothing about the rows it never read. Together they are what stops * "0 dangling" from ever being read as "everything is fine". * + * ## …and NOT-ATTEMPTED is a third answer again (#4747) + * + * The corollary that bucket discipline needs to survive contact with a real + * process. `unreadableObjects` means **"I tried to read this object and the + * datasource would not give it to me"** — an operational finding. It does NOT + * mean "the run was called off". Those are different facts with different + * remedies (investigate the datasource / nothing to do), and a bucket that + * holds both stops carrying either. + * + * The distinction is not cosmetic: before #4747 every single `os migrate` + * invocation ended with `unreadableObjects: ['sys_metadata', + * 'sys_view_definition']`, because the sweep this audit rides fired after the + * engine's connection pool had been torn down. An alarm that is true on every + * healthy run carries no information and trains its operator to skip the line — + * so the ONE run where a datasource really was unreadable would have read + * exactly like all the others. + * + * Hence {@link DanglingReferenceAuditOptions.signal}: when the caller says the + * run is being called off, the audit stops issuing reads and marks the report + * {@link DanglingReferenceReport.aborted}. A read that loses the race and fails + * *after* the abort is dropped rather than filed — it is not evidence about the + * datasource. `aborted` keeps the incompleteness loud (the report can never be + * read as a clean bill of health) without spending the finding bucket on it. + * * ## Scope — the same judgments #4441 already made, not new ones * * - **`readonly` reference fields are skipped**, exactly as the write-path @@ -95,6 +119,21 @@ export interface DanglingReferenceReport { * the rows beyond the budget. */ truncatedObjects: string[]; + /** + * [#4747] `true` when the run was called off before it finished — the caller + * aborted it (see {@link DanglingReferenceAuditOptions.signal}), typically + * because the engine it reads through is being torn down. + * + * An aborted run inspected only the objects it reached, so its findings are + * real but its SILENCE proves nothing. Deliberately its own field rather than + * an entry in `unreadableObjects`: "nobody asked" is not "the datasource + * refused". `false` on a run that walked every object it was given. + * + * Optional in the type only so a hand-written {@link DanglingReferenceReport} + * (a test double) still satisfies it; every report this module produces sets + * it explicitly. + */ + aborted?: boolean; } /** Minimal object shape the audit reads — duck-typed so tests need no registry. */ @@ -124,6 +163,16 @@ export interface DanglingReferenceAuditPort { warn?(message: string, meta?: unknown): void; } +/** + * The "stop now" input, narrowed to the one bit the audit reads. Structurally + * satisfied by the platform `AbortSignal`, so a caller that already has one + * passes it directly — but declared here so this module needs no DOM lib and a + * test can hand it a plain object. + */ +export interface AuditAbortSignal { + readonly aborted: boolean; +} + export interface DanglingReferenceAuditOptions { /** Rows read per object. Default {@link DEFAULT_ROWS_PER_OBJECT}. */ rowsPerObject?: number; @@ -131,6 +180,17 @@ export interface DanglingReferenceAuditOptions { maxRows?: number; /** Restrict the scan to these objects (diagnostics / tests). */ objects?: string[]; + /** + * [#4747] Called off — checked before every read, so an aborted run issues no + * further queries and reports {@link DanglingReferenceReport.aborted} instead + * of filing the objects it never reached. + * + * The audit reads through a live engine; the engine outlives it only until + * the host tears the datasource down. Without this the sweep kept querying a + * closed pool, which surfaced as an `ERROR Find operation failed` on a + * SUCCESSFUL command and as a permanently non-empty `unreadableObjects`. + */ + signal?: AuditAbortSignal; } /** Bounded per object so one enormous table cannot starve every other. */ @@ -199,8 +259,17 @@ export async function auditDanglingReferences( ): Promise { const report: DanglingReferenceReport = { scanned: 0, dangling: [], undetermined: 0, unreadableObjects: [], truncatedObjects: [], + aborted: false, }; + const signal = options?.signal; + /** Cheap enough to ask before every read; the answer can flip mid-run. */ + const calledOff = (): boolean => signal?.aborted === true; + if (calledOff()) { + report.aborted = true; + return report; + } + let all: AuditableObject[]; try { all = port.objects() ?? []; @@ -224,13 +293,23 @@ export async function auditDanglingReferences( // it out of code search and every grep-based lint (`pnpm check:nul-bytes` // enforces this). The escape is byte-identical at runtime. const probed = new Map(); - const exists = async (target: string, value: string): Promise => { + /** + * `'called-off'` is a THIRD answer alongside the probe's own three: it means + * the question was withdrawn, so the value must not be counted as + * `undetermined` either — that bucket is for probes that ran and could not + * tell (#4747). + */ + const exists = async (target: string, value: string): Promise => { + if (calledOff()) return 'called-off'; const key = `${target}\u0000${value}`; if (probed.has(key)) return probed.get(key)!; let answer: boolean | null; try { answer = await port.probe(target, value); } catch { + // A probe that threw because the run was called off underneath it says + // nothing about the target — it is withdrawn, not undetermined. + if (calledOff()) return 'called-off'; // A throwing probe is "could not determine", never "does not exist". answer = null; } @@ -238,8 +317,11 @@ export async function auditDanglingReferences( return answer; }; - for (const obj of prioritise(all)) { + objects: for (const obj of prioritise(all)) { if (report.scanned >= maxRows) break; + // Called off before this object was read: it was never attempted, so it is + // not a finding about the object — the run reports that it stopped instead. + if (calledOff()) { report.aborted = true; break; } const name = obj?.name; if (!name || (only && !only.has(name))) continue; const refFields = auditableReferenceFields(obj); @@ -254,6 +336,11 @@ export async function auditDanglingReferences( context: { isSystem: true }, })) ?? []; } catch (err) { + // A read that failed because the run was called off underneath it is not + // evidence about the datasource — the pool was closed on purpose. Filing + // it would put a non-finding in the one bucket that must only ever hold + // findings (#4747). + if (calledOff()) { report.aborted = true; break; } // Unreadable ⇒ unknown. Recorded so the report cannot be mistaken for a // clean bill of health on an object nothing could look at. report.unreadableObjects.push(name); @@ -275,6 +362,7 @@ export async function auditDanglingReferences( // An expanded record in the slot is a read shape, not an id write. if (typeof v === 'object') continue; const answer = await exists(target, v); + if (answer === 'called-off') { report.aborted = true; break objects; } if (answer === null) { report.undetermined++; continue; } if (answer) continue; report.dangling.push({ @@ -296,6 +384,9 @@ export async function auditDanglingReferences( undetermined: report.undetermined, unreadableObjects: report.unreadableObjects, truncatedObjects: report.truncatedObjects, + // Carried into the log line too: findings from a run that stopped early + // are real, but its silence about everything else is not a verdict. + aborted: report.aborted, references: report.dangling.map( (d) => `${d.objectName}#${d.recordId}.${d.field} → ${d.target}#${d.value}`, ), diff --git a/packages/objectql/src/lifecycle/lifecycle-service.test.ts b/packages/objectql/src/lifecycle/lifecycle-service.test.ts index 588307c819..1a40f79017 100644 --- a/packages/objectql/src/lifecycle/lifecycle-service.test.ts +++ b/packages/objectql/src/lifecycle/lifecycle-service.test.ts @@ -794,7 +794,119 @@ describe('LifecycleService reference audit leg (#4551)', () => { return { scanned: 0, dangling: [], undetermined: 0, unreadableObjects: [], truncatedObjects: [] }; }; - await service(engine, { referenceAudit: { enabled: true, rowsPerObject: 7, maxRows: 21 } }).sweep(); - expect(seen).toEqual({ rowsPerObject: 7, maxRows: 21 }); + const svc = service(engine, { referenceAudit: { enabled: true, rowsPerObject: 7, maxRows: 21 } }); + await svc.sweep(); + // …and it hands the audit the teardown bit (#4747) — the audit's lifetime + // is this service's lifetime, so `signal` is the service's own, never a + // configured one. + expect(seen).toEqual({ rowsPerObject: 7, maxRows: 21, signal: { aborted: false } }); + }); +}); + +/** + * [#4747] The sweep must not outlive the engine it sweeps through. + * + * `stop()` used to clear the timers and nothing else — which was moot anyway, + * because the only caller was `ObjectQLPlugin.stop()`, a hook the kernel never + * invokes (the Plugin contract is `init`/`start`/`destroy`). So on a one-shot + * host the sweep woke 60s after boot, inside a process whose datasource had + * already been disconnected, and the #4551 audit filed the objects it could not + * read as findings — on every healthy run. + * + * Note what is NOT the fix here: switching the audit off for one-shot commands. + * That would make the audit permanently blind exactly where an operator has no + * other tool, which is the opposite of why #4551 built it. What the audit gets + * is the teardown bit, so it reads while the engine is live and stops when it + * is not. + */ +describe('LifecycleService teardown (#4747)', () => { + const AUDIT_CLEAN = { + scanned: 0, dangling: [], undetermined: 0, + unreadableObjects: [], truncatedObjects: [], aborted: false, + }; + + it('a sweep that starts after stop() reads nothing at all', async () => { + const { engine, deletes } = captureEngine([ + { name: 'sys_job_run', lifecycle: { class: 'telemetry', retention: { maxAge: '30d' } } }, + ]); + let audits = 0; + engine.inspectDanglingReferences = async () => { audits += 1; return AUDIT_CLEAN; }; + + const svc = service(engine); + svc.stop(); + const report = await svc.sweep(); + + expect(svc.stopped).toBe(true); + expect(audits).toBe(0); // no read → no `ERROR Find operation failed` + expect(deletes).toEqual([]); // and no write against a closing pool either + expect(report.swept).toEqual([]); + expect(report.danglingReferences).toBeUndefined(); + }); + + it('stop() DURING a sweep calls the audit off instead of racing it', async () => { + // The real shape of the bug: the sweep is async, so clearing a timer says + // nothing about the work already in flight. The audit is handed the bit, + // and by the time it runs it can see that teardown began. + const { engine } = captureEngine([ + { name: 'sys_job_run', lifecycle: { class: 'telemetry', retention: { maxAge: '30d' } } }, + ]); + let seenSignal: { aborted: boolean } | undefined; + engine.inspectDanglingReferences = async (opts: any) => { + seenSignal = opts?.signal; + return AUDIT_CLEAN; + }; + const svc = service(engine); + // Teardown lands while the reaping leg is running. + const original = engine.delete.bind(engine); + engine.delete = async (object: string, options: any) => { + svc.stop(); + return original(object, options); + }; + + const report = await svc.sweep(); + + // The audit leg is not entered at all once the service is stopped … + expect(seenSignal).toBeUndefined(); + expect(report.danglingReferences).toBeUndefined(); + // … and the sweep unwinds rather than issuing more work. + expect(svc.stopped).toBe(true); + }); + + it('an audit already running sees the abort through the signal it was given', async () => { + // Belt to the timing braces: even if a sweep gets into the audit leg before + // stop() lands, the object it is holding flips to aborted, so the audit + // stops reading and its report says it did not finish. + const { engine } = captureEngine([]); + const svc = service(engine); + let signalDuringAudit: { aborted: boolean } | undefined; + engine.inspectDanglingReferences = async (opts: any) => { + signalDuringAudit = opts.signal; + expect(opts.signal.aborted).toBe(false); + svc.stop(); // teardown mid-audit + return { ...AUDIT_CLEAN, aborted: opts.signal.aborted }; + }; + + const report = await svc.sweep(); + + expect(signalDuringAudit!.aborted).toBe(true); + expect(report.danglingReferences?.aborted).toBe(true); + }); + + it('stop() then start() re-arms the service — teardown is not one-way', async () => { + const { engine } = captureEngine([]); + let audits = 0; + engine.inspectDanglingReferences = async () => { audits += 1; return AUDIT_CLEAN; }; + + // A far-off first sweep: this test drives `sweep()` directly, and an armed + // 1ms timer would race its own assertion. + const svc = service(engine, { initialDelayMs: 600_000 }); + svc.stop(); + expect(svc.stopped).toBe(true); + svc.start(); + expect(svc.stopped).toBe(false); + + await svc.sweep(); + expect(audits).toBe(1); + svc.stop(); }); }); diff --git a/packages/objectql/src/lifecycle/lifecycle-service.ts b/packages/objectql/src/lifecycle/lifecycle-service.ts index 554107867b..b6da2e9f9a 100644 --- a/packages/objectql/src/lifecycle/lifecycle-service.ts +++ b/packages/objectql/src/lifecycle/lifecycle-service.ts @@ -134,8 +134,13 @@ export interface LifecycleServiceOptions { * [#4551] Referential-integrity audit tuning. The audit rides this sweep's * clock deliberately (see {@link LifecycleService.sweep}); `enabled: false` * drops that leg while leaving lifecycle enforcement alone. + * + * `signal` is deliberately NOT configurable (#4747): the audit's lifetime is + * this service's lifetime, so the abort bit comes from {@link + * LifecycleService.stop} and nowhere else. A second, caller-owned signal + * would be a second answer to "may this still read?". */ - referenceAudit?: DanglingReferenceAuditOptions & { enabled?: boolean }; + referenceAudit?: Omit & { enabled?: boolean }; } /** Per-sweep governance snapshot resolved from the `lifecycle` namespace. */ @@ -257,6 +262,14 @@ export class LifecycleService { private governance: GovernanceSnapshot = DEFAULT_GOVERNANCE; /** Per-object reap guards ({@link LifecycleReapGuard}). */ private readonly reapGuards = new Map(); + /** + * [#4747] The "the engine is going away" bit, handed to the work in flight. + * + * Replaced (never mutated back) by {@link start}, so a sweep that is still + * running when {@link stop} is called keeps the object it was given and sees + * the abort even if the service is re-armed afterwards. + */ + private abort: { aborted: boolean } = { aborted: false }; constructor(private readonly opts: LifecycleServiceOptions) { this.now = opts.now ?? (() => Date.now()); @@ -267,10 +280,22 @@ export class LifecycleService { return this.opts.enabled !== false; } + /** + * [#4747] `true` between {@link stop} and the next {@link start}: the service + * has been torn down and will neither begin a sweep nor let one in flight + * carry on. + */ + get stopped(): boolean { + return this.abort.aborted; + } + /** Arm the periodic sweep. Idempotent; timers are unref'ed so a kernel * shutdown is never held open by the lifecycle schedule. */ start(): void { if (!this.enabled || this.timer || this.initialTimer) return; + // A fresh bit per armed run — the one a previous sweep captured stays + // aborted forever, which is what makes stop() irreversible for that sweep. + this.abort = { aborted: false }; const interval = this.opts.sweepIntervalMs ?? DEFAULT_LIFECYCLE_SWEEP_MS; const initial = this.opts.initialDelayMs ?? DEFAULT_LIFECYCLE_INITIAL_DELAY_MS; this.initialTimer = setTimeout(() => { @@ -282,11 +307,27 @@ export class LifecycleService { this.initialTimer.unref?.(); } + /** + * Disarm the schedule AND call off the work. + * + * [#4747] Clearing the timers is only half of it: the sweep is async, so one + * already in flight would otherwise keep reading and deleting through an + * engine whose datasource the host is closing underneath it — the reads fail + * as `Unable to acquire a connection` and the audit files the objects it + * could not read as findings, on every single healthy run. + * + * So `stop()` raises the abort bit the running sweep captured, and the sweep + * checks it at each leg boundary. That makes teardown a fact the work can + * see, rather than a race it loses. Synchronous by contract (the kernel's + * `destroy()` awaits the caller, not this) — it does not wait for the sweep + * to unwind, it only guarantees no FURTHER work is issued. + */ stop(): void { if (this.initialTimer) clearTimeout(this.initialTimer); if (this.timer) clearInterval(this.timer); this.initialTimer = undefined; this.timer = undefined; + this.abort.aborted = true; } /** @@ -315,6 +356,10 @@ export class LifecycleService { alerts: [], }; if (this.sweeping || !this.enabled) return report; + // [#4747] Torn down ⇒ there is no engine to sweep through, whatever the + // timer that woke us thinks. A one-shot host (`os migrate`) disconnects its + // datasource on the way out; work started after that reads a closed pool. + if (this.stopped) return report; const engine = this.opts.getEngine(); if (!engine || typeof engine.delete !== 'function' || !engine.registry) { this.opts.logger.debug?.('[lifecycle] no data engine available; sweep skipped'); @@ -339,6 +384,9 @@ export class LifecycleService { const reclaimable = new Set(); for (const obj of declared) { + // [#4747] Leg boundary: stop() during the sweep ends it here rather + // than pushing more deletes at a datasource that is being closed. + if (this.stopped) return report; const lc = obj.lifecycle as Lifecycle; try { const outcomes = await this.reapObject(engine, obj, lc, report); @@ -414,9 +462,18 @@ export class LifecycleService { const cfg = this.opts.referenceAudit; if (cfg?.enabled === false) return; if (typeof engine.inspectDanglingReferences !== 'function') return; + // [#4747] Not a config switch keyed on "is this a one-shot process" — the + // audit stays wired on every host, exactly as #4551 intends. What it gets + // is the teardown bit: it reads while the engine is live and stops when the + // engine is going away, so `unreadableObjects` keeps meaning "the + // datasource refused" and nothing else. + if (this.stopped) return; try { const { enabled: _enabled, ...auditOptions } = cfg ?? {}; - report.danglingReferences = await engine.inspectDanglingReferences(auditOptions); + report.danglingReferences = await engine.inspectDanglingReferences({ + ...auditOptions, + signal: this.abort, + }); } catch (err) { this.opts.logger.warn( `[lifecycle] reference audit failed (${(err as Error)?.message ?? err})`, diff --git a/packages/objectql/src/plugin.ts b/packages/objectql/src/plugin.ts index d3e542a038..12d783e94c 100644 --- a/packages/objectql/src/plugin.ts +++ b/packages/objectql/src/plugin.ts @@ -122,7 +122,7 @@ export interface ObjectQLPluginOptions { enabled?: boolean; sweepIntervalMs?: number; initialDelayMs?: number; - referenceAudit?: DanglingReferenceAuditOptions & { enabled?: boolean }; + referenceAudit?: Omit & { enabled?: boolean }; }; } @@ -574,15 +574,30 @@ export class ObjectQLPlugin implements Plugin { this.lifecycleService?.start(); } - stop = async (ctx: PluginContext) => { - // ADR-0057: disarm the lifecycle sweep timers. + /** + * Kernel teardown. + * + * **This used to be `stop()`, which the kernel never calls** (#4747). The + * Plugin contract is `init` / `start` / `destroy` — `packages/core/src/ + * types.ts`, and `DefaultDatasourcePlugin.destroy` says so in as many words + * ("`stop()` exists nowhere in the Plugin contract and is never called"). + * So the one line that disarmed the ADR-0057 sweep never ran, on any host: + * the timers outlived the engine, and 60s after a one-shot `os migrate` + * boot the sweep woke up and queried a datasource its own host had already + * disconnected — an `ERROR Find operation failed` on a SUCCESSFUL command, + * and two objects filed as `unreadableObjects` by the #4551 audit on every + * healthy run. A hook nobody calls is not defence in depth; it is the + * absence of defence, spelled like its presence. + */ + destroy = async () => { + // ADR-0057: disarm the sweep timers AND call off a sweep in flight, before + // the datasource plugin (destroyed after us — reverse registration order) + // closes the pool underneath it. this.lifecycleService?.stop(); - // ADR-0008 PR-7: tear down metadata subscriptions on plugin stop so - // tests don't leak watchers and reloaded plugins don't double-subscribe. + // ADR-0008 PR-7: tear down metadata subscriptions on teardown so tests + // don't leak watchers and reloaded plugins don't double-subscribe. for (const unsub of this.metadataUnsubscribes) { - try { unsub(); } catch (e: any) { - ctx.logger.debug('[ObjectQLPlugin] metadata-event unsubscribe failed', { error: e?.message }); - } + try { unsub(); } catch { /* teardown is best-effort — the kernel is going away */ } } this.metadataUnsubscribes = []; } From a89f712708c45f507d0831d24f4e6b3211af1365 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 08:35:48 +0000 Subject: [PATCH 2/2] =?UTF-8?q?chore(changeset):=20objectql=20=E5=AE=9A?= =?UTF-8?q?=E7=BA=A7=20minor=20=E2=80=94=E2=80=94=20=E6=9C=AC=20PR=20?= =?UTF-8?q?=E6=96=B0=E5=A2=9E=E4=BA=86=E5=85=AC=E5=85=B1=20API?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AuditAbortSignal` 从包根导出,加上 `DanglingReferenceAuditOptions.signal` 与 `DanglingReferenceReport.aborted` 两个新字段:新增公开面按仓内先例 (#4791 因新增 `sealNodeTypeVocabulary()` 等公共 API 定 minor)是 minor, 不是 patch。`@objectstack/cli` 只动了内部的 schema-migrate.ts,维持 patch。 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny --- .changeset/hungry-donkeys-repeat.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/hungry-donkeys-repeat.md b/.changeset/hungry-donkeys-repeat.md index 586bd8d3b9..96d121fe33 100644 --- a/.changeset/hungry-donkeys-repeat.md +++ b/.changeset/hungry-donkeys-repeat.md @@ -1,5 +1,5 @@ --- -'@objectstack/objectql': patch +'@objectstack/objectql': minor '@objectstack/cli': patch ---