store args/context separately from work when large#170
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR introduces size-aware batching and payload storage mechanisms across the work queue system. Large function arguments and completion context exceeding 8KB are stored in separate payload documents with references in work items. Enqueue and completion operations batch items by encoded size. Workers lazily load large arguments from payloads. Cursor-based pagination is replaced with iterative loops using size guards. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Enqueue as Enqueue Handler
participant DB as Database
participant Worker as Worker
participant Complete as Complete Handler
rect rgba(100, 150, 200, 0.5)
Note over Client,DB: Large Args Storage Path
Client->>Enqueue: enqueue({ args: large_payload })
Enqueue->>Enqueue: Check size with getConvexSize
alt args + context exceed INLINE_METADATA_THRESHOLD
Enqueue->>DB: INSERT payload { args, context }
DB-->>Enqueue: payloadId
Enqueue->>DB: INSERT work { payloadId, fnHandle }
else fits inline
Enqueue->>DB: INSERT work { fnArgs, context_inline }
end
DB-->>Enqueue: workId
Enqueue-->>Client: workId
end
rect rgba(100, 200, 150, 0.5)
Note over Worker,Complete: Lazy Loading Execution Path
Worker->>DB: fetch work by workId
alt work.payloadId exists
Worker->>DB: getWorkArgs(payloadId)
DB-->>Worker: { args, context }
else inline args
Worker->>Worker: use work.fnArgs directly
end
Worker->>Worker: execute function with args
Worker->>Complete: schedule completion with result
end
rect rgba(200, 150, 100, 0.5)
Note over Complete,DB: Batched Completion Path
Complete->>Complete: collect jobs, batch by size
loop For each batch
Complete->>DB: update work, record completion
alt large context in payload
Complete->>DB: delete payload
end
Complete->>DB: delete work record
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
commit: |
ianmacartney
changed the title
ianmacartney
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/component/worker.ts (1)
108-108: Minor: Comment incorrectly states "mutation" but this is a query.📝 Proposed fix
-// Helper mutation for actions to fetch work args +// Helper query for actions to fetch work args export const getWorkArgs = internalQuery({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/worker.ts` at line 108, The inline comment "Helper mutation for actions to fetch work args" is incorrect; update the comment above the helper to indicate it's a query (e.g., "Helper query for actions to fetch work args") so the comment matches the implementation; locate the comment text "Helper mutation for actions to fetch work args" in src/component/worker.ts and replace "mutation" with "query" to keep semantics and documentation consistent.src/component/danger.ts (1)
110-119: Debug log may printundefinedfor fnArgs when stored in payload.When args are offloaded to the payload table,
entry.fnArgswill beundefined. This could make debug logs less useful.📝 Proposed improvement
console.debug( - `cleared ${entry.fnName}: ${entry.fnArgs} (${Object.entries({ + `cleared ${entry.fnName}${entry.payloadId ? ' (args in payload)' : `: ${JSON.stringify(entry.fnArgs)}`} (${Object.entries({ pendingStart, pendingCompletion, pendingCancelation, }) .filter(([_, v]) => v !== null) .map(([name]) => name) .join(", ")})`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/danger.ts` around lines 110 - 119, The debug message currently interpolates entry.fnArgs which can be undefined when args are offloaded to payload; update the console.debug call in this block to compute a safe readableArgs (e.g., use entry.fnArgs if present, otherwise extract and JSON.stringify a fallback from entry.payload, and guard stringification to avoid throwing) and use readableArgs in the template; reference the existing symbols entry.fnArgs, entry.payload, console.debug and the surrounding pendingStart/pendingCompletion/pendingCancelation mapping when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/component/danger.ts`:
- Around line 36-43: The calls to ctx.db.delete in the block handling work are
using two-argument forms (e.g., ctx.db.delete("payload", work.payloadId) and
ctx.db.delete("work", work._id)) but the API accepts a single document ID;
change these to pass only the ID values (use work.payloadId and work._id
respectively), keeping the existing ctx.db.delete(entry._id) call as-is so
deletion uses the correct single-argument API.
---
Nitpick comments:
In `@src/component/danger.ts`:
- Around line 110-119: The debug message currently interpolates entry.fnArgs
which can be undefined when args are offloaded to payload; update the
console.debug call in this block to compute a safe readableArgs (e.g., use
entry.fnArgs if present, otherwise extract and JSON.stringify a fallback from
entry.payload, and guard stringification to avoid throwing) and use readableArgs
in the template; reference the existing symbols entry.fnArgs, entry.payload,
console.debug and the surrounding
pendingStart/pendingCompletion/pendingCancelation mapping when making the
change.
In `@src/component/worker.ts`:
- Line 108: The inline comment "Helper mutation for actions to fetch work args"
is incorrect; update the comment above the helper to indicate it's a query
(e.g., "Helper query for actions to fetch work args") so the comment matches the
implementation; locate the comment text "Helper mutation for actions to fetch
work args" in src/component/worker.ts and replace "mutation" with "query" to
keep semantics and documentation consistent.
| await ctx.db.delete(entry._id); | ||
| if (work) { | ||
| // Clean up any large data stored separately | ||
| if (work.payloadId) { | ||
| await ctx.db.delete("payload", work.payloadId); | ||
| } | ||
| }), | ||
| ); | ||
| if (!entries.isDone) { | ||
| await ctx.db.delete("work", work._id); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and check the exact content at lines 36-43
head -50 src/component/danger.ts | tail -20Repository: get-convex/workpool
Length of output: 707
🏁 Script executed:
# Find all ctx.db.delete usages to identify the correct API pattern
rg -n "ctx\.db\.delete\(" --type ts -B2 -A2 | head -60Repository: get-convex/workpool
Length of output: 3317
🏁 Script executed:
# Look for type definitions or implementations of ctx.db.delete
fd -e ts -e tsx | xargs grep -l "\.db\.delete" | head -10Repository: get-convex/workpool
Length of output: 322
🏁 Script executed:
# Search for Convex db type definitions or documentation
find . -name "*.ts" -o -name "*.d.ts" | xargs grep -l "delete.*payloadId\|delete.*work\._id" 2>/dev/null | head -5Repository: get-convex/workpool
Length of output: 116
🏁 Script executed:
# Check complete.ts for ctx.db.delete usage patterns
rg -n "ctx\.db\.delete\(" src/component/complete.ts -B2 -A2Repository: get-convex/workpool
Length of output: 407
🏁 Script executed:
# Look at the actual type definition or import of db from ctx
rg -n "ctx\.db" src/component/danger.ts | head -20Repository: get-convex/workpool
Length of output: 743
🏁 Script executed:
# Check if there are any Convex library files that define the db.delete signature
find . -name "*.d.ts" -o -name "convex*" -type f | xargs grep -l "delete" 2>/dev/null | head -10Repository: get-convex/workpool
Length of output: 86
🏁 Script executed:
# Get more context on the complete.ts deletions to confirm the pattern
rg -n "ctx\.db\.delete\(" src/component/complete.ts -B3 -A1Repository: get-convex/workpool
Length of output: 422
🏁 Script executed:
# Also check danger.ts for all delete calls to see the inconsistency
cat -n src/component/danger.ts | sed -n '35,125p'Repository: get-convex/workpool
Length of output: 3564
Incorrect ctx.db.delete API usage at lines 40 and 42 — will cause runtime errors.
The ctx.db.delete method takes a single argument (the document ID). Lines 40 and 42 incorrectly pass two arguments with a table name prefix, which contradicts the correct single-argument usage used throughout the rest of the codebase.
🐛 Proposed fix
await ctx.db.delete(entry._id);
if (work) {
// Clean up any large data stored separately
if (work.payloadId) {
- await ctx.db.delete("payload", work.payloadId);
+ await ctx.db.delete(work.payloadId);
}
- await ctx.db.delete("work", work._id);
+ await ctx.db.delete(work._id);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await ctx.db.delete(entry._id); | |
| if (work) { | |
| // Clean up any large data stored separately | |
| if (work.payloadId) { | |
| await ctx.db.delete("payload", work.payloadId); | |
| } | |
| }), | |
| ); | |
| if (!entries.isDone) { | |
| await ctx.db.delete("work", work._id); | |
| } | |
| await ctx.db.delete(entry._id); | |
| if (work) { | |
| // Clean up any large data stored separately | |
| if (work.payloadId) { | |
| await ctx.db.delete(work.payloadId); | |
| } | |
| await ctx.db.delete(work._id); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/danger.ts` around lines 36 - 43, The calls to ctx.db.delete in
the block handling work are using two-argument forms (e.g.,
ctx.db.delete("payload", work.payloadId) and ctx.db.delete("work", work._id))
but the API accepts a single document ID; change these to pass only the ID
values (use work.payloadId and work._id respectively), keeping the existing
ctx.db.delete(entry._id) call as-is so deletion uses the correct single-argument
API.
There was a problem hiding this comment.
convex now allows for both syntax
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Convex ctx.db.delete two argument syntax table name
💡 Result:
In Convex (since convex v1.31.0), ctx.db.delete supports the two-argument form where the table name comes first:
await ctx.db.delete("messages", messageId);- New (recommended):
ctx.db.delete(tableName, id)(table name is currently optional but encouraged) [1][2] - Old (still supported for now):
ctx.db.delete(id)(table inferred from the ID encoding) [1][2]
Sources: [1] [2]
@ianmacartney Thank you for the correction! You're absolutely right. Since Convex v1.31.0, ctx.db.delete supports the two-argument form ctx.db.delete(tableName, id), which is actually the newer recommended syntax, while the single-argument form remains supported for backward compatibility.
I apologize for the incorrect review comment. The code in danger.ts is using the correct, modern API. I've noted this for future reviews.
✏️ Learnings added
Learnt from: ianmacartney
Repo: get-convex/workpool PR: 170
File: src/component/danger.ts:36-43
Timestamp: 2026-02-19T08:37:48.834Z
Learning: In Convex (since v1.31.0), `ctx.db.delete` supports both the two-argument form `ctx.db.delete(tableName, id)` (newer, recommended) and the single-argument form `ctx.db.delete(id)` (older, still supported). Both are valid API usage.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: sethconvex
Repo: get-convex/workpool PR: 167
File: src/component/batch.ts:465-485
Timestamp: 2026-02-14T00:53:51.808Z
Learning: In the workpool BatchWorkpool implementation (src/component/batch.ts), the `cancel` mutation intentionally does not guard against cancelling claimed (actively running) tasks. This is safe by design: the executor's `complete`/`fail` calls handle missing tasks gracefully (early return on null), and adding a status check before deletion would introduce a TOCTOU race condition between the check and the delete. The delete-first approach with idempotent cleanup is the correct pattern.
Learnt from: sethconvex
Repo: get-convex/workpool PR: 167
File: src/component/pool.test.ts:0-0
Timestamp: 2026-02-14T00:53:37.586Z
Learning: In the workpool repository using convex-test, afterEach cleanup with `t.finishAllScheduledFunctions(vi.runAllTimers)` is not required in test files like `src/component/pool.test.ts` because convex-test creates isolated environments per test, making such cleanup purely cosmetic.
949c850 to
90aabe7
Compare
ianmacartney
changed the base branch from
ian/load-test
to
graphite-base/170
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example/convex/test/work.ts (1)
18-35:⚠️ Potential issue | 🟡 Minor
generateDatadouble-counts the remainder, producing strings longer thanlen.Lines 22-24 already append a partial chunk for
len % 10. Lines 30-33 then append the same remainder again, so the result islen + (len % 10)characters instead oflen.🐛 Proposed fix: remove the duplicate remainder block
let result = chunks.join(""); - // Add remaining bytes for exact length - const remaining = len % 10; - if (remaining > 0) { - result += chunk.substring(0, remaining); - } return result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/test/work.ts` around lines 18 - 35, The generateData function appends the partial remainder twice, causing output longer than len; remove the second remainder append (the block that computes remaining and appends chunk.substring(0, remaining)) and rely only on the earlier conditional that pushes the partial chunk into the chunks array, ensuring the final result = chunks.join("") has exactly len characters; check references to chunk, numChunks, remaining, and chunks to validate the duplicate logic is removed.
🧹 Nitpick comments (3)
src/component/danger.ts (1)
110-119: Debug log may serialize largefnArgsfor deleted work items.Line 111 interpolates
entry.fnArgsinto the log string. With the new payload externalization,fnArgson the work document could beundefined(displaying as "undefined"), which is fine. But when args are inlined (small args), the entire args object is stringified. For a cleanup operation that can process many rows, this may generate a lot of log output.Consider using
entry.fnNameonly, or truncating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/danger.ts` around lines 110 - 119, The debug log in the console.debug call currently interpolates entry.fnArgs which may stringify large payloads; update the log in the clearing/cleanup routine to avoid serializing full args — reference the existing console.debug and the entry object (entry.fnName, entry.fnArgs) and replace the interpolation of entry.fnArgs with either only entry.fnName or a truncated/summary form (e.g. JSON.stringify(entry.fnArgs) sliced to a short max length or a flag like "<omitted>") so the cleanup loop doesn't emit massive logs; keep the existing pendingStart/pendingCompletion/pendingCancelation name extraction intact.src/component/lib.ts (1)
86-111: Shallow spread silently sharesonCompletereference — safe here but fragile.
workItem = { ...workArgs, attempts: 0 }shallow-copies, soworkItem.onCompleteis the same object asworkArgs.onComplete. Thedelete workItem.onComplete!.contexton lines 102 and 109 mutates the caller's object too. This is safe today becauseworkArgsisn't read after this point, but it's a subtle coupling that could bite during future refactoring.♻️ Optional: deep-copy onComplete before mutating
const workItem: WithoutSystemFields<Doc<"work">> = { ...workArgs, + onComplete: workArgs.onComplete ? { ...workArgs.onComplete } : undefined, attempts: 0, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/lib.ts` around lines 86 - 111, The shallow spread creating workItem copies a reference to workArgs.onComplete so subsequent delete workItem.onComplete!.context mutations also modify the caller; to fix, ensure workItem gets its own copy of onComplete (e.g., when constructing workItem or immediately after, set workItem.onComplete = workArgs.onComplete ? { ...workArgs.onComplete } : undefined) so the later delete workItem.onComplete!.context calls do not mutate workArgs; apply this change before any code paths that call delete workItem.onComplete!.context (references: workItem, workArgs, onComplete, delete workItem.onComplete!.context).src/component/worker.ts (1)
34-36: Standardizectx.db.getto two-argument form and strengthen assertion narrowingThree issues in this block:
Deprecated
ctx.db.getform: Line 34 uses the single-argumentctx.db.get(args.payloadId), while line 115 in the same file correctly uses the modern two-argument formctx.db.get("payload", args.payloadId). Since Convex v1.31.0, the two-argument form with explicit table name is the recommended API. Update line 34 to match.Implicit assertion narrowing:
assert(payload?.args)at line 35 asserts the result of an optional-chain expression, which may not reliably narrowpayloaditself to excludenull. Line 36 then accessespayload.args. The pattern ingetWorkArgs(lines 116–117) is clearer: separate assertions forpayloadandpayload.argsprovide explicit narrowing and clearer failure attribution.Incorrect comment: Line 108 labels
getWorkArgsas "Helper mutation for actions" but it is aninternalQuery, not a mutation.🛠️ Proposed fix
- const payload = await ctx.db.get(args.payloadId); - assert(payload?.args); - fnArgs = payload.args; + const payload = await ctx.db.get("payload", args.payloadId); + assert(payload); + assert(payload.args); + fnArgs = payload.args;-// Helper mutation for actions to fetch work args +// Helper query for actions to fetch work args🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/worker.ts` around lines 34 - 36, Update the single-argument db call to the two-argument form and strengthen assertion narrowing: replace ctx.db.get(args.payloadId) with ctx.db.get("payload", args.payloadId) inside the worker code, then assert the payload itself (assert(payload)) followed by asserting its property (assert(payload.args)) before using payload.args to ensure proper type narrowing; also fix the misleading comment above getWorkArgs to label it as an internalQuery (not a mutation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/component/worker.ts`:
- Around line 108-109: The comment above getWorkArgs incorrectly calls it a
"mutation"; update the comment to say "Helper query for actions to fetch work
args" (or similar) to match that getWorkArgs is exported via internalQuery;
locate the comment that begins "Helper mutation for actions to fetch work args"
and change "mutation" to "query" to keep wording consistent with the exported
internalQuery.
---
Outside diff comments:
In `@example/convex/test/work.ts`:
- Around line 18-35: The generateData function appends the partial remainder
twice, causing output longer than len; remove the second remainder append (the
block that computes remaining and appends chunk.substring(0, remaining)) and
rely only on the earlier conditional that pushes the partial chunk into the
chunks array, ensuring the final result = chunks.join("") has exactly len
characters; check references to chunk, numChunks, remaining, and chunks to
validate the duplicate logic is removed.
---
Duplicate comments:
In `@src/component/danger.ts`:
- Around line 36-43: The clearPending function mixes ctx.db.delete forms
(single-arg and two-arg), which reduces readability; update the two-argument
calls in clearPending (ctx.db.delete("payload", work.payloadId) and
ctx.db.delete("work", work._id)) to the single-argument form used in
clearOldWork by passing the full document id value (e.g., payloadId and work._id
directly) so all ctx.db.delete calls in this file use the single-argument style
for consistency.
---
Nitpick comments:
In `@src/component/danger.ts`:
- Around line 110-119: The debug log in the console.debug call currently
interpolates entry.fnArgs which may stringify large payloads; update the log in
the clearing/cleanup routine to avoid serializing full args — reference the
existing console.debug and the entry object (entry.fnName, entry.fnArgs) and
replace the interpolation of entry.fnArgs with either only entry.fnName or a
truncated/summary form (e.g. JSON.stringify(entry.fnArgs) sliced to a short max
length or a flag like "<omitted>") so the cleanup loop doesn't emit massive
logs; keep the existing pendingStart/pendingCompletion/pendingCancelation name
extraction intact.
In `@src/component/lib.ts`:
- Around line 86-111: The shallow spread creating workItem copies a reference to
workArgs.onComplete so subsequent delete workItem.onComplete!.context mutations
also modify the caller; to fix, ensure workItem gets its own copy of onComplete
(e.g., when constructing workItem or immediately after, set workItem.onComplete
= workArgs.onComplete ? { ...workArgs.onComplete } : undefined) so the later
delete workItem.onComplete!.context calls do not mutate workArgs; apply this
change before any code paths that call delete workItem.onComplete!.context
(references: workItem, workArgs, onComplete, delete
workItem.onComplete!.context).
In `@src/component/worker.ts`:
- Around line 34-36: Update the single-argument db call to the two-argument form
and strengthen assertion narrowing: replace ctx.db.get(args.payloadId) with
ctx.db.get("payload", args.payloadId) inside the worker code, then assert the
payload itself (assert(payload)) followed by asserting its property
(assert(payload.args)) before using payload.args to ensure proper type
narrowing; also fix the misleading comment above getWorkArgs to label it as an
internalQuery (not a mutation).
| // Helper mutation for actions to fetch work args | ||
| export const getWorkArgs = internalQuery({ |
There was a problem hiding this comment.
Incorrect comment: "Helper mutation" should be "Helper query"
The comment on line 108 describes getWorkArgs as a "mutation," but it is exported as internalQuery.
✏️ Proposed fix
-// Helper mutation for actions to fetch work args
+// Helper query for actions to fetch work args📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Helper mutation for actions to fetch work args | |
| export const getWorkArgs = internalQuery({ | |
| // Helper query for actions to fetch work args | |
| export const getWorkArgs = internalQuery({ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/worker.ts` around lines 108 - 109, The comment above
getWorkArgs incorrectly calls it a "mutation"; update the comment to say "Helper
query for actions to fetch work args" (or similar) to match that getWorkArgs is
exported via internalQuery; locate the comment that begins "Helper mutation for
actions to fetch work args" and change "mutation" to "query" to keep wording
consistent with the exported internalQuery.
Discarding the forwards-compatible changes
1 participant
payloadtable when they exceed 8KBpayloadIdfield to the work table to reference this separate storageenqueueBatchto handle large batches by splitting them into smaller piecesThese changes allow us to handle much larger function arguments and context data without hitting document size limits, while also making the system more efficient when processing large batches of work.
Summary by CodeRabbit
Refactor
Chores