Answer the review: send the worker one request at a time, keep only the latest, and never wait for a dead one
The adversarial review confirmed three defects in this PR, all reproduced in the browser. 1. Superseded graphs queued up in front of routes. The worker answers messages one at a time and cannot drop one it has started. With the jump-link layer on, every pause on the range slider posted a full graph build, seconds of work at 6-8 pc. Answers no longer wanted were thrown away only once built. A route asked for afterwards waited behind every one of them: a one-jump route took 44 s. RoutingClient now holds requests and sends them one at a time. While one is out, only the latest of each kind waits: a newer graph replaces an older one before it is ever built, and the older promise is rejected with SupersededRequest. Routes go ahead of graphs. The same question asked again while outstanding shares the answer rather than being worked twice, as when the layer is turned off and on during a build. The same scenario in the browser (layer on, range stepped 5 -> 8 pc with 400 ms pauses, then Sol to Proxima): the route came back in 110 ms. The worker was sent "links 3, links 5, route, links 8"; 6 and 7 were never built. 2. A worker that failed left the panel stuck. With no error handling, a worker that failed to load (a 404 on its chunk after a redeploy) or threw left "Plotting…" and a disabled button for good, and a graph at a range could not be asked for again. The worker now answers an exception with a 'failed' message, which rejects that request. A worker that fails to load or dies is abandoned, and what it left outstanding, and everything asked afterwards, is answered in place. The scene releases the panel when a route fails, and forgets a graph range that was never drawn so it can be asked for again. 3. Nothing type-checked the worker. The application builder never reads webWorkerTsConfig, and bundles the worker with esbuild, which strips types without checking them. tsconfig.app.json leaves the file out. A type error in the worker shipped. `npm run worker:typecheck` (tsc -p tsconfig.worker.json) now runs in CI beside the other project checks. webWorkerTsConfig is removed from angular.json, since it only suggested that something checked the worker. Tests with a fake worker cover one request at a time, a waiting graph replaced and a route sent ahead of it, a question shared, a failure rejected and the next request sent, and a failed worker's requests answered in place. A scene test covers the panel released after a failed route. Negative controls, each caught: several requests sent at once, a waiting graph kept, graphs ahead of routes, a question asked twice, a failure answered as a success, a failed worker waited on, the panel left pending, and a type error in the worker (caught by worker:typecheck). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016jxMkwA2rbicdGxHosecYi
This commit is contained in:
@@ -1,9 +1,10 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
|
||||
import { jumpLinkSegments, routeBetween } from '../../shared/astro/jump-links';
|
||||
import { RoutingRequest, RoutingResponse } from '../../shared/astro/routing';
|
||||
import { StarNeighbourhood } from '../../shared/astro/star-neighbourhood';
|
||||
import { StarRecord } from '../../shared/models/star.model';
|
||||
import { RoutingClient } from './routing-client';
|
||||
import { RoutingClient, SupersededRequest } from './routing-client';
|
||||
|
||||
const STARS: StarRecord[] = Array.from({ length: 6 }, (_, i) => ({
|
||||
id: 100 + i,
|
||||
@@ -16,11 +17,51 @@ const STARS: StarRecord[] = Array.from({ length: 6 }, (_, i) => ({
|
||||
colorIndex: 0.6
|
||||
}));
|
||||
const POSITIONS = Float32Array.from(STARS.flatMap((star) => [star.x, star.y, star.z]));
|
||||
const index = new StarNeighbourhood(STARS);
|
||||
|
||||
/** Flushes settled promises and their handlers. */
|
||||
const flush = () => new Promise((resolve) => setTimeout(resolve, 0));
|
||||
|
||||
/** A worker that records what it is sent and answers only when told to. */
|
||||
class FakeWorker {
|
||||
readonly sent: Array<RoutingRequest | { kind: 'catalogue' }> = [];
|
||||
private readonly listeners: Record<string, Array<(event: { data?: unknown }) => void>> = {};
|
||||
terminated = false;
|
||||
|
||||
postMessage(message: RoutingRequest | { kind: 'catalogue' }): void {
|
||||
this.sent.push(message);
|
||||
}
|
||||
|
||||
addEventListener(type: string, listener: (event: { data?: unknown }) => void): void {
|
||||
(this.listeners[type] ??= []).push(listener);
|
||||
}
|
||||
|
||||
terminate(): void {
|
||||
this.terminated = true;
|
||||
}
|
||||
|
||||
/** The requests sent so far, catalogue aside. */
|
||||
get requests(): RoutingRequest[] {
|
||||
return this.sent.filter((message): message is RoutingRequest => message.kind !== 'catalogue');
|
||||
}
|
||||
|
||||
answer(response: RoutingResponse): void {
|
||||
for (const listener of this.listeners['message'] ?? []) listener({ data: response });
|
||||
}
|
||||
|
||||
fail(): void {
|
||||
for (const listener of this.listeners['error'] ?? []) listener({});
|
||||
}
|
||||
}
|
||||
|
||||
function clientWithFake(): { client: RoutingClient; worker: FakeWorker } {
|
||||
const worker = new FakeWorker();
|
||||
const client = new RoutingClient(STARS, POSITIONS, index, () => worker as unknown as Worker);
|
||||
return { client, worker };
|
||||
}
|
||||
|
||||
// The unit tests' DOM has no Worker, which is exactly the case the client answers in place.
|
||||
describe('RoutingClient without a worker', () => {
|
||||
const index = new StarNeighbourhood(STARS);
|
||||
|
||||
it('has no Worker to use here, so the in-place answers are what is being tested', () => {
|
||||
expect(typeof Worker).toBe('undefined');
|
||||
});
|
||||
@@ -49,3 +90,87 @@ describe('RoutingClient without a worker', () => {
|
||||
client.dispose();
|
||||
});
|
||||
});
|
||||
|
||||
describe('RoutingClient with a worker', () => {
|
||||
it('sends the catalogue first, then one request at a time', () => {
|
||||
const { client, worker } = clientWithFake();
|
||||
|
||||
void client.links(8);
|
||||
void client.links(3);
|
||||
|
||||
expect(worker.sent[0].kind).toBe('catalogue');
|
||||
expect(worker.requests).toHaveLength(1);
|
||||
client.dispose();
|
||||
});
|
||||
|
||||
// A graph at 8 pc is seconds of work the worker cannot drop once started. Every pause on the
|
||||
// range slider used to queue another, and a route asked for after them waited behind them all.
|
||||
it('replaces a waiting graph with the newer one before it is ever built, and sends a route ahead of it', async () => {
|
||||
const { client, worker } = clientWithFake();
|
||||
const first = client.links(5);
|
||||
const superseded = client.links(6).catch((error: unknown) => error);
|
||||
const latest = client.links(8);
|
||||
const route = client.route(100, 104, 1.5, 8);
|
||||
|
||||
const building = worker.requests[0];
|
||||
worker.answer({ kind: 'links', requestId: building.requestId, segments: new Float32Array(6) });
|
||||
await flush();
|
||||
|
||||
expect(await superseded).toBeInstanceOf(SupersededRequest);
|
||||
expect(worker.requests.map((request) => request.kind)).toEqual(['links', 'route']);
|
||||
await expect(first).resolves.toHaveLength(6);
|
||||
|
||||
const routeRequest = worker.requests[1];
|
||||
worker.answer({ kind: 'route', requestId: routeRequest.requestId, route: null, neededRangePc: 4 });
|
||||
await expect(route).resolves.toEqual({ route: null, neededRangePc: 4 });
|
||||
await flush();
|
||||
|
||||
expect(worker.requests.map((request) => (request.kind === 'links' ? request.rangePc : request.kind))).toEqual([5, 'route', 8]);
|
||||
const lastGraph = worker.requests[2];
|
||||
worker.answer({ kind: 'links', requestId: lastGraph.requestId, segments: new Float32Array(12) });
|
||||
await expect(latest).resolves.toHaveLength(12);
|
||||
client.dispose();
|
||||
});
|
||||
|
||||
it('shares the answer to a question already on its way rather than asking it twice', async () => {
|
||||
const { client, worker } = clientWithFake();
|
||||
const once = client.links(7.5);
|
||||
const again = client.links(7.5);
|
||||
|
||||
expect(worker.requests).toHaveLength(1);
|
||||
worker.answer({ kind: 'links', requestId: worker.requests[0].requestId, segments: new Float32Array(6) });
|
||||
|
||||
expect(await again).toBe(await once);
|
||||
expect(worker.requests).toHaveLength(1);
|
||||
client.dispose();
|
||||
});
|
||||
|
||||
it('rejects a request the worker failed on, and goes on to the next', async () => {
|
||||
const { client, worker } = clientWithFake();
|
||||
const failing = client.route(100, 104, 1.5, 8).catch((error: unknown) => error);
|
||||
const next = client.links(3);
|
||||
|
||||
worker.answer({ kind: 'failed', requestId: worker.requests[0].requestId, message: 'out of memory' });
|
||||
|
||||
expect(((await failing) as Error).message).toBe('out of memory');
|
||||
await flush();
|
||||
expect(worker.requests.map((request) => request.kind)).toEqual(['route', 'links']);
|
||||
worker.answer({ kind: 'links', requestId: worker.requests[1].requestId, segments: new Float32Array(0) });
|
||||
await expect(next).resolves.toHaveLength(0);
|
||||
client.dispose();
|
||||
});
|
||||
|
||||
it('answers in place what a worker that failed to load left outstanding, and everything after', async () => {
|
||||
const { client, worker } = clientWithFake();
|
||||
const route = client.route(100, 104, 1.5, 8);
|
||||
const graph = client.links(1.5);
|
||||
|
||||
worker.fail();
|
||||
|
||||
await expect(route).resolves.toEqual({ route: routeBetween(index, 100, 104, 1.5), neededRangePc: null });
|
||||
expect(Array.from(await graph)).toEqual(Array.from(jumpLinkSegments(index, 1.5)));
|
||||
await expect(client.route(100, 105, 1.5, 8)).resolves.toMatchObject({ route: null });
|
||||
expect(worker.terminated).toBe(true);
|
||||
client.dispose();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user