From 117116db64649e9686c0382229704acc33d8ec5f Mon Sep 17 00:00:00 2001 From: Varixo Date: Mon, 20 Oct 2025 21:59:37 +0200 Subject: [PATCH] fix: reblocking chores in scheduler --- .changeset/slick-coats-exist.md | 5 + packages/qwik/src/core/shared/scheduler.ts | 8 +- .../qwik/src/core/shared/scheduler.unit.tsx | 119 +++++++++++++++++- 3 files changed, 128 insertions(+), 4 deletions(-) create mode 100644 .changeset/slick-coats-exist.md diff --git a/.changeset/slick-coats-exist.md b/.changeset/slick-coats-exist.md new file mode 100644 index 00000000000..a19c58c3e3a --- /dev/null +++ b/.changeset/slick-coats-exist.md @@ -0,0 +1,5 @@ +--- +'@qwik.dev/core': patch +--- + +fix: reblocking chores in scheduler diff --git a/packages/qwik/src/core/shared/scheduler.ts b/packages/qwik/src/core/shared/scheduler.ts index 6d90a42863c..1af9fc58baa 100644 --- a/packages/qwik/src/core/shared/scheduler.ts +++ b/packages/qwik/src/core/shared/scheduler.ts @@ -454,7 +454,10 @@ This is often caused by modifying a signal in an already rendered component duri container ); if (blockingChore) { - addBlockedChore(blockedChore, blockingChore, blockedChores); + // Chore is still blocked, move it to the new blocking chore's list + // Note: chore is already in blockedChores Set and vnode.blockedChores, + // so we only add to the new blocking chore's list + (blockingChore.$blockedChores$ ||= new ChoreArray()).add(blockedChore); } else { blockedChores.delete(blockedChore); if (vnode_isVNode(blockedChore.$host$)) { @@ -785,8 +788,7 @@ export function addBlockedChore( undefined, blockedChores ); - blockingChore.$blockedChores$ ||= new ChoreArray(); - blockingChore.$blockedChores$.add(blockedChore); + (blockingChore.$blockedChores$ ||= new ChoreArray()).add(blockedChore); blockedChores.add(blockedChore); if (vnode_isVNode(blockedChore.$host$)) { (blockedChore.$host$.blockedChores ||= new ChoreArray()).add(blockedChore); diff --git a/packages/qwik/src/core/shared/scheduler.unit.tsx b/packages/qwik/src/core/shared/scheduler.unit.tsx index 3fe23ff730b..aa4b8bd875a 100644 --- a/packages/qwik/src/core/shared/scheduler.unit.tsx +++ b/packages/qwik/src/core/shared/scheduler.unit.tsx @@ -1,7 +1,7 @@ import { $, _jsxSorted, type JSXOutput, type OnRenderFn, type QRL } from '@qwik.dev/core'; import { createDocument } from '@qwik.dev/core/testing'; -import { beforeEach, describe, expect, it, vi, type Mocked } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi, type Mocked } from 'vitest'; import { getDomContainer } from '../client/dom-container'; import { vnode_insertBefore, @@ -676,6 +676,11 @@ describe('scheduler', () => { vHost.setProp('q:id', 'test-host'); }); + afterEach(() => { + // Restore all mocks to prevent interference with other tests + vi.restoreAllMocks(); + }); + it('should return false when there are no running chores', async () => { const mockHost = vnode_newVirtual(); mockHost.setProp('q:id', 'test-1'); @@ -857,6 +862,118 @@ describe('scheduler', () => { expect(chore1!.$blockedChores$!.length).toBe(1); }); }); + + it('should keep blockedChores Set and vnode.blockedChores in sync when re-blocking', async () => { + // Create three tasks in sequence + const task1 = mockTask(vBHost1, { + index: 0, + qrl: $(() => testLog.push('task1')), + }); + const task2 = mockTask(vBHost1, { + index: 1, + qrl: $(() => testLog.push('task2')), + }); + const task3 = mockTask(vBHost1, { + index: 2, + qrl: $(() => testLog.push('task3')), + }); + + vBHost1.setProp(ELEMENT_SEQ, [task1, task2, task3]); + + // Schedule all three tasks + const chore1 = scheduler(ChoreType.TASK, task1); + const chore2 = scheduler(ChoreType.TASK, task2); + const chore3 = scheduler(ChoreType.TASK, task3); + + // chore1 should be scheduled, chore2 blocked by chore1, chore3 blocked by chore2 + expect(choreQueue.length).toBe(1); + expect(choreQueue[0]).toBe(chore1); + expect(blockedChores.size).toBe(2); + expect(blockedChores.has(chore2!)).toBe(true); + expect(blockedChores.has(chore3!)).toBe(true); + expect(vBHost1.blockedChores?.length).toBe(2); + expect(vBHost1.blockedChores).toContain(chore2); + expect(vBHost1.blockedChores).toContain(chore3); + + // chore2 is blocked by chore1 (immediate previous task) + expect(chore1?.$blockedChores$?.length).toBe(1); + expect(chore1?.$blockedChores$).toContain(chore2); + + // chore3 is blocked by chore2 (immediate previous task), not chore1 + // When chore3 was scheduled, it found chore2 in blockedChores as its blocking chore + expect(chore2?.$blockedChores$?.length).toBe(1); + expect(chore2?.$blockedChores$).toContain(chore3); + + // Wait for drain - this will execute all tasks + await waitForDrain(); + + // After chore1 completes, all tasks should have executed + // The key test here is that during execution, when chore1 finished: + // - chore2 was unblocked (removed from blockedChores and vnode.blockedChores) + // - chore3 was checked for re-blocking and found chore2 still blocks it + // - chore3 stayed in both blockedChores and vnode.blockedChores (the bug would have caused desync) + // - chore3 was moved to chore2's $blockedChores$ list + // Then chore2 executed and unblocked chore3, then chore3 executed + + expect(testLog).toEqual(['task1', 'task2', 'task3', 'journalFlush']); + + // After drain, everything should be clear + expect(blockedChores.size).toBe(0); + expect(vBHost1.blockedChores?.length).toBe(0); + }); + + it('should maintain sync when multiple hosts have blocked chores', async () => { + // Create a scenario with multiple hosts where each has task chains + const taskA1 = mockTask(vAHost, { + index: 0, + qrl: $(() => testLog.push('taskA1')), + }); + const taskA2 = mockTask(vAHost, { + index: 1, + qrl: $(() => testLog.push('taskA2')), + }); + const taskB1 = mockTask(vBHost1, { + index: 0, + qrl: $(() => testLog.push('taskB1')), + }); + const taskB2 = mockTask(vBHost1, { + index: 1, + qrl: $(() => testLog.push('taskB2')), + }); + + vAHost.setProp(ELEMENT_SEQ, [taskA1, taskA2]); + vBHost1.setProp(ELEMENT_SEQ, [taskB1, taskB2]); + + // Schedule tasks + scheduler(ChoreType.TASK, taskA1); + const choreA2 = scheduler(ChoreType.TASK, taskA2); + scheduler(ChoreType.TASK, taskB1); + const choreB2 = scheduler(ChoreType.TASK, taskB2); + + // Initial state: A1 and B1 scheduled (depth-first), A2 and B2 blocked + expect(choreQueue.length).toBe(2); // A1, B1 + expect(blockedChores.size).toBe(2); // A2, B2 + expect(blockedChores.has(choreA2!)).toBe(true); + expect(blockedChores.has(choreB2!)).toBe(true); + + // vnode blocked chores should match + expect(vAHost.blockedChores?.length).toBe(1); + expect(vAHost.blockedChores).toContain(choreA2); + expect(vBHost1.blockedChores?.length).toBe(1); + expect(vBHost1.blockedChores).toContain(choreB2); + + // Wait for drain - this executes all tasks + await waitForDrain(); + + // All tasks should have executed + // The execution order is: A1, A2 (unblocked after A1), B1, B2 (unblocked after B1) + expect(testLog).toEqual(['taskA1', 'taskA2', 'taskB1', 'taskB2', 'journalFlush']); + + // After drain, everything should be clear + expect(blockedChores.size).toBe(0); + expect(vAHost.blockedChores?.length).toBe(0); + expect(vBHost1.blockedChores?.length).toBe(0); + }); }); function mockTask(host: VNode, opts: { index?: number; qrl?: QRL; visible?: boolean }): Task {