From 2300582a2a7d0d36d63322e9c9bcba930ae4b23b Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 26 Oct 2026 19:08:00 +0100 Subject: [PATCH 1/3] chore(comments): delete comments that say what, not why, in src/daemon --- src/daemon/connection-host.ts | 1 - src/daemon/dispatcher.ts | 155 +++++---------------------------- src/daemon/main.test.ts | 142 +++++------------------------- src/daemon/main.ts | 159 +++++----------------------------- 4 files changed, 66 insertions(+), 391 deletions(-) diff --git a/src/daemon/connection-host.ts b/src/daemon/connection-host.ts index 3cb515ddb..e0c93e3e8 100644 --- a/src/daemon/connection-host.ts +++ b/src/daemon/connection-host.ts @@ -64,7 +64,6 @@ export interface DaemonEndpointHostOptions { readonly logger?: Logger; } -/** Owns endpoint claiming, stale-entry recovery, and listener lifecycle. */ export class DaemonEndpointHost implements ConnectionHost { readonly endpoint: string; readonly #logger: Logger; diff --git a/src/daemon/dispatcher.ts b/src/daemon/dispatcher.ts index 2187d61ce..9a2b83750 100644 --- a/src/daemon/dispatcher.ts +++ b/src/daemon/dispatcher.ts @@ -85,12 +85,6 @@ import type { UpdateCoordinator, UpdateProgress } from "./update-coordinator.js" export { DispatchError, type ContractDispatcher, type DispatchSession } from "./dispatch.js"; -/* - * `DispatchSession`, `DispatchError`, and `ContractDispatcher` moved to `./dispatch.js` when - * ADR 0005 gave the contract a second dispatcher implementation (`src/gateway/`), which needs - * them without importing this file's `src/core` dependencies. They are re-exported above, so - * every existing `from "./dispatcher.js"` import keeps working. - */ /** * How long a timed-out `device.exec` child gets between SIGTERM and SIGKILL (ADR 0005 §19e). * Fixed rather than derived from `exec.timeoutMs`: it is a termination-cleanup budget, not a @@ -99,7 +93,7 @@ export { DispatchError, type ContractDispatcher, type DispatchSession } from "./ */ const EXEC_SIGKILL_GRACE_MS = 10_000; -/** The `Promise.race` marker for "the timeout won". A unique object rather than a string, so +/** why: The `Promise.race` marker for "the timeout won". A unique object rather than a string, so * it can never collide with something a `StreamingProcessHandle` could resolve with. */ const EXEC_EXPIRED = Symbol("exec-expired"); @@ -130,15 +124,9 @@ export interface DispatcherOptions { readonly historyPath?: string; readonly capabilitySources?: readonly CapabilitySource[]; readonly capacity: CapacityReader; - /** The warm pool as its last pass left it, for `status.get`. */ readonly warmPool: WarmPoolReader; readonly catalog: CatalogReader; readonly clock: Clock; - /** - * The one installer (ADR 0010 §3), shared with the lease path so `component.install` and a - * lease request's download join one install, and read for `status.get`'s installs in progress - * and for `component.list`. `component.remove` goes through it too (ADR 0010 §8). - */ readonly components: Pick; readonly config: Config; readonly doctor?: Doctor; @@ -146,21 +134,15 @@ export interface DispatcherOptions { readonly deviceModes: DeviceModeReader; readonly leases: LeaseCommands; readonly logger?: Logger; - /** Classifies a thrown error for the `operation` log line (`classifyError` in production). + /** why: Classifies a thrown error for the `operation` log line (`classifyError` in production). * Injected because `error-code.ts` imports this module. Unset or `undefined`: `INTERNAL`. */ readonly errorCode?: (error: unknown) => string | undefined; readonly nuke?: Nuke; readonly enrollment?: Pick; - /** Optional so the many tests that never issue a passthrough need not fabricate a resolver. */ readonly passthrough?: PassthroughResolver; - /** - * Runs the command `device.exec` resolves (ADR 0005 §19a). Optional for the same reason as - * `passthrough`: the many tests that never exec should not have to fabricate a runner -- - * `#deviceExec` answers `INTERNAL` when it is missing rather than crashing. - */ readonly processRunner?: ProcessRunner; /** - * The environment a `device.exec` child starts from, before the driver's own scoping keys are + * why: The environment a `device.exec` child starts from, before the driver's own scoping keys are * layered on top. Injected rather than read from `process.env` here (architecture rule 9), * and layered rather than replaced because `ProcessRunner` replaces a child's environment * wholesale: a child given only `--set`/`-P` scoping would lose `PATH` and never find the @@ -171,13 +153,6 @@ export interface DispatcherOptions { readonly presence?: (driverDeviceId: string) => LatestPresence | undefined; readonly reaper: CleanupReaper; readonly registry: Registry; - /** - * What the `stalled` flag `status.get` and `list.get` put on a device is worked out from - * (`isStalledTransition`, with `config.stalledTransition` and the clock): the drivers this - * daemon runs, and its device operation claims, so a live operation is never read as a - * stall. Optional so the tests that never stall a device need not fabricate either; absent, - * no device is flagged. - */ readonly stalls?: { readonly drivers: readonly Pick[]; readonly claims: NonNullable; @@ -189,28 +164,18 @@ export interface DispatcherOptions { * clear `DispatchError` when it is missing rather than crashing on `undefined.create(...)`. */ readonly tokens?: TokenOperations; - /** Reports current daemon health for `status.get`. */ readonly health: () => "starting" | "running" | "failed"; - /** `status.get`'s host block, from memory: answering never waits for a read (ADR 0008 §7). */ readonly hostFacts: () => HostFacts; readonly load?: () => StatusLoad | undefined; readonly awaitReady: () => Promise; - /** This daemon's instance id (`instance.json`), the id it presents to a gateway. `worker.list` - * answers with it as the one worker's id (ADR 0012 §1). */ readonly instanceId: string; - /** This daemon's own version, the one its `hello` reply carries; `worker.list`'s `version`. */ readonly version: string; - /** - * Where `worker.list` hears `WORKER_VIEW_CATALOG_EVENTS`, so its kept catalog is read again at - * once rather than on its next interval. Optional for the tests that never call `worker.list`; - * without it the kept catalog is only re-read on the interval. - */ readonly eventBus?: Pick; readonly update?: UpdateCoordinator; } /** - * A handler's declared *input* type is tied to its operation's contract schema (real + * why: A handler's declared *input* type is tied to its operation's contract schema (real * type-checking value: a handler that reads `input.wrongField` fails to compile). Its return * type is deliberately left as `unknown` rather than `z.infer<...Output>`: core's domain types * (`LeaseRecord`, `DoctorReport`, ...) are hand-mirrored by the contract's schemas, not reused @@ -225,27 +190,13 @@ type Handler = ( session: DispatchSession, ) => Promise | unknown; -/** Every operation this (worker-mode) dispatcher implements: the contract's set minus - * `daemon.stop` (intercepted by `DaemonServer` itself, ADR 0003 §6). */ type WorkerOperationName = Exclude; -/** - * The transport-independent dispatcher (ADR 0003 §2). One `dispatch()` call does, in order: - * parse input, role check, `authorize` hook, park on startup readiness, call handler, parse - * output. Handlers never see a raw payload or run their own role/ownership check -- both - * already happened by the time a handler's function body runs. - * - * Deliberately excludes `hello` (protocol-level, answered before a session exists) and - * `daemon.stop` (ADR §6's frozen exception -- scoped to the protocol-version gate only, so it - * stays reachable across a version mismatch; still requires a completed handshake and the - * `admin` role, checked in `DaemonServer#dispatchLine` itself) -- both stay in `DaemonServer`, - * same as before this PR. - */ export class Dispatcher { readonly #logger: Logger; readonly #dispatchLogger: Logger; /** - * Total over every operation but `daemon.stop`. Deliberately *not* a partial map: a declared + * why: Total over every operation but `daemon.stop`. Deliberately *not* a partial map: a declared * operation whose handler was never written is otherwise invisible to the compiler and only * shows up as `UNKNOWN_REQUEST` at runtime -- which is exactly how `driver.passthrough` came * to be declared, dispatched, and unimplemented at once. @@ -265,7 +216,6 @@ export class Dispatcher { * share it; a read that fails is dropped, so the next call reads again. */ #viewCatalog: { readonly readAt: number; readonly catalog: Promise } | undefined; - /** Ends the bus subscriptions that drop `#viewCatalog`; see `dispose`. */ readonly #unsubscribe: (() => void)[] = []; readonly #usage: UsageReader; @@ -290,7 +240,6 @@ export class Dispatcher { }, ], }); - // Observers only (architecture rule 5): dropping a kept read decides nothing. for (const event of WORKER_VIEW_CATALOG_EVENTS) { const unsubscribe = options.eventBus?.subscribe(event, () => { this.#viewCatalog = undefined; @@ -384,8 +333,7 @@ export class Dispatcher { }); } - // ---- handlers --------------------------------------------------------------------------- - // Arrow-function class fields (not methods): each closes over `this` so it can be stored in + // why: Arrow-function class fields (not methods): each closes over `this` so it can be stored in // `#handlers` and invoked as a plain function, the same way any other transport-independent // callback would be. None of these run a role or ownership check -- `dispatch()` above // already did, per the ADR's ordering. @@ -509,7 +457,7 @@ export class Dispatcher { const requesterId = input.requesterId ?? session.principal; const requestedAllowDownload = input.allowDownload ?? false; const downloadsPolicy = this.options.config.downloads.policy; - // `owner` is read only from the gateway's uplink, never from role alone: HTTP's `operator` token + // why: `owner` is read only from the gateway's uplink, never from role alone: HTTP's `operator` token // maps to `admin`, so a role check would let any operator name someone else's owner. Refused, not ignored. if (input.owner !== undefined && session.isGatewayUplink !== true) { throw new DispatchError( @@ -517,7 +465,7 @@ export class Dispatcher { "Only the gateway's own uplink session may set lease.request's `owner` field", ); } - // Uplink only, refused before anything is stored: a probe is never queued on a worker, so it must say `noWait`. + // why: Uplink only, refused before anything is stored: a probe is never queued on a worker, so it must say `noWait`. if (input.fleetRequestId !== undefined) { if (session.isGatewayUplink !== true) { throw new DispatchError( @@ -565,7 +513,7 @@ export class Dispatcher { throw new DispatchError("NO_CAPACITY", error.message, { capacityReason }); } } - // The lease path sees only the clamped-to-false permission, so this is the one place that can + // why: The lease path sees only the clamped-to-false permission, so this is the one place that can // tell the caller that config, not missing consent, blocked the download. if ( downloadsPolicy === "never" && @@ -584,24 +532,11 @@ export class Dispatcher { return { result }; }; - /** - * ADR 0004 §1: the one keep-alive, on every transport. An omitted `ttlMs` re-applies the - * lease's own stored width (`LeaseLifecycle#renew`), never `lease.defaultTtlMs`. - */ #leaseRenew: Handler<"lease.renew"> = async (input) => { this.#requireTtlWithinCap(input.ttlMs); return this.options.leases.renew(input.leaseId, input.ttlMs); }; - /** - * ADR 0004 §4's cap, applied identically to a request and a renew. It lives here rather than - * in the contract schema because `lease.maxTtlMs` is a daemon config value, and the contract - * module cannot see one; it lives in the dispatcher rather than in each transport because - * every transport reaches leases through this one shared object (ADR 0003 §2), so HTTP gets - * the same `400 BAD_REQUEST` the socket gets from the same line of code. Rejecting rather - * than clamping is the point: a caller silently given less time than it asked for would go - * on believing it had the time it named. - */ #requireTtlWithinCap(ttlMs: number | undefined): void { const maxTtlMs = this.options.config.lease.maxTtlMs; if (ttlMs !== undefined && ttlMs > maxTtlMs) { @@ -617,13 +552,6 @@ export class Dispatcher { return { leaseId: input.leaseId }; }; - /** - * `killed`, not `explicit`: `docs/internal/EVENTS.md` splits the two by who ended the lease and - * whether its holder asked. An operator's `simlock release --all` (and `nuke`, which already - * reports `killed` through `NukeService`) takes leases away from holders that never asked -- - * which is exactly what a `lease-lost` reader needs to tell apart from the holder's own - * `lease.release` on its way out. - */ #leaseReleaseAll: Handler<"lease.release-all"> = async () => { const leaseIds = await this.options.leases.releaseAll("killed"); return { leaseIds: [...leaseIds] }; @@ -660,7 +588,7 @@ export class Dispatcher { } /** - * Resolution only: the daemon builds the command and never runs it. Spawning it here would + * why: Resolution only: the daemon builds the command and never runs it. Spawning it here would * attach a user's interactive `adb shell` to the daemon's stdio, and the CLI is the process * that actually has a terminal (ADR 0001, decision 7). * @@ -758,25 +686,17 @@ export class Dispatcher { } const handle = this.options.processRunner.spawnStreaming(command.command, command.args, { env: execEnv, - // Returned, not fired and forgotten: whatever the transport hands back is what pauses + // why: Returned, not fired and forgotten: whatever the transport hands back is what pauses // the child until the chunk has actually gone somewhere (ADR 0005 §19e). onChunk: (stream, chunk) => session.onOutput?.(stream, chunk), ...(input.stdin === undefined ? {} : { input: input.stdin }), }); - // Announced before the first chunk can arrive, because "it started" is what a transport + // why: Announced before the first chunk can arrive, because "it started" is what a transport // needs to decide its response shape on -- see `DispatchSession.onStarted`. session.onStarted?.(); return { exitCode: await this.#awaitExec(handle, input.tool, input.args) }; }; - /** - * Waits for an exec'd command, killing it if it outruns `exec.timeoutMs` (ADR 0005 §19e). - * SIGTERM first, SIGKILL after a grace window, because a tool that ignores the first must - * not be able to hold this operation -- and the caller's connection -- open forever; that - * escalation mirrors `NodeProcessRunner#run`'s own. The timeout is reported as - * `EXEC_TIMEOUT` rather than as 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. - */ async #awaitExec( handle: StreamingProcessHandle, tool: string, @@ -790,28 +710,28 @@ export class Dispatcher { }); let killTimer: TimerHandle | undefined; try { - // Raced rather than decided by a flag the timer sets: a command that exits in the same + // why: Raced rather than decided by a flag the timer sets: a command that exits in the same // turn the timer fires has *finished*, and reporting that as a timeout would blame the // limit for a command that met it. `Promise.race` settles on whichever actually // happened first, which is exactly the question. const outcome = await Promise.race([waited, expired]); if (outcome !== EXEC_EXPIRED) return exitCodeOf(outcome); - // SIGTERM first, SIGKILL after a grace window: a tool that ignores the first must not be + // why: SIGTERM first, SIGKILL after a grace window: a tool that ignores the first must not be // able to hold this operation -- and the caller's connection -- open forever. The same // escalation `NodeProcessRunner#run` makes for its own timeout. killQuietly(handle, "SIGTERM"); killTimer = this.options.clock.setTimer(EXEC_SIGKILL_GRACE_MS, () => { killQuietly(handle, "SIGKILL"); }); - // Once the kill is issued, the timeout is the authoritative fact (ADR 0005 §19e) -- + // why: Once the kill is issued, the timeout is the authoritative fact (ADR 0005 §19e) -- // `waited` rejecting with `ExecOutputDeliveryStalledError` (a consumer that stopped // reading is *why* the command outran its timeout in the first place: the pause that // stops it finishing is the same pause that stalls delivery) is a consequence of the // timeout, not a competing answer to it. Swallowed here rather than left to reject // this `await` and skip the throw below. await waited.catch(() => undefined); - // `EXEC_TIMEOUT` rather than the exit code the kill produced: "we stopped it" and "it + // why: `EXEC_TIMEOUT` rather than 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. throw new DispatchError( "EXEC_TIMEOUT", @@ -825,13 +745,11 @@ export class Dispatcher { #doctorRun: Handler<"doctor.run"> = async (input) => { if (this.options.doctor === undefined) throw new DoctorUnavailableError(); - // `purgeOrphans` travels as its own flag all the way down, never folded into `fix`: + // why: `purgeOrphans` travels as its own flag all the way down, never folded into `fix`: // someone already running `doctor --fix` unattended in CI must not acquire a // destructive behaviour by upgrading (ADR 0001, decision 6). return this.options.doctor.reconcile({ fix: input.fix ?? false, - // Only an operator's `doctor` looks at the machine's prerequisites; startup convergence - // runs no tool. prerequisites: true, purgeOrphans: input.purgeOrphans ?? false, }); @@ -897,16 +815,6 @@ export class Dispatcher { #configGet: Handler<"config.get"> = () => this.options.config; - /** - * ADR 0012 §1: this host as its own fleet of one. The fields a gateway reads over the uplink - * come from the same four reads, made here against this dispatcher's own handlers; the - * contract's one builder turns them into a view. The parses only turn the handlers' core - * records into the wire types the builder takes: `worker.list`'s own output schema, applied - * by `dispatch()`, is what bounds and narrows the view on the wire. The catalog is the kept - * one (see `#viewCatalog`); status and devices are read on every call, as a gateway reads - * them on every worker event. `drained` is always `false`: a gateway that drained this worker - * holds that flag, not this host. - */ #workerList: Handler<"worker.list"> = async (_input, session) => { // why: taken as status resolves, not after the slower catalog read, so the load is not made to look fresher. let receivedAt = this.options.clock.now(); @@ -940,12 +848,6 @@ export class Dispatcher { }; }; - /** - * The kept catalog for `worker.list`, read again once it is older than the interval. The age - * is wall-clock time, so one that is negative (the clock was set back) counts as stale rather - * than keeping the catalog for as long as the clock went back. A failed read drops whatever is - * kept, at worst one read newer than it, which costs one more read. - */ #keptViewCatalog(session: DispatchSession): Promise { const now = this.options.clock.now(); const kept = this.#viewCatalog; @@ -966,13 +868,6 @@ export class Dispatcher { return catalog; } - /** - * ADR 0010 §4 and §6: an operator's explicit install. The command itself is the consent, so the - * one consent function is asked with `true`; only `downloads.policy: "never"` refuses it, and it - * does so before the installer is reached. Everything else -- joining an install already - * running, waiting a platform's turn, the disk reservation, the budget, the events -- is the - * installer's, exactly as for a lease request's download. - */ #componentInstall: Handler<"component.install"> = async (input, session) => { if (!effectiveAllowDownload(this.options.config.downloads.policy, true)) { throw new DispatchError( @@ -992,7 +887,7 @@ export class Dispatcher { : { onProgress: (progress) => onComponentProgress(toWireProgress(progress)) }), ...(onStarted === undefined ? {} : { onAdmitted: onStarted }), }); - // No `stillNeeded` is passed, so `not-needed` cannot come back; a version is always present + // why: No `stillNeeded` is passed, so `not-needed` cannot come back; a version is always present // for the two outcomes that can. if (result.outcome === "not-needed" || result.version === undefined) { throw new Error(`Internal: component install ended as ${result.outcome} with no version`); @@ -1005,16 +900,10 @@ export class Dispatcher { }; }; - /** ADR 0010 §8: a read for any session; the installer merges the drivers and the registry. */ #componentList: Handler<"component.list"> = async (input) => ({ components: await this.options.components.list(input.platform), }); - /** - * ADR 0010 §8: an operator's removal of a component Simlock installed. Every check and the - * removal itself are the installer's; this names the admin principal for `component.removed` - * (safety rule 6) and puts the in-use counts on the wire as `COMPONENT_IN_USE`'s details. - */ #componentRemove: Handler<"component.remove"> = async (input, session) => { try { const result = await this.options.components.remove({ @@ -1149,7 +1038,7 @@ export class Dispatcher { } /** - * The installer's progress in the contract's words: its percentage, already within 0..100, + * why: The installer's progress in the contract's words: its percentage, already within 0..100, * becomes a fraction from 0 to 1, rounded to three decimals so that 1.1 percent is 0.011 and not * the 0.011000000000000001 a plain division gives. Only 100 percent becomes 1: 99.96 would round * up to it, and 1 is what an install says once it is complete. With none (the install has just @@ -1162,14 +1051,12 @@ function toWireProgress(progress: ComponentInstallerProgress): ComponentProgress return { fraction: progress.percent < 100 ? Math.min(fraction, 0.999) : 1, stage: "downloading" }; } -/** A child that exited between the timer firing and the signal landing is not an error worth +/** why: A child that exited between the timer firing and the signal landing is not an error worth * failing the operation over -- the wait below is about to report how it ended anyway. */ function killQuietly(handle: StreamingProcessHandle, signal: NodeJS.Signals): void { try { handle.kill(signal); - } catch { - // Already gone. - } + } catch {} } function describeEnrollmentError(error: unknown): unknown { diff --git a/src/daemon/main.test.ts b/src/daemon/main.test.ts index f6c560251..47bdc9261 100644 --- a/src/daemon/main.test.ts +++ b/src/daemon/main.test.ts @@ -94,8 +94,6 @@ const STANDALONE_OFF = { http: { enabled: false }, } as const; -/** `drivers: undefined` is spelled out by a caller that wants real (module-substituted) - * discovery rather than the fake driver below. */ async function start( overrides: Partial> & { readonly deviceDataTables?: StartDaemonOptions["deviceDataTables"] | undefined; @@ -409,7 +407,6 @@ describe("the event history", () => { it("judges retention by the injected clock, so an event younger than the retention on that clock survives the start-up prune", async () => { const directory = await mkdtemp(join(tmpdir(), "simlock-main-")); temporaryDirectories.push(directory); - // The fake clock reads 1_000; a real clock would put the 7-day cutoff far past 500. await seedHistory(directory, [500]); await start({ dataDirectory: directory }); @@ -446,7 +443,7 @@ async function queryState( } } -/** A gateway URL on a port just released. Nothing listens there, so a worker's uplink dial +/** why: A gateway URL on a port just released. Nothing listens there, so a worker's uplink dial * fails fast, and the fake clock never fires its retry. */ async function unreachableGatewayUrl(): Promise { const probe = createServer(); @@ -491,7 +488,6 @@ async function writeModule(contents: string): Promise { return modulePath; } -/** A driver whose disposal is observable, which `FakeDriver` deliberately is not. */ class DisposableFakeDriver extends FakeDriver { constructor( private readonly disposed: string[], @@ -517,7 +513,6 @@ async function agentStatus(daemon: DaemonServer) { )) as { readonly host: { readonly tools: readonly unknown[] } }; } -/** A fake driver that reports tool versions, which `FakeDriver` does not. */ class ToolsFakeDriver extends FakeDriver { constructor( options: ConstructorParameters[0], @@ -532,7 +527,7 @@ class ToolsFakeDriver extends FakeDriver { } /** - * Binds an ephemeral loopback port and releases it. A fixed port fails whenever two runs of + * why: Binds an ephemeral loopback port and releases it. A fixed port fails whenever two runs of * this file overlap -- two worktrees running the suite at once, or Stryker's parallel runners * -- and another run's daemon can even answer on it. */ @@ -565,7 +560,7 @@ describe("startDaemon", () => { readonly gateway: { readonly token?: string }; }; - // The fixture is real: the daemon did log its start and emit `daemon.started` to both + // why: The fixture is real: the daemon did log its start and emit `daemon.started` to both // the ring and the file, so "absent" below means redacted, not never written. expect(sink.records.some((record) => record.message === "Daemon started")).toBe(true); expect(eventFile).toContain('"daemon.started"'); @@ -634,13 +629,12 @@ describe("startDaemon", () => { role: "admin", } as const; - // doctor.run parks until startup convergence has finished, so by the time it answers, + // why: doctor.run parks until startup convergence has finished, so by the time it answers, // convergence's own reconcile has run -- and must not have counted. const first = (await daemon.dispatch("doctor.run", {}, admin)) as { readonly findings: readonly { readonly kind: string; readonly prerequisite?: string }[]; }; expect(runs).toEqual({ android: 1, ios: 1 }); - // Android has no driver in this daemon, so the finding is the platform's, not running. expect(first.findings).toContainEqual( expect.objectContaining({ kind: "prerequisite-missing", platform: "android" }), ); @@ -801,8 +795,6 @@ describe("startDaemon", () => { .catch(() => undefined); } - // With the default cap both failures stay stored; with an unclassified failure the code - // would be INTERNAL. await expect .poll(() => queryState(directory, "SELECT requester_id, failure_code FROM lease_requests")) .toEqual([{ failure_code: "RUNTIME_MISSING", requester_id: "second" }]); @@ -821,14 +813,14 @@ describe("startDaemon", () => { await first.daemon.stop("test"); await start({ dataDirectory: directory, filesystem }); - // Never regenerated: a second id would strand every device already sitting in a root + // why: Never regenerated: a second id would strand every device already sitting in a root // marked with the first one (ADR 0001, decision 2). expect(written.instanceId).not.toBe(""); expect(JSON.parse(await filesystem.readFile(identityPath))).toEqual(written); }); it("disposes every driver on shutdown, and reports the one that failed without stopping", async () => { - // The wiring, not the halves: `DaemonServer` awaiting its `dispose` option is tested in + // why: The wiring, not the halves: `DaemonServer` awaiting its `dispose` option is tested in // `server.test.ts` and a driver reaping its adb server in the Android driver's own, and // between them sat the composition that connects the two. Losing it silently abandons // Simlock's adb server on every shutdown, and only `Driver.dispose` can stop one. @@ -911,10 +903,8 @@ describe("startDaemon", () => { const clock = new FakeClock(1_000); const { daemon } = await start({ drivers: [ - new ToolsFakeDriver( - { availableOsVersions: ["26.5"], clock, platform: "ios" }, - // What an iOS driver whose `xcodebuild` fails looks like from here. - () => Promise.reject(new Error("xcodebuild -version exited with 1")), + new ToolsFakeDriver({ availableOsVersions: ["26.5"], clock, platform: "ios" }, () => + Promise.reject(new Error("xcodebuild -version exited with 1")), ), new ToolsFakeDriver({ availableOsVersions: ["35"], clock, platform: "android" }, () => Promise.resolve([{ name: "emulator", version: "35.4.9" }]), @@ -1024,7 +1014,7 @@ describe("startDaemon wires core and leasing together", () => { readonly device: { readonly id: string }; }; - /** Lets the work a mutated daemon would still be doing finish, so "nothing happened" is a + /** why: Lets the work a mutated daemon would still be doing finish, so "nothing happened" is a * statement about the daemon and not about how early the test looked. Only sound when that * work is all in memory: a timer fires after it however loaded the machine is, but real I/O * (a process spawn, a real file) can outlast it. */ @@ -1178,7 +1168,6 @@ describe("startDaemon wires core and leasing together", () => { const clock = new FakeClock(1_000); const directory = await mkdtemp(join(tmpdir(), "simlock-main-")); temporaryDirectories.push(directory); - // The previous process's registry: one request still open when it stopped. const state = await openStateDatabase({ dataDirectory: directory, filesystem: new MemoryFilesystem(), @@ -1320,7 +1309,6 @@ describe("startDaemon wires core and leasing together", () => { }); await daemon.dispatch("lease.release", { leaseId: grant.lease.id }, agent); - // Well past the stalled-transition threshold, but the erase holds its claim on the device. clock.advance(300_000); const devices = (await daemon.dispatch("list.get", { kind: "devices" }, admin)) as readonly { readonly id: string; @@ -1367,7 +1355,6 @@ describe("startDaemon wires core and leasing together", () => { const grant = (await started.daemon.dispatch("lease.request", ios, agent)) as Grant; await started.daemon.dispatch("lease.release", { leaseId: grant.lease.id }, agent); - // Shutdown waits for the failed purge to settle into quarantine, which arms the retry. await started.daemon.stop("test"); clock.advance(30_000); await settleRealTime(); @@ -1457,11 +1444,6 @@ describe("startDaemon wires core and leasing together", () => { }); describe("startDaemon startup readiness", () => { - // Reproduces the issue #41 symptom end-to-end through the real startDaemon wiring: - // doctor.reconcile() shells out to driver.listManaged() per driver, which is where - // real convergence time is lost. Here it's held open on the FakeClock so the test - // can prove the socket answers hello/status.get ("starting"), parks lease.request, - // and only then converges -- without waiting on a real clock. it("claims the socket and answers hello/status.get while doctor.reconcile is still in flight", async () => { const directory = await mkdtemp(join(tmpdir(), "simlock-main-slow-")); temporaryDirectories.push(directory); @@ -1506,7 +1488,7 @@ describe("startDaemon startup readiness", () => { void parkedLease.then(() => { leaseSettled = true; }); - // Round-tripping another status.get proves the parked request was already + // why: Round-tripping another status.get proves the parked request was already // dispatched (and is awaiting the readiness gate) without relying on a timer. await client.request("status.get", {}); expect(leaseSettled).toBe(false); @@ -1559,7 +1541,6 @@ describe("startDaemon stopped while starting", () => { protocolVersion: DAEMON_PROTOCOL_VERSION, }); expect((await client.request("daemon.stop", {})).ok).toBe(true); - // `daemon.stop` answers first and tears down after, so give the teardown real time to begin. await new Promise((resolve) => setTimeout(resolve, 25)); } finally { client.socket.end(); @@ -1573,11 +1554,6 @@ describe("startDaemon stopped while starting", () => { }); describe("startDaemon HTTP gateway startup readiness", () => { - // ADR 0003 §2: "an HTTP request during startup now waits like a socket request instead of - // being refused." Mirrors the socket-side test above (same FakeDriver latency trick to hold - // convergence open on the FakeClock), but drives a real HTTP request instead of a socket - // frame -- proving the gateway is actually listening (and its request actually parks) before - // convergence finishes, not just that `dispatch()` would park it in principle. it("parks an HTTP request until convergence completes, instead of refusing the connection", async () => { const directory = await mkdtemp(join(tmpdir(), "simlock-main-http-slow-")); temporaryDirectories.push(directory); @@ -1607,7 +1583,7 @@ describe("startDaemon HTTP gateway startup readiness", () => { return daemon; }); - // Polls rather than a fixed delay: the gateway binds asynchronously (`onSocketClaimed` + // why: Polls rather than a fixed delay: the gateway binds asynchronously (`onSocketClaimed` // fires once the socket claim resolves, then `gateway.start()` itself awaits a real // `listen()`), so there is no single microtask boundary to await here the way the // socket-side test above can just retry-connect on the unix socket. @@ -1621,8 +1597,6 @@ describe("startDaemon HTTP gateway startup readiness", () => { void parkedStatusPromise.then(() => { settled = true; }); - // Give the parked request's own microtasks a chance to run before asserting it hasn't -- - // it must still be awaiting `dispatch()`'s startup-readiness gate, not merely slow. await new Promise((resolve) => setTimeout(resolve, 20)); expect(settled).toBe(false); @@ -1900,7 +1874,7 @@ describe("startDaemon stop with a warm target", () => { .poll(() => driver.calls.filter((call) => call.operation === "makeReady").length) .toBe(1); - // The creation ends while the HTTP gateway is still being stopped: a pool not closed by then + // why: The creation ends while the HTTP gateway is still being stopped: a pool not closed by then // would start the second one, and the drain would wait for its whole boot. const stopping = daemon.stop("test-stop-target"); driver.releaseMakeReady(); @@ -1911,24 +1885,6 @@ describe("startDaemon stop with a warm target", () => { }); describe("startDaemon HTTP gateway stop-during-start race (review finding S5)", () => { - // Before this fix, `stopAuxiliary` (this file's `stopAuxiliary` closure passed to - // `DaemonServer`) returned immediately whenever the concurrently-started HTTP gateway - // (`onSocketClaimed` -> `startHttpGateway()`) hadn't finished its own `listen()` yet -- - // `stopHttpGateway` isn't assigned until it does. `#stop()` (server.ts) awaits - // `stopAuxiliary()` first, so it would go on to release leases, settle, and dispose while the - // gateway was still binding, then start accepting HTTP requests -- against a torn-down engine - // -- once its own `listen()` finally resolved. The fix makes `stopAuxiliary` await - // `gatewayStarted` (settled only once the gateway's own bind attempt finishes, one way or the - // other) before doing anything else. - // - // The window this reproduces (`onSocketClaimed` firing to the gateway's `listen()` actually - // resolving) is normally a handful of real milliseconds -- reachable, per the review, via - // `simlock daemon stop` during startup, but not something a test can land inside reliably by - // just racing real I/O against real I/O. So this test holds the window open deterministically - // instead: it patches `net.Server.prototype.listen` (restored in `finally`) to delay only a - // TCP bind (the gateway's) by 200ms, leaving the daemon's own Unix-socket `listen()` (a string - // first argument, not a port) untouched -- so the socket claims and answers `daemon.stop` - // long before the patched gateway bind is even allowed to start. it("never lets the HTTP gateway outlive full teardown when daemon.stop lands while the gateway is still mid-bind", async () => { const directory = await mkdtemp(join(tmpdir(), "simlock-main-http-race-")); temporaryDirectories.push(directory); @@ -1952,14 +1908,8 @@ describe("startDaemon HTTP gateway stop-during-start race (review finding S5)", socketPath, version: "1.2.3", } as StartDaemonOptions); - // `stop()` is idempotent, so it is safe for `afterEach` to also stop whatever this - // resolves to (it will, once the delayed gateway bind above finishes settling). void startPromise.then((daemon) => runningDaemons.push(daemon)).catch(() => undefined); - // Connects and authenticates as admin, then sends `daemon.stop` -- comfortably inside the - // 200ms window the patch above holds the gateway's own bind open for. The admin secret is - // written (`AdminSecretManager#persist`) before `onSocketClaimed` fires, so it is already - // there by the time the socket itself is reachable. const client = await connectRetrying(socketPath); const secret = (await readFileRetrying(filesystem, join(directory, "admin.token"))).trim(); await client.request("hello", { @@ -1971,8 +1921,6 @@ describe("startDaemon HTTP gateway stop-during-start race (review finding S5)", expect(stopReply.ok).toBe(true); client.socket.end(); - // Lets the delayed gateway bind actually fire and settle (one way or the other) before - // this test reads the log for it. await new Promise((resolve) => setTimeout(resolve, 400)); const stoppingIndex = sink.records.findIndex( @@ -1985,20 +1933,11 @@ describe("startDaemon HTTP gateway stop-during-start race (review finding S5)", const gatewayListeningIndex = sink.records.findIndex( (record) => record.message === "HTTP gateway listening", ); - // The gateway actually got to bind (it does here -- the port is free and 200ms is ample - // real time for a loopback `listen()`), so this asserts the meaningful case directly - // rather than treating it as merely possible: it bound, and it was stopped, and both - // happened strictly before "Daemon stopping" (logged only once `stopAuxiliary` resolves -- - // see `server.ts#stop`). Before the fix, `stoppingIndex` would be *smaller* than both of - // these -- `stopAuxiliary` returned immediately, so "Daemon stopping" logged right away, - // well before the (patch-delayed) gateway bind even started. expect(gatewayListeningIndex).toBeGreaterThanOrEqual(0); expect(gatewayListeningIndex).toBeLessThan(stoppingIndex); expect(gatewayStoppedIndex).toBeGreaterThanOrEqual(0); expect(gatewayStoppedIndex).toBeLessThan(stoppingIndex); - // And the port must never be reachable once the daemon has finished stopping -- the - // gateway did not outlive teardown just because it bound after `stop()` was requested. await expect( fetch(`http://127.0.0.1:${port}/v1/status`, { headers: { authorization: "Bearer irrelevant" }, @@ -2012,12 +1951,6 @@ describe("startDaemon HTTP gateway stop-during-start race (review finding S5)", }); describe("startDaemon socket race with HTTP enabled", () => { - // Review finding V1: `gatewayStarted` is settled only from inside `onSocketClaimed`'s - // handler, and `start()` fires that callback only after the socket claim succeeds. A daemon - // that loses the start race therefore rejected without the callback ever running, so nothing - // settled `gatewayStarted` and `Promise.allSettled` waited on it forever -- `startDaemon()` - // hung instead of reporting the lost race, with no rejection and no non-zero exit code. - // Reachable by racing `simlock daemon start`, or by the CLI's own auto-launch. it("rejects rather than hanging when the socket is already claimed", async () => { const directory = await mkdtemp(join(tmpdir(), "simlock-main-socket-race-")); temporaryDirectories.push(directory); @@ -2046,7 +1979,6 @@ describe("startDaemon socket race with HTTP enabled", () => { const first = await startDaemon(options(await freeLoopbackPort())); try { - // A distinct port, so the only thing that can fail is the socket claim itself. const second = startDaemon(options(await freeLoopbackPort())); const outcome = await Promise.race([ second.then( @@ -2063,7 +1995,7 @@ describe("startDaemon socket race with HTTP enabled", () => { }); describe("startDaemon gateway uplink when the socket claim fails", () => { - // #395: the uplink is dialled from `onSocketClaimed`, so a daemon that loses the claim never + // why: #395: the uplink is dialled from `onSocketClaimed`, so a daemon that loses the claim never // dials its gateway -- it would otherwise join as a worker it is about to stop being. it("never dials the gateway when the worker's socket is already claimed", async () => { const directory = await mkdtemp(join(tmpdir(), "simlock-main-uplink-claim-")); @@ -2107,7 +2039,6 @@ describe("startDaemon gateway uplink when the socket claim fails", () => { () => "resolved" as const, () => "rejected" as const, ); - // The dial, were it made, reaches a loopback listener within milliseconds. await new Promise((resolve) => setTimeout(resolve, 500)); expect({ dials, outcome }).toEqual({ dials: [], outcome: "rejected" }); } finally { @@ -2118,8 +2049,6 @@ describe("startDaemon gateway uplink when the socket claim fails", () => { }); describe("startDaemon with HTTP off", () => { - // #395: the socket-claim callback now runs with HTTP off too (it dials the uplink), so it - // must still leave the HTTP frontend unstarted. it("starts no HTTP gateway when http.enabled is false", async () => { const port = await freeLoopbackPort(); await start({ configOverrides: { http: { enabled: false, host: "127.0.0.1", port } } }); @@ -2137,12 +2066,6 @@ describe("startDaemon with HTTP off", () => { }); describe("startDaemon HTTP gateway bind failure", () => { - // Review finding B6: before this fix, an HTTP bind failure (occupied port) logged and - // stopped the daemon from inside `onSocketClaimed`'s handler without `startDaemon()` itself - // ever seeing it -- the daemon's own `start()` would go on to resolve successfully once - // convergence finished, so the CLI reported success (exit code 0) with no socket, no HTTP, - // and no daemon actually running. This proves `startDaemon()` now rejects instead, and that - // the daemon never reports having started after it already reported stopping. it("rejects startDaemon() when the configured HTTP port is already in use, without emitting a started record after a stopping one", async () => { const directory = await mkdtemp(join(tmpdir(), "simlock-main-http-bind-")); temporaryDirectories.push(directory); @@ -2174,10 +2097,6 @@ describe("startDaemon HTTP gateway bind failure", () => { const startedIndex = sink.records.findIndex((record) => record.message === "Daemon started"); const stoppingIndex = sink.records.findIndex((record) => record.message === "Daemon stopping"); expect(stoppingIndex).toBeGreaterThanOrEqual(0); - // Either "Daemon started" never appears (the common case: the bind failure is discovered - // and the whole thing torn down without convergence ever seeing readiness), or -- if - // convergence happened to finish first -- it appears strictly before "Daemon stopping", - // never after (ADR events rule 3: a fact must be true when emitted). if (startedIndex >= 0) { expect(startedIndex).toBeLessThan(stoppingIndex); } @@ -2343,7 +2262,7 @@ describe("discoverDrivers", () => { const { drivers, rejections } = await discoverIos({ hostPlatform: "linux" }); expect(drivers.some((driver) => driver.platform === "ios")).toBe(false); - // Nothing refused anything here: a host without Xcode is not a fail-closed refusal + // why: Nothing refused anything here: a host without Xcode is not a fail-closed refusal // and must not be reported as one. expect(rejections).toEqual([]); }); @@ -2355,7 +2274,7 @@ describe("discoverDrivers", () => { }); it("builds both prerequisite checks on macOS, whether or not a driver started", async () => { - // The refused root costs the iOS driver; it must not cost the check that explains it. + // why: The refused root costs the iOS driver; it must not cost the check that explains it. const { drivers, prerequisiteChecks } = await discoverIos({ filesystem: await foreignOwnedRoot(), }); @@ -2390,7 +2309,7 @@ describe("discoverDrivers", () => { const { drivers, rejections } = await discoverIos({ filesystem }); - // Safety rule 9: the platform goes, the daemon stays -- a daemon that will not start + // why: Safety rule 9: the platform goes, the daemon stays -- a daemon that will not start // is a daemon that cannot tell anyone why. expect(drivers.some((driver) => driver.platform === "ios")).toBe(false); expect(rejections.map((rejection) => rejection.platform)).toEqual(["ios"]); @@ -2402,7 +2321,7 @@ describe("discoverDrivers", () => { const { rejections } = await discoverIos({ filesystem }); - // Built from a real `OwnedRootError`, key by key: the payload is a wire contract + // why: Built from a real `OwnedRootError`, key by key: the payload is a wire contract // `simlock events --json` publishes, so renaming `root` has to fail here. expect(rejections[0]).toEqual({ event: "driver.root-rejected", @@ -2418,7 +2337,7 @@ describe("discoverDrivers", () => { }); it("still fails startup when the iOS driver fails for a reason that is not a refusal", async () => { - // Anything that is not an `OwnedRootError` is a bug rather than a fail-closed + // why: Anything that is not an `OwnedRootError` is a bug rather than a fail-closed // decision, and swallowing it would hide it behind a silently missing platform. await expect( discoverIos({ @@ -2449,7 +2368,7 @@ describe("discoverDrivers on a host with an Android SDK", () => { const { drivers, rejections } = await discoverAndroid(filesystem, new FakeTcpProbe([5038])); expect(drivers.some((driver) => driver.platform === "android")).toBe(false); - // The payload is a wire contract `simlock events --json` publishes for + // why: The payload is a wire contract `simlock events --json` publishes for // `driver.adb-server-rejected`, so a renamed key has to fail here. expect(rejections[0]).toMatchObject({ event: "driver.adb-server-rejected", @@ -2464,7 +2383,6 @@ describe("discoverDrivers on a host with an Android SDK", () => { const filesystem = await androidSdk(); const probe = new FakeTcpProbe([5038]); await filesystem.mkdirp(SIMLOCK_HOME); - // The server a previous daemon left behind, with the pid that proves it is Simlock's. await filesystem.writeFileAtomic( join(SIMLOCK_HOME, "adb-server.json"), JSON.stringify({ pid: 4242, port: 5038, startedAt: 1 }), @@ -2503,8 +2421,6 @@ describe("discoverDrivers on a host with an Android SDK", () => { join(SIMLOCK_HOME, "adb-server.json"), JSON.stringify({ pid: 4242, port: 5038, startedAt: 1 }), ); - // Only the console-port lookup a boot starts with is scripted: the emulator launch after it - // is recorded and then refused, so the boot stops right after showing its argv. const processRunner = new ScriptedProcessRunner([ { match: { args: ["devices"], command: "/android-sdk/platform-tools/adb" }, @@ -2644,7 +2560,6 @@ describe("discoverDrivers on a host with an Android SDK", () => { ]); }); - /** The minimum layout `discoverSdk` accepts, in memory. */ async function androidSdk(): Promise { const filesystem = new MemoryFilesystem(); process.env.ANDROID_HOME = "/android-sdk"; @@ -2697,7 +2612,6 @@ const SIMLOCK_HOME = "/home/.simlock"; const IOS_ROOT = join(SIMLOCK_HOME, "devices", "ios"); const INSTANCE_ID = "instance-1"; -/** A root marked by a different Simlock instance -- the cheapest real `OwnedRootError`. */ async function foreignOwnedRoot(): Promise { const filesystem = new MemoryFilesystem(); await filesystem.mkdirp(IOS_ROOT); @@ -2713,7 +2627,6 @@ async function foreignOwnedRoot(): Promise { return filesystem; } -/** The very `OwnedRootError` the driver raises for that root, never a hand-written copy. */ async function refusedRoot(filesystem: MemoryFilesystem): Promise { try { await IosSimctlDriver.create({ @@ -2731,7 +2644,6 @@ async function refusedRoot(filesystem: MemoryFilesystem): Promise { it("writes no daemon log line for component.installed and device.slimmed", async () => { const directory = await mkdtemp(join(tmpdir(), "simlock-main-event-file-")); temporaryDirectories.push(directory); - // The daemon's own bus is reachable only through what it hands discovery. process.env.SIMLOCK_DRIVERS_MODULE = await writeModule( `export function createDrivers({ eventBus }) { eventBus.emit("component.installed", { componentId: "18.6", durationMs: 1, platform: "ios" }, "test"); @@ -3098,7 +3009,6 @@ describe("discoverDrivers with SIMLOCK_DRIVERS_MODULE", () => { }); it("a throwing event subscriber writes one JSON line through the logger and nothing through console.error", async () => { - // The drivers module is the one seam that reaches the daemon's own bus from outside. process.env.SIMLOCK_DRIVERS_MODULE = await writeModule( `export function createDrivers(context) { context.eventBus.subscribe("daemon.started", () => { @@ -3135,7 +3045,6 @@ describe("discoverDrivers with SIMLOCK_DRIVERS_MODULE", () => { `export function createDrivers() { return []; }`, ); - // No `prerequisiteChecks` export: `doctor` runs no check rather than the real ones. await expect(discover(new MemoryLogSink())).resolves.toEqual({ drivers: [], prerequisiteChecks: [], @@ -3168,7 +3077,6 @@ interface MinimalClient { ): Promise<{ readonly ok: boolean; readonly payload?: unknown }>; } -/** Minimal newline-delimited-JSON client, mirroring the real daemon protocol framing. */ function connectClient(socketPath: string): Promise { return new Promise((resolve, reject) => { const socket = connect(socketPath); @@ -3218,7 +3126,7 @@ async function connectRetrying(socketPath: string, timeoutMs = 2_000): Promise { throw Object.assign(new Error("statfs failed"), { code: "EIO" }); @@ -3321,7 +3225,7 @@ describe("startDaemon logger wiring", () => { it("startDaemon hands its logger to the cleanup reaper: a scheduled cleanup run that fails is logged", async () => { const { sink } = await start({ filesystem: new UnmeasurableDiskFilesystem() }); - // `daemon.started` triggers a run nobody awaits; its failure has no caller to reach. + // why: `daemon.started` triggers a run nobody awaits; its failure has no caller to reach. await pollUntil(() => sink.records.some((record) => record.message === "scheduled cleanup run failed"), ); @@ -3337,7 +3241,7 @@ describe("startDaemon logger wiring", () => { }); it("startDaemon hands the gateway dispatcher its error classifier: a typed refusal is logged with its code at info, not as INTERNAL at error", async () => { - // A gateway serves HTTP; on its default port two overlapping runs of this file collide. + // why: A gateway serves HTTP; on its default port two overlapping runs of this file collide. const { daemon, sink } = await start({ configOverrides: { http: { enabled: true, host: "127.0.0.1", port: await freeLoopbackPort() }, @@ -3372,8 +3276,6 @@ describe("startDaemon logger wiring", () => { }); }); -/** Polls until `condition` holds or the time runs out, without failing: the caller's own - * assertion afterwards is what names a miss. */ async function pollUntil(condition: () => boolean, timeoutMs = 2_000): Promise { const deadline = Date.now() + timeoutMs; while (!condition() && Date.now() < deadline) { diff --git a/src/daemon/main.ts b/src/daemon/main.ts index 61fc3738e..3540b583e 100644 --- a/src/daemon/main.ts +++ b/src/daemon/main.ts @@ -238,7 +238,6 @@ function updateOptionsFor( }; } -/** Constructs the daemon's real adapters once; all state remains in the daemon. */ export async function startDaemon(options: StartDaemonOptions = {}): Promise { let opened: StateDatabase | undefined; let check: VersionCheck | undefined; @@ -310,7 +309,7 @@ async function composeDaemon( for (const message of configWarnings) logger.warn(message); if (dataDirectoryWarning !== undefined) logger.warn(dataDirectoryWarning); const processRunner = processRunnerFor(config.log.level, baseProcessRunner, logger, clock); - // ADR 0008 §6: the machine is read once, here. Bounded and never throwing, so a slow or + // why: ADR 0008 §6: the machine is read once, here. Bounded and never throwing, so a slow or // missing `sw_vers` costs a fallback value, not the daemon. const hostSystem = await ( options.hostInfo ?? new NodeHostInfo({ platform: process.platform, processRunner }) @@ -357,10 +356,6 @@ async function composeDaemon( secrets: new CryptoTokenSecrets(), }); const auth = { ...created, adminSecret: reportingAdminSecret(created.adminSecret, options) }; - // ADR 0005 §1/§2: one process, one mode. A gateway starts no drivers, validates no device - // roots, loads no registry, and runs no reaper, health monitor or capacity strategy -- so the - // branch is here, before any of that is built, rather than as a set of conditionals threaded - // through the worker's composition below. const npm = options.npm ?? new NodeNpmCli({ runner: baseProcessRunner, filesystem, dataDirectory }); const versionCheck = startVersionCheck({ @@ -408,7 +403,7 @@ async function composeDaemon( }, state, }); - // Before discovery, because every root a driver validates is checked against it, and it + // why: Before discovery, because every root a driver validates is checked against it, and it // is written exactly once per home and never regenerated (ADR 0001, decision 2). const instanceId = await loadInstanceId({ filesystem, @@ -441,8 +436,6 @@ async function composeDaemon( prerequisiteChecks: options.prerequisiteChecks ?? [], rejections: [], }; - // ADR 0008 §7: read at startup in the background, served from memory, re-read when stale. - // Nothing waits on the first read; `status.get` reports no tools until it lands. const hostFacts = new HostFactsReader({ clock, drivers, @@ -458,10 +451,7 @@ async function composeDaemon( hostLoadProbe === null ? undefined : new HostLoadSampler({ clock, logger: logger.child("host-load"), probe: hostLoadProbe }); - // One gate for every registry write: leasing's, core's and the installer's records. const decisions = new SerializedDecision(); - // The one caller of `Driver.installComponent` (ADR 0010 §3). Its `DiskSpaceGuard` is the only - // one, so an iOS and an Android install see each other's reservations. const components = new ComponentInstaller({ clock, decisions, @@ -528,7 +518,6 @@ async function composeDaemon( tokens: auth.tokens, role: "worker", }); - // Builds leasing on top of core (ADR 0018 §2). const leasing = createLeasing({ clock, components, @@ -543,7 +532,7 @@ async function composeDaemon( modelPreferences, }); cancelPendingRequest = (requesterId) => leasing.cancelPending(requesterId); - // Core declared ports only leasing can implement; this is the one place they are handed over, + // why: Core declared ports only leasing can implement; this is the one place they are handed over, // before any request is admitted (ADR 0018 §2). core.connect(leasing.corePorts); const reaper = new CleanupReaper({ @@ -558,11 +547,6 @@ async function composeDaemon( targetedDevices: () => core.targetedDevices(), }); const nuke = new Nuke({ executor: core.nuke, registry }); - /** - * ADR 0005 §3/§4: two config keys join a fleet. Undefined here means this worker has no - * gateway, which is every worker today -- and the daemon is otherwise completely unchanged - * either way, which is the point of the uplink being outbound. - */ const gatewayUplink = config.gateway.url === undefined || config.gateway.token === undefined ? undefined @@ -576,11 +560,11 @@ async function composeDaemon( workerId: instanceId, ...(config.gateway.label === undefined ? {} : { label: config.gateway.label }), }); - // Reassigned once the HTTP frontend actually starts, but must exist as a stable closure now: + // why: Reassigned once the HTTP frontend actually starts, but must exist as a stable closure now: // `stopAuxiliary` is fixed at `DaemonServer` construction time, before the frontend itself // exists. let stopHttpGateway: (() => Promise) | undefined; - // Settles once the HTTP gateway either finishes starting or fails to -- resolved/rejected from + // why: Settles once the HTTP gateway either finishes starting or fails to -- resolved/rejected from // inside `onSocketClaimed`'s handler below. `startDaemon()` awaits this alongside // `daemon.start()` itself (see the bottom of this function) so a bind failure (occupied port, // invalid host) makes `startDaemon()` reject and the entrypoint report a non-zero exit code, @@ -594,8 +578,6 @@ async function composeDaemon( // immediately. let resolveGatewayStarted: (() => void) | undefined; let rejectGatewayStarted: ((error: unknown) => void) | undefined; - /** Whether `onSocketClaimed` ever fired, i.e. whether anything will ever settle - * `gatewayStarted`. See the `finally` on `daemon.start()` below. */ let socketClaimed = false; const gatewayStarted: Promise = config.http.enabled ? new Promise((resolve, reject) => { @@ -603,7 +585,7 @@ async function composeDaemon( rejectGatewayStarted = reject; }) : Promise.resolve(); - // ADR 0019 §1, in this order: settle the open requests, read each platform once, doctor's + // why: ADR 0019 §1, in this order: settle the open requests, read each platform once, doctor's // startup pass on that read, end every lease whose device is not running and restore the // timers of the rest, then core's device convergence, then the queue's first depth, which every // run begins with. One after another: the reconciler needs the read, and convergence must not @@ -640,7 +622,7 @@ async function composeDaemon( leasing.announceQueueDepth(); }; const daemon = new DaemonServer({ - // Closes the warm pool the moment a stop is asked for, ahead of every await in the stop. + // why: Closes the warm pool the moment a stop is asked for, ahead of every await in the stop. beginStop: () => core.closeWarmPool(), // why: the source's name is the config key's; the core names no module, so the composition supplies it. capabilitySources: [...moduleHost.capabilitySources], @@ -712,12 +694,12 @@ async function composeDaemon( await leasing.settle(); await core.drain(); }, - // Every driver is tried even when another throws: Android's disposal is the only thing that can stop the adb server it started. + // why: Every driver is tried even when another throws: Android's disposal is the only thing that can stop the adb server it started. dispose: async () => { versionCheck.dispose(); core.dispose(); leasing.dispose(); - // Before the drivers: a running install is ended through its signal while its driver + // why: Before the drivers: a running install is ended through its signal while its driver // can still stop the installer process. await components.close(); await moduleHost.beforeDriversDispose(); @@ -733,7 +715,7 @@ async function composeDaemon( await eventHistory.stop(); state.close(); }, - // Review finding S5: waits for the concurrently-started gateway to either finish binding + // why: Review finding S5: waits for the concurrently-started gateway to either finish binding // (so `stopHttpGateway` is assigned and can actually be called) or fail to bind (nothing to // stop) *before* returning -- `#stop()` awaits this call before settling and disposing // (see `server.ts`; the leases themselves are left alone, ADR 0004 §3), so by the time @@ -746,13 +728,13 @@ async function composeDaemon( await moduleHost.beforeDrain(); await gatewayStarted.catch(() => undefined); await stopHttpGateway?.(); - // Stopped with the other auxiliary frontends, and for the same reason: the uplink is a + // why: Stopped with the other auxiliary frontends, and for the same reason: the uplink is a // live connection into this daemon's dispatcher, so it must be shut before lease/queue // teardown begins. Stopping it also stops the reconnect loop, so a shutting-down worker // does not redial the gateway it just left. await gatewayUplink?.stop(); }, - // ADR 0003 §2: "an HTTP request during startup now waits like a socket request instead of + // why: ADR 0003 §2: "an HTTP request during startup now waits like a socket request instead of // being refused". Firing the gateway's own `start()` from here (not after `daemon.start()` // resolves) is what makes that true: the gateway is listening, and every route calls // `daemon.dispatch(...)`, which parks on the same startup-readiness gate a socket request @@ -818,7 +800,7 @@ async function composeDaemon( await gateway.stop(); app.dispose(); }; - // No self-stop check here: `stopAuxiliary` above is the single place that decides whether + // why: No self-stop check here: `stopAuxiliary` above is the single place that decides whether // to stop the gateway, and it does so by awaiting `gatewayStarted` (settled once this // function's caller -- `onSocketClaimed`'s handler below -- resolves or rejects) before // calling `stopHttpGateway`. Stopping here too would double-stop it. @@ -841,7 +823,7 @@ async function composeDaemon( }); const [daemonResult, gatewayResult] = await Promise.allSettled([daemonStarted, gatewayStarted]); if (gatewayResult.status === "rejected" && daemonResult.status === "fulfilled") { - // The daemon itself came all the way up -- convergence succeeded, `daemon.started` was + // why: The daemon itself came all the way up -- convergence succeeded, `daemon.started` was // already emitted -- but its HTTP gateway never bound. Tear the whole thing down now, // *after* that success is a settled fact, so `daemon.stopping` never precedes (or follows // out of order) `daemon.started`; see this function's own comment above `onSocketClaimed` @@ -864,7 +846,6 @@ interface GatewayDaemonOptions { readonly filesystem: Filesystem; readonly gatewayRelay?: (relay: GatewayRelay) => void; readonly gatewayUplinks?: (uplinks: WebSocketUplinkListenerFactory) => void; - /** The gateway's own machine. It runs no drivers, so its host facts carry no tools. */ readonly hostSystem: HostSystem; readonly hostLoadProbe?: HostLoadProbe | null; readonly idGenerator: IdGenerator; @@ -880,17 +861,6 @@ interface GatewayDaemonOptions { readonly webhookPoster?: WebhookPoster; } -/** - * A daemon in gateway mode (ADR 0005 §2, §32): no drivers, no registry, no reaper, no health - * monitor, no capacity strategy. What it has instead is a worker registry fed by uplinks, and - * `GatewayDispatcher` answering the same contract from those views. - * - * Two things are unconditional here where a worker makes them optional, and both follow from - * §2's "a gateway always listens on HTTP (it is the fleet's contact point) and on its unix - * socket": HTTP is started (the config loader refuses `http.enabled: false` in this mode), and - * the uplink listener is attached to that same HTTP server, so the whole fleet has exactly one - * inbound port. - */ // fallow-ignore-next-line complexity -- explicit production composition, exactly like `startDaemon`'s worker half. async function startGatewayDaemon(options: GatewayDaemonOptions): Promise { const { @@ -905,18 +875,14 @@ async function startGatewayDaemon(options: GatewayDaemonOptions): Promise leaseIndex.all().map((entry) => ({ id: entry.gatewayLeaseId, ownerId: entry.ownerId })), logger: logger.child("server"), ownerRoutedFacts: gatewayOwnerRoutedFacts, resolveRole: { resolve: (hello) => auth.resolveHelloRole(hello) }, version: options.version, - // A gateway's "convergence" is starting to accept uplinks and arming the refresh tick. + // why: A gateway's "convergence" is starting to accept uplinks and arming the refresh tick. // Running it here rather than before `start()` keeps one lifecycle: a listener that cannot // start fails the daemon start, and `status.get` answers throughout (a fleet with no views // yet is a true answer, not an unready one). A worker that dials in the window before this @@ -1137,32 +1088,21 @@ async function startGatewayDaemon(options: GatewayDaemonOptions): Promise uplinks.attach(server), port: config.http.port, }); @@ -1192,7 +1132,7 @@ async function startGatewayDaemon(options: GatewayDaemonOptions): Promise undefined); @@ -1220,22 +1160,18 @@ function errorMessage(error: unknown): string { } export interface DriverDiscoveryContext { - /** Threaded into the Android driver's `downloads.acceptAndroidLicenses` gate; defaults to `false` when omitted. */ readonly acceptAndroidLicenses?: boolean; readonly clock: Clock; - /** Whole `drivers` config section: each driver is handed its own block, unread. */ readonly driversConfig: Config["drivers"]; - /** Bridges the iOS driver's `device.slimmed` fact onto the bus -- see `emitSlimDiagnostic`. */ readonly eventBus: Pick; readonly filesystem: Filesystem; /** - * Which platform's tooling this host has, `process.platform` in production and supplied + * why: Which platform's tooling this host has, `process.platform` in production and supplied * by the composition root rather than read here. Discovery's fail-closed branch -- the * one that must cost a platform and never the daemon -- is otherwise reachable only on a * Mac, and so is untestable everywhere Simlock's own CI runs. */ readonly hostPlatform: NodeJS.Platform; - /** The host port's architecture; the Android driver prefers images of the ABI it runs. */ readonly hostArch: string; readonly idGenerator: IdGenerator; readonly instanceId: string; @@ -1244,31 +1180,14 @@ export interface DriverDiscoveryContext { readonly processSupervisor: ProcessSupervisor; readonly simlockHome: string; readonly tcpProbe: TcpProbe; - /** - * How the iOS driver makes a slim device (`ios.slim` in config), whatever the default mode. - * Never threaded into the Android driver -- slim is iOS-only until Android slim lands. Omitted - * means the iOS driver slims nothing. - */ readonly slim?: { readonly categories?: readonly string[]; readonly bootTimeoutMs: number; }; - /** - * Whether `ios.defaultMode` is `slim`, so the iOS driver reports runtimes it cannot slim in - * `doctor` (ADR 0007 §14). The driver is told this boolean, never the default itself. - */ readonly slimByDefault?: boolean; - /** - * How the Android driver launches emulators (`android.emulator` in config). Handed over - * unread; omitted or undefined leaves the driver's own default launch untouched. - */ readonly androidEmulator?: AndroidEmulatorLaunchOptions; } -/** - * Drivers that started, and the platforms that refused to -- both are startup outcomes -- with - * the prerequisite checks for every platform this host could run, started or not. - */ export interface DriverDiscovery { readonly drivers: readonly Driver[]; readonly prerequisiteChecks: readonly PrerequisiteCheck[]; @@ -1314,7 +1233,7 @@ export async function discoverDrivers(options: DriverDiscoveryContext): Promise< const logger = options.logger.child("driver-discovery"); const driversModule = process.env.SIMLOCK_DRIVERS_MODULE; if (driversModule !== undefined) { - // A substituted driver set owns whatever roots it wants, so there is nothing here to + // why: A substituted driver set owns whatever roots it wants, so there is nothing here to // refuse on its behalf. return { ...(await loadDriversModule(driversModule, options, logger)), rejections: [] }; } @@ -1342,11 +1261,6 @@ export async function discoverDrivers(options: DriverDiscoveryContext): Promise< }; } -/** - * The checks `doctor` runs for the platforms this host could run: iOS only on macOS, as - * discovery itself decides, and Android everywhere. Built whether or not the platform's - * driver started, because a driver that did not start is exactly when they are needed. - */ export function platformPrerequisiteChecks(options: { readonly env: Readonly>; readonly filesystem: Filesystem; @@ -1370,7 +1284,7 @@ export function platformPrerequisiteChecks(options: { } /** - * A refused root costs the daemon one platform, never the whole daemon: the other platform + * why: A refused root costs the daemon one platform, never the whole daemon: the other platform * may be perfectly healthy, and a daemon that will not start is a daemon that cannot tell * anyone why (safety rule 9). Every other failure still fails startup -- an unreadable root * is already an `OwnedRootError`, so what is left is a genuine bug. @@ -1406,7 +1320,7 @@ async function discoverIosDriver( simlockHome: options.simlockHome, ...(options.slim === undefined ? {} : { slim: options.slim }), ...(options.slimByDefault === undefined ? {} : { slimByDefault: options.slimByDefault }), - // Ambient like `homedir()` above, and read here rather than in the driver so the + // why: Ambient like `homedir()` above, and read here rather than in the driver so the // composition root stays the only place that touches process state. ...(process.getuid === undefined ? {} : { uid: process.getuid() }), pid: process.pid, @@ -1442,7 +1356,7 @@ export function logAndroidDiagnostic(logger: Logger, diagnostic: AndroidDriverDi } /** - * Android refuses for one more reason than iOS does -- it needs a private adb server as + * why: Android refuses for one more reason than iOS does -- it needs a private adb server as * well as an owned root -- and both refusals cost the platform rather than the daemon. * A missing SDK is not a refusal at all: this host simply has no Android tooling, which is * ordinary and reported by the absence of the platform. @@ -1470,7 +1384,7 @@ async function discoverAndroidDriver( simlockHome: options.simlockHome, tcpProbe: options.tcpProbe, tcpStreamConnector: new NodeTcpStreamConnector(), - // Ambient like `homedir()` above, and read here rather than in the driver so the + // why: Ambient like `homedir()` above, and read here rather than in the driver so the // composition root stays the only place that touches process state. ...(process.getuid === undefined ? {} : { uid: process.getuid() }), pid: process.pid, @@ -1533,16 +1447,6 @@ function rootRejection( }; } -/** - * Testing/advanced hook: substitutes real driver discovery with a module supplied via - * `SIMLOCK_DRIVERS_MODULE`. The daemon always runs as a separately spawned process, so - * the module is resolved as a file path (relative to `process.cwd()`) and dynamically - * imported -- this is how the e2e suite injects a scriptable fake driver without the - * daemon ever knowing it isn't talking to real hardware. A missing module, an import - * error, or a module without a `createDrivers` export fails daemon startup loudly - * rather than silently falling back to real discovery. Its optional `prerequisiteChecks` - * export replaces the real checks too; without one, `doctor` runs none. - */ async function loadDriversModule( modulePath: string, context: DriverDiscoveryContext, @@ -1572,12 +1476,6 @@ async function loadDriversModule( return { drivers, prerequisiteChecks: imported.prerequisiteChecks ?? [] }; } -/** - * What the config says about device modes, in the two shapes the composition root hands out - * (ADR 0007 §2, §14): the worker's default mode per platform for leasing, and whether - * the iOS default is slim for the iOS driver's advisory. Android has no key until Android slim - * lands, so it is left out and falls to the core's own `"full"`. - */ export function deviceModeWiring(config: Pick): { readonly defaultModes: Readonly>>; readonly slimByDefault: boolean; @@ -1588,11 +1486,6 @@ export function deviceModeWiring(config: Pick): { }; } -/** - * The one place a class's preference list is merged (ADR 0015 §4): the operator's names from - * `ios.defaultModels` or `android.defaultModels` first, then the driver's built-in ones, per - * platform that has a driver. The catalog reads the result; nothing else builds the list. - */ export function modelPreferenceWiring( config: Pick, drivers: readonly Pick[], @@ -1674,11 +1567,6 @@ function stopForRefusedHistory(daemon: { stop(reason: string): Promise }): }; } -/** - * The runner every driver and `device.exec` shell out through. At `log.level: debug` each - * command leaves a `process` line; at any other level the runner is used unwrapped, so a - * daemon not asked for that detail pays nothing for it. - */ export function processRunnerFor( level: LogLevel, runner: ProcessRunner, @@ -1689,7 +1577,6 @@ export function processRunnerFor( return new LoggingProcessRunner({ clock, inner: runner, logger: logger.child("process") }); } -/** Writes a failing event subscriber to the daemon log as one JSON line, not to stderr. */ export function eventBusLogger(logger: Logger): EventBusLogger { const bus = logger.child("bus"); return { From 9f3c43fcd5d8c4498479bc9f1536d30f4cb86f89 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 26 Oct 2026 19:09:44 +0100 Subject: [PATCH 2/3] chore(comments): prefix a why block the first pass missed --- src/daemon/dispatcher.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/daemon/dispatcher.ts b/src/daemon/dispatcher.ts index 9a2b83750..75bd894ea 100644 --- a/src/daemon/dispatcher.ts +++ b/src/daemon/dispatcher.ts @@ -86,7 +86,7 @@ import type { UpdateCoordinator, UpdateProgress } from "./update-coordinator.js" export { DispatchError, type ContractDispatcher, type DispatchSession } from "./dispatch.js"; /** - * How long a timed-out `device.exec` child gets between SIGTERM and SIGKILL (ADR 0005 §19e). + * why: How long a timed-out `device.exec` child gets between SIGTERM and SIGKILL (ADR 0005 §19e). * Fixed rather than derived from `exec.timeoutMs`: it is a termination-cleanup budget, not a * fraction of the command's own allowance -- the same reasoning, and the same ten seconds, as * `NodeProcessRunner`'s `SIGTERM_TO_SIGKILL_GRACE_MS`. From 28fa28d76201e99dfd54c0ed7cd076cf3cfc37c9 Mon Sep 17 00:00:00 2001 From: Szymon Chmal Date: Mon, 26 Oct 2026 19:14:20 +0100 Subject: [PATCH 3/3] chore(comments): keep two comments whose removal re-wrapped code lines --- src/daemon/dispatcher.ts | 4 +++- src/daemon/main.test.ts | 6 ++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/daemon/dispatcher.ts b/src/daemon/dispatcher.ts index 75bd894ea..d6919d3d9 100644 --- a/src/daemon/dispatcher.ts +++ b/src/daemon/dispatcher.ts @@ -1056,7 +1056,9 @@ function toWireProgress(progress: ComponentInstallerProgress): ComponentProgress function killQuietly(handle: StreamingProcessHandle, signal: NodeJS.Signals): void { try { handle.kill(signal); - } catch {} + } catch { + // Already gone. + } } function describeEnrollmentError(error: unknown): unknown { diff --git a/src/daemon/main.test.ts b/src/daemon/main.test.ts index 47bdc9261..bb17845ec 100644 --- a/src/daemon/main.test.ts +++ b/src/daemon/main.test.ts @@ -903,8 +903,10 @@ describe("startDaemon", () => { const clock = new FakeClock(1_000); const { daemon } = await start({ drivers: [ - new ToolsFakeDriver({ availableOsVersions: ["26.5"], clock, platform: "ios" }, () => - Promise.reject(new Error("xcodebuild -version exited with 1")), + new ToolsFakeDriver( + { availableOsVersions: ["26.5"], clock, platform: "ios" }, + // What an iOS driver whose `xcodebuild` fails looks like from here. + () => Promise.reject(new Error("xcodebuild -version exited with 1")), ), new ToolsFakeDriver({ availableOsVersions: ["35"], clock, platform: "android" }, () => Promise.resolve([{ name: "emulator", version: "35.4.9" }]),