fix(server): drain on SIGTERM instead of waiting out podman's SIGKILL #109

Sammanfogat

supernaut sammanfogade 1 incheckning från fix/sigterm-graceful-shutdown in i main

2026-07-30 16:40:05 +00:00

supernaut kommenterad

2026-07-30 16:36:58 +00:00

Ägare

Closes #107. Every deploy spent a fixed ~10 s waiting for a graceful stop that never came, then got
SIGKILLed — dropping in-flight requests rather than draining them. Measured on production: 10.07 s
of ~12.7 s total deploy unavailability.

The mechanism is narrower than the issue says

#107 (and my own comment on it) said the server "does not handle SIGTERM" and that Node's default for an
unhandled signal is to do nothing. That is not true in general — the first attempt to reproduce this
locally showed the old entry exiting in ~1 s.

It only fails as pid 1: the kernel does not apply default signal dispositions to pid 1, so a signal
whose handler was never explicitly installed is discarded (signal(7)). The Containerfile execs the
server, making it pid 1, and @astrojs/node 11.0.3 installs no signal handling at all.

Proven in a container as pid 1 with --stop-timeout 5:

	`podman stop`	exit code
no handler	5338 ms (full timeout)	137 (SIGKILL)
with handler	156 ms	0

So installing any handler is what makes the signal deliverable; draining gracefully is then what we
choose to do with it. Worth recording, because "add a SIGTERM handler" and "make SIGTERM reach the
process at all" are different problems with the same fix.

Why a wrapper

dist/server/entry.mjs starts listening as an import side effect and returns no handle on the
server it started, so there is nothing to attach a handler to. It does expose the two hooks needed:
ASTRO_NODE_AUTOSTART=disabled suppresses the auto-start, and startServer is exported.

scripts/server.mjs sets the env var and then uses a dynamic import — a static import is hoisted
above the assignment, so the server would autostart regardless.

scripts/shutdown.mjs holds the logic with the server injected as three callbacks
(close / closeIdleConnections / destroy), so the deadline path is testable without binding a port
or hanging a real socket — the same reason src/lib/health.ts takes a probeDb callback.

Behaviour

close() to stop accepting, so in-flight requests finish.
closeIdleConnections() as well. Without it close() waits for the client to disconnect, so a
browser holding a keep-alive socket open would burn the entire deadline and force the destroy path
with nothing actually in flight.
A 5 s hard deadline, comfortably below podman's 10 s StopTimeout, then force-close and exit — a
stuck request must not reintroduce the very SIGKILL this fixes. A test asserts the default stays
under 10 s.
Exit 0 on both paths. A forced close after a deliberate deadline is a successful shutdown, not a
crash; non-zero would make systemd log every deploy as a failure.
Idempotent — a second SIGTERM mid-drain is logged and ignored rather than racing a second
close() and resetting the deadline.

Verification

End to end against the real image, as pid 1, with production's default 10 s StopTimeout:

pid 1 = node ./scripts/server.mjs
GET / → HTTP 200
podman stop took 190 ms      exit code 0
[shutdown] SIGTERM received — draining in-flight requests
[shutdown] drained cleanly

Also confirmed the wrapper starts from a foreign cwd (ESM imports are module-relative), since the
container runs it from /app.

11 new tests, 92 total — the graceful path, the deadline path (the acceptance criterion: a hung
request still exits cleanly), that it does not force-close one tick early, a server missing both optional
callbacks, and re-entrancy. pnpm check 0 errors, pnpm lint clean.

vitest.config.ts now also collects scripts/**/*.test.mjs, since the runtime entry points there run
outside the Astro bundle. scripts/shutdown.mjs is added to coverage individually rather than globbing
scripts/, whose other files (the migrator, the e2e harness) are covered by being run rather than
unit-tested — globbing would add permanent 0% noise, which the existing comment in that file argues
against.

Effect

Deploy unavailability ~12.7 s → ~2.8 s, on top of the ~109 s already removed by bitborg-infra #254.

The acceptance criterion "no resorting to SIGKILL warning in the journal on a deploy" is verifiable on
the very next deploy — which is the one this PR triggers.

Follow-up, deliberately not in scope

At ~2.8 s a modest Caddy lb_try_duration genuinely bridges a deploy, which reverses bitborg-infra
#250's conclusion that no sane retry window could. Filing separately.

Also not in scope: making /healthz report 503 while draining. It needs no change with a single
container, but would matter if active upstream health checks are ever enabled.

Closes #107. Every deploy spent a fixed **~10 s** waiting for a graceful stop that never came, then got `SIGKILL`ed — dropping in-flight requests rather than draining them. Measured on production: **10.07 s of ~12.7 s** total deploy unavailability. ## The mechanism is narrower than the issue says #107 (and my own comment on it) said the server "does not handle SIGTERM" and that Node's default for an unhandled signal is to do nothing. **That is not true in general** — the first attempt to reproduce this locally showed the *old* entry exiting in ~1 s. It only fails **as pid 1**: the kernel does not apply default signal dispositions to pid 1, so a signal whose handler was never explicitly installed is *discarded* (`signal(7)`). The Containerfile `exec`s the server, making it pid 1, and `@astrojs/node` 11.0.3 installs no signal handling at all. Proven in a container as pid 1 with `--stop-timeout 5`: | | `podman stop` | exit code | | --- | --- | --- | | no handler | **5338 ms** (full timeout) | **137** (SIGKILL) | | with handler | **156 ms** | **0** | So installing **any** handler is what makes the signal deliverable; draining gracefully is then what we choose to do with it. Worth recording, because "add a SIGTERM handler" and "make SIGTERM reach the process at all" are different problems with the same fix. ## Why a wrapper `dist/server/entry.mjs` starts listening as an **import side effect** and returns no handle on the server it started, so there is nothing to attach a handler to. It does expose the two hooks needed: `ASTRO_NODE_AUTOSTART=disabled` suppresses the auto-start, and `startServer` is exported. `scripts/server.mjs` sets the env var and then uses a **dynamic** import — a static `import` is hoisted above the assignment, so the server would autostart regardless. `scripts/shutdown.mjs` holds the logic with the server injected as three callbacks (`close` / `closeIdleConnections` / `destroy`), so the deadline path is testable without binding a port or hanging a real socket — the same reason `src/lib/health.ts` takes a `probeDb` callback. ## Behaviour - `close()` to stop accepting, so in-flight requests finish. - **`closeIdleConnections()` as well.** Without it `close()` waits for the *client* to disconnect, so a browser holding a keep-alive socket open would burn the entire deadline and force the destroy path with nothing actually in flight. - A **5 s hard deadline**, comfortably below podman's 10 s `StopTimeout`, then force-close and exit — a stuck request must not reintroduce the very `SIGKILL` this fixes. A test asserts the default stays under 10 s. - **Exit 0 on both paths.** A forced close after a deliberate deadline is a successful shutdown, not a crash; non-zero would make systemd log every deploy as a failure. - **Idempotent** — a second `SIGTERM` mid-drain is logged and ignored rather than racing a second `close()` and resetting the deadline. ## Verification **End to end against the real image, as pid 1, with production's default 10 s `StopTimeout`:** ``` pid 1 = node ./scripts/server.mjs GET / → HTTP 200 podman stop took 190 ms exit code 0 [shutdown] SIGTERM received — draining in-flight requests [shutdown] drained cleanly ``` Also confirmed the wrapper starts from a foreign cwd (ESM imports are module-relative), since the container runs it from `/app`. **11 new tests, 92 total** — the graceful path, the deadline path (the acceptance criterion: a hung request still exits cleanly), that it does not force-close one tick early, a server missing both optional callbacks, and re-entrancy. `pnpm check` 0 errors, `pnpm lint` clean. `vitest.config.ts` now also collects `scripts/**/*.test.mjs`, since the runtime entry points there run outside the Astro bundle. `scripts/shutdown.mjs` is added to coverage individually rather than globbing `scripts/`, whose other files (the migrator, the e2e harness) are covered by being run rather than unit-tested — globbing would add permanent 0% noise, which the existing comment in that file argues against. ## Effect Deploy unavailability **~12.7 s → ~2.8 s**, on top of the ~109 s already removed by bitborg-infra #254. The acceptance criterion "no `resorting to SIGKILL` warning in the journal on a deploy" is verifiable on the very next deploy — which is the one this PR triggers. ## Follow-up, deliberately not in scope At ~2.8 s a modest Caddy `lb_try_duration` genuinely bridges a deploy, which reverses bitborg-infra #250's conclusion that no sane retry window could. Filing separately. Also not in scope: making `/healthz` report 503 while draining. It needs no change with a single container, but would matter if active upstream health checks are ever enabled.

supernaut lade till 1 incheckning

2026-07-30 16:36:58 +00:00

fix(server): drain on SIGTERM instead of waiting out podman's SIGKILL

ci / ci (pull_request) Successful in 1m14s

Detaljer

ca92347b69

Closes #107. Every deploy spent a fixed ~10s waiting for a graceful stop that
never came, then got SIGKILLed, dropping in-flight requests rather than draining
them. Measured on prod: 10.07s of ~12.7s total deploy unavailability.

The mechanism is narrower than "Node ignores SIGTERM" — that is not true in
general, and the first attempt to reproduce this locally showed the old entry
exiting in ~1s. It only fails as pid 1: the kernel does not apply default signal
dispositions to pid 1, so a signal whose handler was never explicitly installed is
discarded (signal(7)). The Containerfile `exec`s the server, making it pid 1, and
@astrojs/node 11.0.3 installs no signal handling at all.

Proven in a container, pid 1, `--stop-timeout 5`:

  no handler    podman stop took 5338 ms   exit 137 (SIGKILL)
  with handler  podman stop took  156 ms   exit 0

So installing ANY handler is what makes the signal deliverable; draining
gracefully is then what we choose to do with it.

The adapter's built entry starts listening as an import side effect and returns no
handle on the server, so this needs a wrapper rather than a few lines appended.
It does expose the two hooks required: ASTRO_NODE_AUTOSTART=disabled suppresses
the auto-start, and startServer is exported. scripts/server.mjs sets the env var
and then uses a DYNAMIC import — a static import is hoisted above the assignment
and would autostart anyway.

scripts/shutdown.mjs holds the logic, with the server injected as three callbacks
(close / closeIdleConnections / destroy) so the deadline path is testable without
binding a port or hanging a real socket — the same reason src/lib/health.ts takes
a `probeDb` callback.

Behaviour:

- close() to stop accepting, so in-flight requests finish.
- closeIdleConnections() as well: without it close() waits for the CLIENT to
  disconnect, so a browser holding a keep-alive socket would burn the whole
  deadline and force the destroy path with nothing actually in flight.
- A 5s hard deadline, comfortably below podman's 10s StopTimeout, then force-close
  and exit — a stuck request must not reintroduce the SIGKILL this fixes.
- Exit 0 on either path: a forced close after a deliberate deadline is a
  successful shutdown, not a crash, and non-zero would log every deploy as failed.
- Idempotent: a second SIGTERM during the drain is logged and ignored rather than
  racing a second close() and resetting the deadline.

Verified end to end against the real image, pid 1, with prod's default 10s
StopTimeout: podman stop took 190 ms, exit code 0, and the container log shows the
drain. 11 new tests (92 total), covering the graceful path, the deadline path, that
it does not force-close one tick early, a server missing the optional callbacks,
and re-entrancy.

vitest config now also collects scripts/**/*.test.mjs, since the runtime entry
points there run outside the Astro bundle. scripts/shutdown.mjs is added to
coverage individually rather than globbing scripts/, whose other files are covered
by being run rather than unit-tested.

Expected effect: deploy unavailability ~12.7s -> ~2.8s, on top of the ~109s
already removed by gitborg-infra #254.

supernaut sammanfogade incheckning 43ed5987d6 till main

2026-07-30 16:40:05 +00:00

supernaut tog bort grenen fix/sigterm-graceful-shutdown

2026-07-30 16:40:05 +00:00

supernaut refererade denna ändringsförfrågan från en incheckning

2026-07-30 16:40:07 +00:00

fix(server): drain on SIGTERM instead of waiting out podman's SIGKILL (#109)

supernaut refererade denna ändringsförfrågan från en incheckning

2026-08-03 09:41:50 +00:00

fix(server): drain on SIGTERM instead of waiting out podman's SIGKILL (#109)

Logga in för att delta i denna konversation.

Inga granskare