[cristim]

Summary

Closes #664.

StartHTTPServer started the background DB initializer but never waited for it. On SIGTERM it ran server.Shutdown and returned, so main ran defer app.Close() while ensureDB could still be writing app.DB: an unsynchronized read/write, and a pool created just after Close read nil was never closed.

• startBackgroundInit now returns a channel closed when the initializer exits (already closed when there is no dbConfig).
• StartHTTPServer waits on it after server.Shutdown, inside the same 30s shutdownCtx; if the grace period runs out it logs and returns as before.

Out of scope (noted in the issue): runMigrationsBounded takes a pool, not a context, so SIGTERM mid-migration is still not interrupted.

Verification (honest labels)

• TestStartHTTPServerWaitsForBackgroundInitOnShutdown drives the real StartHTTPServer: a secret resolver blocks inside GetSecret (keeps ensureDB running), the test sends SIGTERM, and asserts StartHTTPServer does not return until the initializer exits.
• Fails before the fix ("StartHTTPServer returned while background initialization was still running"), passes after. Behavioral ordering test, run without -race (disk rules); it does not need it.
• go vet, go test ./internal/server (whole package), golangci-lint run ./internal/server/... (0 issues, GOTOOLCHAIN=go1.26.6) all pass locally.
• Expected CI state: Security Scanning (govulncheck GO-2026-6617) and Build Docker Image are red on main for dependency advisories unrelated to this change; the Go 1.26.9 / x/net bump PR is separate. This PR will be rebased onto main once that lands.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 39 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 85 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 0cfeb768-f0b9-436e-9d2e-4458d6f48335

---

📥 Commits

Reviewing files that changed from the base of the PR and between 0e34519 and b764ddc.

---

📒 Files selected for processing (3)

• internal/server/http.go
• internal/server/http_shutdown_test.go
• internal/server/readiness_test.go

---

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Subject: PR 691 (#664) review at head 232fc2c

VERDICT: no blocking findings at 232fc2c. Four P3 test/robustness notes below. CI not checked by me.

Method: read-only. Source from the writer's worktree (HEAD 232fc2c, clean; base e5baaab is an ancestor). Checks via go test -overlay from scratch copies, GOWORK=off GOTOOLCHAIN=go1.26.6, one package (./internal/server), no -race, platform test-suite lockf, df 9.9 GiB. Worktree sha and git status unchanged afterwards. Real StartHTTPServer, fake secret resolver: not a live DB scenario.

1. Fail-before reproduced: base e5baaab http.go + the PR test file: --- FAIL: TestStartHTTPServerWaitsForBackgroundInitOnShutdown (0.00s) http_shutdown_test.go:58: StartHTTPServer returned while background initialization was still running. At 232fc2c: full ./internal/server ok (14.1s); the new test with -count=5 ok.

2. Hang/deadlock review:

• initDone is closed by defer close(done) once per goroutine, or immediately when dbConfig is nil: no double close.
• Wait is select { initDone | shutdownCtx.Done } with the same 30s ctx that Shutdown uses, so Shutdown + init wait are bounded together by 30s. An initializer that ignores ctx (e.g. a secret store call with no timeout) costs at most the grace period, is logged, then StartHTTPServer returns and main's deferred app.Close may still race with it. That residual is the pre-existing behaviour, now bounded and logged; acceptable.
• serveErr branch not waiting: not a problem. main.go:79 calls log.Fatalf on a non-nil error, which exits without running the deferred app.Close (main.go:52).
• Callers of startBackgroundInit: http.go:171 (uses the channel) and readiness_test.go:298 (ignores the return, compiles fine).
• Race closure: app.Close reads app.DB without dbMu; the channel close gives the happens-before from the initializer's write of app.DB, so waiting is the right fix.

3. Test robustness: SIGTERM is sent only after GetSecret is entered, which is after StartHTTPServer installed its NotifyContext, so the process cannot die from it. It is the only signal.Notify/NotifyContext/syscall.Kill user in the repo, so no cross-test interference; other packages run as separate processes. The 300ms window is a negative assertion: pre-fix source returned in 0.00s, so detection has a wide margin, and with the fix the return is lower-bounded by the explicit release, so it cannot flake into a false failure. Note stop() inside the signal case restores default SIGTERM disposition: a second SIGTERM in that test would kill the test process (the test sends one).

4. Mutants (scratch overlays on http.go, tests in ./internal/server):

• wait removed (_ = initDone): killed, same failure as above.
• wait done-closed-early (close(done) before the goroutine): killed.
• bound on wrong signal (time.After(1ms) instead of shutdownCtx.Done): killed.
• wait unbounded (<-initDone, no shutdownCtx bound): SURVIVED.
• dbConfig==nil path leaves done unclosed: SURVIVED (would stall every non-DB shutdown for the full 30s).
• wait placed before server.Shutdown: SURVIVED (ordering not pinned; harmless).

P3 notes (none block):
a. Add an assertion in readiness_test.go:298's no-DB test that the returned channel is already closed; it kills the nil-config mutant, which is the one survivor with a user-visible effect (30s slower shutdown).
b. The 30s bound is untested (hardcoded in StartHTTPServer); fine to leave, but the unbounded mutant surviving means the bound is covered by review only.
c. In the new test, close(r.entered) panics if GetSecret is ever called twice, and release is not closed on early t.Fatal paths, leaving the resolver goroutine blocked until process exit. sync.Once for entered and a t.Cleanup that closes release would harden it.

[cristim]

Subject: PR 691 delta review 232fc2c -> a0a64ba

VERDICT: no blocking findings. 1 non-blocking finding (listener assertion can pass vacuously, ~20% on the wait-before-Shutdown mutant), everything else as claimed. CI not checked by me.

Method: read-only. Delta read with git diff in the writer's worktree (HEAD a0a64ba, clean); go test -overlay from scratch copies, GOWORK=off GOTOOLCHAIN=go1.26.6, ./internal/server only, no -race, platform test-suite lockf, df 20 GiB. Worktree unchanged afterwards. Real StartHTTPServer + fake resolver, not a live DB.

Delta review: http.go change is the only production change: shutdownGracePeriod package var (30s) replaces the literal in context.WithTimeout; no other behaviour change. Tests: Once on entered/release, t.Cleanup unblock, reserved-port helper, SIGTERM helper, Bounded test (200ms grace, return required within 5s), NoDatabase test asserts the channel is closed. Baseline full package ok (14.3s); StartHTTPServer + StartBackgroundInit tests -count=5 ok.

Mutants at a0a64ba (scratch overlays on http.go):

• wait removed (_ = initDone): killed at 232fc2c; at a0a64ba the form that compiled in my harness did not, but the removal equivalent is already the fail-before shown in my 232fc2c review, and the main test's 300ms window is unchanged. Not re-confirmed on this SHA: say if you want it.
• wait unbounded: KILLED (TestStartHTTPServerShutdownWaitIsBounded, 5.00s timeout).
• done closed early: KILLED (main test, :104).
• nil-config done unclosed: KILLED (TestStartBackgroundInitSkipsWhenNoDatabase).
• bound on wrong signal: KILLED (main test).
• grace hardcoded to 30s (new, tests the var): KILLED (Bounded test).
• wait before server.Shutdown: SURVIVED in the combined run; repeated on the main test alone, -count=20: 16 failed ("listener still accepting connections", :97), 4 passed. So the claimed kill is real but not deterministic.

FINDING (P3, test robustness): the order assertion dials the port after SIGTERM and treats the first dial error as "listener closed". The test only waits for the resolver to be entered, which does not imply ListenAndServe has bound the port yet. If SIGTERM and the first dial happen before the bind, the dial fails with connection refused, the loop breaks, and the assertion passes without having observed a closed listener. Suggested fix: before sending SIGTERM, poll-dial until a connection succeeds (bounded, e.g. 5s), then send SIGTERM and require the dial to fail. That makes the assertion deterministic and removes the ~20% escape.

[cristim]

Subject: PR 691 delta review #2 a0a64ba -> 7b4b88d

VERDICT: no findings. The racy-listener note is fixed: the wait-before-Shutdown mutant is now killed 20/20 (was 16/20). CI not checked by me.

Method: read-only. git diff in the writer's worktree (HEAD 7b4b88d, clean); go test -overlay from a scratch copy of http.go, GOWORK=off GOTOOLCHAIN=go1.26.6, ./internal/server only, no -race, platform test-suite lockf, df 16 GiB. Worktree sha and git status unchanged afterwards. Real StartHTTPServer + fake resolver, not a live DB.

Delta: 1 file, +16 lines, internal/server/http_shutdown_test.go only; http.go is byte-identical to a0a64ba (git diff shows no http.go change). startServerWithBlockedInit now polls (10ms step, 100ms dial timeout, 5s deadline, t.Fatalf on timeout) until a TCP connect succeeds before returning, so SIGTERM is only sent once the listener is provably bound. The poll connection is closed immediately; the server sees an idle connection that Shutdown closes, no effect on later assertions.

Evidence:

• Unmutated: StartHTTPServer + StartBackgroundInit tests, -count=30: ok (17.0s), no flake.
• Mutant wait-before-server.Shutdown (same scratch mutant as before), main test alone, -count=20: 20 of 20 FAIL at http_shutdown_test.go:113 "listener still accepting connections while shutdown should have started". Previously 16 of 20.
• Other mutants (unbounded wait, nil-config, done closed early, wrong-signal bound, hardcoded grace) were killed at a0a64ba by tests this delta does not touch; not re-run on 7b4b88d since only the helper's preamble changed.

[cristim]

Delta review 7b4b88d..b764ddc (rebase onto main 0e34519). Reviewer: archera-quotes-kimi (Claude Sonnet 5.5).

Verdict: pure rebase, no findings. The per-file patches of the PR at 7b4b88d (GitHub compare against main, merge base e5baaab) and at b764ddc (PR files API) are byte-identical for all three files (internal/server/http.go, http_shutdown_test.go, readiness_test.go), including hunk headers and context, so the rebase moved no hunk and the 3 commits keep their content and order. The earlier verdicts (no blocking findings; wait-before-Shutdown mutant killed 20/20 at 7b4b88d) therefore carry over. I did not re-run tests or mutants myself for this delta; platform-new reports go vet and go test ./internal/server ok on Go 1.27.1 and 1.26.9.

CI at b764ddc, read just now: the Build & Test run (37928136610) is still pending on most jobs (Unit Tests, Lint Code, Build Docker Image, Security Scanning, E2E and Integration pending); pre-commit passes. The Review gate needs the full CI green on this SHA before merge; that is not yet the case.

[cristim]

Merged at reviewed SHA b764ddc (squash, merge commit 16aec29).
Review chain: archera-quotes-kimi independent review clean at 232fc2c; deltas a0a64ba and 7b4b88d clean; b764ddc is a pure rebase (git diff of each against its own merge-base with main, index lines ignored, is identical: sha 2ac82809...).
CI: all checks on b764ddc successful, mergeStateStatus CLEAN, no unresolved review threads.
Local (macOS, exact-head git archive, GOTOOLCHAIN=go1.26.9): go build ./... exit 0; go test ./internal/server/... exit 0 (changed files: internal/server/http.go, http_shutdown_test.go, readiness_test.go).