feat: add a retry predicate to the Node and Java SDKs#498
Conversation
The JS callback now resolves a JsTaskOutcome instead of rejecting, so a failure can carry its retry verdict — a napi rejection is only a string.
Predicate lives on Task, not RetryPolicy: RetryPolicy is wire-encoded into the scheduler's per-task config, which a lambda cannot cross.
📝 WalkthroughWalkthroughNode and Java tasks now support per-error retry predicates. Retryability is carried through SDK worker execution, structured task outcomes, JNI/N-API bridges, dispatchers, and queue backends, with tests and documentation covering accepted, rejected, and predicate-failure cases. ChangesRetry Predicate Support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TaskHandler
participant Worker
participant Dispatcher
participant QueueBackend
TaskHandler->>Worker: Throw error
Worker->>Worker: Evaluate retryOn
Worker->>Dispatcher: Return error and retryable
Dispatcher->>QueueBackend: Submit retry-aware failure
QueueBackend->>QueueBackend: Retry or dead-letter
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdks/node/src/worker.ts (1)
182-212: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
retryOnclassifies more than "errors the handler throws."The
TaskOptions.retryOndoc says the predicate applies to errors the handler throws, but this try block also wrapsmw.before/mw.afterhooks andserializer.serialize(result). An error from any of those is routed through the sameisRetryable(task, error)call as a genuine handler failure, so a predicate scoped to handler-specific error types could inadvertently dead-letter a middleware or serialization failure that has nothing to do with task-level retry semantics.Either narrow the predicate's scope to the handler invocation specifically, or update the
retryOndoc insdks/node/src/types.tsto state it covers any error surfaced during this try block (middleware hooks and result serialization included), not just handler-thrown errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/src/worker.ts` around lines 182 - 212, Update the retryOn documentation in TaskOptions within types.ts to state that its predicate receives any error surfaced during task execution, including middleware before/after hooks and result serialization, rather than only errors thrown by the handler. Keep the existing isRetryable behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@sdks/node/src/worker.ts`:
- Around line 182-212: Update the retryOn documentation in TaskOptions within
types.ts to state that its predicate receives any error surfaced during task
execution, including middleware before/after hooks and result serialization,
rather than only errors thrown by the handler. Keep the existing isRetryable
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29eda922-5a5b-4920-83d1-a37ad77bc567
📒 Files selected for processing (21)
crates/taskito-java/src/dispatcher.rscrates/taskito-java/src/worker.rscrates/taskito-node/src/convert/job.rscrates/taskito-node/src/convert/mod.rscrates/taskito-node/src/dispatcher.rsdocs/content/docs/java/api-reference/task.mdxdocs/content/docs/node/api-reference/task.mdxdocs/content/docs/shared/guides/reliability/retries.mdxsdks/java/src/main/java/org/byteveda/taskito/internal/JniWorkerControl.javasdks/java/src/main/java/org/byteveda/taskito/internal/NativeWorker.javasdks/java/src/main/java/org/byteveda/taskito/spi/WorkerControl.javasdks/java/src/main/java/org/byteveda/taskito/task/Task.javasdks/java/src/main/java/org/byteveda/taskito/worker/RegisteredTask.javasdks/java/src/main/java/org/byteveda/taskito/worker/Worker.javasdks/java/src/main/java/org/byteveda/taskito/worker/WorkerDispatchBridge.javasdks/java/src/test/java/org/byteveda/taskito/worker/RetryPredicateTest.javasdks/java/test-support/src/main/java/org/byteveda/taskito/test/InMemoryQueueBackend.javasdks/node/src/native.tssdks/node/src/types.tssdks/node/src/worker.tssdks/node/test/worker/retryOn.test.ts
The predicate also sees middleware, decode and serialization failures — a whitelist dead-letters those too.
|
Valid — the try block also wraps the Fixed by widening the docs rather than narrowing the scope, in b9a2442. Narrowing would make a serialization failure permanently retryable, which is worse — a bad result blob never becomes serializable on the next attempt, so letting the predicate rule on it is the useful behavior. Updated |
2 participants
Closes #479.
Python can already decide per-error whether a failure is worth retrying
(
retry_on/dont_retry_on). Node and Java could not — both dispatchershardcoded
should_retry: true, so every failure burned the whole retry budgetand a permanently-broken payload took the full backoff curve before reaching the
DLQ.
API
One predicate per task, evaluated against the live error rather than a type name:
Returning
falsedead-letters the job at once, whatever budget is left. Unsetretries everything, and so does a predicate that itself throws — a broken
classifier must not silently turn transient failures into dead letters. The
predicate is synchronous (the job cannot settle until it answers) and covers
both of Python's whitelist and blacklist forms.
No core change:
scheduler/result_handler.rsalready routesshould_retry: falsestraight to the DLQ.Scope
The predicate rules on every error raised while running the task, not only the
handler's —
before/aftermiddleware hooks and result serialization can failtoo (on the Java side also payload decoding and the middleware-disable read), so
a whitelist predicate dead-letters those as well. That is deliberate: a result
blob that will not serialize never becomes serializable on a later attempt, so
letting the predicate rule on it beats retrying it forever. Errors raised before
the task starts (Node's payload decode, an unregistered task) and timeouts sit
outside the predicate and always consume a retry, matching Python.
Notes on placement and plumbing
Java: the predicate lives on
Task, notRetryPolicy.RetryPolicyiswire-encoded into the per-task config the native scheduler reads
(
Worker.encodeRetryPolicy), and a Java lambda cannot cross that boundary. Itrides on
RegisteredTasktoWorkerDispatchBridgeinstead, and is deliberatelykept out of
Worker.Builder.hasPolicy()so a predicate-only task doesn't pushan empty config to the native side.
failJobgains aretryableflag throughthe JNI entry point and
TaskOutcome::Failure.Node needed a protocol change. The JS callback used to smuggle the canonical
error JSON out through a rejection (
new Error(json)withname = ""soString(err)was the bare JSON); napi hands Rust only that string, with no roomfor a verdict. The callback now resolves a
JsTaskOutcome(
{ result?, error?, retryable? }), which also removes that hack. A rejectionstill exists but now means only "the shell threw before it could shape an
outcome" — an unregistered task, an undecodable payload — and stays retryable,
exactly as before.
Verification
cargo check --workspace(default /postgres/redis),cargo test --workspace(also--features workflows),cargo clippy --workspace --all-targets— clean../gradlew test— 378 tests, 0 failures, including the newRetryPredicateTest(rejected exception dead-letters after one attempt andemits no RETRY; accepted exception still spends its budget).
tsc,biome check, and vitest including the newretryOn.test.ts(rejected error, accepted error, throwing predicate). The 9 dashboard test
files fail locally only because
viteisn't installed indashboard/.pnpm --dir docs build— the Node and Java retry pages now renderretryOnand the "no exception-type filter" note is gone.