fix(service-storage): give failed/expired upload-session statuses a producer (#7667) - #7844
Merged
huangyiirene merged 2 commits intoAug 12, 2026
Merged
Conversation
… a producer (#7667) `sys_upload_session.status` declared `failed` and `expired`, the retention backstop reaped on both, and `UploadProgressSchema` published both to every client reading the contract — while nothing in the service ever wrote either. A scan of every session row could only return `in_progress`/`completed`, so the retention rule named two states the system could not enter. ADR-0049 enforce-or-remove, taking the ENFORCE branch: removal would have forked the object from the spec's progress contract, and both failure states are real and were previously invisible. - `failed`: a completion whose backend `completeChunkedUpload` threw left the row at `completing` — non-terminal, so the 7d retention backstop never reaped it and a progress poll read "still assembling" indefinitely. The completion route now stamps `failed` on that path. It records an attempt rather than locking the session: a retry runs the happy path and overwrites it with `completed`. - `expired`: a session past its own `expires_at` kept answering `in_progress` and kept accepting chunks until the TTL sweep deleted the row out from under the caller, so the deadline the init response announced bound nothing. A chunk PUT or a complete against an overdue session is now refused 410 `UPLOAD_SESSION_EXPIRED` (registered under `@objectstack/service-storage` in `ERROR_CODE_LEDGER`) and the row is durably stamped `expired`. Progress REPORTS the status rather than refusing — `expired` is a declared member of `UploadProgressSchema.status` and the SDK's `resumeUpload` polls it first. A row with no `expires_at` carries no declared deadline and is left alone; a `completed` row does not become `expired` by waiting for the reaper. The `failed` stamp is best-effort and loud on failure, so a metadata-store error never replaces the real backend cause on its way to the 500. Checklist item `attachments-storage.upload-session-abort` revision 3 records the producers, adds steps that drive both statuses, and records transient `completing` as a knownGap rather than an unreachable-variant FAIL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0198Jr94CUGy2vDGtT1L8pka
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 107 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 7 release-owned page(s) also reference the affected code. These are read-only:
|
github-actions
Bot
added
documentation
Collaborator
Author
|
PATCH ROUND (services seat, session The red: CI's "TypeScript Type Check" job fails on The fix, in order (for the original dev on
⛔ Nothing else on the PR changes; no CI idle-polling — the PM owns the ready-flip and landing. Generated by Claude Code |
…edger entry (#7667) 产物随源走: registering `UPLOAD_SESSION_EXPIRED` in `ERROR_CODE_LEDGER` widens the `ErrorCode` union every enveloped response references, so all 11 `content/docs/references/api/*.mdx` pages that render it were stale and `check:docs` (`build-docs.ts --check`) failed the TypeScript Type Check job. The whole diff is that one addition propagating: a new `UPLOAD_SESSION_EXPIRED` bullet in `error-code-ledger.mdx`, and the union arity in every rendered `error` column moving `+260 more` → `+261 more`. No unrelated drift was absorbed. Generated, not hand-written: `pnpm --filter @objectstack/spec gen:schema && gen:docs` on a clean tree with no merge in progress (#5370). `authorable-surface.base.json` was not touched, so no re-anchoring rode along; `json-schema/openapi.json` was not cleared by the rmSync and was refreshed anyway (#5371, gitignored either way). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0198Jr94CUGy2vDGtT1L8pka
huangyiirene
marked this pull request as ready for review
August 12, 2026 00:39
huangyiirene
deleted the
claude/issue-7667-upload-session-status-liveness
branch
August 12, 2026 00:56
This was referenced Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7667
Premise: verified on
origin/main(098f4bb)All three anchors held. Enum members at
system-upload-session.object.ts:104-105, retentiononlyWhenat:134, type union atmetadata-store.ts:54, and a repo grep confirms no writer for either status anywhere inservice-storage.Branch taken: ENFORCE, not remove
The deciding evidence is one file the issue does not mention:
packages/spec/src/api/storage.zod.ts:247declaresUploadProgressSchema.statusasz.enum(['in_progress','completing','completed','failed','expired']). Both statuses are published to every client that reads the contract — so they are not merely declared-and-unread; the API promises them. Removing them from the object enum would have forked the object from the spec's progress contract and forced apackages/specnarrowing (the dispatch's STOP condition). Enforcing gives that contract teeth instead.Both statuses also name real failure states that were previously invisible:
failed— a completion whose backendcompleteChunkedUploadthrew left the row atcompleting. That is a non-terminal status, so the 7d retention backstop ($in ['completed','failed','expired']) never reaped it, and a progress poll reported "still assembling" for a session that had already given up. The completion route now stampsfailedon that path. It records an attempt rather than locking the session — nothing readsfailedas a refusal, so a retry of the sameuploadIdruns the happy path and overwrites it withcompleted(pinned by a test).expired— a session past its ownexpires_atkept answeringin_progressand kept accepting chunks, until the TTL sweep deleted the row out from under the caller. The deadline the init response already announced (expiresAt) bound nothing. A chunkPUTor acompleteagainst an overdue session is now refused 410UPLOAD_SESSION_EXPIREDand the row is durably stampedexpired.GET .../progressreports the status rather than refusing —expiredis a declared member ofUploadProgressSchema.status, and the SDK'sresumeUploadpolls progress first.The retention
onlyWhenis now consistent with the enum in the strong direction: every member it names has a writer instorage-routes.ts. The reap guard (createUploadSessionReapGuard) already handled both statuses — it aborts the backend multipart for any non-completedrow carrying abackend_upload_id— so this closes that loop rather than opening a new one.Deliberate non-behaviours, each pinned by a test: a row with no
expires_atcarries no declared deadline and is left alone (the guard enforces the row's own deadline, it does not invent one); acompletedrow does not becomeexpiredby waiting for the reaper; expiry is checked after the resume-token check, so a caller who cannot prove it owns the session learns nothing about its state.packages/spec/src/api/error-code-ledger.zod.ts— one additive line, registeringUPLOAD_SESSION_EXPIREDunder@objectstack/service-storage.ErrorCodeis the closed unionStandardErrorCode ∪ ERROR_CODE_LEDGER, and an unregistered code fails schema parse → fails the envelope conformance suites → fails CI (ADR-0112). The alternative was reusing a semantically wrong existing code (UPLOAD_SESSION_NOT_FOUNDon a session that plainly exists), which is precisely the bypass the ledger's own header warns about. Purely additive: the union grows, nothing that validated before stops validating.The
#5536ride-along clause did not trigger — this diff does not touchstorage-service-plugin.ts.Checklist maintenance
docs/qa/platform-checklist/areas/attachments-storage.json→attachments-storage.upload-session-abortrevision 3, closing the clause-5 finding revision 2 explicitly deferred to this issue: two steps that drivefailedandexpired, a new acceptance clause for the 410 refusal, and the producers named insource. Transientcompletingis recorded as aknownGaprather than an unreachable-variant FAIL — the complete route writes it and overwrites it in the same request, so a scan of settled rows will never show it, and that is correct.Gates (all run locally, all green)
pnpm build(full closure)service-storagesuitespec—error-code-ledger+storagepnpm check:docs-audit-scopepnpm check:platform-checklistpnpm check:route-envelopesendError)pnpm typecheck(spec)service-storagedeclares notypecheckscript (not type-check-covered); an ad-hoctsc --noEmitover it reports only the pre-existing extension-less test-import noise, none from this diff.No new test fake with update/delete verbs was added — the tests drive the real
StorageMetadataStoreon its no-engine Map path, so theassertEngineUpdateDispatch/assertEngineDeleteDispatchgate does not apply.Follow-up (not taken here — out of this card's file surface)
packages/client'sstorage.resumeUploadreads the progress body but ignoresstatus, so resuming an expired session now surfaces as a 410 on the first chunkPUTrather than a clean early exit. That is an honest failure, not a regression, but a short-circuit onstatus === 'expired'would be a better client experience. Worth a separatedomain:clientcard.Generated by Claude Code