Repository navigation
fix(desktop): keep draining the worker queue past a pre-aborted task - #9482
Bruno-rose wants to merge 2 commits into
Conversation
drainQueue moved to the next slot after rejecting a pre-aborted task, so a live task queued behind it stayed queued while the slot sat idle. Pull from the queue until the slot dispatches or the queue is empty, as the host-service runner already does. No caller passes a signal today.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 19 minutes. View limit details
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. |
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/workers/WorkerTaskRunner.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/workers/WorkerTaskRunner.ts:282">
P1: This new loop can reach an uncloneable payload after skipping a pre-aborted task, but `postMessage` throws synchronously and can crash the desktop process. Catch the dispatch error, clear `activeTaskId`, and reject the task so draining can continue.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Client Code
participant Runner as WorkerTaskRunner
participant Queue as Internal Task Queue
participant Slot as Worker Slot
participant Worker as Worker Process
Note over Client,Worker: Task dispatch with pre-aborted task handling
Client->>Runner: runTask("first", payload)
Runner->>Queue: Enqueue first task
Runner->>Slot: Acquire free slot (concurrency=1)
Slot->>Worker: postMessage(task "first")
Worker-->>Slot: Task complete
Slot-->>Runner: Empty slot
Client->>Runner: runTask("aborted", { signal: AbortSignal.abort() })
Runner->>Queue: Enqueue aborted task
Client->>Runner: runTask("third", payload)
Runner->>Queue: Enqueue third task (behind aborted)
Runner->>Runner: drainQueue() triggered
loop While queue not empty and slot free
Runner->>Queue: Shift next task (aborted)
Runner->>Runner: Check abortSignal
alt Task pre-aborted
Runner->>Runner: rejectTask("cancelled")
Runner->>Queue: Continue loop, shift next task
else Task valid
Runner->>Runner: Assign to slot, set timeout
Slot->>Worker: postMessage(task "third")
Worker-->>Slot: Task complete
Slot-->>Runner: Empty slot
end
end
Runner-->>Client: Resolve "first"
Runner-->>Client: Reject "aborted" (WorkerTaskAbortedError)
Runner-->>Client: Resolve "third" with result
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| taskType: task.taskType, | ||
| payload: task.payload, | ||
| }; | ||
| slot.worker.postMessage(request); |
There was a problem hiding this comment.
P1: This new loop can reach an uncloneable payload after skipping a pre-aborted task, but postMessage throws synchronously and can crash the desktop process. Catch the dispatch error, clear activeTaskId, and reject the task so draining can continue.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/desktop/src/lib/trpc/workers/WorkerTaskRunner.ts, line 282:
<comment>This new loop can reach an uncloneable payload after skipping a pre-aborted task, but `postMessage` throws synchronously and can crash the desktop process. Catch the dispatch error, clear `activeTaskId`, and reject the task so draining can continue.</comment>
<file context>
@@ -256,29 +256,31 @@ export class WorkerTaskRunner {
+ taskType: task.taskType,
+ payload: task.payload,
+ };
+ slot.worker.postMessage(request);
}
-
</file context>
| slot.worker.postMessage(request); | |
| try { | |
| slot.worker.postMessage(request); | |
| } catch (error) { | |
| slot.activeTaskId = null; | |
| this.rejectTask( | |
| task.taskId, | |
| new WorkerTaskError( | |
| `postMessage failed for task "${task.taskType}": ${(error as Error).message}`, | |
| ), | |
| ); | |
| } |
What & why
Fixes #9463.
drainQueuerejected a pre-aborted task and moved to the next slot, so the task queued behind it waited with no timeout until another task arrived. This copies the inner loop the host-serviceWorkerTaskRunneralready has. No current caller passes asignal, so this cannot happen today.How I tested it
Failing regression test with the existing
MockWorker. The bug was first found by checking every order of events in a small model, then reproduced against the real code.WorkerTaskRunner.test.ts: 6 pass, 0 fail.Checklist
type(scope): subject)bun run lintandbun run typecheckpass (CI fails on lint warnings too)Summary by cubic
Fixes #9463:
drainQueuerejected a pre-aborted task and moved to the next slot, leaving the next queued task stranded with no timeout until another task arrived to trigger another drain.The slot now keeps pulling from the queue until it dispatches a task or the queue is empty, matching the inner loop the host-service
WorkerTaskRunneralready uses. No current caller passes asignal, so this can't happen today; a regression test covers the scenario.Written for commit d8647ea. Summary will update on new commits.