-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(run_delivery): deliver triggered run failures to the creator (#6896) #7131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9dc9f0c
31765a3
8ab5646
08a0711
f6cdf61
01e887f
f8af109
c2460ed
d8f5bcc
93713dc
3452f43
e0d341f
060d70f
b1ed4ec
767f95d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ use crate::{ | |
| use super::{ | ||
| TriggerActiveRunState, TriggerActiveRunStateRequest, TriggerPollerFailureReason, | ||
| TriggerPollerFireOutcome, TriggerPollerFireReport, TriggerPollerTickReport, | ||
| TriggerPollerWorker, failure::classify_failure, | ||
| TriggerPollerWorker, TriggerRunFailureSettlement, failure::classify_failure, | ||
| }; | ||
|
|
||
| struct ActiveLookupItem { | ||
|
|
@@ -145,7 +145,7 @@ impl TriggerPollerWorker { | |
| }; | ||
| match clear { | ||
| Some((cleared_outcome, status)) => { | ||
| let outcome = if self | ||
| let cleared = self | ||
| .deps | ||
| .repository | ||
| .clear_active_fire(ClearActiveFireRequest { | ||
|
|
@@ -156,8 +156,31 @@ impl TriggerPollerWorker { | |
| status, | ||
| }) | ||
| .await? | ||
| .is_some() | ||
| { | ||
| .is_some(); | ||
| // A terminal failure (run ended `Failed` / `Cancelled` / | ||
| // `RecoveryRequired`, cleared as `Error`) previously had no | ||
| // settlement failure hook — the active-cleanup sweep cleared | ||
| // the slot with no observability. Surface it to the | ||
| // settlement observer so automation-health telemetry can see | ||
| // post-accept failures (#6896). `Ok`/`Running` and | ||
| // already-cleared fires do not fire the hook — the former | ||
| // are not failures, and the latter already did (or never | ||
| // reached terminal here). The observer is invoked inline | ||
| // per the `TriggerFireSettlementObserver` contract: it | ||
| // must be cheap and non-blocking, detaching any heavy | ||
| // work internally. | ||
| if cleared && status == TriggerRunHistoryStatus::Error { | ||
| self.deps | ||
| .fire_settlement_observer | ||
| .on_run_failure_settled(TriggerRunFailureSettlement { | ||
| tenant_id: record.tenant_id.clone(), | ||
| trigger_id: record.trigger_id, | ||
| fire_slot, | ||
| run_id, | ||
| }) | ||
| .await; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Low — Inline await of on_failed_fire_settled stalls active-cleanup sweep. The new failure-settlement hook is awaited inline inside the per-record sweep loop, between the durable clear_active_fire and report/cursor advancement. Today's impls are cheap (production logs one warn; Noop does nothing; tests push to a Mutex), and the loop already awaits backend I/O per record, so marginal latency is negligible. But the trait is async and open (Send+Sync, any implementor); the sibling on_accepted_fire_settled path deliberately decouples via bounded spawn_post_submit_delivery + bounded buffer, while this path couples sweep latency and scan-cursor progress to observer behavior. A future heavy observer (e.g., network telemetry sink) would stall active-fire cleanup for the whole tick. No lock is held across the await, so no deadlock is introduced. Fix: Detach the observer call with a bounded spawn (mirroring on_accepted_fire_settled), or harden the trait contract docs to require cheap non-blocking observers. |
||
| } | ||
| let outcome = if cleared { | ||
| cleared_outcome | ||
| } else { | ||
| TriggerPollerFireOutcome::SkippedAlreadyCleared { run_id } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.