diff --git a/src/daemon/dispatcher.test.ts b/src/daemon/dispatcher.test.ts index 2f67a46f9..63b8ea411 100644 --- a/src/daemon/dispatcher.test.ts +++ b/src/daemon/dispatcher.test.ts @@ -93,22 +93,6 @@ vi.mock("node:worker_threads", async (importOriginal) => { const gibibyte = 1024 ** 3; const HOST_SYSTEM = { arch: "arm64", os: "macOS", osVersion: "15.5" }; -/** - * ADR 0003 §12: "full contract coverage at the dispatcher: one suite ... against a fake driver - * and a scripted session, covering parsing, role rejection, ownership, and error codes." This - * suite drives `Dispatcher` directly -- no socket, no `DaemonServer` -- against a real - * `createCore`/`createLeasing`/`Registry`/`CleanupReaper` backed by `FakeDriver`, the same harness style - * `server.test.ts` uses for its own socket-level tests. It deliberately does not re-walk every - * operation the way `server.test.ts` already does at the framing/connection level (ADR §12: - * "re-walking every operation through each transport would test nothing new") -- this suite is - * where the actual role/ownership/parsing logic gets proven once, not per transport. - */ -/** Picks `buildDispatcher`'s `passthrough` dependency -- a caller-supplied override, or the - * engine, exactly as every other `overrides.x ?? y` default in that function does. Pulled out - * on its own so adding this one override does not push `buildDispatcher` itself over fallow's - * complexity threshold: the branch is real either way, but it belongs to a one-line function - * that cannot get any more complicated, not to the 130-line one already carrying a dozen of - * these. */ function resolvePassthroughOverride( engine: TestEngine, override: PassthroughResolver | undefined, @@ -116,9 +100,6 @@ function resolvePassthroughOverride( return override ?? engine; } -/** What the stall test reads: a test's own claims or the engine's, and this driver after any - * other platforms' a test lists. Pulled out of `buildDispatcher` for the same reason as - * `resolvePassthroughOverride`. */ function stallOptions( engine: TestEngine, driver: FakeDriver, @@ -140,8 +121,6 @@ function resolveEventHistoryOverride( return override ?? ringEventHistory(eventBus); } -/** The fake driver's `simlock ` wrapper, when a test asks for one; nothing otherwise. Pulled - * out of `buildDispatcher` for the same reason as `resolvePassthroughOverride`. */ function fakePassthroughOptions( tool: string | undefined, contextSink: unknown[] | undefined, @@ -150,9 +129,6 @@ function fakePassthroughOptions( const options = { passthrough: (_tool: string, _device: unknown, args: readonly string[], context?: unknown) => { contextSink?.push(context); - // The refusal half of a real driver's passthrough, in the smallest form that - // proves `device.exec` inherits it: `driver.passthrough` and `device.exec` call - // this same function, so a verb refused for one is refused for the other. if (args.includes("delete")) { throw new PassthroughRefusedError( tool, @@ -191,7 +167,6 @@ function recordingResolver( return resolver as unknown as PassthroughResolver; } -/** A fake clock that can also be set back, as a wall clock can be. */ class SettableClock extends FakeClock { #offset = 0; @@ -204,12 +179,10 @@ class SettableClock extends FakeClock { } } -/** `config` with `gateway.label` set, or unchanged when there is no label. */ function withGatewayLabel(config: Config, label: string | undefined): Config { return label === undefined ? config : { ...config, gateway: { ...config.gateway, label } }; } -/** `config` with its `http` block replaced, or unchanged when there is none. */ function withHttp(config: Config, http: Config["http"] | undefined): Config { return http === undefined ? config : { ...config, http }; } @@ -265,41 +238,25 @@ async function buildDispatcher( readonly exec?: Partial; readonly passthroughContextSink?: unknown[]; readonly clock?: FakeClock; - /** Replaces the fake driver's own `passthrough` (which unconditionally prepends - * `--set /root`) with a caller-supplied resolver -- for the one suite that needs a - * `device.exec` command a *real* `NodeProcessRunner` can actually spawn (`node -e ...` - * takes no `--set`), rather than `ScriptedProcessRunner`'s scripted chunks. */ readonly passthroughOverride?: PassthroughResolver; readonly execEnv?: NodeJS.ProcessEnv; - /** Stands in for the event history, so `events.replay` can be checked against it. */ readonly eventHistory?: Pick; readonly historyPath?: string; readonly withoutTokens?: boolean; - /** `status.get`'s host block; a fixed machine with no tools by default. */ readonly hostFacts?: () => HostFacts; - /** Replaces the capacity block, for a test about one strategy's options. */ readonly capacity?: Config["capacity"]; - /** Changes keys of the `warmPool` block, for a test about its targets or its switch. */ readonly warmPool?: Partial; - /** Stands in for the component installer: a test that needs to see whether it was reached, - * or one that reads `status.get`'s installs from an installer of its own. */ readonly components?: Pick< ComponentInstaller, "claimProvision" | "inProgress" | "install" | "list" | "remove" >; - /** The daemon's health, as `status.get` reports it; `running` by default. */ readonly health?: "starting" | "running" | "failed"; - /** `gateway.label` in this daemon's config; unset by default. */ readonly gatewayLabel?: string; - /** The `http` block; disabled by default. */ readonly http?: Config["http"]; - /** Stands in for the engine's operation claims in the stall test, so a test can hold one. */ readonly claims?: { isClaimed(deviceId: string): boolean }; - /** Other platforms' drivers, listed before this one's in the stall test's driver list. */ readonly otherStallDrivers?: readonly FakeDriver[]; /** why: a second platform's driver for the leasing engine, for a test that leases on both. */ readonly extraDrivers?: readonly FakeDriver[]; - /** Extra leasing options, such as `defaultModes` (a platform's default device mode). */ readonly engine?: Pick[0], "defaultModes">; readonly physicalDevices?: readonly SeededPhysicalDevice[]; readonly presence?: (driverDeviceId: string) => LatestPresence | undefined; @@ -484,8 +441,6 @@ describe("Dispatcher: parsing", () => { async (field, value) => { const { dispatcher } = await buildDispatcher(); const valid = { model: "Pixel 8", osVersion: "34", platform: "android" }; - // The same request without the field is well-formed, so the refusal below is about the - // field and nothing else. expect(OPERATIONS["lease.request"].input.safeParse(valid).success).toBe(true); await expect( @@ -495,9 +450,6 @@ describe("Dispatcher: parsing", () => { ); it("rejects a ttlMs above lease.maxTtlMs on a request, rather than clamping it, and creates no lease", async () => { - // ADR 0004 §4: the cap is enforced here rather than in the contract schema, because - // `lease.maxTtlMs` is a daemon config value the contract module cannot see -- and every - // transport reaches leases through this one dispatcher, so HTTP inherits the same answer. const { dispatcher, registry } = await buildDispatcher({ lease: { maxTtlMs: 1_000 } }); await expect( dispatcher.dispatch( @@ -516,7 +468,6 @@ describe("Dispatcher: parsing", () => { { model: "iPhone 17 Pro", platform: "ios", ttlMs: 1_000 }, session(), ); - // Time has passed, so a renew that went through would have moved the deadline. clock.advance(10); await expect( @@ -568,18 +519,12 @@ describe("Dispatcher: parsing", () => { it("rejects an operation this dispatcher has no handler for with UNKNOWN_REQUEST", async () => { const { dispatcher } = await buildDispatcher(); - // "daemon.stop" is ADR §6's frozen exception -- `DaemonServer#dispatchLine` intercepts it - // before it ever reaches a `Dispatcher`, so this dispatcher genuinely has no handler for it. await expect( dispatcher.dispatch("daemon.stop", {}, session({ role: "admin" })), ).rejects.toMatchObject({ code: "UNKNOWN_REQUEST" }); }); }); -/** - * ADR 0012: a worker answers the fleet operations as a fleet of one. `worker.list` is this host's - * own view; the operations that act on a gateway's workers refuse with their own code. - */ describe("Dispatcher: the fleet operations on a worker", () => { const admin = session({ principal: "operator", role: "admin" }); @@ -755,8 +700,6 @@ describe("Dispatcher: the fleet operations on a worker", () => { tools: [{ build: "16F6", name: "xcode", platform: "ios", version: "16.4" }], }), }); - // One lease on one device, and one install held mid-download: every field has something - // in it, so a field the view dropped or took from the wrong read cannot pass as empty. await dispatcher.dispatch( "lease.request", { model: "iPhone 17 Pro", platform: "ios" }, @@ -826,8 +769,6 @@ describe("Dispatcher: the fleet operations on a worker", () => { operation, })), ); - // The refusal is the whole answer: nothing was installed on this host instead, and the - // message says how to install here. expect(install).not.toHaveBeenCalled(); expect(String((refusals[4] as { error?: unknown } | undefined)?.error)).toContain( "install on this host without naming workers", @@ -910,10 +851,6 @@ describe("Dispatcher: role rejection", () => { ).resolves.toEqual([]); }); - // Both halves of this shipped broken once: the handler forwarded `fix` alone, so - // `doctor --purge-orphans` prompted for confirmation and then destroyed nothing. The flag - // stays its own all the way down -- an unattended `--fix` must not acquire a destructive - // behaviour by upgrading (ADR 0001, decision 6) -- which is only true if it also *arrives*. it("doctor.run forwards purgeOrphans to the Doctor, distinctly from fix, and always asks for prerequisites", async () => { const { dispatcher, doctor } = await buildDispatcher(); const reconcile = vi.spyOn(doctor, "reconcile"); @@ -940,10 +877,6 @@ describe("Dispatcher: role rejection", () => { }); }); - // `driver.passthrough` was declared in the contract and dispatched by the socket switch while - // no handler existed for it, which the partial handler map hid from the compiler: every - // `simlock simctl` / `simlock adb` answered UNKNOWN_REQUEST. The map is total now, so this - // guards the routing rather than the registration. it("driver.passthrough resolves the scoped command through the driver that claims the tool", async () => { const { dispatcher } = await buildDispatcher({ passthroughTool: "simctl" }); @@ -992,13 +925,6 @@ describe("Dispatcher: ownership", () => { return (grant as { lease: { id: string } }).lease.id; } - // H3 (round 3 review): §27a's gate used to be `role !== "admin"`, which an HTTP `operator` - // bearer token satisfies just as well as the gateway's own uplink -- on a plain worker with no - // gateway anywhere, any operator token could name someone else as owner. `isGatewayUplink` is - // the narrower signal `DaemonServer#session` stamps only for a connection accepted through - // `acceptUplink` (this worker dialled its own configured `gateway.url`); a session merely - // carrying `role: "admin"` from anywhere else -- an operator token's HTTP session included -- - // no longer suffices. it.each([ ["a non-admin session", { principal: "tok_agent", role: "agent" }], [ @@ -1040,7 +966,6 @@ describe("Dispatcher: ownership", () => { ); expect((grant as { lease: { ownerId: string } }).lease.ownerId).toBe("agent-7"); - // The lease is now gated on the named owner, not the uplink session that requested it. await expect( dispatcher.dispatch( "lease.renew", @@ -1212,9 +1137,6 @@ describe("Dispatcher: ownership", () => { it("lease.cancel: naming a requesterId with no pending request always passes (not-found), admin bypasses too", async () => { const { dispatcher } = await buildDispatcher(); - // A requesterId nobody has a pending request under: not gated, since there is no owner to - // compare against -- `pendingRequestOwner` resolves `undefined`, and `not-found` is what - // surfaces (the same convention `ownsLease` uses for an unknown lease id). await expect( dispatcher.dispatch( "lease.cancel", @@ -1223,8 +1145,6 @@ describe("Dispatcher: ownership", () => { ), ).resolves.toMatchObject({ result: "not-found" }); - // An omitted requesterId always passes (defaults to the principal) -- see the operation's - // `authorize` hook doc in `operations.ts`. await expect( dispatcher.dispatch("lease.cancel", {}, session({ principal: "tok_agent", role: "agent" })), ).resolves.toMatchObject({ result: "not-found" }); @@ -1241,8 +1161,6 @@ describe("Dispatcher: ownership", () => { it("lease.cancel: gated on the pending request's recorded owner (ADR §4), not the requesterId -- so a proxy connection can cancel what it created", async () => { const { dispatcher, engine } = await buildDispatcher(); - // Fill iOS capacity (maxRunning: 2, see testConfig) so a third request queues instead of - // granting immediately -- cancellability requires a still-`queued` waiter. await dispatcher.dispatch( "lease.request", { @@ -1264,8 +1182,6 @@ describe("Dispatcher: ownership", () => { session({ principal: "tok_owner2" }), ); - // ADR §4's proxy case: one connection, principal "host", requesting under a different - // requesterId ("agent-7") than its own principal. const queuedRequest = dispatcher.dispatch( "lease.request", { @@ -1278,9 +1194,6 @@ describe("Dispatcher: ownership", () => { ); await expect.poll(() => engine.queueDepth).toBe(1); - // Naming "agent-7" from a session that is neither "host" (the recorded owner) nor admin is - // FORBIDDEN, even though "agent-7" is the pending request's own requesterId -- the ADR - // incoherence this fixes: `lease.cancel` is no longer gated on `requesterId === principal`. await expect( dispatcher.dispatch( "lease.cancel", @@ -1289,9 +1202,6 @@ describe("Dispatcher: ownership", () => { ), ).rejects.toMatchObject({ code: "FORBIDDEN" }); - // The owning principal ("host") cancels what it created, under the requesterId it created - // it with -- this is the case the old `requesterId === principal` comparison forbade - // outright. await expect( dispatcher.dispatch( "lease.cancel", @@ -1379,12 +1289,6 @@ describe("Dispatcher: ownership", () => { }); }); -// Review finding S9: `Dispatcher#leaseReleaseAll` and `#nukeRun` were exercised by no test -// anywhere (`nuke.run` appeared in `server.test.ts` only inside a comment) -- both are -// admin-only and destructive, which is exactly what `docs/internal/agent-rules/safety.md` rules 1 -// ("registry-only destruction") and 5 ("destructive CLI commands confirm or require --yes") are -// about. `--yes`/confirmation is a CLI-layer concern (not this dispatcher's), but role -// enforcement and "it actually destroys only what it should" are, and neither had coverage. describe("Dispatcher: lease.release-all", () => { it("rejects an agent session with FORBIDDEN, without releasing anything", async () => { const { dispatcher, registry } = await buildDispatcher(); @@ -1397,8 +1301,6 @@ describe("Dispatcher: lease.release-all", () => { await expect( dispatcher.dispatch("lease.release-all", {}, session({ role: "agent" })), ).rejects.toMatchObject({ code: "FORBIDDEN" }); - // The lease this agent session doesn't even own must still be untouched -- a rejected - // `lease.release-all` released nothing, it didn't just fail to report what it released. expect(registry.snapshot.leases).toHaveLength(1); }); @@ -1424,8 +1326,6 @@ describe("Dispatcher: lease.release-all", () => { expect(new Set(result.leaseIds)).toEqual(new Set([first.lease.id, second.lease.id])); expect(registry.snapshot.leases).toHaveLength(0); - // `killed`, not `explicit`: nobody's holder asked for this (docs/internal/EVENTS.md), and that is - // the distinction a `lease-lost` reader acts on. expect( eventBus .replay() @@ -1944,8 +1844,6 @@ describe("Dispatcher: device mode on every surface", () => { spec: { physical: false, model: "iPhone 17 Pro", osVersion: "26.5", platform: "ios" }, }); const stuck = await register("driver-stuck"); - // The fake driver estimates 0, so the threshold is the 60 s floor. The stuck device is past - // it; the fresh one, provisioning too, is not. clock.advance(60_001); const fresh = await register("driver-fresh"); @@ -1972,7 +1870,6 @@ describe("Dispatcher: device mode on every surface", () => { }); it("status leaves stalled off a device past its threshold that a live operation holds", async () => { - // The same rule as `doctor`: a claim says work is in progress, however long it takes. const claimed = new Set(); const { clock, dispatcher, registry } = await buildDispatcher({ claims: { isClaimed: (deviceId) => claimed.has(deviceId) }, @@ -1996,7 +1893,6 @@ describe("Dispatcher: device mode on every surface", () => { }); it("status measures a stall against the driver for the device's own platform", async () => { - // Android's estimate puts its threshold at 30 min; the iOS fake's 0 leaves the 60 s floor. const android = new FakeDriver({ clock: new FakeClock(1_000), estimateMs: { boot: 300_000, provision: 300_000 }, @@ -2035,19 +1931,8 @@ describe("Dispatcher: device mode on every surface", () => { }); describe("Dispatcher: #parseOutput", () => { - // ADR §1's central claim -- "the daemon maps its own records onto contract types in exactly - // one place ... this is what keeps private types out of the public package surface" -- names - // `#parseOutput` as the enforcement mechanism (see `schemas.ts`'s module doc). It had no test - // of its own failure path: every other dispatcher test exercises a handler whose real output - // already satisfies its schema, so `#parseOutput`'s `safeParse` always took the success - // branch. This forces the failure branch with a `catalog: CatalogReader` fake that returns a - // payload no real `CatalogReader` implementation would -- one that does not satisfy - // `platformCatalogSchema` -- and checks the dispatcher does not leak that malformed payload - // out, or the underlying zod issue detail, to the caller. it("fails closed with an opaque Internal error when a handler's output violates its contract schema, instead of leaking the malformed payload", async () => { const badCatalog: CatalogReader = { - // `platformCatalogSchema` requires (among other fields) `platform`/`models`/`runtimes` -- - // this satisfies none of them. listCatalog: async () => [{ notAValidCatalogEntry: true } as never], }; const { dispatcher } = await buildDispatcher({ catalog: badCatalog }); @@ -2090,7 +1975,6 @@ describe("Dispatcher: image tags", () => { }; it("grants a request naming an installed tag a device whose spec shows the tag, and one naming none a spec without it", async () => { - // One dispatcher each: the test config's capacity holds one device. const tagged = await ( await buildDispatcher({ driverOptions: androidImages }) ).dispatcher.dispatch( @@ -2184,10 +2068,6 @@ describe("Dispatcher: image tags", () => { }); describe("Dispatcher: the download policy clamp applies regardless of caller", () => { - // This is the exact behaviour ADR 0003 §2 says HTTP was missing ("the socket path applies - // `config.downloads.policy`, HTTP passes `allowDownload` through unclamped"). Since HTTP now - // calls this same `lease.request` handler (see `src/http/tracker.ts`), proving the clamp - // here proves it for both frontends -- there is exactly one implementation of it left. it("starts no install for allowDownload:true when downloads.policy is 'never'", async () => { const { dispatcher, driver } = await buildDispatcher({ downloadsPolicy: "never" }); await expect( @@ -2233,7 +2113,6 @@ describe("Dispatcher: the download policy clamp applies regardless of caller", ( // why: only what is on the machine is listed, however much of the rest a policy would download. expect(catalog.platforms[0]?.runtimes).toEqual(["26.5"]); expect(catalog.platforms[0]?.modelRuntimes).toEqual({ "iPhone 17 Pro": ["26.5"] }); - // The driver is asked with no policy, so no policy can add an image it would download. const listCalls = driver.calls.filter((call) => call.operation === "listCatalog"); expect(listCalls.map((call) => call.arguments)).toEqual([[]]); }, @@ -2262,8 +2141,6 @@ describe("Dispatcher: the download policy clamp applies regardless of caller", ( describe("Dispatcher: status.get installs in progress", () => { const installTimeoutMs = 60_000; - /** A dispatcher whose `status.get` reads a real installer over an iOS fake driver that holds - * every install until the test releases it. */ async function withInstaller() { const clock = new FakeClock(1_000); const driver = new FakeDriver({ availableOsVersions: [], clock, platform: "ios" }); @@ -2371,7 +2248,6 @@ describe("Dispatcher: list.get requests and status.get waiting", () => { platform: "ios", } as const; - /** A lease request from `requesterId`, left running: it resolves once granted. */ function request( dispatcher: Dispatcher, requesterId: string, @@ -2384,7 +2260,6 @@ describe("Dispatcher: list.get requests and status.get waiting", () => { ); } - /** Fills iOS's two devices (`testConfig`), so the next request queues. */ async function fillIos(dispatcher: Dispatcher): Promise { await request(dispatcher, "holder-1"); await request(dispatcher, "holder-2"); @@ -2457,7 +2332,6 @@ describe("Dispatcher: list.get requests and status.get waiting", () => { stage: "queued", }, ]); - // The idempotency key and the owner stay the requester's. expect(JSON.stringify(listed)).not.toContain("key-a"); expect(JSON.stringify(listed)).not.toContain('"host"'); }); @@ -2508,7 +2382,6 @@ describe("Dispatcher: list.get requests and status.get waiting", () => { const { dispatcher, engine } = await buildDispatcher({ driverOptions: { latencyMs: { provision: 60_000 } }, }); - // Both iOS devices are being made: the two requests start, the third queues behind them. void request(dispatcher, "agent-a"); void request(dispatcher, "agent-b"); void request(dispatcher, "agent-c"); @@ -2526,7 +2399,6 @@ describe("Dispatcher: list.get requests and status.get waiting", () => { }); }); -/** Lets every settled promise and queued continuation run. */ async function flushPromises(): Promise { for (let count = 0; count < 100; count += 1) await Promise.resolve(); } @@ -2535,7 +2407,6 @@ describe("Dispatcher: component.install", () => { const admin = (overrides: Partial = {}) => session({ principal: "tok_operator", role: "admin", ...overrides }); - /** An installer stand-in that records every call and installs nothing. */ function spyInstaller() { const install = vi.fn(() => Promise.resolve({ outcome: "installed" as const, version: "unreachable" }), @@ -2555,7 +2426,6 @@ describe("Dispatcher: component.install", () => { .map((call) => call.arguments); } - /** Lets the installer and the fake driver run their awaits until `predicate` holds. */ async function until(predicate: () => boolean, label: string): Promise { for (let attempt = 0; attempt < 200; attempt += 1) { if (predicate()) return; @@ -2636,7 +2506,6 @@ describe("Dispatcher: component.install", () => { const { components, dispatcher, driver } = await buildDispatcher({ driverOptions: { knownModels: ["iPhone 17 Pro"] }, }); - // The engine and the dispatcher hold this same installer, so the spy sees both callers. const installerCalls = vi.spyOn(components, "install"); driver.holdInstalls(); @@ -2656,7 +2525,6 @@ describe("Dispatcher: component.install", () => { }, session(), ); - // The lease reaches the installer while the install is still held, so it can only join. await until(() => installerCalls.mock.calls.length === 2, "the lease reached the installer"); expect(installerCalls.mock.calls.map(([request]) => request.component)).toEqual([ "27.0", @@ -2775,9 +2643,7 @@ describe("Dispatcher: component.install", () => { ); expect(progress).toEqual([ - // The install's start, before the driver reports anything. { stage: "downloading" }, - // The driver's NaN, with its fraction left out. { stage: "downloading" }, { fraction: 0, stage: "downloading" }, { fraction: 0.5, stage: "downloading" }, @@ -2785,7 +2651,6 @@ describe("Dispatcher: component.install", () => { ]); }); - /** The fractions one install's `component-progress` pushes carry, the driver reporting `percents`. */ async function fractionsFor(percents: readonly number[]): Promise { const { dispatcher } = await buildDispatcher({ driverOptions: { installProgress: percents } }); const fractions: unknown[] = []; @@ -2802,9 +2667,9 @@ describe("Dispatcher: component.install", () => { } it.each([ - // 1.1 / 100 is 0.011000000000000001 in floating point. + // why: 1.1 / 100 is 0.011000000000000001 in floating point. { expected: [0.011, 1], percents: [1.1] }, - // 64.1 / 100 is 0.6409999999999999 in floating point. + // why: 64.1 / 100 is 0.6409999999999999 in floating point. { expected: [0.641, 1], percents: [64.1] }, { expected: [0.999, 1], percents: [99.96, 100] }, ])( @@ -2815,8 +2680,6 @@ describe("Dispatcher: component.install", () => { ); it("sends no fraction with more than three decimals for any percentage from 0 to 100 in steps of 0.1", async () => { - // Summed rather than computed, so the percentages carry the noise a driver's own arithmetic - // would. const percents: number[] = []; for (let percent = 0, step = 0; step <= 1000; step += 1, percent += 0.1) percents.push(percent); @@ -2856,7 +2719,6 @@ describe("Dispatcher: component.install", () => { { stage: "waiting" }, { stage: "downloading" }, { fraction: 0.41, stage: "downloading" }, - // The installer's own report that the install it ran is complete. { fraction: 1, stage: "downloading" }, ]); }); @@ -3045,7 +2907,6 @@ describe("Dispatcher: status.get RAM budget", () => { .map((device) => device.mode === "slim" ? sizes.iosSlimBytesPerDevice : sizes.iosBytesPerDevice, ); - // The provisioning device is listed `full` until its slim pass, and counts at that size. expect(status.devices?.map((device) => device.mode).sort()).toEqual(["full", "full", "slim"]); expect(status.capacity?.ramBudget).toEqual({ limitBytes: 28 * gibibyte, @@ -3056,7 +2917,6 @@ describe("Dispatcher: status.get RAM budget", () => { }); describe("atRamBudget", () => { - // 32 GiB of RAM leaves a 28 GiB budget. A full iOS device takes 2 GiB, an Android one 4. async function withIosDevices(count: number) { const built = await buildDispatcher({ capacity: { @@ -3088,7 +2948,6 @@ describe("Dispatcher: status.get RAM budget", () => { } as const; it("is true for a platform whose next full device the planner refuses for RAM, and false while it would be admitted", async () => { - // 13 devices use 26 of 28 GiB: one more iOS device fits, an Android one does not. const roomy = await withIosDevices(13); const roomyStatus = await roomy.dispatcher.dispatch("status.get", {}, session()); expect(roomyStatus.capacity?.ios.atRamBudget).toBe(false); @@ -3097,7 +2956,6 @@ describe("Dispatcher: status.get RAM budget", () => { roomy.dispatcher.dispatch("lease.request", { ...ios, noWait: true }, session()), ).resolves.toBeDefined(); - // 14 devices use all 28 GiB: not even one more iOS device fits. const full = await withIosDevices(14); const fullStatus = await full.dispatcher.dispatch("status.get", {}, session()); expect(fullStatus.capacity?.ios.atRamBudget).toBe(true); @@ -3283,12 +3141,6 @@ async function flush(): Promise { } } -/** - * ADR 0005 §19a-§19e. The operation's whole job is to run, on this machine, exactly what - * `driver.passthrough` would have handed a caller who was already on it -- so these check the - * two things that are genuinely new (the process and its output) and the three the operation - * inherits and must not lose (root scoping, the refusal list, ownership). - */ describe("Dispatcher: device.exec", () => { const command = { args: ["--set", "/root", "list", "devices"], command: "simctl" }; @@ -3324,9 +3176,6 @@ describe("Dispatcher: device.exec", () => { ), ).resolves.toEqual({ exitCode: 3 }); - // The scoping the driver injected is on the command, and its environment is layered over - // the daemon's own rather than replacing it -- a child given only `SIMLOCK_SCOPED` would - // have no PATH to find `simctl` with. expect(runner.calls[0]).toMatchObject({ args: ["--set", "/root", "list", "devices"], command: "simctl", @@ -3359,8 +3208,6 @@ describe("Dispatcher: device.exec", () => { }), ); - // Order across both streams, and each chunk exactly as written: a handler that buffered or - // line-joined would show two stdout chunks merged, or stderr after both of them. expect(seen).toEqual(["stdout:first", "stderr:warning\n", "stdout: and second\n"]); }); @@ -3423,22 +3270,13 @@ describe("Dispatcher: device.exec", () => { await flush(); clock.advance(1_000); - // EXEC_TIMEOUT, not the exit code the kill produced: "we stopped it" and "it failed" are - // different facts, and only the first tells a caller to raise the limit (ADR §19e). await expect(pending).rejects.toMatchObject({ code: "EXEC_TIMEOUT" }); }); - /** - * The four combinations of the operation's own hook (ADR 0005 §19b/§27), which is - * `ownsLease` for an agent and something stricter for an admin. The lease under test was - * granted to principal `tok_agent`, so its `ownerId` and its `requesterId` are both that. - */ it("gates an agent session on the lease it owns, and refuses any requesterId it sends outright (round 4, F4)", async () => { const runner = new ScriptedProcessRunner([{ match: command }]); const { dispatcher, leaseId } = await withLease({ processRunner: runner }); - // (1) Someone else's lease: refused -- and naming its requester does not help, because a - // non-admin session may not name a `requesterId` at all, regardless of whose it is. await expect( dispatcher.dispatch( "device.exec", @@ -3455,11 +3293,6 @@ describe("Dispatcher: device.exec", () => { ).rejects.toMatchObject({ code: "FORBIDDEN" }); expect(runner.calls).toEqual([]); - // (2) Its own lease, but naming a `requesterId` -- refused outright, whether or not the - // name it chose happens to be its own. Round 4, F4: this used to be read and silently - // ignored (a rule that lived only in the HTTP route, `src/http/app.ts`, and so answered - // differently over the unix socket); a non-admin session naming an identity at all is now - // `FORBIDDEN` here, uniformly, on every transport. await expect( dispatcher.dispatch( "device.exec", @@ -3476,8 +3309,6 @@ describe("Dispatcher: device.exec", () => { ).rejects.toMatchObject({ code: "FORBIDDEN" }); expect(runner.calls).toEqual([]); - // (2') Its own lease, no requesterId at all: allowed -- this is the ordinary case an agent - // actually uses. await expect( dispatcher.dispatch( "device.exec", @@ -3488,15 +3319,10 @@ describe("Dispatcher: device.exec", () => { }); it("holds an admin session to the lease's requester instead of letting it bypass", async () => { - // This is the check a gateway's own uplink session runs against: it holds one admin - // session and proxies many agents through it, so "admin bypasses" would let any - // admin-role connection drive every lease on the worker (ADR 0005 §19b). const runner = new ScriptedProcessRunner([{ match: command }]); const { dispatcher, leaseId } = await withLease({ processRunner: runner }); const admin = session({ principal: "gw:instance-1", role: "admin" }); - // (3) No `requesterId`, so it defaults to the admin's own principal -- which is not who - // the lease was granted to. await expect( dispatcher.dispatch( "device.exec", @@ -3513,7 +3339,6 @@ describe("Dispatcher: device.exec", () => { ).rejects.toMatchObject({ code: "FORBIDDEN" }); expect(runner.calls).toEqual([]); - // (4) Naming the agent it is proxying for: allowed. await expect( dispatcher.dispatch( "device.exec", @@ -3681,9 +3506,6 @@ describe("Dispatcher: device.exec", () => { }); it("tells the driver there is no terminal, so it can refuse what needs one", async () => { - // ADR 0005 §19c: the command runs here, on pipes. The driver is the only thing that knows - // which of its own commands that rules out (a bare `adb shell`), so the fact travels to - // it rather than being decided here. const seen: unknown[] = []; const { dispatcher, leaseId } = await withLease({ passthroughContextSink: seen, @@ -3705,8 +3527,6 @@ describe("Dispatcher: device.exec", () => { }); it("escalates to SIGKILL when a timed-out command ignores SIGTERM", async () => { - // SIGTERM is a request. A tool that ignores it (or is itself stuck) must not be able to - // hold the operation -- and the caller's connection -- open past the grace window. const runner = new ScriptedProcessRunner([ { hangs: true, ignoresSigterm: true, match: command }, ]); @@ -3724,8 +3544,6 @@ describe("Dispatcher: device.exec", () => { let settled = false; void pending.catch(() => (settled = true)); - // The timeout fires and SIGTERM lands; the command ignores it and the operation is still - // in flight, which is the state the escalation exists for. clock.advance(1_000); await flush(); expect(settled).toBe(false); @@ -3737,7 +3555,7 @@ describe("Dispatcher: device.exec", () => { }); it("resolves with the exit code, not EXEC_TIMEOUT, when the command finishes before the timeout timer's callback runs -- even scheduled in the same synchronous burst", async () => { - // `Promise.race([waited, expired])` -- not a flag either callback sets -- is what this + // why: `Promise.race([waited, expired])` -- not a flag either callback sets -- is what this // proves the value of: `handle.finish(7)` and `clock.advance(1_000)` are both called here // synchronously, one right after the other, with nothing in between observing either // settle. A flag-based implementation (`let timedOut = false; timer sets it`) would be at @@ -3760,18 +3578,15 @@ describe("Dispatcher: device.exec", () => { ); await flush(); - // The exit settles first, in program order; the timer's callback (which would otherwise - // report EXEC_TIMEOUT) runs only afterward, in the same synchronous burst. handle.finish(7); clock.advance(1_000); await expect(pending).resolves.toEqual({ exitCode: 7 }); - // And nothing was signalled: a command that finished is not one to kill. expect(handle.signals).toEqual([]); }); describe("output delivered across a real exit, against a real NodeProcessRunner", () => { - // `ScriptedProcessRunner` can't reproduce this class of defect: its scripted handle always + // why: `ScriptedProcessRunner` can't reproduce this class of defect: its scripted handle always // awaits a chunk's delivery before it will settle at all, so it cannot model "the child has // already exited and closed its pipe while a chunk's delivery to a slow consumer is still // outstanding" -- exactly the state a real OS pipe can be in (verified against real Node: @@ -3847,7 +3662,7 @@ describe("Dispatcher: device.exec", () => { session({ onOutput: () => new Promise(() => { - // A consumer whose backpressure never clears -- a dead SSE write, a socket that + // why: A consumer whose backpressure never clears -- a dead SSE write, a socket that // never drains. }), principal: "tok_agent", @@ -3858,11 +3673,6 @@ describe("Dispatcher: device.exec", () => { }, 30_000); it("reports EXEC_TIMEOUT, not the stalled-delivery error it provokes, when a consumer that stopped reading is why the command outran its timeout", async () => { - // The common path, not an exotic one: a consumer that stops reading is *why* a - // streaming command outruns its timeout in the first place -- the backpressure pause - // that stops the child finishing is the same pause that stalls this chunk's delivery. - // `exec.timeoutMs` firing and killing the child must still be the answer the caller - // sees, not the stalled-delivery rejection that killing it provokes as a side effect. const { clock, dispatcher, leaseId } = await withLease({ exec: { timeoutMs: 50 }, passthroughOverride: realCommand("process.stdout.write('x'); setInterval(() => {}, 1000);"), @@ -3875,7 +3685,7 @@ describe("Dispatcher: device.exec", () => { session({ onOutput: () => new Promise(() => { - // Never resolves: the stream is paused on this chunk's delivery for the rest + // why: Never resolves: the stream is paused on this chunk's delivery for the rest // of the test, exactly as a client that opened the stream and stopped reading // would leave it. }), @@ -3883,8 +3693,6 @@ describe("Dispatcher: device.exec", () => { }), ); - // Let the real child actually spawn and write its one chunk before the timeout fires, - // so the pending delivery this test depends on genuinely exists. await new Promise((resolve) => setTimeout(resolve, 200)); clock.advance(50); @@ -3893,8 +3701,6 @@ describe("Dispatcher: device.exec", () => { }); }); -/** A streamed child that settles only when a test says so, and records what it was signalled - * with -- enough to tell "we killed it" from "it finished" without a real process. */ class SettleOnCueHandle { readonly pid = 99; readonly signals: string[] = []; @@ -4300,7 +4106,6 @@ describe("Dispatcher: component.remove", () => { .finally(() => { leaseSettled = true; }); - // Rejected while the removal is still running, not queued until it ends. await vi.waitFor(() => expect(leaseSettled).toBe(true)); expect(classifyError(leaseError)).toBe("RUNTIME_MISSING"); diff --git a/src/daemon/server.test.ts b/src/daemon/server.test.ts index 4361b0948..f11232f52 100644 --- a/src/daemon/server.test.ts +++ b/src/daemon/server.test.ts @@ -113,9 +113,6 @@ afterEach(async () => { describe("DaemonServer", () => { it("keeps a lease when its client connection closes, and ends it at its own deadline", async () => { - // ADR 0004 §3: connection close means nothing to a lease. The daemon keeps no - // per-connection lease state and releases nothing on close, on any transport -- the TTL - // is the only thing that ends a lease nobody released. const harness = await createHarness({ lease: { defaultTtlMs: 40 } }); const holder = await createClient(harness.socketPath); @@ -137,12 +134,10 @@ describe("DaemonServer", () => { const observer = await createClient(harness.socketPath); await hello(observer); - // Still granted, still leasing its device, with nothing renewing it. await expect(observer.request("status.get", {})).resolves.toMatchObject({ payload: { devices: [{ state: "leased" }], leases: [{ id: leaseId }] }, }); - // The deadline is what ends it. harness.clock.advance(40); await expect.poll(() => harness.registry.snapshot.leases).toHaveLength(0); await expect.poll(() => harness.registry.snapshot.devices[0]?.state).toBe("ready"); @@ -185,12 +180,6 @@ describe("DaemonServer", () => { }); await missingHello.close(); - // ADR 0003 §6: protocol versions are now negotiated as ranges. A bare `protocolVersion` - // with no overlap against the daemon's range is `PROTOCOL_VERSION_UNSUPPORTED`, carrying - // both ranges and the daemon version -- there is no more exact-match - // `PROTOCOL_VERSION_MISMATCH` on this daemon (a real protocol-2 daemon out in the world - // still answers with the old code; see `simlock-client/client.test.ts`'s legacy-daemon test - // for how a client maps that). const wrongVersion = await createClient(harness.socketPath); await expect( wrongVersion.request("hello", { @@ -254,7 +243,7 @@ describe("DaemonServer", () => { ); it("grants the queued client when the holder's lease expires, not when it disconnects", async () => { - // A log sink rather than a sleep: "the waiter stays queued" is a negative assertion, and + // why: A log sink rather than a sleep: "the waiter stays queued" is a negative assertion, and // the only honest way to make one is to wait for the daemon to have actually processed // the close first. `Connection closed` is that receipt. const sink = new MemoryLogSink(); @@ -283,14 +272,12 @@ describe("DaemonServer", () => { .poll(() => harness.eventBus.replay().some((event) => event.event === "lease.queued")) .toBe(true); - // The holder's socket dying frees nothing (ADR 0004 §3) -- the waiter stays queued. await holder.close(); await expect .poll(() => sink.records.some((record) => record.message === "Connection closed")) .toBe(true); expect(harness.registry.snapshot.leases.map((lease) => lease.requesterId)).toEqual(["holder"]); - // Its deadline does free it, and the queue is served from there. harness.clock.advance(40); await expect(queuedGrant).resolves.toMatchObject({ ok: true }); await expect @@ -316,8 +303,6 @@ describe("DaemonServer", () => { osVersion: "26.5", platform: "ios", }); - // ADR 0003 §8: `progress` now carries the originating request's frame id alongside the - // progress payload, under `progress`, rather than the bare stage object at the top level. await expect(waiter.nextFrame((frame) => frame.push === "progress")).resolves.toMatchObject({ payload: { progress: { queuePosition: 1, stage: "queued" } }, push: "progress", @@ -333,7 +318,6 @@ describe("DaemonServer", () => { ?.progress?.stage === "queued", ), ).toEqual([]); - // Closing the holder frees nothing (ADR 0004 §3); releasing does. await holder.request("lease.release", { leaseId: leaseIdOf(holderGrant) }); await expect(queuedGrant).resolves.toMatchObject({ ok: true }); await holder.close(); @@ -571,9 +555,6 @@ describe("DaemonServer", () => { }, ); - // A well-formed request for a wrapper no driver claims is not a malformed one: it gets its - // own ERROR_TABLE row so the socket and HTTP transports name the condition identically - // (ADR 0003 §7), and so the CLI can tell "you typed it wrong" from "no driver serves this". it("distinguishes an unknown passthrough tool from a malformed payload", async () => { const harness = await createHarness(); const client = await createClient(harness.socketPath); @@ -587,11 +568,6 @@ describe("DaemonServer", () => { }); }); - /** - * ADR 0005 §19a over the socket. The dispatcher's own suite proves what the command does; - * this proves the one thing only this transport can: the chunks reach the wire as `output` - * pushes keyed by the request's frame id, ahead of the reply that carries the exit code. - */ it("pushes device.exec output frames keyed by the request id, then replies with the exit code", async () => { const clock = new FakeClock(1_000); const driver = passthroughFake({ @@ -642,7 +618,7 @@ describe("DaemonServer", () => { }, ]); - // ADR 0005 §19a: `started` reaches the socket exactly once, keyed by the same frame id. + // why: ADR 0005 §19a: `started` reaches the socket exactly once, keyed by the same frame id. // Round 5 review: this send site could be deleted with the entire suite green, because // nothing asserted the frame ever reached a socket at all. // @@ -657,10 +633,6 @@ describe("DaemonServer", () => { ]); }); - /** - * ADR 0010 §6 over the socket: an install's progress reaches the wire as `component-progress` - * pushes keyed by the request's frame id, ahead of the reply that carries the outcome. - */ it("pushes component.install progress frames keyed by the request id, then replies with the outcome", async () => { const clock = new FakeClock(1_000); const driver = new FakeDriver({ @@ -778,10 +750,6 @@ describe("DaemonServer", () => { }); it("stops pushing when the connection dies, and lets the command finish anyway", async () => { - // ADR 0004 §3's reasoning applied to a process: a dropped connection is not a reason to - // kill a half-applied `simctl install`. What stops is the pushing -- writing to a dead - // socket is the transport's problem to not have, and the operation itself carries on to - // its own end (or to `exec.timeoutMs`). const clock = new FakeClock(1_000); const driver = passthroughDriver(clock); const runner = new ControllableStreamingRunner(); @@ -809,15 +777,11 @@ describe("DaemonServer", () => { await client.close(); await flush(); - // Chunks written after the socket is gone reach no one and throw nothing, and the command - // still runs to its own end: the daemon is not left with a half-run command, and nothing - // about a dead socket reaches the operation. runner.handle?.emit("stdout", "after-the-drop"); runner.handle?.finish(0); await flush(); expect(runner.handle?.settled).toBe(true); - // Still serving: writing to a socket that went away mid-command took nothing down with it. const survivor = await createClient(harness.socketPath); await hello(survivor); await expect(survivor.request("status.get", {})).resolves.toMatchObject({ ok: true }); @@ -825,7 +789,7 @@ describe("DaemonServer", () => { }); it("keeps #pushOutput's promise unresolved while a frame is still going out, resolving it once the write drains", async () => { - // ADR 0005 §19e on this transport: `#pushOutput`'s returned promise is what + // why: ADR 0005 §19e on this transport: `#pushOutput`'s returned promise is what // `device.exec` hands the process runner as a chunk's delivery, and it is that promise -- // not anything this test can observe about a real stream -- staying unresolved while a // client's socket has not drained that would eventually stop the command at its pipe. @@ -864,7 +828,6 @@ describe("DaemonServer", () => { ); await waitFor(() => runner.handle !== undefined); - // The client stops draining: the next frame's write never completes. connection.block(); let firstDelivered = false; void Promise.resolve(runner.handle?.emit("stdout", "first")).then(() => { @@ -900,8 +863,6 @@ describe("DaemonServer", () => { await expect(second.daemon.start()).rejects.toMatchObject({ name: "DaemonAlreadyRunningError", }); - // The claim (and its DaemonAlreadyRunningError) happens before convergence: - // a lost startup race must not run device work at all. expect(convergeCalls).toBe(0); await second.daemon.stop("failed-start"); const client = await createClient(socketPath); @@ -928,15 +889,10 @@ describe("DaemonServer", () => { const secrets = new CryptoTokenSecrets(); const winnerFilesystem = new MemoryFilesystem(); - // Not bound to a name: `createHarness` already registers its daemon in `runningDaemons` - // for `afterEach` to stop, and this test only needs the winner alive on `socketPath` (so - // the loser's own `start()` below actually loses) plus its filesystem. await createHarness({ adminSecret: (await adminAuth(winnerFilesystem, secrets)).adminSecret, socketPath, }); - // The winner (the only one whose `start()` actually reached `host.start()`'s success path) - // did persist its secret. await expect(winnerFilesystem.readFile("/admin.token")).resolves.toContain("\n"); const loserFilesystem = new MemoryFilesystem(); @@ -948,10 +904,6 @@ describe("DaemonServer", () => { await expect(second.daemon.start()).rejects.toMatchObject({ name: "DaemonAlreadyRunningError", }); - // The claim under test: the loser's own filesystem was never written to, because its - // `start()` threw before reaching `adminSecret.persist()`. `startDaemon()` (`main.ts`) - // never calls `stop()` on a `daemon.start()` rejection like this one either, so this - // asserts the state exactly as production leaves it, before any test-hygiene cleanup below. await expect(loserFilesystem.readFile("/admin.token")).rejects.toThrow(); await second.daemon.stop("failed-start"); @@ -1034,8 +986,6 @@ describe("DaemonServer", () => { await expect(client.request("daemon.stop", {})).resolves.toMatchObject({ ok: true }); - // ADR 0004 §3: a stop ends connections, not leases -- they persist, and the next daemon - // judges each by its device (ADR 0019) and restores the timers of those it keeps. expect(harness.registry.snapshot.leases).toMatchObject([{ id: leaseId }]); expect(harness.state.database.prepare("SELECT id FROM leases").all()).toEqual([ { id: leaseId }, @@ -1066,8 +1016,6 @@ describe("DaemonServer", () => { }); await expect.poll(() => harness.registry.snapshot.leases).toEqual([]); - // The connection no longer believes it holds the expired lease, so closing it - // (which releases any still-held leases) must not error or hang the daemon. await holder.close(); const observer = await createClient(harness.socketPath); await hello(observer); @@ -1096,9 +1044,6 @@ describe("DaemonServer", () => { push: "lease-lost", }); - // Acceptance criterion: the (now stale) holder's own release attempt is rejected - // by the daemon rather than causing a transport error; the MCP layer maps this - // local-ownership loss to LEASE_NOT_OWNED without even asking the daemon. await expect(holder.request("lease.release", { leaseId })).resolves.toMatchObject({ error: { code: "UNKNOWN_LEASE" }, ok: false, @@ -1122,8 +1067,6 @@ describe("DaemonServer", () => { await expect(holder.request("lease.release", { leaseId })).resolves.toMatchObject({ ok: true, }); - // A subsequent request on the same connection lets us assert, by frame order, that - // no lease-lost push arrived for the release this connection asked for itself. await expect(holder.request("status.get", {})).resolves.toMatchObject({ ok: true }); expect(holder.frames().filter((frame) => frame.push === "lease-lost")).toEqual([]); await holder.close(); @@ -1234,8 +1177,6 @@ describe("DaemonServer", () => { push: "device-unhealthy", }); - // The lease is untouched by the notice: it is a fact about the device, not about the - // lease, and it is still there to be released the normal way. expect(harness.registry.snapshot.leases).toMatchObject([{ id: leaseId }]); await holder.request("lease.release", { leaseId }); await expect.poll(() => harness.registry.snapshot.leases).toEqual([]); @@ -1275,7 +1216,6 @@ describe("DaemonServer", () => { push: "device-recovered", }); - // The lease survived the crash and the recovery both, and ends the normal way. expect(harness.registry.snapshot.leases).toMatchObject([{ id: leaseId }]); await holder.request("lease.release", { leaseId }); await expect.poll(() => harness.registry.snapshot.leases).toEqual([]); @@ -1297,10 +1237,6 @@ describe("DaemonServer", () => { (lease) => lease.id === leaseId, )?.deviceId; - // Simulates the health monitor giving up on recovery and releasing the lease as - // `device-lost` -- proves the *existing* `lease.released` subscription already - // reaches the holder with the reason verbatim, so the failure path needs no new - // push of its own. harness.eventBus.emit( "lease.released", { deviceId: deviceId as string, leaseId, ownerId: "test-process", reason: "device-lost" }, @@ -1432,10 +1368,6 @@ describe("DaemonServer startup readiness", () => { await hello(client); const parked = client.request("list.get", { kind: "devices" }); - // Requests on one connection are dispatched in arrival order without waiting for - // the previous one to finish (`#read` fires `#dispatchLine` per line without - // awaiting), so once this status.get round-trips, the parked request's dispatch - // has already reached (and registered on) the readiness gate. await expect(client.request("status.get", {})).resolves.toMatchObject({ ok: true }); converge.resolve(); @@ -1453,13 +1385,6 @@ describe("DaemonServer startup readiness", () => { const client = await createClientRetrying(harness.socketPath); await hello(client); const parked = client.request("list.get", { kind: "devices" }); - // Round-tripping status.get first proves this request's dispatch has already - // started (and so is already tracked in `#parkedDispatches`) before the failure - // below -- this is not a race the assertion happens to win: `start()`'s catch - // path explicitly drains `#parkedDispatches` (awaiting each dispatch's own - // response write) before it closes any socket, so a request tracked at this - // point is genuinely guaranteed a DAEMON_STARTUP_FAILED response, not merely - // likely to get one before the connection drops. await expect(client.request("status.get", {})).resolves.toMatchObject({ ok: true }); converge.reject(new Error("boom")); @@ -1472,12 +1397,6 @@ describe("DaemonServer startup readiness", () => { }); it("does not arm live machinery when a stop completes while convergence is still running", async () => { - // `daemon.stop` is accepted during startup (see `#dispatchLine`), and an auxiliary - // frontend that fails to bind asks for a stop of its own -- so a stop can run to - // completion before convergence resolves. `start()` must then bail rather than - // subscribe to a disposed bus, schedule a heartbeat tick on a dead daemon, and emit - // `daemon.started` after `daemon.stopping` (a fact untrue when emitted, which - // `docs/internal/agent-rules/events.md` rule 3 forbids). const converge = deferred(); const harness = await createHarness({ converge: () => converge.promise, start: false }); const emitted: string[] = []; @@ -1552,14 +1471,6 @@ describe("DaemonServer startup readiness", () => { }); it("has lease-lost subscriptions wired before a lease.request parked on convergence proceeds", async () => { - // Guards the invariant documented in `start()`: subscriptions are set up only - // after convergence resolves, and a parked request must never observe a window - // where its own grant can be silently force-released without a `lease-lost` - // push. Grants the held lease while convergence is still pending, then -- - // immediately once convergence resolves and the grant comes back -- force - // releases that very lease from a second connection and asserts the push - // arrives, proving the subscription was already active by the time the grant - // reached the client. const converge = deferred(); const harness = await createHarness({ converge: () => converge.promise, start: false }); const startPromise = harness.daemon.start(); @@ -1589,11 +1500,6 @@ describe("DaemonServer startup readiness", () => { }); }); -/** - * `daemon.start()` claims the socket well before its own promise settles (that promise - * also waits on convergence), so a test that connects while convergence is deliberately - * held open must poll for the listener rather than assume it exists synchronously. - */ describe("DaemonServer stored lease requests", () => { const iPhone = { model: "iPhone 16", @@ -1601,7 +1507,6 @@ describe("DaemonServer stored lease requests", () => { platform: "ios", } as const; - /** `holder` takes the one device; `waiter` queues behind it under `key-1`. */ async function queueBehindHolder(harness: Awaited>) { const holder = await createClient(harness.socketPath); const waiter = await createClient(harness.socketPath); @@ -1698,7 +1603,6 @@ describe("DaemonServer stored lease requests", () => { error: { code: "NO_CAPACITY" }, }); - // Capacity frees up: a fresh evaluation would now grant the device. await holder.request("lease.release", { leaseId: held.payload.lease.id }); await expect .poll(() => harness.registry.snapshot.devices.map((device) => device.state)) @@ -1914,8 +1818,6 @@ describe("DaemonServer driver rejections", () => { payload: { platform: "ios", reason: "wrong-instance", root: "/Devices" }, }), ]); - // The daemon did come up -- the refusal costs one platform, not the process -- and - // the buffer says so in that order. const started = events.find((event) => event.event === "daemon.started"); expect(started?.seq).toBeLessThan(rejected[0]?.seq ?? 0); }); @@ -1941,8 +1843,6 @@ describe("DaemonServer driver rejections", () => { platform: "android", }); - // Safety rule 9's other half: the platform's driver does not start *and Simlock - // reports why*. A bare `NO_DRIVER` reads exactly like "this host has no Android SDK". expect(response.error?.code).toBe("NO_DRIVER"); expect(response.error?.message).toContain("port 5038 is held by an adb server we do not own"); }); @@ -1968,8 +1868,6 @@ describe("DaemonServer driver rejections", () => { platform: "android", }); - // The refusal on file is another platform's; attaching it here would blame iOS for - // Android's absence. expect(response.error?.code).toBe("NO_DRIVER"); expect(response.error?.message).toBe("No driver registered for platform: android"); }); @@ -1998,9 +1896,6 @@ describe("DaemonServer driver rejections", () => { tool: "adb", }); - // Unexplained, "No driver provides a adb passthrough" reads as "this host has no - // Android SDK" and sends the operator off to install one, while the summary naming the - // port conflict sits unread in `driverRejections` (safety rule 9). expect(response.error?.code).toBe("UNKNOWN_PASSTHROUGH_TOOL"); expect(response.error?.message).toContain("port 5038 is held by an adb server we do not own"); }); @@ -2063,9 +1958,7 @@ describe("DaemonServer driver rejections", () => { it("refuses at compile time to pair an event with another event's payload", () => { const rejection: DriverRejection = { event: "driver.adb-server-rejected", - // @ts-expect-error -- `port` is a number on the wire (docs/internal/EVENTS.md). The check has - // to happen where a refusal is written, because the daemon forwards `payload` to the - // ring buffer -- and to `simlock events --json` -- without ever reading it. + // @ts-expect-error -- `port` is a number on the wire (docs/internal/EVENTS.md). payload: { port: "5038", reason: "occupied" }, platform: "android", reason: "occupied", @@ -2098,8 +1991,6 @@ describe("DaemonServer lease liveness (ADR 0004)", () => { const leaseId = leaseIdOf(grant); expect((grant.payload as { lease: { ttlDeadline: number } }).lease.ttlDeadline).toBe(1_040); - // Renew on the client's own cadence, well past the grant-time deadline. Each renew names - // no TTL, so each re-applies the lease's own 40ms width from now. let expectedDeadline = 1_040; for (let cycle = 0; cycle < 6; cycle += 1) { harness.clock.advance(10); @@ -2109,11 +2000,8 @@ describe("DaemonServer lease liveness (ADR 0004)", () => { payload: { id: leaseId, ttlDeadline: expectedDeadline, ttlMs: 40 }, }); } - // 60ms have passed since the grant, well past the deadline it carried, and the lease is - // alive because something kept renewing it. expect(harness.registry.snapshot.leases).toMatchObject([{ id: leaseId }]); - // Stop renewing and the deadline catches up, with no separate backstop behind it. harness.clock.advance(40); await expect.poll(() => harness.registry.snapshot.leases).toEqual([]); await expect(holder.nextFrame((frame) => frame.push === "lease-lost")).resolves.toMatchObject({ @@ -2255,15 +2143,6 @@ describe("DaemonServer lease liveness (ADR 0004)", () => { payload: { ttlDeadline: 1_050 }, }); - // Simulate an abrupt daemon crash: the first daemon's process just disappears, leaving - // the last persisted registry state -- the renewed deadline -- on disk. It is - // intentionally left running here; the afterEach hook tears it down. - - // "Restart": a fresh daemon reloads that persisted state. ADR 0004 removed the orphan - // sweep that used to release this lease on the theory that a restart proves its holder - // is dead -- it proves nothing of the sort, so the lease stands and its timer is - // restored from the renewed deadline. The underlying device (unlike the daemon process) - // survives a restart, so the "restarted" harness shares the same fake driver instance. const second = await createHarness({ clock: new FakeClock(1_010), driver: first.driver, @@ -2275,8 +2154,6 @@ describe("DaemonServer lease liveness (ADR 0004)", () => { expect(second.registry.snapshot.leases).toMatchObject([{ id: leaseId, ttlDeadline: 1_050 }]); expect(second.registry.snapshot.devices).toMatchObject([{ id: deviceId, state: "leased" }]); - // The restored timer is the renewed one, not the grant-time one: nothing expires at - // 1_040, and the lease ends at 1_050 with no client left to renew it. second.clock.advance(30); await expect.poll(() => second.registry.snapshot.leases).toHaveLength(1); second.clock.advance(10); @@ -2299,7 +2176,6 @@ describe("DaemonServer lease liveness (ADR 0004)", () => { const deviceId = (grant.payload as { device: { id: string } }).device.id; await holder.close(); - // The next daemon starts well after the persisted deadline (1_040). const second = await createHarness({ clock: new FakeClock(2_000), driver: first.driver, @@ -2325,8 +2201,6 @@ describe("DaemonServer lease liveness (ADR 0004)", () => { platform: "ios", }); const leaseId = leaseIdOf(grant); - // Written at grant, before any renew -- it is a stored field, not a derived one, so - // there is always an answer. expect((grant.payload as { lease: { lastRenewedAt: number } }).lease.lastRenewedAt).toBe(1_000); harness.clock.advance(10); @@ -2394,9 +2268,6 @@ describe("DaemonServer decorations", () => { platform: "ios", }); - // The stop itself releases nothing (ADR 0004 §3), so the reclaim `stop` has to drain - // is one somebody else started: the holder releasing on its way out, which commits - // the registry half and hands the purge off. await holder.request("lease.release", { leaseId: leaseIdOf(grant) }); expect(harness.registry.snapshot.leases).toHaveLength(0); @@ -2405,7 +2276,7 @@ describe("DaemonServer decorations", () => { finishSettle(); await stopping; - // Disposal last: a reclaim that settles into a purge failure arms a quarantine + // why: Disposal last: a reclaim that settles into a purge failure arms a quarantine // retry timer, and cancelling before the drain would strand it armed. expect(order).toEqual(["settle-start", "settle-end", "dispose"]); }); @@ -2447,7 +2318,7 @@ describe("DaemonServer decorations", () => { const stopping = harness.daemon.stop("test-async-dispose"); await expect.poll(() => disposed).toBe(false); - // A driver's release is asynchronous -- Android's reaps the adb server it started -- + // why: A driver's release is asynchronous -- Android's reaps the adb server it started -- // and a stop that returned before it finished would report a shutdown that had not // happened, leaving the next daemon to find the port still held. finishDispose(); @@ -2480,8 +2351,6 @@ describe("DaemonServer decorations", () => { await harness.daemon.stop("test-stop-auxiliary"); expect(order).toEqual(["stopAuxiliary", "settle", "dispose"]); - // The lease is untouched: ADR 0004 §3 -- a stop tears down connections and timers, - // and leaves every lease standing with its deadline for the next daemon. expect(harness.registry.snapshot.leases).toHaveLength(1); }); @@ -2550,7 +2419,6 @@ describe("DaemonServer download policy", () => { }); const client = await createClient(harness.socketPath); await hello(client); - // The fake names the requested version as the component: 65 characters, one past the bound. const osVersion = "9".repeat(65); const grant = await client.request("lease.request", { @@ -2616,9 +2484,6 @@ describe("DaemonServer download policy", () => { const client = await createClient(harness.socketPath); await hello(client); - // No allowDownload on the request and the default policy, so this fails exactly as it - // did before the policy existed -- and, unlike the never-policy case above, the message - // is not attributed to configuration, since nothing in config forced the outcome. const response = await client.request("lease.request", { requesterId: "agent-1", model: "iPhone 16", @@ -2635,8 +2500,6 @@ describe("DaemonServer download policy", () => { it("never attaches the download-policy suffix to an undownloadable RuntimeMissingError, even under the never policy", async () => { const clock = new FakeClock(1_000); const driver = new FakeDriver({ availableOsVersions: [], clock, platform: "ios" }); - // Stands in for a real out-of-range / unpaired-runtime error: no download could ever have - // fixed this request, so the policy is not what blocked it. driver.failOn( "resolveSpec", 1, @@ -2750,10 +2613,6 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { await client.close(); }); - // ADR 0003 §6's "frozen exception" for `daemon.stop` is scoped to the protocol-version gate - // only -- it must still require a completed handshake and the `admin` role, per §3's operation - // matrix and the ADR's Context section ("Any local connection can release any lease, nuke, or - // stop the daemon" is named as the defect this ADR fixes). describe("daemon.stop authorization (ADR 0003 §3, §5, §6)", () => { it("refuses daemon.stop from an agent-role connection with FORBIDDEN", async () => { const harness = await createHarness({ resolveRole: { resolve: () => "agent" } }); @@ -2816,10 +2675,6 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { ok: false, }); - // The handshake failed outright: `#handleHello` closes the connection right after the - // rejected credential, before `connection.helloReceived` is ever set. `daemon.stop` -- - // despite ADR §6's frozen exception, which is scoped to protocol version only -- never - // gets a chance to run on this connection: there is no connection left to run it on. await socketClosed; expect(harness.daemon.health).toBe("running"); }); @@ -2843,7 +2698,6 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { ok: false, }); - // Every other operation on this mismatched connection is refused the same way, repeatedly. await expect(client.request("status.get", {})).resolves.toMatchObject({ error: { code: "PROTOCOL_VERSION_UNSUPPORTED" }, ok: false, @@ -2853,9 +2707,6 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { ok: false, }); - // But `daemon.stop` -- the ADR §6 frozen exception -- still works, because the role was - // resolved from the handshake before the version check ran, and this connection's role is - // admin. await expect(client.request("daemon.stop", {})).resolves.toMatchObject({ ok: true, payload: { stopping: true }, @@ -2892,9 +2743,6 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { await hello(client); await expect(client.request("doctor.run", { fix: false })).resolves.toMatchObject({ - // No `doctor` collaborator wired into this harness, so a fix:false call clears the - // role gate and fails DOCTOR_UNAVAILABLE downstream -- proof the rejection was not - // FORBIDDEN, which is the only thing this test cares about. error: { code: "DOCTOR_UNAVAILABLE" }, ok: false, }); @@ -3030,9 +2878,6 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { platform: "ios", }); expect(granted.ok).toBe(true); - // ADR 0004 §4: `ttlMs` is accepted on every request -- it used to be `BAD_REQUEST` unless - // the request also named `mode: "detached"` -- and the width it asks for is stored on the - // record, which is what a later body-less renew re-applies. expect( (granted.payload as { lease: { grantedAt: number; ttlMs: number; ttlDeadline: number } }) .lease, @@ -3048,8 +2893,6 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { }); it("cancels a still-queued lease.cancel without closing the connection, and the original request settles rather than hanging", async () => { - // Default harness capacity is one iOS device (see `testConfig`), so a second held request - // for the same spec queues behind the first without any extra configuration. const harness = await createHarness(); const holder = await createClient(harness.socketPath); await hello(holder); @@ -3075,8 +2918,6 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { expect(cancel).toMatchObject({ ok: true, payload: { result: "cancelled" } }); await expect(queuedRequest).resolves.toMatchObject({ ok: false }); - // The connection is still alive and usable after cancelling -- ADR §9: "Cancellation no - // longer means closing the connection." await expect(waiter.request("status.get", {})).resolves.toMatchObject({ ok: true }); await holder.close(); @@ -3094,10 +2935,6 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { }); const leaseId = leaseIdOf(grant); - // A second, otherwise-idle connection sharing the same principal -- today's bug (fixed by - // this PR) is that only the connection holding the lease ever learned of its end; a - // detached lease has no "holding" connection at all, so this second connection previously - // learned nothing. const observer = await createClient(harness.socketPath); await helloAs(observer, "alice"); @@ -3107,20 +2944,12 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { const push = await observer.nextFrame((frame) => frame.push === "lease-lost"); expect(push.payload).toMatchObject({ leaseId, reason: "explicit" }); - // The releasing connection itself does not get a redundant self-push. expect(requester.frames().filter((frame) => frame.push === "lease-lost")).toEqual([]); await requester.close(); await observer.close(); }); - /** - * ADR 0005 §5: over the uplink the gateway is the protocol client, and the worker grants that - * session `admin` because *it* opened the connection to the gateway named in its own config. - * The harness resolves every ordinary session to `agent` here, so the contrast is the point: - * the same `hello`, with no credential at all, is agent over the socket and admin over the - * uplink. - */ it("grants an accepted uplink the admin role without a credential (ADR 0005 §5)", async () => { const harness = await createHarness({ resolveRole: { resolve: () => "agent" } }); const pair = linkPair(); @@ -3134,12 +2963,10 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { }); expect(reply).toMatchObject({ ok: true, payload: { role: "admin" } }); - // ...and the role is real, not cosmetic: an admin-only operation answers. await expect(gateway.request("list.get", { kind: "devices" })).resolves.toMatchObject({ ok: true, }); - // Meanwhile a plain socket connection on the same daemon is still an agent. const local = await createClient(harness.socketPath); await local.request("hello", { clientVersion: "test", @@ -3200,7 +3027,6 @@ describe("DaemonServer roles and ownership (ADR 0003 §2-4)", () => { ok: false, error: { code: "PROTOCOL_VERSION_UNSUPPORTED" }, }); - // Both ranges travel, which is what a gateway puts on an `incompatible` worker view. expect(reply.error).toMatchObject({ details: { client: { min: 99, max: 99 }, daemon: PROTOCOL_VERSION_RANGE }, }); @@ -3828,9 +3654,6 @@ describe("daemon.update", () => { }); }); -/** Drives an in-memory `IpcConnection` the way `createClient` drives a real socket -- enough to - * send request frames and await their replies. Used for uplink tests, where there is no socket - * at all: the connection is the one a worker dialled out and handed to its own daemon. */ function driveConnection(connection: IpcConnection): { request(type: string, payload: unknown): Promise; } { @@ -4086,11 +3909,6 @@ async function createHarness( }; } -/** - * A `ConnectionHost` that hands `DaemonServer` one connection this test owns, so a frame's - * write can be held open deliberately. `DaemonEndpointHost` over a real socket cannot express - * "this write has not completed" without filling a kernel buffer. - */ class SingleConnectionHost implements ConnectionHost { readonly endpoint = "memory://test"; @@ -4141,8 +3959,6 @@ class BlockingConnection implements IpcConnection { this.#blocked = true; } - /** From here on every write fails as a Node socket's does after its peer's FIN and before it is - * destroyed: `closed` is still false, and the write rejects with EPIPE. */ hangUp(): void { this.#hungUp = true; } @@ -4202,11 +4018,6 @@ function rejectionNaming( return { ...rejection, passthroughTools }; } -/** - * A `ProcessRunner` whose streamed child is driven by the test rather than scripted up front -- - * `ScriptedProcessRunner` emits every chunk at spawn, which cannot express "a chunk arrives - * *after* the client disconnected", the case that matters here. - */ class ControllableStreamingRunner implements ProcessRunner { handle: ControllableStreamingHandle | undefined; diff --git a/src/gateway/service.test.ts b/src/gateway/service.test.ts index 413304386..359d3df8c 100644 --- a/src/gateway/service.test.ts +++ b/src/gateway/service.test.ts @@ -29,17 +29,13 @@ import { import { MAX_CONSECUTIVE_REFRESH_TIMEOUTS, WORKER_CALL_TIMEOUT_MS } from "./worker-link.js"; const RETENTION_MS = 24 * 60 * 60_000; -/** A worker one protocol version behind this gateway (ADR 0007 §12: no shim, no widened range). */ const previousProtocol = { min: PROTOCOL_VERSION_RANGE.min - 1, max: PROTOCOL_VERSION_RANGE.max - 1, }; const REFRESH_MS = 30_000; -/** Matches `core/config.ts`'s own default (ADR 0005 §15's own cap). */ const DEFAULT_LEASE_MAX_TTL_MS = 4 * 60 * 60_000; -/** H4: records every line at `warn` and `error` (the levels a link's own failure paths use), so - * a test can assert what a failure was actually logged *as* -- not just that something failed. */ class RecordingLogger implements Logger { readonly warnings: Array<{ message: string; fields?: Record }> = []; @@ -69,10 +65,7 @@ function fleet( const events: EventEnvelope[] = []; eventBus.subscribeAll((envelope) => events.push(envelope)); const transport = new MemoryUplinkTransport(); - /** The scripted worker each uplink resolves to, keyed by the principal-free order they join. */ const clients: ScriptedWorkerClient[] = []; - // C3: set to make the *next* `connect()` -- the `hello` round trip itself -- never resolve, - // the way a peer that completes the WebSocket upgrade and then falls silent would. const connectState = { hangNext: false }; const service = new GatewayService({ authenticate: async (credential) => options.authenticate?.(credential) ?? "accept", @@ -102,9 +95,6 @@ function fleet( events, service, transport, - /** Queues the worker the next uplink resolves to, then dials it. `token` defaults to the - * fixed secret every other test relies on; C-2's test overrides it per worker so a custom - * `authenticate` can hand back a distinct `tokenId` for each. */ join: async ( workerId: string, client: ScriptedWorkerClient, @@ -119,8 +109,6 @@ function fleet( ...(label === undefined ? {} : { label }), }); }, - /** Dials an uplink whose `hello` never answers -- no `ScriptedWorkerClient` is ever built, - * since C3's whole point is that the handshake itself must not be able to hang forever. */ joinSilent: async (workerId: string, label?: string) => { connectState.hangNext = true; return transport.connect({ @@ -202,7 +190,6 @@ describe("GatewayService", () => { const harness = fleet(); await harness.service.start(); const worker = new ScriptedWorkerClient(); - // A starting worker answers `status.get` with `daemon` and `host` only. worker.status = { daemon: { health: "starting", @@ -232,7 +219,6 @@ describe("GatewayService", () => { ]) { expect(Object.keys(view ?? {})).not.toContain(field); } - // Nothing but the status read, and the event subscription, was asked of it. expect( worker.calls.filter((call) => !["status.get", "events.subscribe"].includes(call)), ).toEqual([]); @@ -242,8 +228,6 @@ describe("GatewayService", () => { worker.pushEvent({ event: "lease.granted" }); await vi.waitFor(() => expect(harness.service.workers.view("wrk_1")?.health).toBe("running")); - // The refresh a lease event asks for reads no catalog of its own, but the starting answer - // dropped the last one, so this refresh reads it: the worker takes requests again at once. expect(harness.service.workers.view("wrk_1")).toMatchObject({ catalog: [], catalogReadAt: harness.clock.now(), @@ -253,11 +237,6 @@ describe("GatewayService", () => { await harness.service.stop(); }); - // #414, modelled on a real worker: its `events.subscribe` parks until startup ends (only - // `status.get` answers while it is `starting`), and it emits `daemon.started` before that parked - // call subscribes, so the gateway never hears it. A startup longer than `WORKER_CALL_TIMEOUT_MS` - // times the subscription out, and the refresh after it reads a `starting` answer. The clock - // stops there, short of the periodic tick. it("builds the full view of a worker that was starting when first read, once its startup ends, before the next tick", async () => { const harness = fleet(); await harness.service.start(); @@ -295,7 +274,6 @@ describe("GatewayService", () => { await vi.waitFor(() => expect(harness.service.workers.view("wrk_1")?.health).toBe("starting")); const startedAt = harness.clock.now(); - // Startup ends: `status.get` answers in full, and the parked subscription is answered. worker.status = statusFixture({ leases: [leaseFixture("lease_1", "dev_1")], queueDepth: 1 }); endStartup(); @@ -357,7 +335,6 @@ describe("GatewayService", () => { const catalogReads = () => worker.calls.filter((call) => call === "catalog.get").length; expect(catalogReads()).toBe(1); - // A refresh with no catalog is in flight, held at its first call, when a catalog refresh asks. const release = worker.holdStatus(); worker.pushEvent({ event: "device.state-changed" }); await vi.waitFor(() => expect(worker.calls.at(-1)).toBe("status.get")); @@ -366,7 +343,6 @@ describe("GatewayService", () => { release(); await vi.waitFor(() => expect(catalogReads()).toBe(2)); - // The view says so too: the catalog was read after the clock moved. await vi.waitFor(() => expect(harness.service.workers.view("wrk_a")?.catalogReadAt).toBe(harness.clock.now()), ); @@ -374,11 +350,6 @@ describe("GatewayService", () => { }); it("the worker view builder gives a gateway the view it built before the builder moved, plus when it read the catalog", async () => { - // Every field a refresh fills, set to a value that is not a default, and the whole view - // compared as one literal: the expected value is what the gateway built before the view - // builder moved into the contract, so any field the move dropped, renamed or reshaped - // fails here by name. `catalogReadAt` is the registry's own record of the read (ADR 0009 - // §4), not the builder's. const harness = fleet(); await harness.service.start(); const worker = new ScriptedWorkerClient("admin", "9.8.7"); @@ -762,8 +733,6 @@ describe("GatewayService", () => { const callsBefore = worker.calls.length; worker.pushEvent({ event: "doctor.reconciled" }); - // Still republished -- every worker fact reaches the gateway's buffer -- just not a reason - // to re-read anything. await vi.waitFor(() => expect(eventNames(harness.events)).toContain("doctor.reconciled")); expect(worker.calls).toHaveLength(callsBefore); @@ -870,7 +839,6 @@ describe("GatewayService", () => { "iPhone 17": ["26.0"], }), ); - // Same models and runtimes; only the pairing moved, as when a runtime gains support. worker.catalog = catalogFixture([ { modelRuntimes: { "iPhone 17": ["25.4", "26.0"] }, @@ -954,7 +922,6 @@ describe("GatewayService", () => { worker.status = statusFixture({ queueDepth: 1, waiting: [queued] }); worker.pushEvent({ event: "lease.queued" }); - // Well inside the periodic refresh, which this test never lets come round. await vi.waitFor(() => expect(harness.service.workers.view("wrk_1")?.waiting).toEqual([queued]), ); @@ -1079,8 +1046,6 @@ describe("GatewayService", () => { worker.catalog = catalogFixture([ { models: ["iPhone 17"], platform: "ios", runtimes: ["26.0", "26.4"] }, ]); - // The first starts a refresh without the catalog; the other two share the one queued - // behind it, and the last does not ask for the catalog. worker.pushEvent({ event: "lease.granted" }); worker.pushEvent({ event: "component.installed" }); worker.pushEvent({ event: "lease.released" }); @@ -1182,9 +1147,6 @@ describe("GatewayService", () => { ).length; expect(warningsAfterJoin).toBe(1); - // The periodic backstop tick re-reads config.get alongside the catalog - // (`#runTick` -> `link.refresh({ includeCatalog: true })`) -- the worker's cap has not - // moved, so this must not warn again. const callsBefore = worker.calls.filter((call) => call === "config.get").length; harness.clock.advance(REFRESH_MS); await vi.waitFor(() => @@ -1217,7 +1179,6 @@ describe("GatewayService", () => { }); it("marks a worker that speaks only the previous protocol version incompatible with both ranges, and asks it nothing else", async () => { - // Protocol 5 is the version before ADR 0008 made `modelRuntimes` required. const previous = { min: 5, max: 5 }; expect(negotiateProtocolVersion(PROTOCOL_VERSION_RANGE, previous)).toBeUndefined(); const harness = fleet(); @@ -1380,7 +1341,6 @@ describe("GatewayService", () => { await harness.service.start(); const worker = new ScriptedWorkerClient(); worker.failWith = protocolMismatchError(worker10); - // A gateway that subscribed anyway would get a live listener, so the push below could reach it. worker.subscribeDespiteFailure = true; await harness.join("wrk_1", worker); @@ -1402,8 +1362,6 @@ describe("GatewayService", () => { const harness = fleet(); await harness.service.start(); const worker = new ScriptedWorkerClient(); - // What `connectSimlockAdmin`'s degraded client does after a failed negotiation: every call - // rejects with the captured error, carrying both ranges. worker.failWith = protocolMismatchError(previousProtocol); await harness.join("wrk_1", worker); @@ -1416,8 +1374,6 @@ describe("GatewayService", () => { }); expect(worker.calls).toEqual(["status.get"]); expect(worker.subscribed).toBe(false); - // ADR 0005 §31: that uplink authenticated, so it is not a `worker.rejected` -- and there - // is no `worker.connected` either, because nothing usable connected. expect(eventNames(harness.events)).not.toContain("worker.rejected"); expect(eventNames(harness.events)).not.toContain("worker.connected"); @@ -1433,7 +1389,6 @@ describe("GatewayService", () => { await workerEnd.close(); - // Not on the next tick: the uplink is the reachability signal (ADR 0005 §6). await vi.waitFor(() => expect(harness.service.workers.view("wrk_1")?.connection).toBe("disconnected"), ); @@ -1442,11 +1397,6 @@ describe("GatewayService", () => { await harness.service.stop(); }); - // D1: `WorkerRegistry` is keyed by worker id, not by link, and a stale link's own close used - // to call `registry.disconnected` with no check at all -- so a socket the OS had not yet - // reported dead could mark the *live* successor's view disconnected, permanently (`refresh()` - // never touches `connection`). Against the code before this fix, no test joined the same - // worker id twice against a live service; this is that test. it("does not let a stale link's late close mark a reconnected worker disconnected (D1)", async () => { const harness = fleet(); await harness.service.start(); @@ -1457,22 +1407,12 @@ describe("GatewayService", () => { expect(harness.service.workers.view("wrk_1")?.connection).toBe("connected"), ); - // A's own close, once the reconnect below triggers it, must never actually complete during - // this test: a hung `events.unsubscribe` (D2) is exactly what stretches a stale link's close - // from microseconds to minutes in the wild, and here it lets the test control precisely - // when A's connection actually goes away. clientA.hangUnsubscribe = true; - // The same worker id redials with a new connection -- a restart, or a new TCP path after a - // NAT rebind. The service replaces the link; A's old one is asked to close but (per above) - // never gets there. const clientB = new ScriptedWorkerClient("admin", "9.9.9"); await harness.join("wrk_1", clientB); await vi.waitFor(() => expect(harness.service.workers.view("wrk_1")?.version).toBe("9.9.9")); - // Only now does the OS finally report the stale socket dead. Closing the worker's own end - // fires the connection's real close event directly, bypassing A's still-stuck `close()` - // chain -- exactly what a genuinely half-open socket closing on its own would do. await workerEndA.close(); expect(harness.service.workers.view("wrk_1")?.connection).toBe("connected"); @@ -1482,9 +1422,6 @@ describe("GatewayService", () => { await harness.service.stop(); }); - // D2: `close()` awaited `events.unsubscribe` -- a real round trip -- with nothing bounding it. - // On a half-open socket that promise never settled, so `close()` never reached - // `connection.close()` and the link (and its WebSocket) leaked for the life of the process. it("does not hang close() forever behind an events.unsubscribe that never answers (D2)", async () => { const harness = fleet(); await harness.service.start(); @@ -1495,20 +1432,14 @@ describe("GatewayService", () => { const stopped = harness.service.stop(); const stoppedState = promiseState(stopped); - // `stop()` is now blocked inside `link.close()`, awaiting the bounded wrapper around the - // hung unsubscribe -- wait for that timer to actually be armed before advancing past it. await vi.waitFor(() => expect(harness.clock.pendingTimerCount).toBeGreaterThan(0)); harness.clock.advance(WORKER_CALL_TIMEOUT_MS); - // Settled by the timer, not merely eventually: a broken bound fails here, by name. await vi.waitFor(() => expect(stoppedState.state).toBe("fulfilled")); await expect(stopped).resolves.toBeUndefined(); expect(worker.closed).toBe(true); }); - // D3: a hung round trip inside `#rebuildView` used to latch `#refreshing` forever, since it - // was cleared only in a `finally` on a promise that never settled -- freezing the view while - // it still reported `connected`. it("does not latch refreshing forever when a round trip hangs, and recovers past the timeout (D3)", async () => { const harness = fleet(); await harness.service.start(); @@ -1520,12 +1451,9 @@ describe("GatewayService", () => { worker.status = statusFixture({ leases: [leaseFixture("lease_9", "dev_9")] }); worker.pushEvent({ event: "lease.granted" }); - // The refresh this event triggered is now stuck inside `#rebuildView`'s bounded wait. await vi.waitFor(() => expect(harness.clock.pendingTimerCount).toBeGreaterThan(0)); harness.clock.advance(WORKER_CALL_TIMEOUT_MS); - // Once the hung refresh times out and un-latches `#refreshing`, a later event must still be - // able to trigger a real one -- proving the latch did not stay stuck. worker.hangingCalls.delete("status.get"); worker.pushEvent({ event: "lease.granted" }); @@ -1538,11 +1466,6 @@ describe("GatewayService", () => { await harness.service.stop(); }); - // H2: the same asymmetry D1 fixed in `#handleClosed`, one write later -- `#rebuildView` wrote - // to `registry.refresh` after its own `await` with no re-check of `#closed` or - // `isCurrentLink`. A stale link's in-flight refresh, started before a reconnect replaced it, - // could land after its successor's fresher one and overwrite it with stale data for up to - // `WORKER_CALL_TIMEOUT_MS`. it("does not let a stale link's late-arriving refresh overwrite its successor's fresher view (H2)", async () => { const harness = fleet(); await harness.service.start(); @@ -1551,8 +1474,6 @@ describe("GatewayService", () => { await harness.join("wrk_1", clientA); await vi.waitFor(() => expect(harness.service.workers.view("wrk_1")?.version).toBe("0.1.0")); - // A's refresh, triggered by a worker event, is in flight but held open by hand -- long - // enough for a reconnect to fully replace this link before it ever answers. let resolveStatus: ((status: ReturnType) => void) | undefined; clientA.getStatus = () => new Promise((resolve) => { @@ -1561,16 +1482,12 @@ describe("GatewayService", () => { clientA.pushEvent({ event: "lease.granted" }); await vi.waitFor(() => expect(resolveStatus).toBeDefined()); - // The same worker id redials with a new connection -- service.ts replaces A's link with B's, - // which builds its own, current view. const clientB = new ScriptedWorkerClient("admin", "9.9.9"); await harness.join("wrk_1", clientB); await vi.waitFor(() => expect(harness.service.workers.view("wrk_1")?.version).toBe("9.9.9")); - // Only now does A's stale round trip finally answer -- exactly what a slow or half-open path - // answering late would do, well after B has already built the current view. resolveStatus?.(statusFixture()); - // A few real event-loop turns for A's now-resolved promise chain to run to completion (or, + // why: A few real event-loop turns for A's now-resolved promise chain to run to completion (or, // with the fix, to stop itself before writing) -- nothing here is clock-scheduled, so there // is no fake-clock timer to advance instead. await new Promise((resolve) => setTimeout(resolve, 20)); @@ -1580,8 +1497,6 @@ describe("GatewayService", () => { await harness.service.stop(); }); - // H2's case again, for the answer a starting worker gives: a replaced link's late `starting` - // must not strip the successor's view of its devices and leases. it("does not let a stale link's late starting answer overwrite its successor's view", async () => { const harness = fleet(); await harness.service.start(); @@ -1596,7 +1511,6 @@ describe("GatewayService", () => { await harness.join("wrk_1", clientB); await vi.waitFor(() => expect(harness.service.workers.view("wrk_1")?.version).toBe("9.9.9")); - // A's held `status.get` now answers `starting`, after B built the current view. clientA.status = { daemon: { health: "starting", @@ -1619,12 +1533,6 @@ describe("GatewayService", () => { await harness.service.stop(); }); - // P1: a refresh timeout used to be swallowed into a debug log with nothing acting on it, so a - // half-open uplink (NAT rebind, cable pull, kernel panic -- no FIN) had `connection` report - // `connected` forever with a `lastSeenAt` that never moved again. One timeout recovering on - // the very next attempt (D3, above) must still not disconnect a worker -- that is a real, slow - // response, not a dead link -- but the same failure MAX_CONSECUTIVE_REFRESH_TIMEOUTS times in a - // row, with nothing in between, must make the gateway give up and close the uplink. it("closes the uplink once refreshes keep timing out, so a half-open worker stops reporting connected (P1)", async () => { expect(MAX_CONSECUTIVE_REFRESH_TIMEOUTS).toBeGreaterThan(1); const harness = fleet(); @@ -1633,26 +1541,22 @@ describe("GatewayService", () => { await harness.join("wrk_1", worker); await vi.waitFor(() => expect(harness.service.workers.view("wrk_1")?.capacity).toBeDefined()); - // Never un-hung, unlike D3's test: this is the half-open case, not a briefly slow one. worker.hangingCalls.add("status.get"); for (let attempt = 1; attempt < MAX_CONSECUTIVE_REFRESH_TIMEOUTS; attempt++) { harness.clock.advance(REFRESH_MS); await vi.waitFor(() => expect(harness.clock.pendingTimerCount).toBeGreaterThan(0)); harness.clock.advance(WORKER_CALL_TIMEOUT_MS); - // Wait for the *next* tick to actually be armed, not just for the (already, trivially + // why: Wait for the *next* tick to actually be armed, not just for the (already, trivially // true) `connected` check to pass -- that value never changes on a timeout, so checking // it alone would resolve before the tick's own promise chain (refresh's catch, then // `pruneExpired`, then rescheduling) has actually finished, and the next iteration's // `advance(REFRESH_MS)` would fast-forward the clock past a timer that has not been // registered yet -- silently skipping a whole tick instead of exercising it. await vi.waitFor(() => expect(harness.clock.pendingTimerCount).toBeGreaterThan(0)); - // Below the threshold, the view must still say `connected` -- exactly D3's guarantee, - // repeated for every attempt short of the last one. expect(harness.service.workers.view("wrk_1")?.connection).toBe("connected"); } - // The attempt that crosses the threshold. harness.clock.advance(REFRESH_MS); await vi.waitFor(() => expect(harness.clock.pendingTimerCount).toBeGreaterThan(0)); harness.clock.advance(WORKER_CALL_TIMEOUT_MS); @@ -1665,15 +1569,8 @@ describe("GatewayService", () => { await harness.service.stop(); }); - // C-1: P1's counter used to be driven by the whole bundled refresh timing out, which conflated - // a dead transport with a merely busy dispatcher -- on a worker mid-`converge`, `runDispatch` - // parks every call except `status.get` behind the startup-readiness gate, so `list.get` can - // legitimately hang for tens of seconds while the socket is completely healthy. Reproduces the - // reviewer's repro: `status.get` keeps answering, `list.get` never does, for well more than - // `MAX_CONSECUTIVE_REFRESH_TIMEOUTS` refresh cycles in a row. A gateway that still infers - // transport death from this would flip the link to `disconnected` long before this loop ends. it("does not close the link when status.get keeps answering but the dispatcher is busy (C-1)", async () => { - // A refreshIntervalMs far outside this test's clock advances: `#runTick` only reschedules + // why: A refreshIntervalMs far outside this test's clock advances: `#runTick` only reschedules // its own timer once a refresh settles (`service.ts`'s `.finally(() => this.#scheduleTick())`), // so on a link whose refresh keeps timing out the tick's real cadence drifts to roughly // `refreshIntervalMs + WORKER_CALL_TIMEOUT_MS` per cycle -- exercising this via event-driven @@ -1685,17 +1582,12 @@ describe("GatewayService", () => { await harness.join("wrk_1", worker); await vi.waitFor(() => expect(harness.service.workers.view("wrk_1")?.capacity).toBeDefined()); - // Unlike P1's test (which hangs `status.get` itself), the worker answers `status.get` on - // every single call -- only the readiness-gated `list.get` never returns, exactly what a - // worker stuck in a long `converge` looks like from the gateway's side. worker.hangingCalls.add("list.get:devices"); for (let attempt = 1; attempt <= MAX_CONSECUTIVE_REFRESH_TIMEOUTS + 2; attempt++) { const listCallsBefore = worker.calls.filter((call) => call === "list.get:devices").length; - // A worker event, not the tick, triggers each refresh here -- `#onWorkerEvent` calls - // `refresh()` directly, with none of the tick's own scheduling drift. worker.pushEvent({ event: "lease.granted" }); - // Waits for `list.get` to have actually been called -- proof `status.get`'s real promise + // why: Waits for `list.get` to have actually been called -- proof `status.get`'s real promise // was given a genuine chance to settle and `#rebuildView` moved on to the hung batch. // Waiting on `pendingTimerCount` instead would be a false positive: `status.get`'s own // bounded wait registers a timer *synchronously*, before its real response has had any @@ -1709,11 +1601,7 @@ describe("GatewayService", () => { ), ); harness.clock.advance(WORKER_CALL_TIMEOUT_MS); - // Real microtask turns for the timeout rejection to propagate through `#rebuildView` and - // `refresh()`'s catch/finally -- nothing here is clock-scheduled once the timer fires. await new Promise((resolve) => setTimeout(resolve, 20)); - // `status.get` answered this round, so liveness holds no matter how many cycles the - // dispatcher stays busy -- unlike P1's test, this must stay true past the threshold. expect(harness.service.workers.view("wrk_1")?.connection).toBe("connected"); } expect(eventNames(harness.events)).not.toContain("worker.disconnected"); @@ -1721,16 +1609,8 @@ describe("GatewayService", () => { await harness.service.stop(); }); - // C-2, ADR 0005 §8 ("revoking closes the uplink"): a join token was checked once, at upgrade, - // and nothing ever re-verified a live link -- so revoking a worker's token used to have no - // observable effect on an already-open uplink at all. `closeLinksForToken` is what - // `GatewayDispatcher#tokenRevoke` calls after a successful revoke; this exercises it end to - // end against real `WorkerLink`s, matched by the exact token each authenticated with, so a - // shared or reused credential (however unlikely) cannot silently take out an unrelated worker. it("closes exactly the link a token authorized, and leaves every other link alone (C-2)", async () => { const harness = fleet({ - // Mirrors `main.ts`'s real `authenticate`: `accept` names the token id that authorized - // it, which is the whole plumbing this fix adds. authenticate: (credential) => credential === undefined ? "unauthenticated" @@ -1754,7 +1634,6 @@ describe("GatewayService", () => { await vi.waitFor(() => expect(harness.service.workers.view("wrk_a")?.connection).toBe("disconnected"), ); - // B authenticated with a different token entirely -- revoking A's must not touch it. expect(harness.service.workers.view("wrk_b")?.connection).toBe("connected"); expect(eventNames(harness.events)).toEqual( expect.arrayContaining(["worker.connected", "worker.connected", "worker.disconnected"]), @@ -1763,12 +1642,6 @@ describe("GatewayService", () => { await harness.service.stop(); }); - // C3: `hello` -- inside `connect()`, the first round trip `start()` makes -- had no timeout - // at all, unlike every call after it. A peer that completes the WebSocket upgrade with a - // valid join token and then never answers `hello` (a half-open TCP right after upgrade, or a - // deliberately silent client) used to hang `start()` forever: the link stayed in `#links` - // (registered before `start()` even runs), holding an open socket and no view, invisible in - // `simlock worker list` while consuming a slot -- D2's exact failure mode, one call earlier. it("does not hang the handshake forever when hello itself never answers (C3)", async () => { const harness = fleet(); await harness.service.start(); @@ -1777,21 +1650,11 @@ describe("GatewayService", () => { await vi.waitFor(() => expect(harness.clock.pendingTimerCount).toBeGreaterThan(0)); harness.clock.advance(WORKER_CALL_TIMEOUT_MS); - // The decisive check: a timed-out hello must make the link actually give up and close its - // end of the socket -- closing either end of an in-memory pair closes both, so this is only - // true once the gateway's side has been closed. Against the unbounded `connect()` this - // never happens: nothing ever times out, so nothing ever closes, and this assertion is what - // tells the two apart (a looser "no view was built" check passes either way, since a - // permanently-hung connect() also never builds one). await vi.waitFor(() => expect(workerEnd.closed).toBe(true)); - // Timing out must not fabricate a view for a worker the gateway never actually spoke to. expect(eventNames(harness.events)).not.toContain("worker.connected"); expect(harness.service.workers.view("wrk_1")).toBeUndefined(); - // And the slot must actually be freed: the same worker id reconnecting for real (a client - // that does answer) must build a normal view rather than being shut out by a link that - // leaked past its timeout. const worker = new ScriptedWorkerClient(); await harness.join("wrk_1", worker); await vi.waitFor(() => @@ -1820,8 +1683,6 @@ describe("GatewayService", () => { await harness.service.stop(); }); - // The two refusals reach the event stream apart, because they point at different fixes: - // the join token itself, or the role it was minted with (ADR 0005 §4). it.each(["unauthenticated", "forbidden"] as const)( "reports a %s uplink it turned away, without inventing a worker for it", async (reason) => { @@ -1835,9 +1696,6 @@ describe("GatewayService", () => { }); expect(harness.service.workers.views()).toEqual([]); - // M3: `workerId`/`label` are whatever the dial *claimed* in its headers -- never verified - // (a refused peer proves no identity), but still worth an operator's while, and something - // this event used to always report as `undefined` regardless of what the dial sent. expect(harness.events.at(-1)).toMatchObject({ event: "worker.rejected", payload: { label: "mac-mini-1", reason, workerId: "wrk_1" }, @@ -1847,13 +1705,6 @@ describe("GatewayService", () => { }, ); - // P-2: `GET /v1/uplink` needs no credential to reach `authenticate`, and before this fix every - // refusal emitted its own `worker.rejected` -- so a flood of refused dials (deliberate, or a - // misconfigured worker retrying fast) could fill the event bus's ring buffer, bounded by count - // and not bytes, with nothing but refusals, evicting everything else in it. No test dialed - // twice before this round. This asserts the coalescing itself: two dials with the same claimed - // id and reason inside one window produce exactly one event, and the first dial after the - // window closes reports the closed window's total as `count`. it("coalesces a flood of identical refusals into one event per window, with a count only above one (P-2)", async () => { const harness = fleet({ authenticate: () => "unauthenticated" }); await harness.service.start(); @@ -1867,14 +1718,10 @@ describe("GatewayService", () => { harness.join("wrk_flood", new ScriptedWorkerClient(), "mac-mini-1"), ).rejects.toMatchObject({ code: "rejected" }); - // Both refusals were real (each dial above genuinely rejected), but only the first became - // an event -- and it carries no `count` at all, the exact pre-P-2 payload shape. expect(rejectedEvents()).toHaveLength(1); expect(rejectedEvents()[0]?.payload).toMatchObject({ workerId: "wrk_flood" }); expect(rejectedEvents()[0]?.payload).not.toHaveProperty("count"); - // Past the window: the next matching refusal is reported on its own, carrying the tally the - // just-closed window absorbed (both dials above). harness.clock.advance(REJECTION_COALESCE_WINDOW_MS); await expect( harness.join("wrk_flood", new ScriptedWorkerClient(), "mac-mini-1"), @@ -1883,8 +1730,6 @@ describe("GatewayService", () => { expect(rejectedEvents()).toHaveLength(2); expect(rejectedEvents()[1]?.payload).toMatchObject({ count: 2, workerId: "wrk_flood" }); - // A different claimed id is its own window from the start -- coalescing must not blend - // unrelated identities together into one gate. await expect( harness.join("wrk_other", new ScriptedWorkerClient(), "mac-mini-2"), ).rejects.toMatchObject({ code: "rejected" }); @@ -1909,8 +1754,6 @@ describe("GatewayService", () => { it("closes the uplink when a worker does not grant the gateway admin", async () => { const harness = fleet(); await harness.service.start(); - // A worker older than #117, or one that resolved the session some other way: nothing the - // gateway asks would be answered, so there is no view worth keeping. const worker = new ScriptedWorkerClient("agent"); const workerEnd = await harness.join("wrk_1", worker); @@ -1937,9 +1780,6 @@ describe("GatewayService", () => { expect(worker.calls).toHaveLength(callsAfterStop); }); - // `./fleet-ports.ts`'s WorkerDispatchTarget#client -- #118's seam. Both halves of the - // contract: undefined before start() has completed the handshake, and undefined again once - // the link has closed. describe("target() / WorkerDispatchTarget (fleet-ports)", () => { it("has no client before the handshake completes", async () => { const harness = fleet();