From 7c800c62f05b4e954f9592ca82dacfbdc503cb8a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 03:11:42 +0000 Subject: [PATCH] fix(storage): abort backend multipart when reaping abandoned sys_upload_session (#2970) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sys_upload_session lifecycle (#2984) reaps abandoned/terminal chunked-upload session rows but not the underlying backend multipart — on S3 an initiated-but-not-completed multipart keeps its parts billable and invisible until AbortMultipartUpload, so reaping only the row stranded them (backend_upload_id, the sole pointer, gone). createUploadSessionReapGuard registers a LifecycleReapGuard on sys_upload_session that aborts the backend multipart before the row is deleted: skips 'completed' sessions (multipart already an object — an abort would NoSuchUpload-error), re-seeds the S3 uploadId->key map from the row (cold sweeps lack the live in-process map), and vetoes (keeps the row for retry) on abort failure so the pointer survives. Local adapter parts dir removed the same way. Verified: 7 new unit tests (S3 key re-seed, completed-skip, no-backend, veto-on-failure, local adapter, no-abort adapter); service-storage 99; dogfood matrix drives a real chunked upload → part on disk → sweep → row gone AND parts dir aborted. Closes the last #2970 sub-follow-up. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_0187NT3Qer9oep5dCRb9b8Lt --- .../upload-session-multipart-abort-guard.md | 20 ++++ ...achments-permission-matrix.dogfood.test.ts | 25 ++++- .../src/attachment-lifecycle.test.ts | 91 +++++++++++++++++++ .../src/attachment-lifecycle.ts | 64 +++++++++++++ .../services/service-storage/src/index.ts | 2 +- .../src/storage-service-plugin.test.ts | 6 +- .../src/storage-service-plugin.ts | 10 +- 7 files changed, 208 insertions(+), 10 deletions(-) create mode 100644 .changeset/upload-session-multipart-abort-guard.md diff --git a/.changeset/upload-session-multipart-abort-guard.md b/.changeset/upload-session-multipart-abort-guard.md new file mode 100644 index 0000000000..8b925a1132 --- /dev/null +++ b/.changeset/upload-session-multipart-abort-guard.md @@ -0,0 +1,20 @@ +--- +'@objectstack/service-storage': patch +--- + +fix(storage): abort the backend multipart upload when reaping an abandoned sys_upload_session (#2970) + +The `sys_upload_session` lifecycle (added in #2984) reaps abandoned/terminal +chunked-upload session ROWS, but not the underlying backend multipart upload — +on S3 an initiated-but-not-completed multipart keeps its already-uploaded parts +billable and invisible to normal listing until an explicit +`AbortMultipartUpload`, so reaping only the row stranded them (with +`backend_upload_id`, the sole pointer, gone). + +`createUploadSessionReapGuard` registers a `LifecycleReapGuard` on +`sys_upload_session` that aborts the backend multipart before the row is +deleted: it skips `completed` sessions (their multipart already became a real +object — an abort would `NoSuchUpload`-error), re-seeds the S3 adapter's +`uploadId → key` map from the row (a cold sweep lacks the live in-process map), +and vetoes (keeps the row for retry) on abort failure so the pointer survives. +The local adapter's parts directory is removed the same way. diff --git a/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts b/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts index d35b3220c0..a12a71b2b8 100644 --- a/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts +++ b/packages/dogfood/test/attachments-permission-matrix.dogfood.test.ts @@ -476,19 +476,34 @@ describe('attachments permission matrix (#2755)', () => { expect(await ql.findOne('sys_file', { where: { id: data.fileId }, context: SYS })).toBeNull(); }); - it('(item 4) abandoned sys_upload_session rows are reaped past their expiry window', async () => { - // Initiate a chunked upload (creates a sys_upload_session) but never - // complete it. + it('(item 4 + multipart-abort guard) an abandoned chunked upload is reaped AND its uploaded parts are aborted', async () => { + // Initiate a chunked upload (creates a sys_upload_session) and upload one + // chunk — but never complete it. The chunk lands as a part on disk. const init = await stack.api('/storage/upload/chunked', { method: 'POST', headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${memberATok}` }, body: JSON.stringify({ filename: 'big.bin', mimeType: 'application/octet-stream', totalSize: 10_485_760 }), }); expect(init.status).toBe(200); - const { uploadId } = ((await init.json()) as any).data; + const { uploadId, resumeToken } = ((await init.json()) as any).data; const session = await ql.findOne('sys_upload_session', { where: { id: uploadId }, context: SYS }); expect(session?.id, 'session row created').toBeTruthy(); + const chunkRes = await stack.api(`/storage/upload/chunked/${uploadId}/chunk/0`, { + method: 'PUT', + headers: { + Authorization: `Bearer ${memberATok}`, + 'Content-Type': 'application/octet-stream', + 'x-resume-token': resumeToken, + }, + body: Buffer.from('a partial chunk of bytes'), + }); + expect(chunkRes.status, await chunkRes.clone().text()).toBeLessThan(300); + + // The local adapter stores parts under `/.parts//`. + const partsDir = join(rootDir, '.parts', String(uploadId)); + await expect(fs.access(partsDir), 'part exists before the sweep').resolves.toBeUndefined(); + // Backdate expires_at past the 1d TTL grace. await ql.update( 'sys_upload_session', @@ -498,7 +513,9 @@ describe('attachments permission matrix (#2755)', () => { const report = await lifecycle.sweep(); expect(report.errors, JSON.stringify(report.errors)).toEqual([]); + // Row gone AND the backend multipart parts aborted (dir removed). expect(await ql.findOne('sys_upload_session', { where: { id: uploadId }, context: SYS })).toBeNull(); + await expect(fs.access(partsDir), 'parts aborted by the reap guard').rejects.toThrow(); }); }); }); diff --git a/packages/services/service-storage/src/attachment-lifecycle.test.ts b/packages/services/service-storage/src/attachment-lifecycle.test.ts index 13b5ba7543..656d185ab1 100644 --- a/packages/services/service-storage/src/attachment-lifecycle.test.ts +++ b/packages/services/service-storage/src/attachment-lifecycle.test.ts @@ -4,6 +4,7 @@ import { describe, it, expect, vi } from 'vitest'; import { installAttachmentLifecycleHooks, createSysFileReapGuard, + createUploadSessionReapGuard, type AttachmentLifecycleEngine, } from './attachment-lifecycle.js'; @@ -247,3 +248,93 @@ describe('createSysFileReapGuard', () => { expect(confirmed).toEqual([]); }); }); + +describe('createUploadSessionReapGuard', () => { + /** Swappable-style storage fake: `getInner()` exposes an S3-like inner with + * `setUploadKey`; `abortChunkedUpload` is forwarded. */ + const s3Storage = (abortImpl?: () => Promise) => { + const inner = { + setUploadKey: vi.fn((_id: string, _key: string) => {}), + }; + return { + getInner: () => inner, + abortChunkedUpload: vi.fn(abortImpl ?? (async () => {})), + _inner: inner, + } as any; + }; + + it('aborts the backend multipart (re-seeding the key) then reaps an abandoned session', async () => { + const s = s3Storage(); + const guard = createUploadSessionReapGuard(() => s, silentLogger()); + + const confirmed = await guard('sys_upload_session', [ + { id: 'u1', backend_upload_id: 'mp-1', key: 'attachments/u1.bin', status: 'in_progress' }, + ]); + + expect(s._inner.setUploadKey).toHaveBeenCalledWith('mp-1', 'attachments/u1.bin'); + expect(s.abortChunkedUpload).toHaveBeenCalledWith('mp-1'); + expect(confirmed).toEqual(['u1']); + }); + + it('does NOT abort a completed session (its multipart is already an object) — just reaps the row', async () => { + const s = s3Storage(); + const guard = createUploadSessionReapGuard(() => s, silentLogger()); + + const confirmed = await guard('sys_upload_session', [ + { id: 'u2', backend_upload_id: 'mp-2', key: 'attachments/u2.bin', status: 'completed' }, + ]); + + expect(s.abortChunkedUpload).not.toHaveBeenCalled(); + expect(confirmed).toEqual(['u2']); + }); + + it('reaps a session with no backend_upload_id without calling abort', async () => { + const s = s3Storage(); + const guard = createUploadSessionReapGuard(() => s, silentLogger()); + + const confirmed = await guard('sys_upload_session', [ + { id: 'u3', key: 'attachments/u3.bin', status: 'expired' }, + ]); + + expect(s.abortChunkedUpload).not.toHaveBeenCalled(); + expect(confirmed).toEqual(['u3']); + }); + + it('VETOES on abort failure so backend_upload_id survives for the retry', async () => { + const s = s3Storage(async () => { + throw new Error('S3 abort transient failure'); + }); + const logger = silentLogger(); + const guard = createUploadSessionReapGuard(() => s, logger); + + const confirmed = await guard('sys_upload_session', [ + { id: 'u4', backend_upload_id: 'mp-4', key: 'attachments/u4.bin', status: 'failed' }, + ]); + + expect(confirmed).toEqual([]); // kept + expect(logger.warn).toHaveBeenCalled(); + }); + + it('works with a local-style adapter (no setUploadKey / getInner) — aborts by id', async () => { + const local = { abortChunkedUpload: vi.fn(async () => {}) } as any; + const guard = createUploadSessionReapGuard(() => local, silentLogger()); + + const confirmed = await guard('sys_upload_session', [ + { id: 'u5', backend_upload_id: 'local-5', key: 'attachments/u5.bin', status: 'expired' }, + ]); + + expect(local.abortChunkedUpload).toHaveBeenCalledWith('local-5'); + expect(confirmed).toEqual(['u5']); + }); + + it('reaps the row when the adapter cannot abort at all', async () => { + const noAbort = {} as any; + const guard = createUploadSessionReapGuard(() => noAbort, silentLogger()); + + const confirmed = await guard('sys_upload_session', [ + { id: 'u6', backend_upload_id: 'mp-6', key: 'k', status: 'in_progress' }, + ]); + + expect(confirmed).toEqual(['u6']); + }); +}); diff --git a/packages/services/service-storage/src/attachment-lifecycle.ts b/packages/services/service-storage/src/attachment-lifecycle.ts index e705b7f7f2..1549ab4224 100644 --- a/packages/services/service-storage/src/attachment-lifecycle.ts +++ b/packages/services/service-storage/src/attachment-lifecycle.ts @@ -259,3 +259,67 @@ export function createSysFileReapGuard( return confirmed; }; } + +/** + * The `sys_upload_session` reap guard (#2970 sub-follow-up). The + * LifecycleService reaps abandoned/terminal chunked-upload session ROWS by the + * declared TTL (`expires_at`) / retention (terminal statuses); this guard + * aborts the underlying BACKEND multipart upload before the row is deleted, so + * a session's already-uploaded parts don't leak. On S3 an initiated-but-not- + * completed multipart keeps its parts billable and invisible to normal listing + * until an explicit AbortMultipartUpload — reaping only the row would strand + * them, with `backend_upload_id` (the sole pointer) gone. + * + * - `completed`: the multipart was already finalized into a real object — + * nothing to abort (an abort would `NoSuchUpload`-error and wedge the reap). + * Confirm the row. + * - no `backend_upload_id`, or an adapter without `abortChunkedUpload`: + * nothing to abort. Confirm. + * - otherwise (in_progress / failed / expired with a backend upload): abort + * the backend multipart, re-seeding the S3 `uploadId → key` map from the row + * first (a cold sweep lacks the live in-process map; a no-op for adapters + * that don't track keys, e.g. local). Confirm on success; VETO on failure + * so the row — the only pointer to the leaked multipart — is retried. + */ +export function createUploadSessionReapGuard( + getStorage: () => IStorageService | null | undefined, + logger: AttachmentLifecycleLogger, +): (object: string, rows: Array>) => Promise> { + return async (_object, rows) => { + const confirmed: Array = []; + const storage = getStorage(); + for (const row of rows) { + const id = row?.id as string | number | undefined; + if (id === undefined || id === null) continue; + + const backendId = typeof row.backend_upload_id === 'string' ? row.backend_upload_id : ''; + // Nothing to abort → just reap the row: no backend multipart, an already + // -completed upload (its parts became an object), or an adapter that + // can't abort. + if (!backendId || row.status === 'completed' || !storage || typeof storage.abortChunkedUpload !== 'function') { + confirmed.push(id); + continue; + } + + try { + // A cold sweep runs long after the live session, so the S3 adapter's + // in-process `uploadId → key` map (populated by `setUploadKey` during + // upload) is empty — re-seed it from the row so the abort can resolve + // the S3 key. `setUploadKey` is S3-specific and not forwarded by the + // swappable proxy, so reach the inner adapter; a no-op for `local`. + const inner: any = typeof (storage as any).getInner === 'function' ? (storage as any).getInner() : storage; + if (typeof row.key === 'string' && row.key && typeof inner?.setUploadKey === 'function') { + inner.setUploadKey(backendId, row.key); + } + await storage.abortChunkedUpload(backendId); + confirmed.push(id); + } catch (err) { + logger.warn( + `[storage] reap guard: multipart abort failed for sys_upload_session ${id} (${(err as Error)?.message ?? err}); retrying next sweep`, + ); + // veto — keep the row so `backend_upload_id` survives for the retry. + } + } + return confirmed; + }; +} diff --git a/packages/services/service-storage/src/index.ts b/packages/services/service-storage/src/index.ts index 0e5434f511..f40cd462cc 100644 --- a/packages/services/service-storage/src/index.ts +++ b/packages/services/service-storage/src/index.ts @@ -12,7 +12,7 @@ export type { FileRecord, UploadSessionRecord } from './metadata-store.js'; export { registerStorageRoutes } from './storage-routes.js'; export type { StorageRoutesOptions, FileReadVerdict } from './storage-routes.js'; export { SystemFile, SystemUploadSession } from './objects/index.js'; -export { installAttachmentLifecycleHooks, createSysFileReapGuard } from './attachment-lifecycle.js'; +export { installAttachmentLifecycleHooks, createSysFileReapGuard, createUploadSessionReapGuard } from './attachment-lifecycle.js'; export type { AttachmentLifecycleEngine, AttachmentLifecycleLogger } from './attachment-lifecycle.js'; export { installAttachmentAccessHooks, installAttachmentReadVisibility } from './attachment-access-hooks.js'; export type { AttachmentSharingLike } from './attachment-access-hooks.js'; diff --git a/packages/services/service-storage/src/storage-service-plugin.test.ts b/packages/services/service-storage/src/storage-service-plugin.test.ts index 3821d37b1f..e7a678ec39 100644 --- a/packages/services/service-storage/src/storage-service-plugin.test.ts +++ b/packages/services/service-storage/src/storage-service-plugin.test.ts @@ -229,9 +229,9 @@ describe('StorageServicePlugin: sys_file orphan lifecycle wiring (#2755)', () => 'beforeDelete', 'beforeInsert', ]); - expect(guards).toHaveLength(1); - expect(guards[0].object).toBe('sys_file'); - expect(typeof guards[0].guard).toBe('function'); + // Two reap guards: sys_file (#2755) + sys_upload_session multipart-abort (#2970). + expect(guards.map((g) => g.object).sort()).toEqual(['sys_file', 'sys_upload_session']); + expect(guards.every((g) => typeof g.guard === 'function')).toBe(true); // Read-visibility middleware registered on sys_attachment (#2970 item 1). expect(middlewares).toEqual([{ object: 'sys_attachment' }]); }); diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index f64e890b62..c2bc80b5cc 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -16,7 +16,7 @@ import { StorageMetadataStore } from './metadata-store.js'; import type { FileRecord } from './metadata-store.js'; import { registerStorageRoutes } from './storage-routes.js'; import type { FileReadVerdict } from './storage-routes.js'; -import { installAttachmentLifecycleHooks, createSysFileReapGuard } from './attachment-lifecycle.js'; +import { installAttachmentLifecycleHooks, createSysFileReapGuard, createUploadSessionReapGuard } from './attachment-lifecycle.js'; import { installAttachmentAccessHooks, installAttachmentReadVisibility } from './attachment-access-hooks.js'; import { SystemFile, SystemUploadSession } from './objects/index.js'; // ADR-0052 §3 ownership: `sys_attachment` (a file↔record link) belongs with the @@ -228,7 +228,13 @@ export class StorageServicePlugin implements Plugin { 'sys_file', createSysFileReapGuard(engine as any, () => this.storage, ctx.logger), ); - ctx.logger.info('StorageServicePlugin: sys_file reap guard registered with the lifecycle service'); + // Abort the backend multipart upload before an abandoned/terminal + // sys_upload_session row is reaped, so its parts don't leak (#2970). + lifecycle.registerReapGuard( + 'sys_upload_session', + createUploadSessionReapGuard(() => this.storage, ctx.logger), + ); + ctx.logger.info('StorageServicePlugin: sys_file + sys_upload_session reap guards registered with the lifecycle service'); } } catch { // lifecycle service absent (bare kernel) — the sys_file lifecycle