Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .changeset/resume-upload-expired-short-circuit.md
Original file line number Diff line number Diff line change
@@ -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.
42 changes: 41 additions & 1 deletion packages/client/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<CompleteChunkedUploadResponse> => {
const route = this.getRoute('storage');
Expand All @@ -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<string, any> };
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
Expand Down
110 changes: 110 additions & 0 deletions packages/client/src/storage-wire-dialect.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
Loading