Add onSuccess, onFailure, and onCancel handlers#175
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds outcome-specific callbacks (onSuccess, onFailure, onCancel) alongside the existing onComplete, enforces mutual-exclusivity in enqueue options, splits RunResult schemas, resolves and executes per-outcome handlers (onSuccess inline when possible), and updates context/payload handling and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (enqueue)
participant Scheduler as Scheduler
participant Worker as Worker
participant DB as Database
participant Complete as completeHandler
Client->>Scheduler: enqueue(work with onComplete OR byOutcome handlers)
Scheduler->>DB: persist work item (payloadId if large context)
Scheduler->>Worker: schedule work execution
Worker->>DB: run function, produce RunResult
alt onSuccess inline allowed (hasOnSuccess = true)
Worker->>Complete: invoke completeHandler inline with runResult
Complete->>DB: load/remove payload if needed, run onSuccess mutation
else schedule completion
Worker->>Scheduler: schedule completion job
Scheduler->>Complete: completion job -> load work, resolve handler
Complete->>DB: run onFailure/onCancel or byOutcome handler in separate txn
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
📝 Coding Plan
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 |
commit: |
reeceyang
force-pushed
the
reece/onSuccess
branch
from
7a60353 to
0e11dc4
Compare
reeceyang
force-pushed
the
reece/onSuccess
branch
5 times, most recently
from
ba787de to
0e15e01
Compare
reeceyang
changed the title
reeceyang
force-pushed
the
reece/onSuccess
branch
2 times, most recently
from
b89443a to
5754bc6
Compare
reeceyang
force-pushed
the
reece/onSuccess
branch
from
5754bc6 to
31aac46
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/component/lib.ts (1)
95-103:⚠️ Potential issue | 🟠 MajorStore the shared outcome context once.
src/client/index.tsnow copies oneoptions.contextvalue into every activeonSuccess/onFailure/onCancelhandler, and this code sizes/persists each copy independently. That means attaching all three hooks can turn a single ~350 KB context into a >1 MB enqueue here, even though only one outcome handler will ever run. Please keep that shared context in one field/payload entry instead of duplicating it per handler.Also applies to: 125-137, 291-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/lib.ts` around lines 95 - 103, The current logic in getContext/workArgs handling computes and persists each handler's copy of the same options.context separately, inflating contextSize and storage; change the code that builds the context payload so the shared options.context is stored once and referenced by all outcome handlers (i.e., collapse duplicated per-handler context entries into a single shared context field), compute getConvexSize only once for that shared entry (use the same check against MAX_DOC_SIZE), and update the packing logic used around getContext, the onSuccess/onFailure/onCancel payload construction, and the similar blocks at the other locations (lines around 125-137 and 291-295) so they all reference a single context payload instead of duplicating it per handler while still enforcing the size limit using MAX_DOC_SIZE and including workArgs.fnName in the error message.
🧹 Nitpick comments (2)
src/component/schema.ts (1)
71-74: Schema extension looks good.The new optional fields for
onSuccess,onFailure, andonCancelare well-structured and correctly typed. The backwards compatibility comment is helpful.Minor typo: "compatability" → "compatibility" on line 71.
📝 Typo fix
- // onComplete should be undefined when any of onSuccess, onFailure, or onCancel are defined. This is enforced at the client level, but not in the schema for backwards compatability. + // onComplete should be undefined when any of onSuccess, onFailure, or onCancel are defined. This is enforced at the client level, but not in the schema for backwards compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/schema.ts` around lines 71 - 74, Fix the minor typo in the comment above the schema fields: change "compatability" to "compatibility" in the comment that precedes the onSuccess/onFailure/onCancel definitions (the comment referencing onComplete and backwards compatibility) so the wording is correct.src/component/worker.ts (1)
110-120: Silent error suppression may hide issues.The empty
catchblock on lines 117-119 silently swallows any errors from therunMutationcall. While falling back to the scheduler is reasonable, consider logging a debug/warn message to aid troubleshooting when the inline path fails.📝 Add logging for fallback path
if (args.hasOnSuccess) { try { // Attempt to run the onSuccess handler now await ctx.runMutation(internal.complete.complete, { jobs: [{ workId, runResult, attempt }], }); return; - } catch { + } catch (e) { + console.debug("Inline onSuccess failed, falling back to scheduler", e); // Fall through and schedule complete instead } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/worker.ts` around lines 110 - 120, The empty catch on the inline onSuccess path (the block around args.hasOnSuccess and the ctx.runMutation(internal.complete.complete, { jobs: [{ workId, runResult, attempt }] })) should not silently swallow errors; update the catch to log the caught error and context before falling back to scheduling (e.g., log a debug/warn with the error and identifiers workId, attempt, maybe runResult summary) using the existing logger (e.g., ctx.logger or processLogger) so failures of ctx.runMutation are visible while preserving the fallback behavior.
🤖 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/client/index.ts`:
- Around line 445-466: The defineOnCancel declaration uses the wrong types:
update its return type from RegisteredMutation<"internal", OnFailureArgs, null>
to RegisteredMutation<"internal", OnCancelArgs, null>, and change the handler's
result parameter type from FailureResult to the cancel-specific type (RunResult
& { kind: "canceled" }) so it matches the args produced by
vOnCancelArgs(context); ensure you only adjust the generic signature and the
handler args in defineOnCancel (preserving GenericDataModel, V, context,
vOnCancelArgs, and internalMutationGeneric usage) so callers and onCancel
handlers type-check correctly.
---
Outside diff comments:
In `@src/component/lib.ts`:
- Around line 95-103: The current logic in getContext/workArgs handling computes
and persists each handler's copy of the same options.context separately,
inflating contextSize and storage; change the code that builds the context
payload so the shared options.context is stored once and referenced by all
outcome handlers (i.e., collapse duplicated per-handler context entries into a
single shared context field), compute getConvexSize only once for that shared
entry (use the same check against MAX_DOC_SIZE), and update the packing logic
used around getContext, the onSuccess/onFailure/onCancel payload construction,
and the similar blocks at the other locations (lines around 125-137 and 291-295)
so they all reference a single context payload instead of duplicating it per
handler while still enforcing the size limit using MAX_DOC_SIZE and including
workArgs.fnName in the error message.
---
Nitpick comments:
In `@src/component/schema.ts`:
- Around line 71-74: Fix the minor typo in the comment above the schema fields:
change "compatability" to "compatibility" in the comment that precedes the
onSuccess/onFailure/onCancel definitions (the comment referencing onComplete and
backwards compatibility) so the wording is correct.
In `@src/component/worker.ts`:
- Around line 110-120: The empty catch on the inline onSuccess path (the block
around args.hasOnSuccess and the ctx.runMutation(internal.complete.complete, {
jobs: [{ workId, runResult, attempt }] })) should not silently swallow errors;
update the catch to log the caught error and context before falling back to
scheduling (e.g., log a debug/warn with the error and identifiers workId,
attempt, maybe runResult summary) using the existing logger (e.g., ctx.logger or
processLogger) so failures of ctx.runMutation are visible while preserving the
fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d4a1e5d-4696-4618-beb7-4adf53babfac
⛔ Files ignored due to path filters (1)
src/component/_generated/component.tsis excluded by!**/_generated/**
📒 Files selected for processing (10)
src/client/index.tssrc/component/complete.test.tssrc/component/complete.tssrc/component/lib.tssrc/component/loop.test.tssrc/component/loop.tssrc/component/recovery.test.tssrc/component/schema.tssrc/component/shared.tssrc/component/worker.ts
reeceyang
force-pushed
the
reece/onSuccess
branch
from
31aac46 to
f7e495d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/index.ts (1)
445-461:⚠️ Potential issue | 🟠 Major
defineOnCancelstill exposes the failure result type.
vOnCancelArgs()andOnCancelArgsboth describe a canceled result, but the handler signature here still saysFailureResult. That makes cancel handlers type-check against the wrong payload shape.🐛 Proposed fix
handler: ( ctx: GenericMutationCtx<DataModel>, args: { workId: WorkId; context: Infer<V>; - result: FailureResult; + result: RunResult & { kind: "canceled" }; }, ) => Promise<void>;#!/bin/bash # Verify that defineOnCancel's handler type matches vOnCancelArgs/OnCancelArgs. rg -n -A12 -B4 'defineOnCancel<|vOnCancelArgs|type OnCancelArgs|result:\s*FailureResult' src/client/index.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/index.ts` around lines 445 - 461, The handler signature for defineOnCancel incorrectly types the payload as FailureResult; update it to use the cancel-args type so it matches vOnCancelArgs/OnCancelArgs. Concretely, in defineOnCancel (the handler arg shaped { workId, context, result }), replace FailureResult with the cancel args result type (e.g. OnCancelArgs['result'] or the project's CanceledResult alias) so the handler's result matches vOnCancelArgs/OnCancelArgs and type-checks correctly.
🤖 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/lib.ts`:
- Around line 95-99: getContext() flattens per-handler contexts into a single
array and deleteContext() removes inline copies so completeHandler() ends up
only seeing payload.context (breaking handler-specific large-context paths); fix
by changing how context is stored in the payload: instead of a plain array use a
map keyed by handler name (or a dedicated shared field that preserves a single
handler's context), update the code paths that compute contextSize (uses
getContext and context.map/getConvexSize), the code that calls deleteContext,
and the completion code in completeHandler to read the selected handler's entry
from payload.contextByHandler (or the dedicated field) so the chosen handler
receives its original context value.
---
Duplicate comments:
In `@src/client/index.ts`:
- Around line 445-461: The handler signature for defineOnCancel incorrectly
types the payload as FailureResult; update it to use the cancel-args type so it
matches vOnCancelArgs/OnCancelArgs. Concretely, in defineOnCancel (the handler
arg shaped { workId, context, result }), replace FailureResult with the cancel
args result type (e.g. OnCancelArgs['result'] or the project's CanceledResult
alias) so the handler's result matches vOnCancelArgs/OnCancelArgs and
type-checks correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3b65e9a-20bb-436b-b429-04ed015cc777
⛔ Files ignored due to path filters (1)
src/component/_generated/component.tsis excluded by!**/_generated/**
📒 Files selected for processing (10)
src/client/index.tssrc/component/complete.test.tssrc/component/complete.tssrc/component/lib.tssrc/component/loop.test.tssrc/component/loop.tssrc/component/recovery.test.tssrc/component/schema.tssrc/component/shared.tssrc/component/worker.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/component/loop.test.ts
- src/component/recovery.test.ts
- src/component/complete.test.ts
- src/component/worker.ts
- src/component/loop.ts
reeceyang
force-pushed
the
reece/onSuccess
branch
from
f7e495d to
38d16f9
Compare
reeceyang
commented
Mar 10, 2026
Member
ianmacartney
left a comment
ianmacartney
left a comment
There was a problem hiding this comment.
it'd be nice to have some examples that highlight patterns, such as passing the same onComplete handler to all of onSuccess / onError / onCancel - the types should allow it, in case they want to centralize handling of statuses, but want transactional callbacks on success
reeceyang
force-pushed
the
reece/onSuccess
branch
4 times, most recently
from
8a162ea to
82f152d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/component/lib.ts (1)
91-104:⚠️ Potential issue | 🟠 MajorValidate the combined payload document size before insert.
When both
fnArgsandcontextcross the inline threshold, this branch stores both in onepayloaddoc. They’re checked individually, but the combined size is never bounded, so medium-large args plus medium-large context will fail later insidectx.db.insert("payload", ...)with a generic document-size error.💡 Suggested fix
if (fnArgsSize >= INLINE_METADATA_THRESHOLD) { // Args are large, store separately const payloadDoc: WithoutSystemFields<Doc<"payload">> = { args: workArgs.fnArgs, }; - workItem.payloadSize = fnArgsSize + PAYLOAD_DOC_OVERHEAD; delete workItem.fnArgs; if (contextSize >= INLINE_METADATA_THRESHOLD) { // Context is also too big to inline payloadDoc.context = context; - workItem.payloadSize += contextSize; if (workItem.onComplete) { delete workItem.onComplete.context; } } + const payloadDocSize = getConvexSize(payloadDoc); + if (payloadDocSize > MAX_DOC_SIZE) { + throw new Error( + `Function arguments and callback context for function ${workArgs.fnName} too large: ${payloadDocSize} bytes (max: ${MAX_DOC_SIZE} bytes)`, + ); + } + workItem.payloadSize = payloadDocSize; workItem.payloadId = await ctx.db.insert("payload", payloadDoc); } else if (fnArgsSize + contextSize >= INLINE_METADATA_THRESHOLD) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/component/lib.ts` around lines 91 - 104, The code currently adds both fnArgs and context into payloadDoc without checking their combined serialized size before calling ctx.db.insert; update the logic around payloadDoc, workItem.payloadSize and PAYLOAD_DOC_OVERHEAD to compute the total serialized/encoded size (fnArgs + context + PAYLOAD_DOC_OVERHEAD) and compare it against a safe DB max document size (or introduce a constant like MAX_PAYLOAD_DOC_SIZE) before inserting; if the combined size would exceed that limit, avoid inlining both (e.g., omit context from payloadDoc and persist it separately or create a separate payload record) and adjust workItem.payloadSize and any onComplete/context deletions accordingly, ensuring ctx.db.insert("payload", payloadDoc) is only called with a validated-size document.
🤖 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/client/index.ts`:
- Around line 674-693: In enqueueArgs, add a runtime validation that rejects
mixed callback styles: if opts.onComplete is provided and any of opts.onSuccess,
opts.onFailure, or opts.onCancel are also provided, throw a clear Error instead
of silently preferring onComplete; implement this check before creating function
handles (before the existing createFunctionHandle calls for
onComplete/onSuccess/onFailure/onCancel) so the code never continues to build a
mixed onComplete object, and include a message referencing the exclusive usage
(e.g., "Provide either onComplete OR onSuccess/onFailure/onCancel, not both") to
help callers correct the call site.
In `@src/component/worker.ts`:
- Around line 47-58: When args.hasOnSuccess is true the code calls
completeHandler(...) but swallows or converts any exception into a success path;
instead, ensure exceptions from completeHandler are not downgraded: let errors
thrown by completeHandler bubble up (or rethrow them) so the work is not
finalized as success and normal failure/retry/onFailure logic runs. Locate the
block that calls completeHandler when args.hasOnSuccess and remove or change the
catch that converts errors into a success fallback; if you must catch, rethrow
the caught error immediately so onFailure and retry semantics are preserved.
---
Outside diff comments:
In `@src/component/lib.ts`:
- Around line 91-104: The code currently adds both fnArgs and context into
payloadDoc without checking their combined serialized size before calling
ctx.db.insert; update the logic around payloadDoc, workItem.payloadSize and
PAYLOAD_DOC_OVERHEAD to compute the total serialized/encoded size (fnArgs +
context + PAYLOAD_DOC_OVERHEAD) and compare it against a safe DB max document
size (or introduce a constant like MAX_PAYLOAD_DOC_SIZE) before inserting; if
the combined size would exceed that limit, avoid inlining both (e.g., omit
context from payloadDoc and persist it separately or create a separate payload
record) and adjust workItem.payloadSize and any onComplete/context deletions
accordingly, ensuring ctx.db.insert("payload", payloadDoc) is only called with a
validated-size document.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33620a62-617e-45ee-90a7-d9b467f815b9
⛔ Files ignored due to path filters (1)
src/component/_generated/component.tsis excluded by!**/_generated/**
📒 Files selected for processing (12)
README.mdexample/convex/example.tssrc/client/index.tssrc/component/complete.test.tssrc/component/complete.tssrc/component/lib.test.tssrc/component/lib.tssrc/component/loop.test.tssrc/component/loop.tssrc/component/recovery.test.tssrc/component/shared.tssrc/component/worker.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/component/loop.ts
- src/component/recovery.test.ts
- src/component/loop.test.ts
- src/component/complete.test.ts
onFailure and onCancel (new)
reeceyang
force-pushed
the
reece/onSuccess
branch
from
82f152d to
4a5e30a
Compare
ianmacartney
approved these changes
Mar 13, 2026
Member
ianmacartney
left a comment
ianmacartney
left a comment
There was a problem hiding this comment.
Overall LGTM! Excited to see what impact this has when we change Workflow to only use onFailure / onCancel for the workflow handler, and to use onSuccess for step completion.
Let's put this in an alpha for now.
If the latter doesn't improve performance much, we might keep using onComplete for steps... wouldn't want a step OCC'ing just b/c it was fighting to start the workpool (until the debouncing works well)
Comment on lines
+692
to
+698
| ? await createFunctionHandle(opts.onSuccess) | ||
| : undefined, | ||
| onFailureHandle: opts.onFailure | ||
| ? await createFunctionHandle(opts.onFailure) | ||
| : undefined, | ||
| onCancelHandle: opts.onCancel | ||
| ? await createFunctionHandle(opts.onCancel) |
Member
There was a problem hiding this comment.
if these are all the same, we should try to reduce these calls - each function handle creation from an action is a request to conductor.
Ideally we'd cache this at the isolate level - maybe we do that already.
| payloadDoc.context = context; | ||
| workItem.payloadSize += contextSize; | ||
| delete workItem.onComplete!.context; | ||
| if (workItem.onComplete) { |
Member
There was a problem hiding this comment.
if this isn't set then there's likely a bug - assert might be more idiomatic, but this is fine too
| jobs: [{ workId, runResult, attempt }], | ||
| }); | ||
| return; | ||
| } catch { |
Member
There was a problem hiding this comment.
can we catch OCC's or other retry-able things explicitly here somehow? If it failed b/c it read/wrote too much data, scheduling it will just have it happen again. Maybe not that common / not the worst thing.
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
2 participants
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.
Add onSuccess, onFailure, and onCancel handlers. The onSuccess handler runs in the same mutation or action as the work, while onFailure and onCancel are scheduled to run after (like onComplete). The new handlers cannot be used with onComplete. Users must choose between using onSuccess/onFailure/onCancel and using onComplete.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Summary by CodeRabbit
New Features
Tests
Documentation