From 472c7c6de3e662c9bd4e46876ea7115952c43369 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 10:20:24 +0000 Subject: [PATCH] fix(plugin-audit): exclude sys_upload_session from audit/activity writes (#5202) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SKIP_OBJECTS group (2) — ADR-0057 decision 5 "stop the amplifier" — gained sys_job_queue in #5193/#5201; sys_upload_session is the same gap one table over. It declares lifecycle.class: 'transient' and its own object comment settles what the rows are worth: "an upload session is ephemeral state, never business truth" (ADR-0057 / #2970 item 4). Nothing connected that declaration to the exemption list, which is hand-written. The audit writers register for all objects and there is no system-context exemption, so StorageMetadataStore's own writes were mirrored into sys_audit_log AND sys_activity. A chunked upload of N parts costs 1 + N writes — the createSession() insert plus one updateSession() per chunk — then a terminal status update and the row's removal (deleteSession, or the TTL/retention reaper), so 2 × (1 + N) ledger rows for one file, each with its own beforeUpdate snapshot read. Each row was unusually fat too: updateSession() writes the merged FULL record, so the `parts` JSON blob that grows with every chunk rode along in every diff's old_value/new_value. sys_file stays audited on purpose: it declares transient as well, but only to reap tombstones and unfinished uploads — its rows are mostly permanent business truth with real compliance value. Tests pin a completed 8-chunk lifecycle and an aborted-then-reaped one producing zero rows, the skipped snapshot read (with a business-object control), the deliberate non-exemption of sys_file, and that ordinary writes are still audited. Removing the one list entry turns the first three red (22, 12 and 5 unwanted writes respectively). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017MCKJaEomEqg4tvz4SzdNd --- .changeset/audit-skip-sys-upload-session.md | 35 ++++ .../plugin-audit/src/audit-writers.test.ts | 159 ++++++++++++++++++ .../plugins/plugin-audit/src/audit-writers.ts | 13 ++ 3 files changed, 207 insertions(+) create mode 100644 .changeset/audit-skip-sys-upload-session.md diff --git a/.changeset/audit-skip-sys-upload-session.md b/.changeset/audit-skip-sys-upload-session.md new file mode 100644 index 0000000000..240c93de7c --- /dev/null +++ b/.changeset/audit-skip-sys-upload-session.md @@ -0,0 +1,35 @@ +--- +"@objectstack/plugin-audit": patch +--- + +fix(plugin-audit): stop mirroring chunked-upload progress into the audit ledger (#5202) + +`SKIP_OBJECTS` in `audit-writers.ts` excludes operational telemetry / plumbing +from `sys_audit_log` and `sys_activity` — ADR-0057 decision 5, *"stop the +amplifier"*. `sys_upload_session` was the second table missing from group (2) +for the same reason `sys_job_queue` was (#5193): it declares +`lifecycle.class: 'transient'` and its own object comment says what the rows are +worth — *"an upload session is ephemeral state, never business truth"* +(ADR-0057 / #2970 item 4) — but nothing connected that declaration to the +exemption list, which is hand-written. + +The audit hooks register for **all** objects and there is no "writes made under +a system context are not audited" exemption, so `StorageMetadataStore`'s own +writes were recorded like user edits. A chunked upload of N parts costs 1 + N +writes — the `createSession()` insert plus one `updateSession()` per chunk — and +then a terminal status update and the row's removal, each producing an +`sys_audit_log` **and** an `sys_activity` row: 2 × (1 + N) rows for one file, +with a `beforeUpdate` snapshot read apiece. Each of those rows was also unusually +fat, because `updateSession()` writes the merged **full** record, so the `parts` +JSON blob that grows with every chunk rode along in each diff's `old_value` / +`new_value`. + +Nothing else changes: the exemption is one name in one list, and ordinary +business objects are audited exactly as before. In particular `sys_file` stays +audited — it declares `transient` too, but only to reap tombstones and +unfinished uploads; its rows are mostly permanent business truth and keep their +compliance value. + +Operators who tracked upload activity through `sys_activity` should read +`sys_upload_session` (in-progress state) and `sys_file` (the durable record of +what was actually stored) instead. diff --git a/packages/plugins/plugin-audit/src/audit-writers.test.ts b/packages/plugins/plugin-audit/src/audit-writers.test.ts index 75bc5bff7b..963f3a694c 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.test.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.test.ts @@ -353,6 +353,165 @@ describe('audit writers — operational plumbing is excluded (#5193, ADR-0057 D5 }); }); +describe('audit writers — chunked upload sessions are excluded (#5202, ADR-0057 D5)', () => { + // The upload-session table as `StorageMetadataStore` writes it, plus + // `sys_file` (its sibling, deliberately NOT exempted) and a business object. + // Same wildcard hooks a business object goes through — `SKIP_OBJECTS` is the + // only thing between an upload and the ledger. + const SCHEMA = { + sys_audit_log: SINGLE_TENANT.sys_audit_log, + sys_activity: SINGLE_TENANT.sys_activity, + sys_upload_session: ['id', 'file_id', 'status', 'uploaded_chunks', 'uploaded_size', 'parts', 'started_at', 'expires_at', 'updated_at'], + sys_file: ['id', 'name', 'size', 'status'], + crm_lead: ['id', 'name'], + }; + + /** + * Every write `StorageMetadataStore` performs for ONE chunked upload of + * `chunks` parts, in order: `createSession()` insert, one `updateSession()` + * per chunk, the terminal `updateSession()`, and the row's removal. + * + * `updateSession()` writes the MERGED FULL record, so `parts` — the JSON blob + * that grows by one entry per chunk — is in every single diff. The fixture + * grows it for real rather than sending a placeholder, because the size of + * those `old_value`/`new_value` payloads is half of what #5202 is about. + */ + function uploadLifecycle( + chunks: number, + terminal: { status: string; via: 'afterUpdate' | 'afterDelete' }, + ): Array<[string, Record]> { + const partsAfter = (n: number) => + JSON.stringify(Array.from({ length: n }, (_, i) => ({ part: i + 1, etag: `etag-${i + 1}` }))); + const snapshot = (n: number, status: string) => ({ + id: 'ups-1', + file_id: 'file-1', + status, + uploaded_chunks: n, + uploaded_size: n * 5_242_880, + parts: partsAfter(n), + updated_at: `2026-08-04T00:00:${String(n).padStart(2, '0')}.000Z`, + }); + + const writes: Array<[string, Record]> = [ + // createSession() — engine.insert with the full seeded record. + ['afterInsert', { input: { id: 'ups-1' }, result: snapshot(0, 'in_progress') }], + ]; + // updateSession() — ONE per chunk, each carrying the whole grown record. + for (let n = 1; n <= chunks; n += 1) { + writes.push([ + 'afterUpdate', + { + input: snapshot(n, 'in_progress'), + __previous: snapshot(n - 1, 'in_progress'), + result: snapshot(n, 'in_progress'), + }, + ]); + } + // complete() / abort() — a final updateSession() flipping status… + writes.push([ + 'afterUpdate', + { + input: { id: 'ups-1', status: terminal.status }, + __previous: snapshot(chunks, 'in_progress'), + result: snapshot(chunks, terminal.status), + }, + ]); + // …then deleteSession(), or the ADR-0057 TTL/retention reaper. + if (terminal.via === 'afterDelete') { + writes.push([ + 'afterDelete', + { input: { id: 'ups-1' }, __previous: snapshot(chunks, terminal.status), result: { id: 'ups-1' } }, + ]); + } + return writes; + } + + it('writes NO audit/activity row for a completed 8-chunk upload (create → 8 × update → complete → delete)', async () => { + const { engine, fire, created } = makeEngine(SCHEMA); + installAuditWriters(engine as any, 'test.audit'); + + const writes = uploadLifecycle(8, { status: 'completed', via: 'afterDelete' }); + // Sanity-check the fixture itself: 1 insert + 8 chunk updates + 1 terminal + // update + 1 delete. Without the exemption that is 2 × 11 = 22 ledger rows + // for one file — the 2 × (1 + N) amplifier #5202 names. + expect(writes).toHaveLength(11); + + for (const [event, ctx] of writes) { + await fire(event, { object: 'sys_upload_session', session: { isSystem: true }, ...ctx }); + } + + // Not "fewer rows" — zero. + expect(created).toEqual([]); + }); + + it('writes NO audit/activity row when the session is aborted and reaped instead', async () => { + const { engine, fire, created } = makeEngine(SCHEMA); + installAuditWriters(engine as any, 'test.audit'); + + // Abandoned mid-upload: the terminal write is the TTL reaper's DELETE of a + // row 1d past `expires_at`, not a user action. + for (const [event, ctx] of uploadLifecycle(3, { status: 'expired', via: 'afterDelete' })) { + await fire(event, { object: 'sys_upload_session', session: { isSystem: true }, ...ctx }); + } + expect(created).toEqual([]); + }); + + it('does not re-read the growing session row before every chunk update', async () => { + const { engine, fire } = makeEngine(SCHEMA); + installAuditWriters(engine as any, 'test.audit'); + const reads: string[] = []; + const ql = { + async findOne(object: string) { + reads.push(object); + return { id: 'ups-1' }; + }, + }; + + // `captureBefore` would otherwise snapshot the row — `parts` blob and all — + // once per chunk, on top of the write the store is already doing. + for (let n = 1; n <= 4; n += 1) { + await fire('beforeUpdate', { object: 'sys_upload_session', input: { id: 'ups-1', uploaded_chunks: n }, ql }); + } + await fire('beforeDelete', { object: 'sys_upload_session', input: { id: 'ups-1' }, ql }); + expect(reads).toEqual([]); + + // Control — the assertion above can fail: a business object on the same + // harness DOES get snapshotted. + await fire('beforeUpdate', { object: 'crm_lead', input: { id: 'lead-1', name: 'Acme' }, ql }); + expect(reads).toEqual(['crm_lead']); + }); + + it('still audits sys_file — mostly permanent business truth, deliberately NOT exempted', async () => { + const { engine, fire, created } = makeEngine(SCHEMA); + installAuditWriters(engine as any, 'test.audit'); + + // `sys_file` also declares `lifecycle.class: 'transient'`, but only to reap + // tombstones and unfinished uploads; the rows themselves are business truth + // with compliance value. #5202 exempts the SESSION, never the FILE — if a + // later "finish the sweep" change adds `sys_file` to `SKIP_OBJECTS`, this + // is the test that says no. + await fire('afterInsert', { + object: 'sys_file', + input: { id: 'file-1' }, + result: { id: 'file-1', name: 'contract.pdf', size: 41_943_040, status: 'ready' }, + session: { userId: 'user-1' }, + }); + expect(created.map((c) => c.object)).toEqual(['sys_audit_log', 'sys_activity']); + }); + + it('still audits ordinary business writes (the skip stays narrow)', async () => { + const { engine, fire, created } = makeEngine(SCHEMA); + installAuditWriters(engine as any, 'test.audit'); + await fire('afterInsert', { + object: 'crm_lead', + input: { id: 'lead-1' }, + result: { id: 'lead-1', name: 'Acme' }, + session: { userId: 'user-1' }, + }); + expect(created.map((c) => c.object)).toEqual(['sys_audit_log', 'sys_activity']); + }); +}); + describe('audit writers — declarative trackHistory activity (ADR-0052 §5b)', () => { // crm_opportunity with a tracked select field (Stage) carrying option labels. const SCHEMA = { diff --git a/packages/plugins/plugin-audit/src/audit-writers.ts b/packages/plugins/plugin-audit/src/audit-writers.ts index 6e05541cd2..c4697e4831 100644 --- a/packages/plugins/plugin-audit/src/audit-writers.ts +++ b/packages/plugins/plugin-audit/src/audit-writers.ts @@ -125,6 +125,19 @@ const SKIP_OBJECTS = new Set([ 'sys_notification_receipt', 'sys_inbox_message', // per-user fan-out of every notification 'sys_http_delivery', // webhook/outbound transport log + // [#5202, ADR-0057 D5 — the same gap as #5193, one table over] Also + // `lifecycle.class: 'transient'`, and its own object comment settles what the + // rows are worth: "an upload session is ephemeral state, **never business + // truth**" (ADR-0057 / #2970 item 4). `StorageMetadataStore` is its only + // writer — `createSession()` inserts, `updateSession()` runs ONCE PER CHUNK, + // and `deleteSession()` (plus the TTL/retention reaper) removes the row — so + // an N-chunk upload cost 2 × (1 + N) `sys_audit_log` + `sys_activity` rows, + // each with its own `beforeUpdate` snapshot read. Worse per row than the + // count suggests: `updateSession()` writes the MERGED FULL record, so every + // chunk's diff drags along the `parts` JSON blob that grows with each part. + // Deliberately NOT `sys_file`: those rows are mostly permanent business truth + // and keep their compliance value, so they stay audited (see #5202). + 'sys_upload_session', // chunked-upload progress (1 write per chunk) 'ai_traces', // LLM trace telemetry ]);