phase 2 review: fix SSE deadlock, MCP panic, lifecycle 500, NOT NULL bug
Reviewed the phase-2 implementation (parts 2–5) end to end. The suite hung
for 600s and several handlers were never exercised because there were no
tests for the new mutation/event/MCP surface. Fixes:
- CRITICAL: sseListener ran on context.Background() and held a pooled
connection forever, so pool.Close() deadlocked (600s test timeout).
NewHandler now takes a ctx that governs the listener; ListenAndServe and
the test helper cancel it before closing the pool.
- CRITICAL: MCP AddTool panicked ("missing input schema") at construction
under go-sdk v1.6.1 — so NewHandler (and every API handler) panicked.
Added object input schemas to all 8 tools via an objSchema helper.
- HIGH: PatchEntity parsed lifecycle transitions as map[string][]string but
the shape is {from:{to:{requires:[]}}}, so every state-change PATCH 500'd.
Parse the nested shape; allow same-state no-ops.
- HIGH: CreateEntity bound SQL NULL for attributes when omitted, violating
the NOT NULL column (the default only applies when omitted). Default to
'{}'.
- MED: serveSSEWriter ignored the request ctx (per-client goroutine leak on
disconnect) and set an invalid Content-Length: -1. Thread ctx through;
omit the header. writeSSE now nil-checks the flusher (io.Pipe path passed
nil → would have panicked on first event).
- MED: SSE `data:` leaked raw sqlcgen.Event (PascalCase, base64 JSONB).
Emit canonical gen.Event so SSE matches GET /events. Verified live.
- LOW: CreateEntity uses uuid.NewV7 (ADR-0005) + real actor from context in
audit; removed dead bearerAuth; fixed vet unkeyed-field warnings.
Tests (would have caught all of the above): entity create/patch with
If-Match 409/400, valid+invalid lifecycle transitions, idempotency replay,
duplicate-slug 409, abstract-type 422, event+audit side effects, MCP tool
registration. Live smoke test confirmed NOTIFY→listener→SSE delivery.
Also adds the missing Phase 2 deliverable: Gitea Actions CI (vet,
golangci-lint, govulncheck, generated-code drift guard, race tests against
TimescaleDB, docker build) and wires sqlc into `make generate`.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -21,7 +21,6 @@ import (
|
||||
|
||||
"github.com/dtoro/oikos/internal/config"
|
||||
"github.com/dtoro/oikos/internal/db"
|
||||
"github.com/dtoro/oikos/internal/db/sqlcgen"
|
||||
"github.com/dtoro/oikos/internal/httpapi/gen"
|
||||
mcphandler "github.com/dtoro/oikos/internal/mcp"
|
||||
"github.com/go-chi/chi/v5"
|
||||
@@ -53,7 +52,12 @@ type Server struct {
|
||||
|
||||
// NewHandler builds the full HTTP handler: /healthz (unauthenticated,
|
||||
// SG18) + the OpenAPI surface under /api/v1 behind bearer auth.
|
||||
func NewHandler(pool *db.Pool, cfg config.Config) http.Handler {
|
||||
//
|
||||
// ctx governs the lifetime of the background SSE listener goroutine, which
|
||||
// holds a dedicated pooled connection for LISTEN. Callers MUST cancel ctx
|
||||
// before closing the pool — otherwise the held connection never releases
|
||||
// and pool.Close() deadlocks.
|
||||
func NewHandler(ctx context.Context, pool *db.Pool, cfg config.Config) http.Handler {
|
||||
s := &Server{
|
||||
pool: pool,
|
||||
cfg: cfg,
|
||||
@@ -61,8 +65,8 @@ func NewHandler(pool *db.Pool, cfg config.Config) http.Handler {
|
||||
sseSubs: make(map[*sseSubscriber]struct{}),
|
||||
}
|
||||
|
||||
// Start background SSE listener
|
||||
go s.sseListener(context.Background())
|
||||
// Start background SSE listener, tied to ctx for clean shutdown.
|
||||
go s.sseListener(ctx)
|
||||
|
||||
r := chi.NewRouter()
|
||||
r.Use(middleware.Recoverer)
|
||||
@@ -420,46 +424,6 @@ func GetActor(ctx context.Context) *actor {
|
||||
return &a
|
||||
}
|
||||
|
||||
// bearerAuth is the interim Phase 2 auth: a static bearer token
|
||||
// (OIKOS_API_TOKEN / OIKOS_MCP_BEARER_TOKEN from Infisical in prod). In
|
||||
// dev mode with no token configured, requests pass as the operator.
|
||||
// Authentik OIDC JWT validation (operator/viewer scopes) lands later in
|
||||
// Phase 2 — tracked in the plan's AuthN/AuthZ table.
|
||||
func bearerAuth(cfg config.Config) func(http.Handler) http.Handler {
|
||||
tokens := [][]byte{}
|
||||
if cfg.APIToken != "" {
|
||||
tokens = append(tokens, []byte(cfg.APIToken))
|
||||
}
|
||||
if cfg.MCPBearerToken != "" {
|
||||
tokens = append(tokens, []byte(cfg.MCPBearerToken))
|
||||
}
|
||||
devOpen := cfg.APIEnv == "dev" && len(tokens) == 0
|
||||
|
||||
return func(next http.Handler) http.Handler {
|
||||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if devOpen {
|
||||
next.ServeHTTP(w, r)
|
||||
return
|
||||
}
|
||||
auth := r.Header.Get("Authorization")
|
||||
raw, ok := strings.CutPrefix(auth, "Bearer ")
|
||||
if ok {
|
||||
for _, t := range tokens {
|
||||
if subtle.ConstantTimeCompare([]byte(raw), t) == 1 {
|
||||
next.ServeHTTP(w, r)
|
||||
return
|
||||
}
|
||||
}
|
||||
}
|
||||
writeProblem(w, r, http.StatusUnauthorized, "unauthorized",
|
||||
"missing or invalid bearer token")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// Ensure sqlcgen is imported (used in sse.go but referenced here for build safety).
|
||||
var _ = &sqlcgen.Queries{}
|
||||
|
||||
// requestLogger logs one line per request with method, path, status,
|
||||
// duration, and the chi request id.
|
||||
func requestLogger(next http.Handler) http.Handler {
|
||||
@@ -482,7 +446,7 @@ func requestLogger(next http.Handler) http.Handler {
|
||||
func ListenAndServe(ctx context.Context, pool *db.Pool, cfg config.Config) error {
|
||||
srv := &http.Server{
|
||||
Addr: cfg.APIListen,
|
||||
Handler: NewHandler(pool, cfg),
|
||||
Handler: NewHandler(ctx, pool, cfg),
|
||||
ReadHeaderTimeout: 10 * time.Second,
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user