diff --git a/doc/rfc/stovepipe/steps/buildsignal.md b/doc/rfc/stovepipe/steps/buildsignal.md index cf0b7f78b..bb3bf1719 100644 --- a/doc/rfc/stovepipe/steps/buildsignal.md +++ b/doc/rfc/stovepipe/steps/buildsignal.md @@ -10,11 +10,11 @@ It handles only the poll loop: it does not decide build strategy, write greennes `buildsignal` consumes a build id, published by `build` — once per phase, since Phase 1 (whole-repo) and Phase 2 (per-project) each run their own `build` → `buildsignal` cycle against builds tied to the same `Request` (see [workflow.md](../workflow.md#workflow)). -Its logic does not branch on phase: it loads the `Build`, polls it toward terminal, persists the result, and publishes the request id onward to `record`. What differs between phases is what `record` does with that publish (whole-repo vs. per-project greenness) — not anything `buildsignal` decides. +Its logic does not branch on phase: it loads the `Build`, polls it toward terminal, persists the result, and queues terminal build analysis. Cancelled outcomes also go directly to `record`. What differs between phases is what `record` does with that publish (whole-repo vs. per-project greenness) — not anything `buildsignal` decides. `buildsignal` is the sole writer of `Build.Status`/`Build.Version` after `build` creates the row (see [build.md](build.md#input-partitioning-and-the-single-writer-property)). It reads `Request` via `RequestStore.Get` (for `R.Queue`, to resolve the build-runner) and writes it exactly once, at the terminal transition, to record the build's outcome — the one `Request.State` write outside `process` and the DLQ reconciler. -Its early-exit guard is deliberately narrower than `State.IsTerminal()`: it proceeds when the request is `processing` **or** already carries a build outcome. The second case matters because a redelivery after the outcome was stamped but before the `record` publish landed must re-publish rather than drop the signal; everything it re-runs is a no-op (the status is unchanged, the outcome is already recorded, the slot is not released twice) and the `record` publish is idempotent. +Its early-exit guard is deliberately narrower than `State.IsTerminal()`: it proceeds when the request is `processing` **or** already carries a build outcome. The second case matters because a redelivery after the outcome was stamped but before the terminal-analysis publish landed must re-publish rather than drop the signal; everything it re-runs is a no-op (the status is unchanged, the outcome is already recorded, the slot is not released twice) and the downstream publish is idempotent. The states the guard actually rejects are terminal-without-an-outcome — today `superseded` alone, which `process` sets only from `accepted`, so it cannot reach the poll loop at all. The guard is kept rather than dropped because the slot release in step 7a is conditioned on *not* already having an outcome, so a superseded request arriving here would decrement a slot that superseding never claimed. @@ -34,7 +34,7 @@ For a delivery carrying build id `B`: 3. If R.State is neither processing nor already an outcome: ack and return. - a request reaches the poll loop only after process admitted it, so it is processing — or already carries this build's outcome on a redelivery, which falls through so the - signal is re-published to record instead of dropped. + terminal analysis is re-published instead of dropped. - the only other terminal-without-an-outcome state is superseded, and process supersedes solely from accepted, so it cannot occur here. Guarded anyway: step 7a would otherwise release a build slot the request never claimed, since superseding consumes none. @@ -68,14 +68,13 @@ For a delivery carrying build id `B`: succeeded -> succeeded, failed -> failed, cancelled -> cancelled. First writer wins. c. If the winning durable R.State is succeeded, advance the last-green bookmark in a CAS retry loop, even on a terminal redelivery. Preserve a newer bookmark. - Then publish R.ID to the record topic, partitioned by request id; ack, return. - No re-publish to buildsignal. - - record loads the Request directly by this key, derives greenness from its outcome, and - supplies its terminal build id to project-result resolution; it never reaches a Build. - - the message id is the request id, so a redelivery republishing the same terminal signal - dedups into the original message; record is idempotent regardless. - - publish failure -> return raw (non-retryable); the outcome is persisted, operational - republish recovers. + Then publish terminal BuildAnalysis, partitioned by build id. Cancelled requests + also publish R.ID directly to record, partitioned by request id. Ack and return. + - successful or failed outcomes reach record through completed build analysis + and project analysis, not directly from this poll. + - stable handoff IDs make republishing safe. + - publication failures return raw; DLQ recovery repairs the handoff without + polling the runner again. 8. Else hold the delivery for delayMs (postpone: the same message redelivers after the delay): - delayMs = pollDelay(status): shorter while running, longer while accepted. @@ -150,8 +149,8 @@ When the budget does run out the message dead-letters, and the buildsignal DLQ r Every branch is safe under at-least-once redelivery: - **Build not found** — non-retryable; storage's read-after-write guarantee means a miss here is a storage defect, not a lag condition to retry through. -- **Status already persisted** — a redelivery re-runs the whole algorithm from step 1, including a redundant `Status` poll (harmless — the runner reports the same thing); step 6 no-ops on the unchanged status, and the delivery proceeds to hold for the next poll (non-terminal) or republish the request id to `record` (terminal, idempotent). No corruption. -- **Terminal already published** — a redelivery reloads, re-polls, no-ops at step 6, republishes the same terminal signal to `record` (idempotent), and acks. Harmless. +- **Status already persisted** — a redelivery re-runs the whole algorithm from step 1, including a redundant `Status` poll (harmless — the runner reports the same thing); step 6 no-ops on the unchanged status, and the delivery proceeds to hold for the next poll (non-terminal) or retry the terminal-analysis handoff (terminal, idempotent). No corruption. +- **Terminal already published** — a redelivery reloads, re-polls, no-ops at step 6, retries the same terminal-analysis handoff (idempotent), and acks. Harmless. - **Postpone write failed** — the framework abandons the delivery; the visibility timeout lapses into a normal redelivery, which re-runs from step 1 and no-ops at step 6. The poll loop's continuation is framework-owned. The window to guard is between persisting status (step 6) and ack (steps 7–8); because status writes are CAS-guarded, monotonic, and write-once at terminal, a redelivery always observes a consistent row. @@ -169,4 +168,4 @@ One boundary of that posture is worth stating: the `MaxAttempts` path fires only ## Entity, storage, and queue additions -No additions beyond [build.md](build.md#entity-and-storage-additions-needed): `buildsignal` calls `BuildStore.Get`/`Update` and `RequestStore.Get`/`Update` against the `Build`/`Request` shapes defined there — `Build.ID` being the runner-assigned id it hands straight back to `Status` — plus `QueueStore.Get`/`Update` to release the build slot, and consumes the `BuildSignal` message on `TopicKeyBuildSignal` introduced there. A non-terminal poll calls `delivery.Hold` on that delivery rather than publishing another one. `Request.State` gains the three build outcomes (`succeeded`, `failed`, `cancelled`), all terminal. The message it publishes to `record`, and the `record` topic key itself, are owned by the `record` stage and land with `record.md`; `buildsignal` only needs that the **request id** reaches the record topic once the build is terminal, partitioned by request id. +No additions beyond [build.md](build.md#entity-and-storage-additions-needed): `buildsignal` calls `BuildStore.Get`/`Update` and `RequestStore.Get`/`Update` against the `Build`/`Request` shapes defined there — `Build.ID` being the runner-assigned id it hands straight back to `Status` — plus `QueueStore.Get`/`Update` to release the build slot, and consumes the `BuildSignal` message on `TopicKeyBuildSignal` introduced there. A non-terminal poll calls `delivery.Hold` on that delivery rather than publishing another one. `Request.State` gains the three build outcomes (`succeeded`, `failed`, `cancelled`), all terminal. Terminal analysis owns the next normal handoff; only cancellation publishes directly to the record topic. Both messages use their existing build-id or request-id partitions. diff --git a/doc/rfc/stovepipe/steps/record.md b/doc/rfc/stovepipe/steps/record.md index 8bfa58cef..0cf6b587d 100644 --- a/doc/rfc/stovepipe/steps/record.md +++ b/doc/rfc/stovepipe/steps/record.md @@ -2,14 +2,14 @@ `record` turns a terminal build outcome into a durable validation fact. -- Phase 1 records whole-repository greenness and optional project facts. On green it promotes the commit onto the Queue's promotion ref if the Request still holds the last-green bookmark, then announces the outcome as a hook event. This is implemented: see [stovepipe/controller/record/record.go](../../../../stovepipe/controller/record/record.go). +- Phase 1 records whole-repository greenness using project results already persisted by `projectanalysis`. On green it promotes the commit onto the Queue's promotion ref if the Request still holds the last-green bookmark, then announces the outcome as a hook event. This is implemented: see [stovepipe/controller/record/record.go](../../../../stovepipe/controller/record/record.go). - Phase 2 records greenness per project instead of per repository. Sketched here, to be expanded before implementation. Notifying downstream systems rides the cross-domain hook framework rather than a Stovepipe-specific extension; see [Hooks](#hooks). See [workflow.md](../workflow.md) for the whole pipeline, [build.md](build.md) for how builds are created, and [buildsignal.md](buildsignal.md) for the terminal-only handoff into this stage. -`record` owns the validation facts and the promotion ref. `buildsignal` advances the last-green build baseline before handing off to this stage. `record` does not decide build scope, poll a build, release the Queue's build slot, interpret a target graph, or map targets to projects. Those belong to `process`, `buildsignal`, and `analyze`. +`record` owns the repository fact and promotion ref; `projectanalysis` owns project facts and issue attribution. `buildsignal` advances the last-green build baseline before handing off to this stage. `record` does not decide build scope, poll a build, release the Queue's build slot, interpret a target graph, or map targets to projects. Those belong to `process`, `buildsignal`, and `projectanalysis`. ## Phase 1 algorithm @@ -39,8 +39,8 @@ For a delivery carrying a `Record` payload: - superseded -> ack, no fact. Unreachable in practice. - accepted / processing / anything else -> return a non-retryable invariant error. -6. Resolve optional project results using R and its TerminalBuildID, and validate - the entire result set before writing facts. A resolver error returns raw. +6. Load the request summary and its persisted ProjectCount. Project analysis + has already recorded results or abandonment before publishing here. 7. Map R.State and the project results to a whole-repository degree and create the ValidationFact keyed by (R.URI, empty project) in the queue-bound fact store. @@ -50,9 +50,8 @@ For a delivery carrying a `Record` payload: - ErrAlreadyExists -> load and reconcile the existing immutable fact. - other store error -> return raw. -8. Record each project fact with its resolver-provided degree and persist history - for the repository fact and completed project facts. A failure returns raw; - replay completes missing writes without replacing immutable facts. +8. Persist history for the repository fact. A failure returns raw; replay adopts + the immutable fact and repairs its history. 9. If the persisted repository fact is not green, skip to step 11. Report how long the break went undetected only if step 7 created the fact (see "Observability"). @@ -108,7 +107,7 @@ Absence is still distinct from degree `0`. Callers gating deployments must treat A fact is written only when the Request reaches `succeeded` or `failed`. A `cancelled` build is acked with no fact. -The fail-closed path is subtler than "no fact". A DLQ reconciler forces the Request to `failed` and does not itself publish here, so reconciliation on its own records nothing. That is not the same as nothing being recorded: a `buildsignal` delivery still in flight can reach this stage afterwards, and it will write `DegreeBroken` from the forced state. See [DLQ and fail-closed behavior](#dlq-and-fail-closed-behavior). +Project-analysis recovery hands off after retaining abandonment. A failed request without a completed set of project facts therefore records `DegreeBroken`; partial writes alone do not supply a project count. Build-status recovery also repairs the downstream handoff without polling the runner again. See [DLQ and fail-closed behavior](#dlq-and-fail-closed-behavior). ### Phase 1 degree mapping @@ -123,7 +122,7 @@ Whole-repository degree depends on the outcome `buildsignal` stamped on the Requ | `cancelled` | no fact | -`DegreePartial` is hard coded to `0.5` for now; it does not measure the fraction of projects failing. Each project fact preserves the degree returned by the resolver, unless its immutable identity already exists. After persisting every project fact and its degree index, `record` retains total and broken project counts in `project_facts_recorded` for the request summary. Broken means a persisted degree above zero; retries count adopted facts rather than contradictory resolver results. Empty results record zero counts. +`DegreePartial` is hard coded to `0.5` for now; it does not measure the fraction of projects failing. Each project fact preserves the degree returned by the resolver, unless its immutable identity already exists. After persisting every project fact and its degree index, `projectanalysis` retains total and broken project counts in `project_facts_recorded` for the request summary. Broken means a persisted degree above zero; retries count adopted facts rather than contradictory resolver results. Empty results record zero counts. ## Last-green advancement (Queue bookmark) @@ -131,7 +130,7 @@ Whole-repository degree depends on the outcome `buildsignal` stamped on the Requ The write goes through `QueueStore.Update(ctx, queue, oldVersion, newVersion)`, so version arithmetic stays in the controller: it computes `newVersion = oldVersion + 1` and the store does a pure conditional write. `ErrVersionMismatch` is absorbed by the loop rather than returned, since a concurrent writer moved the row and reloading converges. -The bookmark is a successful-build baseline and can precede validation facts. Project resolution and Git promotion remain in `record`. Releasing capacity and advancing the bookmark are separate Queue updates, so a new build may be admitted with the previous baseline before the advance lands. A terminal redelivery retries the bookmark update without releasing the slot again. +The bookmark is a successful-build baseline and can precede validation facts. Project resolution runs in `projectanalysis`; Git promotion remains in `record`. Releasing capacity and advancing the bookmark are separate Queue updates, so a new build may be admitted with the previous baseline before the advance lands. A terminal redelivery retries the bookmark update without releasing the slot again. A failed or cancelled build never moves the bookmark, and so never promotes either. diff --git a/doc/rfc/stovepipe/workflow.md b/doc/rfc/stovepipe/workflow.md index f385371e0..2d69f04f5 100644 --- a/doc/rfc/stovepipe/workflow.md +++ b/doc/rfc/stovepipe/workflow.md @@ -4,11 +4,11 @@ Stovepipe answers one question for the rest of the company: **at which commit is The pipeline is a queue-driven chain of small, single-purpose controllers, in the same style as SubmitQueue (SQ). Each controller consumes one topic, advances one entity, and publishes to the next topic. Most hops carry only an **ID** and the controller reloads the entity from storage; the entry hop carries the caller's input because there is no row to load yet. What runs today is: -> **poll for a new head → ingest → process the build strategy → build → buildsignal → record whole-repo greenness (and any project results the resolver returns) → hook.** +> **poll for a new head → ingest → process the build strategy → build → buildsignal → terminal build analysis → project analysis → record whole-repo greenness → hook.** -The [build issue observation](steps/buildanalysis.md) branch runs alongside this chain. It accumulates issues while builds run and attempts final analysis after build completion. Record does not wait for analysis to succeed. The reference host wires a no-op detector; deployments supply a real detector factory. +The [build issue observation](steps/buildanalysis.md) branch runs alongside this chain. It accumulates issues while builds run and performs final analysis after build completion. Repository recording follows project analysis, either after successful completion or recovery has recorded abandonment. The reference host wires a no-op detector; deployments supply a real detector factory. -There is no `analyze` controller and no `analyze` topic. A later project-analysis stage that would map a target graph onto another `build` pass is design only, described under [Designed, not built](#designed-not-built-project-analysis). `record` resolves project results inline on the same delivery that writes the repository fact. +`projectanalysis` resolves project status and issue attribution after final build issues are durable. It does not start another build; project-scoped revalidation remains a [future design](#future-project-scoped-revalidation). ## What Stovepipe is agnostic about @@ -45,7 +45,7 @@ Greenness is recorded as a **health degree** where **`0` means green** and **hig ### Project — greenness at a finer grain -A **project** is a caller-defined slice of the repository. Whole-repo greenness answers "is the branch green at this URI"; project greenness answers the question deployments actually need — **"is *this project* green at this URI"**, and its dual, "what is the latest URI at which this project is green". The running pipeline does not derive projects from a target graph. On a succeeded or failed request, `record` gives the request's terminal build id to `projectstatus.Resolver` and writes one validation fact per result it returns. The example server wires the noop resolver, which returns no results. How a resolver chooses projects is implementer-specific. The separate analyze stage that would map a target graph to project-scoped builds is not built; see [Designed, not built](#designed-not-built-project-analysis). +A **project** is a caller-defined slice of the repository. Whole-repo greenness answers "is the branch green at this URI"; project greenness answers the question deployments actually need — **"is *this project* green at this URI"**, and its dual, "what is the latest URI at which this project is green". The running pipeline does not derive projects from a target graph. On a succeeded or failed request, `projectanalysis` passes the request to `projectstatus.Resolver` and records one validation fact per result it returns. The example server wires the noop resolver, which returns no results. How a resolver chooses projects is implementer-specific. The separate analyze stage that would map a target graph to project-scoped builds is not built; see [Future project-scoped revalidation](#future-project-scoped-revalidation). ### Promotion ref — the last green commit, by name @@ -66,7 +66,7 @@ Hooks are the notification boundary. When validation of a commit begins, when a ## Workflow -What runs is one pass per Request. It establishes whole-repository greenness, and on a succeeded or failed outcome `record` also gives the Request's terminal build id to `projectstatus.Resolver` before writing whatever project facts it returns. That call is inline on the record delivery. It does not publish to another stage, and it does not start another build. +Each request has one build. Terminal build analysis finishes collecting issues before project analysis records project facts and issue attribution. Repository recording then consumes the persisted project count; it does not call the resolver or start another build. Build creation also starts background issue detection; the diagram shows the terminal handoff used for project results. ``` external poller ──(Queue name)──► ┌──────────────────────────────┐ @@ -101,12 +101,25 @@ What runs is one pass per Request. It establishes whole-repository greenness, an │ Poll until terminal; record │ │ status; release the slot; │ │ save outcome; advance green │ + └───────────────┬──────────────┘ + │ BuildID + ▼ + ┌──────────────────────────────┐ + │ buildanalysis │ + │ Persist final build issues │ + └───────────────┬──────────────┘ + │ RequestID + ▼ + ┌──────────────────────────────┐ + │ projectanalysis │ + │ Record project results and │ + │ issue/project associations │ └───────────────┬──────────────┘ │ RequestID ▼ ┌──────────────────────────────┐ Hooks │ record │┄┄┄┄┄► "URI green, - │ Resolve project results; │ not green, or + │ Use recorded project count; │ not green, or │ record facts; on green move │ cancelled" │ the promotion ref; │ │ announce recorded outcome │ @@ -117,12 +130,16 @@ What runs is one pass per Request. It establishes whole-repository greenness, an 1. **ingest** — invoked by the external poller with a **Queue name**. It asks `SourceControl` for that Queue's current head URI, mints a Request namespaced by the Queue, persists it with no recorded greenness yet, and dedups on `(Queue, head URI)` so a re-reported head is processed once. It publishes the RequestID onward. It can also record an optional [enablement decision](queue-state.md) at that head. 2. **process** — decides build strategy (incremental since last-green vs full monorepo), gates concurrent work per Queue, coalesces backlog to the latest head, publishes a **hook event** announcing that validation of the commit has begun, and publishes to `build`. See [process.md](steps/process.md). 3. **build** — runs the build-runner for the chosen scope. A flag derived from `process` decides whether to build relative to the last-green **baseline URI** (incremental) or from scratch (full). It records a build and publishes the BuildID. -4. **buildsignal** — polls until terminal, records the status, releases the build slot, persists the winning Request outcome, and advances a successful-build baseline before publishing the RequestID to `record`. Slot release and baseline advancement are separate updates, so the next admission may still select the previous baseline. -5. **record** — resolves project results, then records repository degree `0` for success, `0.5` for failure with project results, or `1` for failure without results. Project facts preserve their resolved degrees. Once facts and history are durable, a green fact holding the current baseline promotes in Git; the recorded hook follows. A cancelled Request writes no fact and publishes `validation.repository.cancelled`. +4. **buildsignal** — polls until terminal, records the status, releases the build slot, persists the winning Request outcome, and advances a successful-build baseline before requesting terminal build analysis. Cancelled requests bypass project analysis and go directly to `record`. Slot release and baseline advancement are separate updates, so the next admission may still select the previous baseline. +5. **buildanalysis** — persists final build issues and completion history before queuing project analysis. +6. **projectanalysis** — resolves project status and issue attribution, persists their results, and queues repository recording. Recovery retains incomplete analysis and still hands off. +7. **record** — uses the persisted project count and records repository degree `0` for success, `0.5` for failure with project results, or `1` for failure without results. Project facts preserve their resolved degrees. Once facts and history are durable, a green fact holding the current baseline promotes in Git; the recorded hook follows. A cancelled Request writes no fact and publishes `validation.repository.cancelled`. -### Designed, not built: project analysis +### Future project-scoped revalidation -An `analyze` stage is not part of the pipeline. `stovepipe/core/messagequeue` has no analyze topic, and `stovepipe/controller` has no analyze package. The design, still only a design, is a stage that would take a build's target graph, map broken or at-risk targets to projects, and publish project-scoped builds back through `build` → `buildsignal` → `record`. That second pass, a per-project hook event, and intermediate greenness degrees are not what the controllers do. Open questions for that design stay at the bottom of this doc. +The current project-analysis stage reports results from the existing build. A +separate design could dispatch new project-scoped builds for broken or at-risk +targets. That second pass and per-project hook events are not implemented. ## Per-controller summary @@ -132,7 +149,9 @@ An `analyze` stage is not part of the pipeline. `stovepipe/core/messagequeue` ha | **process** | RequestID | build, hook topic | Build strategy, concurrency gate, backlog coalescing; announce validation start on admit → [process.md](steps/process.md) | | **build** | RequestID | buildsignal | Run the build-runner for the chosen scope; baseline = last-green URI iff incremental | | **buildsignal** | BuildID | record, buildanalysis | Poll until terminal, release capacity, persist the outcome, advance a successful baseline, and publish downstream work | -| **record** | RequestID | hook topic | Resolve projects, record repository and project facts, promote a green baseline holder, and publish the repository hook | +| **buildanalysis** | BuildID | projectanalysis | Persist final build issues before handing off | +| **projectanalysis** | RequestID | record | Record project facts and issue attribution, or retain abandonment | +| **record** | RequestID | hook topic | Use persisted project results, record the repository fact, promote a green baseline holder, and publish the repository hook | | **hook** | HookEvent | — | Invoke the hooks the resolver returns. The example server resolves every event to noop | ## Step RFCs diff --git a/service/stovepipe/server/BUILD.bazel b/service/stovepipe/server/BUILD.bazel index 27a199acd..b4dd2ed29 100644 --- a/service/stovepipe/server/BUILD.bazel +++ b/service/stovepipe/server/BUILD.bazel @@ -32,6 +32,7 @@ go_library( "//stovepipe/controller/fix:go_default_library", "//stovepipe/controller/issuetrack:go_default_library", "//stovepipe/controller/process:go_default_library", + "//stovepipe/controller/projectanalysis:go_default_library", "//stovepipe/controller/record:go_default_library", "//stovepipe/core/messagequeue:go_default_library", "//stovepipe/core/requestlog:go_default_library", @@ -41,6 +42,7 @@ go_library( "//stovepipe/extension/buildrunner/fake:go_default_library", "//stovepipe/extension/fixer:go_default_library", "//stovepipe/extension/fixer/noop:go_default_library", + "//stovepipe/extension/projectissues/noop:go_default_library", "//stovepipe/extension/projectstatus/noop:go_default_library", "//stovepipe/extension/queueconfig/default:go_default_library", "//stovepipe/extension/sourcecontrol:go_default_library", diff --git a/service/stovepipe/server/main.go b/service/stovepipe/server/main.go index 96802c72a..bdd3dc5e6 100644 --- a/service/stovepipe/server/main.go +++ b/service/stovepipe/server/main.go @@ -54,6 +54,7 @@ import ( "github.com/uber/submitqueue/stovepipe/controller/fix" "github.com/uber/submitqueue/stovepipe/controller/issuetrack" "github.com/uber/submitqueue/stovepipe/controller/process" + "github.com/uber/submitqueue/stovepipe/controller/projectanalysis" "github.com/uber/submitqueue/stovepipe/controller/record" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" "github.com/uber/submitqueue/stovepipe/core/requestlog" @@ -63,6 +64,7 @@ import ( buildrunnerfake "github.com/uber/submitqueue/stovepipe/extension/buildrunner/fake" "github.com/uber/submitqueue/stovepipe/extension/fixer" fixernoop "github.com/uber/submitqueue/stovepipe/extension/fixer/noop" + projectissuesnoop "github.com/uber/submitqueue/stovepipe/extension/projectissues/noop" projectstatusnoop "github.com/uber/submitqueue/stovepipe/extension/projectstatus/noop" queueconfigdefault "github.com/uber/submitqueue/stovepipe/extension/queueconfig/default" "github.com/uber/submitqueue/stovepipe/extension/sourcecontrol" @@ -420,7 +422,7 @@ func registerPrimaryControllers( } count++ - analysisController := buildanalysis.NewController(logger, scope, store, materializer, detectorFactory, stovepipemq.TopicKeyBuildAnalysis, "stovepipe-buildanalysis") + analysisController := buildanalysis.NewController(logger, scope, store, materializer, detectorFactory, registry, stovepipemq.TopicKeyBuildAnalysis, "stovepipe-buildanalysis") if err := c.Register(analysisController); err != nil { return count, fmt.Errorf("failed to register buildanalysis controller: %w", err) } @@ -438,7 +440,13 @@ func registerPrimaryControllers( } count++ - recordController := record.NewController(logger, scope, store, materializer, projectstatusnoop.New(), sourceControl, registry, stovepipemq.TopicKeyRecord, "stovepipe-record") + projectController := projectanalysis.NewController(logger, scope, store, materializer, projectstatusnoop.New(), projectissuesnoop.New(), registry, stovepipemq.TopicKeyProjectAnalysis, "stovepipe-projectanalysis") + if err := c.Register(projectController); err != nil { + return count, fmt.Errorf("failed to register projectanalysis controller: %w", err) + } + count++ + + recordController := record.NewController(logger, scope, store, materializer, sourceControl, registry, stovepipemq.TopicKeyRecord, "stovepipe-record") if err := c.Register(recordController); err != nil { return count, fmt.Errorf("failed to register record controller: %w", err) } @@ -477,13 +485,13 @@ func registerDLQControllers( } count++ - buildSignalDLQController := dlq.NewDLQBuildSignalController(logger, scope, store, materializer, dlq.TopicKey(stovepipemq.TopicKeyBuildSignal), "stovepipe-buildsignal-dlq") + buildSignalDLQController := dlq.NewDLQBuildSignalController(logger, scope, store, materializer, registry, dlq.TopicKey(stovepipemq.TopicKeyBuildSignal), "stovepipe-buildsignal-dlq") if err := c.Register(buildSignalDLQController); err != nil { return count, fmt.Errorf("failed to register buildsignal dlq controller: %w", err) } count++ - analysisDLQController := buildanalysis.NewDLQController(logger, scope, store, materializer, dlq.TopicKey(stovepipemq.TopicKeyBuildAnalysis), "stovepipe-buildanalysis-dlq") + analysisDLQController := buildanalysis.NewDLQController(logger, scope, store, materializer, registry, dlq.TopicKey(stovepipemq.TopicKeyBuildAnalysis), "stovepipe-buildanalysis-dlq") if err := c.Register(analysisDLQController); err != nil { return count, fmt.Errorf("failed to register buildanalysis dlq controller: %w", err) } @@ -501,6 +509,12 @@ func registerDLQControllers( } count++ + projectDLQController := projectanalysis.NewDLQController(logger, scope, store, materializer, registry, dlq.TopicKey(stovepipemq.TopicKeyProjectAnalysis), "stovepipe-projectanalysis-dlq") + if err := c.Register(projectDLQController); err != nil { + return count, fmt.Errorf("failed to register projectanalysis dlq controller: %w", err) + } + count++ + recordDLQController := record.NewDLQController(logger, scope, store, materializer, registry, dlq.TopicKey(stovepipemq.TopicKeyRecord), "stovepipe-record-dlq") if err := c.Register(recordDLQController); err != nil { return count, fmt.Errorf("failed to register record dlq controller: %w", err) @@ -516,15 +530,7 @@ func registerDLQControllers( return count, nil } -// newTopicRegistry builds the TopicRegistry for Stovepipe's internal pipeline queues. ingest -// publishes to the process topic and the process consumer subscribes to it; process publishes -// to the build topic and the build consumer subscribes to it; build publishes to the buildsignal -// topic and the buildsignal consumer subscribes to it, and also republishes to itself while -// polling. buildsignal publishes to the record topic once a build reaches a terminal status, -// and the record consumer subscribes to it. -// -// The hook topic name is domain-qualified because its key is shared across domains: two -// domains pointed at one queue backend would otherwise consume each other's events. +// Hook topics are domain-qualified because several domains share the logical hook key. func newTopicRegistry(q extqueue.Queue, subscriberName string) (consumer.TopicRegistry, error) { return consumer.NewTopicRegistry([]consumer.TopicConfig{ { @@ -551,6 +557,10 @@ func newTopicRegistry(q extqueue.Queue, subscriberName string) (consumer.TopicRe subscriberName, "stovepipe-buildsignal", ), }, + {Key: stovepipemq.TopicKeyProjectAnalysis, Name: "projectanalysis", Queue: q, + Subscription: extqueue.ExtendedSubscriptionConfig(subscriberName, "stovepipe-projectanalysis")}, + {Key: dlq.TopicKey(stovepipemq.TopicKeyProjectAnalysis), Name: "projectanalysis_dlq", Queue: q, + Subscription: extqueue.DLQSubscriptionConfig(subscriberName, "stovepipe-projectanalysis-dlq")}, { Key: stovepipemq.TopicKeyRecord, Name: "record", diff --git a/stovepipe/controller/build/build_analysis_test.go b/stovepipe/controller/build/build_analysis_test.go index 6b296c2a8..be01646cc 100644 --- a/stovepipe/controller/build/build_analysis_test.go +++ b/stovepipe/controller/build/build_analysis_test.go @@ -143,7 +143,7 @@ func TestBuildAnalysisPipeline(t *testing.T) { ) analysisController := buildanalysis.NewController(zap.NewNop().Sugar(), tally.NoopScope, - staticStorageFactory{store: m.store}, requestlog.NewMaterializer(tally.NoopScope), detectorFactory, stovepipemq.TopicKeyBuildAnalysis, "analysis") + staticStorageFactory{store: m.store}, requestlog.NewMaterializer(tally.NoopScope), detectorFactory, buildController.registry, stovepipemq.TopicKeyBuildAnalysis, "analysis") statusController := buildsignal.NewController(zap.NewNop().Sugar(), tally.NoopScope, staticStorageFactory{store: m.store}, m.materializer, m.runnerFactory, buildController.registry, stovepipemq.TopicKeyBuildSignal, "signal") @@ -170,7 +170,7 @@ func TestBuildAnalysisPipeline(t *testing.T) { assert.True(t, persisted[b]) require.NoError(t, statusController.Process(ctx, delivered(published["buildsignal"][0]))) - require.Len(t, published["record"], 1) + require.Empty(t, published["record"]) require.Len(t, published["buildanalysis"], 2) assert.Equal(t, entity.BuildStatusFailed, savedBuild.Status) assert.Equal(t, entity.RequestStateFailed, request.State) diff --git a/stovepipe/controller/build/build_test.go b/stovepipe/controller/build/build_test.go index 2b2d5779f..401292ab0 100644 --- a/stovepipe/controller/build/build_test.go +++ b/stovepipe/controller/build/build_test.go @@ -101,6 +101,7 @@ func newController(t *testing.T, ctrl *gomock.Controller) (*Controller, buildMoc registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{ {Key: stovepipemq.TopicKeyBuildSignal, Name: "buildsignal", Queue: queue}, {Key: stovepipemq.TopicKeyBuildAnalysis, Name: "buildanalysis", Queue: queue}, + {Key: stovepipemq.TopicKeyProjectAnalysis, Name: "projectanalysis", Queue: queue}, {Key: stovepipemq.TopicKeyIssueTrack, Name: "issuetrack", Queue: queue}, {Key: stovepipemq.TopicKeyRecord, Name: "record", Queue: queue}, }) diff --git a/stovepipe/controller/buildanalysis/BUILD.bazel b/stovepipe/controller/buildanalysis/BUILD.bazel index b70f9b591..b6b006f64 100644 --- a/stovepipe/controller/buildanalysis/BUILD.bazel +++ b/stovepipe/controller/buildanalysis/BUILD.bazel @@ -14,6 +14,7 @@ go_library( "//platform/metrics:go_default_library", "//stovepipe/core/loader:go_default_library", "//stovepipe/core/messagequeue:go_default_library", + "//stovepipe/core/projectanalysis:go_default_library", "//stovepipe/core/requestlog:go_default_library", "//stovepipe/entity:go_default_library", "//stovepipe/extension/buildissue:go_default_library", @@ -37,6 +38,7 @@ go_test( "//platform/consumer:go_default_library", "//platform/consumer/mock:go_default_library", "//platform/errs:go_default_library", + "//platform/extension/messagequeue/mock:go_default_library", "//platform/metrics:go_default_library", "//stovepipe/core/messagequeue:go_default_library", "//stovepipe/core/requestlog:go_default_library", diff --git a/stovepipe/controller/buildanalysis/README.md b/stovepipe/controller/buildanalysis/README.md index b54b192dd..d10f504b0 100644 --- a/stovepipe/controller/buildanalysis/README.md +++ b/stovepipe/controller/buildanalysis/README.md @@ -32,10 +32,20 @@ Detection, persistence, configuration, state-loading, and message-validation err The recovery controller records a `build_analysis_abandoned` event in the Request's history, with the build ID, to indicate that analysis could not complete and the recorded build issues may be incomplete. It does not rerun detection, delete earlier build issues, change Request or Build state, or release a build slot. Original failure details are logged rather than included in the immutable history event. -History identity is stable per build, so repeated recovery deliveries and separate analysis notices for the same build retain one event. Different builds retain different events, even for the same Request. A later successful analysis does not erase the historical occurrence. +History identity is stable per build, so repeated recovery deliveries and separate analysis notices for the same build retain one event. Different builds retain different events, even for the same Request. A delivery already in flight can still finish successfully without erasing the historical occurrence. New deliveries recognize abandonment rather than rerunning detection. State-read and history-write errors are returned for retry. Hosts must run recovery with the DLQ consumer's always-retryable processor and unlimited-retry subscription; only recovery work is retried, not analysis. Unusable payloads, invalid or unresolvable queues, missing Build or Request records, and queue mismatches are logged and acknowledged rather than retried indefinitely. ## Host integration The reference service supplies a no-op detector, registers tracking at admission, and publishes analysis notices at build start and completion. It registers both analysis and issue tracking with bounded normal retries and the existing DLQ consumer for recovery. See [issue tracking](../issuetrack/README.md) for reconciliation and recovery behavior and [build issue detection](../../../doc/rfc/stovepipe/steps/buildanalysis.md) for the publication rules and retry configuration. + +## Project-analysis handoff + +Completed terminal analysis queues project analysis only after its final issues +and completion event are durable. Cancellation is recorded directly by +buildsignal. Abandoned analysis also hands off once the request is terminal, +so project recovery can record the missing assessment and let repository +recording proceed. An abandonment during a running build waits for the terminal +notice rather than publishing nonterminal work. Losing builds never initiate +project analysis for the winning outcome. diff --git a/stovepipe/controller/buildanalysis/buildanalysis.go b/stovepipe/controller/buildanalysis/buildanalysis.go index eaa448ad3..c8205151c 100644 --- a/stovepipe/controller/buildanalysis/buildanalysis.go +++ b/stovepipe/controller/buildanalysis/buildanalysis.go @@ -28,6 +28,7 @@ import ( "github.com/uber/submitqueue/platform/metrics" "github.com/uber/submitqueue/stovepipe/core/loader" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" + "github.com/uber/submitqueue/stovepipe/core/projectanalysis" "github.com/uber/submitqueue/stovepipe/core/requestlog" "github.com/uber/submitqueue/stovepipe/entity" "github.com/uber/submitqueue/stovepipe/extension/buildissue" @@ -46,6 +47,7 @@ type Controller struct { metricsScope tally.Scope stores storage.Factory materializer requestlog.Materializer + registry consumer.TopicRegistry detectorFactory buildissue.DetectorFactory topicKey consumer.TopicKey consumerGroup string @@ -57,6 +59,7 @@ func NewController( stores storage.Factory, materializer requestlog.Materializer, detectorFactory buildissue.DetectorFactory, + registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, ) *Controller { @@ -65,6 +68,7 @@ func NewController( metricsScope: scope.SubScope("buildanalysis_controller"), stores: stores, materializer: materializer, + registry: registry, detectorFactory: detectorFactory, topicKey: topicKey, consumerGroup: consumerGroup, @@ -116,6 +120,15 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r return nil } + buildAnalysisAbandonedLog := requestlog.NewRequestEventLog(request, entity.RequestEventBuildAnalysisAbandoned, build.ID, nil) + _, err = store.GetRequestLogStore().Get(ctx, request.ID, buildAnalysisAbandonedLog.ID) + if err == nil { + return publishProjectAnalysis(ctx, c.registry, request, build.ID) + } + if !errors.Is(err, storage.ErrNotFound) { + return fmt.Errorf("read abandoned build analysis: %w", err) + } + // The Build may finish between the initial Build read and the Request read. // Recheck its status so stopping here does not skip final analysis. if !build.Status.IsTerminal() && request.State.IsTerminal() { @@ -127,9 +140,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r if !build.Status.IsTerminal() { c.logger.Warnw("stopping analysis for a terminal request with nonterminal build", "queue", request.Queue, "build_id", build.ID, "request_id", request.ID, "request_state", string(request.State)) - log := requestlog.NewRequestEventLog(request, entity.RequestEventBuildAnalysisAbandoned, build.ID, - map[string]string{requestlog.MetadataKeyBuildID: build.ID}) - return c.materializer.PersistLog(ctx, store, log) + return c.abandonBuildAnalysis(ctx, store, request, build.ID) } } @@ -172,9 +183,16 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r if err := c.retainCompletedAnalysis(ctx, store, request, build.ID, len(issues)); err != nil { return fmt.Errorf("retain completed analysis for build %q: %w", build.ID, err) } + if err := publishProjectAnalysis(ctx, c.registry, request, build.ID); err != nil { + return err + } c.logger.Infow("terminal issue analysis ready", "queue", request.Queue, "request_id", request.ID, "build_id", build.ID, "issue_count", len(issues)) } + if !complete && request.State.IsTerminal() { + return c.abandonBuildAnalysis(ctx, store, request, build.ID) + } + if hold { delivery.Hold(PollDelayMs) } @@ -227,6 +245,31 @@ func (c *Controller) retainCompletedAnalysis(ctx context.Context, store storage. return c.materializer.PersistLog(ctx, store, log) } +func publishProjectAnalysis(ctx context.Context, registry consumer.TopicRegistry, request entity.Request, buildID string) error { + switch request.State { + case entity.RequestStateSucceeded, entity.RequestStateFailed: + default: + return nil + } + if request.TerminalBuildID != "" { + if request.TerminalBuildID != buildID { + return nil + } + } else if request.BuildID != buildID { + return nil + } + return projectanalysis.Publish(ctx, registry, request) +} + +func (c *Controller) abandonBuildAnalysis(ctx context.Context, store storage.Storage, request entity.Request, buildID string) error { + buildAnalysisAbandonedLog := requestlog.NewRequestEventLog(request, entity.RequestEventBuildAnalysisAbandoned, buildID, + map[string]string{requestlog.MetadataKeyBuildID: buildID}) + if err := c.materializer.PersistLog(ctx, store, buildAnalysisAbandonedLog); err != nil { + return err + } + return publishProjectAnalysis(ctx, c.registry, request, buildID) +} + func (c *Controller) Name() string { return "buildanalysis" } func (c *Controller) TopicKey() consumer.TopicKey { return c.topicKey } diff --git a/stovepipe/controller/buildanalysis/buildanalysis_test.go b/stovepipe/controller/buildanalysis/buildanalysis_test.go index e542026fd..9dedc1ed9 100644 --- a/stovepipe/controller/buildanalysis/buildanalysis_test.go +++ b/stovepipe/controller/buildanalysis/buildanalysis_test.go @@ -23,8 +23,10 @@ import ( "github.com/stretchr/testify/require" "github.com/uber-go/tally" entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" + "github.com/uber/submitqueue/platform/consumer" consumermock "github.com/uber/submitqueue/platform/consumer/mock" "github.com/uber/submitqueue/platform/errs" + queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" "github.com/uber/submitqueue/platform/metrics" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" "github.com/uber/submitqueue/stovepipe/core/requestlog" @@ -60,6 +62,10 @@ type analysisMocks struct { detectorFactory *buildissuemock.MockDetectorFactory detector *buildissuemock.MockDetector scope tally.TestScope + registry consumer.TopicRegistry + published *[]entityqueue.Message + publishFailure *error + onPublish *func(entityqueue.Message) } func newTestController(t *testing.T) (*Controller, analysisMocks) { @@ -69,7 +75,7 @@ func newTestController(t *testing.T) (*Controller, analysisMocks) { m := analysisMocks{ ctrl: ctrl, requests: storagemock.NewMockRequestStore(ctrl), - request: &entity.Request{ID: "request/1", Queue: testQueue, State: entity.RequestStateProcessing}, + request: &entity.Request{ID: "1", Queue: testQueue, BuildID: testBuildID, State: entity.RequestStateProcessing}, stores: storagemock.NewMockFactory(ctrl), store: storagemock.NewMockStorage(ctrl), builds: storagemock.NewMockBuildStore(ctrl), @@ -79,6 +85,22 @@ func newTestController(t *testing.T) (*Controller, analysisMocks) { scope: tally.NewTestScope("test", nil), } + publisher := queuemock.NewMockPublisher(ctrl) + m.published = new([]entityqueue.Message) + m.publishFailure = new(error) + m.onPublish = new(func(entityqueue.Message)) + publisher.EXPECT().Publish(gomock.Any(), "projectanalysis", gomock.Any()).DoAndReturn(func(_ context.Context, _ string, msg entityqueue.Message) error { + if *m.onPublish != nil { + (*m.onPublish)(msg) + } + *m.published = append(*m.published, msg) + return *m.publishFailure + }).AnyTimes() + queue := queuemock.NewMockQueue(ctrl) + queue.EXPECT().Publisher().Return(publisher).AnyTimes() + var err error + m.registry, err = consumer.NewTopicRegistry([]consumer.TopicConfig{{Key: stovepipemq.TopicKeyProjectAnalysis, Name: "projectanalysis", Queue: queue}}) + require.NoError(t, err) m.store.EXPECT().GetBuildStore().Return(m.builds).AnyTimes() m.store.EXPECT().GetRequestStore().Return(m.requests).AnyTimes() m.requestRead = m.requests.EXPECT().Get(gomock.Any(), gomock.Any()).DoAndReturn(func(context.Context, string) (entity.Request, error) { return *m.request, nil }).AnyTimes() @@ -100,14 +122,17 @@ func newTestController(t *testing.T) (*Controller, analysisMocks) { m.history[log.ID] = log return nil }).AnyTimes() - m.historyRead = history.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, requestID, logID string) (entity.RequestLog, error) { + readHistory := func(_ context.Context, requestID, logID string) (entity.RequestLog, error) { log, exists := m.history[logID] if !exists || log.RequestID != requestID { return entity.RequestLog{}, storage.ErrNotFound } return log, nil - }).AnyTimes() - c := NewController(zap.NewNop().Sugar(), m.scope, m.stores, m.materializer, m.detectorFactory, + } + history.EXPECT().Get(gomock.Any(), gomock.Any(), "event/build_analysis_abandoned/"+testBuildID).DoAndReturn(readHistory).AnyTimes() + m.historyRead = history.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(readHistory).AnyTimes() + + c := NewController(zap.NewNop().Sugar(), m.scope, m.stores, m.materializer, m.detectorFactory, m.registry, stovepipemq.TopicKeyBuildAnalysis, "stovepipe-buildanalysis") return c, m @@ -135,6 +160,14 @@ func (m analysisMocks) delivery(t *testing.T) *consumermock.MockDelivery { func (m analysisMocks) expectPass(status entity.BuildStatus, issues []entity.BuildIssue, err error) *gomock.Call { if status.IsTerminal() { m.request.TerminalBuildID = testBuildID + switch status { + case entity.BuildStatusSucceeded: + m.request.State = entity.RequestStateSucceeded + case entity.BuildStatusCancelled: + m.request.State = entity.RequestStateCancelled + default: + m.request.State = entity.RequestStateFailed + } } resolve := m.stores.EXPECT().For(storage.Config{QueueName: testQueue}).Return(m.store, nil) load := m.builds.EXPECT().Get(gomock.Any(), testBuildID). @@ -165,7 +198,6 @@ func TestProcessPersistsBeforeHoldingOrCompleting(t *testing.T) { d := m.delivery(t) if status.IsTerminal() { - m.historyRead.After(last) m.historyCreate.After(last) } else { d.EXPECT().Hold(PollDelayMs).After(last) @@ -518,6 +550,7 @@ func TestProcessWaitsForWinningBuildBeforeFinalizing(t *testing.T) { c, m := newTestController(t) detected := m.expectPass(entity.BuildStatusFailed, nil, nil) m.request.TerminalBuildID = "" + m.request.State = entity.RequestStateProcessing if outcome == "abandoned" { m.request.State = entity.RequestStateFailed } @@ -541,7 +574,7 @@ func TestProcessWaitsForWinningBuildBeforeFinalizing(t *testing.T) { require.NoError(t, c.Process(context.Background(), d)) - if outcome == "recorded during detection" { + if outcome == "recorded during detection" || outcome == "abandoned" { require.Len(t, m.history, 1) } else { require.Empty(t, m.history) @@ -584,3 +617,59 @@ func TestProcessRejectsQueueMismatchOnOutcomeReload(t *testing.T) { require.Error(t, c.Process(context.Background(), m.delivery(t))) } + +func TestProjectAnalysisHandoffRequiresDurableCompletion(t *testing.T) { + for _, status := range []entity.BuildStatus{entity.BuildStatusSucceeded, entity.BuildStatusFailed, entity.BuildStatusCancelled} { + t.Run(string(status), func(t *testing.T) { + c, m := newTestController(t) + issue := entity.BuildIssue{TaskID: "task", UnitID: "unit"} + m.expectPass(status, []entity.BuildIssue{issue}, nil) + saved := false + m.issues.EXPECT().Create(gomock.Any(), testBuildID, issue).DoAndReturn(func(context.Context, string, entity.BuildIssue) error { saved = true; return nil }) + *m.onPublish = func(msg entityqueue.Message) { + require.True(t, saved) + log := requestlog.NewRequestEventLog(*m.request, entity.RequestEventBuildAnalysisCompleted, testBuildID, nil) + require.Contains(t, m.history, log.ID) + payload := &stovepipemq.ProjectAnalysis{} + require.NoError(t, stovepipemq.Unmarshal(msg.Payload, payload)) + require.Equal(t, m.request.ID, payload.GetId()) + require.Equal(t, m.request.ID, msg.PartitionKey) + require.Equal(t, testQueue, msg.Tenant) + } + require.NoError(t, c.Process(context.Background(), m.delivery(t))) + if status == entity.BuildStatusCancelled { + require.Empty(t, *m.published) + } else { + require.Len(t, *m.published, 1) + } + }) + } +} + +func TestProjectAnalysisPublicationRetries(t *testing.T) { + c, m := newTestController(t) + *m.publishFailure = assert.AnError + m.expectPass(entity.BuildStatusFailed, nil, nil) + require.ErrorIs(t, c.Process(context.Background(), m.delivery(t)), assert.AnError) + require.Len(t, m.history, 1) + *m.publishFailure = nil + m.expectPass(entity.BuildStatusFailed, nil, nil) + require.NoError(t, c.Process(context.Background(), m.delivery(t))) + require.Len(t, *m.published, 2) + require.Equal(t, (*m.published)[0].ID, (*m.published)[1].ID) + require.Equal(t, (*m.published)[0].Payload, (*m.published)[1].Payload) +} + +func TestAbandonedRunningAnalysisHandsOffAfterOutcome(t *testing.T) { + c, m := newTestController(t) + log := requestlog.NewRequestEventLog(*m.request, entity.RequestEventBuildAnalysisAbandoned, testBuildID, nil) + m.history[log.ID] = log + m.stores.EXPECT().For(storage.Config{QueueName: testQueue}).Return(m.store, nil).Times(2) + m.builds.EXPECT().Get(gomock.Any(), testBuildID).Return(entity.Build{ID: testBuildID, RequestID: m.request.ID, Status: entity.BuildStatusRunning}, nil).Times(2) + require.NoError(t, c.Process(context.Background(), m.delivery(t))) + require.Empty(t, *m.published) + m.request.State = entity.RequestStateFailed + m.request.TerminalBuildID = testBuildID + require.NoError(t, c.Process(context.Background(), m.delivery(t))) + require.Len(t, *m.published, 1) +} diff --git a/stovepipe/controller/buildanalysis/dlq.go b/stovepipe/controller/buildanalysis/dlq.go index 36dad5636..cd9849e28 100644 --- a/stovepipe/controller/buildanalysis/dlq.go +++ b/stovepipe/controller/buildanalysis/dlq.go @@ -39,6 +39,7 @@ type DLQController struct { metricsScope tally.Scope stores storage.Factory materializer requestlog.Materializer + registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string } @@ -49,6 +50,7 @@ func NewDLQController( scope tally.Scope, stores storage.Factory, materializer requestlog.Materializer, + registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, ) *DLQController { @@ -58,6 +60,7 @@ func NewDLQController( metricsScope: scope.SubScope(name), stores: stores, materializer: materializer, + registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, } @@ -173,7 +176,7 @@ func (c *DLQController) Process(ctx context.Context, delivery consumer.Delivery) ) } c.logger.Errorw("build analysis could not complete; issue observations may be incomplete", fields...) - return nil + return publishProjectAnalysis(ctx, c.registry, request, build.ID) } func (c *DLQController) Name() string { return string(c.topicKey) } diff --git a/stovepipe/controller/buildanalysis/dlq_test.go b/stovepipe/controller/buildanalysis/dlq_test.go index 6280d71b4..066b5a3fa 100644 --- a/stovepipe/controller/buildanalysis/dlq_test.go +++ b/stovepipe/controller/buildanalysis/dlq_test.go @@ -26,6 +26,7 @@ import ( entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" consumermock "github.com/uber/submitqueue/platform/consumer/mock" + queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" "github.com/uber/submitqueue/stovepipe/core/requestlog" requestlogmock "github.com/uber/submitqueue/stovepipe/core/requestlog/mock" @@ -37,12 +38,16 @@ import ( ) type dlqMocks struct { - ctrl *gomock.Controller - factory *storagemock.MockFactory - store *storagemock.MockStorage - builds *storagemock.MockBuildStore - requests *storagemock.MockRequestStore - materializer *requestlogmock.MockMaterializer + ctrl *gomock.Controller + factory *storagemock.MockFactory + store *storagemock.MockStorage + builds *storagemock.MockBuildStore + requests *storagemock.MockRequestStore + materializer *requestlogmock.MockMaterializer + registry consumer.TopicRegistry + published *[]entityqueue.Message + publishFailure *error + onPublish *func(entityqueue.Message) } func newDLQTestController(t *testing.T) (*DLQController, dlqMocks) { @@ -60,7 +65,23 @@ func newDLQTestController(t *testing.T) (*DLQController, dlqMocks) { m.store.EXPECT().GetBuildStore().Return(m.builds).AnyTimes() m.store.EXPECT().GetRequestStore().Return(m.requests).AnyTimes() - c := NewDLQController(zap.NewNop().Sugar(), tally.NoopScope, m.factory, m.materializer, + publisher := queuemock.NewMockPublisher(ctrl) + m.published = new([]entityqueue.Message) + m.publishFailure = new(error) + m.onPublish = new(func(entityqueue.Message)) + publisher.EXPECT().Publish(gomock.Any(), "projectanalysis", gomock.Any()).DoAndReturn(func(_ context.Context, _ string, msg entityqueue.Message) error { + if *m.onPublish != nil { + (*m.onPublish)(msg) + } + *m.published = append(*m.published, msg) + return *m.publishFailure + }).AnyTimes() + queue := queuemock.NewMockQueue(ctrl) + queue.EXPECT().Publisher().Return(publisher).AnyTimes() + var err error + m.registry, err = consumer.NewTopicRegistry([]consumer.TopicConfig{{Key: stovepipemq.TopicKeyProjectAnalysis, Name: "projectanalysis", Queue: queue}}) + require.NoError(t, err) + c := NewDLQController(zap.NewNop().Sugar(), tally.NoopScope, m.factory, m.materializer, m.registry, consumer.TopicKey("buildanalysis_dlq"), "stovepipe-buildanalysis-dlq") return c, m @@ -89,7 +110,7 @@ func TestDLQRetainsHistoryWithoutChangingValidation(t *testing.T) { } { t.Run(string(state), func(t *testing.T) { c, m := newDLQTestController(t) - request := entity.Request{ID: "request/1", Queue: testQueue, State: state, Version: 3} + request := entity.Request{ID: "1", Queue: testQueue, State: state, Version: 3} m.expectLoad(request) m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).DoAndReturn( func(_ context.Context, _ storage.Storage, log entity.RequestLog) error { @@ -120,13 +141,13 @@ func TestDLQReturnsStateAndHistoryErrorsForRetry(t *testing.T) { m.builds.EXPECT().Get(gomock.Any(), testBuildID).Return(entity.Build{}, assert.AnError) } else { m.builds.EXPECT().Get(gomock.Any(), testBuildID). - Return(entity.Build{ID: testBuildID, RequestID: "request/1", Status: entity.BuildStatusFailed}, nil) + Return(entity.Build{ID: testBuildID, RequestID: "1", Status: entity.BuildStatusFailed}, nil) if step == "load request" { - m.requests.EXPECT().Get(gomock.Any(), "request/1").Return(entity.Request{}, assert.AnError) + m.requests.EXPECT().Get(gomock.Any(), "1").Return(entity.Request{}, assert.AnError) } else { - m.requests.EXPECT().Get(gomock.Any(), "request/1"). - Return(entity.Request{ID: "request/1", Queue: testQueue}, nil) + m.requests.EXPECT().Get(gomock.Any(), "1"). + Return(entity.Request{ID: "1", Queue: testQueue}, nil) m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).Return(assert.AnError) } } @@ -170,13 +191,13 @@ func TestDLQAcknowledgesUnusableMessages(t *testing.T) { m.builds.EXPECT().Get(gomock.Any(), testBuildID).Return(entity.Build{ID: testBuildID}, nil) case "missing request": m.builds.EXPECT().Get(gomock.Any(), testBuildID). - Return(entity.Build{ID: testBuildID, RequestID: "request/1"}, nil) - m.requests.EXPECT().Get(gomock.Any(), "request/1").Return(entity.Request{}, storage.ErrNotFound) + Return(entity.Build{ID: testBuildID, RequestID: "1"}, nil) + m.requests.EXPECT().Get(gomock.Any(), "1").Return(entity.Request{}, storage.ErrNotFound) case "request queue mismatch": m.builds.EXPECT().Get(gomock.Any(), testBuildID). - Return(entity.Build{ID: testBuildID, RequestID: "request/1"}, nil) - m.requests.EXPECT().Get(gomock.Any(), "request/1"). - Return(entity.Request{ID: "request/1", Queue: "other/main"}, nil) + Return(entity.Build{ID: testBuildID, RequestID: "1"}, nil) + m.requests.EXPECT().Get(gomock.Any(), "1"). + Return(entity.Request{ID: "1", Queue: "other/main"}, nil) } } @@ -209,14 +230,14 @@ func TestDLQDeduplicatesHistoryAcrossRetriesAndNotices(t *testing.T) { return nil }, ).Times(4) - logStore.EXPECT().Get(gomock.Any(), "request/1", gomock.Any()).DoAndReturn( + logStore.EXPECT().Get(gomock.Any(), "1", gomock.Any()).DoAndReturn( func(_ context.Context, _ string, id string) (entity.RequestLog, error) { return retained[id], nil }, ).Times(2) for i, buildID := range []string{testBuildID, testBuildID, testBuildID, "build/43"} { - request := entity.Request{ID: "request/1", Queue: testQueue, State: entity.RequestStateProcessing, Version: int32(i + 1)} + request := entity.Request{ID: "1", Queue: testQueue, State: entity.RequestStateProcessing, Version: int32(i + 1)} if i > 0 { request.State = entity.RequestStateSucceeded } @@ -246,3 +267,57 @@ func TestDLQDeduplicatesHistoryAcrossRetriesAndNotices(t *testing.T) { assert.Equal(t, buildID, log.Metadata[requestlog.MetadataKeyBuildID]) } } + +func TestDLQHandsTerminalAnalysisToProjectRecovery(t *testing.T) { + for _, outcome := range []string{"running", "winner", "loser", "forced failure", "cancelled"} { + t.Run(outcome, func(t *testing.T) { + c, m := newDLQTestController(t) + request := entity.Request{ID: "1", Queue: testQueue, State: entity.RequestStateFailed, BuildID: testBuildID, TerminalBuildID: testBuildID} + switch outcome { + case "running": + request.State, request.TerminalBuildID = entity.RequestStateProcessing, "" + case "loser": + request.TerminalBuildID = "other-build" + case "forced failure": + request.TerminalBuildID = "" + case "cancelled": + request.State = entity.RequestStateCancelled + } + m.expectLoad(request) + retained := false + m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).DoAndReturn(func(_ context.Context, _ storage.Storage, log entity.RequestLog) error { + require.Equal(t, entity.RequestEventBuildAnalysisAbandoned, log.Event) + retained = true + return nil + }) + *m.onPublish = func(entityqueue.Message) { require.True(t, retained) } + require.NoError(t, c.Process(context.Background(), m.delivery(analysisMessage(t), true))) + if outcome == "winner" || outcome == "forced failure" { + require.Len(t, *m.published, 1) + } else { + require.Empty(t, *m.published) + } + }) + } +} + +func TestDLQRetriesProjectHandoff(t *testing.T) { + c, m := newDLQTestController(t) + request := entity.Request{ID: "1", Queue: testQueue, State: entity.RequestStateFailed, TerminalBuildID: testBuildID} + for _, fail := range []bool{true, false} { + m.expectLoad(request) + m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).Return(nil) + *m.publishFailure = nil + if fail { + *m.publishFailure = assert.AnError + } + err := c.Process(context.Background(), m.delivery(analysisMessage(t), true)) + if fail { + require.ErrorIs(t, err, assert.AnError) + } else { + require.NoError(t, err) + } + } + require.Len(t, *m.published, 2) + require.Equal(t, (*m.published)[0].ID, (*m.published)[1].ID) +} diff --git a/stovepipe/controller/buildanalysis/history_test.go b/stovepipe/controller/buildanalysis/history_test.go index 0d8943d9c..1ddcdb1af 100644 --- a/stovepipe/controller/buildanalysis/history_test.go +++ b/stovepipe/controller/buildanalysis/history_test.go @@ -72,6 +72,7 @@ func TestCompletedAnalysisHistoryRepairsOnRetry(t *testing.T) { historyFailure := pass == 0 && tt.failHistory if waiting { m.request.TerminalBuildID = "" + m.request.State = entity.RequestStateProcessing } if historyFailure { *m.historyFailure = assert.AnError diff --git a/stovepipe/controller/buildsignal/buildsignal.go b/stovepipe/controller/buildsignal/buildsignal.go index 305ddaf0e..ee6ae6fc8 100644 --- a/stovepipe/controller/buildsignal/buildsignal.go +++ b/stovepipe/controller/buildsignal/buildsignal.go @@ -16,8 +16,8 @@ // BuildSignal messages (a build id), polls the build-runner until the build // reaches a terminal status, persists that status, and — once terminal — // releases the queue's build slot, projects the outcome onto the request, -// advances the successful-build bookmark, and publishes directly to record plus -// terminal analysis. See +// advances the successful-build bookmark, and queues terminal analysis. +// Cancelled requests bypass project analysis and publish directly to record. See // doc/rfc/stovepipe/steps/buildsignal.md. package buildsignal @@ -58,7 +58,7 @@ var ( // Controller consumes BuildSignal messages, polls the build-runner toward a // terminal status, persists the result, and either holds the delivery until // the next poll or releases the queue's build slot, projects the outcome onto -// the request, and publishes the request id to record. Implements +// the request, and queues terminal analysis. Implements // consumer.Controller. type Controller struct { logger *zap.SugaredLogger @@ -100,8 +100,8 @@ func NewController( // Process reloads the build referenced by the delivery, polls its runner for // the latest status, persists a real transition, and either holds the delivery // until the next poll or, once terminal, releases the queue's build slot, -// projects the outcome onto the request, and publishes the request id to -// record. Returns nil to ack (success) or an error to nack (retry) / reject +// projects the outcome onto the request, and queues terminal analysis. +// Returns nil to ack (success) or an error to nack (retry) / reject // (DLQ). func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) error { msg := delivery.Message() @@ -143,9 +143,9 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er // A request only reaches the poll loop after process admitted it, so it is // `processing` — or it already carries this build's outcome, on a redelivery - // after the outcome was stamped but before record was published. Both cases - // proceed: the redelivery re-publishes to record rather than dropping the - // signal, and every write below is a no-op the second time. + // after the outcome was stamped but before the downstream handoff. Both cases + // proceed: the redelivery retries the handoff rather than dropping the signal, + // and every write below is a no-op the second time. // // Anything else is terminal without a build outcome, which today means only // `superseded` — and process supersedes solely from `accepted` (see @@ -188,9 +188,9 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er if err := c.persistOutcomeLog(ctx, store, request); err != nil { return err } - recordErr := c.publishRecord(ctx, request.ID, request.Queue) - if recordErr != nil { - recordErr = fmt.Errorf("failed to publish record for request %s: %w", request.ID, recordErr) + var recordErr error + if request.State == entity.RequestStateCancelled { + recordErr = c.publishRecord(ctx, request.ID, request.Queue) } analysisErr := c.publishBuildAnalysis(ctx, build.ID, request.Queue) @@ -263,23 +263,11 @@ func (c *Controller) finishRequest(ctx context.Context, store storage.Storage, r } func (c *Controller) persistOutcomeLog(ctx context.Context, store storage.Storage, request entity.Request) error { - var reason entity.RequestOutcomeReason - // The durable request is authoritative when duplicate builds race to record different outcomes. - switch request.State { - case entity.RequestStateSucceeded: - reason = entity.RequestOutcomeReasonBuildSucceeded - case entity.RequestStateFailed: - reason = entity.RequestOutcomeReasonBuildFailed - case entity.RequestStateCancelled: - reason = entity.RequestOutcomeReasonBuildCancelled - default: - return fmt.Errorf("request %s has no build outcome to record", request.ID) + log, err := requestlog.NewBuildOutcomeLog(request) + if err != nil { + return err } - log := requestlog.NewRequestStateLog(request, reason) - if request.TerminalBuildID != "" { - log.Metadata[requestlog.MetadataKeyBuildID] = request.TerminalBuildID - } if err := c.materializer.PersistLog(ctx, store, log); err != nil { return fmt.Errorf("failed to record %s state for request %s: %w", request.State, request.ID, err) } diff --git a/stovepipe/controller/buildsignal/buildsignal_test.go b/stovepipe/controller/buildsignal/buildsignal_test.go index 5731831d7..6676889fb 100644 --- a/stovepipe/controller/buildsignal/buildsignal_test.go +++ b/stovepipe/controller/buildsignal/buildsignal_test.go @@ -355,7 +355,6 @@ func TestProcess(t *testing.T) { m.runner.EXPECT().Status(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(entity.BuildStatusSucceeded, nil, nil) m.buildStore.EXPECT().Update(gomock.Any(), build(entity.BuildStatusSucceeded, 2), int32(2), int32(3)).Return(nil) logCall := expectFinish(m, entity.RequestStateSucceeded) - m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil).After(logCall) m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, @@ -368,7 +367,6 @@ func TestProcess(t *testing.T) { m.runner.EXPECT().Status(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(entity.BuildStatusFailed, nil, nil) m.buildStore.EXPECT().Update(gomock.Any(), build(entity.BuildStatusFailed, 2), int32(2), int32(3)).Return(nil) logCall := expectFinish(m, entity.RequestStateFailed) - m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil).After(logCall) m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, @@ -402,7 +400,6 @@ func TestProcess(t *testing.T) { queue.LastGreenURI = request.URI getCall := m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(queue, nil).After(eventCall) logCall := expectOutcomeLog(m, entity.RequestStateSucceeded, 2).After(getCall) - m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil).After(logCall) m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, @@ -458,7 +455,6 @@ func TestProcess(t *testing.T) { // No build Update: a stored terminal status is write-once. The stored // status, not the poll, decides the request's outcome. logCall := expectFinish(m, entity.RequestStateSucceeded) - m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil).After(logCall) m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) }, }, @@ -538,7 +534,7 @@ func TestProcess(t *testing.T) { }, }, { - name: "publish to record failure is not retryable", + name: "terminal analysis publication failure is not retryable", wantErr: true, wantRetry: false, setup: func(m buildsignalMocks) { @@ -548,8 +544,7 @@ func TestProcess(t *testing.T) { m.runner.EXPECT().Status(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(entity.BuildStatusSucceeded, nil, nil) m.buildStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(2), int32(3)).Return(nil) logCall := expectFinish(m, entity.RequestStateSucceeded) - m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(errors.New("queue down")).After(logCall) - m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil).After(logCall) + m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(errors.New("queue down")).After(logCall) }, }, { @@ -657,13 +652,13 @@ func TestTerminalAnalysisPublicationDoesNotGateRecordPublication(t *testing.T) { t.Run(tt.name, func(t *testing.T) { ctrl := gomock.NewController(t) c, m := newController(t, ctrl) - m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(entity.BuildStatusFailed, 2), nil) - request := requestWithState(entity.RequestStateFailed) + m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(entity.BuildStatusCancelled, 2), nil) + request := requestWithState(entity.RequestStateCancelled) m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(request, nil) m.runnerFactory.EXPECT().For(buildrunner.Config{QueueName: testQueue}).Return(m.runner, nil) - m.runner.EXPECT().Status(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(entity.BuildStatusFailed, nil, nil) + m.runner.EXPECT().Status(gomock.Any(), entity.BuildID{ID: testBuildID}).Return(entity.BuildStatusCancelled, nil, nil) m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).Return(nil).Times(2) @@ -751,7 +746,6 @@ func TestProcessRetriesBookmarkWithoutReleasingSlotAgain(t *testing.T) { updated.LastGreenURI = request.URI m.queueStore.EXPECT().Update(gomock.Any(), updated, int32(5), int32(6)).Return(nil) } - m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil) m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil) require.NoError(t, c.Process(queueContext(), delivery(t, ctrl, buildSignalPayload(t, testBuildID)))) }) @@ -776,7 +770,6 @@ func TestProcessBookmarkPreservesConcurrentQueueUpdates(t *testing.T) { m.queueStore.EXPECT().Update(gomock.Any(), gomock.Any(), int32(5), int32(6)).Return(storage.ErrVersionMismatch), m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(fresh, nil), ) - m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil) m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil) require.NoError(t, c.Process(queueContext(), delivery(t, ctrl, buildSignalPayload(t, testBuildID)))) } @@ -804,7 +797,6 @@ func TestProcessBookmarkUsesWinningRequestOutcome(t *testing.T) { if state == entity.RequestStateSucceeded { expectAdvanceLastGreen(m, nil) } - m.publisher.EXPECT().Publish(gomock.Any(), "record", gomock.Any()).Return(nil) m.publisher.EXPECT().Publish(gomock.Any(), "buildanalysis", gomock.Any()).Return(nil) require.NoError(t, c.Process(queueContext(), delivery(t, ctrl, buildSignalPayload(t, testBuildID)))) }) diff --git a/stovepipe/controller/dlq/BUILD.bazel b/stovepipe/controller/dlq/BUILD.bazel index da7c418f1..9a15ff40a 100644 --- a/stovepipe/controller/dlq/BUILD.bazel +++ b/stovepipe/controller/dlq/BUILD.bazel @@ -14,7 +14,9 @@ go_library( "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", "//platform/metrics:go_default_library", + "//platform/publish:go_default_library", "//stovepipe/core/messagequeue:go_default_library", + "//stovepipe/core/projectanalysis:go_default_library", "//stovepipe/core/requestlog:go_default_library", "//stovepipe/entity:go_default_library", "//stovepipe/extension/storage:go_default_library", @@ -35,6 +37,7 @@ go_test( "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", "//platform/consumer/mock:go_default_library", + "//platform/extension/messagequeue/mock:go_default_library", "//platform/metrics:go_default_library", "//stovepipe/core/messagequeue:go_default_library", "//stovepipe/core/requestlog:go_default_library", diff --git a/stovepipe/controller/dlq/build_test.go b/stovepipe/controller/dlq/build_test.go index c97686202..d18d166ba 100644 --- a/stovepipe/controller/dlq/build_test.go +++ b/stovepipe/controller/dlq/build_test.go @@ -24,6 +24,7 @@ import ( stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" requestlogmock "github.com/uber/submitqueue/stovepipe/core/requestlog/mock" "github.com/uber/submitqueue/stovepipe/entity" + "github.com/uber/submitqueue/stovepipe/extension/storage" storagemock "github.com/uber/submitqueue/stovepipe/extension/storage/mock" "go.uber.org/mock/gomock" "go.uber.org/zap" @@ -43,6 +44,10 @@ func newBuildController(t *testing.T, ctrl *gomock.Controller) (consumer.Control m.store.EXPECT().GetRequestStore().Return(m.reqStore).AnyTimes() m.store.EXPECT().GetQueueStore().Return(m.queueStore).AnyTimes() + history := storagemock.NewMockRequestLogStore(ctrl) + m.store.EXPECT().GetRequestLogStore().Return(history).AnyTimes() + history.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(entity.RequestLog{}, storage.ErrNotFound).AnyTimes() + c := NewDLQBuildController(zap.NewNop().Sugar(), scope, staticStorageFactory{store: m.store}, m.materializer, TopicKey(stovepipemq.TopicKeyBuild), "stovepipe-build-dlq") return c, m } diff --git a/stovepipe/controller/dlq/buildsignal.go b/stovepipe/controller/dlq/buildsignal.go index 69b2b9399..a06f9f5e6 100644 --- a/stovepipe/controller/dlq/buildsignal.go +++ b/stovepipe/controller/dlq/buildsignal.go @@ -23,7 +23,9 @@ import ( entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" "github.com/uber/submitqueue/platform/consumer" "github.com/uber/submitqueue/platform/metrics" + "github.com/uber/submitqueue/platform/publish" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" + "github.com/uber/submitqueue/stovepipe/core/projectanalysis" "github.com/uber/submitqueue/stovepipe/core/requestlog" "github.com/uber/submitqueue/stovepipe/entity" "github.com/uber/submitqueue/stovepipe/extension/storage" @@ -57,13 +59,11 @@ type buildSignalController struct { metricsScope tally.Scope stores storage.Factory materializer requestlog.Materializer + registry consumer.TopicRegistry topicKey consumer.TopicKey consumerGroup string } -// Verify buildSignalController implements consumer.Controller at compile time. -var _ consumer.Controller = (*buildSignalController)(nil) - // NewDLQBuildSignalController creates a DLQ controller for the buildsignal stage's // dead-letter topic. topicKey is typically // dlq.TopicKey(stovepipemq.TopicKeyBuildSignal). @@ -72,6 +72,7 @@ func NewDLQBuildSignalController( scope tally.Scope, stores storage.Factory, materializer requestlog.Materializer, + registry consumer.TopicRegistry, topicKey consumer.TopicKey, consumerGroup string, ) consumer.Controller { @@ -81,6 +82,7 @@ func NewDLQBuildSignalController( metricsScope: scope.SubScope(name), stores: stores, materializer: materializer, + registry: registry, topicKey: topicKey, consumerGroup: consumerGroup, } @@ -158,8 +160,57 @@ func (c *buildSignalController) Process(ctx context.Context, delivery consumer.D return err } + request, err := store.GetRequestStore().Get(ctx, build.RequestID) + if errors.Is(err, storage.ErrNotFound) { + return nil + } + if err != nil { + return fmt.Errorf("reload reconciled build request: %w", err) + } + if request.Queue != sig.GetQueueName() { + return fmt.Errorf("reconciled build request belongs to another queue") + } metrics.NamedCounter(c.metricsScope, _buildSignalOpName, "reconciled", 1, metrics.TagsFromContext(ctx)...) - return nil + if request.TerminalBuildID != "" { + buildOutcomeLog, err := requestlog.NewBuildOutcomeLog(request) + if err != nil { + return err + } + if err := c.materializer.PersistLog(ctx, store, buildOutcomeLog); err != nil { + return err + } + } + switch request.State { + case entity.RequestStateSucceeded, entity.RequestStateFailed: + if request.TerminalBuildID == "" { + return projectanalysis.Publish(ctx, c.registry, request) + } + return c.publishTerminalBuildAnalysis(ctx, request, build.ID) + case entity.RequestStateCancelled: + payload, err := stovepipemq.Marshal(&stovepipemq.Record{Id: request.ID, QueueName: request.Queue}) + if err != nil { + return err + } + recordErr := publish.Message(ctx, c.registry, stovepipemq.TopicKeyRecord, publish.MessageParams{ + Tenant: request.Queue, ID: publish.IntentID(request.ID), Payload: payload, PartitionKey: request.ID, + }) + return errors.Join(recordErr, c.publishTerminalBuildAnalysis(ctx, request, build.ID)) + default: + return nil + } +} + +func (c *buildSignalController) publishTerminalBuildAnalysis(ctx context.Context, request entity.Request, buildID string) error { + if request.TerminalBuildID != buildID { + return nil + } + payload, err := stovepipemq.Marshal(&stovepipemq.BuildAnalysis{Id: buildID, QueueName: request.Queue}) + if err != nil { + return err + } + return publish.Message(ctx, c.registry, stovepipemq.TopicKeyBuildAnalysis, publish.MessageParams{ + Tenant: request.Queue, ID: publish.IntentID(buildID, "finished"), Payload: payload, PartitionKey: buildID, + }) } // Name returns the controller name for logging and metrics. @@ -176,3 +227,5 @@ func (c *buildSignalController) TopicKey() consumer.TopicKey { func (c *buildSignalController) ConsumerGroup() string { return c.consumerGroup } + +var _ consumer.Controller = (*buildSignalController)(nil) diff --git a/stovepipe/controller/dlq/buildsignal_test.go b/stovepipe/controller/dlq/buildsignal_test.go index 08c0f4aab..38b4f778e 100644 --- a/stovepipe/controller/dlq/buildsignal_test.go +++ b/stovepipe/controller/dlq/buildsignal_test.go @@ -15,8 +15,12 @@ package dlq import ( + "context" "testing" + entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" + queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/uber-go/tally" @@ -34,12 +38,17 @@ import ( const testBuildID = "go-code-on-odin-submitqueue/builds/2867068" type buildSignalDLQMocks struct { - reqStore *storagemock.MockRequestStore - queueStore *storagemock.MockQueueStore - buildStore *storagemock.MockBuildStore - store *storagemock.MockStorage - materializer *requestlogmock.MockMaterializer - metricsScope tally.TestScope + reqStore *storagemock.MockRequestStore + queueStore *storagemock.MockQueueStore + buildStore *storagemock.MockBuildStore + store *storagemock.MockStorage + materializer *requestlogmock.MockMaterializer + metricsScope tally.TestScope + publisher *queuemock.MockPublisher + published map[string][]entityqueue.Message + publishFailure *error + history *storagemock.MockRequestLogStore + retainedLogs map[string]entity.RequestLog } func newBuildSignalController(t *testing.T, ctrl *gomock.Controller) (consumer.Controller, buildSignalDLQMocks) { @@ -59,11 +68,36 @@ func newBuildSignalController(t *testing.T, ctrl *gomock.Controller) (consumer.C m.store.EXPECT().GetQueueStore().Return(m.queueStore).AnyTimes() m.store.EXPECT().GetBuildStore().Return(m.buildStore).AnyTimes() + m.publisher = queuemock.NewMockPublisher(ctrl) + m.published = map[string][]entityqueue.Message{} + m.publishFailure = new(error) + m.publisher.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, topic string, msg entityqueue.Message) error { + m.published[topic] = append(m.published[topic], msg) + return *m.publishFailure + }).AnyTimes() + queue := queuemock.NewMockQueue(ctrl) + queue.EXPECT().Publisher().Return(m.publisher).AnyTimes() + registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{ + {Key: stovepipemq.TopicKeyProjectAnalysis, Name: "projectanalysis", Queue: queue}, + {Key: stovepipemq.TopicKeyBuildAnalysis, Name: "buildanalysis", Queue: queue}, + {Key: stovepipemq.TopicKeyRecord, Name: "record", Queue: queue}, + }) + require.NoError(t, err) + m.history = storagemock.NewMockRequestLogStore(ctrl) + m.retainedLogs = map[string]entity.RequestLog{} + m.store.EXPECT().GetRequestLogStore().Return(m.history).AnyTimes() + m.history.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, _ string, id string) (entity.RequestLog, error) { + if log, ok := m.retainedLogs[id]; ok { + return log, nil + } + return entity.RequestLog{}, storage.ErrNotFound + }).AnyTimes() + c := NewDLQBuildSignalController( zap.NewNop().Sugar(), scope, staticStorageFactory{store: m.store}, - m.materializer, + m.materializer, registry, TopicKey(stovepipemq.TopicKeyBuildSignal), "stovepipe-buildsignal-dlq", ) @@ -113,6 +147,7 @@ func TestBuildSignalProcess(t *testing.T) { updateCall := m.reqStore.EXPECT().Update(gomock.Any(), updated, int32(2), int32(3)).Return(nil) request := requestWithState(entity.RequestStateFailed) request.Version = 3 + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(request, nil) m.materializer.EXPECT().PersistLog( gomock.Any(), m.store, @@ -124,7 +159,12 @@ func TestBuildSignalProcess(t *testing.T) { name: "already terminal request is a no-op", setup: func(m buildSignalDLQMocks) { m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(), nil) - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateSucceeded), nil) + request := requestWithState(entity.RequestStateSucceeded) + request.TerminalBuildID = testBuildID + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(request, nil).Times(2) + log, err := requestlog.NewBuildOutcomeLog(request) + require.NoError(t, err) + m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, log).Return(nil) }, }, { @@ -214,3 +254,72 @@ func TestBuildSignalTopicKey(t *testing.T) { assert.Equal(t, "buildsignal_dlq", c.Name()) assert.Equal(t, "stovepipe-buildsignal-dlq", c.ConsumerGroup()) } + +func TestBuildSignalRecoveryRepairsTerminalHandoff(t *testing.T) { + for _, tt := range []struct { + name string + state entity.RequestState + terminalBuild string + topic string + }{ + {name: "succeeded outcome", state: entity.RequestStateSucceeded, terminalBuild: testBuildID, topic: "buildanalysis"}, + {name: "failed outcome", state: entity.RequestStateFailed, terminalBuild: testBuildID, topic: "buildanalysis"}, + {name: "cancelled outcome", state: entity.RequestStateCancelled, terminalBuild: testBuildID, topic: "record"}, + {name: "unknown outcome", state: entity.RequestStateFailed, topic: "projectanalysis"}, + {name: "losing build", state: entity.RequestStateFailed, terminalBuild: "another-build"}, + } { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + c, m := newBuildSignalController(t, ctrl) + request := requestWithState(tt.state) + request.TerminalBuildID = tt.terminalBuild + m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(), nil).Times(2) + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(request, nil).Times(4) + if tt.terminalBuild != "" { + log, err := requestlog.NewBuildOutcomeLog(request) + require.NoError(t, err) + m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, log).Return(nil).Times(2) + } + if tt.state == entity.RequestStateFailed && tt.terminalBuild == "" { + m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).Return(nil).Times(2) + } + *m.publishFailure = assert.AnError + err := c.Process(queueContext(), delivery(t, ctrl, buildSignalPayload(t, testBuildID), "")) + if tt.topic == "" { + require.NoError(t, err) + } else { + require.ErrorIs(t, err, assert.AnError) + } + *m.publishFailure = nil + require.NoError(t, c.Process(queueContext(), delivery(t, ctrl, buildSignalPayload(t, testBuildID), ""))) + if tt.topic == "" { + require.Empty(t, m.published) + return + } + if tt.state == entity.RequestStateCancelled { + require.Len(t, m.published, 2) + require.Len(t, m.published["buildanalysis"], 2) + } else { + require.Len(t, m.published, 1) + } + attempts := m.published[tt.topic] + require.Len(t, attempts, 2) + require.Equal(t, attempts[0].ID, attempts[1].ID) + require.Equal(t, attempts[0].Payload, attempts[1].Payload) + }) + } +} + +func TestBuildSignalRecoveryPreservesAnotherFailureReason(t *testing.T) { + ctrl := gomock.NewController(t) + c, m := newBuildSignalController(t, ctrl) + request := requestWithState(entity.RequestStateFailed) + retained := requestlog.NewRequestStateLog(request, entity.RequestOutcomeReasonProcessingFailed) + retained.TimestampMs = 1 + m.retainedLogs[retained.ID] = retained + m.buildStore.EXPECT().Get(gomock.Any(), testBuildID).Return(build(), nil) + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(request, nil).Times(2) + m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, retained).Return(nil) + require.NoError(t, c.Process(queueContext(), delivery(t, ctrl, buildSignalPayload(t, testBuildID), ""))) + require.Len(t, m.published["projectanalysis"], 1) +} diff --git a/stovepipe/controller/dlq/dlq.go b/stovepipe/controller/dlq/dlq.go index 9d9a6c134..b170c5bcc 100644 --- a/stovepipe/controller/dlq/dlq.go +++ b/stovepipe/controller/dlq/dlq.go @@ -101,9 +101,16 @@ func failRequest( } if request.State.IsTerminal() { - if request.State == entity.RequestStateFailed { - // The originating DLQ remains the retry trigger after the state write. Its stage-specific - // reason repairs that write's missing log; PersistLog rejects an already-retained conflict. + if request.State == entity.RequestStateFailed && request.TerminalBuildID == "" { + // A prior reconciler may own the failure reason; repair its retained log rather than replacing it. + failedRequestLog := requestlog.NewRequestStateLog(request, reason) + retained, err := store.GetRequestLogStore().Get(ctx, request.ID, failedRequestLog.ID) + if err == nil { + return materializer.PersistLog(ctx, store, retained) + } + if !errors.Is(err, storage.ErrNotFound) { + return fmt.Errorf("read retained failed request state: %w", err) + } return persistFailureLog(ctx, store, materializer, request, reason) } logger.Infow("dlq reconcile: request already terminal, skipping", diff --git a/stovepipe/controller/dlq/dlq_test.go b/stovepipe/controller/dlq/dlq_test.go index eeb7c838c..75643db1c 100644 --- a/stovepipe/controller/dlq/dlq_test.go +++ b/stovepipe/controller/dlq/dlq_test.go @@ -75,6 +75,10 @@ func newController(t *testing.T, ctrl *gomock.Controller) (consumer.Controller, m.store.EXPECT().GetRequestStore().Return(m.reqStore).AnyTimes() m.store.EXPECT().GetQueueStore().Return(m.queueStore).AnyTimes() + history := storagemock.NewMockRequestLogStore(ctrl) + m.store.EXPECT().GetRequestLogStore().Return(history).AnyTimes() + history.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(entity.RequestLog{}, storage.ErrNotFound).AnyTimes() + c := NewDLQRequestController(zap.NewNop().Sugar(), scope, staticStorageFactory{store: m.store}, m.materializer, TopicKey(stovepipemq.TopicKeyProcess), "stovepipe-process-dlq") return c, m } diff --git a/stovepipe/controller/projectanalysis/README.md b/stovepipe/controller/projectanalysis/README.md index e6042fc3b..6b0dd2db6 100644 --- a/stovepipe/controller/projectanalysis/README.md +++ b/stovepipe/controller/projectanalysis/README.md @@ -4,17 +4,17 @@ Project analysis consumes request IDs after terminal build analysis has retained all final build issues. A missing terminal-analysis completion is an invalid handoff, not a reason to poll. -The controller resolves project statuses and records their facts and lookup entries before attributing issues. It materializes both directions of each issue/project association. Each step retains its completion event after its writes; repository recording is queued only after both steps finish. +The controller resolves project statuses and records their facts and lookup entries before attributing issues. It materializes both directions of each issue/project association. Each step retains its own event after its writes. Once both steps finish, the controller records `project_analysis_completed`, updates the request-summary completion timestamp, and queues repository recording. Project-fact and attribution events do not set that timestamp; attribution queries still require the successful attribution event. Retries adopt immutable facts and repair missing lookup entries. A completed -request skips provider calls and retries only the recording handoff. Empty +request skips provider calls, repairs its summary if needed, and retries the recording handoff. Empty results still record completion. Cancelled requests bypass project analysis; superseded requests require no work. ## Recovery -DLQ recovery leaves existing facts and association rows untouched. If completion -was already recorded, it retries only the repository-recording handoff. Otherwise +DLQ recovery leaves existing facts and association rows untouched. If `project_analysis_completed` +was already recorded, it repairs the summary and retries the repository-recording handoff. Otherwise it records `project_analysis_abandoned` before handing off, without calling either provider or marking attribution complete. History writes and handoff publication are safe to retry. diff --git a/stovepipe/controller/projectanalysis/dlq.go b/stovepipe/controller/projectanalysis/dlq.go index 928cd8862..e10bbffd3 100644 --- a/stovepipe/controller/projectanalysis/dlq.go +++ b/stovepipe/controller/projectanalysis/dlq.go @@ -119,9 +119,12 @@ func (c *DLQController) Process(ctx context.Context, delivery consumer.Delivery) } if request.TerminalBuildID != "" { - projectIssuesRecordedLog := requestlog.NewRequestEventLog(request, entity.RequestEventProjectIssuesRecorded, request.TerminalBuildID, nil) - _, err := store.GetRequestLogStore().Get(ctx, request.ID, projectIssuesRecordedLog.ID) + projectAnalysisCompletedLog := requestlog.NewRequestEventLog(request, entity.RequestEventProjectAnalysisCompleted, request.TerminalBuildID, nil) + retained, err := store.GetRequestLogStore().Get(ctx, request.ID, projectAnalysisCompletedLog.ID) if err == nil { + if err := c.requestLogMaterializer.PersistLog(ctx, store, retained); err != nil { + return err + } return publishRecord(ctx, c.registry, request) } if !errors.Is(err, storage.ErrNotFound) { diff --git a/stovepipe/controller/projectanalysis/dlq_test.go b/stovepipe/controller/projectanalysis/dlq_test.go index 22d15626c..674cfff5d 100644 --- a/stovepipe/controller/projectanalysis/dlq_test.go +++ b/stovepipe/controller/projectanalysis/dlq_test.go @@ -42,7 +42,7 @@ func (f *fixture) recover(ctx context.Context, msg entityqueue.Message) error { func TestDLQRetainsPartialResults(t *testing.T) { for _, operation := range []string{ "resolve", "attribute", "reverse/create", "fact/create", "index/create", - "log/project_facts_recorded", "log/project_issues_recorded", + "log/project_facts_recorded", "log/project_issues_recorded", "log/project_analysis_completed", } { t.Run(operation, func(t *testing.T) { f := newFixture(t) diff --git a/stovepipe/controller/projectanalysis/projectanalysis.go b/stovepipe/controller/projectanalysis/projectanalysis.go index a38bf627b..8cf12a147 100644 --- a/stovepipe/controller/projectanalysis/projectanalysis.go +++ b/stovepipe/controller/projectanalysis/projectanalysis.go @@ -119,8 +119,11 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r } history := store.GetRequestLogStore() projectAnalysisAbandonedLog := requestlog.NewRequestEventLog(request, entity.RequestEventProjectAnalysisAbandoned, "projects", nil) - _, err = history.Get(ctx, request.ID, projectAnalysisAbandonedLog.ID) + retained, err := history.Get(ctx, request.ID, projectAnalysisAbandonedLog.ID) if err == nil { + if err := c.requestLogMaterializer.PersistLog(ctx, store, retained); err != nil { + return err + } return publishRecord(ctx, c.registry, request) } if !errors.Is(err, storage.ErrNotFound) { @@ -130,9 +133,12 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r return fmt.Errorf("project analysis requires the request's terminal build") } - projectIssuesRecordedLog := requestlog.NewRequestEventLog(request, entity.RequestEventProjectIssuesRecorded, request.TerminalBuildID, nil) - _, err = history.Get(ctx, request.ID, projectIssuesRecordedLog.ID) + projectAnalysisCompletedLog := requestlog.NewRequestEventLog(request, entity.RequestEventProjectAnalysisCompleted, request.TerminalBuildID, nil) + retained, err = history.Get(ctx, request.ID, projectAnalysisCompletedLog.ID) if err == nil { + if err := c.requestLogMaterializer.PersistLog(ctx, store, retained); err != nil { + return err + } return publishRecord(ctx, c.registry, request) } if !errors.Is(err, storage.ErrNotFound) { @@ -149,6 +155,9 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r if err := c.recordIssueAttribution(ctx, store, request); err != nil { return err } + if err := c.requestLogMaterializer.PersistLog(ctx, store, projectAnalysisCompletedLog); err != nil { + return fmt.Errorf("record project analysis completion: %w", err) + } return publishRecord(ctx, c.registry, request) } diff --git a/stovepipe/controller/projectanalysis/projectanalysis_test.go b/stovepipe/controller/projectanalysis/projectanalysis_test.go index 084051c4b..265ded273 100644 --- a/stovepipe/controller/projectanalysis/projectanalysis_test.go +++ b/stovepipe/controller/projectanalysis/projectanalysis_test.go @@ -156,6 +156,10 @@ func newFixture(t *testing.T) *fixture { factsLog := requestlog.NewRequestEventLog(f.request, entity.RequestEventProjectFactsRecorded, "all", nil) require.Contains(t, f.logs, factsLog.ID) } + if log.Event == entity.RequestEventProjectAnalysisCompleted { + projectIssuesRecordedLog := requestlog.NewRequestEventLog(f.request, entity.RequestEventProjectIssuesRecorded, f.request.TerminalBuildID, nil) + require.Contains(t, f.logs, projectIssuesRecordedLog.ID) + } if err := f.fail("log/" + string(log.Event)); err != nil { return err } @@ -213,7 +217,7 @@ func newFixture(t *testing.T) *fixture { } func (f *fixture) completion() entity.RequestLog { - return requestlog.NewRequestEventLog(f.request, entity.RequestEventProjectIssuesRecorded, f.request.TerminalBuildID, nil) + return requestlog.NewRequestEventLog(f.request, entity.RequestEventProjectAnalysisCompleted, f.request.TerminalBuildID, nil) } func (f *fixture) fail(operation string) error { @@ -312,7 +316,7 @@ func TestProcessRecordsProjectStatuses(t *testing.T) { func TestProcessRetriesPartialWork(t *testing.T) { for _, operation := range []string{ "storage/for", "request/get", "abandonment/get", "completion/get", "analysis/get", "resolver/for", "resolve", "attributer/for", "attribute", - "source/create", "reverse/create", "fact/create", "index/create", "log/project_facts_recorded", "log/project_issues_recorded", "record/publish", + "source/create", "reverse/create", "fact/create", "index/create", "log/project_facts_recorded", "log/project_issues_recorded", "log/project_analysis_completed", "record/publish", } { t.Run(operation, func(t *testing.T) { f := newFixture(t) @@ -331,6 +335,9 @@ func TestProcessRetriesPartialWork(t *testing.T) { require.Zero(t, f.attributerCalls) require.Empty(t, f.source) require.Empty(t, f.reverse) + case "log/project_analysis_completed": + projectIssuesRecordedLog := requestlog.NewRequestEventLog(f.request, entity.RequestEventProjectIssuesRecorded, f.request.TerminalBuildID, nil) + require.Contains(t, f.logs, projectIssuesRecordedLog.ID) case "attributer/for", "attribute", "source/create", "reverse/create": require.Len(t, f.facts, 2) require.Len(t, f.indexes, 2) @@ -478,6 +485,46 @@ func TestProcessRejectsInvalidIdentity(t *testing.T) { } } +func TestCompletedAnalysisRepairsProjectionBeforeHandoff(t *testing.T) { + for _, recovery := range []bool{false, true} { + name := "normal delivery" + if recovery { + name = "dead-letter recovery" + } + t.Run(name, func(t *testing.T) { + f := newFixture(t) + msg := f.message(t) + f.failures["record/publish"] = assert.AnError + require.ErrorIs(t, f.deliver(context.Background(), msg), assert.AnError) + f.failures["log/project_analysis_completed"] = assert.AnError + deliver := f.deliver + if recovery { + deliver = f.recover + } + require.ErrorIs(t, deliver(context.Background(), msg), assert.AnError) + require.Empty(t, f.records) + require.Equal(t, 1, f.resolverCalls) + require.Equal(t, 1, f.attributerCalls) + require.NoError(t, deliver(context.Background(), msg)) + require.Len(t, f.records, 1) + }) + } +} + +func TestAbandonedAnalysisRepairsProjectionBeforeHandoff(t *testing.T) { + f := newFixture(t) + msg := f.message(t) + f.failures["record/publish"] = assert.AnError + require.ErrorIs(t, f.recover(context.Background(), msg), assert.AnError) + f.failures["log/project_analysis_abandoned"] = assert.AnError + require.ErrorIs(t, f.deliver(context.Background(), msg), assert.AnError) + require.Empty(t, f.records) + require.Zero(t, f.resolverCalls) + require.Zero(t, f.attributerCalls) + require.NoError(t, f.deliver(context.Background(), msg)) + require.Len(t, f.records, 1) +} + const projectStatusTestQueue = "monorepo/main" var projectStatusTestChangeTime = time.Unix(1_700_000_000, 0).UTC() diff --git a/stovepipe/controller/record/BUILD.bazel b/stovepipe/controller/record/BUILD.bazel index 1d984c2cc..ac7106148 100644 --- a/stovepipe/controller/record/BUILD.bazel +++ b/stovepipe/controller/record/BUILD.bazel @@ -21,7 +21,6 @@ go_library( "//stovepipe/core/requestlog:go_default_library", "//stovepipe/core/validationfact:go_default_library", "//stovepipe/entity:go_default_library", - "//stovepipe/extension/projectstatus:go_default_library", "//stovepipe/extension/sourcecontrol:go_default_library", "//stovepipe/extension/storage:go_default_library", "@com_github_uber_go_tally//:go_default_library", @@ -49,9 +48,6 @@ go_test( "//stovepipe/core/requestlog:go_default_library", "//stovepipe/core/requestlog/mock:go_default_library", "//stovepipe/entity:go_default_library", - "//stovepipe/extension/projectstatus:go_default_library", - "//stovepipe/extension/projectstatus/mock:go_default_library", - "//stovepipe/extension/projectstatus/noop:go_default_library", "//stovepipe/extension/sourcecontrol:go_default_library", "//stovepipe/extension/sourcecontrol/mock:go_default_library", "//stovepipe/extension/storage:go_default_library", diff --git a/stovepipe/controller/record/dlq.go b/stovepipe/controller/record/dlq.go index ea605edf9..ffd3f3564 100644 --- a/stovepipe/controller/record/dlq.go +++ b/stovepipe/controller/record/dlq.go @@ -142,7 +142,7 @@ func (c *DLQController) Process(ctx context.Context, delivery consumer.Delivery) } if request.State == entity.RequestStateFailed { - fact, _, err := recordFact(ctx, c.logger, c.metricsScope, store, request, nil) + fact, _, err := recordFact(ctx, c.logger, c.metricsScope, store, request, 0) if err != nil { return err } diff --git a/stovepipe/controller/record/facts.go b/stovepipe/controller/record/facts.go index 3612b6d99..2c344b05a 100644 --- a/stovepipe/controller/record/facts.go +++ b/stovepipe/controller/record/facts.go @@ -30,7 +30,6 @@ import ( "github.com/uber/submitqueue/stovepipe/core/requestlog" "github.com/uber/submitqueue/stovepipe/core/validationfact" "github.com/uber/submitqueue/stovepipe/entity" - "github.com/uber/submitqueue/stovepipe/extension/projectstatus" "github.com/uber/submitqueue/stovepipe/extension/storage" "go.uber.org/zap" ) @@ -41,12 +40,12 @@ func recordFact( scope tally.Scope, store storage.Storage, request entity.Request, - results []projectstatus.Result, + projectCount int32, ) (entity.ValidationFact, bool, error) { return validationfact.Persist(ctx, logger, scope, store.GetValidationFactStore(), entity.ValidationFact{ URI: request.URI, Project: wholeRepositoryProject, - Degree: degreeFor(request.State, results), + Degree: degreeFor(request.State, projectCount), RequestID: request.ID, CreatedAt: time.Now().UnixMilli(), }) diff --git a/stovepipe/controller/record/record.go b/stovepipe/controller/record/record.go index d35968f16..1f14c03c2 100644 --- a/stovepipe/controller/record/record.go +++ b/stovepipe/controller/record/record.go @@ -13,10 +13,10 @@ // limitations under the License. // Package record holds the record-stage queue controller. It consumes Record -// messages (a request id) published by buildsignal once a build reaches a -// terminal status, and turns that outcome into durable validation state. +// messages (a request id) published after project analysis or cancellation, +// and turns that outcome into durable repository validation state. // -// The durable state is a ValidationFact per validated commit and project. Build +// The durable state is a repository ValidationFact per validated commit. Build // completion owns the queue's last-green bookmark, which process reads to choose // an incremental build baseline. A green commit is also promoted after its facts // are persisted, moving the queue's promotion ref so downstream systems can pull @@ -30,10 +30,7 @@ package record import ( "context" - "errors" "fmt" - "math" - "strconv" "time" "github.com/uber-go/tally" @@ -44,9 +41,7 @@ import ( "github.com/uber/submitqueue/stovepipe/core/loader" stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" "github.com/uber/submitqueue/stovepipe/core/requestlog" - "github.com/uber/submitqueue/stovepipe/core/validationfact" "github.com/uber/submitqueue/stovepipe/entity" - "github.com/uber/submitqueue/stovepipe/extension/projectstatus" "github.com/uber/submitqueue/stovepipe/extension/sourcecontrol" "github.com/uber/submitqueue/stovepipe/extension/storage" "go.uber.org/zap" @@ -56,15 +51,14 @@ import ( // when that fact is green and holds the queue's last-green bookmark promotes // the commit. Implements consumer.Controller. type Controller struct { - logger *zap.SugaredLogger - metricsScope tally.Scope - materializer requestlog.Materializer - registry consumer.TopicRegistry - stores storage.Factory - projectStatusResolverFactory projectstatus.ResolverFactory - sourceControl sourcecontrol.Factory - topicKey consumer.TopicKey - consumerGroup string + logger *zap.SugaredLogger + metricsScope tally.Scope + materializer requestlog.Materializer + registry consumer.TopicRegistry + stores storage.Factory + sourceControl sourcecontrol.Factory + topicKey consumer.TopicKey + consumerGroup string } // Verify Controller implements consumer.Controller interface at compile time. @@ -82,7 +76,6 @@ func NewController( scope tally.Scope, stores storage.Factory, materializer requestlog.Materializer, - projectStatusResolverFactory projectstatus.ResolverFactory, sourceControl sourcecontrol.Factory, registry consumer.TopicRegistry, topicKey consumer.TopicKey, @@ -90,15 +83,14 @@ func NewController( ) *Controller { name := string(topicKey) + "_controller" return &Controller{ - logger: logger.Named(name), - metricsScope: scope.SubScope(name), - materializer: materializer, - registry: registry, - stores: stores, - projectStatusResolverFactory: projectStatusResolverFactory, - sourceControl: sourceControl, - topicKey: topicKey, - consumerGroup: consumerGroup, + logger: logger.Named(name), + metricsScope: scope.SubScope(name), + materializer: materializer, + registry: registry, + stores: stores, + sourceControl: sourceControl, + topicKey: topicKey, + consumerGroup: consumerGroup, } } @@ -144,20 +136,17 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er switch request.State { case entity.RequestStateSucceeded, entity.RequestStateFailed: - results, err := c.resolveProjectResults(ctx, request) + summary, err := store.GetRequestSummaryStore().Get(ctx, request.ID) if err != nil { return err } - fact, created, err := recordFact(ctx, c.logger, c.metricsScope, store, request, results) + fact, created, err := recordFact(ctx, c.logger, c.metricsScope, store, request, summary.ProjectCount) if err != nil { return err } if err := persistValidationFactRecordedLog(ctx, c.materializer, store, request, fact); err != nil { return err } - if err := c.persistProjectFacts(ctx, store, request, results); err != nil { - return err - } if err := c.applyFactToDerivedCaches(ctx, store, request, fact, created); err != nil { return err } @@ -184,99 +173,6 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) er } } -func (c *Controller) resolveProjectResults(ctx context.Context, request entity.Request) (results []projectstatus.Result, retErr error) { - op := metrics.Begin(c.metricsScope, "resolve_project_results", metrics.LongLatencyBuckets, metrics.TagsFromContext(ctx)...) - defer func() { op.Complete(retErr) }() - - resolver, err := c.projectStatusResolverFactory.For(projectstatus.ResolverConfig{QueueName: request.Queue}) - if err != nil { - return nil, fmt.Errorf("resolve project status resolver for queue %q: %w", request.Queue, err) - } - results, err = resolver.Resolve(ctx, request) - if err != nil { - return nil, fmt.Errorf("resolve project status results for request %q: %w", request.ID, err) - } - - if err := validateProjectStatusResults(request, results); err != nil { - return nil, err - } - metrics.NamedHistogram(c.metricsScope, "resolve_project_results", "projects", metrics.CountBuckets, - metrics.TagsFromContext(ctx)..., - ).RecordValue(float64(len(results))) - return results, nil -} - -func (c *Controller) persistProjectFacts(ctx context.Context, store storage.Storage, request entity.Request, results []projectstatus.Result) (retErr error) { - op := metrics.Begin(c.metricsScope, "persist_project_facts", metrics.LongLatencyBuckets, metrics.TagsFromContext(ctx)...) - defer func() { op.Complete(retErr) }() - - brokenCount := 0 - for _, result := range results { - fact, _, err := validationfact.Persist(ctx, c.logger, c.metricsScope, store.GetValidationFactStore(), entity.ValidationFact{ - URI: request.URI, - Project: result.ProjectInfo.Name, - ProjectType: result.ProjectInfo.ProjectType, - Degree: result.Degree, - RequestID: request.ID, - CreatedAt: time.Now().UnixMilli(), - }) - if err != nil { - return err - } - if !fact.IsGreen() { - brokenCount++ - } - - entry, err := entity.NewProjectFactByDegree(fact) - if err != nil { - return err - } - - if err := store.GetProjectFactByDegreeStore().Create(ctx, entry); err != nil && !errors.Is(err, storage.ErrAlreadyExists) { - return fmt.Errorf("failed to index project fact for request %q and project %q: %w", request.ID, fact.Project, err) - } - } - return c.persistProjectFactsRecordedLog(ctx, store, request, len(results), brokenCount) -} - -func validateProjectStatusResults(request entity.Request, results []projectstatus.Result) error { - seen := make(map[string]struct{}, len(results)) - for _, result := range results { - if result.ProjectInfo.Name == "" { - return fmt.Errorf("project status result for request %q has an empty project", request.ID) - } - if _, ok := seen[result.ProjectInfo.Name]; ok { - return fmt.Errorf("project status result for request %q contains duplicate project %q", request.ID, result.ProjectInfo.Name) - } - seen[result.ProjectInfo.Name] = struct{}{} - if math.IsNaN(result.Degree) || result.Degree < entity.DegreeGreen || result.Degree > entity.DegreeBroken { - return fmt.Errorf("project status result for request %q and project %q has invalid degree %v", request.ID, result.ProjectInfo.Name, result.Degree) - } - } - return nil -} - -func (c *Controller) persistProjectFactsRecordedLog( - ctx context.Context, - store storage.Storage, - request entity.Request, - count, brokenCount int, -) error { - log := requestlog.NewRequestEventLog( - request, - entity.RequestEventProjectFactsRecorded, - "all", - map[string]string{ - requestlog.MetadataKeyProjectFactCount: strconv.Itoa(count), - requestlog.MetadataKeyBrokenProjectFactCount: strconv.Itoa(brokenCount), - }, - ) - if err := c.materializer.PersistLog(ctx, store, log); err != nil { - return fmt.Errorf("failed to record project facts for request %s: %w", request.ID, err) - } - return nil -} - // applyFactToDerivedCaches follows the persisted fact: a green fact promotes the // commit when this request holds the queue's bookmark, set by build completion. // A broken fact skips promotion, and instead reports how long the break it names @@ -389,11 +285,11 @@ func (c *Controller) failureDetectionUnobserved(ctx context.Context, request ent // degreeFor maps a request's build outcome onto a whole-repository degree. // A failed build with project results has a partial degree. -func degreeFor(state entity.RequestState, results []projectstatus.Result) float64 { +func degreeFor(state entity.RequestState, projectCount int32) float64 { if state == entity.RequestStateSucceeded { return entity.DegreeGreen } - if len(results) > 0 { + if projectCount > 0 { return entity.DegreePartial } return entity.DegreeBroken diff --git a/stovepipe/controller/record/record_test.go b/stovepipe/controller/record/record_test.go index ed5840db7..62da43bcc 100644 --- a/stovepipe/controller/record/record_test.go +++ b/stovepipe/controller/record/record_test.go @@ -17,7 +17,6 @@ package record import ( "context" "errors" - "math" "testing" "time" @@ -35,9 +34,6 @@ import ( "github.com/uber/submitqueue/stovepipe/core/requestlog" requestlogmock "github.com/uber/submitqueue/stovepipe/core/requestlog/mock" "github.com/uber/submitqueue/stovepipe/entity" - "github.com/uber/submitqueue/stovepipe/extension/projectstatus" - projectstatusmock "github.com/uber/submitqueue/stovepipe/extension/projectstatus/mock" - projectstatusnoop "github.com/uber/submitqueue/stovepipe/extension/projectstatus/noop" "github.com/uber/submitqueue/stovepipe/extension/sourcecontrol" sourcecontrolmock "github.com/uber/submitqueue/stovepipe/extension/sourcecontrol/mock" "github.com/uber/submitqueue/stovepipe/extension/storage" @@ -74,7 +70,9 @@ type recordMocks struct { reqStore *storagemock.MockRequestStore queueStore *storagemock.MockQueueStore factStore *storagemock.MockValidationFactStore - indexStore *storagemock.MockProjectFactByDegreeStore + summary *entity.RequestSummary + summaries *storagemock.MockRequestSummaryStore + summaryErr *error store *storagemock.MockStorage materializer *requestlogmock.MockMaterializer sourceControl *sourcecontrolmock.MockSourceControl @@ -149,7 +147,9 @@ func newControllerForTopic(t *testing.T, ctrl *gomock.Controller, topicKey consu reqStore: storagemock.NewMockRequestStore(ctrl), queueStore: storagemock.NewMockQueueStore(ctrl), factStore: storagemock.NewMockValidationFactStore(ctrl), - indexStore: storagemock.NewMockProjectFactByDegreeStore(ctrl), + summary: &entity.RequestSummary{RequestID: testID, Queue: testQueue, URI: testURI, ProjectResultsCompletedAtMs: 1000}, + summaries: storagemock.NewMockRequestSummaryStore(ctrl), + summaryErr: new(error), store: storagemock.NewMockStorage(ctrl), materializer: requestlogmock.NewMockMaterializer(ctrl), sourceControl: sourcecontrolmock.NewMockSourceControl(ctrl), @@ -160,10 +160,11 @@ func newControllerForTopic(t *testing.T, ctrl *gomock.Controller, topicKey consu m.store.EXPECT().GetRequestStore().Return(m.reqStore).AnyTimes() m.store.EXPECT().GetQueueStore().Return(m.queueStore).AnyTimes() m.store.EXPECT().GetValidationFactStore().Return(m.factStore).AnyTimes() - m.store.EXPECT().GetProjectFactByDegreeStore().Return(m.indexStore).AnyTimes() + m.store.EXPECT().GetRequestSummaryStore().Return(m.summaries).AnyTimes() + m.summaries.EXPECT().Get(gomock.Any(), testID).DoAndReturn(func(context.Context, string) (entity.RequestSummary, error) { return *m.summary, *m.summaryErr }).AnyTimes() m.materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).DoAndReturn( func(_ context.Context, _ storage.Storage, log entity.RequestLog) error { - assert.Contains(t, []entity.RequestEvent{entity.RequestEventValidationFactRecorded, entity.RequestEventProjectFactsRecorded}, log.Event) + assert.Equal(t, entity.RequestEventValidationFactRecorded, log.Event) assert.Empty(t, log.State) assert.Zero(t, log.RequestVersion) assert.Empty(t, log.OutcomeReason) @@ -201,7 +202,6 @@ func newControllerForTopic(t *testing.T, ctrl *gomock.Controller, topicKey consu scope, staticStorageFactory{store: m.store}, m.materializer, - projectstatusnoop.New(), staticSourceControlFactory{sourceControl: m.sourceControl}, registry, topicKey, @@ -292,22 +292,6 @@ func TestProcess_PromotesFinalizedGreenAtCurrentBaseline(t *testing.T) { assert.Len(t, m.hooks.events, 1) } -func TestProcess_WaitsForProjectResolutionBeforeRecordingOrPromotion(t *testing.T) { - ctrl := gomock.NewController(t) - c, m := newController(t, ctrl) - request := requestWithState(entity.RequestStateSucceeded) - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(request, nil) - factory := projectstatusmock.NewMockResolverFactory(ctrl) - resolver := projectstatusmock.NewMockResolver(ctrl) - c.projectStatusResolverFactory = factory - factory.EXPECT().For(projectstatus.ResolverConfig{QueueName: testQueue}).Return(resolver, nil) - resolver.EXPECT().Resolve(gomock.Any(), request).Return(nil, assert.AnError) - require.ErrorIs(t, c.Process(queueContext(), delivery(t, ctrl, recordPayload(t, testID))), assert.AnError) - assert.Empty(t, m.hooks.events) - assertRecordOperationMetrics(t, m.metricsScope, "resolve_project_results", "error") - assert.NotContains(t, m.metricsScope.Snapshot().Counters(), "record_controller.persist_project_facts.start+queue="+testQueue) -} - func TestProcess_RejectsLegacyBookmark(t *testing.T) { ctrl := gomock.NewController(t) c, m := newController(t, ctrl) @@ -324,346 +308,6 @@ func TestProcess_RejectsLegacyBookmark(t *testing.T) { assert.Empty(t, m.hooks.events) } -func TestProcess_RecordsNamedProjectStatusResults(t *testing.T) { - ctrl := gomock.NewController(t) - c, m := newController(t, ctrl) - eventMaterializer := requestlogmock.NewMockMaterializer(ctrl) - c.materializer = eventMaterializer - projectStatusResolverFactory := projectstatusmock.NewMockResolverFactory(ctrl) - projectStatusResolver := projectstatusmock.NewMockResolver(ctrl) - c.projectStatusResolverFactory = projectStatusResolverFactory - projectStatusResolverFactory.EXPECT().For(projectstatus.ResolverConfig{QueueName: testQueue}). - Return(projectStatusResolver, nil) - projectStatusResolver.EXPECT().Resolve( - gomock.Any(), requestWithState(entity.RequestStateFailed), - ).Return([]projectstatus.Result{ - {ProjectInfo: projectstatus.ProjectInfo{Name: "project-a"}, Degree: entity.DegreeBroken}, - {ProjectInfo: projectstatus.ProjectInfo{Name: "project-b"}, Degree: entity.DegreeGreen}, - }, nil) - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateFailed), nil) - - var facts []entity.ValidationFact - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()). - DoAndReturn(func(_ context.Context, fact entity.ValidationFact) error { - facts = append(facts, fact) - return nil - }). - Times(3) - - var indexes []entity.ProjectFactByDegree - m.indexStore.EXPECT().Create(gomock.Any(), gomock.Any()). - DoAndReturn(func(_ context.Context, entry entity.ProjectFactByDegree) error { - indexes = append(indexes, entry) - return nil - }).Times(2) - - var logs []entity.RequestLog - eventMaterializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()). - DoAndReturn(func(_ context.Context, _ storage.Storage, log entity.RequestLog) error { - logs = append(logs, log) - return nil - }). - Times(2) - - require.NoError(t, c.Process(queueContext(), delivery(t, ctrl, recordPayload(t, testID)))) - require.Len(t, facts, 3) - require.Len(t, indexes, 2) - for i, fact := range facts[1:] { - entry, err := entity.NewProjectFactByDegree(fact) - require.NoError(t, err) - assert.Equal(t, entry, indexes[i]) - } - assert.Equal(t, []string{"", "project-a", "project-b"}, []string{facts[0].Project, facts[1].Project, facts[2].Project}) - for _, fact := range facts { - assert.Equal(t, testURI, fact.URI) - assert.Equal(t, testID, fact.RequestID) - if fact.Project == "" { - assert.Equal(t, entity.DegreePartial, fact.Degree) - } else if fact.Project == "project-b" { - assert.Equal(t, entity.DegreeGreen, fact.Degree) - } else { - assert.Equal(t, entity.DegreeBroken, fact.Degree) - } - assert.Positive(t, fact.CreatedAt) - } - require.Len(t, logs, 2) - assert.Equal(t, entity.RequestEventValidationFactRecorded, logs[0].Event) - assert.Equal(t, entity.RequestEventProjectFactsRecorded, logs[1].Event) - assert.Equal(t, "2", logs[1].Metadata[requestlog.MetadataKeyProjectFactCount]) - assertRecordOperationMetrics(t, m.metricsScope, "resolve_project_results", "success") - assertRecordOperationMetrics(t, m.metricsScope, "persist_project_facts", "success") - assert.Equal(t, "1", logs[1].Metadata[requestlog.MetadataKeyBrokenProjectFactCount]) -} - -func TestResolveProjectResults_PassesRequestBuildContext(t *testing.T) { - for _, tt := range []struct { - name string - buildID string - terminalBuildID string - }{ - {name: "terminal outcome", buildID: "triggered-build", terminalBuildID: "terminal-build"}, - {name: "no terminal outcome", buildID: "triggered-build"}, - {name: "no build"}, - } { - t.Run(tt.name, func(t *testing.T) { - ctrl := gomock.NewController(t) - c, _ := newController(t, ctrl) - request := requestWithState(entity.RequestStateFailed) - request.BuildID = tt.buildID - request.TerminalBuildID = tt.terminalBuildID - factory := projectstatusmock.NewMockResolverFactory(ctrl) - resolver := projectstatusmock.NewMockResolver(ctrl) - c.projectStatusResolverFactory = factory - factory.EXPECT().For(projectstatus.ResolverConfig{QueueName: testQueue}).Return(resolver, nil) - resolver.EXPECT().Resolve(gomock.Any(), request).Return(nil, nil) - - results, err := c.resolveProjectResults(queueContext(), request) - - require.NoError(t, err) - assert.Empty(t, results) - }) - } -} - -func TestResolveProjectResultsMetrics(t *testing.T) { - validResults := []projectstatus.Result{ - {ProjectInfo: projectstatus.ProjectInfo{Name: "project-a"}, Degree: entity.DegreeGreen}, - {ProjectInfo: projectstatus.ProjectInfo{Name: "project-b"}, Degree: entity.DegreeBroken}, - } - for _, tt := range []struct { - name string - results []projectstatus.Result - factoryErr error - resolveErr error - invalid bool - wantResult string - }{ - {name: "projects", results: validResults, wantResult: "success"}, - {name: "nil results", wantResult: "success"}, - {name: "empty results", results: []projectstatus.Result{}, wantResult: "success"}, - {name: "factory error", factoryErr: assert.AnError, wantResult: "error"}, - {name: "factory cancellation", factoryErr: context.Canceled, wantResult: "cancel"}, - {name: "resolver error with partial results", results: validResults, resolveErr: assert.AnError, wantResult: "error"}, - {name: "resolver cancellation", resolveErr: context.Canceled, wantResult: "cancel"}, - {name: "resolver deadline", resolveErr: context.DeadlineExceeded, wantResult: "error"}, - {name: "invalid project", results: []projectstatus.Result{{Degree: entity.DegreeGreen}}, invalid: true, wantResult: "error"}, - {name: "invalid degree", results: []projectstatus.Result{{ProjectInfo: projectstatus.ProjectInfo{Name: "project-a"}, Degree: math.NaN()}}, invalid: true, wantResult: "error"}, - {name: "duplicate projects", results: []projectstatus.Result{validResults[0], validResults[0]}, invalid: true, wantResult: "error"}, - } { - t.Run(tt.name, func(t *testing.T) { - ctrl := gomock.NewController(t) - c, m := newController(t, ctrl) - request := failedRequest() - factory := projectstatusmock.NewMockResolverFactory(ctrl) - resolver := projectstatusmock.NewMockResolver(ctrl) - c.projectStatusResolverFactory = factory - factory.EXPECT().For(projectstatus.ResolverConfig{QueueName: testQueue}).Return(resolver, tt.factoryErr) - if tt.factoryErr == nil { - resolver.EXPECT().Resolve(gomock.Any(), request).Return(tt.results, tt.resolveErr) - } - - got, err := c.resolveProjectResults(queueContext(), request) - - switch { - case tt.factoryErr != nil: - require.ErrorIs(t, err, tt.factoryErr) - case tt.resolveErr != nil: - require.ErrorIs(t, err, tt.resolveErr) - case tt.invalid: - require.Error(t, err) - default: - require.NoError(t, err) - assert.Equal(t, tt.results, got) - } - assertRecordOperationMetrics(t, m.metricsScope, "resolve_project_results", tt.wantResult) - histograms := m.metricsScope.Snapshot().Histograms() - const projectsKey = "record_controller.resolve_project_results.projects+queue=" + testQueue - if err != nil { - assert.NotContains(t, histograms, projectsKey) - return - } - projects, ok := histograms[projectsKey] - require.True(t, ok) - assert.EqualValues(t, 1, projects.Values()[float64(len(tt.results))]) - var samples int64 - for _, count := range projects.Values() { - samples += count - } - assert.EqualValues(t, 1, samples) - }) - } -} - -func TestPersistProjectFactsMetrics(t *testing.T) { - for _, tt := range []struct { - name string - empty bool - existing bool - factErr error - indexErr error - completionErr error - wantResult string - }{ - {name: "projects", wantResult: "success"}, - {name: "empty results", empty: true, wantResult: "success"}, - {name: "existing facts and indexes", existing: true, wantResult: "success"}, - {name: "fact write error", factErr: assert.AnError, wantResult: "error"}, - {name: "fact write cancellation", factErr: context.Canceled, wantResult: "cancel"}, - {name: "index write error", indexErr: assert.AnError, wantResult: "error"}, - {name: "index write cancellation", indexErr: context.Canceled, wantResult: "cancel"}, - {name: "completion error", completionErr: assert.AnError, wantResult: "error"}, - {name: "completion cancellation", completionErr: context.Canceled, wantResult: "cancel"}, - {name: "empty completion error", empty: true, completionErr: assert.AnError, wantResult: "error"}, - } { - t.Run(tt.name, func(t *testing.T) { - ctrl := gomock.NewController(t) - c, m := newController(t, ctrl) - request := failedRequest() - var results []projectstatus.Result - if !tt.empty { - results = []projectstatus.Result{ - {ProjectInfo: projectstatus.ProjectInfo{Name: "project-a"}, Degree: entity.DegreeGreen}, - {ProjectInfo: projectstatus.ProjectInfo{Name: "project-b"}, Degree: entity.DegreeBroken}, - } - } - materializer := requestlogmock.NewMockMaterializer(ctrl) - c.materializer = materializer - for _, result := range results { - fact := entity.ValidationFact{URI: request.URI, Project: result.ProjectInfo.Name, Degree: result.Degree, RequestID: request.ID, CreatedAt: testChangeTime.UnixMilli()} - createErr := tt.factErr - if tt.existing { - createErr = storage.ErrAlreadyExists - } - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(createErr) - if tt.factErr != nil { - break - } - indexErr := tt.indexErr - if tt.existing { - m.factStore.EXPECT().Get(gomock.Any(), request.URI, result.ProjectInfo.Name).Return(fact, nil) - indexErr = storage.ErrAlreadyExists - } - entry, err := entity.NewProjectFactByDegree(fact) - require.NoError(t, err) - m.indexStore.EXPECT().Create(gomock.Any(), entry).Return(indexErr) - if tt.indexErr != nil { - break - } - } - if tt.factErr == nil && tt.indexErr == nil { - materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).DoAndReturn( - func(_ context.Context, _ storage.Storage, log entity.RequestLog) error { - assert.Equal(t, entity.RequestEventProjectFactsRecorded, log.Event) - return tt.completionErr - }) - } - - err := c.persistProjectFacts(queueContext(), m.store, request, results) - - switch { - case tt.factErr != nil: - require.ErrorIs(t, err, tt.factErr) - case tt.indexErr != nil: - require.ErrorIs(t, err, tt.indexErr) - case tt.completionErr != nil: - require.ErrorIs(t, err, tt.completionErr) - default: - require.NoError(t, err) - } - assertRecordOperationMetrics(t, m.metricsScope, "persist_project_facts", tt.wantResult) - }) - } -} - -func assertRecordOperationMetrics(t *testing.T, scope tally.TestScope, operation, result string) { - t.Helper() - snapshot := scope.Snapshot() - start, ok := snapshot.Counters()["record_controller."+operation+".start+queue="+testQueue] - require.True(t, ok) - assert.EqualValues(t, 1, start.Value()) - finish, ok := snapshot.Histograms()["record_controller."+operation+".finish+queue="+testQueue+",result="+result] - require.True(t, ok) - assert.EqualValues(t, 1, totalSamples(finish.Durations())) -} - -func TestProcessRecordsEmptyProjectCompletion(t *testing.T) { - resolveErr := errors.New("project resolution unavailable") - writeErr := errors.New("completion write unavailable") - for _, tt := range []struct { - name string - results []projectstatus.Result - resolveErr error - writeErr error - }{ - {name: "nil results"}, - {name: "empty results", results: []projectstatus.Result{}}, - {name: "resolution failed", resolveErr: resolveErr}, - {name: "completion write failed", writeErr: writeErr}, - } { - t.Run(tt.name, func(t *testing.T) { - ctrl := gomock.NewController(t) - c, m := newController(t, ctrl) - request := requestWithState(entity.RequestStateFailed) - m.reqStore.EXPECT().Get(gomock.Any(), request.ID).Return(request, nil) - factory := projectstatusmock.NewMockResolverFactory(ctrl) - resolver := projectstatusmock.NewMockResolver(ctrl) - c.projectStatusResolverFactory = factory - factory.EXPECT().For(projectstatus.ResolverConfig{QueueName: testQueue}).Return(resolver, nil) - resolver.EXPECT().Resolve(gomock.Any(), request).Return(tt.results, tt.resolveErr) - materializer := requestlogmock.NewMockMaterializer(ctrl) - c.materializer = materializer - if tt.resolveErr == nil { - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - gomock.InOrder( - materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).DoAndReturn( - func(_ context.Context, _ storage.Storage, log entity.RequestLog) error { - assert.Equal(t, entity.RequestEventValidationFactRecorded, log.Event) - return nil - }), - materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).DoAndReturn( - func(_ context.Context, _ storage.Storage, log entity.RequestLog) error { - assert.Equal(t, "event/project_facts_recorded/all", log.ID) - assert.Equal(t, "0", log.Metadata[requestlog.MetadataKeyProjectFactCount]) - assert.Equal(t, "0", log.Metadata[requestlog.MetadataKeyBrokenProjectFactCount]) - return tt.writeErr - }), - ) - } - err := c.Process(queueContext(), delivery(t, ctrl, recordPayload(t, request.ID))) - switch { - case tt.resolveErr != nil: - require.ErrorIs(t, err, tt.resolveErr) - assert.Empty(t, m.hooks.events) - case tt.writeErr != nil: - require.ErrorIs(t, err, tt.writeErr) - assert.Empty(t, m.hooks.events) - default: - require.NoError(t, err) - assert.Len(t, m.hooks.events, 1) - } - }) - } -} - -func TestProcess_RejectsInvalidProjectStatusResultsBeforeWritingFacts(t *testing.T) { - ctrl := gomock.NewController(t) - c, m := newController(t, ctrl) - projectStatusResolverFactory := projectstatusmock.NewMockResolverFactory(ctrl) - projectStatusResolver := projectstatusmock.NewMockResolver(ctrl) - c.projectStatusResolverFactory = projectStatusResolverFactory - projectStatusResolverFactory.EXPECT().For(projectstatus.ResolverConfig{QueueName: testQueue}). - Return(projectStatusResolver, nil) - projectStatusResolver.EXPECT().Resolve(gomock.Any(), gomock.Any()).Return([]projectstatus.Result{ - {ProjectInfo: projectstatus.ProjectInfo{Name: "project-a"}, Degree: entity.DegreeBroken}, - {ProjectInfo: projectstatus.ProjectInfo{Name: "project-b"}, Degree: math.NaN()}, - }, nil) - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(entity.RequestStateFailed), nil) - - err := c.Process(queueContext(), delivery(t, ctrl, recordPayload(t, testID))) - require.Error(t, err) - assert.ErrorContains(t, err, "invalid degree") -} - func TestProcess_TimestampReportingFailureDoesNotFailRecord(t *testing.T) { tests := []struct { name string @@ -778,7 +422,6 @@ func TestProcess_RecordsFactEventBeforeDerivedWork(t *testing.T) { m.sourceControl.EXPECT().ChangeInfo(gomock.Any(), testURI). Return(sourcecontrol.ChangeInfo{CreatedAt: testChangeTime.UnixMilli()}, nil) m.sourceControl.EXPECT().Promote(gomock.Any(), testURI).Return(nil) - eventMaterializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).Return(nil) require.NoError(t, c.Process(queueContext(), delivery(t, ctrl, recordPayload(t, testID)))) } @@ -1373,184 +1016,41 @@ func TestProcess_QueueMismatchFails(t *testing.T) { require.Error(t, c.Process(queueContext(), d)) } -func TestPersistProjectFactsRepairsIndexOnRetry(t *testing.T) { - for _, retryErr := range []error{nil, storage.ErrAlreadyExists} { - name := "index repaired" - if retryErr != nil { - name = "index already repaired" - } - t.Run(name, func(t *testing.T) { - ctrl := gomock.NewController(t) - c, m := newController(t, ctrl) - request := requestWithState(entity.RequestStateFailed) - stored := entity.ValidationFact{URI: request.URI, Project: "p", Degree: 0.25, RequestID: request.ID, CreatedAt: 10} - entry, err := entity.NewProjectFactByDegree(stored) - require.NoError(t, err) - writeErr := errors.New("index write unavailable") - materializer := requestlogmock.NewMockMaterializer(ctrl) - c.materializer = materializer - - gomock.InOrder( - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil), - m.indexStore.EXPECT().Create(gomock.Any(), entry).Return(writeErr), - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(storage.ErrAlreadyExists), - m.factStore.EXPECT().Get(gomock.Any(), request.URI, "p").Return(stored, nil), - m.indexStore.EXPECT().Create(gomock.Any(), entry).Return(retryErr), - materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).DoAndReturn(func(_ context.Context, _ storage.Storage, log entity.RequestLog) error { - assert.Equal(t, entity.RequestEventProjectFactsRecorded, log.Event) - return nil - }), - ) - require.ErrorIs(t, c.persistProjectFacts(queueContext(), m.store, request, []projectstatus.Result{{ProjectInfo: projectstatus.ProjectInfo{Name: "p"}, Degree: 0.25}}), writeErr) - require.NoError(t, c.persistProjectFacts(queueContext(), m.store, request, []projectstatus.Result{{ProjectInfo: projectstatus.ProjectInfo{Name: "p"}, Degree: 0.75}})) - snapshot := m.metricsScope.Snapshot() - assert.EqualValues(t, 2, snapshot.Counters()["record_controller.persist_project_facts.start+queue="+testQueue].Value()) - for _, result := range []string{"success", "error"} { - finish, ok := snapshot.Histograms()["record_controller.persist_project_facts.finish+queue="+testQueue+",result="+result] - require.True(t, ok) - assert.EqualValues(t, 1, totalSamples(finish.Durations())) - } - }) - } -} - -func TestProcessRecordsDegreeAfterResolvingProjects(t *testing.T) { +func TestProcessRecordsPersistedProjectCount(t *testing.T) { for _, tt := range []struct { - name string - state entity.RequestState - results []projectstatus.Result - degree float64 + name string + state entity.RequestState + count int32 + degree float64 }{ - {name: "successful repository", state: entity.RequestStateSucceeded, degree: entity.DegreeGreen}, - {name: "failed repository without project results", state: entity.RequestStateFailed, degree: entity.DegreeBroken}, - {name: "failed repository with project results", state: entity.RequestStateFailed, degree: entity.DegreePartial, - results: []projectstatus.Result{{ProjectInfo: projectstatus.ProjectInfo{Name: "a", ProjectType: "service"}, Degree: entity.DegreeGreen}, {ProjectInfo: projectstatus.ProjectInfo{Name: "b", ProjectType: "batch"}, Degree: 0.25}}}, - {name: "successful repository with project results", state: entity.RequestStateSucceeded, degree: entity.DegreeGreen, - results: []projectstatus.Result{{ProjectInfo: projectstatus.ProjectInfo{Name: "a", ProjectType: "service"}, Degree: entity.DegreeGreen}}}, + {name: "successful", state: entity.RequestStateSucceeded, degree: entity.DegreeGreen}, + {name: "failed without results", state: entity.RequestStateFailed, degree: entity.DegreeBroken}, + {name: "failed with results", state: entity.RequestStateFailed, count: 2, degree: entity.DegreePartial}, + {name: "successful with results", state: entity.RequestStateSucceeded, count: 2, degree: entity.DegreeGreen}, } { t.Run(tt.name, func(t *testing.T) { ctrl := gomock.NewController(t) c, m := newController(t, ctrl) - logs := requestlogmock.NewMockMaterializer(ctrl) - logs.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).Return(nil).AnyTimes() - c.materializer = logs - request := requestWithState(tt.state) - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(request, nil) - factory := projectstatusmock.NewMockResolverFactory(ctrl) - resolver := projectstatusmock.NewMockResolver(ctrl) - c.projectStatusResolverFactory = factory - factory.EXPECT().For(projectstatus.ResolverConfig{QueueName: testQueue}).Return(resolver, nil) - resolved := false - resolver.EXPECT().Resolve(gomock.Any(), request).DoAndReturn( - func(context.Context, entity.Request) ([]projectstatus.Result, error) { - resolved = true - return tt.results, nil - }) - facts := map[string]entity.ValidationFact{} - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( - func(_ context.Context, fact entity.ValidationFact) error { - assert.True(t, resolved) - facts[fact.Project] = fact - return nil - }).Times(len(tt.results) + 1) - for _, result := range tt.results { - m.indexStore.EXPECT().Create(gomock.Any(), entity.ProjectFactByDegree{ - URI: testURI, Degree: result.Degree, Project: result.ProjectInfo.Name, - }).Return(nil) - } + m.summary.ProjectCount = tt.count + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(requestWithState(tt.state), nil) + var fact entity.ValidationFact + m.expectFactCreated(&fact) if tt.degree == entity.DegreeGreen { - m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(queueRow(testURI, testID, 1), nil) - m.sourceControl.EXPECT().ChangeInfo(gomock.Any(), testURI).Return(sourcecontrol.ChangeInfo{}, nil) - m.sourceControl.EXPECT().Promote(gomock.Any(), testURI).DoAndReturn(func(context.Context, string) error { - assert.Len(t, facts, len(tt.results)+1) - assert.Empty(t, m.hooks.events) - return nil - }) + m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(queueRow("other", "8", 1), nil) } require.NoError(t, c.Process(queueContext(), delivery(t, ctrl, recordPayload(t, testID)))) - assert.Equal(t, tt.degree, facts[""].Degree) - for _, result := range tt.results { - assert.Equal(t, result.Degree, facts[result.ProjectInfo.Name].Degree) - assert.Equal(t, result.ProjectInfo.ProjectType, facts[result.ProjectInfo.Name].ProjectType) - } - assert.Len(t, m.hooks.events, 1) + require.Equal(t, tt.degree, fact.Degree) + require.Empty(t, fact.Project) + require.Len(t, m.hooks.events, 1) }) } } -func TestProcessRetriesProjectPersistenceWithImmutablePartialFact(t *testing.T) { +func TestProcessReturnsProjectSummaryReadFailure(t *testing.T) { ctrl := gomock.NewController(t) c, m := newController(t, ctrl) - logs := requestlogmock.NewMockMaterializer(ctrl) - logs.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).Return(nil).AnyTimes() - c.materializer = logs - request := requestWithState(entity.RequestStateFailed) - m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(request, nil).Times(2) - factory := projectstatusmock.NewMockResolverFactory(ctrl) - resolver := projectstatusmock.NewMockResolver(ctrl) - c.projectStatusResolverFactory = factory - factory.EXPECT().For(projectstatus.ResolverConfig{QueueName: testQueue}).Return(resolver, nil).Times(2) - resolver.EXPECT().Resolve(gomock.Any(), request).Return( - []projectstatus.Result{{ProjectInfo: projectstatus.ProjectInfo{Name: "a"}, Degree: entity.DegreeBroken}}, nil).Times(2) - var stored entity.ValidationFact - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, fact entity.ValidationFact) error { - assert.Empty(t, fact.Project) - stored = fact - return nil - }) - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(assert.AnError) + m.reqStore.EXPECT().Get(gomock.Any(), testID).Return(failedRequest(), nil) + *m.summaryErr = assert.AnError require.ErrorIs(t, c.Process(queueContext(), delivery(t, ctrl, recordPayload(t, testID))), assert.AnError) - assert.Equal(t, entity.DegreePartial, stored.Degree) - assert.Empty(t, m.hooks.events) - - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(storage.ErrAlreadyExists) - m.factStore.EXPECT().Get(gomock.Any(), testURI, "").Return(stored, nil) - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, fact entity.ValidationFact) error { - assert.Equal(t, "a", fact.Project) - assert.Equal(t, entity.DegreeBroken, fact.Degree) - return nil - }) - m.indexStore.EXPECT().Create(gomock.Any(), entity.ProjectFactByDegree{ - URI: testURI, Degree: entity.DegreeBroken, Project: "a", - }).Return(nil) - require.NoError(t, c.Process(queueContext(), delivery(t, ctrl, recordPayload(t, testID)))) - assert.Len(t, m.hooks.events, 1) -} - -func TestPersistProjectFactsCountsPersistedOutcomes(t *testing.T) { - for _, tt := range []struct { - name string - storedDegree, resolvedDegree float64 - broken string - }{ - {name: "new green", storedDegree: entity.DegreeGreen, resolvedDegree: entity.DegreeGreen, broken: "0"}, - {name: "new partially broken", storedDegree: 0.25, resolvedDegree: 0.25, broken: "1"}, - {name: "new fully broken", storedDegree: entity.DegreeBroken, resolvedDegree: entity.DegreeBroken, broken: "1"}, - {name: "adopts existing green", storedDegree: entity.DegreeGreen, resolvedDegree: entity.DegreeBroken, broken: "0"}, - {name: "adopts existing broken", storedDegree: entity.DegreeBroken, resolvedDegree: entity.DegreeGreen, broken: "1"}, - } { - t.Run(tt.name, func(t *testing.T) { - ctrl := gomock.NewController(t) - c, m := newController(t, ctrl) - request := requestWithState(entity.RequestStateFailed) - stored := entity.ValidationFact{URI: request.URI, Project: "p", Degree: tt.storedDegree, RequestID: request.ID, CreatedAt: 10} - if tt.storedDegree != tt.resolvedDegree { - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(storage.ErrAlreadyExists) - m.factStore.EXPECT().Get(gomock.Any(), request.URI, "p").Return(stored, nil) - } else { - m.factStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - } - m.indexStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) - materializer := requestlogmock.NewMockMaterializer(ctrl) - c.materializer = materializer - materializer.EXPECT().PersistLog(gomock.Any(), m.store, gomock.Any()).DoAndReturn(func(_ context.Context, _ storage.Storage, log entity.RequestLog) error { - assert.Equal(t, map[string]string{ - requestlog.MetadataKeyProjectFactCount: "1", - requestlog.MetadataKeyBrokenProjectFactCount: tt.broken, - }, log.Metadata) - return nil - }) - require.NoError(t, c.persistProjectFacts(queueContext(), m.store, request, []projectstatus.Result{{ProjectInfo: projectstatus.ProjectInfo{Name: "p"}, Degree: tt.resolvedDegree}})) - }) - } + require.Empty(t, m.hooks.events) } diff --git a/stovepipe/core/messagequeue/messagequeue.go b/stovepipe/core/messagequeue/messagequeue.go index d5fae1a8f..a5cebffd9 100644 --- a/stovepipe/core/messagequeue/messagequeue.go +++ b/stovepipe/core/messagequeue/messagequeue.go @@ -62,8 +62,7 @@ type ( // ProjectAnalysis requests project-status resolution and issue attribution. ProjectAnalysis = protopb.ProjectAnalysis - // Record is the payload buildsignal publishes to the record stage once a - // build reaches a terminal status: the build id to record. + // Record identifies a terminal request whose repository outcome can be recorded. Record = protopb.Record ) diff --git a/stovepipe/core/messagequeue/proto/record.proto b/stovepipe/core/messagequeue/proto/record.proto index 7f9713c35..53df67537 100644 --- a/stovepipe/core/messagequeue/proto/record.proto +++ b/stovepipe/core/messagequeue/proto/record.proto @@ -23,12 +23,8 @@ option java_multiple_files = true; option java_outer_classname = "RecordProto"; option java_package = "com.uber.submitqueue.stovepipe.messagequeue"; -// Record is the payload buildsignal publishes once a request's build reaches a -// terminal status: only the request id travels on the queue. record reloads the -// full Request from storage by this id (producer and consumer share the store, -// so the id is enough and redelivery stays idempotent). The build's terminal -// status is projected onto Request.State before this is published, so record -// never reaches a Build. See doc/rfc/stovepipe/steps/buildsignal.md. +// Record identifies a terminal request whose repository outcome can be recorded. +// Project analysis publishes normal outcomes; cancellation bypasses analysis. message Record { option (uber.base.messagequeue.topic_keys) = "record"; diff --git a/stovepipe/core/messagequeue/protopb/record.pb.go b/stovepipe/core/messagequeue/protopb/record.pb.go index 9ad042359..2d6108e59 100644 --- a/stovepipe/core/messagequeue/protopb/record.pb.go +++ b/stovepipe/core/messagequeue/protopb/record.pb.go @@ -14,7 +14,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.36.10 +// protoc-gen-go v1.36.11 // protoc v5.29.3 // source: record.proto @@ -37,12 +37,8 @@ const ( _ = protoimpl.EnforceVersion(protoimpl.MaxVersion - 20) ) -// Record is the payload buildsignal publishes once a request's build reaches a -// terminal status: only the request id travels on the queue. record reloads the -// full Request from storage by this id (producer and consumer share the store, -// so the id is enough and redelivery stays idempotent). The build's terminal -// status is projected onto Request.State before this is published, so record -// never reaches a Build. See doc/rfc/stovepipe/steps/buildsignal.md. +// Record identifies a terminal request whose repository outcome can be recorded. +// Project analysis publishes normal outcomes; cancellation bypasses analysis. type Record struct { state protoimpl.MessageState `protogen:"open.v1"` // id is the request id whose build reached a terminal status. Format: diff --git a/stovepipe/core/messagequeue/topics.go b/stovepipe/core/messagequeue/topics.go index 2823b1c2e..b55cfd87d 100644 --- a/stovepipe/core/messagequeue/topics.go +++ b/stovepipe/core/messagequeue/topics.go @@ -54,10 +54,7 @@ const ( // TopicKeyProjectAnalysis queues requests for project-status resolution and issue attribution. TopicKeyProjectAnalysis TopicKey = "projectanalysis" - // TopicKeyRecord carries a request whose build reached a terminal status from - // buildsignal to the record stage. The buildsignal controller publishes a - // Record (the request id) here once, and only once, a build reaches a - // terminal status; non-terminal polls never publish here. Partitioned by - // request id. + // TopicKeyRecord carries requests after project analysis, or directly after cancellation. + // Partitioned by request ID. TopicKeyRecord TopicKey = "record" ) diff --git a/stovepipe/core/projectanalysis/BUILD.bazel b/stovepipe/core/projectanalysis/BUILD.bazel new file mode 100644 index 000000000..22597fc19 --- /dev/null +++ b/stovepipe/core/projectanalysis/BUILD.bazel @@ -0,0 +1,29 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["publish.go"], + importpath = "github.com/uber/submitqueue/stovepipe/core/projectanalysis", + visibility = ["//visibility:public"], + deps = [ + "//platform/consumer:go_default_library", + "//platform/publish:go_default_library", + "//stovepipe/core/messagequeue:go_default_library", + "//stovepipe/entity:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["publish_test.go"], + embed = [":go_default_library"], + deps = [ + "//platform/base/messagequeue:go_default_library", + "//platform/consumer:go_default_library", + "//platform/extension/messagequeue/mock:go_default_library", + "//stovepipe/core/messagequeue:go_default_library", + "//stovepipe/entity:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + "@org_uber_go_mock//gomock:go_default_library", + ], +) diff --git a/stovepipe/core/projectanalysis/publish.go b/stovepipe/core/projectanalysis/publish.go new file mode 100644 index 000000000..7ee374b9e --- /dev/null +++ b/stovepipe/core/projectanalysis/publish.go @@ -0,0 +1,37 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package projectanalysis publishes terminal requests for project processing. +package projectanalysis + +import ( + "context" + "fmt" + + "github.com/uber/submitqueue/platform/consumer" + "github.com/uber/submitqueue/platform/publish" + stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" + "github.com/uber/submitqueue/stovepipe/entity" +) + +// Publish queues one project-analysis handoff per request. +func Publish(ctx context.Context, registry consumer.TopicRegistry, request entity.Request) error { + payload, err := stovepipemq.Marshal(&stovepipemq.ProjectAnalysis{Id: request.ID, QueueName: request.Queue}) + if err != nil { + return fmt.Errorf("serialize project analysis: %w", err) + } + return publish.Message(ctx, registry, stovepipemq.TopicKeyProjectAnalysis, publish.MessageParams{ + Tenant: request.Queue, ID: publish.IntentID(request.ID), Payload: payload, PartitionKey: request.ID, + }) +} diff --git a/stovepipe/core/projectanalysis/publish_test.go b/stovepipe/core/projectanalysis/publish_test.go new file mode 100644 index 000000000..79a96a995 --- /dev/null +++ b/stovepipe/core/projectanalysis/publish_test.go @@ -0,0 +1,63 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package projectanalysis + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + entityqueue "github.com/uber/submitqueue/platform/base/messagequeue" + "github.com/uber/submitqueue/platform/consumer" + queuemock "github.com/uber/submitqueue/platform/extension/messagequeue/mock" + stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" + "github.com/uber/submitqueue/stovepipe/entity" + "go.uber.org/mock/gomock" +) + +func TestPublishRetriesSameRequest(t *testing.T) { + ctrl := gomock.NewController(t) + queue := queuemock.NewMockQueue(ctrl) + publisher := queuemock.NewMockPublisher(ctrl) + queue.EXPECT().Publisher().Return(publisher).AnyTimes() + registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{ + {Key: stovepipemq.TopicKeyProjectAnalysis, Name: "project-analysis", Queue: queue}, + }) + require.NoError(t, err) + var messages []entityqueue.Message + publisher.EXPECT().Publish(gomock.Any(), "project-analysis", gomock.Any()).DoAndReturn( + func(_ context.Context, _ string, msg entityqueue.Message) error { + messages = append(messages, msg) + if len(messages) == 1 { + return context.DeadlineExceeded + } + return nil + }).Times(3) + request := entity.Request{ID: "42", Queue: "validation/main"} + require.ErrorIs(t, Publish(context.Background(), registry, request), context.DeadlineExceeded) + require.NoError(t, Publish(context.Background(), registry, request)) + request.ID = "43" + require.NoError(t, Publish(context.Background(), registry, request)) + require.Equal(t, messages[0].ID, messages[1].ID) + require.Equal(t, messages[0].Payload, messages[1].Payload) + require.NotEqual(t, messages[1].ID, messages[2].ID) + for _, msg := range messages { + payload := &stovepipemq.ProjectAnalysis{} + require.NoError(t, stovepipemq.Unmarshal(msg.Payload, payload)) + require.Equal(t, payload.GetId(), msg.PartitionKey) + require.Equal(t, payload.GetQueueName(), msg.Tenant) + require.Equal(t, msg.Tenant, msg.Metadata[entityqueue.MetadataKeyQueueName]) + } +} diff --git a/stovepipe/core/requestlog/materializer.go b/stovepipe/core/requestlog/materializer.go index 1034f84f1..4e95299c8 100644 --- a/stovepipe/core/requestlog/materializer.go +++ b/stovepipe/core/requestlog/materializer.go @@ -101,6 +101,27 @@ func NewRequestStateLog(request entity.Request, outcomeReason entity.RequestOutc } } +// NewBuildOutcomeLog requires a build-derived terminal outcome and retains its winning build identity. +func NewBuildOutcomeLog(request entity.Request) (entity.RequestLog, error) { + if request.TerminalBuildID == "" { + return entity.RequestLog{}, fmt.Errorf("request %s has no build outcome", request.ID) + } + var reason entity.RequestOutcomeReason + switch request.State { + case entity.RequestStateSucceeded: + reason = entity.RequestOutcomeReasonBuildSucceeded + case entity.RequestStateFailed: + reason = entity.RequestOutcomeReasonBuildFailed + case entity.RequestStateCancelled: + reason = entity.RequestOutcomeReasonBuildCancelled + default: + return entity.RequestLog{}, fmt.Errorf("request %s has no terminal build state", request.ID) + } + log := NewRequestStateLog(request, reason) + log.Metadata[MetadataKeyBuildID] = request.TerminalBuildID + return log, nil +} + // NewRequestEventLog constructs a stable occurrence for a durable request lifecycle event. func NewRequestEventLog( request entity.Request, @@ -131,10 +152,13 @@ func (m *materializer) PersistLog(ctx context.Context, stores storage.Storage, l return err } - if retained.State == entity.RequestStateUnknown && - retained.Event != entity.RequestEventProjectFactsRecorded && - retained.Event != entity.RequestEventRecordAbandoned { - return nil + if retained.State == entity.RequestStateUnknown { + switch retained.Event { + case entity.RequestEventProjectFactsRecorded, + entity.RequestEventProjectAnalysisCompleted, entity.RequestEventProjectAnalysisAbandoned: + default: + return nil + } } if err := materializeRequestProjections(ctx, stores, retained); err != nil { m.count(ctx, "projection_failure") diff --git a/stovepipe/core/requestlog/materializer_test.go b/stovepipe/core/requestlog/materializer_test.go index 4586a82be..fc2f972a4 100644 --- a/stovepipe/core/requestlog/materializer_test.go +++ b/stovepipe/core/requestlog/materializer_test.go @@ -427,3 +427,33 @@ func TestRepairEventsRemainHistoryOnlyAndDeduplicate(t *testing.T) { }) } } + +func TestBuildOutcomeLogPreservesWinningOutcome(t *testing.T) { + for _, tt := range []struct { + state entity.RequestState + reason entity.RequestOutcomeReason + }{ + {state: entity.RequestStateSucceeded, reason: entity.RequestOutcomeReasonBuildSucceeded}, + {state: entity.RequestStateFailed, reason: entity.RequestOutcomeReasonBuildFailed}, + {state: entity.RequestStateCancelled, reason: entity.RequestOutcomeReasonBuildCancelled}, + } { + t.Run(string(tt.state), func(t *testing.T) { + request := entity.Request{ID: testRequestID, Queue: testQueue, State: tt.state, Version: 3, TerminalBuildID: "winning-build", BuildStrategy: entity.BuildStrategyIncrementalSinceGreen, BaseURI: testBaseURI} + log, err := NewBuildOutcomeLog(request) + require.NoError(t, err) + want := NewRequestStateLog(request, tt.reason) + want.Metadata[MetadataKeyBuildID] = request.TerminalBuildID + require.Equal(t, want, log) + }) + } +} + +func TestBuildOutcomeLogRequiresBuildDerivedOutcome(t *testing.T) { + for _, request := range []entity.Request{ + {ID: testRequestID, State: entity.RequestStateFailed}, + {ID: testRequestID, State: entity.RequestStateProcessing, TerminalBuildID: "build"}, + } { + _, err := NewBuildOutcomeLog(request) + require.Error(t, err) + } +} diff --git a/stovepipe/core/requestlog/request_summary.go b/stovepipe/core/requestlog/request_summary.go index ec6af16a3..b63a23c2e 100644 --- a/stovepipe/core/requestlog/request_summary.go +++ b/stovepipe/core/requestlog/request_summary.go @@ -117,7 +117,10 @@ func projectRequestSummaryFacts(summary entity.RequestSummary, log entity.Reques log.RequestID, summary.RequestID, log.Queue, summary.Queue, ) } - if log.Event == entity.RequestEventProjectFactsRecorded || log.Event == entity.RequestEventRecordAbandoned { + switch log.Event { + case entity.RequestEventProjectFactsRecorded: + return projectRequestSummaryProjectCounts(summary, log) + case entity.RequestEventProjectAnalysisCompleted, entity.RequestEventProjectAnalysisAbandoned: if log.TimestampMs <= 0 { return entity.RequestSummary{}, fmt.Errorf("request summary completion time must be positive request_id=%q", log.RequestID) } @@ -125,9 +128,6 @@ func projectRequestSummaryFacts(summary entity.RequestSummary, log entity.Reques if summary.ProjectResultsCompletedAtMs == 0 { summary.ProjectResultsCompletedAtMs = log.TimestampMs } - if log.Event == entity.RequestEventProjectFactsRecorded { - return projectRequestSummaryProjectCounts(summary, log) - } } if log.State == entity.RequestStateAccepted { if summary.AcceptedAtMs != 0 && summary.AcceptedAtMs != log.TimestampMs { diff --git a/stovepipe/core/requestlog/request_summary_test.go b/stovepipe/core/requestlog/request_summary_test.go index 82535b424..72c273b7b 100644 --- a/stovepipe/core/requestlog/request_summary_test.go +++ b/stovepipe/core/requestlog/request_summary_test.go @@ -519,15 +519,15 @@ func TestMaterializerProjectsProjectCompletion(t *testing.T) { count string projectCount int32 }{ - {name: "completed projects", event: entity.RequestEventProjectFactsRecorded, count: "3", projectCount: 3}, - {name: "completed empty set", event: entity.RequestEventProjectFactsRecorded, count: "0"}, - {name: "duplicate completion", event: entity.RequestEventProjectFactsRecorded, completedAt: testNowMs, count: "3", projectCount: 3}, + {name: "facts recorded while attribution pending", event: entity.RequestEventProjectFactsRecorded, count: "3", projectCount: 3}, + {name: "empty facts while attribution pending", event: entity.RequestEventProjectFactsRecorded, count: "0"}, + {name: "facts replayed after completion", event: entity.RequestEventProjectFactsRecorded, completedAt: testNowMs, count: "3", projectCount: 3}, {name: "recording after abandonment", event: entity.RequestEventProjectFactsRecorded, completedAt: testNowMs - 1, count: "3", projectCount: 3}, {name: "delayed recording after abandonment", event: entity.RequestEventProjectFactsRecorded, completedAt: testNowMs + 1, count: "3", projectCount: 3}, - {name: "abandoned recording", event: entity.RequestEventRecordAbandoned}, - {name: "duplicate abandonment", event: entity.RequestEventRecordAbandoned, completedAt: testNowMs}, - {name: "abandonment after recording", event: entity.RequestEventRecordAbandoned, completedAt: testNowMs - 1}, - {name: "delayed abandonment after recording", event: entity.RequestEventRecordAbandoned, completedAt: testNowMs + 1}, + {name: "project analysis completed", event: entity.RequestEventProjectAnalysisCompleted}, + {name: "project analysis completion replayed", event: entity.RequestEventProjectAnalysisCompleted, completedAt: testNowMs - 1}, + {name: "project analysis abandoned", event: entity.RequestEventProjectAnalysisAbandoned}, + {name: "project analysis abandonment replayed", event: entity.RequestEventProjectAnalysisAbandoned, completedAt: testNowMs - 1}, } { t.Run(tt.name, func(t *testing.T) { materializer, stores, logs, summaries, _ := newTestMaterializer(t) @@ -548,7 +548,7 @@ func TestMaterializerProjectsProjectCompletion(t *testing.T) { current.ProjectCount = 5 current.BrokenProjectCount = 2 want := current - if want.ProjectResultsCompletedAtMs == 0 { + if tt.event != entity.RequestEventProjectFactsRecorded && want.ProjectResultsCompletedAtMs == 0 { want.ProjectResultsCompletedAtMs = log.TimestampMs } if tt.event == entity.RequestEventProjectFactsRecorded { @@ -567,7 +567,7 @@ func TestMaterializerProjectsProjectCompletion(t *testing.T) { } func TestMaterializerRepairsCompletionAfterProjectionFailure(t *testing.T) { - for _, event := range []entity.RequestEvent{entity.RequestEventProjectFactsRecorded, entity.RequestEventRecordAbandoned} { + for _, event := range []entity.RequestEvent{entity.RequestEventProjectFactsRecorded, entity.RequestEventProjectAnalysisCompleted, entity.RequestEventProjectAnalysisAbandoned} { t.Run(string(event), func(t *testing.T) { materializer, stores, logs, summaries, _ := newTestMaterializer(t) log := NewRequestEventLog(entity.Request{ID: testRequestID, Queue: testQueue}, event, "test", map[string]string{MetadataKeyProjectFactCount: "3", MetadataKeyBrokenProjectFactCount: "1"}) @@ -578,10 +578,11 @@ func TestMaterializerRepairsCompletionAfterProjectionFailure(t *testing.T) { current.RequestVersion = 3 current.StateTimestampMs = testNowMs - 1000 updated := current - updated.ProjectResultsCompletedAtMs = retained.TimestampMs if event == entity.RequestEventProjectFactsRecorded { updated.ProjectCount = 3 updated.BrokenProjectCount = 1 + } else { + updated.ProjectResultsCompletedAtMs = retained.TimestampMs } writeErr := errors.New("summary unavailable") gomock.InOrder( @@ -606,7 +607,7 @@ func TestMaterializerRepairsCompletionAfterProjectionFailure(t *testing.T) { } func TestMaterializerRetriesCompletionAfterStateProjectionWins(t *testing.T) { - for _, event := range []entity.RequestEvent{entity.RequestEventProjectFactsRecorded, entity.RequestEventRecordAbandoned} { + for _, event := range []entity.RequestEvent{entity.RequestEventProjectFactsRecorded, entity.RequestEventProjectAnalysisCompleted, entity.RequestEventProjectAnalysisAbandoned} { t.Run(string(event), func(t *testing.T) { materializer, stores, logs, summaries, _ := newTestMaterializer(t) log := NewRequestEventLog(entity.Request{ID: testRequestID, Queue: testQueue}, event, "test", map[string]string{MetadataKeyProjectFactCount: "3", MetadataKeyBrokenProjectFactCount: "1"}) @@ -615,10 +616,11 @@ func TestMaterializerRetriesCompletionAfterStateProjectionWins(t *testing.T) { current.State = entity.RequestStateProcessing current.RequestVersion = 2 updated := current - updated.ProjectResultsCompletedAtMs = log.TimestampMs if event == entity.RequestEventProjectFactsRecorded { updated.ProjectCount = 3 updated.BrokenProjectCount = 1 + } else { + updated.ProjectResultsCompletedAtMs = log.TimestampMs } winner := current winner.State = entity.RequestStateFailed @@ -627,10 +629,11 @@ func TestMaterializerRetriesCompletionAfterStateProjectionWins(t *testing.T) { winner.OutcomeReason = entity.RequestOutcomeReasonBuildFailed winner.Version++ updatedWinner := winner - updatedWinner.ProjectResultsCompletedAtMs = log.TimestampMs if event == entity.RequestEventProjectFactsRecorded { updatedWinner.ProjectCount = 3 updatedWinner.BrokenProjectCount = 1 + } else { + updatedWinner.ProjectResultsCompletedAtMs = log.TimestampMs } gomock.InOrder( logs.EXPECT().Create(gomock.Any(), log).Return(nil), @@ -645,7 +648,7 @@ func TestMaterializerRetriesCompletionAfterStateProjectionWins(t *testing.T) { } func TestMaterializerCompletionRequiresExistingSummary(t *testing.T) { - for _, event := range []entity.RequestEvent{entity.RequestEventProjectFactsRecorded, entity.RequestEventRecordAbandoned} { + for _, event := range []entity.RequestEvent{entity.RequestEventProjectFactsRecorded, entity.RequestEventProjectAnalysisCompleted, entity.RequestEventProjectAnalysisAbandoned} { t.Run(string(event), func(t *testing.T) { materializer, stores, logs, summaries, _ := newTestMaterializer(t) log := NewRequestEventLog(entity.Request{ID: testRequestID, Queue: testQueue}, event, "test", nil) @@ -687,7 +690,6 @@ func TestProjectRequestSummaryProjectCounts(t *testing.T) { } require.NoError(t, err) want := current - want.ProjectResultsCompletedAtMs = testNowMs want.ProjectCount = tt.count want.BrokenProjectCount = tt.broken require.Equal(t, want, got) @@ -698,6 +700,67 @@ func TestProjectRequestSummaryProjectCounts(t *testing.T) { } } +func TestNonCompletionEventsLeaveProjectSummaryUnchanged(t *testing.T) { + for _, event := range []entity.RequestEvent{ + entity.RequestEventProjectIssuesRecorded, + entity.RequestEventRecordAbandoned, + } { + t.Run(string(event), func(t *testing.T) { + materializer, stores, logs, _, _ := newTestMaterializer(t) + log := NewRequestEventLog(entity.Request{ID: testRequestID, Queue: testQueue}, event, "test", nil) + log.TimestampMs = testNowMs + logs.EXPECT().Create(gomock.Any(), log).Return(nil) + require.NoError(t, materializer.PersistLog(context.Background(), stores, log)) + + current := seededRequestSummary() + got, err := projectRequestSummary(current, log) + require.NoError(t, err) + require.Equal(t, current, got) + }) + } +} + +func TestProjectRequestSummaryCompletesAfterProjectFacts(t *testing.T) { + for _, event := range []entity.RequestEvent{ + entity.RequestEventProjectAnalysisCompleted, + entity.RequestEventProjectAnalysisAbandoned, + } { + t.Run(string(event), func(t *testing.T) { + request := entity.Request{ID: testRequestID, Queue: testQueue} + projectFactsRecordedLog := NewRequestEventLog(request, entity.RequestEventProjectFactsRecorded, "all", map[string]string{ + MetadataKeyProjectFactCount: "3", MetadataKeyBrokenProjectFactCount: "1", + }) + projectFactsRecordedLog.TimestampMs = testNowMs + summary, err := projectRequestSummary(seededRequestSummary(), projectFactsRecordedLog) + require.NoError(t, err) + require.Equal(t, int32(3), summary.ProjectCount) + require.Equal(t, int32(1), summary.BrokenProjectCount) + require.Zero(t, summary.ProjectResultsCompletedAtMs) + + projectIssuesRecordedLog := NewRequestEventLog(request, entity.RequestEventProjectIssuesRecorded, "test", nil) + projectIssuesRecordedLog.TimestampMs = testNowMs + 500 + attributed, err := projectRequestSummary(summary, projectIssuesRecordedLog) + require.NoError(t, err) + require.Equal(t, summary, attributed) + + completionLog := NewRequestEventLog(request, event, "test", nil) + completionLog.TimestampMs = testNowMs + 1000 + completed, err := projectRequestSummary(summary, completionLog) + require.NoError(t, err) + want := summary + want.ProjectResultsCompletedAtMs = completionLog.TimestampMs + require.Equal(t, want, completed) + + replayed, err := projectRequestSummary(completed, projectFactsRecordedLog) + require.NoError(t, err) + require.Equal(t, completed, replayed) + replayed, err = projectRequestSummary(completed, completionLog) + require.NoError(t, err) + require.Equal(t, completed, replayed) + }) + } +} + func TestProjectRequestSummaryUsesRetainedCounts(t *testing.T) { current := seededRequestSummary() current.ProjectResultsCompletedAtMs = testNowMs diff --git a/stovepipe/entity/request_log.go b/stovepipe/entity/request_log.go index fe6c8ce26..fce6f852a 100644 --- a/stovepipe/entity/request_log.go +++ b/stovepipe/entity/request_log.go @@ -32,6 +32,8 @@ const ( RequestEventProjectFactsRecorded RequestEvent = "project_facts_recorded" // RequestEventProjectIssuesRecorded records completed project-issue attribution for a request. RequestEventProjectIssuesRecorded RequestEvent = "project_issues_recorded" + // RequestEventProjectAnalysisCompleted records that project status and issue attribution are complete. + RequestEventProjectAnalysisCompleted RequestEvent = "project_analysis_completed" // RequestEventProjectAnalysisAbandoned records unfinished project analysis after primary processing stopped. RequestEventProjectAnalysisAbandoned RequestEvent = "project_analysis_abandoned" // RequestEventRecordAbandoned records that record-stage work stopped after exhausting retries. @@ -158,7 +160,7 @@ func (e RequestLog) validateEvent() error { } switch e.Event { case RequestEventBuildTriggered, RequestEventBuildFinished, RequestEventValidationFactRecorded, - RequestEventProjectFactsRecorded, RequestEventProjectIssuesRecorded, RequestEventProjectAnalysisAbandoned, + RequestEventProjectFactsRecorded, RequestEventProjectIssuesRecorded, RequestEventProjectAnalysisCompleted, RequestEventProjectAnalysisAbandoned, RequestEventRecordAbandoned, RequestEventBuildAnalysisAbandoned, RequestEventBuildAnalysisCompleted, RequestEventIssueTrackingStarted, RequestEventIssuesReconciled, RequestEventIssueTrackingAbandoned, RequestEventFixSubmitted, RequestEventFixSubmissionSkipped, RequestEventFixSubmissionAbandoned: diff --git a/stovepipe/entity/request_log_test.go b/stovepipe/entity/request_log_test.go index 99aa17aa7..480b0da9e 100644 --- a/stovepipe/entity/request_log_test.go +++ b/stovepipe/entity/request_log_test.go @@ -126,6 +126,15 @@ func TestRequestLogValidate(t *testing.T) { return entry }, }, + { + name: "project analysis completed", + mutate: func(entry RequestLog) RequestLog { + entry.State = RequestStateUnknown + entry.Event = RequestEventProjectAnalysisCompleted + entry.RequestVersion = 0 + return entry + }, + }, { name: "project analysis abandoned", mutate: func(entry RequestLog) RequestLog { diff --git a/stovepipe/entity/request_summary.go b/stovepipe/entity/request_summary.go index 451521e6a..660bc2346 100644 --- a/stovepipe/entity/request_summary.go +++ b/stovepipe/entity/request_summary.go @@ -32,7 +32,8 @@ type RequestSummary struct { StateTimestampMs int64 // AcceptedAtMs is the original acceptance timestamp in Unix milliseconds; zero means unknown and a positive value is immutable. AcceptedAtMs int64 - // ProjectResultsCompletedAtMs is the completion timestamp in Unix milliseconds; zero means unknown and a positive value is immutable. + // ProjectResultsCompletedAtMs is when project analysis finished or was abandoned, in Unix milliseconds. + // Zero means unknown and a positive value is immutable. ProjectResultsCompletedAtMs int64 // ProjectCount is the number of recorded project results; zero means none recorded. ProjectCount int32 diff --git a/test/e2e/stovepipe/BUILD.bazel b/test/e2e/stovepipe/BUILD.bazel index 8db007a7c..175933d7c 100644 --- a/test/e2e/stovepipe/BUILD.bazel +++ b/test/e2e/stovepipe/BUILD.bazel @@ -9,6 +9,7 @@ go_test( "list_test.go", "open_issues_test.go", "project_issues_test.go", + "projectanalysis_test.go", "queue_status_test.go", "request_issues_test.go", "suite_test.go", @@ -33,6 +34,7 @@ go_test( "//platform/publish:go_default_library", "//stovepipe/core/issuelog:go_default_library", "//stovepipe/core/messagequeue:go_default_library", + "//stovepipe/core/projectanalysis:go_default_library", "//stovepipe/core/projectissues:go_default_library", "//stovepipe/core/queuelog:go_default_library", "//stovepipe/core/queuestate:go_default_library", diff --git a/test/e2e/stovepipe/project_issues_test.go b/test/e2e/stovepipe/project_issues_test.go index f4bbd90ac..de75d79e3 100644 --- a/test/e2e/stovepipe/project_issues_test.go +++ b/test/e2e/stovepipe/project_issues_test.go @@ -31,8 +31,10 @@ func (s *StovepipeE2ESuite) TestProjectIssues() { t := s.T() stores, err := s.appStorage.For(projectIssuesQueue) require.NoError(t, err) - request := entity.Request{ID: "900", Queue: projectIssuesQueue, TerminalBuildID: "build/900"} + request := entity.Request{ID: "900", Queue: projectIssuesQueue, URI: "git://repository/main/project-issues", State: entity.RequestStateFailed, Version: 1, TerminalBuildID: "build/900"} + require.NoError(t, stores.GetRequestStore().Create(s.ctx, request)) history := requestlog.NewMaterializer(tally.NoopScope) + require.NoError(t, history.PersistLog(s.ctx, stores, requestlog.NewRequestStateLog(request, entity.RequestOutcomeReasonBuildFailed))) metadata := map[string]string{requestlog.MetadataKeyBuildID: request.TerminalBuildID} require.NoError(t, history.PersistLog(s.ctx, stores, requestlog.NewRequestEventLog(request, entity.RequestEventBuildAnalysisCompleted, request.TerminalBuildID, metadata))) materializer := projectissuescore.NewMaterializer(tally.NoopScope) diff --git a/test/e2e/stovepipe/projectanalysis_test.go b/test/e2e/stovepipe/projectanalysis_test.go new file mode 100644 index 000000000..e3cd3e464 --- /dev/null +++ b/test/e2e/stovepipe/projectanalysis_test.go @@ -0,0 +1,117 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package e2e_test + +import ( + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + pb "github.com/uber/submitqueue/api/stovepipe/protopb" + "github.com/uber/submitqueue/platform/consumer" + queuemysql "github.com/uber/submitqueue/platform/extension/messagequeue/mysql" + stovepipemq "github.com/uber/submitqueue/stovepipe/core/messagequeue" + "github.com/uber/submitqueue/stovepipe/core/projectanalysis" + "github.com/uber/submitqueue/stovepipe/core/requestlog" + "github.com/uber/submitqueue/stovepipe/entity" + "github.com/uber/submitqueue/stovepipe/extension/storage" + "go.uber.org/zap" +) + +const ( + projectAnalysisQueue = "e2e-stovepipe/project-analysis" + projectAnalysisDLQQueue = "e2e-stovepipe/project-analysis-dlq" +) + +func (s *StovepipeE2ESuite) TestProjectAnalysisRecordsBeforeRepository() { + t := s.T() + id := s.ingest(projectAnalysisQueue) + stores, err := s.appStorage.For(projectAnalysisQueue) + require.NoError(t, err) + request, err := stores.GetRequestStore().Get(s.ctx, id) + require.NoError(t, err) + s.awaitRepositoryFact(stores, request.URI) + logs, err := stores.GetRequestLogStore().List(s.ctx, id) + require.NoError(t, err) + events := map[entity.RequestEvent]bool{} + var projectAnalysisCompletedAtMs int64 + for _, log := range logs { + events[log.Event] = true + if log.Event == entity.RequestEventProjectAnalysisCompleted { + projectAnalysisCompletedAtMs = log.TimestampMs + } + } + for _, event := range []entity.RequestEvent{entity.RequestEventBuildAnalysisCompleted, entity.RequestEventProjectFactsRecorded, entity.RequestEventProjectIssuesRecorded, entity.RequestEventProjectAnalysisCompleted, entity.RequestEventValidationFactRecorded} { + require.True(t, events[event], "missing event %s", event) + } + require.False(t, events[entity.RequestEventProjectAnalysisAbandoned]) + summary, err := stores.GetRequestSummaryStore().Get(s.ctx, id) + require.NoError(t, err) + require.Positive(t, summary.ProjectResultsCompletedAtMs) + require.Equal(t, projectAnalysisCompletedAtMs, summary.ProjectResultsCompletedAtMs) + result, err := s.client.ListRequestIssuesByProject(s.ctx, &pb.ListRequestIssuesByProjectRequest{Queue: projectAnalysisQueue, RequestId: id, Project: "project"}) + require.NoError(t, err) + require.True(t, result.GetAttributionComplete()) + require.Empty(t, result.GetAttributions()) +} + +func (s *StovepipeE2ESuite) TestProjectAnalysisDLQStillRecordsRepository() { + t := s.T() + stores, err := s.appStorage.For(projectAnalysisDLQQueue) + require.NoError(t, err) + request := entity.Request{ID: "900", Queue: projectAnalysisDLQQueue, URI: "git://repository/project-analysis/failure", State: entity.RequestStateFailed, Version: 1} + require.NoError(t, stores.GetRequestStore().Create(s.ctx, request)) + history := requestlog.NewMaterializer(tally.NoopScope) + require.NoError(t, history.PersistLog(s.ctx, stores, requestlog.NewRequestStateLog(request, entity.RequestOutcomeReasonProcessingFailed))) + mq, err := queuemysql.NewQueue(queuemysql.Params{DB: s.queueDB, Logger: zap.NewNop(), MetricsScope: tally.NoopScope, Tenants: []string{request.Queue}}) + require.NoError(t, err) + defer func() { require.NoError(t, mq.Close()) }() + registry, err := consumer.NewTopicRegistry([]consumer.TopicConfig{{Key: stovepipemq.TopicKeyProjectAnalysis, Name: "projectanalysis", Queue: mq}}) + require.NoError(t, err) + require.NoError(t, projectanalysis.Publish(s.ctx, registry, request)) + fact := s.awaitRepositoryFact(stores, request.URI) + require.Equal(t, entity.DegreeBroken, fact.Degree) + summary, err := stores.GetRequestSummaryStore().Get(s.ctx, request.ID) + require.NoError(t, err) + require.Positive(t, summary.ProjectResultsCompletedAtMs) + log := requestlog.NewRequestEventLog(request, entity.RequestEventProjectAnalysisAbandoned, "projects", nil) + retained, err := stores.GetRequestLogStore().Get(s.ctx, request.ID, log.ID) + require.NoError(t, err) + require.Equal(t, retained.TimestampMs, summary.ProjectResultsCompletedAtMs) + result, err := s.client.ListRequestIssuesByProject(s.ctx, &pb.ListRequestIssuesByProjectRequest{Queue: request.Queue, RequestId: request.ID, Project: "project"}) + require.NoError(t, err) + require.False(t, result.GetAttributionComplete()) + require.Empty(t, result.GetAttributions()) +} + +func (s *StovepipeE2ESuite) awaitRepositoryFact(stores storage.Storage, uri string) entity.ValidationFact { + s.T().Helper() + var fact entity.ValidationFact + pollUntil(convergencePollInterval, func() bool { + var err error + fact, err = stores.GetValidationFactStore().Get(s.ctx, uri, "") + if storage.IsNotFound(err) { + return false + } + require.NoError(s.T(), err) + logs, err := stores.GetRequestLogStore().List(s.ctx, fact.RequestID) + require.NoError(s.T(), err) + for _, log := range logs { + if log.Event == entity.RequestEventValidationFactRecorded { + return true + } + } + return false + }) + return fact +} diff --git a/test/e2e/stovepipe/suite_test.go b/test/e2e/stovepipe/suite_test.go index c2b54d0c8..09f8aa8bc 100644 --- a/test/e2e/stovepipe/suite_test.go +++ b/test/e2e/stovepipe/suite_test.go @@ -65,7 +65,7 @@ func (s *StovepipeE2ESuite) SetupSuite() { t.Setenv("MQ_TENANTS", strings.Join([]string{ "monorepo/main", "monorepo/release", "monorepo/slow?buildrunner-fake=build-slow", listPaginationQueue, listTimeBoundsQueue, listPrimaryQueue, listOtherQueue, listValidationQueue, queueStateTestQueue, - issueTrackingQueue, issueTrackingDLQQueue, openIssuesQueue, requestIssuesQueue, projectIssuesQueue, + issueTrackingQueue, issueTrackingDLQQueue, openIssuesQueue, requestIssuesQueue, projectIssuesQueue, projectAnalysisQueue, projectAnalysisDLQQueue, }, ",")) s.log.Logf("Starting Stovepipe e2e test suite using docker-compose")