diff --git a/.changeset/share-link-record-existence.md b/.changeset/share-link-record-existence.md new file mode 100644 index 0000000000..b212d33fc1 --- /dev/null +++ b/.changeset/share-link-record-existence.md @@ -0,0 +1,52 @@ +--- +"@objectstack/plugin-sharing": patch +--- + +fix(plugin-sharing): a deleted record kills its share links — resolve fails closed, and the delete cascades (#5190) + +`ShareLinkService.resolveToken` checked the token, `revoked_at`, `expires_at`, +the audience and the password — **but never whether the record the link points +at still exists**. Nothing revoked links on delete either: #5103's cascade +covers `sys_record_share` only. So a share link outlived its record, kept +resolving, and kept stamping `use_count` / `last_used_at`. + +That is worse than the `sys_record_share` orphan #5103 fixed, and for a +structural reason: a share row names its beneficiaries, while a share link is an +identity-less **capability token** — holding the URL *is* the authorisation. The +moment a record id is reused (custom primary keys, an import that preserves ids, +any future id recycling) a link that morally died with its record starts +authorising a brand-new record, for whoever kept it. + +Both halves of the fix ship together, and the first does not depend on the +second having run: + +- **`resolveToken` re-asks whether the record exists**, and returns `null` + through the *same* branch as revoked / expired — no distinct code, no distinct + error, nothing an unauthorised holder can read "that record was deleted" out + of. The probe sits after the cheap in-memory gates (a revoked link still costs + no query) and *before* the usage stamp, so a dead record no longer ticks + `use_count` / `last_used_at`. It fails **closed**: a probe that throws denies, + because "cannot ask" must not authorise. +- **Record deletes now cascade to `sys_share_link`**, on #5103's existing seam + rather than a parallel one — the same global `beforeDelete` row-set stash, the + same `afterDelete` set-based revoke, the same serialized sweep queue for + unbounded deletes, and the same `kernel:bootstrapped` orphan sweep (keyset + pages, a scan cap that reports itself, one batched existence probe per object + per page, and rows left strictly alone when that probe fails). The two halves + are isolated, so a driver error reclaiming grants cannot also skip the tokens. + +The link half judges posture from `publicSharing`, which is *independent* of +`sharingModel`: the object most likely to hold links is a platform object that +opted into link sharing, and that is exactly the object the record-share +predicate skips. `publicSharing` declared counts even when it is currently +`enabled: false` — links minted while it was on outlive the flip. + +An orphaned link row is **deleted**, not stamped `revoked_at`: its subject is +gone, so there is no live link left to keep a revocation record of, and the +table would otherwise only grow (with Setup's link lists pointing at records +that no longer exist). Links an admin revokes keep their audit row exactly as +before. + +No metadata, spec or API shape changes. Deployments see fewer rows in +`sys_share_link` after the next boot, and links whose record was already deleted +stop resolving immediately — which is the point. diff --git a/packages/plugins/plugin-sharing/src/index.ts b/packages/plugins/plugin-sharing/src/index.ts index bda00f31c9..ca248d4ba9 100644 --- a/packages/plugins/plugin-sharing/src/index.ts +++ b/packages/plugins/plugin-sharing/src/index.ts @@ -57,10 +57,21 @@ export { bindRecordShareCascade, unbindRecordShareCascade, objectCanCarryRecordShares, + objectCanCarryShareLinks, orphanShareSweepQueue, RECORD_SHARE_CASCADE_PACKAGE, type CascadeEngine, + type ShareLinkCascade, } from './record-share-cascade.js'; +export { + deleteRowsForDeletedRecords, + sweepOrphanedRowsByRecordExistence, + ORPHAN_SWEEP_PAGE_SIZE, + ORPHAN_SWEEP_MAX_ROWS, + RECORD_SCOPED_DELETE_CHUNK, + type OrphanCleanupEngine, + type OrphanSweepSubject, +} from './record-orphan-cleanup.js'; export { parseCriteria, isMatchAllCriteria, diff --git a/packages/plugins/plugin-sharing/src/record-orphan-cleanup.ts b/packages/plugins/plugin-sharing/src/record-orphan-cleanup.ts new file mode 100644 index 0000000000..42db8b705e --- /dev/null +++ b/packages/plugins/plugin-sharing/src/record-orphan-cleanup.ts @@ -0,0 +1,299 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5103 / #5190] The record-existence cleanup primitives. + * + * Two tables in this package store "(`object_name`, `record_id`) → some + * access": `sys_record_share` (principal-based grants) and `sys_share_link` + * (capability tokens). Both have the SAME invariant — **record gone ⇒ the row + * cannot describe any access at all** — and therefore the same two operations: + * + * - a set-based revoke keyed on the ids a delete just removed, and + * - a sweep that asks, per row, whether its record still exists. + * + * #5103 built both for `sys_record_share`. #5190 needs them for + * `sys_share_link`, and a second copy would be the fork this module exists to + * prevent: one chunk size, one keyset walk, one "a failed probe deletes + * NOTHING" rule, one truncation report. The tables' owning services keep their + * own public methods (`SharingService.sweepOrphanedRecordShares`, + * `ShareLinkService.sweepOrphanedShareLinks`) — `sys_share_link` is + * `managedBy: 'engine-owned'` and its writes flow through `IShareLinkService`, + * so ownership stays where the object declares it; only the mechanism is + * shared. + * + * Nothing here knows what a share or a link MEANS. It knows a table name, an + * `(object_name, record_id)` pair per row, and that deleting on an unanswered + * question is the one thing it must never do. + */ + +import { keysetWalk } from '@objectstack/types'; + +/** System-elevated context for the plugin's own queries / mutations. */ +const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; + +/** The slice of the engine these primitives need. */ +export interface OrphanCleanupEngine { + find(object: string, options?: any): Promise; + delete(object: string, options?: any): Promise; +} + +/** + * [#5103] Ids per `$in`. Mirrors the chunk + * `SharingRuleService.revokeRuleGrantsForRecords` already uses: a single + * statement binding a thousand parameters is a portability trap (SQLite's + * default `SQLITE_MAX_VARIABLE_NUMBER` is 999 on older builds), and one number + * for every revoke path keeps them from drifting. + */ +export const RECORD_SCOPED_DELETE_CHUNK = 200; + +/** [#5103] Rows read per page by an orphan sweep. */ +export const ORPHAN_SWEEP_PAGE_SIZE = 500; + +/** + * [#5103] Rows one sweep will scan before stopping and reporting truncation. + * The sweep runs on every boot, so it must cost a bounded amount on a table + * that only grows; the next boot resumes from the start and the rows it did not + * reach stay reachable by the object-scoped sweep. A cap is not a failure — but + * an unreported cap turns a partial scan into a false "nothing to clean", which + * is why {@link OrphanShareSweepResult} carries it. + */ +export const ORPHAN_SWEEP_MAX_ROWS = 50_000; + +/** [#5103] Options for a record-existence orphan sweep. */ +export interface OrphanShareSweepOptions { + /** Restrict the sweep to one object. Default: every object with rows. */ + object?: string; + /** Rows per page. Default {@link ORPHAN_SWEEP_PAGE_SIZE}. */ + batchSize?: number; + /** Stop after scanning this many rows. Default {@link ORPHAN_SWEEP_MAX_ROWS}. */ + max?: number; +} + +/** [#5103] What one orphan-sweep pass did. */ +export interface OrphanShareSweepResult { + /** Rows examined. */ + scanned: number; + /** Rows revoked because their record no longer exists. */ + revoked: number; + /** + * Objects whose existence probe could not be run (unregistered object, + * driver error). Their rows were LEFT ALONE — "could not ask" is not + * "the record is gone", and only the second one may delete anything. + */ + unresolvedObjects: string[]; + /** True when {@link OrphanShareSweepOptions.max} stopped the scan early. */ + truncated: boolean; +} + +/** + * Structural and loose on purpose — it has to accept both owning services' + * option shapes (`SharingServiceOptions['logger']`, `ShareLinkServiceOptions`) + * and a bare `{ warn }` stub in a test. + */ +interface MinimalLogger { + info?: Function; + warn?: Function; +} + +/** How one caller's rows are named in this module's log lines. */ +export interface OrphanSweepSubject { + /** Table to sweep, e.g. `sys_record_share`. */ + table: string; + /** Noun for log messages: `share` → "orphan share sweep", "share rows". */ + noun: string; + /** Issue reference appended to the "revoked N rows" warning. */ + issue: string; +} + +/** + * [#5103] Delete every row of `table` that belongs to records just deleted from + * `object`. + * + * Set-based and chunked, so its cost tracks the number of ids, not the number + * of rows. Returns nothing: counting would need a read the hot delete path + * should not pay for, and callers that need a count (tests, the sweep) can read + * the table. + */ +export async function deleteRowsForDeletedRecords( + engine: OrphanCleanupEngine, + table: string, + object: string, + recordIds: readonly string[], +): Promise { + if (!table || !object || recordIds.length === 0) return; + for (let i = 0; i < recordIds.length; i += RECORD_SCOPED_DELETE_CHUNK) { + const batch = recordIds.slice(i, i + RECORD_SCOPED_DELETE_CHUNK); + await engine.delete(table, { + where: { object_name: object, record_id: { $in: batch } }, + multi: true, + context: SYSTEM_CTX, + } as any); + } +} + +/** + * [#5103] Which of `recordIds` still exist on `object`. Batched by + * {@link RECORD_SCOPED_DELETE_CHUNK} so the `$in` never outgrows a driver's + * bind-parameter limit. Throws on a query failure — the caller MUST treat that + * as "unknown", never as "none of them exist". + */ +export async function findLiveRecordIds( + engine: OrphanCleanupEngine, + object: string, + recordIds: readonly string[], +): Promise> { + const live = new Set(); + for (let i = 0; i < recordIds.length; i += RECORD_SCOPED_DELETE_CHUNK) { + const batch = recordIds.slice(i, i + RECORD_SCOPED_DELETE_CHUNK); + const rows = await engine.find(object, { + where: { id: { $in: batch } }, + fields: ['id'], + limit: batch.length, + context: SYSTEM_CTX, + }); + for (const row of (rows ?? [])) { + if ((row as any)?.id != null) live.add(String((row as any).id)); + } + } + return live; +} + +/** [#5103] Set-based delete of rows by id, chunked like the revoke. */ +export async function deleteRowsByIds( + engine: OrphanCleanupEngine, + table: string, + rowIds: readonly string[], +): Promise { + for (let i = 0; i < rowIds.length; i += RECORD_SCOPED_DELETE_CHUNK) { + const batch = rowIds.slice(i, i + RECORD_SCOPED_DELETE_CHUNK); + await engine.delete(table, { + where: { id: { $in: batch } }, + multi: true, + context: SYSTEM_CTX, + } as any); + } +} + +/** + * [#5103] Remove every row of `subject.table` whose RECORD no longer exists. + * + * The convergence half of the record-delete cascade, and the shape + * `SharingRuleService.sweepOrphanedRuleGrants` (#4433) established — with a + * different predicate, which is the whole point: that sweep asks "does the RULE + * row still exist", so it can never see a manual share, nor a rule grant whose + * rule is alive and whose record is not. This one asks "does the RECORD still + * exist", which is the question the invariant is actually made of, and it is + * source-agnostic (and, for `sys_share_link`, holder-agnostic). + * + * Two callers, one primitive: + * - `kernel:bootstrapped`, unscoped — historical orphans from before the + * cascade existed, plus anything a crashed hook missed, converge on the next + * boot; + * - the cascade's unbounded-delete branch, scoped to one object — a bulk + * delete whose row set could not be enumerated cannot name the ids to + * revoke, but the sweep does not need them: it reads the rows and asks about + * each record. This is deliberately NOT the rule path's "revoke everything on + * the object and re-grant asynchronously" — that trade is only available + * where a reconcile can put the grants back, and nothing can re-create a + * manual share or re-mint a link someone already holds. + * + * Bounded on both axes: rows are read by keyset page (never `OFFSET`, which + * skips rows in a walk that deletes as it goes — #4363), the scan stops at + * `max` and SAYS so, and existence is probed one batched `id IN (…)` per object + * per page rather than one query per row. + * + * Fails SAFE per object: a probe that throws leaves that object's rows + * untouched and is reported in `unresolvedObjects`. "Nothing was queried" is not + * "nothing matched" — deleting on a failed probe would turn a transient driver + * error into permanent access loss. (The RESOLVE path fails the other way, and + * for the same reason: there, "cannot ask" must not grant. Both refuse to act on + * an unanswered question; only the safe direction differs.) + */ +export async function sweepOrphanedRowsByRecordExistence( + engine: OrphanCleanupEngine, + subject: OrphanSweepSubject, + options?: OrphanShareSweepOptions, + logger?: MinimalLogger, +): Promise { + const result: OrphanShareSweepResult = { + scanned: 0, + revoked: 0, + unresolvedObjects: [], + truncated: false, + }; + const unresolved = new Set(); + const walk = keysetWalk( + (q) => engine.find(subject.table, { + ...q, + fields: ['id', 'object_name', 'record_id'], + context: SYSTEM_CTX, + }), + { + where: options?.object ? { object_name: options.object } : undefined, + pageSize: Math.max(1, options?.batchSize ?? ORPHAN_SWEEP_PAGE_SIZE), + max: options?.max ?? ORPHAN_SWEEP_MAX_ROWS, + }, + ); + + try { + for await (const page of walk.pages()) { + result.scanned += page.length; + + // Group the page by object so existence is one probe per object, not + // one per row. + const byObject = new Map>(); + for (const row of page) { + const objectName = row?.object_name == null ? '' : String(row.object_name); + const recordId = row?.record_id == null ? '' : String(row.record_id); + const rowId = row?.id == null ? '' : String(row.id); + if (!objectName || !recordId || !rowId) continue; + const perRecord = byObject.get(objectName) ?? new Map(); + const rowIds = perRecord.get(recordId) ?? []; + rowIds.push(rowId); + perRecord.set(recordId, rowIds); + byObject.set(objectName, perRecord); + } + + for (const [objectName, perRecord] of byObject) { + if (unresolved.has(objectName)) continue; + const recordIds = [...perRecord.keys()]; + let live: Set; + try { + live = await findLiveRecordIds(engine, objectName, recordIds); + } catch (err: any) { + unresolved.add(objectName); + logger?.warn?.( + `[sharing] orphan ${subject.noun} sweep could not check whether records still exist — ` + + `its ${subject.noun} rows were left in place (they are re-checked on the next sweep)`, + { object: objectName, error: err?.message }, + ); + continue; + } + const orphanRowIds: string[] = []; + for (const [recordId, rowIds] of perRecord) { + if (live.has(recordId)) continue; + orphanRowIds.push(...rowIds); + } + if (orphanRowIds.length === 0) continue; + await deleteRowsByIds(engine, subject.table, orphanRowIds); + result.revoked += orphanRowIds.length; + } + } + } catch (err: any) { + logger?.warn?.( + `[sharing] orphan ${subject.noun} sweep stopped early — remaining rows are re-checked on the next sweep`, + { object: options?.object, error: err?.message, scanned: result.scanned }, + ); + result.truncated = true; + } + + result.unresolvedObjects = [...unresolved]; + result.truncated = result.truncated || walk.truncated; + if (result.revoked > 0) { + logger?.warn?.( + `[sharing] revoked ${subject.noun} rows whose record no longer exists (${subject.issue})`, + { rows: result.revoked, scanned: result.scanned, object: options?.object }, + ); + } + return result; +} diff --git a/packages/plugins/plugin-sharing/src/record-share-cascade.test.ts b/packages/plugins/plugin-sharing/src/record-share-cascade.test.ts index d8d0cb2bcb..08fe60c8f4 100644 --- a/packages/plugins/plugin-sharing/src/record-share-cascade.test.ts +++ b/packages/plugins/plugin-sharing/src/record-share-cascade.test.ts @@ -20,10 +20,12 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import { assertEngineDeleteDispatch } from '@objectstack/objectql'; import { SharingService } from './sharing-service.js'; import { SharingRuleService } from './sharing-rule-service.js'; +import { ShareLinkService } from './share-link-service.js'; import { bindRecordShareCascade, unbindRecordShareCascade, objectCanCarryRecordShares, + objectCanCarryShareLinks, orphanShareSweepQueue, RECORD_SHARE_CASCADE_PACKAGE, } from './record-share-cascade.js'; @@ -197,6 +199,29 @@ function manualShare(engine: Engine, object: string, recordId: string, recipient return id; } +// ── [#5190] the `sys_share_link` half ──────────────────────────────────────── +const shareLinks = (engine: Engine) => engine._tables.sys_share_link ?? []; +const shareLinkIds = (engine: Engine) => shareLinks(engine).map((r) => String(r.id)).sort(); + +/** A capability token on `(object, recordId)` — no recipient, by design. */ +function shareLink(engine: Engine, object: string, recordId: string, id = `shl_${recordId}`, extra: Row = {}) { + (engine._tables.sys_share_link ??= []).push({ + id, + token: `tok_${id}_aaaaaaaaaaaa`, + object_name: object, + record_id: recordId, + permission: 'view', + audience: 'link_only', + expires_at: null, + revoked_at: null, + created_by: 'admin', + use_count: 0, + last_used_at: null, + ...extra, + }); + return id; +} + describe('#5103 objectCanCarryRecordShares — the runtime, metadata-driven posture', () => { it('accepts any object that DECLARES a sharing model', () => { for (const sharingModel of ['private', 'public_read', 'public_read_write', 'controlled_by_parent']) { @@ -664,3 +689,563 @@ describe('#5103 coexistence with the #5102 rule hooks', () => { expect(engine.boundFor(RECORD_SHARE_CASCADE_PACKAGE)).toHaveLength(0); }); }); + +/** + * [#5190] The same cascade, for `sys_share_link`. + * + * `sys_record_share` orphans (#5103) at least name their beneficiaries. A share + * link is an identity-less CAPABILITY token: whoever holds the URL has the + * access. So the same orphan is strictly worse here — on a reused record id the + * new record is handed to whoever kept a link that morally died with the old + * one, and that holder can be anyone the record was ever shared with. + * + * Two halves, and BOTH are tested: `resolveToken`'s existence check + * (share-link-service.test.ts) holds whether or not a hook ever ran; this file + * covers the cascade and boot sweep that stop the rows from accumulating in the + * first place. + */ +describe('#5190 objectCanCarryShareLinks — a DIFFERENT posture question', () => { + it('accepts any object that declares publicSharing', () => { + expect(objectCanCarryShareLinks({ name: 'ai_conversations', publicSharing: { enabled: true } })).toBe(true); + }); + + /** + * THE PREDICATE REPRO. Link minting is gated by `publicSharing`, which is + * INDEPENDENT of `sharingModel` — so the object most likely to hold links (a + * platform object that opted into link sharing) is exactly one the + * record-share predicate skips. Reuse `objectCanCarryRecordShares` for links + * and this object's links outlive their records forever. + */ + it('covers a publicSharing object the RECORD-SHARE predicate skips', () => { + const schema = { name: 'sys_report', isSystem: true, publicSharing: { enabled: true } }; + expect(objectCanCarryRecordShares(schema)).toBe(false); + expect(objectCanCarryShareLinks(schema)).toBe(true); + }); + + it('still covers an object whose publicSharing was turned OFF (links outlive the flip)', () => { + expect(objectCanCarryShareLinks({ name: 'sys_report', isSystem: true, publicSharing: { enabled: false } })).toBe(true); + }); + + it('covers everything the record-share cascade already covers (system mints need no opt-in)', () => { + expect(objectCanCarryShareLinks({ name: 'contract', sharingModel: 'private' })).toBe(true); + expect(objectCanCarryShareLinks({ name: 'inquiry', fields: {} })).toBe(true); + }); + + it('skips an UNMARKED system object — the same documented boundary #5103 drew', () => { + expect(objectCanCarryShareLinks({ name: 'sys_audit_log', isSystem: true })).toBe(false); + }); + + it("never cascades on the sharing subsystem's own tables", () => { + expect(objectCanCarryShareLinks({ name: 'sys_share_link', publicSharing: { enabled: true } })).toBe(false); + expect(objectCanCarryShareLinks({ name: 'sys_record_share' })).toBe(false); + }); + + it('falls toward cleanup when the schema cannot be resolved', () => { + expect(objectCanCarryShareLinks(undefined)).toBe(true); + expect(objectCanCarryShareLinks(null)).toBe(true); + }); +}); + +describe('#5190 record delete revokes the share LINKS of that record', () => { + let engine: Engine; + let sharing: SharingService; + let linkService: ShareLinkService; + let logger: any; + + beforeEach(() => { + logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; + engine = makeEngine(); + engine._schemas.contract = { name: 'contract', sharingModel: 'private', fields: { owner_id: {} } }; + engine._tables.contract = [ + { id: 'ctr1', owner_id: 'boss' }, + { id: 'ctr2', owner_id: 'boss' }, + ]; + engine._tables.sys_record_share = []; + engine._tables.sys_share_link = []; + sharing = new SharingService({ engine: engine as any, logger }); + linkService = new ShareLinkService({ engine: engine as any, logger }); + bindRecordShareCascade(engine as any, sharing, logger, () => linkService); + }); + + /** + * THE REPRO. Revert the link half (or the `links` argument) and the token row + * survives its record, so this reads both ids instead of one. + */ + it('deletes the link rows of a deleted record, on an object with no rules', async () => { + shareLink(engine, 'contract', 'ctr1'); + shareLink(engine, 'contract', 'ctr2'); + + await engine.simulateDeleteById('contract', 'ctr1'); + + expect(shareLinkIds(engine)).toEqual(['shl_ctr2']); + }); + + it('leaves the links of records that were NOT deleted, and of other objects', async () => { + shareLink(engine, 'contract', 'ctr1'); + shareLink(engine, 'contract', 'ctr2', 'shl_keep'); + engine._schemas.invoice = { name: 'invoice', sharingModel: 'private', fields: { owner_id: {} } }; + // Same record id on a DIFFERENT object — the revoke is keyed on both. + shareLink(engine, 'invoice', 'ctr1', 'shl_other_object'); + + await engine.simulateDeleteById('contract', 'ctr1'); + + expect(shareLinkIds(engine)).toEqual(['shl_keep', 'shl_other_object']); + }); + + it('revokes on a SYSTEM-context delete too', async () => { + shareLink(engine, 'contract', 'ctr1'); + + await engine.simulateDeleteById('contract', 'ctr1', SYS); + + expect(shareLinkIds(engine)).toEqual([]); + }); + + it('revokes every id of a BOUNDED predicate delete', async () => { + shareLink(engine, 'contract', 'ctr1'); + shareLink(engine, 'contract', 'ctr2'); + engine._tables.contract.push({ id: 'ctr3', owner_id: 'other' }); + shareLink(engine, 'contract', 'ctr3'); + + await engine.simulateBulkDelete('contract', { owner_id: 'boss' }); + + expect(shareLinkIds(engine)).toEqual(['shl_ctr3']); + }); + + it('issues a set-based revoke per table, not one delete per link row', async () => { + shareLink(engine, 'contract', 'ctr1', 'shl_a'); + shareLink(engine, 'contract', 'ctr1', 'shl_b'); + shareLink(engine, 'contract', 'ctr2', 'shl_c'); + manualShare(engine, 'contract', 'ctr1', 'carol'); + engine._deleteCalls.length = 0; + + await engine.simulateBulkDelete('contract', { owner_id: 'boss' }); + + const linkDeletes = engine._deleteCalls.filter((c) => c.object === 'sys_share_link'); + expect(linkDeletes).toHaveLength(1); + expect(linkDeletes[0].options).toMatchObject({ + multi: true, + where: { object_name: 'contract', record_id: { $in: ['ctr1', 'ctr2'] } }, + }); + expect(shareLinks(engine)).toEqual([]); + // …and the share half still issued exactly its own one statement. + expect(engine._deleteCalls.filter((c) => c.object === 'sys_record_share')).toHaveLength(1); + }); + + /** + * The publicSharing-only object: the SHARE half declines it (unmarked system + * object), the LINK half must not. A single shared predicate for both halves + * fails here. + */ + it('revokes links on a publicSharing object the share half skips', async () => { + engine._schemas.sys_report = { name: 'sys_report', isSystem: true, publicSharing: { enabled: true } }; + engine._tables.sys_report = [{ id: 'rep1' }]; + shareLink(engine, 'sys_report', 'rep1'); + manualShare(engine, 'sys_report', 'rep1', 'carol'); + engine._deleteCalls.length = 0; + + await engine.simulateDeleteById('sys_report', 'rep1'); + + expect(shareLinkIds(engine)).toEqual([]); + // The share row is untouched — that posture is still the boot sweep's job. + expect(engine._deleteCalls.filter((c) => c.object === 'sys_record_share')).toHaveLength(0); + expect(shareIds(engine)).toEqual(['shr_rep1_carol']); + }); + + it('skips an object outside BOTH postures (no query on the hot path)', async () => { + engine._schemas.sys_audit_log = { name: 'sys_audit_log', isSystem: true }; + engine._tables.sys_audit_log = [{ id: 'evt1' }]; + shareLink(engine, 'sys_audit_log', 'evt1'); + engine._deleteCalls.length = 0; + + await engine.simulateDeleteById('sys_audit_log', 'evt1'); + + expect(engine._deleteCalls.filter((c) => c.object === 'sys_share_link')).toHaveLength(0); + expect(shareLinkIds(engine)).toEqual(['shl_evt1']); // the boot sweep's job + }); + + it('covers an object that gains `publicSharing` AFTER boot — no rebind needed', async () => { + engine._schemas.late = { name: 'late', isSystem: true }; // outside both postures + engine._tables.late = [{ id: 'late1' }, { id: 'late2' }]; + shareLink(engine, 'late', 'late1'); + await engine.simulateDeleteById('late', 'late1'); + expect(shareLinkIds(engine)).toEqual(['shl_late1']); + + engine._schemas.late = { name: 'late', isSystem: true, publicSharing: { enabled: true } }; + shareLink(engine, 'late', 'late2'); + + await engine.simulateDeleteById('late', 'late2'); + + expect(shareLinkIds(engine)).toEqual(['shl_late1']); // only the pre-flip orphan + }); + + /** + * The two halves are isolated: they are different tables, and the token is the + * more dangerous leftover, so a driver error reclaiming grants must not also + * skip the links. One shared `try` around both fails this test. + */ + it('still revokes the LINKS when the share revoke throws', async () => { + shareLink(engine, 'contract', 'ctr1'); + const boom = { + revokeSharesForDeletedRecords: vi.fn(async () => { throw new Error('driver down'); }), + sweepOrphanedRecordShares: vi.fn(async () => ({ scanned: 0, revoked: 0, unresolvedObjects: [], truncated: false })), + }; + unbindRecordShareCascade(engine as any); + bindRecordShareCascade(engine as any, boom as any, logger, () => linkService); + + await expect(engine.simulateDeleteById('contract', 'ctr1')).resolves.toBeDefined(); + + expect(boom.revokeSharesForDeletedRecords).toHaveBeenCalled(); + expect(shareLinkIds(engine)).toEqual([]); + }); + + it('never fails the write when the LINK revoke throws — and names the repair', async () => { + manualShare(engine, 'contract', 'ctr1', 'carol'); + const brokenLinks = { + revokeLinksForDeletedRecords: vi.fn(async () => { throw new Error('link table down'); }), + sweepOrphanedShareLinks: vi.fn(async () => ({ scanned: 0, revoked: 0, unresolvedObjects: [], truncated: false })), + }; + unbindRecordShareCascade(engine as any); + bindRecordShareCascade(engine as any, sharing, logger, () => brokenLinks); + + await expect(engine.simulateDeleteById('contract', 'ctr1')).resolves.toBeDefined(); + + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('share links'), + expect.objectContaining({ object: 'contract' }), + ); + // The share half still ran — isolation cuts both ways. + expect(shareIds(engine)).toEqual([]); + }); + + it('degrades to shares-only when no link service is wired (and never throws)', async () => { + manualShare(engine, 'contract', 'ctr1', 'carol'); + shareLink(engine, 'contract', 'ctr1'); + unbindRecordShareCascade(engine as any); + bindRecordShareCascade(engine as any, sharing, logger); // no `links` argument + + await expect(engine.simulateDeleteById('contract', 'ctr1')).resolves.toBeDefined(); + + expect(shareIds(engine)).toEqual([]); + expect(shareLinkIds(engine)).toEqual(['shl_ctr1']); // left to the boot sweep + }); + + it('survives a link-service getter that throws', async () => { + manualShare(engine, 'contract', 'ctr1', 'carol'); + unbindRecordShareCascade(engine as any); + bindRecordShareCascade(engine as any, sharing, logger, () => { throw new Error('registry tearing down'); }); + + await expect(engine.simulateDeleteById('contract', 'ctr1')).resolves.toBeDefined(); + expect(shareIds(engine)).toEqual([]); + }); + + /** + * Both halves of the fix, on one timeline: a real link minted through the real + * service stops resolving AND stops existing when its record is deleted. + */ + it('end to end — a minted link is gone from the table and unresolvable', async () => { + engine._schemas.contract.publicSharing = { enabled: true }; + const link = await linkService.createLink( + { object: 'contract', recordId: 'ctr1', audience: 'link_only', permission: 'view' }, + { isSystem: true }, + ); + expect(await linkService.resolveToken(link.token)).not.toBeNull(); + + await engine.simulateDeleteById('contract', 'ctr1'); + + expect(shareLinks(engine)).toEqual([]); + expect(await linkService.resolveToken(link.token)).toBeNull(); + }); +}); + +describe('#5190 an UNBOUNDED delete reclaims the links by sweep too', () => { + let engine: Engine; + let sharing: SharingService; + let linkService: ShareLinkService; + let logger: any; + + beforeEach(() => { + logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; + engine = makeEngine(); + engine._schemas.contract = { name: 'contract', sharingModel: 'private', fields: { owner_id: {} } }; + engine._tables.contract = [{ id: 'ctr1', owner_id: 'boss' }, { id: 'ctr2', owner_id: 'boss' }]; + engine._tables.sys_record_share = []; + engine._tables.sys_share_link = []; + sharing = new SharingService({ engine: engine as any, logger }); + linkService = new ShareLinkService({ engine: engine as any, logger }); + bindRecordShareCascade(engine as any, sharing, logger, () => linkService); + }); + + it('sweeps links by record-existence when the delete names no predicate at all', async () => { + shareLink(engine, 'contract', 'ctr1'); + shareLink(engine, 'contract', 'ctr2'); + + await engine.simulateBulkDelete('contract', undefined); + await orphanShareSweepQueue.whenIdle(); + + expect(engine._tables.contract).toEqual([]); + expect(shareLinkIds(engine)).toEqual([]); + }); + + it('spares links whose record survived the unbounded delete', async () => { + engine._tables.contract.push({ id: 'ctr3', owner_id: 'boss' }); + shareLink(engine, 'contract', 'ctr1'); + shareLink(engine, 'contract', 'ctr3', 'shl_survivor'); + + const ctx: any = { + object: 'contract', + event: 'beforeDelete', + input: { id: undefined, options: { where: undefined, multi: true } }, + session: ADMIN_SESSION, + }; + await engine.fire('beforeDelete', 'contract', ctx); + engine._tables.contract = engine._tables.contract.filter((r) => r.id !== 'ctr1'); + ctx.event = 'afterDelete'; + await engine.fire('afterDelete', 'contract', ctx); + await orphanShareSweepQueue.whenIdle(); + + expect(shareLinkIds(engine)).toEqual(['shl_survivor']); + }); + + it('never revokes the object wholesale — a token nobody can re-mint is unrecoverable', async () => { + shareLink(engine, 'contract', 'ctr2'); + engine._deleteCalls.length = 0; + + await engine.simulateBulkDelete('contract', undefined); + + for (const call of engine._deleteCalls.filter((c) => c.object === 'sys_share_link')) { + expect(call.options.where).not.toEqual({ object_name: 'contract' }); + } + }); + + it('queues both sweeps on the SAME serialized queue (no parallel table walks)', async () => { + shareLink(engine, 'contract', 'ctr1'); + manualShare(engine, 'contract', 'ctr1', 'carol'); + + await engine.simulateBulkDelete('contract', undefined); + // One `whenIdle` settles both halves — a second queue would leave rows here. + await orphanShareSweepQueue.whenIdle(); + + expect(shareLinkIds(engine)).toEqual([]); + expect(shareIds(engine)).toEqual([]); + }); +}); + +describe('#5190 boot sweep — orphaned share links', () => { + let engine: Engine; + let linkService: ShareLinkService; + let logger: any; + + beforeEach(() => { + logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; + engine = makeEngine(); + engine._schemas.contract = { name: 'contract', sharingModel: 'private', fields: { owner_id: {} } }; + engine._tables.contract = [{ id: 'ctr_live', owner_id: 'boss' }]; + engine._tables.sys_share_link = []; + linkService = new ShareLinkService({ engine: engine as any, logger }); + }); + + it('removes historical orphans and keeps the valid links', async () => { + shareLink(engine, 'contract', 'ctr_live', 'shl_live'); + shareLink(engine, 'contract', 'ctr_gone', 'shl_dead'); + // A REVOKED link on a live record: still a valid audit row, not an orphan. + shareLink(engine, 'contract', 'ctr_live', 'shl_revoked', { revoked_at: '2026-01-01T00:00:00.000Z' }); + + const result = await linkService.sweepOrphanedShareLinks(); + + expect(result).toMatchObject({ scanned: 3, revoked: 1, unresolvedObjects: [], truncated: false }); + expect(shareLinkIds(engine)).toEqual(['shl_live', 'shl_revoked']); + }); + + it('is idempotent — a second boot finds nothing to do', async () => { + shareLink(engine, 'contract', 'ctr_gone'); + expect((await linkService.sweepOrphanedShareLinks()).revoked).toBe(1); + expect((await linkService.sweepOrphanedShareLinks()).revoked).toBe(0); + }); + + it('covers the posture the cascade skips (an unmarked system object)', async () => { + engine._schemas.sys_audit_log = { name: 'sys_audit_log', isSystem: true }; + engine._tables.sys_audit_log = []; + shareLink(engine, 'sys_audit_log', 'evt_gone'); + + expect((await linkService.sweepOrphanedShareLinks()).revoked).toBe(1); + expect(shareLinkIds(engine)).toEqual([]); + }); + + /** + * "Could not ask" is not "the record is gone" — for the LINK table as much as + * for the share table. A probe failure that deleted would turn a transient + * driver error into a link nobody can get back (nothing can re-mint a token + * someone already holds). + */ + it('LEAVES links alone when the existence probe fails, and reports the object', async () => { + shareLink(engine, 'contract', 'ctr_gone'); + engine.failFindOn = 'contract'; + + const result = await linkService.sweepOrphanedShareLinks(); + + expect(result.revoked).toBe(0); + expect(result.unresolvedObjects).toEqual(['contract']); + expect(shareLinkIds(engine)).toEqual(['shl_ctr_gone']); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('could not check whether records still exist'), + expect.objectContaining({ object: 'contract' }), + ); + }); + + it('scopes to one object when asked (the unbounded-delete repair)', async () => { + engine._schemas.invoice = { name: 'invoice', sharingModel: 'private', fields: { owner_id: {} } }; + engine._tables.invoice = []; + shareLink(engine, 'contract', 'ctr_gone', 'shl_contract_orphan'); + shareLink(engine, 'invoice', 'inv_gone', 'shl_invoice_orphan'); + + const result = await linkService.sweepOrphanedShareLinks({ object: 'contract' }); + + expect(result).toMatchObject({ scanned: 1, revoked: 1 }); + expect(shareLinkIds(engine)).toEqual(['shl_invoice_orphan']); + }); + + it('probes existence in ONE batched query per object per page', async () => { + for (let i = 0; i < 25; i++) shareLink(engine, 'contract', `ctr_gone_${i}`, `shl_${i}`); + engine._findCalls.length = 0; + + await linkService.sweepOrphanedShareLinks({ batchSize: 100 }); + + const probes = engine._findCalls.filter((c) => c.object === 'contract'); + expect(probes).toHaveLength(1); + expect(probes[0].options.where).toMatchObject({ id: { $in: expect.any(Array) } }); + expect(shareLinks(engine)).toEqual([]); + }); + + it('pages by keyset and REPORTS a scan that its cap cut short', async () => { + for (let i = 0; i < 12; i++) shareLink(engine, 'contract', `ctr_gone_${i}`, `shl_${String(i).padStart(2, '0')}`); + + const result = await linkService.sweepOrphanedShareLinks({ batchSize: 5, max: 10 }); + + expect(result.scanned).toBe(10); + expect(result.truncated).toBe(true); + expect(result.revoked).toBe(10); + expect(shareLinks(engine)).toHaveLength(2); + }); + + it('walks past rows it just deleted (a seek, never an OFFSET — #4363)', async () => { + for (let i = 0; i < 9; i++) shareLink(engine, 'contract', `ctr_gone_${i}`, `shl_${i}`); + + const result = await linkService.sweepOrphanedShareLinks({ batchSize: 3 }); + + expect(result).toMatchObject({ scanned: 9, revoked: 9 }); + expect(shareLinks(engine)).toEqual([]); + }); + + it('sweeps the two tables independently — neither can hide the other', async () => { + const sharing = new SharingService({ engine: engine as any, logger }); + manualShare(engine, 'contract', 'ctr_gone', 'carol', 'shr_orphan'); + shareLink(engine, 'contract', 'ctr_gone', 'shl_orphan'); + + expect((await sharing.sweepOrphanedRecordShares()).revoked).toBe(1); + expect(shareLinkIds(engine)).toEqual(['shl_orphan']); // untouched by the share sweep + expect((await linkService.sweepOrphanedShareLinks()).revoked).toBe(1); + expect(shareLinkIds(engine)).toEqual([]); + expect(shareIds(engine)).toEqual([]); + }); +}); + +/** + * [#5190] The link half on a RULES-BEARING object. + * + * Every link-cascade case above runs on an object with no sharing rules, which + * leaves the one interaction that only exists on the objects carrying the MOST + * sharing machinery unpinned: where rules exist, #5102's `bindRuleHooks` + * registers its own `beforeDelete` / `afterDelete` under a DIFFERENT hook + * package, and both packages read the same stashed row set + * ({@link AFFECTED_ROWS_STASH_KEY}). A link cascade that happened to work only + * while it was the sole `beforeDelete` writer — or one that let the rule + * package's recompute consume the stash first — would pass all of them and + * still leak tokens exactly where the risk is highest. + * + * So this asserts the three revocations on one timeline, from one delete: the + * rule grant (#5102), the manual share (#5103) and the capability token + * (#5190). + */ +describe('#5190 the LINK cascade on a rules-bearing object', () => { + let engine: Engine; + let sharing: SharingService; + let rules: SharingRuleService; + let linkService: ShareLinkService; + let logger: any; + + beforeEach(async () => { + logger = { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }; + engine = makeEngine(); + // A rules-bearing object that ALSO opted into link sharing — the realistic + // shape, and the one where all three revocation paths meet. + engine._schemas.opportunity = { + name: 'opportunity', + sharingModel: 'private', + publicSharing: { enabled: true }, + fields: { owner_id: {} }, + }; + engine._tables.opportunity = [ + { id: 'opp1', region: 'east', owner_id: 'boss' }, + { id: 'opp2', region: 'east', owner_id: 'boss' }, + ]; + engine._tables.sys_record_share = []; + engine._tables.sys_share_link = []; + engine._tables.sys_sharing_rule = [{ + id: 'srule_east', + name: 'east_to_alice', + label: 'East → Alice', + object_name: 'opportunity', + criteria_json: JSON.stringify({ region: 'east' }), + recipient_type: 'user', + recipient_id: 'alice', + access_level: 'edit', + active: true, + }]; + sharing = new SharingService({ engine: engine as any, logger }); + rules = new SharingRuleService({ engine: engine as any, sharing, logger }); + linkService = new ShareLinkService({ engine: engine as any, logger }); + bindRuleHooks(engine as any, rules, await rules.listRules({ activeOnly: true }, SYS), logger); + bindRecordShareCascade(engine as any, sharing, logger, () => linkService); + }); + + it('revokes the rule grant, the manual share AND the link of the deleted record', async () => { + await rules.evaluateRule('srule_east', SYS); + expect(shares(engine).filter((r) => r.source === 'rule')).toHaveLength(2); + manualShare(engine, 'opportunity', 'opp1', 'carol', 'shr_manual_opp1'); + shareLink(engine, 'opportunity', 'opp1', 'shl_opp1'); + shareLink(engine, 'opportunity', 'opp2', 'shl_opp2'); + + await engine.simulateDeleteById('opportunity', 'opp1'); + + // The token goes with the record… + expect(shareLinkIds(engine)).toEqual(['shl_opp2']); + // …and so do BOTH share sources, while opp2 keeps everything. + expect(shares(engine).every((r) => r.record_id === 'opp2')).toBe(true); + expect(shares(engine).map((r) => r.source)).toEqual(['rule']); + }); + + it('a BOUNDED predicate delete across both hook packages takes every link with it', async () => { + await rules.evaluateRule('srule_east', SYS); + shareLink(engine, 'opportunity', 'opp1', 'shl_opp1'); + shareLink(engine, 'opportunity', 'opp2', 'shl_opp2'); + engine._deleteCalls.length = 0; + + await engine.simulateBulkDelete('opportunity', { region: 'east' }); + + expect(engine._tables.opportunity).toEqual([]); + expect(shareLinkIds(engine)).toEqual([]); + // Still ONE set-based statement for the links, even with the rule package + // sharing the same stash. + expect(engine._deleteCalls.filter((c) => c.object === 'sys_share_link')).toHaveLength(1); + }); + + it('both hook packages stay bound, and unbinding the cascade leaves the rule hooks', () => { + expect(engine.boundFor(SHARING_RULE_HOOK_PACKAGE).length).toBeGreaterThan(0); + expect(engine.boundFor(RECORD_SHARE_CASCADE_PACKAGE)).toHaveLength(2); + + unbindRecordShareCascade(engine as any); + + expect(engine.boundFor(SHARING_RULE_HOOK_PACKAGE).length).toBeGreaterThan(0); + expect(engine.boundFor(RECORD_SHARE_CASCADE_PACKAGE)).toHaveLength(0); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/record-share-cascade.ts b/packages/plugins/plugin-sharing/src/record-share-cascade.ts index 4d67af242f..6dc2cecc19 100644 --- a/packages/plugins/plugin-sharing/src/record-share-cascade.ts +++ b/packages/plugins/plugin-sharing/src/record-share-cascade.ts @@ -1,7 +1,7 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * [#5103] The record-delete → share-revoke cascade. + * [#5103 / #5190] The record-delete → share-revoke cascade. * * ## The invariant * @@ -79,6 +79,23 @@ * {@link objectCanCarryRecordShares} despite refusing manual grants: rows can * exist there (the rule evaluator grants under system context), and this is * the path that reclaims them. + * + * ## [#5190] Two tables, one cascade + * + * `sys_share_link` has the same invariant and needed the same three things (the + * bounded revoke, the unbounded sweep, the boot backstop), so it rides THIS + * seam rather than growing a parallel one: one `beforeDelete` stash, one + * `afterDelete`, one serialized sweep queue, one boot pass. What differs is only + * the posture predicate — a link is minted under `publicSharing`, which is + * orthogonal to `sharingModel` — hence {@link objectCanCarryShareLinks} beside + * {@link objectCanCarryRecordShares}, each gating its own half inside the + * handler. + * + * The link half is reached through the `ShareLinkService` (late-bound: the + * plugin constructs it after this bind, and `sys_share_link` is + * `managedBy: 'engine-owned'` with every write flowing through that service). + * Absent service → the boot sweep is the only reclaim, exactly as when the + * engine has no hook API at all. */ import { @@ -136,6 +153,41 @@ export function objectCanCarryRecordShares(schema: unknown): boolean { return effectiveSharingModel(s) !== 'public'; } +/** + * [#5190] Could `schema`'s object carry `sys_share_link` rows? + * + * NOT the same question as {@link objectCanCarryRecordShares}, and that is the + * reason this predicate exists rather than reusing it. Link minting is gated by + * `publicSharing`, which is INDEPENDENT of `sharingModel`: the object most + * likely to hold links is a public-ish one that opted into link sharing, and + * that object can be exactly the one the record-share predicate skips. Gating + * links on the sharing model would have left the commonest case orphaned. + * + * Wide, on purpose, in three directions: + * + * - `publicSharing` DECLARED counts even when `enabled: false` — links minted + * while it was on outlive the flip, and cleanup must reach them; + * - anything the record-share cascade already covers counts too. `createLink` + * lets a SYSTEM (or `permissive`) caller mint on an object that never opted + * in, so links can exist wherever records do; those objects are already in + * the handler, and the marginal cost is one set-based statement; + * - an unresolvable schema counts — "we cannot tell" must fall toward cleanup. + * + * What is left out is the same documented boundary #5103 drew: an UNMARKED + * system object (no `sharingModel`, no `publicSharing`), which keeps this hook + * off the platform's hottest delete paths. A system-minted link there is the + * boot sweep's to reclaim — and, whether or not it ever is, `resolveToken`'s + * existence check already refuses to honour it. + */ +export function objectCanCarryShareLinks(schema: unknown): boolean { + if (schema == null) return true; + const s = schema as any; + const name = s?.name == null ? '' : String(s.name); + if (SHARING_OWN_OBJECTS.has(name)) return false; + if (s?.publicSharing != null) return true; + return objectCanCarryRecordShares(schema); +} + /** The slice of the engine this module needs. */ export interface CascadeEngine { registerHook( @@ -154,6 +206,17 @@ interface MinimalLogger { warn?: (msg: any, ...rest: any[]) => void; } +/** + * [#5190] The slice of `ShareLinkService` the cascade drives. Structural, so a + * host can supply its own link store, and LATE-BOUND at the call site (see + * {@link bindRecordShareCascade}'s `links` parameter) because the plugin binds + * this cascade before it constructs the share-link service. + */ +export interface ShareLinkCascade { + revokeLinksForDeletedRecords(object: string, recordIds: readonly string[]): Promise; + sweepOrphanedShareLinks(options?: { object?: string }): Promise; +} + /** * The in-process executor for the unbounded-delete branch's object-scoped * sweep. Module-scoped for the same reason `ruleRegrantQueue` is: a rebind @@ -192,6 +255,15 @@ export function bindRecordShareCascade( engine: CascadeEngine, sharing: Pick, logger?: MinimalLogger, + /** + * [#5190] Late-bound `sys_share_link` half. A GETTER, not the service: the + * plugin binds this cascade during `kernel:ready` and constructs the share-link + * service later in the same handler, and the same indirection is how the + * plugin's other optional collaborators are wired (`hierarchyResolver`, + * `securityService`). Absent / returning nullish → links are left to the boot + * sweep, and `resolveToken` refuses them meanwhile. + */ + links?: () => ShareLinkCascade | null | undefined, ): void { if (typeof engine.registerHook !== 'function') return; if (typeof engine.unregisterHooksByPackage === 'function') { @@ -203,15 +275,52 @@ export function bindRecordShareCascade( // narrower, subsystem-owned revoke first. const opts = { packageId: RECORD_SHARE_CASCADE_PACKAGE, priority: 190 }; - const applies = (objectName: string): boolean => { - if (!objectName) return false; - if (SHARING_OWN_OBJECTS.has(objectName)) return false; - return objectCanCarryRecordShares(resolveSchema(engine, objectName)); + /** + * Which halves apply to this object, from ONE schema resolve. Both are + * evaluated per delete (never enumerated at bind time), so an object that + * gains `sharingModel` or `publicSharing` after boot is covered on its very + * next delete with no rebind. + */ + const targets = (objectName: string): { shares: boolean; links: boolean } => { + if (!objectName || SHARING_OWN_OBJECTS.has(objectName)) return { shares: false, links: false }; + const schema = resolveSchema(engine, objectName); + return { + shares: objectCanCarryRecordShares(schema), + links: objectCanCarryShareLinks(schema), + }; + }; + + /** + * Run one half. The delete has already landed; failing here would not bring + * the record back, so nothing rethrows — and each half is isolated, so a + * driver error reclaiming shares cannot also skip the links (they are + * different tables, and the token is the more dangerous leftover). + * + * `warn`, not `error`: the consequence is a stale row that no live record + * matches, and the `kernel:bootstrapped` sweep repairs it on the next boot — + * a functional degradation with a named repair path, not silent durability + * loss. + */ + const attempt = async ( + objectName: string, + what: string, + run: () => Promise, + ): Promise => { + try { + await run(); + } catch (err: any) { + logger?.warn?.( + `[sharing] could not revoke the ${what} of a deleted record — the rows stay until the next ` + + 'boot-time orphan sweep reclaims them', + { object: objectName, error: err?.message }, + ); + } }; engine.registerHook('beforeDelete', async (ctx: any) => { const objectName = String(ctx?.object ?? ''); - if (!applies(objectName)) return; + const t = targets(objectName); + if (!t.shares && !t.links) return; // Must be `before`: the delete is what makes these rows unfindable. // Shared stash — the rule package's own `beforeDelete` reads or writes the // same answer, so a write resolves its row set once however many of our @@ -221,31 +330,57 @@ export function bindRecordShareCascade( engine.registerHook('afterDelete', async (ctx: any) => { const objectName = String(ctx?.object ?? ''); - if (!applies(objectName)) return; + const t = targets(objectName); + if (!t.shares && !t.links) return; + // Belt around everything OUTSIDE the two halves (each of which has its own + // `attempt`): a delete that already landed must never be failed by this + // hook, whatever goes wrong in it. + let affected: ReturnType; try { - const affected = readAffectedRows(ctx); - if (affected.kind === 'rows') { - if (affected.ids.length === 0) return; - await sharing.revokeSharesForDeletedRecords(objectName, affected.ids); - return; - } - // Unbounded: the ids are unknown, so ask the shares instead of the write. - // Queued, because the walk's cost is unrelated to this write's and a hook - // must not hold the caller. Under-cleaning for a moment is the safe - // direction; over-deleting a manual share would be unrecoverable. - // `info`, not `warn`: unlike the rule path's unbounded branch — which - // revokes grants that records still deserve and warns because recipients - // visibly lose access until the re-grant lands — nothing here is taken - // from a surviving record. The sweep only removes rows whose record is - // gone, so a deferred reclaim has no user-visible consequence to warn - // about, and a `warn` on every bulk delete would just erode the level. - // The sweep speaks up (at `warn`) if it actually revokes anything. - logger?.info?.( - '[sharing] a bulk delete touched more rows than could be enumerated — the shares of the ' + - 'deleted records are being reclaimed by a background orphan sweep instead ' + - '(a restart re-runs the same sweep)', - { object: objectName, reason: affected.reason }, + affected = readAffectedRows(ctx); + } catch (err: any) { + logger?.warn?.( + '[sharing] could not read the row set of a deleted record — its shares and links stay until ' + + 'the next boot-time orphan sweep reclaims them', + { object: objectName, error: err?.message }, ); + return; + } + + if (affected.kind === 'rows') { + if (affected.ids.length === 0) return; + if (t.shares) { + await attempt(objectName, 'shares', () => + sharing.revokeSharesForDeletedRecords(objectName, affected.ids)); + } + if (t.links) { + const linkService = readLinkService(links, objectName, logger); + if (linkService) { + await attempt(objectName, 'share links', () => + linkService.revokeLinksForDeletedRecords(objectName, affected.ids)); + } + } + return; + } + + // Unbounded: the ids are unknown, so ask the rows instead of the write. + // Queued, because the walk's cost is unrelated to this write's and a hook + // must not hold the caller. Under-cleaning for a moment is the safe + // direction; over-deleting a manual share would be unrecoverable. + // `info`, not `warn`: unlike the rule path's unbounded branch — which + // revokes grants that records still deserve and warns because recipients + // visibly lose access until the re-grant lands — nothing here is taken + // from a surviving record. The sweep only removes rows whose record is + // gone, so a deferred reclaim has no user-visible consequence to warn + // about, and a `warn` on every bulk delete would just erode the level. + // The sweep speaks up (at `warn`) if it actually revokes anything. + logger?.info?.( + '[sharing] a bulk delete touched more rows than could be enumerated — the shares of the ' + + 'deleted records are being reclaimed by a background orphan sweep instead ' + + '(a restart re-runs the same sweep)', + { object: objectName, reason: affected.reason }, + ); + if (t.shares) { orphanShareSweepQueue.enqueue( () => sharing.sweepOrphanedRecordShares({ object: objectName }).then(() => undefined), (err: any) => logger?.warn?.( @@ -254,25 +389,56 @@ export function bindRecordShareCascade( { object: objectName, error: err?.message }, ), ); - } catch (err: any) { - // The delete has already landed; failing here would not bring the record - // back, so never rethrow. `warn`, not `error`: the consequence is a stale - // share row that no live record matches, and the `kernel:bootstrapped` - // sweep repairs it on the next boot — a functional degradation with a - // named repair path, not silent durability loss. - logger?.warn?.( - '[sharing] could not revoke the shares of a deleted record — the rows stay until the next ' + - 'boot-time orphan sweep reclaims them', - { object: objectName, error: err?.message }, - ); + } + if (t.links) { + const linkService = readLinkService(links, objectName, logger); + if (linkService) { + // Same queue as the share sweep, not a second one: both walk the same + // records, and serializing them keeps a burst of bulk deletes from + // fanning out into parallel table scans. + orphanShareSweepQueue.enqueue( + () => linkService.sweepOrphanedShareLinks({ object: objectName }).then(() => undefined), + (err: any) => logger?.warn?.( + '[sharing] background orphan share-link sweep failed — link rows for the deleted records ' + + 'stay until the next sweep (any bulk delete on this object, or a restart). They cannot ' + + 'be resolved meanwhile: the token check re-asks whether the record exists', + { object: objectName, error: err?.message }, + ), + ); + } } }, opts); logger?.info?.( - '[sharing] record-delete share cascade bound (all objects; sharing posture judged per delete)', + '[sharing] record-delete share cascade bound (all objects; sharing posture judged per delete)' + + (links ? ' — sys_record_share + sys_share_link' : ' — sys_record_share only (no share-link service)'), ); } +/** + * [#5190] Resolve the late-bound link half. A getter that throws (a service + * registry mid-teardown) must not fail the delete hook — it degrades to "no + * link service", which the boot sweep repairs and `resolveToken` covers in the + * meantime. + */ +function readLinkService( + links: (() => ShareLinkCascade | null | undefined) | undefined, + objectName: string, + logger?: MinimalLogger, +): ShareLinkCascade | null { + if (typeof links !== 'function') return null; + try { + return links() ?? null; + } catch (err: any) { + logger?.warn?.( + '[sharing] share-link service unavailable while cascading a record delete — its links stay ' + + 'until the next boot-time orphan sweep (they cannot be resolved meanwhile)', + { object: objectName, error: err?.message }, + ); + return null; + } +} + /** Unbind the cascade. Returns the number of hooks removed. */ export function unbindRecordShareCascade(engine: CascadeEngine): number { if (typeof engine.unregisterHooksByPackage !== 'function') return 0; diff --git a/packages/plugins/plugin-sharing/src/share-link-service.test.ts b/packages/plugins/plugin-sharing/src/share-link-service.test.ts index e496c2dc39..0667bd7dd4 100644 --- a/packages/plugins/plugin-sharing/src/share-link-service.test.ts +++ b/packages/plugins/plugin-sharing/src/share-link-service.test.ts @@ -191,6 +191,174 @@ describe('ShareLinkService', () => { expect(await service.resolveToken('expired-token-xyz-123')).toBeNull(); }); + // ── [#5190] the record-existence gate ───────────────────────────────────── + // + // A share link is an identity-less CAPABILITY token: holding the URL IS the + // authorisation. `resolveToken` checked the token, `revoked_at`, `expires_at`, + // the audience and the password — and never whether the record it points at + // still existed. Delete the record and the link kept resolving; reuse the + // record id and the link starts authorising a brand-new record for whoever + // kept the URL. + // + // This suite pins the FAIL-CLOSED half, which holds whether or not the delete + // cascade (record-share-cascade.test.ts) ever ran. + describe('a deleted record kills the link (#5190)', () => { + /** Mint a link on the live `c1`, then make `c1` disappear. */ + async function mintThenDeleteRecord(): Promise { + const link = await service.createLink( + { object: 'ai_conversations', recordId: 'c1', audience: 'link_only', permission: 'view' }, + { userId: 'u1' }, + ); + engine._tables.ai_conversations = []; + return link.token; + } + + it('THE REPRO — refuses to resolve once the shared record is deleted', async () => { + const token = await mintThenDeleteRecord(); + expect(await service.resolveToken(token)).toBeNull(); + }); + + /** + * The response must not tell an unauthorised holder WHICH failure they hit: + * "that record was deleted" is itself information they have no claim to, + * and a distinct status would turn every leaked token into an existence + * oracle over the object. Same branch, same `null`, no throw — a mutation + * that raises a dedicated error (or returns a marker object) fails here even + * though the link stops resolving. + */ + it('is indistinguishable from revoked / expired / unknown — one `null`, never a throw', async () => { + const attempt = async (token: string) => { + try { + return { threw: false, value: await service.resolveToken(token) }; + } catch (err) { + return { threw: true, value: err }; + } + }; + + // The dead-record link points at `c2`, so removing it leaves `c1` alive + // for the other three cases — every outcome below differs ONLY in why it + // failed. + engine._tables.ai_conversations.push({ id: 'c2', title: 'Second' }); + const deadRecord = await service.createLink( + { object: 'ai_conversations', recordId: 'c2', audience: 'link_only', permission: 'view' }, + { userId: 'u1' }, + ); + engine._tables.ai_conversations = engine._tables.ai_conversations.filter((r) => r.id !== 'c2'); + const revoked = await service.createLink( + { object: 'ai_conversations', recordId: 'c1', audience: 'link_only', permission: 'view' }, + { userId: 'u1' }, + ); + await service.revokeLink(revoked.id, { userId: 'u1' }); + engine._tables.sys_share_link.push({ + id: 'shl_expired', + token: 'expired-token-xyz-123', + object_name: 'ai_conversations', + record_id: 'c1', + permission: 'view', + audience: 'link_only', + expires_at: new Date(Date.now() - 60_000).toISOString(), + revoked_at: null, + }); + + const outcomes = await Promise.all([ + attempt(deadRecord.token), + attempt(revoked.token), + attempt('expired-token-xyz-123'), + attempt('nope-not-a-real-token-xyz'), + ]); + + expect(outcomes).toEqual([ + { threw: false, value: null }, + { threw: false, value: null }, + { threw: false, value: null }, + { threw: false, value: null }, + ]); + }); + + /** + * Follows from the gate sitting BEFORE the usage stamp, and pinned + * separately because the ordering is what makes it true: a dead record must + * not keep ticking `use_count` / `last_used_at`, which is both noise in the + * Setup grid and a bad signal for anyone auditing a leaked link. + */ + it('does not stamp use_count / last_used_at for a dead-record link', async () => { + const token = await mintThenDeleteRecord(); + const row = () => engine._tables.sys_share_link[0]; + + await service.resolveToken(token); + + expect(row().use_count).toBe(0); + expect(row().last_used_at).toBeNull(); + + // Control: the same link on a LIVE record still stamps, so the assertion + // above is about the record's death, not about stamping being broken. + engine._tables.ai_conversations = [{ id: 'c1', title: 'Demo' }]; + await service.resolveToken(token); + expect(row().use_count).toBe(1); + expect(row().last_used_at).not.toBeNull(); + }); + + /** + * "Could not ask" must not authorise. The orphan SWEEP fails the other way + * (a failed probe deletes nothing) — both refuse to act on an unanswered + * question; only the safe direction differs, because one grants access and + * the other destroys rows. + */ + it('fails CLOSED when the existence probe throws', async () => { + const token = await mintThenDeleteRecord(); + engine._tables.ai_conversations = [{ id: 'c1', title: 'Demo' }]; + const broken = { + ...engine, + async find(object: string, options?: any) { + if (object === 'ai_conversations') throw new Error('driver down'); + return engine.find(object, options); + }, + }; + const svc = new ShareLinkService({ engine: broken as any }); + + expect(await svc.resolveToken(token)).toBeNull(); + }); + + it('a link on a record that never existed is refused even when nothing else objects', async () => { + // Minted by a SYSTEM caller (which may mint on any object), then the row + // outlives its record — the path no `publicSharing` opt-in guards. + engine._tables.sys_share_link = [{ + id: 'shl_ghost', + token: 'ghost-token-abcdefgh', + object_name: 'ai_conversations', + record_id: 'never_existed', + permission: 'view', + audience: 'link_only', + expires_at: null, + revoked_at: null, + use_count: 0, + last_used_at: null, + }]; + + expect(await service.resolveToken('ghost-token-abcdefgh')).toBeNull(); + }); + + it('costs no existence query on a link the cheap gates already rejected', async () => { + const probed: string[] = []; + const recording = { + ...engine, + async find(object: string, options?: any) { probed.push(object); return engine.find(object, options); }, + }; + const svc = new ShareLinkService({ engine: recording as any }); + const link = await svc.createLink( + { object: 'ai_conversations', recordId: 'c1', audience: 'link_only', permission: 'view' }, + { userId: 'u1' }, + ); + await svc.revokeLink(link.id, { userId: 'u1' }); + probed.length = 0; + + expect(await svc.resolveToken(link.token)).toBeNull(); + + // Only the token lookup — a revoked link never pays for the record probe. + expect(probed).toEqual(['sys_share_link']); + }); + }); + // ── [Finding-2] verified-authz enforcement ──────────────────────────────── describe('authorization (Finding-2)', () => { it('only the creator may revoke a link (a different user is denied)', async () => { diff --git a/packages/plugins/plugin-sharing/src/share-link-service.ts b/packages/plugins/plugin-sharing/src/share-link-service.ts index 4720bff83a..603304915c 100644 --- a/packages/plugins/plugin-sharing/src/share-link-service.ts +++ b/packages/plugins/plugin-sharing/src/share-link-service.ts @@ -11,10 +11,28 @@ import type { ShareLinkAudience, } from '@objectstack/spec/contracts'; import type { SharingEngine } from './sharing-service.js'; +import { + deleteRowsForDeletedRecords, + sweepOrphanedRowsByRecordExistence, + type OrphanShareSweepOptions, + type OrphanShareSweepResult, +} from './record-orphan-cleanup.js'; /** Service-elevated context for the plugin's own queries / mutations. */ const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; +/** + * [#5190] The table whose orphans this service owns. `sys_share_link` is + * `managedBy: 'engine-owned'` and its object doc states every write flows + * through `IShareLinkService` — so the record-delete cascade reaches it through + * this service, never by another module writing the table behind its back. + */ +const SHARE_LINK_SWEEP_SUBJECT = { + table: 'sys_share_link', + noun: 'share-link', + issue: '#5190', +} as const; + /** URL-safe alphabet (RFC 4648 base64url minus padding). 64 symbols. */ const TOKEN_ALPHABET = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_'; @@ -182,6 +200,8 @@ export interface ShareLinkServiceOptions { recordId: string, context: ShareLinkExecutionContext, ) => Promise; + /** [#5190] Optional logger for the record-delete cascade / orphan sweep. */ + logger?: { info?: Function; warn?: Function; error?: Function; debug?: Function }; } /** @@ -203,6 +223,7 @@ export class ShareLinkService implements IShareLinkService { recordId: string, context: ShareLinkExecutionContext, ) => Promise; + private readonly logger?: ShareLinkServiceOptions['logger']; constructor(opts: ShareLinkServiceOptions) { this.engine = opts.engine; @@ -210,6 +231,7 @@ export class ShareLinkService implements IShareLinkService { this.hashPassword = opts.hashPassword ?? defaultHashPassword; this.verifyPassword = opts.verifyPassword ?? defaultVerifyPassword; this.canManageShares = opts.canManageShares; + this.logger = opts.logger; } async createLink( @@ -395,6 +417,26 @@ export class ShareLinkService implements IShareLinkService { if (!ok) return null; } + // [#5190] Does the shared RECORD still exist? A share link is an + // identity-less CAPABILITY token: whoever holds it has the access, no + // principal required. So an orphaned link is worse than an orphaned + // `sys_record_share` (#5103), whose recipients are at least a named set — + // the moment a record id is reused (custom primary keys, an import that + // preserves ids, any future id recycling) a link that morally died with its + // record starts authorising a BRAND-NEW record, for whoever kept the URL. + // + // This is the fail-closed half of the fix and it is deliberately + // independent of the delete cascade below: it holds for links that predate + // the cascade, for a hook that never ran, and for the postures the cascade + // skips. Same branch as revoked / expired — `null`, no distinct code, no + // distinct error — because "that record is gone" is itself information an + // unauthorised holder must not be able to read out of the endpoint. + // + // Placed AFTER the cheap in-memory gates (a revoked or expired token pays + // no query) and BEFORE the usage stamp, so a dead record never bumps + // `use_count` / `last_used_at` either. + if (!(await this.recordStillExists(row.object_name, row.record_id))) return null; + // Compute the effective redaction set (object default ∪ per-link). const schema = this.engine.getSchema?.(row.object_name); const policy = getPolicy(schema); @@ -419,4 +461,81 @@ export class ShareLinkService implements IShareLinkService { return { link: row, redactFields }; } + + /** + * [#5190] Is `(object, recordId)` still there? Read under the SYSTEM context + * on purpose: the question is EXISTENCE, not the holder's visibility — the + * token is the authorisation, and an anonymous holder has no context to read + * under in the first place. + * + * Fails CLOSED. A probe that throws (driver blip, unregistered object) is an + * unanswered question, and an unanswered question must not authorise: the + * caller treats `false` exactly like revoked. Note this is the OPPOSITE + * direction from the orphan sweep, which leaves rows alone when its probe + * fails — and for the same principle. Neither acts on an unanswered question; + * for a grant the safe direction is "deny", for a deletion it is "keep". + */ + private async recordStillExists( + object: string | null | undefined, + recordId: string | null | undefined, + ): Promise { + if (!object || !recordId) return false; + try { + const rows = await this.engine.find(String(object), { + where: { id: recordId }, + fields: ['id'], + limit: 1, + context: SYSTEM_CTX, + } as any); + return Array.isArray(rows) && rows.length > 0; + } catch { + return false; + } + } + + /** + * [#5190] Delete every `sys_share_link` row belonging to records that have + * just been deleted — the cascade half, driven by `record-share-cascade.ts`. + * + * DELETE, not `revoked_at`: the row's whole subject is gone, so there is no + * link left to keep a revocation record OF, and the issue names the growth + * this table would otherwise show (`sys_share_link` only ever grows, and + * Setup's link lists point at records that do not exist). It is also what + * #5103 does to the sibling table for the same reason. A link the ADMIN + * revoked still keeps its audit row — that path is untouched. + */ + async revokeLinksForDeletedRecords( + object: string, + recordIds: readonly string[], + ): Promise { + await deleteRowsForDeletedRecords( + this.engine, + SHARE_LINK_SWEEP_SUBJECT.table, + object, + recordIds, + ); + } + + /** + * [#5190] Remove every share link whose RECORD no longer exists. + * + * The `sys_share_link` twin of `SharingService.sweepOrphanedRecordShares`, + * running the very same walk (`record-orphan-cleanup.ts`): keyset pages, a + * scan cap that reports itself, one batched existence probe per object per + * page, and rows left strictly alone when that probe fails. + * + * Called on `kernel:bootstrapped` (unscoped — every link that predates the + * cascade, plus anything a crashed hook missed) and from the cascade's + * unbounded-delete branch (scoped to one object). + */ + async sweepOrphanedShareLinks( + options?: OrphanShareSweepOptions, + ): Promise { + return sweepOrphanedRowsByRecordExistence( + this.engine, + SHARE_LINK_SWEEP_SUBJECT, + options, + this.logger, + ); + } } diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index 0b312ce1c2..b2f126f106 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -477,13 +477,21 @@ export class SharingServicePlugin implements Plugin { // Not bound per object: the posture is judged per delete from live // metadata, so an object that gains `sharingModel` after boot is covered // without a rebind (see record-share-cascade.ts). + // + // [#5190] The same hook pair also reclaims `sys_share_link` — a link is a + // capability token, so an orphan of it is worse than an orphaned grant: + // no principal is named, and a reused record id hands the new record to + // whoever kept the URL. The link service is passed as a GETTER because it + // is constructed further down this same handler; `resolveToken` refuses + // dead-record links regardless of whether this hook ever runs. try { if (typeof engine.registerHook === 'function' && typeof engine.unregisterHooksByPackage === 'function') { - bindRecordShareCascade(engine, this.service, ctx.logger as any); + bindRecordShareCascade(engine, this.service, ctx.logger as any, () => this.linkService); } else { ctx.logger.warn( 'SharingServicePlugin: engine has no hook API — record deletes will NOT revoke their ' + - 'sys_record_share rows; the kernel:bootstrapped orphan sweep is the only reclaim', + 'sys_record_share / sys_share_link rows; the kernel:bootstrapped orphan sweeps are the ' + + 'only reclaim', ); } } catch (err: any) { @@ -568,6 +576,8 @@ export class SharingServicePlugin implements Plugin { try { this.linkService = new ShareLinkService({ engine: engine as SharingEngine, + // [#5190] The cascade / orphan sweep report through the plugin logger. + logger: ctx.logger as any, // [ADR-0111 D8] Let a record's share-manager (owner / Modify All) // revoke a link someone else minted on their record. `this.service` // is always constructed above — even under `enforce: false` (the @@ -685,6 +695,29 @@ export class SharingServicePlugin implements Plugin { ctx.logger.warn('SharingServicePlugin: orphaned record-share sweep (kernel:bootstrapped) failed', { error: err?.message }); } + // [#5190] The same pass for `sys_share_link`. Separate try/catch, not a + // second statement inside the one above: a driver error reclaiming grants + // must not also skip the capability tokens, which are the leftovers that + // do not need a named recipient to be exercised. Same bounded shape + // (keyset pages, a self-reporting scan cap), and — like the share sweep — + // it runs in every posture, including `enforce: false`, where a host + // mounts this plugin purely for the share-link surface. + try { + if (this.linkService) { + const swept = await this.linkService.sweepOrphanedShareLinks(); + if (swept.truncated) { + ctx.logger.info( + 'SharingServicePlugin: orphaned share-link sweep hit its per-boot scan cap — the ' + + 'remaining rows are examined on the next boot (they cannot be resolved meanwhile: ' + + 'the token check re-asks whether the record exists)', + { scanned: swept.scanned, revoked: swept.revoked }, + ); + } + } + } catch (err: any) { + ctx.logger.warn('SharingServicePlugin: orphaned share-link sweep (kernel:bootstrapped) failed', { error: err?.message }); + } + if (!this.ruleService) return; try { // [#4433] EVERY rule, not `activeOnly` — a deactivated rule's grants diff --git a/packages/plugins/plugin-sharing/src/sharing-service.ts b/packages/plugins/plugin-sharing/src/sharing-service.ts index 5a0b84a140..678ffaba9f 100644 --- a/packages/plugins/plugin-sharing/src/sharing-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-service.ts @@ -8,8 +8,21 @@ import type { SharingExecutionContext, ShareAccessLevel, } from '@objectstack/spec/contracts'; -import { keysetWalk } from '@objectstack/types'; import { WRITE_ACCESS_LEVELS, normalizeAccessLevel } from './access-level.js'; +import { + deleteRowsForDeletedRecords, + sweepOrphanedRowsByRecordExistence, + type OrphanShareSweepOptions, + type OrphanShareSweepResult, +} from './record-orphan-cleanup.js'; + +/** + * [#5103] Re-exported from their new home so this module's public surface is + * unchanged: #5190 moved the record-existence cleanup MECHANISM into + * `record-orphan-cleanup.ts` (`sys_share_link` needs the identical walk), and a + * type that moved house is not an API change callers should have to notice. + */ +export type { OrphanShareSweepOptions, OrphanShareSweepResult }; /** * Shape of the data engine the service actually needs. Kept narrow so @@ -103,53 +116,12 @@ export interface SharingSecurityProbe { ): Promise<'own' | 'own_and_reports' | 'unit' | 'unit_and_below' | 'org'>; } -/** - * [#5103] Ids per `$in` on the record-delete cascade's revoke. Mirrors the - * chunk `SharingRuleService.revokeRuleGrantsForRecords` already uses: a single - * statement binding a thousand parameters is a portability trap (SQLite's - * default `SQLITE_MAX_VARIABLE_NUMBER` is 999 on older builds), and one number - * for both revoke paths keeps them from drifting. - */ -const RECORD_SHARE_REVOKE_CHUNK = 200; - -/** [#5103] Share rows read per page by the orphan sweep. */ -const ORPHAN_SWEEP_PAGE_SIZE = 500; - -/** - * [#5103] Share rows one sweep will scan before stopping and reporting - * truncation. The sweep runs on every boot, so it must cost a bounded amount - * on a table that only grows; the next boot resumes from the start and the - * rows it did not reach stay reachable by the object-scoped sweep. A cap is - * not a failure — but an unreported cap turns a partial scan into a false - * "nothing to clean", which is why {@link OrphanShareSweepResult} carries it. - */ -const ORPHAN_SWEEP_MAX_ROWS = 50_000; - -/** [#5103] Options for {@link SharingService.sweepOrphanedRecordShares}. */ -export interface OrphanShareSweepOptions { - /** Restrict the sweep to one object. Default: every object with share rows. */ - object?: string; - /** Share rows per page. Default {@link ORPHAN_SWEEP_PAGE_SIZE}. */ - batchSize?: number; - /** Stop after scanning this many rows. Default {@link ORPHAN_SWEEP_MAX_ROWS}. */ - max?: number; -} - -/** [#5103] What one {@link SharingService.sweepOrphanedRecordShares} pass did. */ -export interface OrphanShareSweepResult { - /** Share rows examined. */ - scanned: number; - /** Share rows revoked because their record no longer exists. */ - revoked: number; - /** - * Objects whose existence probe could not be run (unregistered object, - * driver error). Their rows were LEFT ALONE — "could not ask" is not - * "the record is gone", and only the second one may delete anything. - */ - unresolvedObjects: string[]; - /** True when {@link OrphanShareSweepOptions.max} stopped the scan early. */ - truncated: boolean; -} +/** [#5103] The table whose orphans this service owns. */ +const RECORD_SHARE_SWEEP_SUBJECT = { + table: 'sys_record_share', + noun: 'share', + issue: '#5103', +} as const; export interface SharingServiceOptions { engine: SharingEngine; @@ -816,20 +788,20 @@ export class SharingService implements ISharingService { * number of share rows. Returns nothing: counting would need a read the hot * delete path should not pay for, and callers that need a count (tests, the * sweep) can read the table. + * + * [#5190] The mechanism now lives in `record-orphan-cleanup.ts`, shared with + * `sys_share_link`'s identical cascade. This method keeps the meaning. */ async revokeSharesForDeletedRecords( object: string, recordIds: readonly string[], ): Promise { - if (!object || recordIds.length === 0) return; - for (let i = 0; i < recordIds.length; i += RECORD_SHARE_REVOKE_CHUNK) { - const batch = recordIds.slice(i, i + RECORD_SHARE_REVOKE_CHUNK); - await this.engine.delete('sys_record_share', { - where: { object_name: object, record_id: { $in: batch } }, - multi: true, - context: SYSTEM_CTX, - } as any); - } + await deleteRowsForDeletedRecords( + this.engine, + RECORD_SHARE_SWEEP_SUBJECT.table, + object, + recordIds, + ); } /** @@ -864,133 +836,24 @@ export class SharingService implements ISharingService { * untouched and is reported in `unresolvedObjects`. "Nothing was queried" is * not "nothing matched" — deleting on a failed probe would turn a transient * driver error into permanent access loss. + * + * [#5190] The walk itself lives in `record-orphan-cleanup.ts` — `sys_share_link` + * runs the identical one, and a second copy is how two sweeps that must agree + * start disagreeing (chunk size, cap, the failed-probe rule). */ async sweepOrphanedRecordShares( options?: OrphanShareSweepOptions, ): Promise { - const result: OrphanShareSweepResult = { - scanned: 0, - revoked: 0, - unresolvedObjects: [], - truncated: false, - }; - const unresolved = new Set(); - const walk = keysetWalk( - (q) => this.engine.find('sys_record_share', { - ...q, - fields: ['id', 'object_name', 'record_id'], - context: SYSTEM_CTX, - }), - { - where: options?.object ? { object_name: options.object } : undefined, - pageSize: Math.max(1, options?.batchSize ?? ORPHAN_SWEEP_PAGE_SIZE), - max: options?.max ?? ORPHAN_SWEEP_MAX_ROWS, - }, + return sweepOrphanedRowsByRecordExistence( + this.engine, + RECORD_SHARE_SWEEP_SUBJECT, + options, + this.logger, ); - - try { - for await (const page of walk.pages()) { - result.scanned += page.length; - - // Group the page by object so existence is one probe per object, not - // one per row. - const byObject = new Map>(); - for (const row of page) { - const objectName = row?.object_name == null ? '' : String(row.object_name); - const recordId = row?.record_id == null ? '' : String(row.record_id); - const shareId = row?.id == null ? '' : String(row.id); - if (!objectName || !recordId || !shareId) continue; - const perRecord = byObject.get(objectName) ?? new Map(); - const shareIds = perRecord.get(recordId) ?? []; - shareIds.push(shareId); - perRecord.set(recordId, shareIds); - byObject.set(objectName, perRecord); - } - - for (const [objectName, perRecord] of byObject) { - if (unresolved.has(objectName)) continue; - const recordIds = [...perRecord.keys()]; - let live: Set; - try { - live = await this.findLiveRecordIds(objectName, recordIds); - } catch (err: any) { - unresolved.add(objectName); - this.logger?.warn?.( - '[sharing] orphan share sweep could not check whether records still exist — ' + - 'its share rows were left in place (they are re-checked on the next sweep)', - { object: objectName, error: err?.message }, - ); - continue; - } - const orphanShareIds: string[] = []; - for (const [recordId, shareIds] of perRecord) { - if (live.has(recordId)) continue; - orphanShareIds.push(...shareIds); - } - if (orphanShareIds.length === 0) continue; - await this.deleteSharesByIds(orphanShareIds); - result.revoked += orphanShareIds.length; - } - } - } catch (err: any) { - this.logger?.warn?.( - '[sharing] orphan share sweep stopped early — remaining rows are re-checked on the next sweep', - { object: options?.object, error: err?.message, scanned: result.scanned }, - ); - result.truncated = true; - } - - result.unresolvedObjects = [...unresolved]; - result.truncated = result.truncated || walk.truncated; - if (result.revoked > 0) { - this.logger?.warn?.( - '[sharing] revoked share rows whose record no longer exists (#5103)', - { shares: result.revoked, scanned: result.scanned, object: options?.object }, - ); - } - return result; } // ── helpers ────────────────────────────────────────────────────── - /** - * [#5103] Which of `recordIds` still exist on `object`. Batched by - * {@link RECORD_SHARE_REVOKE_CHUNK} so the `$in` never outgrows a driver's - * bind-parameter limit. Throws on a query failure — the caller MUST treat - * that as "unknown", never as "none of them exist". - */ - private async findLiveRecordIds( - object: string, - recordIds: readonly string[], - ): Promise> { - const live = new Set(); - for (let i = 0; i < recordIds.length; i += RECORD_SHARE_REVOKE_CHUNK) { - const batch = recordIds.slice(i, i + RECORD_SHARE_REVOKE_CHUNK); - const rows = await this.engine.find(object, { - where: { id: { $in: batch } }, - fields: ['id'], - limit: batch.length, - context: SYSTEM_CTX, - }); - for (const row of (rows ?? [])) { - if ((row as any)?.id != null) live.add(String((row as any).id)); - } - } - return live; - } - - /** [#5103] Set-based delete of share rows by id, chunked like the revoke. */ - private async deleteSharesByIds(shareIds: readonly string[]): Promise { - for (let i = 0; i < shareIds.length; i += RECORD_SHARE_REVOKE_CHUNK) { - const batch = shareIds.slice(i, i + RECORD_SHARE_REVOKE_CHUNK); - await this.engine.delete('sys_record_share', { - where: { id: { $in: batch } }, - multi: true, - context: SYSTEM_CTX, - } as any); - } - } - /** * [ADR-0057] Resolve the owner-id set for a DEPTH scope. `own`/unset/`org` * resolve locally to the caller. HIERARCHY scopes (`unit` / `unit_and_below`