Claw ad2ec1aca9 G0 D1 hardened: 3-lineage scrum review on shipped code · 7 fixes applied
Code-review pass after D1 shipped, all three model lineages running
in parallel against the actual Go source (not docs):

Convergent findings (≥2 reviewers — high confidence):
- C1 BLOCK · Run() errCh/select race could silently drop fast bind
  errors. Fixed: net.Listen() now runs synchronously before the
  goroutine; bind errors surface as Run()'s return value.
- C2 BLOCK · scripts/d1_smoke.sh sleep 0.5 races bind on cold boxes.
  Fixed: replaced with poll_health() loop, 5s/svc budget, 50ms poll.
- C3 WARN · LoadConfig silent fallback when file missing. Fixed:
  emits slog.Warn with path + hint when path given but file absent.

Single-reviewer fixes:
- S1 WARN · slog.SetDefault inside Run() mutated global state from a
  library function. Fixed: Run() no longer calls SetDefault.
- S2 WARN · os.IsNotExist → errors.Is(err, fs.ErrNotExist) idiom.
- S6 WARN · smoke double-curl collapsed to single curl -i parse.

Second-pass Opus review on post-fix code caught one more:
- head -1 on curl -i fragile against 1xx interim lines. Fixed:
  awk picks the last HTTP/* status line (robust to 100 Continue).

Accepted with rationale (deferred or planned):
- S3 secrets-in-lakehouse.toml: D2.3 SecretsProvider already planned
- S4 5x cmd/*/main.go duplication: defer until D2 reveals real
  per-service config consumption
- S5 /health log volume: defer post-G0, not on k8s yet
- 2nd-pass theoreticals: clean-exit-no-Shutdown path doesn't trigger,
  defensive defer ln.Close() aspirational, etc.

Verification:
- go build ./cmd/...  exit 0
- go vet ./...         clean
- ./scripts/d1_smoke.sh  D1 acceptance gate: PASSED
- 3-lineage code review · 14 findings · 7 fixed · 0 deferred · 5
  accepted with rationale

Total D1 review coverage across the phase:
- 3 doc-review passes (Opus + Kimi + Qwen) — 13 findings, 10 fixed
- 1 runtime smoke — 1 finding (port 3100 collision), fixed
- 1 code-review parallel pass — 14 findings, 7 fixed
- 1 code-review second pass (Opus) — 1 actionable, fixed
- Cumulative: 29 findings · 19 fixed inline · 5 accepted · 5 deferred

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-28 07:07:50 -05:00

156 lines
4.6 KiB
Go

// Package shared provides common HTTP server bootstrap for every
// Lakehouse-Go service. Each cmd/<service> calls Run with its name,
// bind address, and a route-registration callback. The factory wires
// chi, slog, /health, and graceful shutdown identically across all
// five binaries — the place where uniformity beats per-service
// flexibility.
//
// G1+ note: when queryd needs to drain a cgo DuckDB handle on
// shutdown, the simple shared factory will need a per-service hook
// (an io.Closer slice or an OnShutdown callback). For G0 a plain
// chi.Router + http.Server.Shutdown(ctx) is sufficient.
package shared
import (
"context"
"encoding/json"
"errors"
"log/slog"
"net"
"net/http"
"os"
"os/signal"
"syscall"
"time"
"github.com/go-chi/chi/v5"
"github.com/go-chi/chi/v5/middleware"
)
// HealthResponse is the JSON shape returned by /health on every
// service. Service-specific status hooks can extend it post-G0.
type HealthResponse struct {
Status string `json:"status"`
Service string `json:"service"`
}
// RegisterRoutes is the per-service callback that wires its own
// routes onto the shared router AFTER /health has been mounted.
type RegisterRoutes func(r chi.Router)
// Run boots a chi router with slog logging, the /health endpoint,
// and graceful-shutdown handling. Blocks until SIGINT/SIGTERM or a
// fatal listener error.
//
// The logger is constructed locally and used as the request-logging
// sink. Run does NOT mutate the global slog default — callers that
// want their own slog.Default() should set it before calling Run.
// (Per Kimi review #4: shared library functions shouldn't silently
// mutate package globals.)
func Run(serviceName, addr string, register RegisterRoutes) error {
logger := slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{
Level: slog.LevelInfo,
}))
r := chi.NewRouter()
r.Use(middleware.RequestID)
r.Use(middleware.RealIP)
r.Use(middleware.Recoverer)
r.Use(slogRequest(logger))
r.Get("/health", func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set("Content-Type", "application/json")
_ = json.NewEncoder(w).Encode(HealthResponse{
Status: "ok",
Service: serviceName,
})
})
if register != nil {
register(r)
}
srv := &http.Server{
Addr: addr,
Handler: r,
ReadHeaderTimeout: 10 * time.Second,
}
ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
defer stop()
// Race-safe startup: bind the listener synchronously BEFORE
// returning so a fast bind error (e.g. port already in use) is
// surfaced as Run's return value rather than racing the select.
// Per Opus + Qwen BLOCK #1: the prior pattern could drop bind
// errors when ctx.Done already fired or a fast failure happened
// during select setup.
ln, err := newListener(srv.Addr)
if err != nil {
return err
}
errCh := make(chan error, 1)
go func() {
logger.Info("listening", "service", serviceName, "addr", addr)
if err := srv.Serve(ln); err != nil && !errors.Is(err, http.ErrServerClosed) {
errCh <- err
}
close(errCh)
}()
select {
case <-ctx.Done():
logger.Info("shutdown signal received", "service", serviceName)
case err := <-errCh:
if err != nil {
return err
}
// Server exited cleanly without a signal (unlikely but possible).
return nil
}
shutdownCtx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
defer cancel()
if err := srv.Shutdown(shutdownCtx); err != nil {
return err
}
// Drain errCh so a late error from the listener goroutine
// surfaces as the return value instead of leaking. After Shutdown
// the channel will close on graceful exit; if a real error
// landed first we return it.
if err := <-errCh; err != nil {
return err
}
return nil
}
// newListener binds the TCP listener up-front so bind errors are
// returned synchronously to Run's caller. Extracted into its own
// function for testability + to keep Run readable.
func newListener(addr string) (net.Listener, error) {
return net.Listen("tcp", addr)
}
// slogRequest returns a chi middleware that logs each request via slog.
// Replaces chi's default text logger so all log output stays JSON.
func slogRequest(logger *slog.Logger) func(http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
start := time.Now()
ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
defer func() {
logger.Info("http",
"method", r.Method,
"path", r.URL.Path,
"status", ww.Status(),
"dur_ms", time.Since(start).Milliseconds(),
"req_id", middleware.GetReqID(r.Context()),
)
}()
next.ServeHTTP(ww, r)
})
}
}