Repository navigation
fix(reconciler): isolate permanent batcher failures per work unit - #1201
VLukiantsev merged 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a permanent failure isolation mechanism for the update batcher. Instead of dropping an entire batch upon encountering a permanent storage error, the batcher now groups updates by work unit and resends up to 32 work units individually. It handles consecutive permanent failures as experiment-wide issues, dropping remaining units, while transient errors or context cancellations trigger a requeue of the remaining work units. The feedback suggests using context.WithoutCancel(ctx) when calling b.requeue during context cancellation or transient errors to prevent logging and cleanup operations from being skipped due to a cancelled context.
There was a problem hiding this comment.
Code Review
This pull request introduces a permanent failure isolation mechanism for the update batcher. Instead of dropping an entire batch upon encountering a permanent storage error, the batcher now groups updates by work unit and resends up to 32 work units individually. It handles consecutive permanent failures as experiment-wide issues, dropping remaining units, while transient errors or context cancellations trigger a requeue of the remaining work units. The feedback suggests using context.WithoutCancel(ctx) when calling b.requeue during context cancellation or transient errors to prevent logging and cleanup operations from being skipped due to a cancelled context.
I am having trouble creating individual review comments. Click here to see my feedback.
cmd/reconciler/shared/batcher_retry.go (233-236)
When the context is cancelled (ctx.Err() != nil), passing the cancelled ctx to b.requeue (which subsequently logs via b.recordDropped) can cause logging or other context-bound operations to be skipped or fail, depending on the logging configuration.
Using context.WithoutCancel(ctx) (introduced in Go 1.21) is a safer alternative here. It creates a context that inherits values (like trace IDs for structured logging) from the parent context but discards the cancellation signal, ensuring that cleanup and logging are successfully executed.
if ctx.Err() != nil {
b.requeue(context.WithoutCancel(ctx), xid, slices.Concat(groups[i:]...), ctx.Err())
return false
}
cmd/reconciler/shared/batcher_retry.go (242-245)
If the transient error err is due to context cancellation or deadline expiration, the parent ctx will be cancelled. Passing a cancelled context to b.requeue can cause logging or other context-bound operations to be skipped or fail.
Using context.WithoutCancel(ctx) ensures that cleanup and logging are successfully executed while still preserving trace context.
if !IsPermanentStorageError(err) {
b.requeue(context.WithoutCancel(ctx), xid, slices.Concat(groups[i:]...), err)
return false
}
1acb44b to
fcc91c8
Compare
3388c09 to
093beed
Compare
093beed to
88a9017
Compare
When a multi-work-unit flush fails permanently, resend it one work unit at a time (all of that work unit's sources in one call), since Storage reports missing rows per work unit. Isolation is bounded: - At most 32 work units are isolated per failed batch. - It stops after 3 consecutive work units fail permanently, treating the failure as experiment-wide and dropping the unsent work units. - It aborts on the first transient error or context cancellation and requeues that work unit and all unsent ones. - Work units beyond the cap are dropped if the failure was experiment-wide and requeued otherwise. A batch covering a single work unit is dropped on a permanent failure. Part of b/569896494 TAG=agy CONV=cbf91f29-980f-4054-9fd9-4a0bf6574811
Address review: requeue/logging must still run after the flush context is cancelled. Covered by the cancelled-ctx isolation test case. Part of b/569896494 TAG=agy CONV=cbf91f29-980f-4054-9fd9-4a0bf6574811
88a9017 to
f6e2f28
Compare
…isolation Comment link: #1201 (comment) Comment link: #1201 (comment) Comment link: #1201 (comment) TAG=agy
Summary
When a batch that covers more than one work unit fails permanently, resend it one work unit at a time so a single bad work unit no longer drops valid updates for the others. The resending is bounded: at most 32 work units per batch, stopping after 3 work units fail permanently in a row, and aborting and re-queueing on any temporary error or cancellation. Layer 3 of the b/569896494 stack.
Motivation & Context
Storage fails the whole per-experiment transaction when any one work unit is bad. Layer 2a drops the batch in that case. This layer recovers the valid updates.
Design:
groupByWorkUnitorders groups by descending max retryattempt(preserving ascendingWIDon ties) so previously re-queued overflow work units (attempt > 0) are isolated ahead of fresh (attempt == 0) updates on the next flush and cannot be starved behind active low-WID work units.reason=permanent). Otherwise, work units past the limit are re-queued and handled over later ticks.Key Changes
cmd/reconciler/shared/batcher_retry.go: newgroupByWorkUnit(andmaxGroupAttempt) groups updates per work unit ordered by descending retry attempt; newisolatePermanentFailurereturnsexperimentWide;handleFlushFailureapplies the single-work-unit fast path, isolation, and overflow rule.docs/flows/reconciliation.md§6: isolation bounds and mermaid diagram (including single-work-unit vs multi-work-unit permanent error branches).Assumptions
retry_budget_exhausted). The most common trigger, deleted work units, is handled without any resending by the next PR (typedWorkUnitNotFoundError).Verification & Testing
bazel test //cmd/reconciler/...: 7/7 pass.//cmd/reconciler/shared:shared_test: passes.TestRetry_PermanentMultiWorkUnitIsolation&TestRetry_IsolationPrioritizesRequeuedOverflow):WID 1, 2, 4failing,WID 3, 5succeeding) reset the consecutive streak and try all 5 work units;TestRetry_IsolationPrioritizesRequeuedOverflow: whenWID 40has a permanent failure andWID 1..32emit fresh updates on the next tick, re-queuedWID 33..40(attempt = 1) are isolated first soWID 40is dropped on tick 2;WID 1succeeds: onlyWID 2andWID 3are re-queued;Related Issues / Bugs
Part of b/569896494. Stacked on #1200.
TAG=agy
CONV=cbf91f29-980f-4054-9fd9-4a0bf6574811