Add onSuccess, onFailure, and onCancel handlers#175
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces outcome-specific completion callbacks for enqueued jobs. Users can now specify separate Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Worker
participant CompleteHandler
participant Scheduler
Client->>Worker: enqueueAction with onSuccess/onFailure/onCancel
activate Worker
Worker->>Worker: Execute job to completion
alt Job succeeds and hasOnSuccess=true
Worker->>CompleteHandler: completeHandler(ctx, work, {kind: "success"})
activate CompleteHandler
CompleteHandler->>CompleteHandler: getOnCompleteHandle() → onSuccessHandle
CompleteHandler->>Worker: runMutation with onSuccessHandle
deactivate CompleteHandler
Worker->>Client: success result (inline in same transaction)
else Job fails or onFailure is set
Worker->>Scheduler: schedule internal.complete.complete
activate Scheduler
Scheduler->>CompleteHandler: invoked after delay
activate CompleteHandler
CompleteHandler->>CompleteHandler: getOnCompleteHandle() → onFailureHandle/onCancelHandle
CompleteHandler->>Worker: runMutation with selected handle
deactivate CompleteHandler
deactivate Scheduler
Worker->>Client: failure/cancel result (separate transaction)
end
deactivate Worker
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
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
6 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
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
reeceyang
force-pushed
the
reece/onSuccess
branch
from
82f152d to
4a5e30a
Compare
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)
| ? await createFunctionHandle(opts.onSuccess) | ||
| : undefined, | ||
| onFailureHandle: opts.onFailure | ||
| ? await createFunctionHandle(opts.onFailure) | ||
| : undefined, | ||
| onCancelHandle: opts.onCancel | ||
| ? await createFunctionHandle(opts.onCancel) |
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) { |
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 { |
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.
onFailure and onCancel (new)
reeceyang
force-pushed
the
reece/onSuccess
branch
from
4a5e30a to
361b9a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/client/index.ts (1)
511-518:⚠️ Potential issue | 🟡 MinorUse the shipped
vOn*Argspattern in these examples.
Workpoolin this file only exposesdefineOnComplete, so theworkpool.defineOnSuccess/defineOnFailure/defineOnCancelsnippets will not compile as written. Please rewrite these examples to useinternalMutation({ args: vOn*Args(...) }), like the manual examples elsewhere in the PR.Also applies to: 537-543, 558-564
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/index.ts` around lines 511 - 518, The examples use non-existent methods (workpool.defineOnSuccess/defineOnFailure/defineOnCancel) and must be rewritten to use the shipped vOn*Args pattern and internalMutation; replace each snippet that calls workpool.defineOnSuccess/defineOnFailure/defineOnCancel with internalMutation({ args: vOnSuccessArgs(...)/vOnFailureArgs(...)/vOnCancelArgs(...), handler: async (ctx, { workId, context, result }) => { ... } }) so they compile against the Workpool API which only exposes defineOnComplete; update the three example blocks (the success, failure, and cancel examples) to use internalMutation and the corresponding vOn*Args validators (vOnSuccessArgs, vOnFailureArgs, vOnCancelArgs) and keep the same handler parameter names and 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 `@README.md`:
- Around line 149-152: Clarify in the README that onSuccess is a best-effort,
post-completion callback and is not part of the job's transactional
success/failure semantics: update the text around the
onSuccess/onComplete/onFailure/onCancel descriptions (mentioning onSuccess,
onComplete, onFailure, onCancel explicitly) to state that thrown errors in an
onSuccess handler do not mark the job as failed or affect retry behavior, and
that onSuccess is decoupled from the job transaction (unlike atomic
transactional steps) so users must not rely on it for state that must be tightly
coupled to the work outcome; mirror this clarification in the second location
referenced (lines ~358-362).
In `@src/component/complete.ts`:
- Around line 146-148: The inline-run condition is too broad: update the
runHandlerInline calculation (the variable declared where
job.runOnCompleteInline and work.onComplete are checked) to only allow
work.onComplete?.kind === "byOutcome" when the job outcome is success (e.g.,
outcome === "success"), so that only success handlers run inline; keep
job.runOnCompleteInline behavior as-is for explicit inline requests, and leave
failure/canceled handlers scheduled as before.
---
Duplicate comments:
In `@src/client/index.ts`:
- Around line 511-518: The examples use non-existent methods
(workpool.defineOnSuccess/defineOnFailure/defineOnCancel) and must be rewritten
to use the shipped vOn*Args pattern and internalMutation; replace each snippet
that calls workpool.defineOnSuccess/defineOnFailure/defineOnCancel with
internalMutation({ args:
vOnSuccessArgs(...)/vOnFailureArgs(...)/vOnCancelArgs(...), handler: async (ctx,
{ workId, context, result }) => { ... } }) so they compile against the Workpool
API which only exposes defineOnComplete; update the three example blocks (the
success, failure, and cancel examples) to use internalMutation and the
corresponding vOn*Args validators (vOnSuccessArgs, vOnFailureArgs,
vOnCancelArgs) and keep the same handler parameter names and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72a16e2d-36de-4694-91ef-86b50e9e28e7
⛔ Files ignored due to path filters (1)
src/component/_generated/component.tsis excluded by!**/_generated/**
📒 Files selected for processing (10)
README.mdexample/convex/example.tssrc/client/index.tssrc/component/complete.test.tssrc/component/complete.tssrc/component/lib.test.tssrc/component/lib.tssrc/component/loop.tssrc/component/shared.tssrc/component/worker.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/component/worker.ts
- src/component/shared.ts
- src/component/complete.test.ts
- example/convex/example.ts
- src/component/lib.test.ts
| If you need to define a separate mutation to run in the same transaction as the | ||
| job, you can use the `onSuccess` option instead of `onComplete`. You can then | ||
| define mutations for `onFailure` and `onCancel`. Note that unlike `onSuccess`, | ||
| the `onFailure` and `onCancel` handlers will run in a separate transaction. |
There was a problem hiding this comment.
Clarify that onSuccess is best-effort, not a transactional workflow step.
These sections currently read as if onSuccess participates in the job's success/failure semantics. In this implementation, a thrown onSuccess handler does not flip the work to failed, so users should not rely on it for state that must be coupled to the work outcome or retry behavior.
Based on learnings: if onSuccess throws, work is still finalized as successful; the callback is decoupled from the work's own success/failure and retry semantics.
Also applies to: 358-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 149 - 152, Clarify in the README that onSuccess is a
best-effort, post-completion callback and is not part of the job's transactional
success/failure semantics: update the text around the
onSuccess/onComplete/onFailure/onCancel descriptions (mentioning onSuccess,
onComplete, onFailure, onCancel explicitly) to state that thrown errors in an
onSuccess handler do not mark the job as failed or affect retry behavior, and
that onSuccess is decoupled from the job transaction (unlike atomic
transactional steps) so users must not rely on it for state that must be tightly
coupled to the work outcome; mirror this clarification in the second location
referenced (lines ~358-362).
| const runHandlerInline = | ||
| job.runOnCompleteInline || work.onComplete?.kind === "byOutcome"; | ||
| if (runHandlerInline) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'runOnCompleteInline|hasOnSuccess|runActionWrapper|runMutationWrapper' src/component/complete.ts src/component/worker.tsRepository: get-convex/workpool
Length of output: 4614
🏁 Script executed:
#!/bin/bash
# Check failure/cancel paths in worker.ts and scheduler calls
rg -n -B5 -A5 'runResult.*kind.*failure|runResult.*kind.*cancel|ctx.scheduler' src/component/worker.ts | head -100Repository: get-convex/workpool
Length of output: 2042
🏁 Script executed:
#!/bin/bash
# Verify the complete.ts handler signature and job.runResult usage
sed -n '26,50p' src/component/complete.ts
sed -n '110,160p' src/component/complete.tsRepository: get-convex/workpool
Length of output: 2698
Narrow the inline path to success outcomes only.
Line 147 allows every kind: "byOutcome" handler to run inline. Since job.runOnCompleteInline is only set for success outcomes in worker.ts, the work.onComplete?.kind === "byOutcome" clause forces failure and canceled handlers inline too, pulling them into the complete mutation instead of leaving them scheduled. This changes their runtime and transaction boundaries from the new API contract.
Suggested fix
- const runHandlerInline =
- job.runOnCompleteInline || work.onComplete?.kind === "byOutcome";
+ const runHandlerInline =
+ job.runOnCompleteInline ||
+ (work.onComplete?.kind === "byOutcome" &&
+ job.runResult.kind === "success");📝 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.
| const runHandlerInline = | |
| job.runOnCompleteInline || work.onComplete?.kind === "byOutcome"; | |
| if (runHandlerInline) { | |
| const runHandlerInline = | |
| job.runOnCompleteInline || | |
| (work.onComplete?.kind === "byOutcome" && | |
| job.runResult.kind === "success"); | |
| if (runHandlerInline) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component/complete.ts` around lines 146 - 148, The inline-run condition
is too broad: update the runHandlerInline calculation (the variable declared
where job.runOnCompleteInline and work.onComplete are checked) to only allow
work.onComplete?.kind === "byOutcome" when the job outcome is success (e.g.,
outcome === "success"), so that only success handlers run inline; keep
job.runOnCompleteInline behavior as-is for explicit inline requests, and leave
failure/canceled handlers scheduled as before.
2 participants
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
onSuccess,onFailure,onCancel) as an alternative to the singleonCompleteoption, enabling different behavior based on job outcome.Documentation