Repository navigation
fix(service-storage): the upload commit door refuses a file whose bytes the storage backend does not hold - #24643
Conversation
…backend does not hold POST /storage/upload/complete committed a sys_file row without asking the storage backend, so a presigned upload whose byte PUT failed or was never sent committed with its declared size, and the download door handed out a URL that failed 500. The door now asks exists(key) and compares the backend's getInfo(key).size with the declared size, and answers a file it does not hold 409 RESOURCE_CONFLICT with the row left pending - the answer the chunked-completion sibling already gives. The local raw byte route answers a signed GET for a key with no file 404 FILE_NOT_FOUND (ENOENT only); every other read failure keeps its 500. Claude-Session: https://claude.ai/code/session_01LR1cD9JqgkeqY7k9rrZH8s Co-authored-by: Claude <noreply@anthropic.com>
Five existing suites drove POST /upload/complete over a file whose bytes the backend never held - the state the commit door now refuses 409. Each now stores the declared bytes before the commit, so it keeps measuring what it was written for (ownership, organization reach, the success envelope, the scoped write, the outage answer). Claude-Session: https://claude.ai/code/session_01LR1cD9JqgkeqY7k9rrZH8s Co-authored-by: Claude <noreply@anthropic.com>
…commit; changeset The two booted-wall suites committed presigned uploads whose bytes were never sent - the state the commit door now refuses 409. Each commit that is meant to land now PUTs the declared bytes to the URL its presign answered first. Adds the patch changeset for service-storage. Claude-Session: https://claude.ai/code/session_01LR1cD9JqgkeqY7k9rrZH8s Co-authored-by: Claude <noreply@anthropic.com>
…mplete-holds-bytes
📓 Docs Drift CheckThis PR changes 1 package(s): 1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 9 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 6f609827f6c416e46f0053357396e0989d28a90f && git checkout 6f609827f6c416e46f0053357396e0989d28a90f
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f89d29e382e7d50cfe1b2a98742ac1c3d436cd26 c6ab54cd099ea65c97055d87ec95e1c90b926a86 && git checkout -B drift-repro f89d29e382e7d50cfe1b2a98742ac1c3d436cd26 && git merge --no-ff c6ab54cd099ea65c97055d87ec95e1c90b926a86
node scripts/docs-audit/affected-docs.mjs --json f89d29e382e7d50cfe1b2a98742ac1c3d436cd26
|
The commit door answered 200 and now answers 409 for a completion whose bytes the backend does not hold: an accept-set narrowing. The changeset carries the Clause-② narrowing arm, ships minor under the launch window, and states its ADR-0087 disposition (not-required, no migration prescription). Claude-Session: https://claude.ai/code/session_01LR1cD9JqgkeqY7k9rrZH8s Co-authored-by: Claude <noreply@anthropic.com>
Contract reviewServed-tier: Inputs: card #24548 (body and its three comments: triage 6269100155, claim 6274788835, os-dev report 6276196635), PR #24643 (body, file list, net diff of the head against ① Derived judgments
② Semver levelThe changeset The PR body's ③ Boundary flags
Implemented-by: VERDICT: PASS Generated by Claude Code |
… local adapter answers 409 The local adapter's exists() reports every fs.access failure as absent, so an unreadable local root answers the 409, not a 500. The release note now says so instead of promising a 500 for every backend fault. Claude-Session: https://claude.ai/code/session_01LR1cD9JqgkeqY7k9rrZH8s Co-authored-by: Claude <noreply@anthropic.com>
席位核对ACCEPT · #24548 ·
Generated by Claude Code |
Fixes #24548
Clause-②: no (narrowing)
Summary
POST /api/v1/storage/upload/completecommitted asys_filerow without asking the storage backend. A presigned upload whose byte PUT failed, or was never sent, therefore committed with its declared size. The download door then handed out a signed URL, and fetching it failed500.The commit door now asks the backend before it commits. When the backend holds no bytes for the file, or holds a different number of bytes than the upload declared, the door answers
409/RESOURCE_CONFLICT(ADR-0112 envelope) and the row stayspending. That is the answer the chunked completion door already gives (INCOMPLETE_UPLOAD_STATUS/INCOMPLETE_UPLOAD_CODE), and it is what the download door's servability rule assumes:committedmeans there are bytes to serve. A completion whose bytes are held keeps its200/committedanswer, unchanged on the local and S3 adapters.Secondary: the local raw download route (
GET {basePath}/_local/raw/:token) answers404/FILE_NOT_FOUNDfor a key that holds no file, where it answered500"Download failed". Only that case moves; every other read failure keeps its500.No
packages/spec/**edit. Nothing spec declares moves.What changed
packages/services/service-storage/src/storage-routes.tsheldBytesRefusal(storage, file)asksstorage.exists(file.key), then comparesstorage.getInfo(file.key).sizewith the declared size. A non-null answer is sent as409 RESOURCE_CONFLICTwith a message that says how to finish the upload. A backend fault while asking is not caught there and answers500through the route's existing catch, where the adapter reports one: the S3 adapter does, and so does the size leg. The local adapter'sexistsreports everyfs.accessfailure as absent, so an unreadable local root answers the409(contract review6277131294, item 3).storage.downloadcall is wrapped. Node'sENOENT(whatLocalStorageAdapter.downloadpasses through for a missing file) answers404 FILE_NOT_FOUND. Anything else is rethrown to the existingsendRawByteRouteFailureand keeps its500.storage-routes.complete-holds-bytes.test.ts..changeset/24548-complete-holds-bytes.md(minor,@objectstack/service-storage): a declared narrowing (Clause-②: no (narrowing)) in the launch window, with its ADR-0087 dispositionnot-required (no-migration-prescription).Mechanism assumptions, measured
973917c57the handler checked session, row, uploader and organisation, then wrotecommitted.exists(key)is a requiredIStorageServicemember, implemented by the local adapter (fs.access), the S3 adapter (HeadObject,404read as false, anything else thrown) and the swappable wrapper (forwards).getInfo(key)is a required, already-declared member that reportssizeon both adapters (fs.statsize;HeadObjectContentLength). No new adapter method and no contract member were added. A row that declares no size has nothing to compare and skips that leg.StorageFileInfocarries no ETag, and the S3 adapter'sgetInfodrops theETagitsHeadObjectanswer holds, so no existing path reads the stored ETag. Comparing it would need a new contract member, which this card excludes. The eTag is still stored as sent.ENOENTfromfs.readFileinLocalStorageAdapter.download, passed through unchanged bytrack()and by the swappable wrapper. A key that names a directory (EISDIR) still answers500, pinned.Measured: before and after
Pins committed with the fix in
b80a71c17; the red leg ran the new file against the unfixedstorage-routes.tson973917c57:8 failed | 4 passed. Local adapter and swappable wrapper: a commit with no bytes answered200("status":"committed"), a wrong-size commit answered200. The raw GET for a missing key answered500{"code":"INTERNAL","message":"Download failed"}. The three S3 legs failed in that run on a harness error (the adapter refuses an unsetkeyPrefix), corrected before the green run. Their red evidence is ablation A below.12 passed.Ablations, each through
scripts/ablation-replace.mjs(anchor hit 1, blob changed, restored to blobdab71a45aac7equal to HEAD,git diff HEADempty). These suites import./storage-routes.jsfrom source, so no rebuild sits between the mutation and the run:const notHeld = await heldBytesRefusal(storage, file);replaced by a null):6 failed | 6 passed. The no-bytes and wrong-size cases on local, swappable and S3 go red; the positive and raw-route cases stay green.3 failed | 9 passed, the wrong-size case on each composition. The first attempt used a replacement that was a substring of the anchor. The tool refused it and ran nothing, then B was rerun with a distinct replacement.1 failed | 11 passed, the raw404case.storeTheBytesremoved from the booted-wall POSITIVE commit (storage-upload-door-ownership.wall.test.ts), the realStorageServicePluginover the local adapter answers409(expected 409 to be 200). The refusal is live through the composed runtime, not only at the unit seam.Existing tests that committed without bytes
Seven existing suites drove the commit door over a presigned upload whose bytes were never stored, which is the state now refused. They pinned other properties: ownership, organisation reach, the success envelope, the scoped write, the outage answer. Each now stores the declared bytes before the commit, and no assertion changed:
packages/services/service-storage/src/storage-routes.test.tspackages/services/service-storage/src/upload-door-ownership.test.tspackages/services/service-storage/src/success-envelope.conformance.test.tspackages/services/service-storage/src/tenant-audit-update-delete-half-repairs.test.tspackages/services/service-storage/src/storage-routes.metadata-outage.test.tspackages/plugins/organizations/src/storage-upload-door-ownership.wall.test.tspackages/plugins/organizations/src/storage-upload-organization-change.wall.test.tsThese are outside the claim's declared file surface, which named new test files only. The chunked sibling's landed change (
e36ee5351) updated the same files for the same reason. The dogfood suites that commit (nine files) already PUT bytes matching their declared size and are unchanged.Tests and gates
All at head
2ab6f5bf5(this branch withorigin/mainb12e16a56merged in), after building the dependency closure (pnpm turbo run build --filter='@objectstack/organizations^...', which covers@objectstack/service-storage's):pnpm --filter @objectstack/service-storage test:Test Files 62 passed (62),Tests 1063 passed (1063), exit 0.pnpm --filter @objectstack/service-storage typecheck: exit 0. The test layer compiles undertsconfig.test.jsonwith 0 errors and 0 pinned signatures.pnpm --filter @objectstack/organizations exec vitest run:Test Files 14 passed (14),Tests 166 passed (166). Thenpnpm --filter @objectstack/organizations typecheck: exit 0.node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack, no paths): 71 commands, 69 exited 0. Two exited 3 with PREREQUISITE NOT MET and are NOT MEASURED locally:pnpm check:dual-build-cjs-loadsreads every package'sdist, and this worktree built the closure only;pnpm check:i18nruns the built CLI, whose closure was not built. Both run in CI'sLint & Repo Gates.--ranreconciliation with exit codes recorded:71 derived, 69 run, 2 NOT-MEASURED, 0 UNRUN.test:repo,pnpm lintand the rest of the farm.origin/mainmoved tob61353ffaafter the merge. Those three commits touch metadata-protocol, plugin-approvals and docs, none of them this diff's packages, so they are not merged here.Acceptance notes
CompleteUploadRequestSchema.eTagis described as "S3 ETag verification", and the door stores it without comparing it (A2 above). Comparing it needs the stored ETag onStorageFileInfo, a contract change outside this card.pending. What later reclaims a never-committed pending row and its bytes is not measured here.no (narrowing)(a door that answered200now answers409), the shape the chunked siblinge36ee5351declared. The changeset and this body carry it.Generated by Claude Code