diff --git a/.changeset/dangling-audit-unscanned-bucket.md b/.changeset/dangling-audit-unscanned-bucket.md new file mode 100644 index 0000000000..0e049a65e0 --- /dev/null +++ b/.changeset/dangling-audit-unscanned-bucket.md @@ -0,0 +1,48 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): the dangling-reference audit names the objects a finite row +budget never reached, instead of dropping them (#5718) + +`auditDanglingReferences` stops the moment `report.scanned >= maxRows` +(default 5 000). Every object behind that stop was never opened — and left no +trace in the report at all. Its `dangling: []` was indistinguishable from the +`dangling: []` of a table that really was read and really was clean, which is +the one reading this module is written to prevent: it already carries +`truncatedObjects` (the budget ran out INSIDE a table), `unreadableObjects` +(the datasource refused) and `aborted` (#4747, the run was called off), and the +module header says outright that together they are what stops `0 dangling` from +being read as `everything is fine`. Object-level budget exhaustion was the one +incompleteness path with no bucket of its own. + +| Key | Means | +|:--|:--| +| `unscannedObjects: string[]` | objects this run never opened — the overall row budget was gone before their turn, or the run was called off first | + +Notes on the shape: + +- **Names, in scan order** (the `prioritise` tiers), so a caller can feed them + straight back as `options.objects` to finish the picture on a second run. +- **Optional in the type, always set at runtime** — same contract as `aborted` + and #4743's `provenance`: a hand-written report literal (a test double) still + compiles, while every report this module produces states the key explicitly, + `[]` included. `undefined` therefore never has to be guessed at; it can only + mean "a report shape that predates the key", never "complete". +- **Two things are deliberately absent from it.** Objects with no reference + field at all (nothing referential can break on them, so their silence is + proven rather than assumed) and objects the caller excluded via + `options.objects` (that is the caller's own narrowing, not the audit missing + something). +- **It does not raise the summary warning on its own**, exactly like + `truncatedObjects` and for the reason #4747 wrote down: a large database + exhausts a finite budget on every healthy run, and an alarm that always fires + trains its reader past the run that had a real finding. It rides itemised in + the warning payload and is always in the returned report. + +#4743 did not cause this — it made it easy to reach. Admitting the provenance +family means nearly every object now has an auditable field, so a bounded run +spreads the same budget over far more tables and hits the stop sooner. The +three-tier scan order that shipped with it decides WHO gets a finite budget; +this bucket reports who got none. Only the second one can make a bounded run +honest. diff --git a/packages/objectql/src/integrity/dangling-reference-audit.test.ts b/packages/objectql/src/integrity/dangling-reference-audit.test.ts index 378dfd0e3b..261af66b22 100644 --- a/packages/objectql/src/integrity/dangling-reference-audit.test.ts +++ b/packages/objectql/src/integrity/dangling-reference-audit.test.ts @@ -20,6 +20,8 @@ * - restore the readonly SKIP → every [#4743] test below fails: the provenance * bucket goes empty and the probe is never issued * - merge provenance into `dangling` → the [#4743] separation tests fail + * - drop the `unscannedObjects` filing → the [#5718] tests fail: the budget + * stop goes back to being silent about the objects it never opened */ import { describe, it, expect } from 'vitest'; @@ -738,3 +740,227 @@ describe('[#4743] provenance references are audited, in their OWN bucket', () => expect(out.provenanceUndetermined).toBe(0); }); }); + +/** + * [#5718] A budget that runs out BETWEEN objects used to end the run in + * silence. + * + * `truncatedObjects` covers the budget running out INSIDE a table. The overall + * `maxRows` budget also runs out between tables — the loop just stopped, and + * every object behind the stop left no trace in the report at all. Their + * `dangling: []` was indistinguishable from the `dangling: []` of a table that + * really was read and really was clean, which is the exact confusion every + * bucket in this file exists to prevent. + * + * Reverse verification, direction predicted BEFORE running it: delete the + * `fileUnscanned` call at the budget stop and every test in this block goes + * RED — the list empties while the run keeps returning `dangling: []`. Note + * which assertion does the work: a test that only checked `dangling` (or + * `scanned`) would stay GREEN under the old silent break, because nothing about + * the findings changes. The names ARE the behaviour under test. + */ +describe('[#5718] objects a finite budget never reached are named, not dropped', () => { + /** Every object below carries a reference field, so all three are scannable. */ + const budgetPort = (objects: AuditableObject[]) => makePort({ + objects, + rows: { + sys_position_permission_set: [{ id: 'ppr_1', permission_set_id: 'ps_real' }], + showcase_task: [{ id: 't1', title: 'T', project: 'proj_real' }], + showcase_note: [{ id: 'n1', body: 'b', created_by: 'usr_alive' }], + }, + existing: new Set([ + 'sys_permission_set ps_real', 'showcase_project proj_real', 'sys_user usr_alive', + ]), + }); + + it('the objects behind the stop are listed, in the order the run would have read them', async () => { + // Registration order is the worst case (provenance-only first, security + // surface last) so the list can only be right if it comes from the SCAN + // order — which is what makes it feedable straight back as `objects` on a + // second run. + const reads: string[] = []; + const port = budgetPort([note, task, binding]); + const findSpy = port.find.bind(port); + port.find = async (o, opts) => { reads.push(o); return findSpy(o, opts); }; + + const out = await auditDanglingReferences(port, { maxRows: 1 }); + + expect(reads).toEqual(['sys_position_permission_set']); // budget bought one table + expect(out.scanned).toBe(1); + // …and the report SAYS the other two were never opened, instead of leaving + // them inside an empty `dangling`. + expect(out.unscannedObjects).toEqual(['showcase_task', 'showcase_note']); + expect(out.dangling).toEqual([]); + }); + + it('a run that reached everything says so explicitly — `[]`, never absent', async () => { + // Same reason `aborted: false` is explicit: `undefined` must never be the + // consumer's only clue, because it cannot tell "complete" from "a report + // shape that predates the key". + const port = budgetPort([binding, task, note]); + + const out = await auditDanglingReferences(port); + + expect(out.unscannedObjects).toEqual([]); + expect('unscannedObjects' in out).toBe(true); + }); + + it('an object with NO reference field is not listed — its silence was already proven', async () => { + // Nothing referential can break on it, so `prioritise` drops it before the + // loop and the audit reads zero rows of it by design. Filing it as + // "unscanned" would report a table that has nothing to find as a gap. + const plain: AuditableObject = { + name: 'plain', fields: { id: { type: 'text' }, n: { type: 'number' } }, + }; + const port = budgetPort([binding, plain, task]); + + const out = await auditDanglingReferences(port, { maxRows: 1 }); + + expect(out.unscannedObjects).toEqual(['showcase_task']); + expect(out.unscannedObjects).not.toContain('plain'); + }); + + it('an object the CALLER excluded is not listed — that narrowing is not the audit missing it', async () => { + const port = budgetPort([binding, task, note]); + + const out = await auditDanglingReferences(port, { + maxRows: 1, + objects: ['sys_position_permission_set', 'showcase_task'], + }); + + // `showcase_note` was never on this run's list, so it is not something the + // run failed to reach; `showcase_task` was, and the budget ate it. + expect(out.unscannedObjects).toEqual(['showcase_task']); + }); + + it('a zero budget reads nothing and names EVERYTHING — the whole report is "not looked at"', async () => { + const reads: string[] = []; + const port = budgetPort([binding, task, note]); + const findSpy = port.find.bind(port); + port.find = async (o, opts) => { reads.push(o); return findSpy(o, opts); }; + + const out = await auditDanglingReferences(port, { maxRows: 0 }); + + expect(reads).toEqual([]); + expect(out.scanned).toBe(0); + expect(out.dangling).toEqual([]); // …and this says NOTHING, which is the point + expect(out.unscannedObjects).toEqual([ + 'sys_position_permission_set', 'showcase_task', 'showcase_note', + ]); + }); + + it('a called-off run names what it never reached too, so empty keeps meaning "reached"', async () => { + // The bucket would be a liar otherwise: `unscannedObjects: []` next to + // `aborted: true` reads as "stopped early, but missed nothing". The object + // whose listing the teardown cut off is in the list as well — its rows were + // never examined either (#4747 keeps it out of `unreadableObjects`, which + // is a different fact: the datasource refused). + const signal = { aborted: false }; + const port = makePort({ + objects: [binding, task, note], + rows: { sys_position_permission_set: [{ id: 'ppr_1', permission_set_id: 'ps_gone' }] }, + unreadable: new Set(['showcase_task']), + }); + const findSpy = port.find.bind(port); + port.find = async (o, opts) => { + if (o === 'showcase_task') signal.aborted = true; // the pool closes mid-query + return findSpy(o, opts); + }; + + const out = await auditDanglingReferences(port, { signal }); + + expect(out.aborted).toBe(true); + expect(out.unreadableObjects).toEqual([]); + expect(out.unscannedObjects).toEqual(['showcase_task', 'showcase_note']); + // The finding made before the teardown is untouched by any of this. + expect(out.dangling).toHaveLength(1); + }); + + it('called off MID-object lists what is BEHIND it, not the object it was inside', async () => { + // The `i` vs `i + 1` distinction, pinned: this object was opened and partly + // examined — its findings so far are real and `aborted` is what says the + // rest of it is unreliable. Listing it as "never reached" would be a + // different lie from the one #5718 fixes. + const signal = { aborted: false }; + const port = makePort({ + objects: [binding, task, note], + rows: { sys_position_permission_set: [{ id: 'ppr_1', permission_set_id: 'ps_x' }] }, + // The probe blows up because the pool went away under it — the #4747 + // race, which is what makes the answer "withdrawn" rather than a verdict. + 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.aborted).toBe(true); + expect(out.unscannedObjects).toEqual(['showcase_task', 'showcase_note']); + expect(out.unscannedObjects).not.toContain('sys_position_permission_set'); + }); + + it('called off BEFORE the registry was enumerated: `aborted` carries it, the list is empty', async () => { + // The one boundary of "empty means reached", pinned rather than left to be + // discovered: this run never asked which objects exist, so it has no names + // to give. Completeness is read from `aborted === false` AND an empty list, + // exactly as `truncatedObjects` / `unreadableObjects` are already composed. + const port = budgetPort([binding, task, note]); + + const out = await auditDanglingReferences(port, { signal: { aborted: true } }); + + expect(out.aborted).toBe(true); + expect(out.unscannedObjects).toEqual([]); + }); + + it('a truncated run and an unscanned run are DIFFERENT reports', async () => { + // The distinction this bucket exists for: `truncatedObjects` = "this table + // was sampled", `unscannedObjects` = "this table was not opened". Before + // #5718 the second case had no field of its own and borrowed nothing — + // it simply vanished. + const port = makePort({ + objects: [task, note], + rows: { + showcase_task: [ + { id: 't1', title: 'A', project: 'proj_real' }, + { id: 't2', title: 'B', project: 'proj_real' }, + ], + showcase_note: [{ id: 'n1', body: 'b', created_by: 'usr_alive' }], + }, + existing: new Set(['showcase_project proj_real', 'sys_user usr_alive']), + }); + + // Budget of 2 rows: `showcase_task` is read to its budget (sampled), and + // `showcase_note` never comes up at all. + const out = await auditDanglingReferences(port, { maxRows: 2, rowsPerObject: 2 }); + + expect(out.truncatedObjects).toEqual(['showcase_task']); + expect(out.unscannedObjects).toEqual(['showcase_note']); + }); + + it('rides along in the warning payload, and does NOT raise the line on its own', async () => { + // Same judgement as `truncatedObjects` and as #4743's provenance bucket: a + // large database exhausts a finite budget on EVERY healthy run, so an alarm + // fired by this alone would be #4747's broken alarm — the reader would be + // trained straight past the run that had a real finding in it. + const quiet = budgetPort([binding, task, note]); + await auditDanglingReferences(quiet, { maxRows: 1 }); + expect(quiet.warnings).toEqual([]); // budget exhausted, nothing found, silence + + const loud = makePort({ + objects: [binding, task, note], + rows: { + sys_position_permission_set: [{ id: 'ppr_1', permission_set_id: 'ps_gone' }], + showcase_task: [{ id: 't1', title: 'T', project: 'proj_gone' }], + showcase_note: [{ id: 'n1', body: 'b', created_by: 'usr_deleted' }], + }, + }); + await auditDanglingReferences(loud, { maxRows: 1 }); + + const summary = loud.warnings.find((w) => w[0].includes('#4551')); + expect(summary).toBeDefined(); + const meta = summary![1] as Record; + // Itemised, not merely counted: object-scale, and a reader who has to act + // on it needs the names to re-run them. + expect(meta.unscannedObjects).toEqual(['showcase_task', 'showcase_note']); + }); +}); diff --git a/packages/objectql/src/integrity/dangling-reference-audit.ts b/packages/objectql/src/integrity/dangling-reference-audit.ts index 19cdec1e63..8350a44aaf 100644 --- a/packages/objectql/src/integrity/dangling-reference-audit.ts +++ b/packages/objectql/src/integrity/dangling-reference-audit.ts @@ -120,6 +120,30 @@ import { PLATFORM_OBJECTS_BY_PACKAGE } from '@objectstack/spec/system'; * {@link DanglingReferenceReport.provenanceUndetermined}, on its own side of * the same line. * + * ## …and NEVER-REACHED is the last incompleteness (#5718) + * + * `truncatedObjects` says the budget ran out INSIDE a table. The overall budget + * ({@link DanglingReferenceAuditOptions.maxRows}) also runs out BETWEEN tables: + * the object loop simply stops, and before #5718 every object behind that stop + * left no trace in the report at all. `dangling: []` covered them exactly as it + * covered the tables that WERE read and found clean — which is precisely the + * reading this whole file is written to prevent. + * {@link DanglingReferenceReport.unscannedObjects} names them, so the report + * keeps saying "not looked at" wherever it cannot say "looked at and clean". + * + * #4743 did not cause this, it made it easy to reach: admitting the provenance + * family means nearly every object now has an auditable field, so a bounded run + * spreads the same budget over far more tables and hits the stop sooner. Note + * that `prioritise` and this bucket answer different questions — the tiers + * decide WHO gets a finite budget, the bucket reports who did not get any. A + * scan order cannot make a bounded run complete; only saying what it missed + * can make it honest. + * + * Like `truncatedObjects`, it does **not** raise the summary warning by itself. + * A large database exhausts the default 5 000-row budget on every healthy run, + * and an alarm that always fires is #4747's broken alarm again. It rides along + * in the warning payload and is always present in the returned report. + * * ## Scope — the same judgments #4441 already made, not new ones * * - **Which fields are references** is `referenceTargetOf` — the single @@ -210,6 +234,45 @@ export interface DanglingReferenceReport { * disguised as a fact about the audited data. */ provenanceUndetermined?: number; + /** + * [#5718] Objects this run never reached — the overall row budget + * ({@link DanglingReferenceAuditOptions.maxRows}) was gone before their turn, + * or the run was called off first. **Not one of their rows was read**, so the + * report holds no verdict about them whatsoever. + * + * The sibling of `truncatedObjects`, one level up: that one says "this table + * was sampled", this one says "this table was not opened". The two together + * are what keeps `dangling: []` from ever meaning more than "clean among what + * was read" — the difference between them is only whether the budget ran out + * inside a table or before it. + * + * Names are in **scan order** (the `prioritise` tiers), so a caller that + * wants the rest of the picture can feed them straight back as + * {@link DanglingReferenceAuditOptions.objects} on a second run. + * + * Two things are deliberately NOT in here, because neither is something this + * run failed to look at: + * + * - **Objects with no reference field at all.** Nothing referential can + * break on them, so their absence from `dangling` is proven rather than + * assumed — `prioritise` drops them before the loop ever sees them. + * - **Objects the caller excluded** via + * {@link DanglingReferenceAuditOptions.objects}. Those were never asked + * for; filing them would report the caller's own narrowing back to it as + * an incompleteness of the audit. + * + * Empty means every object this run had queued was reached — with one + * boundary that {@link DanglingReferenceReport.aborted} covers instead: a run + * called off before it enumerated the registry has no list to give, so a + * consumer reads completeness from `aborted === false` AND this being empty, + * the same way it already composes `truncatedObjects` and `unreadableObjects`. + * + * Optional in the type only, like {@link DanglingReferenceReport.aborted} and + * for the same reason (a hand-written report literal predates the key); every + * report this module produces sets it explicitly — `[]` on a run that reached + * everything, never absent, so `undefined` cannot be misread as "complete". + */ + unscannedObjects?: string[]; } /** Minimal object shape the audit reads — duck-typed so tests need no registry. */ @@ -379,9 +442,11 @@ export async function auditDanglingReferences( // predate them); every report this function produces sets them, so the local // view of it requires them and no call site below has to guard. const report: DanglingReferenceReport & - Required> = { + Required> = { scanned: 0, dangling: [], undetermined: 0, unreadableObjects: [], truncatedObjects: [], provenance: [], provenanceUndetermined: 0, + unscannedObjects: [], aborted: false, }; @@ -440,11 +505,34 @@ export async function auditDanglingReferences( return answer; }; - objects: for (const { obj, refFields } of prioritise(all)) { - if (report.scanned >= maxRows) break; + // Materialised rather than iterated inline (#5718): once the loop stops early + // the report has to name what is BEHIND the stop, which needs the queue and + // the position in it, not just the current element. + const targets = prioritise(all); + /** + * [#5718] File every object from `from` onward as never-reached. Applies the + * caller's `only` narrowing for the same reason the loop does: an object the + * caller excluded was not missed by this run, it was never on its list. + */ + const fileUnscanned = (from: number): void => { + for (let i = from; i < targets.length; i++) { + const n = targets[i]?.obj?.name; + if (!n || (only && !only.has(n))) continue; + report.unscannedObjects.push(n); + } + }; + + objects: for (let i = 0; i < targets.length; i++) { + const { obj, refFields } = targets[i]!; + // Budget gone BETWEEN objects. This one and everything behind it is never + // opened, and `dangling: []` about a table nobody read is not a verdict — + // so the run names them instead of stopping in silence (#5718). + if (report.scanned >= maxRows) { fileUnscanned(i); 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; } + // The objects behind the stop are just as unread as a budget stop leaves + // them, so they are filed the same way; `aborted` records WHY it stopped. + if (calledOff()) { report.aborted = true; fileUnscanned(i); break; } const name = obj?.name; if (!name || (only && !only.has(name))) continue; @@ -460,8 +548,9 @@ export async function auditDanglingReferences( // 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; } + // findings (#4747). Its rows were never examined either, so THIS object + // is filed as unreached too — `from = i`, not `i + 1` (#5718). + if (calledOff()) { report.aborted = true; fileUnscanned(i); 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); @@ -483,7 +572,13 @@ 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; } + // Called off mid-object: this one WAS opened and partly examined, so + // it is not unreached — only the objects behind it are (#5718). + if (answer === 'called-off') { + report.aborted = true; + fileUnscanned(i + 1); + break objects; + } // [#4743] Both verdicts are routed by the field's class, not merged: // a provenance answer never lands in a bucket a business reference // shares, in EITHER direction (absent or unknown). @@ -518,6 +613,12 @@ export async function auditDanglingReferences( undetermined: report.undetermined, unreadableObjects: report.unreadableObjects, truncatedObjects: report.truncatedObjects, + // [#5718] Itemised like `truncatedObjects` and for the same reason: this + // is object-scale, not row-scale, and a reader who has to act on it needs + // the names to re-run them. It does not appear in the condition above — + // a bounded run on a large database exhausts its budget every time, and + // an alarm that always fires is #4747's broken alarm. + unscannedObjects: report.unscannedObjects, // 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,