fix: clean up scheduling system (#16741)

Consolidates our scheduling system that had diverged into two - queue_micro_task and Batch.ensure-queues and resolves some edge case race conditions related to flushSync along the way.

Author: dummdidumm

.changeset/old-taxis-relate.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: clean up scheduling system

packages/svelte/src/internal/client/dom/task.js

@@ -1,4 +1,5 @@
 import { run_all } from '../../shared/utils.js';
+import { is_flushing_sync } from '../reactivity/batch.js';
 
 // Fallback for when requestIdleCallback is not available
 const request_idle_callback =
@@ -24,12 +25,27 @@ function run_idle_tasks() {
 	run_all(tasks);
 }
 
+export function has_pending_tasks() {
+	return micro_tasks.length > 0 || idle_tasks.length > 0;
+}
+
 /**
  * @param {() => void} fn
  */
 export function queue_micro_task(fn) {
-	if (micro_tasks.length === 0) {
-		queueMicrotask(run_micro_tasks);
+	if (micro_tasks.length === 0 && !is_flushing_sync) {
+		var tasks = micro_tasks;
+		queueMicrotask(() => {
+			// If this is false, a flushSync happened in the meantime. Do _not_ run new scheduled microtasks in that case
+			// as the ordering of microtasks would be broken at that point - consider this case:
+			// - queue_micro_task schedules microtask A to flush task X
+			// - synchronously after, flushSync runs, processing task X
+			// - synchronously after, some other microtask B is scheduled, but not through queue_micro_task but for example a Promise.resolve() in user code
+			// - synchronously after, queue_micro_task schedules microtask C to flush task Y
+			// - one tick later, microtask A now resolves, flushing task Y before microtask B, which is incorrect
+			// This if check prevents that race condition (that realistically will only happen in tests)
+			if (tasks === micro_tasks) run_micro_tasks();
+		});
 	}
 
 	micro_tasks.push(fn);

packages/svelte/src/internal/client/reactivity/batch.js

@@ -25,7 +25,7 @@ import {
 	update_effect
 } from '../runtime.js';
 import * as e from '../errors.js';
-import { flush_tasks } from '../dom/task.js';
+import { flush_tasks, has_pending_tasks, queue_micro_task } from '../dom/task.js';
 import { DEV } from 'esm-env';
 import { invoke_error_boundary } from '../error-handling.js';
 import { old_values } from './sources.js';
@@ -56,27 +56,14 @@ export let batch_deriveds = null;
 /** @type {Set<() => void>} */
 export let effect_pending_updates = new Set();
 
-/** @type {Array<() => void>} */
-let tasks = [];
-
-function dequeue() {
-	const task = /** @type {() => void} */ (tasks.shift());
-
-	if (tasks.length > 0) {
-		queueMicrotask(dequeue);
-	}
-
-	task();
-}
-
 /** @type {Effect[]} */
 let queued_root_effects = [];
 
 /** @type {Effect | null} */
 let last_scheduled_effect = null;
 
 let is_flushing = false;
-let is_flushing_sync = false;
+export let is_flushing_sync = false;
 
 export class Batch {
 	/**
@@ -470,11 +457,7 @@ export class Batch {
 
 	/** @param {() => void} task */
 	static enqueue(task) {
-		if (tasks.length === 0) {
-			queueMicrotask(dequeue);
-		}
-
-		tasks.unshift(task);
+		queue_micro_task(task);
 	}
 }
 
@@ -505,7 +488,7 @@ export function flushSync(fn) {
 		while (true) {
 			flush_tasks();
 
-			if (queued_root_effects.length === 0) {
+			if (queued_root_effects.length === 0 && !has_pending_tasks()) {
 				current_batch?.flush();
 
 				// we need to check again, in case we just updated an `$effect.pending()`

packages/svelte/tests/runtime-runes/samples/async-top-level-error-nested-obsolete/main.svelte

@@ -3,6 +3,11 @@
 	export let route = $state({ current: 'home' });
 </script>
 
+<script>
+	// reset from earlier tests
+	route.current = 'home'
+</script>
+
 <button onclick={() => route.reject()}>reject</button>
 
 <svelte:boundary>

packages/svelte/tests/runtime-runes/samples/dynamic-component-transition/_config.js

@@ -5,15 +5,16 @@ export default test({
 	async test({ assert, target, raf }) {
 		const btn = target.querySelector('button');
 
-		raf.tick(0);
+		// one tick to not be at 0. Else the flushSync would revert the in-transition which hasn't started, and directly remove the button
+		raf.tick(1);
 
 		flushSync(() => {
 			btn?.click();
 		});
 
 		assert.htmlEqual(target.innerHTML, `<h1>Outside</h1><button style="opacity: 0;">Hide</button>`);
 
-		raf.tick(100);
+		raf.tick(101);
 
 		assert.htmlEqual(target.innerHTML, `<h1>Outside</h1>`);
 	}

packages/svelte/tests/runtime-runes/samples/dynamic-if-component-transition/_config.js

@@ -5,15 +5,16 @@ export default test({
 	async test({ assert, target, raf }) {
 		const btn = target.querySelector('button');
 
-		raf.tick(0);
+		// one tick to not be at 0. Else the flushSync would revert the in-transition which hasn't started, and directly remove the button
+		raf.tick(1);
 
 		flushSync(() => {
 			btn?.click();
 		});
 
 		assert.htmlEqual(target.innerHTML, `<h1>Outside</h1><button style="opacity: 0;">Hide</button>`);
 
-		raf.tick(100);
+		raf.tick(101);
 
 		assert.htmlEqual(target.innerHTML, `<h1>Outside</h1>`);
 	}