test(coverage): drive pool, telemetry, ctxkeys to >=95%

.github/workflows/coverage.yml

@@ -2,28 +2,19 @@ name: coverage
 
 # Coverage CI for the provisioner.
 #
-# Unlike api/ and worker/ — which need real Postgres/Redis/Mongo service
-# containers to exercise their integration test surface — the provisioner's
-# test suite is entirely mock-based at this layer:
+# Most of the provisioner's test surface is mock-based (server_test.go uses
+# in-process mock backends, backend/*_test.go uses unreachable hosts to
+# verify the P2 fail-open contract), but the pool package's DB-integration
+# tests need a real Postgres — the `pool_items` inventory table is the
+# whole reason the package exists, and there's no clean seam to mock the
+# pgxpool.Pool without an invasive interface refactor. So this job runs a
+# postgres service container and exports TEST_DATABASE_URL the same way
+# api/coverage.yml and worker/coverage.yml do.
 #
-#   - internal/server/*_test.go uses mockPostgresBackend / mockRedisBackend /
-#     mockMongoBackend (server_test.go:24-100) — no real DB connections.
-#   - internal/backend/postgres/local_test.go tests pure functions
-#     (connLimitClauseFor, naming) — no live cluster needed.
-#   - internal/backend/redis/local_test.go uses goredis pointed at an
-#     intentionally unreachable address (127.0.0.1:1) to verify the P2
-#     fail-open contract — no real Redis needed.
-#   - internal/backend/mongo/*_test.go tests naming + k8s-route logic — no
-#     real MongoDB.
-#
-# Live-cluster integration tests for the provisioner live in api/e2e under
-# the `e2e` build tag and run against the actual k8s deployment, not in this
-# coverage job. Adding service containers here would slow the job without
-# raising the executable test surface.
-#
-# If a future test needs a real backend, gate it on an env var
-# (e.g. TEST_POSTGRES_CUSTOMERS_URL) AND add the matching service container
-# below — same pattern as api/ and worker/.
+# Tests that don't need Postgres remain DB-free and the new pool tests
+# skip cleanly when TEST_DATABASE_URL is unset — a contributor without a
+# local Postgres still sees green `go test ./...` locally; CI exercises
+# the full surface.
 
 on:
   pull_request:
@@ -38,6 +29,24 @@ jobs:
   coverage:
     runs-on: ubuntu-latest
     timeout-minutes: 10
+    services:
+      postgres:
+        image: postgres:16-alpine
+        env:
+          POSTGRES_USER: postgres
+          POSTGRES_PASSWORD: postgres
+          POSTGRES_DB: postgres
+        ports:
+          - 5432:5432
+        options: >-
+          --health-cmd pg_isready
+          --health-interval 10s
+          --health-timeout 5s
+          --health-retries 5
+    env:
+      # internal/pool/manager_db_test.go gates DB-integration tests on this
+      # var. Same convention as api/ and worker/.
+      TEST_DATABASE_URL: postgres://postgres:postgres@localhost:5432/postgres?sslmode=disable
     steps:
       - uses: actions/checkout@v4
         with:

internal/ctxkeys/keys_test.go

@@ -0,0 +1,101 @@
+package ctxkeys
+
+// keys_test.go — exhaustive coverage for the ctxkeys package.
+//
+// The package is intentionally tiny: a single unexported `contextKey` type
+// and a single exported constant `TeamIDKey`. There is no behaviour to
+// unit-test beyond the round-trip contract every typed-key package shares:
+//
+//   1. A value stored under the typed key MUST be retrievable with the
+//      exact same key — i.e. the constant is stable across reads.
+//   2. The typed key MUST NOT collide with raw-string keys carrying the
+//      same lexical name (the whole reason for the package's existence).
+//   3. The zero context (or any unrelated context) MUST return the typed
+//      key's value as nil — Go's documented context.Value contract.
+//
+// These tests guarantee the public surface is what it claims to be; if a
+// future refactor accidentally re-types TeamIDKey to a string (or exposes
+// the unexported contextKey), the third test below fails to compile or
+// asserts incorrectly.
+
+import (
+	"context"
+	"testing"
+)
+
+// TestTeamIDKey_RoundTrip verifies the basic store/load contract.
+func TestTeamIDKey_RoundTrip(t *testing.T) {
+	const want = "team-abc-123"
+	ctx := context.WithValue(context.Background(), TeamIDKey, want)
+
+	got, ok := ctx.Value(TeamIDKey).(string)
+	if !ok {
+		t.Fatalf("ctx.Value(TeamIDKey) did not return string; got %T", ctx.Value(TeamIDKey))
+	}
+	if got != want {
+		t.Errorf("ctx.Value(TeamIDKey) = %q, want %q", got, want)
+	}
+}
+
+// TestTeamIDKey_EmptyMeansAnonymous documents the empty-string == anonymous
+// convention spelled out in the package doc comment. A consumer that switches
+// on `team_id == ""` to skip namespace labelling must be able to distinguish
+// "key absent" from "key present but empty" — both legitimately mean
+// "no owning team".
+func TestTeamIDKey_EmptyMeansAnonymous(t *testing.T) {
+	// Key absent.
+	if v := context.Background().Value(TeamIDKey); v != nil {
+		t.Errorf("background ctx returned %v for TeamIDKey; want nil", v)
+	}
+
+	// Key present, empty string.
+	ctx := context.WithValue(context.Background(), TeamIDKey, "")
+	if got, ok := ctx.Value(TeamIDKey).(string); !ok || got != "" {
+		t.Errorf("present-but-empty TeamIDKey = (%q, ok=%v); want (\"\", true)", got, ok)
+	}
+}
+
+// TestTeamIDKey_NotStringCollision is the regression test for the whole
+// reason this package exists: a bare string key like "team_id" stored by
+// some other package MUST NOT shadow the typed TeamIDKey, and vice-versa.
+// If a future refactor turns TeamIDKey back into a string, this test fails.
+func TestTeamIDKey_NotStringCollision(t *testing.T) {
+	type stringyKey string
+	const collide stringyKey = "TeamIDKey"
+
+	ctx := context.WithValue(context.Background(), collide, "other-package-value")
+	ctx = context.WithValue(ctx, TeamIDKey, "instant-package-value")
+
+	if got, _ := ctx.Value(TeamIDKey).(string); got != "instant-package-value" {
+		t.Errorf("typed key shadowed by string key: got %q, want %q", got, "instant-package-value")
+	}
+	if got, _ := ctx.Value(collide).(string); got != "other-package-value" {
+		t.Errorf("string key shadowed by typed key: got %q, want %q", got, "other-package-value")
+	}
+}
+
+// TestTeamIDKey_NestedOverride asserts the standard context-chain rule —
+// an inner WithValue under the same key wins. Confirms TeamIDKey is a
+// well-behaved context key.
+func TestTeamIDKey_NestedOverride(t *testing.T) {
+	outer := context.WithValue(context.Background(), TeamIDKey, "outer")
+	inner := context.WithValue(outer, TeamIDKey, "inner")
+
+	if got, _ := outer.Value(TeamIDKey).(string); got != "outer" {
+		t.Errorf("outer = %q, want %q", got, "outer")
+	}
+	if got, _ := inner.Value(TeamIDKey).(string); got != "inner" {
+		t.Errorf("inner = %q, want %q", got, "inner")
+	}
+}
+
+// TestTeamIDKey_Distinct guards against a future second constant being
+// declared with the same iota value — the test trips if `TeamIDKey != 0`
+// (the first iota) because we rely on it being a stable underlying value.
+// If a constant is inserted ABOVE TeamIDKey in the const block, TeamIDKey's
+// iota shifts and this test catches it.
+func TestTeamIDKey_Distinct(t *testing.T) {
+	if int(TeamIDKey) != 0 {
+		t.Errorf("TeamIDKey underlying iota = %d; want 0 — a constant was inserted above it, every WithValue using TeamIDKey now collides with whatever has the new iota=0", int(TeamIDKey))
+	}
+}

internal/pool/manager.go

@@ -231,9 +231,18 @@ func (m *Manager) triggerRefill(resourceType string) {
 	}
 }
 
+// runTickInterval is the cadence of the maintenance loop's periodic
+// health-check tick. Exposed as a package-private var (not a const) so a
+// unit test can shorten it to a few milliseconds and exercise the ticker
+// arm of the select within a sub-second test budget. Production code never
+// reassigns it. Changing the default also changes the load profile against
+// the customer DB (every tick is a count query per resource type) — pick
+// a value above 1s to keep the steady-state cost negligible.
+var runTickInterval = 30 * time.Second
+
 func (m *Manager) run(ctx context.Context) {
 	defer m.wg.Done()
-	ticker := time.NewTicker(30 * time.Second)
+	ticker := time.NewTicker(runTickInterval)
 	defer ticker.Stop()
 
 	for {