diff --git a/.changeset/resume-upload-expired-short-circuit.md b/.changeset/resume-upload-expired-short-circuit.md new file mode 100644 index 0000000000..169943667b --- /dev/null +++ b/.changeset/resume-upload-expired-short-circuit.md @@ -0,0 +1,26 @@ +--- +'@objectstack/client': patch +--- + +client: `storage.resumeUpload` exits on an expired session instead of uploading a chunk into it + +`resumeUpload` polls `GET .../upload/chunked/:uploadId/progress` before it sends +anything, but read only `totalChunks` / `uploadedChunks` off the response and +discarded `status`. Since #7667 a session past its own `expires_at` is durably +stamped `expired` and reported as such by that very poll — so a client resuming +a dead session learned nothing from the response it already had, uploaded a full +chunk, and discovered the expiry from the 410 `UPLOAD_SESSION_EXPIRED` the chunk +`PUT` came back with. Correct, but it spent an upload to rediscover something it +had been told. + +The poll's `status` is now read: `expired` short-circuits before the file is +read or a single byte leaves, throwing an `Error` carrying +`code: 'UPLOAD_SESSION_EXPIRED'` and `httpStatus: 410` — deliberately the same +registered code and status the server answers a chunk `PUT` against that session +with, plus `details: { uploadId, expiresAt }`. A caller already branching on +`err.code === 'UPLOAD_SESSION_EXPIRED'` keeps matching; the difference is only +how early it fires, and that the bytes stay home. + +The guard compares against `'expired'` exactly, so every other declared status +(`in_progress`, `completing`, `completed`, `failed`) resumes as before, and a +server or fixture that omits `status` is unaffected. diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index c0618ae1e0..7ea8a55c9d 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -2943,6 +2943,11 @@ export class ObjectStackClient { /** * Resume an interrupted chunked upload. * Fetches current progress, then uploads remaining chunks and completes. + * + * Throws before uploading anything when the progress poll reports the + * session as `expired` — an `Error` carrying `code` + * `'UPLOAD_SESSION_EXPIRED'` and `httpStatus` 410, the same pair the server + * answers a chunk PUT against a dead session with (#7870). */ resumeUpload: async (uploadId: string, file: Blob | ArrayBuffer, chunkSize: number, resumeToken: string): Promise => { const route = this.getRoute('storage'); @@ -2951,7 +2956,42 @@ export class ObjectStackClient { const progressRes = await this.fetch(`${this.baseUrl}${route}/upload/chunked/${uploadId}/progress`); const progress = await progressRes.json() as UploadProgress; - const { totalChunks, uploadedChunks } = progress.data; + const { totalChunks, uploadedChunks, status, expiresAt } = progress.data; + + // [#7870] A session past its own `expires_at` is durably stamped + // `expired` by the server (#7667), and THIS poll is where it says so — + // `status` is a declared member of `UploadProgressSchema`, populated on + // every progress read. Before this check the value was fetched and + // dropped: resume walked straight into the chunk loop and learned the + // session was dead from the 410 `UPLOAD_SESSION_EXPIRED` its first + // chunk PUT came back with. Honest, but it spent a whole chunk upload + // to rediscover something the response already in hand had told it. + // + // The code and status deliberately MIRROR that 410 rather than naming a + // new condition: `UPLOAD_SESSION_EXPIRED` is the registered code the + // server answers this exact case with (error-code-ledger.zod.ts), so a + // caller's existing `err.code === 'UPLOAD_SESSION_EXPIRED'` branch + // fires identically whether the expiry was caught here or by the + // server. Same error shape as the `fetch` wrapper builds for a real + // non-2xx (message + `code`/`httpStatus`/`details`) — this is an + // earlier detection of one condition, not a second one. + // + // Compared with `=== 'expired'`, never truthiness: the other declared + // statuses (`in_progress`, `completing`, `completed`, `failed`) all + // proceed exactly as before — `failed` and wider status handling are + // deliberately out of scope — and an absent `status` from a server or + // fixture that omits it cannot misfire the short-circuit. + if (status === 'expired') { + const expiredError = new Error( + `Upload session ${uploadId} expired${expiresAt ? ` at ${expiresAt}` : ''}` + + '; start a new chunked upload', + ) as Error & { code: string; httpStatus: number; details: Record }; + expiredError.code = 'UPLOAD_SESSION_EXPIRED'; + expiredError.httpStatus = 410; + expiredError.details = { uploadId, expiresAt }; + throw expiredError; + } + const parts: Array<{ chunkIndex: number; eTag: string }> = []; // 2. Upload remaining chunks diff --git a/packages/client/src/storage-wire-dialect.test.ts b/packages/client/src/storage-wire-dialect.test.ts index 2745c8901d..a5a808f4f6 100644 --- a/packages/client/src/storage-wire-dialect.test.ts +++ b/packages/client/src/storage-wire-dialect.test.ts @@ -136,3 +136,113 @@ describe('the enveloped storage responses match their declared return types (#36 expect(res.data.eTag).toBe('"abc"'); }); }); + +/** + * `resumeUpload` short-circuits on the expiry the progress poll already told it + * about (#7870). + * + * Since #7667 a chunked session past its own `expires_at` is durably stamped + * `expired`, `GET .../progress` reports that status, and a chunk PUT against it + * answers 410 `UPLOAD_SESSION_EXPIRED`. `resumeUpload` polls progress FIRST but + * read only the chunk counters off it, so it uploaded a full chunk into a + * session the poll had already declared dead and learned that from the 410. + * + * What these pin is the pair a caller actually branches on — `code` AND + * `httpStatus` — not merely that something threw: the point of the fix is that + * the early exit is INDISTINGUISHABLE from the server's own refusal, so a + * `catch` written against the 410 keeps matching. Asserting only "it throws" + * would stay green if the short-circuit raised a bare `Error`, which is the one + * outcome that would break every such caller. + */ +describe('storage.resumeUpload exits on an expired session before uploading (#7870)', () => { + /** The progress body the server sends for a session past its deadline. */ + const expiredProgress = { + success: true, + data: { + uploadId: 'up1', + fileId: 'f1', + filename: 'big.bin', + totalSize: 16, + uploadedSize: 8, + totalChunks: 2, + uploadedChunks: 1, + percentComplete: 50, + status: 'expired', + startedAt: '2026-01-01T00:00:00.000Z', + expiresAt: '2026-01-01T01:00:00.000Z', + }, + }; + + it('throws the registered code and status the server answers a dead session with', async () => { + const { client } = clientReturning(expiredProgress); + + // `.rejects.toThrow()` alone cannot see the difference between this and a + // bare Error, so the envelope is asserted off the caught object. + const err = await client.storage + .resumeUpload('up1', new ArrayBuffer(16), 8, 'rtok') + .then(() => null, (e: any) => e); + + expect(err).toBeInstanceOf(Error); + expect(err.code).toBe('UPLOAD_SESSION_EXPIRED'); + expect(err.httpStatus).toBe(410); + // The expiry instant is what tells a caller/operator WHICH deadline passed, + // so it survives into both the human message and `details`. + expect(err.message).toContain('2026-01-01T01:00:00.000Z'); + expect(err.details).toMatchObject({ uploadId: 'up1', expiresAt: '2026-01-01T01:00:00.000Z' }); + }); + + it('sends no chunk PUT and no complete — the poll is the only request made', async () => { + // The whole point of the card: the bytes never leave. If this regresses, + // the throw above would still pass while the client had already spent a + // chunk upload against a dead session. + const { client, fetchMock } = clientReturning(expiredProgress); + + await client.storage.resumeUpload('up1', new ArrayBuffer(16), 8, 'rtok').catch(() => {}); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock.mock.calls[0][0]).toContain('/upload/chunked/up1/progress'); + }); + + it('still resumes normally when the session is live', async () => { + // The reverse direction: `=== 'expired'` must not swallow the happy path. + // One mock, routed by URL — resume makes three hops (progress, chunk PUT, + // complete) and they answer different bodies. + const fetchMock = vi.fn(async (url: string) => { + const body = url.includes('/progress') + ? { success: true, data: { ...expiredProgress.data, status: 'in_progress' } } + : url.includes('/complete') + ? { success: true, data: { fileId: 'f1', size: 16 } } + : { success: true, data: { chunkIndex: 1, eTag: '"e2"', bytesReceived: 8 } }; + return { ok: true, status: 200, statusText: 'OK', json: async () => body, headers: new Headers() }; + }); + const client = new ObjectStackClient({ baseUrl: 'http://localhost:3000', fetch: fetchMock as any }); + + const res = await client.storage.resumeUpload('up1', new ArrayBuffer(16), 8, 'rtok'); + + expect(res.success).toBe(true); + expect(fetchMock.mock.calls.map((c) => String(c[0]))).toEqual([ + expect.stringContaining('/upload/chunked/up1/progress'), + expect.stringContaining('/upload/chunked/up1/chunk/1'), + expect.stringContaining('/upload/chunked/up1/complete'), + ]); + }); + + it('does not misfire when a server or fixture omits status entirely', async () => { + // `status` is declared required, but the client is published separately + // from the server it meets and the SDK's own URL-conformance fixture drives + // this method with a counters-only body. An absent status must resume, not + // abort — which is why the guard compares against 'expired' rather than + // testing truthiness or absence. + const fetchMock = vi.fn(async (url: string) => { + const body = url.includes('/progress') + ? { success: true, data: { totalChunks: 1, uploadedChunks: 0 } } + : { success: true, data: { chunkIndex: 0, eTag: '"e1"', bytesReceived: 8 } }; + return { ok: true, status: 200, statusText: 'OK', json: async () => body, headers: new Headers() }; + }); + const client = new ObjectStackClient({ baseUrl: 'http://localhost:3000', fetch: fetchMock as any }); + + await expect( + client.storage.resumeUpload('up1', new ArrayBuffer(8), 8, 'rtok'), + ).resolves.toBeDefined(); + }); +});