improvement(tables): versioned CSV snapshot cache for table mounts + parallel multipart uploader#5108
Conversation
…parallel multipart uploader
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Invalidation & storage: migration adds Sandbox & API surface: Export path: table export worker and export row paging switch from buffering the whole file / Reviewed by Cursor Bugbot for commit bf6ab96. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@greptile review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR eliminates the web-process OOM on large table mounts by introducing a versioned CSV snapshot cache backed by object storage, a parallel multipart uploader, and a statement-level Postgres trigger to keep
Confidence Score: 5/5Safe to merge behind the feature flag; the existing inline-CSV path is byte-for-byte unchanged, and the new code is well-tested with focused Vitest coverage. The change ships behind a default-OFF feature flag so no production behaviour changes until the flag is enabled. The multipart uploader correctly handles abort, concurrent parts, and the single-shot fast path. The snapshot cache correctly enforces tenant isolation and size limits. The two known cleanup gaps in the re-key path (orphaned torn snapshot and incorrect prior-version deletion) are acknowledged and mitigated by a planned S3 lifecycle rule. The only new finding is that complete() lacks an aborted guard, which is a minor footgun not reachable by any current caller. snapshot-cache.ts and storage-service.ts warrant a second look once the flag is enabled in production, particularly around the re-key cleanup path and the missing aborted guard in complete(). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant FE as function-execute
participant SC as snapshot-cache
participant DB as Postgres
participant S3 as Object Storage
participant SB as E2B Sandbox
FE->>DB: isFeatureEnabled('table-snapshot-cache')
FE->>DB: getTableById(tableId)
alt "flag ON and rowCount >= 500"
FE->>SC: getOrCreateTableSnapshot(table)
SC->>DB: readRowsVersion(tableId)
SC->>S3: headObject vN.csv
alt cache HIT
S3-->>SC: size
else cache MISS
SC->>SC: materialize(table, key)
SC->>DB: selectExportRowPage paginated
SC->>S3: createMultipartUpload write parts complete
SC->>DB: readRowsVersion recheck
alt version unchanged
SC->>S3: deleteFile vN-1.csv best-effort
else version advanced
SC->>S3: headObject or materialize vN+1.csv
SC->>S3: deleteFile vN.csv best-effort
end
end
SC-->>FE: key size version
alt hasCloudStorage
FE->>S3: generatePresignedDownloadUrl key TTL 600s
S3-->>FE: presignedUrl
FE->>SB: mount url type path url
SB->>S3: curl presignedUrl write to disk
else local storage
FE->>S3: downloadFile key
S3-->>FE: buffer
FE->>SB: mount content buffer
end
else flag OFF or small table
FE->>DB: queryRows inline CSV
FE->>SB: mount content csv
end
SB-->>FE: execution result
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant FE as function-execute
participant SC as snapshot-cache
participant DB as Postgres
participant S3 as Object Storage
participant SB as E2B Sandbox
FE->>DB: isFeatureEnabled('table-snapshot-cache')
FE->>DB: getTableById(tableId)
alt "flag ON and rowCount >= 500"
FE->>SC: getOrCreateTableSnapshot(table)
SC->>DB: readRowsVersion(tableId)
SC->>S3: headObject vN.csv
alt cache HIT
S3-->>SC: size
else cache MISS
SC->>SC: materialize(table, key)
SC->>DB: selectExportRowPage paginated
SC->>S3: createMultipartUpload write parts complete
SC->>DB: readRowsVersion recheck
alt version unchanged
SC->>S3: deleteFile vN-1.csv best-effort
else version advanced
SC->>S3: headObject or materialize vN+1.csv
SC->>S3: deleteFile vN.csv best-effort
end
end
SC-->>FE: key size version
alt hasCloudStorage
FE->>S3: generatePresignedDownloadUrl key TTL 600s
S3-->>FE: presignedUrl
FE->>SB: mount url type path url
SB->>S3: curl presignedUrl write to disk
else local storage
FE->>S3: downloadFile key
S3-->>FE: buffer
FE->>SB: mount content buffer
end
else flag OFF or small table
FE->>DB: queryRows inline CSV
FE->>SB: mount content csv
end
SB-->>FE: execution result
Reviews (3): Last reviewed commit: "improvement(tables): mount snapshots by ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const newSize = newHead ? newHead.size : await materialize(table, newKey) | ||
| await deleteFile({ key, context: SNAPSHOT_STORAGE_CONTEXT }).catch(() => {}) | ||
| void deletePreviousVersion(table, after) | ||
| return { key: newKey, size: newSize, version: after } |
There was a problem hiding this comment.
Re-key rebuild lacks second version check
Medium Severity
When rows_version advances during the first materialize, the code re-reads the version once and may call materialize again for the new key, but it never re-checks rows_version after that second build. If the table mutates again while that rebuild runs, the function can still return the intermediate version’s object even though the database has moved on.
Reviewed by Cursor Bugbot for commit c340659. Configure here.
…fetches directly (raise cap to 500MB)
|
@greptile review |
…ct; key snapshot by column shape so schema edits invalidate it
|
Addressed bot findings in f2e6225:
Deferred (best-effort, reaped by the bucket lifecycle TTL being added):
|
|
Triage of the two latest Cursor findings — both acknowledged, deferring with rationale:
Everything green: Test and Build ✓, lint ✓, types ✓, api-validation ✓, migration backward-compatible ✓. Verified live: snapshot miss→materialize→S3, cache hit reuse, and in-sandbox presigned-URL fetch on the mothership template. |
… CSV matches the grid under fractional ordering
|
Update on the fractional-ordering finding: fixed in bf6ab96 rather than deferred. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bf6ab96. Configure here.
| if (head) { | ||
| logger.info(`[${requestId}] Snapshot hit`, { tableId: table.id, version, size: head.size }) | ||
| return { key, size: head.size, version } | ||
| } |
There was a problem hiding this comment.
Stale snapshot on cache hit
Medium Severity
getOrCreateTableSnapshot reads rows_version once, then on a storage headObject hit returns that snapshot immediately. If row writes bump the version after that read, an older v{N} object can still exist and be served even though the table is already at v{N+1}.
Reviewed by Cursor Bugbot for commit bf6ab96. Configure here.
There was a problem hiding this comment.
This is fine, there will always be a little desync between the live state of the table and whatever snapshot we pass to the e2b container. This is acceptable.
1 participant
Summary
rows_versiononuser_table_definitions, bumped by a statement-level Postgres trigger onuser_table_rows(INSERT/UPDATE/DELETE) — bypass-proof; reorders/edits invalidate the cache. Mirrors the 0224 statement-level trigger pattern (one bump per statement, no per-row contention).table-snapshots/{workspaceId}/{tableId}/v{rows_version}.csv:headObjecthit = no row read; miss = page rows → canonical export-format CSV → upload. Best-effort version recheck + previous-version cleanup. Capped at the 10MB mount ceiling (the table branch had no size guard before).createMultipartUpload) to the storage layer — bounded-concurrency parts, single-PutObject fast path for small files — and refactor the table-export worker onto it so it no longer buffers the whole file in heap.table-snapshot-cachefeature flag, default OFF. Flag-off keeps the existing inline-CSV path byte-for-byte.Type of Change
Testing
rows_versiontrigger verified bumping on update (applied locally, sincedb pushdoesn't emit trigger DDL).bun run check:migrations origin/staging✓ (expand-safe: additive defaulted column + triggers),check:api-validation:strict✓,lint:check✓,tsc --noEmit✓ (0 errors).Checklist
Follow-ups (not in this PR)
table-snapshots/prefix (e.g. 7d) to reap old versions — they're a pure regenerable cache.