v1.0.36 - #97
Merged
Merged
Conversation
…stic BackgroundJob.Schedule, .Schedule.OneShot, .Schedule.Delete and .Dispatch called Source.StartActivity directly, bypassing the Verbose gate that the rest of the infrastructure instrumentation goes through. They therefore exported in the default Business profile, adding depth to traces that are already deep with transition chains and subflows without telling the reader anything the job's own execution span does not. Route them through InfrastructureActivitySource.StartDiagnosticActivity, which is the existing mechanism for exactly this. BackgroundJob.Execute is deliberately left alone: it is a real service boundary and the ambient parent of the work the job performs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A caller that must persist a job inside a critical section had no way to keep the scheduler round-trip out of it. EnqueueAsync either armed inline or on the ambient unit of work's completion, and both land inside the caller's lock. Measured on a workflow accept path, that external call WAS the lock hold time under load (p50 214 ms of a 198 ms hold), serialising every other request on the same instance behind it. EnqueueWithDeferredArmAsync persists with the same semantics as directly: true — the row lands Scheduled, an arm failure rolls it back to Pending for the arming poller — and returns an IBackgroundJobArmHandle instead of calling the scheduler. The handle closes over the scheduler arguments captured at enqueue time, so arming later costs one scheduler call: no job-row read, no extra status write. EnqueueAsync's body moved to a shared private core; its behaviour is unchanged. This commit also gates the Enqueue/Update/Delete producer spans as diagnostic, for the same reason as the scheduler and dispatch spans: they live in this file and splitting them into their own commit is not worth the churn. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… fleet Two problems that only bite with several replicas, which is how these workers run (10 each in nonprod and prod). No jitter. The backoff was a deterministic doubling, so pods started together — what a rolling deployment produces — poll in lockstep. That costs twice: the fleet loses the natural staggering that makes N replicas pick work up roughly N times sooner than one, and every tick becomes a burst of simultaneous claim queries over the same rows. Every delay is now jittered by +/-25%, and each loop takes a random startup offset within its idle interval so the first pass is spread too. Errors jumped straight to MaxPollingInterval. One transient fault — a brief database hiccup, a single poison message — stalled every replica for a full maximum interval, right when it was least affordable. An error now backs off one step like an empty round, floored at the idle interval so a hard failure is not retried at the busy cadence, and repeated errors still escalate to the cap. The pacing rules move into PollingDelay, shared by the outbox, inbox and background-job arming loops. The arming loop has no adaptive backoff by design — an unarmed job must be claimed within a bounded time — which makes jitter the only thing keeping its replicas apart. The existing AdaptivePollingTests exercised a private copy of the delay arithmetic rather than the production code, so a change to the real rules could not fail it. They now drive PollingDelay itself, with coverage for jitter, the error floor and the startup offset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-and-arm-handle Background-job span gating, deferred arm handle, and jittered poll pacing
Reviewer's GuideAdds a deferred background-job arming path, centralizes adaptive polling/jitter logic for inbox/outbox/arming loops, and reclassifies various background-job and scheduler spans as diagnostic tracing rather than business spans. Sequence diagram for deferred background-job armingsequenceDiagram
actor Caller
participant BackgroundJobService
participant JobStore
participant DeferredArmHandle as IBackgroundJobArmHandle
participant Scheduler
Caller->>BackgroundJobService: EnqueueWithDeferredArmAsync(handlerName, jobName, payload, schedule)
BackgroundJobService->>JobStore: SaveAsync(jobInfo, cancellationToken)
BackgroundJobService-->>Caller: IBackgroundJobArmHandle
Caller->>DeferredArmHandle: ArmAsync(cancellationToken)
DeferredArmHandle->>BackgroundJobService: ArmNowAsync(handlerName, jobName, schedule, payloadBytes, failurePolicyOptions, jobId, cancellationToken)
BackgroundJobService->>Scheduler: ScheduleAsync(handlerName, jobName, schedule, payloadBytes)
Scheduler-->>BackgroundJobService: result
BackgroundJobService-->>DeferredArmHandle: completion
DeferredArmHandle-->>Caller: completion
Flow diagram for adaptive polling delay with jitterflowchart TD
A["Start polling loop"] --> B["Task.Delay(StartupOffset)"]
B --> C["RunAsync"]
C --> D{"RunAsync threw?"}
D -- Yes --> E["delay = PollingDelay.OnError(delay, IdlePollingInterval, MaxPollingInterval)"]
D -- No --> F{"processed > 0?"}
F -- Yes --> G["delay = PollingDelay.OnProcessed(BusyPollingInterval)"]
F -- No --> H["delay = PollingDelay.OnEmpty(delay, MaxPollingInterval)"]
E --> I["Task.Delay(PollingDelay.Jitter(delay))"]
G --> I
H --> I
I --> J["Next polling iteration"] --> C
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 27 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
EnqueueCoreAsyncthe tuple elementArmis nullable but is immediately dereferenced witharm!when creatingDeferredArmHandle; consider making the delegate non-nullable whendeferArmis true (e.g., via branching or a Debug.Assert) to avoid relying on the null-forgiving operator for correctness. - The XML doc on
IBackgroundJobArmHandle.ArmAsyncpromises that it "never throws", but the implementation simply forwards to the capturedArmNowAsyncdelegate; it would be good to double-check that this delegate always swallows/logs exceptions so that the observable behavior actually matches the documented contract.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `EnqueueCoreAsync` the tuple element `Arm` is nullable but is immediately dereferenced with `arm!` when creating `DeferredArmHandle`; consider making the delegate non-nullable when `deferArm` is true (e.g., via branching or a Debug.Assert) to avoid relying on the null-forgiving operator for correctness.
- The XML doc on `IBackgroundJobArmHandle.ArmAsync` promises that it "never throws", but the implementation simply forwards to the captured `ArmNowAsync` delegate; it would be good to double-check that this delegate always swallows/logs exceptions so that the observable behavior actually matches the documented contract.
## Individual Comments
### Comment 1
<location path="framework/src/BBT.Aether.Infrastructure/BBT/Aether/Polling/PollingDelay.cs" line_range="31-40" />
<code_context>
+ internal static TimeSpan OnEmpty(TimeSpan current, TimeSpan max) => MinOf(Double(current), max);
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against TimeSpan ticks overflow when doubling delays.
`Double()` uses `TimeSpan.FromTicks(value.Ticks * 2)`, which can overflow `long` for very large values (near `TimeSpan.MaxValue`) and throw. To keep the backoff robust against misconfiguration, clamp the doubled value to `TimeSpan.MaxValue` or the provided `max` before creating the new `TimeSpan`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+31
to
+40
| internal static TimeSpan OnEmpty(TimeSpan current, TimeSpan max) => MinOf(Double(current), max); | ||
|
|
||
| /// <summary> | ||
| /// The delay after a failed round. Backs off one step like an empty round, but never below | ||
| /// <paramref name="idleInterval"/> so a hard failure is not retried at the busy cadence. | ||
| /// <para> | ||
| /// Deliberately NOT a jump straight to <paramref name="max"/>. That is what the loops used to do, | ||
| /// and with several replicas a single transient fault — a brief database hiccup, one poison | ||
| /// message — stalled the entire fleet for a full maximum interval, right when it was most needed. | ||
| /// Escalating instead keeps a one-off blip cheap while a persistent fault still ends up at the cap. |
There was a problem hiding this comment.
issue (bug_risk): Guard against TimeSpan ticks overflow when doubling delays.
Double() uses TimeSpan.FromTicks(value.Ticks * 2), which can overflow long for very large values (near TimeSpan.MaxValue) and throw. To keep the backoff robust against misconfiguration, clamp the doubled value to TimeSpan.MaxValue or the provided max before creating the new TimeSpan.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Improve background-job scheduling and event polling to reduce lock contention, desynchronize replicas, and make transient failures recover more quickly.
New Features:
Bug Fixes:
Enhancements:
Tests: