Merge pull request #11 from crydensync/fix/graceful-shutdown

Fix/graceful shutdown

Author: raymondproguy

README.md

@@ -86,7 +86,9 @@ No migration step exists on SQLite. `main.go` calls cryden's own `sqlite.Migrate
 
 The connection is opened with three pragmas, all of them load-bearing: `foreign_keys(1)` (off by default, so the schema's `ON DELETE` clauses would silently not run), `busy_timeout(5000)` (zero by default, so a concurrent writer gets an immediate `SQLITE_BUSY` instead of waiting), and `journal_mode(WAL)`. The server verifies the first two on every boot with cryden's own `CheckPragmas` and refuses to start if the DSN and the driver have drifted apart.
 
-**Backing up a SQLite deployment means copying `api.db`, `api.db-wal` and `api.db-shm` together**, or checkpointing first. With WAL, recent writes — including, on a fresh deployment, the entire schema — live in the `-wal` file until a checkpoint folds them into the main file, and there is no graceful shutdown here yet to force one on exit. Copying `api.db` alone can silently produce an empty database.
+**Backing up a SQLite deployment means copying `api.db`, `api.db-wal` and `api.db-shm` together**, or checkpointing first. With WAL, recent writes — including, on a fresh deployment, the entire schema — live in the `-wal` file until a checkpoint folds them into the main file. Copying `api.db` alone can silently produce an empty database in that window.
+
+A clean stop is what closes that window: on `SIGTERM` or `SIGINT` the server stops accepting connections, waits up to 30 seconds for the requests already in flight, stops the webhook worker and digest scheduler, and closes the database — which on SQLite is the checkpoint that folds the `-wal` file back into `api.db` and removes the sidecar files. So `systemctl stop`, `docker stop` and Ctrl-C all leave a `api.db` that is complete on its own. A `kill -9`, a crash or a power loss does not, which is why the paragraph above still stands.
 
 ## Second factors
 

askai/askai.go

@@ -246,9 +246,9 @@ func (s *Service) Ask(ctx context.Context, req Request) (widget.Answer, error) {
 
 // Close releases the connection pool behind the cached providers. The
 // cached pair is replaced and closed on every rebuild, so this is only
-// about the last one — and nothing calls it yet, because this repo still
-// has no graceful shutdown for it to hang off. It exists so that adding
-// one does not have to start by widening this type's API.
+// about the last one — main.go calls it during shutdown, after the server
+// has drained and the background workers have stopped, so no question can
+// be in flight against a provider this is about to close.
 func (s *Service) Close() error {
 	s.mu.Lock()
 	defer s.mu.Unlock()

digest/schedule.go

@@ -56,11 +56,12 @@ type Scheduler struct {
 // start. A history that grows with restarts rather than with time is not
 // a history of anything.
 //
-// main.go hands this context.Background(), because this repo has no
-// graceful shutdown yet — the same caveat, and the same reasoning, as the
-// webhook worker's goroutine. Nothing here needs stopping today: an
-// interrupted run loses at most one digest, and the next interval builds
-// another.
+// main.go hands this the context the shutdown signal cancels, so a SIGTERM
+// stops it between runs. Nothing here needed to be stoppable for
+// correctness — an interrupted run records nothing and the next interval
+// builds another — but a build cut off halfway is worse than one that
+// never started, because half a window in the history reads as a quiet
+// week rather than as a missing one.
 func (s *Scheduler) Run(ctx context.Context) {
 	if s.Interval <= 0 || s.Store == nil || s.Build == nil {
 		return

docs/development/CURRENT-STATE.md

@@ -391,9 +391,10 @@ deployment does on its next restart, so it is called out in `README.md`,
 
 The digest's new table (`012`) has **never been applied to a real
 database**, the same as `009`–`011` — there is no Postgres in this
-sandbox — and the digest schedule is a goroutine on
-`context.Background()`, because this repo still has no graceful
-shutdown. `PROGRESS.md` says both plainly.
+sandbox. The digest schedule used to be a goroutine on
+`context.Background()`; it now takes the context the shutdown signal
+cancels, so the paragraph that follows in the Tier 6 section applies here
+too. `PROGRESS.md` says both plainly.
 
 ### Stage 2 — the providers and the widget config
 
@@ -568,9 +569,10 @@ foreign key rather than accepting any id, so that the tested branch is
 the one production runs. `-race` was not run this session.
 
 **Still not built** (unchanged from Tier 4, not part of this tier):
-graceful shutdown, and per-user rate limiting on anything that calls a
-model. The widget's own serving endpoint was in this list when Tier 5
-landed and is not any more — see the next section.
+per-user rate limiting on anything that calls a model. Graceful shutdown
+was on this list and is not any more — see the last section of this file.
+The widget's own serving endpoint was in this list when Tier 5 landed and
+is not any more — see the next section.
 
 ## The ask-ai widget's serving endpoint — carried forward from Tier 4
 
@@ -653,18 +655,18 @@ single most important thing about it.
   see the intent that actually reached the query surface. It is also the
   hook a host running a different LLM backend needs, which is why it is
   exported rather than a test-only accessor.
-- **`Service.Close()` exists and nothing calls it.** There is still no
-  graceful shutdown for it to hang off, so it is there so that adding
-  one does not have to start by widening this type's API.
+- **`Service.Close()` is called by the teardown in `main.go`.** It
+  releases the last cached provider pool, after the server has drained
+  and the background workers have stopped, so no question can be in
+  flight against a provider that is being closed.
 
 **What is still owed, said plainly.** No per-user rate limiting on this
 route: it spends money per question and is bounded only by the global
 per-IP edge limiter. That needs policy — per-user or per-deployment, and
 what number — which is a deployment's call rather than something to
-invent here. `Service.Close()` is never called, for the shutdown reason
-above. The Anthropic provider still has never called Anthropic, so the
-live path from a question to a real model is exercised only through the
-`Providers` seam with doubles; the wire shape is covered by
+invent here. The Anthropic provider still has never called Anthropic, so
+the live path from a question to a real model is exercised only through
+the `Providers` seam with doubles; the wire shape is covered by
 `aiprovider`'s own tests against a local fake.
 
 ## Tier 6 — SQLite backend, core auth only
@@ -753,11 +755,86 @@ this repo's server was started on a real SQLite file and passed the full
 `internal/smoketest` run — health, signup, duplicate rejection, login,
 wrong password, verify, session list, missing-header rejection, refresh
 rotation, reuse detection, family revocation, and both OAuth refusals.
-`-race` was still not run. Graceful shutdown is still unbuilt, and on
-SQLite it now has a second reason to exist: with no `Close()` there is
-no checkpoint on exit, so a fresh deployment's entire schema can sit in
+`-race` was still not run. Graceful shutdown was not built by this tier,
+and on SQLite it had a second reason to exist: with no `Close()` there is
+no checkpoint on exit, so a fresh deployment's entire schema could sit in
 the `-wal` file — durable, but a backup that copies `api.db` alone can
-silently produce an empty database. `README.md` warns about that where
-an operator will see it. Per-user rate limiting on `POST /v1/ask-ai` is
+silently produce an empty database. `README.md` warns about that where an
+operator will see it. That gap is now closed — see the last section of
+this file — but the warning stays, because a `kill -9` still leaves the
+WAL uncheckpointed and the advice to copy all three files is still the
+right advice. Per-user rate limiting on `POST /v1/ask-ai` is
 unchanged.
 
+
+## Graceful shutdown — the finding Tier 3 opened and Tier 6 sharpened
+
+This is not a tier. It is the one item that appeared as owed in three
+separate tier write-ups (Tiers 3, 5 and 6), so it is recorded once, here,
+and the three write-ups now point at it instead of restating it.
+
+**What it was.** `main.go` ended at
+`log.Fatal(http.ListenAndServe(...))`. That single line meant three
+things, and only the first was obvious:
+
+1. **No signal handling.** A `SIGTERM` — which is what every process
+   manager sends, including `docker stop` and a Kubernetes rolling
+   deploy — killed the process where it stood. Every request in flight
+   died with it.
+2. **`os.Exit` runs no defers.** `log.Fatalf` calls `os.Exit(1)`, so the
+   `defer db.Close()` two lines above it had never run once in this
+   repo's life. Every clean shutdown leaked the pool.
+3. **On SQLite, no close means no checkpoint.** The `-wal` file is
+   durable — SQLite recovers from it — but the schema and every row a
+   deployment had written lived only there. After two boots of the
+   smoke run, `api.db` was still 4096 bytes while `api.db-wal` held
+   461KB. A backup that copied `api.db` alone produced an empty
+   database that opened without error.
+
+Only the third is visible from outside, and it is the one that would
+have cost somebody data.
+
+**What it is now.** `signal.NotifyContext` on `SIGINT`/`SIGTERM` produces
+one `appCtx` that everything hangs off. `net.Listen` is separated from
+`srv.Serve` so a listen failure is an error to report rather than a
+`Fatal` that skips teardown. The main goroutine selects on either
+`Serve` returning on its own or the signal; on the signal it calls
+`drain`, which is `srv.Shutdown` bounded by `shutdownDrainTimeout`
+(30s, a constant in `shutdown.go` with its own reasoning) and falls back
+to `srv.Close` when the bound is hit. Only then does teardown run, in
+the order the components need: `stopSignals()`, `workers.Wait()` for the
+webhook worker and the digest scheduler, then `askAI.Close()`,
+`redisClient.Close()`, and `db.Close()` last — that last one being the
+WAL checkpoint.
+
+**The three properties `shutdown_test.go` pins**, because they are the
+whole reason `drain` is a function instead of three lines in `main`:
+
+- A request already in flight still gets its 200 after the signal
+  arrives, and the drain does not return before it finishes. The test
+  waits for the handler to actually be running rather than sleeping, so
+  it is not racing the client.
+- A request that will not finish does not hold the process open. The
+  bound is honored and the error names the wait, because the operator
+  reading it is looking at a deploy that took too long.
+- A listener that fails on its own reports that error rather than
+  having it translated into a clean shutdown by the
+  `http.ErrServerClosed` check.
+
+**Verified end to end, not just by unit test.** The binary was built,
+started on a fresh `/tmp/walcheck.db`, driven through the full
+`internal/smoketest` run (13/13), sent a real `SIGTERM`, and then the
+database was opened **read-only with no sidecar files present**: 7
+migrations recorded, 1 user, 2 sessions, all read back out of the
+single `.db` file. Before the change the same sequence left 4096 bytes
+and a 461KB `-wal`.
+
+**What this does not do.** It does not make `shiplog`'s writes
+asynchronous — the comment there has been updated from "there is no
+shutdown path to hang off" to "the buffer and the flush policy are the
+missing pieces, not the lifecycle", which is a different and smaller
+problem. It does not add a readiness endpoint separate from `/v1/health`,
+so a load balancer's behavior during the drain is unchanged. And
+`shutdownDrainTimeout` is a constant rather than an env var on purpose:
+if the ask-ai widget's model calls ever stop being bounded by the
+provider's own client timeout, that becomes a knob.

docs/development/NEXT.md

@@ -239,9 +239,10 @@ Two details were decided rather than assumed, and are recorded in
 > behaviour is tested against `httptest` and an in-memory double
 > **rather than against Postgres `FOR UPDATE SKIP LOCKED`**, the
 > in-memory double cannot reproduce two workers racing (one mutex), and
-> this repo still has **no graceful shutdown** — owed before the
-> shipped-events sink could move off the request goroutine. `PROGRESS.md`
-> has all of it.
+> this repo has **no graceful shutdown** — built since, see
+> `CURRENT-STATE.md`'s last section; the async-sink half of this note is
+> still owed and is now only about the buffer and the flush policy.
+> `PROGRESS.md` has all of it.
 >
 > Two deliberate deviations from the spec below, both argued in
 > `PROGRESS.md`: `webhook_deliveries` uses a `BIGSERIAL` surrogate
@@ -346,8 +347,9 @@ Two details were decided rather than assumed, and are recorded in
 >
 > What is still owed from Stage 1:
 >
-> - the digest schedule is a goroutine on `context.Background()`, because
->   this repo still has no graceful shutdown.
+> - ~~the digest schedule is a goroutine on `context.Background()`~~ —
+>   built since: it takes the context the shutdown signal cancels, see
+>   `CURRENT-STATE.md`'s last section.
 >
 > Three things this tier changed that were not in the spec below, all
 > recorded because they are behaviour rather than plumbing:
@@ -669,15 +671,13 @@ avoid duplicating.
   run from a minimal base), published to a registry on the same tag
   trigger. `docker run --env-file .env -p 8080:8080 <image>` should be
   the entire setup instructions.
-- **Graceful shutdown, pulled forward from Tier 3/4's owed list**:
-  this is the tier where it stops being a nice-to-have. A distributed
-  binary or container is exactly what a real orchestrator (Kubernetes,
-  Fly, Railway, plain systemd) sends `SIGTERM` to on every deploy, and
-  right now `main.go` ends at `log.Fatal(ListenAndServe(...))` with the
-  webhook worker and digest scheduler both running on
-  `context.Background()` — nothing stops them cleanly. Wire a real
-  shutdown context, cancel it on `SIGTERM`/`SIGINT`, and give
-  in-flight requests and the background workers a bounded grace period
-  before exiting. Don't ship distribution before this; a container
-  that gets killed mid-migration or mid-webhook-delivery on every
-  rolling deploy is a worse experience than the one being fixed.
+- ~~**Graceful shutdown, pulled forward from Tier 3/4's owed list**~~ —
+  built ahead of this tier, on its own branch, because the original
+  reasoning here was that shipping distribution first would ship a
+  container whose every `docker stop` kills in-flight requests.
+  `main.go` no longer ends at `log.Fatal(ListenAndServe(...))`, and both
+  background workers take the context the signal cancels; see
+  `CURRENT-STATE.md`'s last section. **What remains for this tier is the
+  packaging**: the `Dockerfile`'s `ENTRYPOINT` must run the binary
+  directly rather than through a shell, or `docker stop` signals
+  `/bin/sh` and the whole thing gains nothing.

docs/development/PROGRESS.md

@@ -1393,3 +1393,57 @@ false. 501 is a statement about the deployment, and it is true.
   entries — was left as found, still a Tier 1 documentation pass.
 - `.env.example` documents the two backends at the top.
 - New `migrations/sqlite/README.md`.
+
+## 2026-09-17 — graceful shutdown (not a tier)
+
+On `feat/tier6-sqlite-backend`'s working tree, branched off as its own
+change. This is the item Tiers 3, 5 and 6 each recorded as owed; it was
+done now rather than inside Tier 7 because Tier 7's own spec says not to
+ship distribution before it — a container whose every `docker stop`
+kills in-flight requests is the bug the packaging would have shipped.
+
+**What was wrong, in three parts.** `main.go` ended at
+`log.Fatal(http.ListenAndServe(...))`. (1) No signal handling, so
+`SIGTERM` killed the process mid-request. (2) `log.Fatalf` calls
+`os.Exit`, which runs no defers, so the `defer db.Close()` above it had
+never once run. (3) On SQLite, no close means no WAL checkpoint — this
+was the finding that started it: after two boots of the smoke run,
+`api.db` was still 4096 bytes while `api.db-wal` held 461KB, so a backup
+copying `api.db` alone produced a database that opened without error and
+was empty.
+
+**What was built.** `signal.NotifyContext` on `SIGINT`/`SIGTERM` gives
+one `appCtx` that the HTTP server and both background workers hang off.
+`net.Listen` is separated from `srv.Serve` so a listen failure is
+reported rather than being a `Fatal` that skips teardown. The main
+goroutine selects on either `Serve` returning on its own or the signal;
+on the signal it calls `drain` (`shutdown.go`), which is `srv.Shutdown`
+bounded by a 30s constant and falls back to `srv.Close` when the bound
+is hit. Teardown then runs in the order the components need:
+`stopSignals()`, `workers.Wait()`, `askAI.Close()`, `redisClient.Close()`,
+`db.Close()` last. Both `sync.WaitGroup.Go` (Go 1.25) and the hoisting of
+`redisClient`/`askAI` exist so the teardown has something to close.
+
+**Verified end to end, not just by unit test.** Built the binary,
+started it on a fresh `/tmp/walcheck.db`, ran the full `internal/smoketest`
+(13/13), sent a real `SIGTERM` to the actual server PID, then opened the
+database **read-only with no `-wal`/`-shm` present**: 7 migrations
+recorded, 1 user, 2 sessions. The sidecar files were gone entirely, which
+is the checkpoint. `shutdown_test.go` also pins the three properties
+`drain` exists for: an in-flight request still gets its 200 and the
+drain waits for it; a request that will not finish is abandoned at the
+bound with an error naming the wait; a listener that fails on its own
+reports that error instead of having it translated to a clean shutdown.
+
+**Also corrected**: four doc comments that asserted this repo has no
+shutdown path and are now false (`askai.Service.Close`,
+`webhook.Worker.Run`, `digest.Scheduler.Run`, and `shiplog`'s
+synchronous-write argument). The `shiplog` one changed its *reasoning*,
+not just its wording — the missing piece for an async sink is now the
+buffer and the flush policy, not a lifecycle to hang it off — so that is
+recorded rather than deleted.
+
+**Not done.** `-race` still has not been run. `shutdownDrainTimeout` is
+a constant, not a knob, on purpose. No readiness endpoint separate from
+`/v1/health`, so load-balancer behaviour during the drain is unchanged.
+`go build ./... && go vet ./... && go test ./...` are all clean.