fix(grpc): wire graceful shutdown into the signal lifecycle

[chrispaskvan]

Summary

Wire the previously unused gRPC shutdown function into the existing Terminus signal handler so SIGINT/SIGTERM also close the gRPC listener. Terminus drains HTTP before invoking this handler; gRPC then gets up to three seconds to drain before forced shutdown.

• Return a promise from gRPC stopServer(), use tryShutdown(), and force shutdown after a three-second grace period.
• Handle shutdown before startup, clear the fallback timer, ignore late callbacks, and log successful shutdown.
• Await the gRPC drain before starting cache, job queue, worker pool, and subscriber cleanup. This ordering is precautionary: the current RPC handler reads an in-memory item array synchronously, not those shared resources.
• Log a rejected or synchronously throwing gRPC shutdown and still run the remaining cleanup.
• Cover the gRPC lifecycle in grpc.spec.js. In server.spec.js, capture Terminus onSignal, keep the gRPC drain pending to prove cleanup waits, and cover shutdown failures. HTTP is mocked; no listener is opened by the unit spec, and independent cleanup tasks have no prescribed relative order.
• Retain the updated Clover report and JSON coverage reporter from the original change.

The unreachable callback-error branch and its test were removed in 1142869; this PR does not claim callback-error fallback behavior. Startup binding behavior and HTTP-only test teardown remain unchanged. Local tasks/ notes are excluded.

Verification

• pnpm lint:ci and pnpm typecheck passed.
• Targeted shutdown specs passed.
• Removing only await from the gRPC drain makes the pending-drain regression test fail; the mutation was reverted.
• The pre-push full suite passed: 60 files, 902 tests passed, 1 skipped.

[Copilot]

Copilot review overview

🟢 Approval recommended

The shutdown behavior is correctly implemented, integrated, and covered by focused lifecycle tests.

Review effort: Lite
Findings: None

What changed in this PR

Adds graceful gRPC shutdown to the existing Terminus lifecycle, ensuring active RPCs drain before shared resources close.

Changes:

• Added graceful gRPC shutdown with timeout and forced fallback.
• Integrated gRPC shutdown into server teardown.
• Added lifecycle tests and JSON coverage reporting.

File	Description
grpc.js	Implements graceful and forced shutdown behavior.
server.js	Awaits gRPC shutdown during termination.
grpc.spec.js	Tests shutdown lifecycle scenarios.
vitest.config.js	Enables JSON coverage output.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chrispaskvan]

Reviewed with the branch checked out. pnpm lint:ci and pnpm typecheck are clean and grpc.spec.js passes 15/15.

Worth stating plainly, because the PR body describes a slightly different bug than the one this fixes: before this change stopServer in grpc.js was exported and never imported anywhere — start.js only ever called grpcStart(). The gRPC listener on 127.0.0.1:1102 was never shut down on SIGTERM at all. That is the real gap being closed; the forceShutdown() → tryShutdown() upgrade is the secondary improvement. Ordering against Terminus is correct, for the record: Terminus runs beforeShutdown → asyncServerStop() → onSignal, so HTTP is already drained by the time stopGrpcServer() runs.

I mutation-tested the new specs against grpc.js, and all six mutations were caught — the tests are load-bearing, not decorative:

Mutation	Result
revert stopServer to plain forceShutdown()	4 failed
drop the 3s fallback timer	1 failed
drop if (completed) return	1 failed
never clear the module server ref	1 failed (via timeout — see the afterEach comment)
drop clearTimeout(timeout)	2 failed
always call forceShutdown()	1 failed

Inline comments below. Nothing here blocks merge; the first one is the only one I would want addressed before this lands.

Two nits not worth their own threads: 3000 now appears in both grpc.js and server.js, and a named constant would make the 6s worst-case shutdown budget visible; and the json coverage reporter looks like scope creep — coverage-final.json is not consumed by anything (codacy-coverage-reporter.yml reads clover.xml) and is gitignored.

---

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

[chrispaskvan]

@copilot fix issues

[Copilot]

@copilot fix issues

Addressed in 1142869. I added a focused server.spec.js that captures Terminus onSignal and asserts gRPC shutdown runs before the shared resource cleanup, tightened grpc.spec.js teardown so it only drains a server a test actually started, and updated gRPC shutdown logging/success-path handling accordingly.

[Copilot]

Copilot review overview

🟡 Changes recommended

tryShutdown callback errors are treated as successful shutdowns instead of triggering forced cleanup.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Handle tryShutdown callback errors before reporting graceful shutdown · New

[chrispaskvan]

Follow-up review of 1142869

All five findings from the previous review are addressed. I verified each one by mutation rather than by reading the diff.

Re-ran against 1142869: pnpm lint:ci clean, pnpm typecheck clean, grpc.spec.js + server.spec.js 23/23.

#	Finding	Status
1	server.js fix untested	Fixed — server.spec.js captures Terminus onSignal and asserts the ordering
2	Rationale overreach	Partly — see below, the PR body is now stale
3	Clean shutdown logs nothing	Fixed — log.info('GRPC server shut down'), asserted
4	Dead err branch	Fixed — branch and its test removed
5	afterEach hangs the suite	Fixed — started guard

Mutation results on the new code, all caught:

Mutation	Result
drop await stopGrpcServer() from server.js	1 failed
move the gRPC drain after shutdownTasks is built	1 failed
never clear the module server ref	1 failed
drop the success log	1 failed
drop forceShutdown() from the timeout branch	1 failed

The second one is the one I cared about most: server.spec.js tests the ordering, not merely that stopGrpcServer gets called. That is the finding-1 gap genuinely closed.

I have replied separately to the Copilot reviewer's comment on grpc.js:123 — its premise does not hold for @grpc/grpc-js@1.14.5, and acting on it would introduce a bug. Worth reading before acting on that suggestion.

Still open

The PR body is now stale and contradicts the code. It still says shutdown is forced "after a three-second grace period or callback error" and that the tests cover "callback errors" — both were removed in 1142869. It also does not mention server.spec.js, which is now the most important file in the change. This was finding 2: the description claims a coupling between in-flight RPCs and the shared resources that does not exist, since createGetAllHandler reads an in-memory array and calls back synchronously. Worth a rewrite before merge so the history reads accurately.

Two minor notes inline, neither blocking. Nothing here needs to hold up a merge once the description is corrected.

---

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

[Copilot]

Copilot review overview

🟡 Changes recommended

The shutdown-order test does not yet verify that cleanup waits for pending gRPC shutdown.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Test does not verify awaiting stopGrpcServer before cleanup · New

Resolved since last review (1)

• Handle tryShutdown callback errors before reporting graceful shutdown

[chrispaskvan]

Third pass — one new issue, verified

All six checks are green on 3d3d174a (the main merge released the workflows that had been sitting at action_required). A deeper review pass surfaced one thing the earlier rounds missed, which I confirmed empirically rather than by reading.

New, and the most important item on the PR right now: the unguarded await on server.js:68 can skip every other shutdown task. Details inline.

Already in flight: the server.spec.js gap on the dropped await — Copilot flagged it, I confirmed it and posted a mutation-checked fix in that thread.

On the grpc.js:123 thread, one clarification so the two suggestions there are not conflated. Reverting to tryShutdown(finish) against the current finish(timedOut = false) signature would be an outright bug — any argument grpc-js passed would land in timedOut. But a minimal tryShutdown(error => finish(Boolean(error))) is a different proposition: it cannot fire on 1.14.5, yet it costs one line and matches the published (error?: Error) => void contract without reintroducing a test for an unreachable path. I still think dropping the branch is defensible under AGENTS.md; I no longer think the one-line guard is objectionable. Either is fine — what matters is that the PR body stops claiming behaviour that is not there.

Still open from round one: the description remains stale. It advertises forcing shutdown "after a three-second grace period or callback error" and a "callback errors" test, both removed in 1142869, and never mentions server.spec.js.

Also worth a look: startServer() assigns server before bindAsync completes and start.js:11 does not wait for the bind, so a signal in that window passes the if (!server) guard, finds http2Servers empty, resolves immediately, and the in-flight bind still registers a listening socket nothing closes. Terminus re-raises the signal so the process dies anyway — small blast radius, but the drain is silently skipped. Having startServer return a promise that resolves in the bindAsync callback would close it.

For the record on shutdown budget: terminus stoppable 1s + gRPC 3s + processExternalPromisesWithTimeout 3s is about 7s, still inside Docker's default 10s grace.

---

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

[chrispaskvan]

Addressed the outstanding inline feedback in 15ca5d9, including the existing local test improvements, and corrected the PR title/body. The gRPC failure guard logs errors without skipping the remaining cleanup; the pending-drain regression catches removal of await. Full pre-push suite: 902 passed, 1 skipped; lint and typecheck passed.

On the additional startup/bind observation in the third-pass review: startup binding is unchanged in this PR. That concern remains a separate follow-up, not a fixed issue here. Returning a startup promise alone would not serialize a signal handler already registered by HTTP startup with an in-flight gRPC bind; it needs lifecycle coordination and a dedicated regression test. The existing timeout literals and JSON coverage reporter are also unchanged in this follow-up.

[chrispaskvan]

Final verification of 15ca5d9

Every finding raised across the three review passes is now closed. I re-verified the fixes by mutation against the current branch rather than by reading the responses.

Re-ran locally on 15ca5d9: pnpm lint:ci clean, pnpm typecheck clean, full suite 60 files / 902 passed, 1 skipped. All six checks green.

Mutation	Result
drop only the await on the gRPC drain	2 failed
remove the try/catch entirely	2 failed
keep the try/catch but swallow the error silently	2 failed

The first is the one that mattered: this is the regression that passed 9/9 before 15ca5d9, and the pending-drain rewrite now catches it. The third is a nice bonus — the spec pins the log call, not just the control flow.

Promise.withResolvers() is fine here: package.json requires node >=26.8.0 <27, well past its Node 22 availability.

Scorecard

Finding	Status
server.js fix untested	Closed — and now catches a dropped await, which the first version did not
Rationale overreach / stale description	Closed — the body now states the ordering is precautionary and says why
Clean shutdown logs nothing	Closed
Dead err branch	Closed
afterEach hangs the suite	Closed
Unguarded await skips all cleanup	Closed — log.error via the repo logger, both reject and sync-throw covered
Spec starts a real listener	Closed — node:http mocked
Over-specified cleanup order	Closed — replaced with the real contract

Two notes, neither blocking and neither needing action:

The startup race is still open by design and correctly scoped out — startServer() assigns server before bindAsync resolves, so a signal in that window skips the drain. Terminus re-raises the signal and the process dies regardless, so the blast radius is nil. Worth a follow-up issue rather than scope creep here.

On the grpc.js:123 thread, Copilot's reviewer bot is still asking for the error branch back. That premise does not hold for @grpc/grpc-js@1.14.5 — wrappedCallback is only ever invoked with no argument — and the current tryShutdown(() => finish()) wrapper is deliberately discarding the argument, so reverting to tryShutdown(finish) would be an actual bug. That thread should be dismissed, not actioned.

Nothing further from me. This reads as ready to merge.

---

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

[Copilot]

Copilot review overview

🟢 Approval recommended

No unresolved review comments remain, and all approval-readiness assessments approve the changes.

Review effort: Lite
Findings: None

Resolved since last review (1)

• Test does not verify awaiting stopGrpcServer before cleanup