feat(sync): add delayed task submission for throttling#1506
feat(sync): add delayed task submission for throttling#1506
Conversation
|
|
@sentry review |
|
@cursor review |
0f6f4b9 to
a4428e0
Compare
a4428e0 to
344664e
Compare
Add sentry__bgworker_submit_delayed() to defer task execution by a given number of milliseconds. Tasks are sorted by readiness time using monotonic timestamps, so a ready delayed task is not bypassed by a later-submitted immediate task. On shutdown, tasks that exceed the deadline (started + timeout) are pruned while the rest execute normally. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The flush marker now uses the last task's deadline (capped at the flush timeout) so it sorts after all current tasks including delayed ones. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract the core submission logic into submit_at which takes an absolute execute_after time. submit and submit_delayed become thin wrappers. Use submit_at in bgworker_delayed_tasks to pin all tasks to a single base timestamp, making the test ordering deterministic regardless of OS preemption between submissions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move delayed task cleanup from the shutdown pre-prune pass into the worker thread itself. When the worker encounters a delayed task that is not yet ready and shutdown has been signaled, it discards all remaining tasks. This catches tasks submitted during execution that the one-time pre-prune in shutdown could not see. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Instead of explicitly discarding delayed tasks in worker_thread, teach sentry__bgworker_is_done to treat pending delayed tasks as done when !running. Remaining tasks are cleaned up by decref. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3bd8b5d to
c28a807
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a delayed task's deadline exceeds the flush timeout, don't use it to delay the flush sentinel. Such tasks cannot complete within the timeout anyway, and capping to the timeout caused the sentinel to race with the caller's deadline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@sentry review |
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| sentry__mutex_lock(&bgw->task_lock); | ||
| if (bgw->last_task && bgw->last_task->execute_after > before | ||
| && bgw->last_task->execute_after - before <= timeout) { | ||
| delay_ms = bgw->last_task->execute_after - before; |
There was a problem hiding this comment.
Flush skips eligible delayed tasks
Medium Severity
bgworker_flush derives delay_ms from bgw->last_task only, so a far-future tail task forces delay_ms to 0 even when earlier delayed tasks are due within timeout. The flush sentinel is then scheduled too early via sentry__bgworker_submit_delayed, and sentry__bgworker_flush can report success before runnable delayed tasks execute.
Additional Locations (1)
| " ms delayed task to background worker thread", | ||
| delay_ms); | ||
| return sentry__bgworker_submit_at(bgw, exec_func, cleanup_func, task_data, | ||
| sentry__monotonic_time() + delay_ms); |
There was a problem hiding this comment.
Delayed deadline can wrap around
Low Severity
s쳻sentry__bgworker_submit_delayedcomputesexecute_afterassentry__monotonic_time() + delay_mswithout overflow handling. For very largedelay_ms`, unsigned wraparound can place the deadline in the past, causing the task to execute immediately instead of being deferred.
| // place the flush sentinel after the last task due within the timeout; | ||
| // tasks delayed beyond the timeout cannot complete in time anyway | ||
| uint64_t delay_ms = 0; | ||
| uint64_t before = sentry__monotonic_time(); | ||
| sentry__mutex_lock(&bgw->task_lock); | ||
| if (bgw->last_task && bgw->last_task->execute_after > before | ||
| && bgw->last_task->execute_after - before <= timeout) { | ||
| delay_ms = bgw->last_task->execute_after - before; | ||
| } | ||
| sentry__mutex_unlock(&bgw->task_lock); |
There was a problem hiding this comment.
Bug: A race condition in sentry__bgworker_flush() can cause it to return prematurely if new tasks are submitted concurrently during the flush operation.
Severity: MEDIUM
Suggested Fix
To fix this, the lock should be held for the entire duration of the operation. Specifically, the lock should be acquired before reading the last_task and only released after the flush sentinel has been successfully submitted to the task queue. This ensures that no new tasks can be inserted between calculating the sentinel's position and enqueuing it.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry_sync.c#L366-L375
Potential issue: A race condition exists in the `sentry__bgworker_flush()` function. The
function calculates a delay for a flush sentinel based on the last task in the queue
while holding a lock. It then releases the lock before submitting the sentinel task. In
the time between releasing the lock and submitting the sentinel, another thread can add
a new task with a later deadline. This results in the sentinel being placed in the queue
before the newly added task, causing the `flush()` function to return prematurely before
all tasks have completed.
Did we get this right? 👍 / 👎 to inform future reviews.


Add
sentry__bgworker_submit_delayed()to allow deferring task execution by a given number of milliseconds. This paves the road for HTTP retries that should be first throttled on startup (similar to Cocoa/iOS and Java/Android SDKs that throttle HTTP retries by 100ms) and then scheduled with exponential backoff (15min, 30min, 1h, 2h, ...).Required for:
#skip-changelog (internal)