Skip to content

Commit f085f8e

Browse files
os-zhuangclaude
andcommitted
fix(hono-server): drain in-flight requests on shutdown (P1-3)
Verification reframed the launch-readiness P1-3 finding: the kernel already handles SIGINT/SIGTERM/SIGQUIT with an ordered, 60s-bounded graceful shutdown (registerShutdownSignals + performShutdown + onShutdown hooks), and serve.ts boots through it — so "no signal handling" was a false positive. The real, narrower bug: HonoHttpServer.close() called closeAllConnections(), force-killing in-flight requests. Now it drains: server.close() + drain active, closeIdleConnections() to release idle keep-alive, and a bounded drain window (default 10s, < kernel 60s) force-closes only stragglers so shutdown can't hang. +2 integration tests (drain completes a slow request; force-close bounds a hung one). docs/launch-readiness.md P1-3 updated with the verification finding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 1e8b680 commit f085f8e

4 files changed

Lines changed: 118 additions & 15 deletions

File tree

.changeset/graceful-http-drain.md

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
'@objectstack/plugin-hono-server': patch
3+
---
4+
5+
fix(hono-server): drain in-flight requests on shutdown instead of force-closing (P1-3)
6+
7+
`HonoHttpServer.close()` called `closeAllConnections()`, which terminated active
8+
connections mid-response — so a SIGTERM during a rolling deploy dropped in-flight
9+
requests. It now drains gracefully: `server.close()` stops accepting new
10+
connections and lets active requests finish, `closeIdleConnections()` releases
11+
idle keep-alive sockets so the process exits promptly, and a bounded drain window
12+
(default 10s, configurable, well under the kernel's 60s `shutdownTimeout`)
13+
force-closes only the stragglers so shutdown can't hang.
14+
15+
Note: the kernel already handles SIGINT/SIGTERM/SIGQUIT with an ordered,
16+
timeout-bounded shutdown — this fixes the one place that wasn't draining.

docs/launch-readiness.md

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -121,14 +121,23 @@ fix or acceptance.**
121121
sweepers at startup; persist automation logs to a table rather than memory.
122122
- **Owner:** _______ · Verify ☐ · Sign-off ☐ · Notes: _______
123123

124-
### P1-3 — No process-level graceful shutdown
125-
- **Area:** `runtime` (`http-server.ts`) + framework adapters
126-
- **Risk:** No SIGTERM/SIGINT handling → in-flight requests dropped on
127-
rolling deploys (K8s SIGKILL after grace period); cluster (Redis) + kernel +
128-
HTTP not drained in a coordinated order.
129-
- **Action:** Ship a graceful-shutdown utility (drain HTTP → stop kernel services
130-
→ close cluster) and wire it into app scaffolding; document a ≥60s grace floor.
131-
- **Owner:** _______ · Verify ☐ · Sign-off ☐ · Notes: _______
124+
### P1-3 — Graceful shutdown (mostly a false positive; one real drain bug fixed)
125+
- **Area:** `core` (`kernel.ts`), `cli` (`serve.ts`), `plugin-hono-server` (`adapter.ts`)
126+
- **Verification finding:** The sweep's "no SIGTERM/SIGINT handling" is **wrong**.
127+
`Kernel.registerShutdownSignals()` (called at start) already handles
128+
SIGINT/SIGTERM/SIGQUIT → `shutdown()``performShutdown()` (ordered plugin
129+
destroy in reverse + `kernel:shutdown` hook + `onShutdown` handlers), bounded by
130+
a **default 60s** `shutdownTimeout`. `serve.ts` boots through the kernel, so the
131+
production path inherits all of this. The ≥60s grace floor already exists.
132+
- **Real (narrower) gap — FIXED:** the standalone Hono server's `close()` called
133+
`closeAllConnections()`, which **force-killed in-flight requests** instead of
134+
draining them. Replaced with: `server.close()` (stop new + drain active) +
135+
`closeIdleConnections()` (release idle keep-alive), and force-close only after a
136+
bounded **drain window** (default 10s, < the kernel's 60s). +2 integration tests.
137+
- **Residual (not blocking):** embedding framework adapters (express/fastify/…)
138+
intentionally leave signal handling to the host app; cluster/Redis close should
139+
be registered via `kernel.onShutdown(...)` by the cluster plugin — confirm it is.
140+
- **Owner:** _______ · Verify ✅ (mostly false positive; drain bug fixed) · Sign-off ☐ · Notes: Kernel shutdown already correct; hono drain fixed + tested. Awaiting human sign-off.
132141

133142
### P1-4 — Per-request hostname → environment resolution (no cache)
134143
- **Area:** `rest``src/rest-server.ts:~504–530`
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import { HonoHttpServer } from './adapter';
5+
6+
const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms));
7+
8+
/**
9+
* P1-3 regression: `close()` must DRAIN in-flight requests (let them finish)
10+
* rather than force-killing them, and must force-close only after the drain
11+
* window elapses so shutdown can't hang.
12+
*/
13+
describe('HonoHttpServer — graceful close drains in-flight requests (P1-3)', () => {
14+
it('lets an in-flight request complete instead of aborting it', async () => {
15+
const server = new HonoHttpServer(0, undefined, 5000); // generous drain window
16+
server.getRawApp().get('/slow', async (c) => {
17+
await sleep(200); // still running when close() is called
18+
return c.text('drained-ok');
19+
});
20+
await server.listen(0);
21+
const port = server.getPort();
22+
23+
const reqP = fetch(`http://127.0.0.1:${port}/slow`);
24+
await sleep(50); // ensure the request is being handled
25+
const closeP = server.close();
26+
27+
const res = await reqP;
28+
expect(res.status).toBe(200);
29+
expect(await res.text()).toBe('drained-ok'); // completed, not reset
30+
await closeP;
31+
});
32+
33+
it('force-closes after the drain window so shutdown cannot hang', async () => {
34+
const server = new HonoHttpServer(0, undefined, 100); // tiny drain window
35+
server.getRawApp().get('/hang', async (c) => {
36+
await sleep(5000); // far longer than the drain window
37+
return c.text('late');
38+
});
39+
await server.listen(0);
40+
const port = server.getPort();
41+
42+
const reqP = fetch(`http://127.0.0.1:${port}/hang`).catch((e) => e); // will be aborted
43+
await sleep(50);
44+
45+
const t0 = Date.now();
46+
await server.close();
47+
const elapsed = Date.now() - t0;
48+
49+
expect(elapsed).toBeLessThan(2000); // didn't wait out the 5s request
50+
await reqP; // the aborted request settled — fine
51+
});
52+
});

packages/plugins/plugin-hono-server/src/adapter.ts

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,13 @@ export class HonoHttpServer implements IHttpServer {
4646

4747
constructor(
4848
private port: number = 3000,
49-
private staticRoot?: string
49+
private staticRoot?: string,
50+
/**
51+
* Max time (ms) to let in-flight requests drain on `close()` before
52+
* force-closing the remainder. Kept well under the kernel's 60s
53+
* `shutdownTimeout` so a slow request can't hang the whole shutdown.
54+
*/
55+
private drainTimeoutMs: number = 10_000,
5056
) {
5157
this.app = new Hono();
5258
}
@@ -285,12 +291,32 @@ export class HonoHttpServer implements IHttpServer {
285291

286292
async close() {
287293
if (!this.server) return;
288-
// Destroy all keep-alive sockets so the server stops immediately
289-
if (typeof this.server.closeAllConnections === 'function') {
290-
this.server.closeAllConnections();
291-
}
292-
await new Promise<void>((resolve, reject) => {
293-
this.server.close((err: any) => (err ? reject(err) : resolve()));
294+
const server = this.server;
295+
// Graceful drain (P1-3): stop accepting new connections and let in-flight
296+
// requests finish rather than force-killing them mid-response.
297+
// `closeIdleConnections()` releases idle keep-alive sockets so the process
298+
// can exit promptly; active requests keep running until they complete or
299+
// the drain window elapses.
300+
await new Promise<void>((resolve) => {
301+
let settled = false;
302+
const finish = () => { if (!settled) { settled = true; resolve(); } };
303+
304+
// Fires once every connection has ended (drained).
305+
server.close(() => finish());
306+
if (typeof server.closeIdleConnections === 'function') {
307+
server.closeIdleConnections();
308+
}
309+
310+
// Safety net: if requests outlast the drain window, force-close the
311+
// remainder so shutdown can't hang past the kernel's shutdownTimeout.
312+
const timer = setTimeout(() => {
313+
if (typeof server.closeAllConnections === 'function') {
314+
server.closeAllConnections();
315+
}
316+
finish();
317+
}, this.drainTimeoutMs);
318+
if (typeof timer.unref === 'function') timer.unref();
294319
});
320+
this.server = undefined;
295321
}
296322
}

0 commit comments

Comments
 (0)