fix(grpc): await the port bind so shutdown cannot orphan a listener

[chrispaskvan]

Closes #745. Follow-up to #744, which scoped this out to keep startup behaviour unchanged.

Summary

startServer() assigned the module-level server before bindAsync completed and returned undefined, so start.js:11's await grpcStart() awaited nothing and a signal arriving during the bind window skipped the drain entirely.

grpc-js registers a listening server inside its own bind callback (node_modules/@grpc/grpc-js/build/src/server.js:419), so a shutdown in that window found http2Servers empty, fired its callback synchronously, logged 'GRPC server shut down' having drained nothing, and cleared the module reference. The in-flight bind then completed and registered a listening socket nothing could reach — neither tryShutdown nor forceShutdown — which kept accepting RPCs for the remainder of onSignal (up to the 3s processExternalPromisesWithTimeout budget) while the cache, job queue, worker pool and subscriber were being torn down underneath it.

• startServer() returns a promise that resolves in the bind callback and rejects on bind error.
• server is assigned only once the port is actually listening.
• A stopping flag makes a bind that completes after stopServer() force-shut its own server instead of orphaning it.

Bind failures now reject instead of throwing inside the callback, so they reach start.js's existing .catch(exitOnError) rather than surfacing as an uncaughtException.

No change to start.js — it already awaited and already had the catch; the await is simply real now.

Test change worth flagging

The pre-existing late-callback spec asserted log.info was never called. startServer() now legitimately logs 'GRPC server is listening' on bind, so that blanket assertion was tightened to the one it actually meant: not.toHaveBeenCalledWith('GRPC server shut down').

Verification

• pnpm lint:ci, pnpm typecheck clean. Full suite: 60 files, 905 passed, 1 skipped.
• Three new specs: the promise resolves only once bound, a bind error rejects, and a server that binds after shutdown is force-shut rather than orphaned.

Mutation-checked — every mutation fails the suite:

Mutation	Result
drop the stopping guard (orphan the late bind)	1 failed
assign server before the bind completes (restore the original race)	2 failed
never set stopping = true in stopServer	1 failed
resolve before the bind rather than in its callback	2 failed

The second is the one that matters: reinstating the exact bug this PR fixes fails the suite.

Risk

Low, but this is startup-path code. The behavioural change is that start() now blocks until the gRPC port is bound, where previously it returned immediately — so a bind failure is now a startup failure rather than an async throw. That is the intended correction, but it is the line to watch on deploy.

🤖 Generated with Claude Code

https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[chrispaskvan]

Non-blocking. The shared stopping flag is reset by any startServer() call. That's safe today: start.js is sequential, and no async gap exists where a signal could land between stopServer() and a fresh start. But start (bind pending) → stop → start would clear the flag before the first bind's callback runs. The first bind would then become server and the second would overwrite it, orphaning a listener again, which is the same class of bug this PR fixes.

A per-attempt token avoids resetting shared state:

let generation = 0;

// startServer
const attempt = generation;
// ...in the bind callback
if (attempt !== generation) { pending.forceShutdown(); ... }

// stopServer
generation += 1;

If you'd rather keep the boolean, a one-line comment noting it assumes a single start per process would be enough.

[chrispaskvan]

Adopted in 76f0057, and this was worth more than non-blocking — I confirmed the failure rather than reasoning about it.

Traced exactly as described:

1. startServer() feat(reorganize): Files organized by feature. Initial test coverage a… #1 sets stopping = false, bind feat(reorganize): Files organized by feature. Initial test coverage a… #1 in flight.
2. stopServer() sets stopping = true, finds no server, resolves.
3. startServer() feat(graphql): ECMAScript 6 refactoring. #2 resets stopping = false, bind feat(graphql): ECMAScript 6 refactoring. #2 in flight.
4. Bind feat(reorganize): Files organized by feature. Initial test coverage a… #1's callback sees stopping === false, so server = pending#1.
5. Bind feat(graphql): ECMAScript 6 refactoring. #2's callback also sees false, so server = pending#2 — and pending#1 is now unreachable.

That is the same orphaned-listener bug this PR exists to remove, reintroduced by the fix's own shared state. Unreachable today, since start.js starts once and nothing restarts the server, but it is a latent instance of the exact class being fixed, and leaving it in would have been the wrong trade.

Went with the generation counter rather than the comment. stopServer() increments generation; startServer() captures const attempt = generation and the bind callback force-shuts when attempt !== generation. No shared state is reset anywhere, which is what made the boolean fragile.

Added a regression test for the sequence — start (bind pending) → stop → start → both binds complete — asserting forceShutdown is called exactly once and only one server ever logs as listening. Mutation-checked: reverting to the shared boolean fails 2 tests, dropping the guard fails 2, and never incrementing generation fails 2.

Full suite green: 60 files, 906 passed, 1 skipped.

---

🤖 Reviewed with Claude Code · https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf

[chrispaskvan]

Nit. This comment describes the pre-fix behaviour. After this change, a shutdown during the bind doesn't clear the module reference, because server isn't assigned until after this check. Suggest something like: "A shutdown that ran while this bind was in flight found no server to drain. Close this one here rather than leave it listening with nothing able to reach it."

[chrispaskvan]

Fixed in 76f0057. You're right that it described the pre-fix behaviour — server isn't assigned until after the guard now, so there is no module reference left to clear.

Used your wording, with one added sentence explaining why the token is per-attempt rather than a shared flag, since that is the non-obvious part after the other change in this commit:

/**
 * grpc-js only registers a listening server inside its own bind callback, so a
 * shutdown that ran while this bind was in flight found no server to drain.
 * Close this one here rather than leave it listening with nothing able to
 * reach it. Comparing against a per-attempt token rather than a shared flag
 * keeps a later startServer() from adopting an earlier attempt's server.
 */

---

🤖 Reviewed with Claude Code · https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf

[chrispaskvan]

If a bind fails after stopServer() has run, this still rejects. The rejection reaches start().catch(exitOnError) and calls process.exit(1) in the middle of the terminus teardown, cutting short cleanup of the cache, job queue, worker pool and subscriber, and turning a normal SIGTERM into a non-zero exit. It's unlikely on 127.0.0.1, but it's the same class of shutdown race this PR fixes.

With the generation token in place, the fix is small:

if (err) {
    if (attempt !== generation) {
        log.warn({ err }, 'GRPC bind failed after shutdown began; ignoring');
        resolve();
        return;
    }
    reject(err);
    return;
}

A spec could mirror "closes a server that finishes binding after shutdown", calling bound(new Error('EADDRINUSE')) in place of bound(null, 1102) and asserting that startServer() resolves and log.warn is called.

[chrispaskvan]

Adopted in 9091321. Traced the chain to confirm it, and it lands exactly as you describe.

await grpcStart() in start.js:11 means a rejected startServer() rejects start(), which hits start().catch(err => exitOnError(err)) at start.js:23, and exitOnError is console.error followed by process.exit(1). During terminus teardown that exits the process while processExternalPromisesWithTimeout is still inside its 3s window, so the cache, job queue, worker pool and subscriber cleanup are cut short — and a routine SIGTERM reports a non-zero exit. That is the same shutdown race in a different guise, and the generation token was already in place to distinguish the cases.

Took your patch as written. An attempt whose generation has been superseded has nobody waiting on it, so logging and resolving is right, and it matches what the success path already does after a shutdown.

Added the spec you sketched — bound(new Error('EADDRINUSE')) after stopServer(), asserting startServer() resolves, log.warn fires, and forceShutdown is not called (there is no listening server to close in this path).

Mutation-checked both directions, since a guard like this can fail by being too narrow or too broad:

Mutation	Result
remove the guard (reject unconditionally)	1 failed — the new spec
swallow every bind error (never reject)	1 failed — the existing rejects when the port cannot be bound spec

The second matters as much as the first: it proves the guard is scoped to post-shutdown binds and has not quietly disabled the startup failure path.

Full suite green: 60 files, 907 passed, 1 skipped.

---

🤖 Reviewed with Claude Code · https://claude.ai/code/session_018iD3V7Zyugg3atXVsogTBf