Uh oh!
There was an error while loading. Please reload this page.
fix(copilot): clamp the legacy int4 size when materializing a chat upload - #6615
Conversation
…load `materialize_file(operation: 'save')` wrote the HEADed object size straight into `workspace_files.size`, which is still `integer NOT NULL`. Since the `size_bytes` widening (0289), a mothership chat attachment may be up to MAX_WORKSPACE_FILE_SIZE (5 GiB): `upload-session/service.ts` gives `mothership_attachment` that ceiling, and `finalizers.ts` already dual-writes the row as `size = 2147483647, size_bytes = <exact>`. Saving such an upload then re-read the true size from `headObject` and issued `SET size = 3221225472` against int4. Postgres raises 22003; the retry filter matches only 23505, so it rethrows, the transaction rolls back and the tool returns `success: false` with no way for the user to complete the save. No corruption — int4 overflow errors, it never truncates — but the file can never be saved. Every other `workspace_files` size writer already pairs `toLegacyWorkspaceFileSize(bytes)` with `sizeBytes: bytes` (metadata.ts x4, workspace-file-manager.ts:243/1706, finalizers.ts:367). This call site was simply missed when the widening landed; the fix converges it with the other six rather than inventing a third shape. Storage accounting keeps using the exact `verifiedSize`, so quota and usage are unaffected. The size source itself also had to widen. `head?.size ?? row.size` fell back to the clamped int4 column, and since this change now writes `sizeBytes` too, that fallback would overwrite an exact `size_bytes` with the clamp — the object is gone, so nothing could recover it, and the row would look internally consistent afterwards. The fallback is live whenever `hasCloudStorage()` is false, since the early return at the HEAD miss is cloud-only. Reading `row.sizeBytes ?? row.size` is the same coalescing shape the readers already use (workspace-file-manager.ts:227, finalizers.ts:399, metadata.ts:46), and the row comes from a full `select()` so the column is present. The clamp is derived once next to `verifiedSize` rather than inline in the update because the value is loop-invariant. Two sibling writers were examined and deliberately left alone. `copy-files.ts` reads `task.size` out of the int4 column itself, so it is arithmetically incapable of overflow, and its missing `sizeBytes` is unreachable behind the 100 MB fork download cap. `workspace-file-manager.ts:963` takes a caller-supplied size, but its insert branch needs an orphaned storage object with no `workspace_files` row, and converting loose external input from a DB error into a JS throw deserves its own review rather than a release patch; it is the next instance of this bug and should be filed as a follow-up. Both new tests were proven red against the unfixed code: the clamp test fails with "expected 3221225472 to be 2147483647", the fallback test with "expected undefined to be 3221225472".
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview The save path now clamps via Reviewed by Cursor Bugbot for commit 5ecc178. Configure here. |
Greptile SummaryThe PR fixes chat-upload materialization by clamping the legacy int4 size projection while preserving the exact byte count in
Confidence Score: 5/5The PR appears safe to merge with no actionable defects identified in the changed paths. The new size precedence preserves exact bytes when available, the legacy projection is safely clamped, and persistence plus accounting remain atomic and consistently use the exact size.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/copilot/tools/handlers/materialize-file.ts | Aligns materialization with the established workspace-file size dual-write contract without changing exact-byte billing. |
| apps/sim/lib/copilot/tools/handlers/materialize-file.test.ts | Adds focused regression tests for int4 clamping, exact-size persistence, and storage accounting. |
Reviews (1): Last reviewed commit: "fix(copilot): clamp the legacy int4 size..." | Re-trigger Greptile
Summary
materialize_file(operation: 'save')wrote the HEADed object size straight intoworkspace_files.size, which is stillinteger NOT NULL. A mothership chat attachment may be up toMAX_WORKSPACE_FILE_SIZE(5 GiB), so saving one issuedSET size = <bytes > 2^31-1>against int4. Postgres raises 22003; the retry filter matches only 23505, so it rethrows, the transaction rolls back, and the tool returnssuccess: falsewith no way for the user to complete the save. No corruption — int4 overflows loudly, it never truncates — but the file could never be saved.toLegacyWorkspaceFileSize()and dual-write the exact count tosizeBytes, which is what every otherworkspace_filessize writer already does (server/metadata.tsx4,workspace-file-manager.ts:243/:1706,finalizers.ts:367). This call site was simply missed when the widening landed — the change converges it with the other six rather than inventing a third shape.head?.size ?? row.sizefell back to the clamped int4 column; now that the write also setssizeBytes, that fallback would have overwritten an exactsize_byteswith the clamp — and since the object is gone in that branch, nothing could recover it.row.sizeBytes ?? row.sizeis the same coalescing precedence the readers already use (workspace-file-manager.ts:227,finalizers.ts:399,metadata.ts:46). This branch is live wheneverhasCloudStorage()is false, since the early return at the HEAD miss is cloud-only.verifiedSize, so quota checks and usage increments are unaffected — only the legacy int4 projection is clamped.Severity — not release-blocking
size_bytesis introduced by migration 0289 in this same release, and the legacysizecolumn isinteger NOT NULL, so an oversized row was never physically creatable. No backfill is needed and no existing row is affected — the original premise that legacy rows need one is inverted.The oversized path is not reachable today given the upload limits elsewhere in the stack, so this is correctness hardening landing before the column ships, not a fix for an active failure. It is a small change (13 source lines) that removes an unrecoverable failure mode at the point the ceiling would otherwise be hit.
Follow-ups (deliberately out of scope)
workspace-file-manager.ts:963(trackChatUpload, insert branch) writes a caller-suppliedsizewith no clamp and nosizeBytes. It is the next instance of this bug and sits upstream of the row this PR repairs. Converting loose external input from a DB error into a JS throw (the helper validates and throws on non-safe-integer/negative input) is a behavior change that deserves its own review rather than riding along here.workspaceFileSizeColumns(bytes)helper returning{ size, sizeBytes }, plus a lint guard so the dual-write cannot be half-applied. The two columns must always agree and are currently kept in sync by convention across independent call sites. That belongs with the contract phase —sizecannot safely be dropped while unconverged writers exist.ee/workspace-forking/lib/copy/copy-files.ts:266/:312copiesmeta.size(already int4-valid, so no overflow risk) but dropssizeBytes, so a forked large file lands withsize_bytes = NULLand bills the clamped value. Read-side fix iscoalesce(sizeBytes, size)at the select.Type of Change
Testing
bunx vitest run lib/copilot/tools/handlers/materialize-file.test.ts— 26 passed. Both new tests were proven red against the unfixed source: the clamp test fails withexpected 3221225472 to be 2147483647, the fallback test withexpected undefined to be 3221225472.bun run type-checkclean; biome clean on both changed files.Checklist