Skip to content

Commit 9a7e82b

Browse files
committed
fix(cli): bound the snapshot waits, so no verb can hang without an exit code
The server `test` job kept dying at its 15-minute cap, and with the per-test timeout in place it finally named itself: `src/cli/wait.test.ts` times out right after its three argv-only tests — i.e. on the first test that actually connects. It reproduces on the runner every time and on nothing else: the suite passes on macOS/Node 26, on Node 22 at concurrency 2, and in `node:22` under Docker at both `--cpus=2` and `--cpus=4` (1623 tests, exit 0). Two hypotheses died on the way here: forcing the stub's sockets shut did not fix it, and RSA keygen is not the cost (a 2048-bit `selfsigned.generate` measures 1ms, four concurrently still 0s). What the investigation did expose is a real defect of the same family as the two already fixed on this branch: `awaitSnapshot()` / `awaitProjects()` reject when the socket *closes*, but a server that stays up and simply never pushes was waited on **forever**. That keeps the socket open and the event loop alive, so the verb produces no output and no exit code — the one outcome D8's contract cannot express, and precisely how a CI job ends up cancelled minutes later with nothing naming the cause. `ls`, `new`, `run`, `fork`, `ask` and `wait` all await a snapshot before doing anything, so all six inherited it. Both waits are now bounded at 15s and reject with a sentence. The server pushes both snapshots immediately after `hello.ack`, so this can only fire when something is genuinely wrong; the timer is unref'd so it can never itself be the reason a process stays alive. Verified: server 1624/1624 on Node 26 and the stub-heavy files 66/66 on Node 22 (CI's version). Whether this is the runner's exact stall or not, the class of failure it removes is the one that was killing the job blind.
1 parent 6c4cb7f commit 9a7e82b

4 files changed

Lines changed: 64 additions & 3 deletions

File tree

.pnpm-store/v11/index.db

8 KB
Binary file not shown.
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
../../../server

server/src/cli/client.test.ts

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import { mkdtempSync, rmSync, existsSync, readFileSync, mkdirSync, writeFileSync
1111
import { tmpdir } from "node:os";
1212
import { join } from "node:path";
1313

14-
import { openClient, resolveBearer, cliCredentialPath, verifiesCert } from "./client.js";
14+
import { openClient, resolveBearer, cliCredentialPath, verifiesCert, SNAPSHOT_TIMEOUT_MS } from "./client.js";
1515
import type { ControlResponse } from "../daemon/protocol.js";
1616
import { startStubWss as startStub } from "../../test/support/stub_wss.js";
1717

@@ -260,3 +260,30 @@ test("a cmd field named like an envelope key cannot corrupt the frame", async ()
260260
await stub.close();
261261
}
262262
});
263+
264+
test("a server that stays up but never pushes a snapshot does not hang the verb", async () => {
265+
// Rejecting on *close* is not enough: a live server that simply never pushes
266+
// leaves the socket open and the event loop alive, so the verb emits no output
267+
// and no exit code at all. That is the one outcome D8's contract cannot
268+
// express, and in CI it appears as a job cancelled minutes later with nothing
269+
// naming the cause.
270+
const stub = await startStub({ acceptBearer: "good" }); // no `sessions` → never pushes
271+
const client = await openClient({ host: "127.0.0.1", port: stub.port, bearer: "good" });
272+
try {
273+
await client.hello();
274+
assert.ok(SNAPSHOT_TIMEOUT_MS >= 10_000, "the bound must be generous enough never to fire in anger");
275+
// Proving the bound exists without waiting it out: the promise must be
276+
// pending now (the server is healthy, just quiet) and must carry a rejection
277+
// path rather than being a promise nobody can ever settle.
278+
const pending = client.awaitSnapshot();
279+
const raced = await Promise.race([
280+
pending.then(() => "settled", () => "rejected"),
281+
new Promise((r) => setTimeout(() => r("still-waiting"), 150)),
282+
]);
283+
assert.equal(raced, "still-waiting", "a healthy quiet server is waited for, not failed instantly");
284+
pending.catch(() => {}); // the timeout will reject it after the test ends
285+
} finally {
286+
client.close();
287+
await stub.close();
288+
}
289+
});

server/src/cli/client.ts

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,15 @@ export interface MakitClient {
7676
hello(): Promise<void>;
7777
/** Send a `cmd` and resolve the `ack` frame that matches its id; reject on `err`. */
7878
cmd(kind: string, fields?: Record<string, unknown>): Promise<Record<string, unknown>>;
79-
/** Resolve with the cached `sessions.snapshot`, or the next one pushed. */
79+
/**
80+
* Resolve with the cached `sessions.snapshot`, or the next one pushed — and
81+
* reject if none arrives within {@link SNAPSHOT_TIMEOUT_MS}. Bounded because a
82+
* verb that never reaches an exit code is the one outcome D8's contract cannot
83+
* express: rejecting on close is not enough, since a server that stays up and
84+
* simply never pushes would wait forever.
85+
*/
8086
awaitSnapshot(): Promise<SessionsSnapshot>;
81-
/** Resolve with the cached `projects.snapshot`, or the next one pushed. */
87+
/** As {@link awaitSnapshot}, for `projects.snapshot`, and bounded the same way. */
8288
awaitProjects(): Promise<ProjectDTO[]>;
8389
/** Send a raw frame (`sub`, `srv.response`, …) with no reply correlation. The
8490
* optional `onSent` fires once the frame is written to the socket, so a
@@ -120,6 +126,31 @@ export class AuthError extends Error {
120126
type Pending = { resolve: (frame: Record<string, unknown>) => void; reject: (err: Error) => void };
121127
type Waiter<T> = { resolve: (value: T) => void; reject: (err: Error) => void };
122128

129+
/**
130+
* How long a verb waits for a snapshot the server pushes unprompted, before it
131+
* gives up and says so.
132+
*
133+
* The server sends both snapshots immediately after `hello.ack`, so this only
134+
* ever fires when something is genuinely wrong. It exists because the failure it
135+
* replaces has no upper bound: an unpushed snapshot left the socket open and the
136+
* event loop alive, so the verb produced no output and no exit code — which in
137+
* CI is a job cancelled minutes later with nothing naming the cause.
138+
*/
139+
export const SNAPSHOT_TIMEOUT_MS = 15_000;
140+
141+
/** Reject `waiters` if nothing has settled them within `SNAPSHOT_TIMEOUT_MS`. */
142+
function armWaiterTimeout<T>(waiters: Waiter<T>[], what: string): void {
143+
const waiter = waiters[waiters.length - 1]!;
144+
const timer = setTimeout(() => {
145+
const i = waiters.indexOf(waiter);
146+
if (i === -1) return; // already settled
147+
waiters.splice(i, 1);
148+
waiter.reject(new Error(`timed out after ${SNAPSHOT_TIMEOUT_MS}ms waiting for ${what}`));
149+
}, SNAPSHOT_TIMEOUT_MS);
150+
// Unref'd: the timer must never be the reason the process stays alive.
151+
timer.unref();
152+
}
153+
123154
/** Open a WSS client, resolving once the socket is open (rejecting on connect error). */
124155
export function openClient(opts: OpenClientOpts): Promise<MakitClient> {
125156
return new Promise((resolve, reject) => {
@@ -242,13 +273,15 @@ export function openClient(opts: OpenClientOpts): Promise<MakitClient> {
242273
if (closed) return Promise.reject(new Error("client closed"));
243274
return new Promise((resolve, reject) => {
244275
snapshotWaiters.push({ resolve, reject });
276+
armWaiterTimeout(snapshotWaiters, "sessions.snapshot");
245277
});
246278
},
247279
awaitProjects() {
248280
if (projects) return Promise.resolve(projects);
249281
if (closed) return Promise.reject(new Error("client closed"));
250282
return new Promise((resolve, reject) => {
251283
projectsWaiters.push({ resolve, reject });
284+
armWaiterTimeout(projectsWaiters, "projects.snapshot");
252285
});
253286
},
254287
send(frame, onSent) {

0 commit comments

Comments
 (0)