|
| 1 | +--- |
| 2 | +name: memory-load-check |
| 3 | +description: Review PRs and diffs for unbounded memory loading, concurrency explosions, oversized payload materialization, and missing pagination or byte caps. Use when reviewing cleanup jobs, background jobs, data imports/exports, file parsing, API fan-out, workflow execution payloads, large arrays/files, or any change that reads many rows, files, responses, logs, or external API pages into process memory. |
| 4 | +--- |
| 5 | + |
| 6 | +# Memory Load Check |
| 7 | + |
| 8 | +Use this skill when a PR or diff could load unbounded data into a Node/Bun process, especially in cron routes, background tasks, API routes, workflow execution, file parsing, cleanup jobs, migrations, import/export flows, and external API integrations. |
| 9 | + |
| 10 | +## Review Goal |
| 11 | + |
| 12 | +Prove each changed path has explicit bounds for: |
| 13 | +- rows held in memory |
| 14 | +- bytes held in memory |
| 15 | +- concurrent promises, DB queries, HTTP calls, storage operations, and jobs |
| 16 | +- number of pages, batches, chunks, retries, and retained intermediate objects |
| 17 | + |
| 18 | +If any bound depends only on current production size or "probably small" data, treat it as a finding. |
| 19 | + |
| 20 | +## References |
| 21 | + |
| 22 | +Read these when doing a deeper pass: |
| 23 | +- Node.js streams/backpressure: https://nodejs.org/learn/modules/backpressuring-in-streams |
| 24 | +- Node.js stream usage: https://nodejs.org/en/learn/modules/how-to-use-streams |
| 25 | +- Keyset/cursor pagination over offset scans: https://blog.sequinstream.com/keyset-cursors-not-offsets-for-postgres-pagination/ |
| 26 | +- Postgres pagination tradeoffs: https://www.citusdata.com/blog/2016/03/30/five-ways-to-paginate/ |
| 27 | + |
| 28 | +## Sim Helpers To Prefer |
| 29 | + |
| 30 | +- `apps/sim/lib/cleanup/batch-delete.ts` |
| 31 | + - `chunkedBatchDelete`: bounded SELECT -> optional side effect -> DELETE loop. |
| 32 | + - `batchDeleteByWorkspaceAndTimestamp`: common workspace/timestamp cleanup wrapper. |
| 33 | + - `selectRowsByIdChunks`: chunks large ID sets and enforces an overall row cap. |
| 34 | + - `chunkArray`: use only after the input set itself is already bounded. |
| 35 | +- `apps/sim/lib/core/utils/stream-limits.ts` |
| 36 | + - `PayloadSizeLimitError` |
| 37 | + - `assertKnownSizeWithinLimit` |
| 38 | + - `assertContentLengthWithinLimit` |
| 39 | + - `readStreamToBufferWithLimit` |
| 40 | + - `readNodeStreamToBufferWithLimit` |
| 41 | + - `readResponseToBufferWithLimit` |
| 42 | + - `readResponseTextWithLimit` |
| 43 | +- Cleanup dispatcher pattern in `apps/sim/lib/billing/cleanup-dispatcher.ts` |
| 44 | + - page active workspaces with `WHERE id > afterId ORDER BY id LIMIT N` |
| 45 | + - dispatch concrete chunks (`workspaceIds`, retention, label) instead of one giant scope |
| 46 | + - prefer Trigger.dev queue/concurrency keys when available |
| 47 | + - execute inline fallback chunks sequentially, not with unbounded `Promise.all` |
| 48 | +- File parse route pattern in `apps/sim/app/api/files/parse/route.ts` |
| 49 | + - cap downloads and parsed output separately |
| 50 | + - preserve partial results when a later item exceeds the cap |
| 51 | + - never read untrusted response bodies without a byte cap |
| 52 | +- Large workflow value payloads |
| 53 | + - prefer durable references/manifests over inlining large arrays or files |
| 54 | + - materialize refs only behind an explicit byte budget |
| 55 | + |
| 56 | +## Review Workflow |
| 57 | + |
| 58 | +1. Identify every changed data source: |
| 59 | + - database queries |
| 60 | + - storage lists/downloads/uploads |
| 61 | + - external API pagination |
| 62 | + - file reads and HTTP responses |
| 63 | + - workflow logs, snapshots, payloads, arrays, and manifests |
| 64 | + - queues, cron routes, and background jobs |
| 65 | +2. For each source, write down the maximum cardinality and maximum bytes. If the code does not enforce one, it is unbounded. |
| 66 | +3. Trace whether data is processed incrementally or accumulated: |
| 67 | + - arrays from `select`, `findMany`, `Promise.all`, `map`, `filter`, `flatMap` |
| 68 | + - maps/sets keyed by all users, workspaces, executions, files, or rows |
| 69 | + - `Buffer.concat`, `response.arrayBuffer()`, `response.text()`, `JSON.stringify`, `JSON.parse` |
| 70 | + - queues of promises or job payloads built before dispatch |
| 71 | +4. Check concurrency separately from memory: |
| 72 | + - no `Promise.all(items.map(...))` unless `items` is already small and bounded |
| 73 | + - use chunks, sequential loops, queue concurrency, or a concurrency limiter |
| 74 | + - align concurrency with DB pool size, storage/API limits, and task queue semantics |
| 75 | +5. Verify SQL shape: |
| 76 | + - every bulk query has `LIMIT` |
| 77 | + - large pagination uses cursor/keyset style (`id > afterId`, timestamps plus unique ID), not deep `OFFSET` |
| 78 | + - `IN (...)` lists are chunked |
| 79 | + - side-effect rows selected before delete have per-batch and per-run caps |
| 80 | +6. Verify byte safety: |
| 81 | + - check `Content-Length` when available |
| 82 | + - stream with cumulative byte accounting |
| 83 | + - cap both input bytes and expanded output bytes |
| 84 | + - reject or reference oversized values before serializing large JSON responses |
| 85 | +7. Confirm failure behavior: |
| 86 | + - exceeding a cap should stop before loading more data |
| 87 | + - partial successful work should be preserved when the API contract expects it |
| 88 | + - retries should not duplicate huge in-memory state |
| 89 | + - cleanup jobs should make progress over future runs instead of widening one run |
| 90 | + |
| 91 | +## Red Flags |
| 92 | + |
| 93 | +- loads all active workspaces, users, executions, logs, files, messages, or subscriptions before filtering |
| 94 | +- builds a full `Map` or `Set` for a platform-wide scope |
| 95 | +- uses `Promise.all` over rows from an unbounded query |
| 96 | +- fetches all pages from an external API before processing |
| 97 | +- reads an entire file, HTTP response, or stream without a max byte budget |
| 98 | +- checks size only after `Buffer.concat`, `arrayBuffer`, `text`, `JSON.parse`, or parse expansion |
| 99 | +- chunks only after loading the complete dataset |
| 100 | +- paginates with unbounded/deep `OFFSET` on a mutable or large table |
| 101 | +- creates one queue job per row without batching or a queue-level concurrency key |
| 102 | +- accumulates per-row errors/results with no maximum |
| 103 | +- adds a cache, singleton, or module-level collection without eviction or size limits |
| 104 | + |
| 105 | +## Preferred Fixes |
| 106 | + |
| 107 | +- Move filters into SQL/API requests and select only needed columns. |
| 108 | +- Replace full-table loads with cursor/keyset pagination and a deterministic order. |
| 109 | +- Process one page/batch at a time; do not keep previous pages unless needed. |
| 110 | +- Add per-batch and per-run row caps so long backlogs drain across repeated jobs. |
| 111 | +- Split large ID lists with `selectRowsByIdChunks` or `chunkArray` after bounding the source. |
| 112 | +- Use `chunkedBatchDelete` for cleanup loops with row side effects. |
| 113 | +- Use stream-limit helpers for file/HTTP/body reads. |
| 114 | +- Store large workflow values as refs/manifests and materialize only within a caller budget. |
| 115 | +- Replace unbounded `Promise.all` with sequential chunk loops, queue concurrency, or a small limiter. |
| 116 | +- Include tests that prove caps stop work early and partial results or progress are preserved. |
| 117 | + |
| 118 | +## Findings Format |
| 119 | + |
| 120 | +Lead with concrete findings, ordered by risk: |
| 121 | + |
| 122 | +```markdown |
| 123 | +## Findings |
| 124 | + |
| 125 | +- **P1 Unbounded workspace load in cleanup dispatch** (`path/to/file.ts`) |
| 126 | + The new path calls `select().from(workspace)` without a limit, then builds maps for every row before dispatch. In production this scales with all active workspaces and can exhaust the app process. Page by `workspace.id` with a fixed limit and dispatch bounded chunks. |
| 127 | + |
| 128 | +## Good Signals |
| 129 | + |
| 130 | +- Uses `readResponseToBufferWithLimit` for external downloads. |
| 131 | +- Inline fallback processes chunks sequentially. |
| 132 | + |
| 133 | +## Residual Risk |
| 134 | + |
| 135 | +- The row cap is explicit, but no test currently proves the loop stops at the cap. |
| 136 | +``` |
| 137 | + |
| 138 | +Only say "good to go" when every changed source has explicit row, byte, and concurrency bounds or the boundedness is proven by a stable invariant. |
0 commit comments