feat: structured task errors across all SDKs#413
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a canonical JSON format for task failures, implements encoding and decoding across Python, Node, and Java SDKs, preserves structured details in raised errors, and updates dashboard error rendering with structured summaries and tracebacks. ChangesStructured task-error contract
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant Storage
participant SDKResult
participant Dashboard
Worker->>Storage: persist canonical JSON task error
Storage->>SDKResult: return stored failure payload
SDKResult->>SDKResult: decode and attach structured details
Storage->>Dashboard: provide stored error string
Dashboard->>Dashboard: parse and render summary and traceback
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@CodeRabbit full review |
✅ Action performedReview finished.
|
✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 58 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
crates/taskito-node/src/dispatcher.rs (1)
122-130: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUnnecessary clone of
err.reason.Since only one branch of the
if/elseexecutes,err.reasoncan be moved out directly instead of cloned.♻️ Proposed diff
let reason = if err.reason.is_empty() { err.to_string() } else { - err.reason.clone() + err.reason }; failure(job, reason, wall_time_ns, false)🤖 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 `@crates/taskito-node/src/dispatcher.rs` around lines 122 - 130, Update the reason selection in the dispatcher failure path to move the non-empty err.reason value instead of cloning it. Preserve the existing err.to_string() fallback when the reason is empty, and ensure the resulting owned value is still passed to failure.crates/taskito-core/BINDING_CONTRACT.md (1)
100-113: 🚀 Performance & Scalability | 🔵 TrivialConsider capping traceback size for storage.
The contract lets
tracebackbe an arbitrary array of strings serialized verbatim intojobs.error/job_errors.error/dead_letter.error. Deeply recursive or repeatedly-retried failures could blow up row size across all three tables with no documented cap or truncation guidance.🤖 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 `@crates/taskito-core/BINDING_CONTRACT.md` around lines 100 - 113, Update the canonical failure contract around JobResult::Failure.error to define a maximum serialized traceback size and specify deterministic truncation behavior when the limit is exceeded. Apply the cap before storing the error in jobs.error, job_errors.error, or dead_letter.error, while preserving the required errtype, message, and traceback keys.crates/taskito-python/src/py_worker.rs (1)
227-239: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent fallback on encoder failure hides real bugs.
If
taskito.task_errorsfails to import, orencode_from_partsraises/returns something un-extractable asString, this silently falls through to plain-text traceback formatting with no log output. That's fine for a genuinely missing module, but ifencode_from_partsthrows due to an actual encoder bug (e.g. non-serializable traceback frame), you'll silently lose the structured error for every task failure in that process with no signal to investigate.Consider logging at debug/warn level when the encode path fails after a successful import, to distinguish "module not installed" from "encoder is broken."
🔍 Proposed diff
if let Ok(errors_mod) = py.import("taskito.task_errors") { - if let Ok(encoded) = errors_mod.call_method1( + match errors_mod.call_method1( "encode_from_parts", (e.get_type(py), e.value(py), e.traceback(py)), ) { - if let Ok(json) = encoded.extract::<String>() { - return json; + Ok(encoded) => { + match encoded.extract::<String>() { + Ok(json) => return json, + Err(err) => log::warn!("[taskito] encode_from_parts returned non-string: {err}"), + } } + Err(err) => log::warn!("[taskito] encode_from_parts failed: {err}"), } }🤖 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 `@crates/taskito-python/src/py_worker.rs` around lines 227 - 239, Update the structured-error encoding flow around the task_errors import and encode_from_parts calls to emit a debug or warning log when the module imports successfully but encoding or String extraction fails, including the failure details. Preserve the silent plain-text fallback only when taskito.task_errors is genuinely unavailable, and keep returning the existing traceback fallback after logging encoder failures.dashboard/src/routes/workflows_.$id.tsx (1)
92-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRender
TaskErrorSummaryin the workflow error row.KvListalready accepts a ReactNode, so this can stay one-line while keeping the tooltip used on other structured-error views.🤖 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 `@dashboard/src/routes/workflows_`.$id.tsx around lines 92 - 97, Update the workflow error row in the data.error handling block to render the TaskErrorSummary component for structured errors instead of converting them with taskErrorSummary. Pass the resulting ReactNode as the KvList value so its existing tooltip behavior is preserved, while continuing to use data.error for unstructured errors.dashboard/src/components/ui/task-error.tsx (1)
41-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using the app's
Tooltipcomponent instead of nativetitlefor consistency.
taskErrorTooltipcan return multi-line traceback text; nativetitletooltips have inconsistent rendering/accessibility across browsers, whereas the dashboard already exports aTooltipprimitive (seedashboard/src/components/ui/index.ts) used elsewhere for hover content. Optional polish, not a blocker.🤖 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 `@dashboard/src/components/ui/task-error.tsx` around lines 41 - 50, Update TaskErrorSummary to use the dashboard’s Tooltip component for taskErrorTooltip(parsed) content instead of the native title attribute, preserving the existing summary text and fallback error behavior. Reuse the exported Tooltip primitive and ensure the tooltip wraps the rendered span so multi-line traceback content is displayed consistently.sdks/node/test/core/task-error.test.ts (1)
24-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest vector doesn't exercise the real V8 stack format.
Manually setting
error.stackbypasses V8's actual.stackshape (header line + frames), so this test can't catch the header-line duplication issue flagged intask-error.ts(Lines 22-27). Consider adding a case that lets a real thrownErrorpopulate.stacknaturally to pin down the expected frame-only (or header-inclusive) format.🤖 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/test/core/task-error.test.ts` around lines 24 - 30, Update the encodeTaskError contract test to exercise a naturally populated V8 stack by creating or throwing and catching the BoomError instead of manually assigning error.stack. Assert the expected serialization against the actual header-plus-frames stack format, ensuring encodeTaskError does not duplicate or incorrectly retain the stack header.
🤖 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.
Inline comments:
In `@crates/taskito-node/src/dispatcher.rs`:
- Around line 122-130: Add Node-side retry filtering in the dispatcher before
calling failure: evaluate the JS error against the configured retry policy, then
pass false for non-retryable exceptions and true otherwise. Extend the Node
error/API path as needed to propagate retry_on and dont_retry_on semantics from
the worker into the core result handler, while preserving the existing reason
construction and failure flow.
In `@sdks/java/src/main/java/org/byteveda/taskito/errors/TaskError.java`:
- Around line 27-30: Update TaskError.summary() to branch on message rather than
errtype, returning only errtype when the message is empty and preserving the
combined errtype-and-message format otherwise. Ensure the result matches the
shared summary contract and avoids a trailing colon or space.
In `@sdks/java/src/main/java/org/byteveda/taskito/errors/TaskErrors.java`:
- Around line 55-57: Update TaskErrors.decode() so missing or non-textual
errtype values default to "Error" instead of an empty string, while preserving
textual errtype values and the existing TaskError construction flow.
In `@sdks/java/src/test/java/org/byteveda/taskito/errors/TaskErrorsTest.java`:
- Around line 73-81: The test method summarizePrefersErrtypeMessageHeadline must
reflect the shared default errtype contract: update the missing-errtype
assertion in TaskErrors.summarize to expect the “Error” prefix, while preserving
the explicit errtype and raw-text fallback assertions.
In `@sdks/node/src/task-error.ts`:
- Around line 39-59: Update decodeTaskError to default missing or non-string
candidate.errtype to an empty string, matching the cross-SDK decoding contract.
Then update JobFailedError’s message construction in errors.ts to omit the
errtype separator when the decoded errtype is empty, while preserving the
existing “type: message” format for non-empty types.
- Around line 22-27: Update the errtype assignment in the Error handling branch
to prefer error.constructor.name, falling back to error.name and then "Error".
Keep the existing message and traceback handling unchanged.
In `@sdks/python/taskito/exceptions.py`:
- Around line 20-26: Update the JobErrorDetails docstring to accurately describe
legacy/plain-string errors: job_id and raw_error remain populated, while only
errtype and traceback default to None when the stored error is not structured
JSON. Keep the structured-error description unchanged.
---
Nitpick comments:
In `@crates/taskito-core/BINDING_CONTRACT.md`:
- Around line 100-113: Update the canonical failure contract around
JobResult::Failure.error to define a maximum serialized traceback size and
specify deterministic truncation behavior when the limit is exceeded. Apply the
cap before storing the error in jobs.error, job_errors.error, or
dead_letter.error, while preserving the required errtype, message, and traceback
keys.
In `@crates/taskito-node/src/dispatcher.rs`:
- Around line 122-130: Update the reason selection in the dispatcher failure
path to move the non-empty err.reason value instead of cloning it. Preserve the
existing err.to_string() fallback when the reason is empty, and ensure the
resulting owned value is still passed to failure.
In `@crates/taskito-python/src/py_worker.rs`:
- Around line 227-239: Update the structured-error encoding flow around the
task_errors import and encode_from_parts calls to emit a debug or warning log
when the module imports successfully but encoding or String extraction fails,
including the failure details. Preserve the silent plain-text fallback only when
taskito.task_errors is genuinely unavailable, and keep returning the existing
traceback fallback after logging encoder failures.
In `@dashboard/src/components/ui/task-error.tsx`:
- Around line 41-50: Update TaskErrorSummary to use the dashboard’s Tooltip
component for taskErrorTooltip(parsed) content instead of the native title
attribute, preserving the existing summary text and fallback error behavior.
Reuse the exported Tooltip primitive and ensure the tooltip wraps the rendered
span so multi-line traceback content is displayed consistently.
In `@dashboard/src/routes/workflows_`.$id.tsx:
- Around line 92-97: Update the workflow error row in the data.error handling
block to render the TaskErrorSummary component for structured errors instead of
converting them with taskErrorSummary. Pass the resulting ReactNode as the
KvList value so its existing tooltip behavior is preserved, while continuing to
use data.error for unstructured errors.
In `@sdks/node/test/core/task-error.test.ts`:
- Around line 24-30: Update the encodeTaskError contract test to exercise a
naturally populated V8 stack by creating or throwing and catching the BoomError
instead of manually assigning error.stack. Assert the expected serialization
against the actual header-plus-frames stack format, ensuring encodeTaskError
does not duplicate or incorrectly retain the stack header.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6c45b92-5e7a-4e06-a6f9-21e325e23be0
📒 Files selected for processing (35)
crates/taskito-core/BINDING_CONTRACT.mdcrates/taskito-node/src/dispatcher.rscrates/taskito-python/src/native_async/task_executor.rscrates/taskito-python/src/py_worker.rsdashboard/src/components/ui/index.tsdashboard/src/components/ui/task-error.tsxdashboard/src/features/dead-letters/components/dead-letter-row.tsxdashboard/src/features/dead-letters/components/dead-letter-table.tsxdashboard/src/features/dead-letters/utils.test.tsdashboard/src/features/dead-letters/utils.tsdashboard/src/features/jobs/components/job-errors-tab.tsxdashboard/src/features/jobs/components/job-overview-tab.tsxdashboard/src/features/jobs/components/job-table.tsxdashboard/src/features/workflows/components/workflow-nodes-table.tsxdashboard/src/lib/task-error.test.tsdashboard/src/lib/task-error.tsdashboard/src/routes/workflows_.$id.tsxsdks/java/src/main/java/org/byteveda/taskito/contrib/SentryMiddleware.javasdks/java/src/main/java/org/byteveda/taskito/errors/TaskError.javasdks/java/src/main/java/org/byteveda/taskito/errors/TaskErrors.javasdks/java/src/main/java/org/byteveda/taskito/errors/package-info.javasdks/java/src/main/java/org/byteveda/taskito/worker/WorkerDispatchBridge.javasdks/java/src/test/java/org/byteveda/taskito/errors/TaskErrorsTest.javasdks/java/test-support/src/test/java/org/byteveda/taskito/test/InMemoryQueueBackendTest.javasdks/node/src/errors.tssdks/node/src/index.tssdks/node/src/task-error.tssdks/node/src/worker.tssdks/node/test/core/task-error.test.tssdks/python/taskito/async_support/executor.pysdks/python/taskito/exceptions.pysdks/python/taskito/prefork/child.pysdks/python/taskito/result.pysdks/python/taskito/task_errors.pysdks/python/tests/core/test_task_errors.py
1 participant
Summary
{errtype, message, traceback}— so any SDK and the dashboard can show the exception class and message without parsing language-specific traceback textChanges
taskito/task_errors.pysingle encoder/decoder (the Rust worker paths call it, collapsing four duplicated format sites into one); outcome exceptions gainerrtype,traceback,job_id, andraw_error; the error-summary helper is structure-awaretask-error.tsencode/decode; the worker rethrows a JSON-carrying error and the dispatcher reads its reason so the stored string is the bare JSON;JobFailedErrorexposeserrtype/tracebackerrors/TaskErrorscodec (fully-qualified class name, stack frames) emitted by the worker bridge; JNI path unchangedTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Tests