Attribute deduplicated failures to the origin task, not a requester#239
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 40 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ 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 |
8b21d31 to
dfc67a5
Compare
A dependency failure re-raises the same error object in every waiter up
the requester chain, and the executor dedupes those into one
TaskFailure. The dedup kept the FIRST occurrence in registry order —
typically the root task, since its wrapper is created first. So for
AppTask -> DatabaseTask (raises "Database connection failed")
the AggregateError reported "- AppTask: Database connection failed",
wrapped as AppTask::Error — and the GUIDE-documented task-specific
rescue pattern (`rescue DatabaseTask::Error`) silently failed to match,
because no DatabaseTask::Error existed in the aggregate. The GUIDE's
Error Handling section documents origin attribution; reality disagreed.
Fix: TaskWrapper stamps a monotonic timestamp when recording a run or
clean failure. In the run phase a waiter can only fail AFTER its
dependency's mark_failed (the failure notification happens-after the
stamp), so for any given error object the chronologically first wrapper
is its origin. The executor sorts failed wrappers by that stamp before
flattening, so the dedup keeps the origin's TaskFailure — now wrapped
as the origin's task-specific Error class, making the documented rescue
pattern work. Failures spliced from a nested executor's AggregateError
are already origin-attributed by that executor and are preferred over a
wrapper-level entry for the same error regardless of stamp order. (The
clean phase has no propagation ordering; there the stamp only makes the
report chronological, and a shared error object means both candidate
attributions are true.)
Pinned with fixtures: a three-task requester chain attributes to the
leaf origin (and OriginFail::Error matches via AggregateAware), two
independent failures each attribute to their own task with the
requester absent, and a nested executor's inner attribution survives
the outer aggregate. Also corrects the GUIDE comment that claimed
`e.task_class` works on the rescued object (e is the AggregateError;
the task-specific error is reached via e.cause).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dfc67a5 to
1529cc0
Compare
1 participant
Problem
Found while recording the theme demo: the failure summary attributed a dependency's error to the requester chain end instead of the task that actually raised. Pre-existing on master, and it breaks behavior the GUIDE explicitly documents.
Probe (the GUIDE's own Error Handling example shape), before:
The GUIDE documents
- DatabaseTask: Database connection failedand the task-specific rescue patternrescue DatabaseTask::Error— which silently failed to match for propagated failures, because the deduplicated error was wrapped asAppTask::Error.Cause
A dependency failure re-raises the same error object in every waiter up the requester chain; the executor dedupes by error identity keeping the first occurrence in registry order — typically the root, whose wrapper is created first.
Fix
TaskWrapperstampsProcess.clock_gettime(CLOCK_MONOTONIC)when recording a run/clean failure. In the run phase a waiter can only fail after its dependency'smark_failed(the notification happens-after the stamp), so the chronologically first wrapper for an error is its origin. The executor sorts failed wrappers by the stamp before dedup.AggregateErrorare already origin-attributed by the inner executor — the dedup now prefers them over a wrapper-level entry for the same error regardless of stamp order.e.task_classworks on the rescued object (eis theAggregateError; the task-specific error is reached viae.cause) — verified inaccurate on master.After:
Tests
test_failure_attribution.rbwith fixture tasks (per CLAUDE.md): three-task requester chain attributes to the leaf origin +OriginFail::Errormatches viaAggregateAware ===; two independent failures each attribute to their own task; a nested executor's inner attribution survives the outer aggregate. The existing skip-revival assertion is strengthened by this change.Suite 790 runs / 0 failures,
rake standardclean.Adversarial review
28-agent review across the three follow-up PRs: for this PR, origin attribution verified under nested-executor, abort, clean, late-failure and stress probes; both ordering mutants killed outright by the new tests. Confirmed findings (nested-aggregate preference, run-phase-only documentation) folded in.
🤖 Generated with Claude Code