feat(briefs): add brief + campaign api and async orchestrator by mrautela365 · Pull Request #11 · linuxfoundation/lfx-v2-campaign-service

mrautela365 · 2026-07-07T21:58:53Z

Summary

Brief/campaign API contract + handlers + async multi-platform dispatch orchestrator (LFXV2-2626) — the brief/campaign equivalent of the connection design+wiring split.

Stacked on #9 (2556 connection wiring) and #10 (2557 brief/campaign models). Targets main; the diff collapses to just this ticket's code once the bases merge.

design/brief.go — Goa design for the briefs service: briefs (create / get / replace with If-Match / approve / archive), campaigns (async create → JobCreateResponse, get, replace), and job poll (JobPollResponse). All nested under /projects/{projectId}, gated on campaign_manager, ETag/If-Match on writes, no List (Query Service owns lists).
internal/service/orchestrator.go — async multi-platform dispatch. On create it records a queued job, then dispatches per platform in parallel (errgroup.SetLimit(5), context.WithoutCancel), tolerant of partial failure; folds outcomes into queued|running|succeeded|partial|failed and persists one campaign per platform. Platform dispatchers are an injected interface — none registered yet (added as provider adapters land; a missing dispatcher records a failed result), but the job lifecycle + persistence run end to end.
internal/service/brief.go — BriefService handler mapping payloads ↔ models, calling the repos + orchestrator; requires an approved brief before campaign creation; JWT actor extraction for attribution.
container + server.go wire and mount the briefs service.

Orchestrator tests cover all-succeed / partial / all-fail aggregation and the async job lifecycle (race-clean under -race). build/vet/test/lint/format all pass. Live-DB integration + real platform dispatch land later (LFXV2-2559 + the per-provider dispatcher adapters).

LFXV2-2626 · LFXV2-2023

🤖 Generated with Claude Code

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

MRashad26 · 2026-07-08T19:45:07Z

Review findings

The changed files in this PR are generated (OpenAPI schemas, Goa gen/ output) and design DSL. The findings below are in the service/orchestrator code on this branch (introduced in earlier stacked PRs) — surfaced here since they will be live once this PR merges.

internal/service/brief.go:184 — Dangling pointer to loop variable in GetJob()

if j.Error != "" {
    resp.Error = &j.Error  // BUG: pointer to local field
}

j is a local struct value. Taking &j.Error exposes a pointer that may alias a loop variable or stack-allocated value, and the pointed-to string can be modified after the return. Copy the string instead: s := j.Error; resp.Error = &s (or check whether the generated type actually expects *string — if not, drop the pointer entirely).

internal/service/orchestrator.go:115 — JSON marshal error silently swallowed

payload, _ := json.Marshal(results)

If marshaling fails, payload is nil and the job result is permanently stored as null — the job becomes unpollable. Capture the error and record it in the job's error field instead of silently proceeding.

internal/service/orchestrator.go:97-101 — Campaign ownership fields set after Dispatch(), before persist

campaign.JobID, campaign.BriefID, campaign.ProjectID, and campaign.Platform are assigned after Dispatch() returns but before UpsertCampaign(). If UpsertCampaign fails and the goroutine is retried (or a second job picks up the brief), a new job may write a campaign with conflicting JobID ownership, breaking the invariant that each campaign belongs to exactly one job. Set all required fields before dispatch, or validate ownership post-persist.

internal/container/container.go:73 — Nil dispatcher map documented but risky

// No platform dispatchers are registered yet; campaign creation records a job but no dispatch occurs.
orch := service.NewOrchestrator(briefRepo, campaignRepo, jobRepo, nil)

NewOrchestrator correctly initialises an empty map when nil is passed (line 40-42), so this won't panic. The risk is operational: campaign creation silently succeeds and records a JobRunning status that never transitions to JobComplete or JobFailed — jobs will be stuck in running state permanently once real dispatchers are needed. Consider logging a startup warning when no dispatchers are registered so the gap is visible in production logs.

internal/service/orchestrator.go:112 — g.Wait() error ignored

_ = g.Wait()

Individual platform goroutines return nil (recording per-platform failures internally), so g.Wait() will rarely error. However, if the errgroup context is cancelled externally (e.g. pod shutdown), that cancellation signal is lost. At minimum, log the error so context cancellations mid-dispatch are visible.

Address review comments from MRashad26 on the brief/orchestrator code: - brief.go GetJob: copy j.Error into a local before taking its address (don't hand out a pointer aliasing the source struct field). - orchestrator.go: capture the json.Marshal error on the job result and fail the job with a recorded error, instead of silently storing a null result (which made the job unpollable). - orchestrator.go: log a non-nil g.Wait() error so an errgroup context cancellation (e.g. pod shutdown) mid-dispatch is visible, not dropped. - container.go: log a startup warning when no platform dispatchers are registered, so the 'jobs never dispatch' gap is visible in prod logs. - (ownership fields are stamped before persist, as noted.) Resolves the PR #11 review findings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

mrautela365 · 2026-07-08T20:55:41Z

Thanks @MRashad26 — all five addressed in 489edbc (the service/orchestrator code these findings live in):

brief.go:184 dangling pointer — copy j.Error into a local before taking its address.
orchestrator.go:115 marshal swallow — capture the json.Marshal error; on failure the job is set to failed with the error recorded rather than storing a null (unpollable) result.
orchestrator.go:97 ownership before persist — ownership fields (JobID/BriefID/ProjectID/Platform) are stamped before UpsertCampaign; the UNIQUE(brief_id, platform) upsert keeps one campaign per (brief, platform).
container.go:73 nil dispatcher — added a startup slog.Warn when no dispatchers are registered, so the 'jobs never dispatch' gap is visible in prod logs.
orchestrator.go:112 g.Wait() ignored — a non-nil group error (context cancellation, e.g. pod shutdown) is now logged.

All build/test(-race)/lint green.

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

MRashad26

All 5 findings addressed (dangling pointer, marshal error, campaign ownership, nil dispatcher warning, g.Wait() logging). LGTM.

Add the brief/campaign/job persistence layer alongside connections: - domain models (brief, campaign) + brief_port - pgx repos: brief_repo, campaign_repo, job_repo (reuse the shared marshalActor/nullStr/isUniqueViolation helpers from the connection layer) - migration 000002 (campaign_briefs, campaigns, campaign_jobs) Rebuilt cleanly on top of merged #7+#8+#9: the original branch predated the connection wiring and its shared-file versions (container/service) would have reverted #9's DB wiring and the architecture/deployment docs. Reconstructed as the purely-additive brief/campaign files so nothing merged is reverted. The brief/campaign SERVICE + API layer is #11. LFXV2-2557 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

Copilot reviewed 16 out of 33 changed files in this pull request and generated 4 comments.

Files not reviewed (14)

gen/http/cli/lfx_v2_campaign_service/cli.go: Generated file
gen/http/lfx_v2_campaign_service_briefs/client/cli.go: Generated file
gen/http/lfx_v2_campaign_service_briefs/client/client.go: Generated file
gen/http/lfx_v2_campaign_service_briefs/client/encode_decode.go: Generated file
gen/http/lfx_v2_campaign_service_briefs/client/paths.go: Generated file
gen/http/lfx_v2_campaign_service_briefs/client/types.go: Generated file
gen/http/lfx_v2_campaign_service_briefs/server/encode_decode.go: Generated file
gen/http/lfx_v2_campaign_service_briefs/server/paths.go: Generated file
gen/http/lfx_v2_campaign_service_briefs/server/server.go: Generated file
gen/http/lfx_v2_campaign_service_briefs/server/types.go: Generated file
gen/http/lfx_v2_campaign_service_connections/client/cli.go: Generated file
gen/lfx_v2_campaign_service_briefs/client.go: Generated file
gen/lfx_v2_campaign_service_briefs/endpoints.go: Generated file
gen/lfx_v2_campaign_service_briefs/service.go: Generated file

Add the brief/campaign/job persistence layer alongside connections: - domain models (brief, campaign) + brief_port - pgx repos: brief_repo, campaign_repo, job_repo (reuse the shared marshalActor/nullStr/isUniqueViolation helpers from the connection layer) - migration 000002 (campaign_briefs, campaigns, campaign_jobs) Rebuilt cleanly on top of merged #7+#8+#9: the original branch predated the connection wiring and its shared-file versions (container/service) would have reverted #9's DB wiring and the architecture/deployment docs. Reconstructed as the purely-additive brief/campaign files so nothing merged is reverted. The brief/campaign SERVICE + API layer is #11. LFXV2-2557 Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Address the 3 Copilot findings that were open on #10 (merged) — fix them here in the stacked #11 since it owns this area: - migration 000002: replace the full UNIQUE (project_id, event_slug) with a partial unique index excluding archived rows, so archiving a brief frees its (project_id, event_slug) slot and a new brief for the same event can be created. Mirrors the connection tables' partial-unique pattern. - brief_repo / campaign_repo: on the version-mismatch path, surface a non-nil, non-ErrNotFound re-fetch error instead of masking it as 412, consistent with ConnectionRepo.Update (a transient DB error shouldn't tell the caller to retry with a fresh ETag). LFXV2-2626 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…ixes (PR #11, LFXV2-2665) Replaces the held-connection advisory lock (which held one pooled connection across the HTTP dispatch while fn acquired another — deadlock risk at low pool sizes, and a blocking acquire on the non-cancelable dispatch context could hang shutdown) with an atomic claim row: - ClaimCampaignDispatch does INSERT ... ON CONFLICT (brief_id, platform) DO NOTHING of a 'pending' campaign. Exactly one worker across replicas wins (the unique index arbitrates) — no held connection, no blocking lock. A loser reuses the existing row (or, if it's still pending elsewhere, reports in-progress instead of dispatching a duplicate). The pending row also survives an upstream-create-then-crash, making the orphaned upstream campaign recoverable. - Removed WithDispatchLock (and its advisory-lock helper/const). - design: platforms binding array now has MinLength(1) so the schema rejects an empty array (dup rejection stays in the handler — OpenAPI can't express uniqueItems), fixing the generated CLI example. Tests: dispatch-goes-through-claim, claim-error-is-failure, already-claimed-pending-skips (dispatcher not called). gen/ regenerated; build/vet/golangci-lint/okfvalidate/test -race all clean. Address Copilot review comments on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…campaign_id (PR #11) - Shutdown now cancels in-flight dispatch runs (via a shared root context the orchestrator owns) when the drain deadline expires, instead of merely stopping to wait and leaving them running against a closing pool. Runs are parented on that root context rather than context.WithoutCancel. - design: document that PlatformResult.campaign_id is present when ok AND on the specific create-succeeded-but-persist-failed path (so the orphaned upstream id isn't lost), matching the implementation. Test: shutdown drain timeout cancels the dispatch context. gen/ regenerated; build/vet/golangci-lint/okfvalidate/test -race all clean. Address Copilot review comments on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

- Resolve the dispatcher and reuse an already-completed campaign BEFORE claiming, so a platform with no registered dispatcher (the current default) never leaves a permanent 'pending' claim row that blocks the pair forever. - Release the pending claim (DeleteDispatchClaim, status='pending'-guarded, so it can never delete a created campaign) when dispatch fails before the upstream campaign is created, so a transient failure doesn't block retries. - Bound concurrent provider dispatches process-wide with a shared semaphore in the orchestrator; the per-job errgroup SetLimit let N concurrent jobs each get maxParallelDispatch slots, leaving total provider calls unbounded. Tests: no-dispatcher leaves no pending claim; existing behaviors preserved. gen/ unchanged; build/vet/golangci-lint/okfvalidate/test -race all clean. Address Copilot review comments on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…; drain grace (PR #11) - On a dispatch error (which does NOT prove the provider rejected the create — a timeout can leave a campaign created upstream), retain the pending claim instead of releasing it, so a blind retry can't double-create. Only release on definite no-create cases (nil campaign / empty upstream id, i.e. a dispatcher bug). - Shutdown, after cancelling in-flight runs on a drain timeout, now waits a bounded grace for them to unwind before returning, so Container.Close doesn't close the pool mid-statement. - Update stale comments/log that still described the removed advisory-lock design (the shipped design is the atomic claim row) and the context.WithoutCancel note (runs are parented on the root context). build/vet/golangci-lint/okfvalidate/test -race all clean. Address Copilot review comments on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…ch errors (PR #11) - Emit quoted HTTP entity-tags (RFC 7232): brief and campaign responses now return ETag: "3" (via briefETag), matching what parseBriefIfMatch already accepts, so a standards-compliant client can round-trip the validator. - Distinguish dispatcher error kinds: a dispatcher may implement NoUpstreamCreate() to signal the error occurred before any upstream create (input/config validation) — in that case the pending claim is released so the pair can be retried. An ambiguous error (e.g. timeout, which may have created a campaign) still retains the claim to prevent a duplicate. Tests: quoted ETag round-trips; pre-create error releases the claim. gen/ unchanged; build/vet/golangci-lint/okfvalidate/test -race all clean. Address Copilot review comments on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

- Shut down the HTTP server BEFORE closing the container (was concurrent): srv.Shutdown stops accepting requests and drains handlers first, so no new dispatch can be Started, then cont.Close drains in-flight dispatch and closes the pool. Removes the race where a just-accepted request could Start a dispatch while the pool is being closed. - ClaimCampaignDispatch: if the INSERT wins but the follow-up read fails, roll back the just-inserted pending row (best effort) and report claimed=false, so a failed claim doesn't leave a pending row that blocks the pair forever. build/vet/golangci-lint/okfvalidate/test -race all clean. Address Copilot review comments on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

The new migration 000003 up/down SQL files were missing the required license header, failing the License Header Check. Add it to match the other migrations. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…ntainer nits (PR #11) - releaseClaim (DeleteDispatchClaim) now runs on a fresh bounded context detached from the dispatch context. On shutdown/timeout the dispatch ctx is already cancelled, so reusing it would fail the cleanup DELETE and leak the pending claim exactly when we need to free it. - The already-claimed-by-another-worker path now reports a clear "skipped: another concurrent dispatch owns this platform" message (was "in progress", which contradicted the resulting non-ok/failed aggregate for this job). - no-DB startup warning now names both connection AND brief/campaign endpoints; dispatchers is an initialized empty map (not a typed-nil) to remove the accidental-mutation footgun. build/vet/golangci-lint/okfvalidate/test -race all clean. Address Copilot review comments on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…ollback ctx (PR #11) - The terminal job-status write now runs on a context detached from the dispatch context (bounded by jobFinalizeTimeout). On the drain-timeout path Shutdown cancels the run ctx, and using it for the terminal write was guaranteed to fail and leave the job stuck non-terminal — now it always reaches a terminal state. - ClaimCampaignDispatch's rollback-on-read-failure runs on a detached ctx too (the read typically fails because ctx was cancelled, which would also fail the rollback DELETE and leak the placeholder). - Graceful shutdown now shares ONE deadline across both phases: server.go passes the shutdown context into Container.Close(ctx), so HTTP-drain + dispatch-drain are bounded by DefaultShutdownTimeout together rather than summing two independent timeouts that could exceed the Kubernetes grace period. build/vet/golangci-lint/okfvalidate/test -race all clean. Address Copilot review comments on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

If-Match requires the strong comparison function (RFC 7232 §3.1), so a weak validator W/"3" must NOT authorize a conditional write. parseBriefIfMatch now rejects a W/ (or w/) prefixed tag with a 400 instead of stripping the prefix and treating it as a match. Strong quoted tags ("3") and the bare version are still accepted. Test updated to assert weak tags are rejected. build/vet/golangci-lint/ okfvalidate/test -race all clean. Address Copilot review comment on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…out (PR #11) - parseBriefIfMatch strips exactly one BALANCED pair of surrounding quotes and rejects an unbalanced quote (`"3` or `3"`) with a 400, instead of strings.Trim removing any number of quotes and accepting it as version 3. - cancelGracePeriod is now jobFinalizeTimeout+1s (was a fixed 2s < the 10s finalize timeout): a run cancelled at the drain deadline needs at least the finalize window to write its terminal status on the detached context before the pool closes. build/vet/golangci-lint/okfvalidate/test -race all clean. Address Copilot review comments on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…grace to budget (PR #11) - A (nil, nil) dispatcher result or an empty upstream id is ambiguous — it does NOT prove no upstream campaign was created — so RETAIN the pending claim (as with an ambiguous dispatch error) instead of releasing it, preventing a blind retry from double-creating. (Supersedes the earlier release-on-nil behavior; the conservative retain is the safe choice.) - Bound graceful shutdown so drain + post-cancel grace can't overrun the overall budget: dispatchDrainTimeout reduced to 12s and an init() asserts dispatchDrainTimeout + service.CancelGracePeriod <= DefaultShutdownTimeout, so the grace no longer runs an unbounded 11s beyond the shutdown deadline. Exported CancelGracePeriod for that check. build/vet/golangci-lint/okfvalidate/test -race all clean. Address Copilot review comments on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…11) Start now copies the caller-owned platforms slice and config bytes before handing them to the dispatch goroutine, so a caller that reuses or mutates either after Start returns can't race the async run. Cheap and removes the latent aliasing footgun regardless of the current call site. build/vet/golangci-lint/test -race all clean. Address Copilot review comment on PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…#11) The two shutdown phases (srv.Shutdown then Container.Close) shared one 25s context, but the orchestrator's post-cancel grace timer is wall-clock, not ctx-bound. So HTTP drain could consume most of the budget and Container.Close would still add its full drain+grace on top — worst case ~36s, past DefaultShutdownTimeout and any matched terminationGracePeriodSeconds, risking a SIGKILL mid-drain. The old init() assertion silently assumed HTTP drain was instantaneous. Fix (addresses the Dealako review [major] on shutdown budgeting): - Budget the phases separately: HTTPShutdownTimeout and ContainerCloseTimeout (= dispatchDrainTimeout + CancelGracePeriod), each with its own Background()-derived deadline, so the total is a true sum bounded by DefaultShutdownTimeout. Reduced dispatchDrainTimeout 12s->8s so the HTTP phase keeps a positive (6s) share; init() now also panics if the HTTP phase budget is non-positive. - Cap the orchestrator grace wait at the caller's remaining context budget so it can never overrun the reserved phase window. - Correct the server.go comment that wrongly claimed a shared deadline bounded the total. - Tests: TestOrchestrator_ShutdownGraceBoundedByContext and TestShutdownBudgetComposes lock in the invariant. Also run go mod tidy so golang.org/x/sync (now a direct import via errgroup) is no longer marked // indirect (Asitha review hygiene flag). Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

The prior fix budgeted the HTTP and container-close phases separately, but Container.Close still passed Orchestrator.Shutdown a context limited to only dispatchDrainTimeout. Combined with the grace timer being capped at the context's remaining budget, the grace collapsed to ~0: when the drain deadline hit, the pool could close while a just-cancelled dispatch was still persisting job/campaign state — risking stuck jobs or partial writes. Fix (addresses the Copilot finding on container.go:182): - Orchestrator.Shutdown now takes an explicit drainTimeout and runs two separately-budgeted phases: a clean drain bounded by drainTimeout, then (only if that elapses and the OUTER ctx still has budget) a post-cancel grace bounded by CancelGracePeriod and the remaining ctx. The outer ctx must carry the full drain+grace budget. - Container.Close passes the full closeCtx (ContainerCloseTimeout = dispatchDrainTimeout + CancelGracePeriod) and the drain timeout as a param, so the grace phase always has its reserved window. - Tests: ShutdownGivesGraceWhenBudgetRemains proves the grace actually runs when budget remains; ShutdownGraceBoundedByContext still proves it can't overrun the deadline. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…#11) Two durable-dispatch gaps from review: - The dispatch context had no deadline (only cancelled at shutdown), so a provider call that never returns would hold its job "running" and one of the maxParallelDispatch semaphore slots forever. Bound each Dispatch call with providerCallTimeout (2m), derived from the dispatch ctx so a shutdown cancel still propagates. A hung upstream now fails the job and frees the slot. - The one-time startup scan can't recover a job orphaned by a crash younger than staleJobCutoff (too new to look stuck at boot, never re-examined). Add a background StartRecoverySweeper goroutine that re-runs FailStuckJobs every recoverySweepInterval (5m). It is wg-tracked and stops promptly on Shutdown via a dedicated stopSweeper signal (it's maintenance, not in-flight work, so it must not block the dispatch drain). Tests: RecoverySweeperStopsOnShutdown verifies the sweeper is tracked and stops without blocking the drain or firing a tick. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…t (PR #11) Two follow-up review findings: - The post-cancel grace select observed only <-done and the grace timer, not <-ctx.Done(). A caller that cancels ctx (rather than hitting the deadline we pre-accounted for) would still block the full CancelGracePeriod. Added a <-ctx.Done() case so a cancel ends the wait promptly. - A platform's wait on the process-wide dispatch semaphore was unbounded, so a large backlog could keep a job queued longer than staleJobCutoff and the recovery sweep would wrongly fail a still-live job. Bounded the wait with dispatchQueueTimeout (10m); on timeout the platform is recorded failed and the job finalizes. queue + provider-call + finalize now stays within the 15m cutoff, so a progressing job can't look stuck. Updated FailStuckJobs' doc to state the cutoff-vs-live-job invariant and that the sweep is periodic. Test: ShutdownGraceHonorsContextCancel locks in the cancel-during-grace path. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

… on shutdown (PR #11) Addresses copilot[bot] review threads on PR #11: - Add migration 000004: partial index idx_campaign_jobs_recovery on campaign_jobs (updated_at) WHERE status IN ('queued','running') so the periodic FailStuckJobs recovery sweep no longer full-scans the table as terminal job history grows. Up/down provided; concept doc updated. - Give the recovery sweeper a dedicated cancel context (sweeperCtx), cancelled at the very start of Shutdown before the dispatch drain, so a sweep blocked in the DB is interrupted promptly and the sweeper's shutdown never competes with the dispatch-drain budget. Still tracked by wg so the pool isn't closed under it. Removes the detached-context design that could not be interrupted. - server.go: after graceful Shutdown(ctx) expires with handlers still running, force srv.Close() before container/pool close so no straggler handler touches the pool as it closes. Full handler-completion tracking is residual under LFXV2-2665. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…e shutdown order (PR #11) - Close the approve->dispatch TOCTOU race: CreateCampaigns now passes the brief version it read as approved into Orchestrator.Start, which creates the job via a new JobRepository.CreateJobForApprovedBrief. That method re-verifies the brief is still approved at that version atomically with the job insert (INSERT ... SELECT ... WHERE EXISTS on approved+version), so a concurrent replace/archive committing in the window bumps the version, inserts zero rows, and the request fails 409 instead of launching paid campaigns from a stale approved snapshot. Reported by copilot[bot]. - Persist a successful provider result on a context detached from the cancelled dispatch context (context.WithoutCancel + persistResultTimeout). Once the paid upstream campaign exists, recording its id must not be abandoned because phase-two shutdown cancelled the dispatch context; the bounded detached ctx completes within the grace window and cannot hang shutdown. Grew CancelGracePeriod to cover both detached writes (persist + finalize) and trimmed dispatchDrainTimeout so the total stays within the shutdown budget (asserted in container init). Reported by copilot[bot]. - Shutdown ordering: graceful srv.Shutdown is the primary path (waits for handlers up to the deadline), srv.Close is the last-resort force on deadline expiry, and the container/pool close runs only after that bounded HTTP drain. Documented that stdlib cannot await stragglers past Close; residual cross-request handler tracking noted under LFXV2-2665. Reported by copilot[bot]. Fully coordinating handler completion across replicas / cross-request tracking is out of scope and tracked under LFXV2-2665. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Address 4 Copilot review threads on PR #11: - GetJob: a persisted per-platform result that won't decode (JSON error, or a terminal succeeded/partial job with no results) is now surfaced as a 500 InternalServerError instead of a 200 poll response with empty results, so corruption is never masked as success (copilot). - CreateJobForApprovedBrief: the approve->dispatch guard now returns a distinct ErrStaleApproval sentinel, mapped to a 409 whose message tells the client to refresh and re-approve, rather than the misleading uniqueness "already exists" (copilot). Uniqueness ErrConflict keeps its own message. - Container.Close: capture the orchestrator Shutdown error and RETURN it (still always closing the pool) so a drain timeout with dispatches still running is observable to the caller's "container close error" branch, instead of always returning nil (copilot). - Shutdown path: add a lightweight in-flight-handler tracking middleware (WaitGroup per request) and wait on it, bounded by the remaining HTTP budget, between the forced srv.Close and cont.Close, so a straggler handler is actually awaited before the pool closes (copilot, re-flag). Residual window past the bounded wait tracked under LFXV2-2665. - Tests: malformed/empty-terminal/valid-result GetJob cases; stale-approval conflict-message assertion; Close error propagation; inflight tracker Wait. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…#11) Address Copilot: InflightTracker.Wait spawned a goroutine blocked on wg.Wait(); if the timeout elapsed, Wait returned but that goroutine stayed parked until every handler finished — a leak across repeated calls / shutdown retries, and wg.Add racing wg.Wait is unsafe. - inflight.go: dropped the WaitGroup entirely and implemented Wait as a bounded POLL of the existing atomic in-flight counter (5ms ticker up to the timeout). No spawned goroutine, safe to call multiple times, same bounded-wait semantics (returns immediately when already drained, never blocks past the budget). Verified: gofmt, go build, go vet, golangci-lint (0 issues), go test -race ./... (all pass). Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Address Copilot: Wait always blocked for the next 5ms ticker tick before checking the deadline, so a timeout shorter than inflightPollInterval could overshoot the shutdown budget. - inflight.go: Wait now selects on BOTH a deadline timer and the poll ticker, so the deadline fires precisely even for sub-tick timeouts. Verified: gofmt, go build, go vet, golangci-lint (0 issues), go test -race ./internal/middleware. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

#11) Address Copilot: three comments (orchestrator.Start, job_repo guarded INSERT, brief.go CreateCampaigns) said the approve→dispatch TOCTOU guard surfaces as ErrConflict, but the implementation returns domain.ErrStaleApproval (still mapped to 409). Updated the comments to name the sentinel actually returned so the observable error semantics match the docs. Verified: gofmt, go build, go vet, golangci-lint (0 issues). Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…ath doc (PR #11) Address Copilot: - server.go + container.go: the post-force-close inflight-handler wait derived its budget from the HTTP-shutdown context's remaining time — but when srv.Shutdown TIMES OUT that context is already exhausted, so the wait returned immediately and the container/pool closed while stragglers ran (the exact race the tracker exists to prevent). Added a dedicated container.HandlerDrainTimeout (2s, carved from HTTPShutdownTimeout) used for the wait, and reserved it out of the graceful-Shutdown budget so the drain always has real time even on timeout. Removed the now-unused httpDeadline helper. - internal-service.md: corrected the stale claim that a transient existing- campaign fast-path lookup error is recorded as a failure — it now falls through to ClaimCampaignDispatch's atomic claim (claims/reuses safely, no duplicate), matching the implementation. Verified: gofmt, go build, go vet, golangci-lint (0 issues), go test -race ./cmd/... ./internal/container/..., okfvalidate (conformant). Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

- job_repo: replace the lone guarded INSERT ... WHERE EXISTS with a SELECT ... FOR UPDATE row lock + status/version re-check inside one transaction before the job insert. The single-statement guard did not hold under READ COMMITTED (its snapshot could miss a replace/archive that committed just before the statement); the row lock serializes the check-then-insert against any concurrent brief mutation. Returns ErrStaleApproval on the locked-row check failure. (copilot[bot] thread 1) - orchestrator: a single-flight SKIP (another dispatch owns the pair) is no longer recorded as a terminal failure. platformResult gains a Skipped flag; aggregateStatus excludes skips from the failure tally and keeps a job with outstanding skips and no real failure non-terminal (running) so a re-poll after the owner finishes reflects the true outcome. Full cross-request adoption of the owner's result: LFXV2-2665. (copilot[bot] thread 3) - brief: note near CreateBrief that Query Service indexing (lists + revision history) is a deliberate follow-up (LFXV2-2665); this PR is the persistence source of truth. (copilot[bot] thread 2) - docs/architecture.md: campaign_briefs/campaigns project_id is TEXT (project UUID or slug), matching migration 000003 and the API contract; the ER diagram and prose said UUID. (copilot[bot] thread 4) - tests: cover skip-not-fail in aggregateStatus and the orchestrator; add waitForFinalized for jobs that finalize non-terminal. - PR #11. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…EXT (PR #11) Address Copilot: - orchestrator.go: a job whose only non-success was a single-flight SKIP (another dispatch owns the (brief,platform) claim) previously stayed `running` forever — nothing revisits a running job (GetJob only reads it) and the recovery sweeper would eventually fail it after staleJobCutoff, turning a correct deferral into a spurious failure. A skip is a deferral to the owner, not this job's work to finish, so aggregateStatus now terminalizes it as SUCCEEDED (the per-platform result still carries Skipped=true). Full cross-request adoption of the owner's outcome stays tracked under LFXV2-2665. Updated the 3 affected tests. - docs/channel-connections-schema.md: campaign_briefs/campaigns project_id is now TEXT (project UUID or slug) matching migration 000003, and the (project_id, event_slug) uniqueness is the archived-aware PARTIAL unique index — the linked canonical schema no longer contradicts architecture.md / the migration. Verified: gofmt, go build, go vet, golangci-lint (0 issues), go test -race ./internal/service, okfvalidate (conformant). Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…ll (PR #11) Address Copilot: - server.go: moved the inflight tracker to the OUTERMOST middleware (applied last) so a request is counted from the instant it enters the chain — before the request-ID/debug/OTel wrappers. Inner, a handler that started but hadn't reached the inner wrapper was invisible to Wait, so shutdown could observe zero, close the pool, and let that straggler touch a closing pool. - brief.go: the GetJob result-decode shape dropped the persisted `skipped` field, so a skip-only (succeeded) job returned a per-platform ok=false with no explanation — reading as a silent failure. Decode `skipped` and, for a skipped platform, surface an explicit non-failure "skipped: a concurrent request already owns this platform's creation" message via Error (a dedicated skipped field needs a Goa design change, tracked LFXV2-2665). - brief_test.go: TestBriefService_GetJob_SkippedSurfacesNonFailure. Verified: gofmt, go build, go vet, golangci-lint (0 issues), go test -race ./cmd/... ./internal/service. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…clamp Address Copilot review on PR #11: - brief.go: check Skipped before Error when decoding job results so a skipped platform surfaces the explicit "not a failure" message instead of the internal error string the orchestrator also persists - brief.go: reject an empty decoded result slice (null/[]) on a succeeded/partial job as corruption, closing the gap where those JSON values slipped past the raw-length guard - server.go: clamp the derived shutdown wait to a positive value so a misconfigured drain timeout cannot force-close immediately - orchestrator.go/orchestrator_test.go: update stale comments to describe the current skip-only -> succeeded terminalization Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…iliation Dispatch can return a non-nil campaign (carrying the created upstream ID) alongside an error, per the platform clients' partial-result contract. The error branch discarded that campaign, leaving an anonymous pending claim and an unreconcilable orphaned upstream campaign. Persist the upstream ID onto the retained pending row when a dispatch error carries a partial campaign, so the orphan is reconcilable. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

Copilot AI review requested due to automatic review settings July 7, 2026 21:58

mrautela365 requested a review from a team as a code owner July 7, 2026 21:58

Copilot started reviewing on behalf of mrautela365 July 7, 2026 21:59 View session

Copilot AI reviewed Jul 7, 2026

Copilot AI review requested due to automatic review settings July 7, 2026 22:20

Copilot started reviewing on behalf of mrautela365 July 7, 2026 22:20 View session

Copilot AI reviewed Jul 7, 2026

Copilot AI review requested due to automatic review settings July 8, 2026 20:51

Copilot started reviewing on behalf of mrautela365 July 8, 2026 20:52 View session

Copilot AI reviewed Jul 8, 2026

Copilot AI review requested due to automatic review settings July 8, 2026 22:04

Copilot started reviewing on behalf of mrautela365 July 8, 2026 22:04 View session

Copilot AI reviewed Jul 8, 2026

Copilot AI review requested due to automatic review settings July 8, 2026 22:16

Copilot started reviewing on behalf of mrautela365 July 8, 2026 22:16 View session

Copilot AI reviewed Jul 8, 2026

MRashad26 previously approved these changes Jul 9, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 9, 2026 17:58

mrautela365 dismissed MRashad26’s stale review via 20e41b2 July 9, 2026 17:58

mrautela365 force-pushed the feat/LFXV2-2626-brief-campaign-api branch from b5adb67 to 20e41b2 Compare July 9, 2026 17:58

Copilot started reviewing on behalf of mrautela365 July 9, 2026 17:59 View session

MRashad26 previously approved these changes Jul 9, 2026

View reviewed changes

Copilot AI reviewed Jul 9, 2026

View reviewed changes

Comment thread internal/container/container.go Outdated

Comment thread internal/service/brief.go

Comment thread internal/service/brief.go

Comment thread internal/service/brief.go Outdated

Copilot AI review requested due to automatic review settings July 9, 2026 18:18

mrautela365 and others added 29 commits July 15, 2026 09:40

fix(ci): add license header to 000003 migration files (PR #11)

77f3279

The new migration 000003 up/down SQL files were missing the required license header, failing the License Header Check. Add it to match the other migrations. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot AI reviewed Jul 15, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

feat(briefs): add brief + campaign api and async orchestrator#11
mrautela365 wants to merge 44 commits into
mainfrom
feat/LFXV2-2626-brief-campaign-api

mrautela365 commented Jul 7, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

MRashad26 commented Jul 8, 2026

Uh oh!

mrautela365 commented Jul 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

MRashad26 left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

mrautela365 commented Jul 7, 2026

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

MRashad26 commented Jul 8, 2026

Review findings

Uh oh!

mrautela365 commented Jul 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

MRashad26 left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants