fix: avoid redundant remote query startup ticks

.changeset/quiet-query-ticks.md

@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+fix: avoid scheduling ticks when reading already-started remote queries

packages/kit/src/runtime/client/remote-functions/query/instance.svelte.js

@@ -93,6 +93,8 @@ export class Query {
 	}
 
 	start() {
+		if (untrack(() => this.#promise !== null)) return;
+
 		// there is a really weird bug with untrack and writes and initializations
 		// every time you see this comment, try removing the `tick.then` here and see
 		// if all the tests still pass with the latest svelte version

packages/kit/src/runtime/client/remote-functions/query/proxy.svelte.spec.js

@@ -13,7 +13,7 @@ vi.mock(new URL('../../client.js', import.meta.url).pathname, () => ({
 }));
 
 const { QueryProxy } = await import('./proxy.js');
-const { query_map } = await import('../../client.js');
+const { query_map, query_responses } = await import('../../client.js');
 
 /**
  * Force GC several times and pump microtasks/macrotasks so the FinalizationRegistry
@@ -80,6 +80,49 @@ describe('QueryProxy', () => {
 		expect(query_map.get('q')?.size).toBe(2);
 	});
 
+	test('a failed SSR-seeded query stays failed until explicitly refreshed', async () => {
+		const error = { status: 418, message: 'teapot' };
+		query_responses['q/'] = { e: error };
+		const fn = vi.fn(() => Promise.resolve('recovered'));
+		const proxy = new QueryProxy('q', undefined, fn);
+
+		expect(proxy.ready).toBe(false);
+		expect(proxy.loading).toBe(false);
+		expect(proxy.current).toBeUndefined();
+		expect(proxy.error).toEqual(error);
+		await expect(proxy).rejects.toMatchObject({ body: error });
+		await tick();
+		expect(fn).not.toHaveBeenCalled();
+
+		await proxy.refresh();
+		await expect(proxy).resolves.toBe('recovered');
+		expect(proxy.ready).toBe(true);
+		expect(proxy.error).toBeUndefined();
+		expect(fn).toHaveBeenCalledTimes(1);
+	});
+
+	test('reading error does not subscribe to changes to the internal promise', async () => {
+		const proxy = new QueryProxy('q', undefined, () => Promise.resolve('value'));
+		proxy.set('value');
+		let runs = 0;
+		const destroy = $effect.root(() => {
+			$effect.pre(() => {
+				runs += 1;
+				void proxy.error;
+			});
+		});
+		try {
+			await tick();
+			expect(runs).toBe(1);
+			await proxy.refresh();
+			await tick();
+			// Refresh replaces the promise, but the error has not changed.
+			expect(runs).toBe(1);
+		} finally {
+			destroy();
+		}
+	});
+
 	test('a proxy that goes out of scope is garbage collected and the cache entry is evicted', async () => {
 		const fn = () => Promise.resolve('value');
 

packages/kit/test/apps/async/src/routes/remote/query-start/[kind]/+page.svelte

@@ -0,0 +1,43 @@
+<script lang="ts">
+	import type { RemoteQuery } from '$app/server';
+	import { single, batch } from '../data.remote.js';
+
+	const { params } = $props();
+	const query = (n: number) => (params.kind === 'batch' ? batch(n) : single(n));
+	const answer = query(1);
+	const initial = await answer;
+	let waiting = $state<RemoteQuery<number>>();
+	let frames = $state<number>();
+
+	function inspect(resource: RemoteQuery<number>) {
+		const original = window.requestAnimationFrame;
+		let count = 0;
+		window.requestAnimationFrame = (callback) => {
+			count += 1;
+			return original.call(window, callback);
+		};
+		try {
+			for (let i = 0; i < 1000; i++) {
+				void resource.ready;
+				void resource.current;
+				void resource.loading;
+				void resource.error;
+				void resource.then;
+				void resource.catch;
+				void resource.finally;
+			}
+		} finally {
+			window.requestAnimationFrame = original;
+		}
+		frames = count;
+	}
+</script>
+
+<p id="initial">{initial}</p>
+<p id="answer">{answer.current}</p>
+<p id="waiting">{waiting?.ready} / {waiting?.current}</p>
+<output>{frames}</output>
+<button id="inspect-answer" onclick={() => inspect(answer)}>Inspect answer</button>
+<button id="start" onclick={() => (waiting = query(2))}>Start</button>
+<button id="inspect-waiting" onclick={() => waiting && inspect(waiting)}>Inspect waiting</button>
+<button id="refresh" onclick={() => answer.refresh()}>Refresh</button>

packages/kit/test/apps/async/src/routes/remote/query-start/data.remote.js

@@ -0,0 +1,5 @@
+import { query } from '$app/server';
+import * as v from 'valibot';
+
+export const single = query(v.number(), (n) => n);
+export const batch = query.batch(v.number(), () => (n) => n);

packages/kit/test/apps/async/test/client.test.js

@@ -22,6 +22,78 @@ test.describe('remote functions', () => {
 		await expect(page.locator('p')).toHaveText('foobaz');
 	});
 
+	for (const kind of ['single', 'batch']) {
+		test(`${kind} query getters do not schedule rendering work after starting`, async ({
+			page,
+			app
+		}) => {
+			let requests = 0;
+			page.on('request', (r) => (requests += r.url().includes('/_app/remote/') ? 1 : 0));
+			await page.goto(`/remote/query-start/${kind}`);
+			await expect(page.locator('#initial')).toHaveText('1');
+			await page.click('#inspect-answer');
+			await expect(page.locator('output')).toHaveText('0');
+			expect(requests).toBe(0);
+
+			const held = Promise.withResolvers();
+			const started = Promise.withResolvers();
+			await page.route('**/_app/remote/**', async (route) => {
+				started.resolve(undefined);
+				await held.promise;
+				await route.continue();
+			});
+			try {
+				await page.click('#start');
+				await started.promise;
+				await expect(page.locator('#waiting')).toHaveText('false /');
+				await page.click('#inspect-waiting');
+				await expect(page.locator('output')).toHaveText('0');
+			} finally {
+				held.resolve(undefined);
+			}
+			await expect(page.locator('#waiting')).toHaveText('true / 2');
+			expect(requests).toBe(1);
+			await page.unrouteAll({ behavior: 'wait' });
+
+			await Promise.all([
+				page.waitForResponse((r) => r.url().includes('/_app/remote/')),
+				page.click('#refresh')
+			]);
+			expect(requests).toBe(2);
+			await expect(page.locator('#answer')).toHaveText('1');
+
+			// Navigation resets both queries, including the non-awaited one.
+			await app.goto(`/remote/query-start/${kind}`, { refreshAll: true });
+			await expect.poll(() => requests).toBe(kind === 'batch' ? 3 : 4);
+			await expect(page.locator('#waiting')).toHaveText('true / 2');
+		});
+
+		test(`${kind} query getters work after committed and discarded preload forks`, async ({
+			page,
+			app
+		}) => {
+			/** @type {string[]} */
+			const errors = [];
+			page.on('pageerror', (error) => errors.push(error.message));
+			await page.goto('/fork');
+			const preloaded = page.waitForResponse((r) => r.url().includes('/_app/remote/'));
+			await app.preloadData(`/remote/query-start/${kind}`);
+			await (await preloaded).finished();
+			await app.preloadData('/'); // discard the first fork
+			await app.preloadData(`/remote/query-start/${kind}`);
+			await app.goto(`/remote/query-start/${kind}`); // commit the second fork
+			await expect(page.locator('#initial')).toHaveText('1');
+			await page.click('#inspect-answer');
+			await expect(page.locator('output')).toHaveText('0');
+
+			const response = page.waitForResponse((r) => r.url().includes('/_app/remote/'));
+			await app.goto(`/remote/query-start/${kind}`, { refreshAll: true });
+			await response;
+			await expect(page.locator('#answer')).toHaveText('1');
+			expect(errors).toEqual([]);
+		});
+	}
+
 	test('remote query responses are not cacheable', async ({ page }) => {
 		// the query is kicked off during SSR but fetched by the client after
 		// hydration, so we can observe the response headers on the wire