feat(briefs): add brief + campaign api and async orchestrator

cmd/campaign-service/server.go

@@ -13,10 +13,12 @@ import (
 	"log/slog"
 	"net/http"
 	"os"
-	"sync"
+	"time"
 
+	briefsvcsvr "github.com/linuxfoundation/lfx-v2-campaign-service/gen/http/lfx_v2_campaign_service_briefs/server"
 	connsvcsvr "github.com/linuxfoundation/lfx-v2-campaign-service/gen/http/lfx_v2_campaign_service_connections/server"
 	svcsvr "github.com/linuxfoundation/lfx-v2-campaign-service/gen/http/lfx_v2_campaign_service_svc/server"
+	briefsvc "github.com/linuxfoundation/lfx-v2-campaign-service/gen/lfx_v2_campaign_service_briefs"
 	connsvc "github.com/linuxfoundation/lfx-v2-campaign-service/gen/lfx_v2_campaign_service_connections"
 	svc "github.com/linuxfoundation/lfx-v2-campaign-service/gen/lfx_v2_campaign_service_svc"
 	"github.com/linuxfoundation/lfx-v2-campaign-service/internal/container"
@@ -39,32 +41,37 @@ func StartServer(ctx context.Context, cfg *config.Config) error {
 
 	// NOTE: debug.LogPayloads() is intentionally NOT applied. Every authenticated
 	// payload carries a BearerToken (a valid JWT), and the connection service's
-	// create/set-credential payloads carry plaintext provider credentials, so
-	// enabling it would leak those secrets into logs. debug.HTTP() IS still
-	// applied below, but in clue v1.2.1 it does not log headers or statuses — it
-	// only propagates the runtime /debug toggle into each request's context (so
-	// debug-level logs elsewhere activate); it decodes no payload.
+	// create/set-credential payloads carry plaintext provider credentials (OAuth
+	// refresh tokens, client secrets), so enabling it would leak those secrets
+	// into logs. debug.HTTP() IS still applied below, but in clue v1.2.1 it does
+	// not log headers or statuses — it only propagates the runtime /debug toggle
+	// into each request's context (so debug-level logs elsewhere activate); it
+	// decodes no payload.
 	endpoints := svc.NewEndpoints(cont.Service)
 
-	// The container always initializes Connections (NewContainer wires it in both
-	// the DB and no-DB paths), so construct the endpoints unconditionally. Fail
-	// loudly if it's unexpectedly nil rather than silently skipping the mount —
-	// a mis-wired container should crash at startup, not serve 404s on the
-	// connection routes.
+	// The container always initializes Connections and Briefs (NewContainer wires
+	// them in both the DB and no-DB paths), so construct the endpoints
+	// unconditionally. Fail loudly if either is unexpectedly nil rather than
+	// silently skipping the mount — a mis-wired container should crash at startup,
+	// not serve 404s on the connection/brief routes.
 	if cont.Connections == nil {
 		return fmt.Errorf("container misconfigured: Connections service is nil")
 	}
 	connEndpoints := connsvc.NewEndpoints(cont.Connections)
+	if cont.Briefs == nil {
+		return fmt.Errorf("container misconfigured: Briefs service is nil")
+	}
+	briefEndpoints := briefsvc.NewEndpoints(cont.Briefs)
 
-	return handleHTTPServer(ctx, cfg, endpoints, connEndpoints, cont)
+	return handleHTTPServer(ctx, cfg, endpoints, connEndpoints, briefEndpoints, cont)
 }
 
-// buildMux constructs the Goa muxer and mounts BOTH the campaign service and the
-// connection service onto it. It is a seam so tests can assert that the
-// connection routes are actually reachable (the bug this fixes — routes that
-// compile but are never mounted return 404) without standing up a full server.
-// It returns an error only for a programmer-level mis-wiring (nil connEndpoints).
-func buildMux(ctx context.Context, cfg *config.Config, endpoints *svc.Endpoints, connEndpoints *connsvc.Endpoints) (goahttp.Muxer, error) {
+// buildMux constructs the Goa muxer and mounts the campaign, connection, and
+// brief services onto it. It is a seam so tests can assert that the routes are
+// actually reachable (the bug this fixes — routes that compile but are never
+// mounted return 404) without standing up a full server. It returns an error
+// only for a programmer-level mis-wiring (nil endpoints).
+func buildMux(ctx context.Context, cfg *config.Config, endpoints *svc.Endpoints, connEndpoints *connsvc.Endpoints, briefEndpoints *briefsvc.Endpoints) (goahttp.Muxer, error) {
 	mux := goahttp.NewMuxer()
 	if cfg.Debug {
 		debug.MountPprofHandlers(debug.Adapt(mux))
@@ -92,27 +99,39 @@ func buildMux(ctx context.Context, cfg *config.Config, endpoints *svc.Endpoints,
 	)
 	svcsvr.Mount(mux, server)
 
-	// Mount the connection routes unconditionally. connEndpoints is always
-	// non-nil (StartServer constructs it and fails loudly if the service is
-	// mis-wired), and the connection service is wired even without a database
-	// (with a nil repo) so its routes return the typed 503 rather than a bare
-	// 404. A nil here would be a programmer error, so fail loudly rather than
-	// silently skipping the mount and serving 404s.
+	// Mount the connection and brief routes unconditionally. Both endpoint sets
+	// are always non-nil (StartServer constructs them and fails loudly if a
+	// service is mis-wired), and both are wired even without a database (nil repo)
+	// so their routes return the typed 503 rather than a bare 404. A nil here would
+	// be a programmer error, so fail loudly rather than silently skipping the mount
+	// and serving 404s.
 	if connEndpoints == nil {
 		return nil, fmt.Errorf("buildMux: connEndpoints is nil (connection routes would be unmounted)")
 	}
 	connServer := connsvcsvr.New(connEndpoints, mux, goahttp.RequestDecoder, goahttp.ResponseEncoder, eh, nil)
 	connsvcsvr.Mount(mux, connServer)
 
+	if briefEndpoints == nil {
+		return nil, fmt.Errorf("buildMux: briefEndpoints is nil (brief routes would be unmounted)")
+	}
+	briefServer := briefsvcsvr.New(briefEndpoints, mux, goahttp.RequestDecoder, goahttp.ResponseEncoder, eh, nil)
+	briefsvcsvr.Mount(mux, briefServer)
+
 	return mux, nil
 }
 
-func handleHTTPServer(ctx context.Context, cfg *config.Config, endpoints *svc.Endpoints, connEndpoints *connsvc.Endpoints, cont *container.Container) error {
-	mux, err := buildMux(ctx, cfg, endpoints, connEndpoints)
+func handleHTTPServer(ctx context.Context, cfg *config.Config, endpoints *svc.Endpoints, connEndpoints *connsvc.Endpoints, briefEndpoints *briefsvc.Endpoints, cont *container.Container) error {
+	mux, err := buildMux(ctx, cfg, endpoints, connEndpoints, briefEndpoints)
 	if err != nil {
 		return err
 	}
 
+	// Track in-flight handlers so shutdown can wait (bounded) for stragglers to
+	// return before the DB pool closes. http.Server.Close does not wait for
+	// handler goroutines, so without this a straggler could touch the pool as it
+	// closes; the tracker is waited on between the forced Close and cont.Close.
+	inflight := middleware.NewInflightTracker()
+
 	var handler http.Handler = mux
 	handler = middleware.RequestIDMiddleware()(handler)
 	if cfg.Debug {
@@ -126,6 +145,12 @@ func handleHTTPServer(ctx context.Context, cfg *config.Config, endpoints *svc.En
 			return p != "/healthz" && p != "/livez" && p != "/readyz"
 		}),
 	)
+	// Wrap the inflight tracker OUTERMOST (applied last), so a request is counted
+	// from the instant it enters the handler chain — before the request-ID / debug /
+	// OTel wrappers. If it were inner, a handler that has started but not yet reached
+	// the inner wrapper would be invisible to Wait, and shutdown could observe zero,
+	// close the pool, and let that straggler touch a closing pool.
+	handler = inflight.Middleware()(handler)
 
 	srv := &http.Server{
 		Addr:              cfg.ServerAddress(),
@@ -135,7 +160,13 @@ func handleHTTPServer(ctx context.Context, cfg *config.Config, endpoints *svc.En
 		IdleTimeout:       constants.DefaultIdleTimeout,
 	}
 
-	return runServerWithContext(ctx, srv, cont)
+	return runServerWithContext(ctx, srv, cont, inflight)
+}
+
+// inflightWaiter is the subset of middleware.InflightTracker the shutdown path
+// needs: wait (bounded) for in-flight handlers to return.
+type inflightWaiter interface {
+	Wait(timeout time.Duration) bool
 }
 
 func errorHandler(logCtx context.Context) func(context.Context, http.ResponseWriter, error) {
@@ -144,7 +175,7 @@ func errorHandler(logCtx context.Context) func(context.Context, http.ResponseWri
 	}
 }
 
-func runServerWithContext(ctx context.Context, srv *http.Server, cont *container.Container) error {
+func runServerWithContext(ctx context.Context, srv *http.Server, cont *container.Container, inflight inflightWaiter) error {
 	serverErr := make(chan error, 1)
 
 	go func() {
@@ -161,25 +192,75 @@ func runServerWithContext(ctx context.Context, srv *http.Server, cont *container
 		slog.InfoContext(ctx, "shutdown initiated")
 	}
 
-	var wg sync.WaitGroup
-	wg.Add(2)
-
-	go func() {
-		defer wg.Done()
-		shutdownCtx, cancel := context.WithTimeout(context.Background(), constants.DefaultShutdownTimeout)
-		defer cancel()
-		if err := srv.Shutdown(shutdownCtx); err != nil {
-			slog.ErrorContext(ctx, "HTTP server shutdown error", log.ErrKey, err)
+	// Shut down in order, not concurrently: first stop the HTTP server (stops
+	// accepting new requests and drains in-flight handlers, so no new dispatch
+	// can be Started), THEN close the container (which drains in-flight dispatch
+	// and only then closes the DB pool). Running these concurrently risks a
+	// just-accepted request Starting a dispatch while the pool is being closed.
+	//
+	// Each phase gets its OWN bounded deadline rather than sharing one context.
+	// A shared context would not bound the total: srv.Shutdown could consume the
+	// whole budget draining HTTP handlers, leaving Container.Close's drain+grace
+	// (whose orchestrator grace timer is otherwise wall-clock, not ctx-bound) to
+	// add its full window on top — pushing the real total past
+	// DefaultShutdownTimeout and risking a SIGKILL mid-drain. Budgeting the
+	// phases separately makes the total a true sum: httpShutdownTimeout +
+	// (dispatchDrainTimeout + CancelGracePeriod) <= DefaultShutdownTimeout, which
+	// container.go asserts at init.
+	// Shutdown ordering is explicit:
+	//   1. graceful srv.Shutdown(httpCtx) — stop accepting, drain handlers.
+	//   2. on httpCtx expiry, force srv.Close() — http.Server.Shutdown RETURNS on
+	//      ctx expiry even if handlers are STILL running; those stragglers could
+	//      then touch the DB pool while container/pool close runs (and pgxpool.Close
+	//      itself can block on a connection a handler still holds). Close() forcibly
+	//      terminates all connections so no handler is still using the pool when it
+	//      closes. Force-close only after a timeout — on a clean Shutdown there are
+	//      no lingering connections and Close() is a harmless no-op.
+	//   3. wait (bounded) for any in-flight handler goroutine to actually RETURN.
+	//      srv.Close() terminates connections but does NOT wait for the handler
+	//      goroutines themselves, so a straggler could still be executing (and
+	//      touching the pool) after Close returns. The inflight tracker lets us
+	//      wait for those goroutines to finish before closing the pool; the wait
+	//      is bounded by whatever remains of the HTTP-shutdown budget so a hung
+	//      handler can never wedge shutdown past its window.
+	//   4. THEN cont.Close(closeCtx) — drain in-flight dispatch, then close the pool.
+	// A residual remains only if a handler outlasts the bounded wait; that window
+	// is tracked under LFXV2-2665.
+	// Reserve HandlerDrainTimeout for the post-force-close inflight wait (step 3),
+	// so the graceful Shutdown gets the rest of the HTTP budget. This guarantees
+	// the inflight wait has a real budget even when Shutdown times out (which would
+	// otherwise exhaust the whole HTTP context).
+	shutdownWait := container.HTTPShutdownTimeout - container.HandlerDrainTimeout
+	if shutdownWait <= 0 {
+		// HandlerDrainTimeout must leave room within the overall shutdown budget;
+		// if misconfigured so it doesn't, fall back to the full budget rather than
+		// a zero/negative deadline that would force-close immediately.
+		shutdownWait = container.HTTPShutdownTimeout
+	}
+	httpCtx, cancelHTTP := context.WithTimeout(context.Background(), shutdownWait)
+	defer cancelHTTP()
+	if err := srv.Shutdown(httpCtx); err != nil {
+		slog.ErrorContext(ctx, "HTTP server shutdown error; force-closing lingering connections", log.ErrKey, err)
+		// Shutdown's ctx expired with handlers still running (or it otherwise
+		// failed). Force-close so no straggler can touch the pool as it closes.
+		if cerr := srv.Close(); cerr != nil {
+			slog.ErrorContext(ctx, "HTTP server force-close error", log.ErrKey, cerr)
 		}
-	}()
-
-	go func() {
-		defer wg.Done()
-		if err := cont.Close(); err != nil {
-			slog.ErrorContext(ctx, "container close error", log.ErrKey, err)
+	}
+	// Wait for in-flight handler goroutines to return before closing the pool. On a
+	// clean Shutdown they are already drained and this returns immediately; on the
+	// force-close path it waits up to the DEDICATED HandlerDrainTimeout (NOT the
+	// HTTP context's remaining time — that is exhausted precisely when Shutdown
+	// timed out and a straggler is running, which would make the wait a no-op).
+	if inflight != nil {
+		if !inflight.Wait(container.HandlerDrainTimeout) {
+			slog.ErrorContext(ctx, "in-flight HTTP handlers did not return before pool close; a straggler may touch the pool (LFXV2-2665)")
 		}
-	}()
-
-	wg.Wait()
+	}
+	closeCtx, cancelClose := context.WithTimeout(context.Background(), container.ContainerCloseTimeout)
+	defer cancelClose()
+	if err := cont.Close(closeCtx); err != nil {
+		slog.ErrorContext(ctx, "container close error", log.ErrKey, err)
+	}
 	return nil
 }

cmd/campaign-service/server_test.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 	"testing"
 
+	briefsvc "github.com/linuxfoundation/lfx-v2-campaign-service/gen/lfx_v2_campaign_service_briefs"
 	connsvc "github.com/linuxfoundation/lfx-v2-campaign-service/gen/lfx_v2_campaign_service_connections"
 	svc "github.com/linuxfoundation/lfx-v2-campaign-service/gen/lfx_v2_campaign_service_svc"
 	"github.com/linuxfoundation/lfx-v2-campaign-service/internal/infrastructure/config"
@@ -25,11 +26,13 @@ import (
 // (any non-404 status, e.g. 401/503, proves it is mounted).
 func TestConnectionRoutesAreMounted(t *testing.T) {
 	endpoints := svc.NewEndpoints(service.NewCampaignService(nil))
-	// The no-DB connection service is a real Service whose routes return a typed
-	// 503 rather than 404 — perfect for proving the route is mounted.
+	// The no-DB connection and brief services are real Services whose routes
+	// return a typed 503 rather than 404 — perfect for proving the route is
+	// mounted.
 	connEndpoints := connsvc.NewEndpoints(service.NewConnectionService(nil, nil))
+	briefEndpoints := briefsvc.NewEndpoints(service.NewBriefService(nil, nil, nil, nil))
 
-	mux, err := buildMux(context.Background(), &config.Config{}, endpoints, connEndpoints)
+	mux, err := buildMux(context.Background(), &config.Config{}, endpoints, connEndpoints, briefEndpoints)
 	if err != nil {
 		t.Fatalf("buildMux: %v", err)
 	}
@@ -41,6 +44,7 @@ func TestConnectionRoutesAreMounted(t *testing.T) {
 		path   string
 	}{
 		{"connection google-ads create", http.MethodPost, "/projects/proj-123/connection-google-ads"},
+		{"brief create", http.MethodPost, "/projects/proj-123/briefs"},
 		{"campaign health livez", http.MethodGet, "/livez"},
 	}
 	for _, tc := range cases {
@@ -56,12 +60,18 @@ func TestConnectionRoutesAreMounted(t *testing.T) {
 	}
 }
 
-// TestBuildMuxNilConnEndpointsFailsLoud verifies the fail-loud guard: a nil
-// connEndpoints (a programmer-level mis-wiring) returns an error rather than
-// silently building a mux with the connection routes unmounted.
-func TestBuildMuxNilConnEndpointsFailsLoud(t *testing.T) {
+// TestBuildMuxNilEndpointsFailsLoud verifies the fail-loud guard: a nil
+// connEndpoints or briefEndpoints (a programmer-level mis-wiring) returns an
+// error rather than silently building a mux with those routes unmounted.
+func TestBuildMuxNilEndpointsFailsLoud(t *testing.T) {
 	endpoints := svc.NewEndpoints(service.NewCampaignService(nil))
-	if _, err := buildMux(context.Background(), &config.Config{}, endpoints, nil); err == nil {
-		t.Fatal("expected buildMux to fail loudly when connEndpoints is nil, got nil error")
+	connEndpoints := connsvc.NewEndpoints(service.NewConnectionService(nil, nil))
+	briefEndpoints := briefsvc.NewEndpoints(service.NewBriefService(nil, nil, nil, nil))
+
+	if _, err := buildMux(context.Background(), &config.Config{}, endpoints, nil, briefEndpoints); err == nil {
+		t.Error("expected buildMux to fail loudly when connEndpoints is nil, got nil error")
+	}
+	if _, err := buildMux(context.Background(), &config.Config{}, endpoints, connEndpoints, nil); err == nil {
+		t.Error("expected buildMux to fail loudly when briefEndpoints is nil, got nil error")
 	}
 }