Repository navigation
Fix a race condition in WorkScheduler - #10463
Conversation
The test creates a controlled version of the narrow race window. It does not try to corrupt a dictionary; it checks the earlier condition that makes that possible: the scheduler reports the request complete while a resolver is still finishing WorkScheduler.Complete. 1. Keep one resolver pending. The test’s Query.GetValueAsync returns a TaskCompletionSource<int> task. It waits for the diagnostic listener’s first StopProcessing notification, which means the scheduler has run out of immediately available work and paused. 2. Release the resolver, then stop it at the critical point. The test completes the resolver’s task. A DEBUG-only hook in WorkScheduler.Complete runs just after work.Complete() has reduced the running-task count to zero, but before _completed and _activePaths are updated under _sync. The hook signals entered and waits on release, holding the resolver precisely in that gap. 3. Wake the scheduler. The test calls scheduler.OnNext with a batch-dispatch event. This simulates a scheduler wake-up that can occur while the resolver is in that gap. The listener’s second StartProcessing signal confirms the scheduler actually resumed. 4. Check which event happens first. Task.WhenAny watches for the GraphQL execution to finish, the scheduler to pause again, or a safety timeout. The finally block releases the resolver regardless of the assertion outcome, preventing a stuck test. The hook and test are compiled only in DEBUG builds.
This moves the reporting of task completion into a lock to avoid stale state in a very specific set of conditions.
|
|
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The locking change closes the documented race, and the regression test verifies the critical ordering.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes a WorkScheduler race where request cleanup could overlap task-completion bookkeeping.
Changes:
- Makes queue and scheduler bookkeeping atomic under
_sync. - Adds a regression test reproducing the completion ordering.
| File | Description |
|---|---|
WorkSchedulerCompletionTests.cs |
Tests premature scheduler completion. |
WorkScheduler.cs |
Synchronizes task completion bookkeeping. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Are there blockers for this? At our traffic volume this happens pretty frequently and as we're upgrading more and more subgraphs to 16, the teams operating them notice new errors and get confused. |
|
@martindisch I assigned it to Michael last week, as he's the one who rewrote the WorkScheduler for 16 and has the most knowledge out of all of us in this region. Unfortunately he's been busy traveling due to various conferences the past week. |
Co-authored-by: Michael Staib <michael@chillicream.com>
We noticed in our telemetry that every now and then we log an unobserved exception. This happens particularly in subgraphs that operate under high load.
The exception is:
We traced the problem to the WorkScheduler and think that it happens as follows:
The
WorkScheduler.Completedecrements the last resolver’s running count before acquiring_syncto update_completedand_activePaths. Meanwhile,TryDispatchOrCompleteUnsafecan acquire_sync, see no running work, and finish the request. Request cleanup can then clear those dictionaries while the resolver was still updating them.The first commit adds a regression test for this ordering (this needed a hook into the WorkScheduler so feel free to reject that); the second moves the count decrement and bookkeeping into the same critical section.
Feel free to pick and choose whatever you want from this PR or throw it away and just treat it as an exploration 🙂