perf(db): stop scanning every workspace file on workflow archive, cover the billing period aggregate - #6015
Merged
Merged
Conversation
…s backing files `cleanupWorkflowAliasBacking` runs on every workflow delete. It loaded every `context='workspace'` row for the workspace — active and soft-deleted, all columns — then discarded >99% of them in JS to find the handful of `.changelogs/<workflowId>.md` and `.plans/<workflowId>/**` rows it owns. In production this was the single worst query at 37.83% of total database runtime: p50 6s, p95 9s, max 13.1s, 641 calls/day, 34.1M rows read. It is index-served, so the cost is not a missing index — it is 59,061 rows and ~630MB of cold buffers read to archive a few files. A file's `folderPath` is derived solely from its `folderId`, so matching on folder membership is equivalent to the path comparison it replaces. The load and JS filter become one targeted UPDATE keyed on a handful of folder ids. Folders that are soft-deleted are still included when resolving which files a workflow owns: path resolution ignores `deletedAt`, so a live file parented to an archived folder previously matched and must continue to. Also: - Add `getWorkspaceShares`, replacing an id-list `IN` clause that grew with the file count (59,061 elements on the worst workspace) with one indexed lookup on `workspace_id`. Callers read the map by id, so a superset is equivalent. - Drop `all` from the client-reachable file scope enum. It drops the `deleted_at` predicate and so cannot use the partial index serving the other two. No client requests it; server callers reach that scope directly. - Set `fetch_types: false` on the app and realtime pools. postgres.js otherwise runs a blocking `pg_catalog.pg_type` roundtrip before each new connection's first query — 95,722 of them per day. It builds array parsers only; Drizzle already parses this schema's two `text[]` columns itself.
…aggregate Dynamic routes prefetch only down to the nearest loading boundary, and the server stops prefetching at the first one. With no loading.tsx anywhere in the workspace tree and a dynamic layout, Link prefetch was yielding almost nothing and every navigation waited on a full server round trip with no feedback. Add loading.tsx only where the fallback is provably what renders today, so perceived speed improves without changing what users see: - home, chat/[chatId]: reuse HomeFallback, already each page's own Suspense fallback. - integrations, skills: reuse the tab-header chrome, byte-identical to each page's own Suspense fallback. Deliberately not added to workspace root, settings, or w, whose pages are redirect-only or already self-fallbacking — a boundary there would paint a skeleton that does not match the destination. Billing: - Add usage_log_billing_period_cost_idx, trailing the remaining predicate columns and `cost` so the period aggregates resolve index-only. Confirmed against production: the aggregate currently runs as an Index Scan touching 478,559 buffers because `cost` is absent from the existing index. That index is superseded but left in place; dropping it is a separate migration so a planner regression costs nothing to revert. - Collapse the two aggregates behind /api/billing into one scan using SUM(...) FILTER. Besides halving the work on a nav-path query, it removes a latent inconsistency: as separate statements the two sums could observe different snapshots, making the copilot subset exceed the total. Caching was considered and rejected for the usage aggregate. Its callers include usage enforcement, threshold billing, and overage calculation, where a stale-low read permits overspend and a stale-high read double-charges.
…are mock cleanupWorkflowAliasBacking had no test coverage, and the rewrite that replaced its load-everything-then-filter body rests on a subtle equivalence: a file's folderPath is derived solely from its folderId, and path resolution ignores deletedAt. The second half is the easy part to get wrong — a live file parented to an archived folder still resolves to a backing path and must still be archived, so folders are filtered by deletedAt only when choosing which folders to archive, never when deciding which files the workflow owns. These tests pin that distinction: they fail if the archived-folder case is dropped from file ownership, and separately assert that archived folders stay out of the folder update and that unrelated workflows are never touched. Also point the workspace files route test at getWorkspaceShares. Its mock still named getSharesForResources, which the route no longer imports. The suite passed regardless because all three cases exercise the upload path, so the GET listing has no coverage — but the stale name would have handed the first GET test an undefined function.
…x order Adversarial review found real regressions in both. Route loading boundaries — all four removed: The premise in their TSDoc was wrong. Each page's existing `<Suspense>` exists so nuqs can prerender; `useSearchParams` only suspends during SSR, so on a client navigation those fallbacks never painted. Hoisting them to loading.tsx did not "show the same frame" — it made a previously invisible blank frame visible. For chat, Next keys the Suspense boundary by cache key, so a chatId change mounts a fresh suspended boundary and always commits its fallback. Switching chats would have gone from "previous chat stays on screen" to a blank surface for the whole RSC round trip, on the highest-frequency navigation in the product. Partial prefetch only warms the fallback, so no latency was saved to offset it. The integrations and skills fallbacks were correct for their own pages but also became the fallback for four detail routes that render different chrome, inserting a wrong intermediate frame. Fixing that needs per-child boundaries and new skeleton UI that cannot be verified without a browser, for pages whose only server work is `await params`. Not worth it. Every remaining loading.tsx in this app paints real chrome. A blank one was against the grain, and the measurable win was zero. usage_log index: Column order was wrong. The daily-refresh rollup filters entity type, id and period_start but NOT period_end, so putting period_end fourth ended the usable prefix at column three and left user_id and created_at as in-index filters rather than scan boundaries — turning a ~2.2k-entry bitmap scan into a ~266k-entry scan. Verified in production that period_start functionally determines period_end (13,612 groups, zero with more than one end), so the slot bought no selectivity. user_id and created_at now follow the shared prefix; period_end rides as payload. Also corrected the claim that this supersedes usage_log_billing_entity_period_idx. That index deduplicates to 17 MB across 1.42M entries because it has no high-cardinality key column, which is what keeps prefix-only bitmap scans cheap. It is retained deliberately, not pending a drop. Harden cleanupWorkflowAliasBacking: gate the UPDATE on the ownership filter list itself. `and()` and `or()` both drop undefined, so a clause that resolved to nothing would have left a WHERE of workspace + context + not-deleted and archived every file in the workspace. Tests now assert no UPDATE is issued when the workflow owns nothing, plus the filename and context predicates. Note fetch_types also disables array serializers, not just parsers; documented.
…testing Ran both settings against a real Postgres with the schema's actual text[] columns. Drizzle-typed selects and .returning() are byte-identical either way; only a raw db.execute projecting an array column differs, yielding the wire form. The previous note also claimed a raw JS-array bind fails because the serializers come from the same catalog fetch. It does fail — but under both settings, because Drizzle expands an array into a row constructor before postgres.js ever sees it. That is unrelated to this flag, so the claim is removed rather than left implying a constraint this change introduces.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview Workflow alias cleanup no longer calls Public shares adds File listing API drops client-reachable scope Billing introduces postgres.js sets Reviewed by Cursor Bugbot for commit 93678a5. Configure here. |
…ation 0271 was hand-written, so drizzle-kit's state never learned about the new index — the next `generate` would have re-emitted it as a fresh migration against an already-migrated database. Ran `drizzle-kit generate` to produce the snapshot, then restored the CONCURRENTLY form: drizzle emits a plain CREATE INDEX, which takes an ACCESS EXCLUSIVE lock and would block writes on a 4M-row table for the duration of the build. The generated column order matched the hand-written SQL exactly, which also confirms schema.ts and the migration agree. `generate` is now a no-op, and check:migrations still passes.
Contributor
Greptile SummaryPerformance-focused DB and billing query work with no UI changes.
Confidence Score: 5/5The PR appears safe to merge; no blocking failures remain from prior Greptile findings or this follow-up pass. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/copilot/vfs/workflow-alias-backing.ts | Archives alias-backing files via folder-id ownership filters with a non-empty guard and context/workspace constraints. |
| apps/sim/lib/billing/core/usage-log.ts | Adds combined period total and source-subset aggregate in one statement for consistent billing snapshots. |
| apps/sim/lib/public-shares/share-manager.ts | Adds workspace-scoped share map lookup used by files API and VFS listing. |
| packages/db/migrations/0271_usage_log_billing_period_cost_idx.sql | Concurrent covering index for billing period cost aggregates; keeps existing compact period index. |
| packages/db/db.ts | Disables postgres.js per-connection pg_type fetch on the shared pool. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Workflow archive] --> B[listWorkspaceFileFolders]
B --> C[ownedFileFolderIds + changelogFolderIds]
C --> D{ownershipFilters non-empty?}
D -->|no| E[Skip file UPDATE]
D -->|yes| F[UPDATE workspaceFiles by folderId/name]
C --> G[Archive live plans folders]
H[GET workspace files / VFS list] --> I[getWorkspaceShares by workspaceId]
J[getPersonalBillingSummary] --> K[getBillingPeriodUsageCostWithSourceSubset]
K --> L[Single SUM + FILTER scan]
Reviews (2): Last reviewed commit: "chore(db): generate the drizzle snapshot..." | Re-trigger Greptile
Collaborator
Author
Collaborator
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 93678a5. Configure here.
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.
Summary
cleanupWorkflowAliasBackingloaded everycontext='workspace'file in the workspace on each workflow archive, then filtered >99% of them out in JS. It was the single worst query in production — 37.8% of total DB runtime, p50 6s, p95 9s, 34.1M rows read/day. Replaced with one targeted UPDATE keyed on folder ids.getWorkspaceShares— the files list built anINclause from every file id (59,061 on the largest workspace). Now one indexed lookup onworkspace_id.allfrom the client-reachable file scope enum. It drops thedeleted_atpredicate so it can't use the partial index; no client ever sent it, and server callers reach that scope directly./api/billinginto oneSUM(...) FILTER (...). Also removes a latent bug: as separate statements the two sums could observe different snapshots, making the copilot subset exceed the total.usage_log_billing_period_cost_idxso the billing period aggregates resolve index-only.fetch_types: falseon the app and realtime pools — postgres.js otherwise runs a blockingpg_catalog.pg_typeroundtrip before each new connection's first query (95,722/day).Verification
Ran against a local Postgres 17 seeded to production shape (1.46M rows):
.plans/wf_10prefix collision, amothership-context row in a plans folder, nullfolder_id, and a different workspace.FILTERform returns identical totals to the two-query form on three real production entities, including one at $15,001.fetch_types— differential test with realtext[]columns: typed selects and.returning()byte-identical under both settings. Only a rawdb.executeprojecting an array differs, which is documented.Notes for review
usage_log_billing_entity_period_idxis deliberately kept, not superseded. It deduplicates to ~12.5 bytes/entry because it has no high-cardinality key column, which is what keeps prefix-only bitmap scans cheap. Dropping it would regress the daily-refresh plan.billing_period_startbut notbilling_period_end, souser_id/created_atmust follow the shared prefix to be usable as scan boundaries. Verified in production thatperiod_startfunctionally determinesperiod_end(13,612 groups, zero with more than one end), soperiod_endrides along as payload only.usage_logwill be partial at first —relallvisibleis ~81% and this insert-only table autovacuums on a ~34-day cadence, so the current billing period still heap-fetches untilautovacuum_vacuum_insert_scale_factoris lowered on it.0271+ itsschema.tsentry backs it out without touching anything else.loading.tsxfiles; they were removed after review showed they would blank the chat surface on every chat switch and leak the wrong skeleton into four detail routes.ANY(${jsArray}::text[])inside a drizzle template expands to a row constructor and throws —lib/execution/payloads/large-value-metadata.ts:399,440,475andlib/webhooks/polling/utils.ts:167, the last silently swallowed by a log-only catch. Same class as fix(cleanup): compare live-paused statuses with IN, not ANY over a row constructor #6004.Type of Change
Testing
7,946 unit tests pass. Verified against a local Postgres seeded to production shape as described above; billing and share equivalence checked against read-only production queries.
Checklist