From 834ee0db53956ac7cd7fa523e2eeb1abe0151745 Mon Sep 17 00:00:00 2001 From: sergeyb Date: Tue, 1 Sep 2026 21:20:58 +0000 Subject: [PATCH] docs: align architecture guides with implementation Summary: Intent: - Keep the repository's canonical architecture guidance consistent with the current service and pipeline boundaries. - Distinguish durable snapshots, request-log projections, and queue wire formats accurately. Changes: - Updated AGENTS.md and domain overviews for current controller, storage, queue, and Stovepipe behavior. - Corrected Gateway and Orchestrator lifecycle documentation and refreshed extension-specific guides. --- Generated by the pr-create skill in devexp-agent-marketplace. --- AGENTS.md | 57 +++++---- README.md | 2 +- submitqueue/README.md | 10 +- submitqueue/core/changeset/README.md | 10 +- submitqueue/extension/conflict/README.md | 43 ++----- submitqueue/extension/storage/README.md | 6 +- submitqueue/gateway/README.md | 16 +-- submitqueue/orchestrator/README.md | 18 +-- submitqueue/orchestrator/controller/README.md | 113 ++++++------------ 9 files changed, 109 insertions(+), 166 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ab8d48e54..f95bf9bbc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,17 +4,17 @@ SubmitQueue is a distributed system for managing code submission workflows. It follows clean architecture with interface-driven extensibility. -**Immutability and Eventual Consistency:** +**Versioned State and Eventual Consistency:** -1. **Immutable entities** — once created, don't modify in place. Create new versions with updated fields. +1. **Immutable values and versioned snapshots** — do not mutate caller-owned values or pre-update in-memory state. Lifecycle entities are persisted as complete replacement snapshots under optimistic locking; records whose identity is immutable (for example mappings and append-only logs) are created rather than updated. 2. **Eventual consistency** — handle stale reads, idempotent operations, and convergence over time. -3. **Event sourcing** — store events (what happened) rather than just current state for critical changes. -4. **Optimistic locking** — use version numbers instead of pessimistic locks. Avoid transactions; prefer optimistic concurrency and retries. **Version arithmetic lives in the controller, not the storage layer.** Update methods take both `oldVersion` (the where-clause guard) and `newVersion` (the value to write); the store performs a pure conditional write. Controllers compute `newVersion = oldVersion + 1`, call the store, and only assign `entity.Version = newVersion` after the call succeeds. Pre-incrementing in memory before the call is a bug pattern — on error the in-memory version drifts ahead of the database. See [submitqueue/extension/storage/README.md](submitqueue/extension/storage/README.md). +3. **Selective event sourcing** — request history and customer-facing request status are represented by append-only log events plus materialized projections. Other orchestration entities retain versioned current-state snapshots. +4. **Optimistic locking** — use version numbers instead of pessimistic locks. Avoid transactions across entities or distributed boundaries; prefer optimistic concurrency and retries. A storage backend may still use a local transaction to implement one backend operation atomically. **Version arithmetic lives in the controller, not the storage layer.** Update methods take both `oldVersion` (the where-clause guard) and `newVersion` (the value to write); the store performs a pure conditional write. Controllers compute `newVersion = oldVersion + 1`, call the store, and only assign `entity.Version = newVersion` after the call succeeds. Pre-incrementing in memory before the call is a bug pattern — on error the in-memory version drifts ahead of the database. See [submitqueue/extension/storage/README.md](submitqueue/extension/storage/README.md). 5. **Idempotency keys** — include unique request IDs, check for duplicates before executing. 6. **Persist before you publish** — when a message hands work to a later stage, write the state first and publish only once the write has succeeded. Publishing first races the consumer against the write: it can load an entity that was never recorded, or a version older than the one the message describes, and then build on an assumption that was never true. Ordered write-then-publish, a failed publish leaves the state durable and the retry re-publishes; the reverse leaves a message describing a state nothing wrote. The exception is a message that depends on nothing the write does — a status log recording a transition, or an idempotent nudge whose consumer re-derives everything from current state. Those publish first on purpose, so that a failure leaves nothing written and the retry redoes the whole step rather than stranding a durable state change with no way to announce it; see `submitqueue/orchestrator/controller/speculate/finalize.go` and `.../buildsignal/buildsignal.go` for the reasoning at those two sites. ```go -// Immutable entity pattern +// Versioned snapshot pattern type Request struct { ID string Version int // For optimistic locking @@ -50,10 +50,13 @@ submitqueue/ # repo root (Go module github.com/uber/submi │ ├── gateway/ # Gateway service (port 8081) - entry point │ ├── orchestrator/ # Orchestrator service (port 8082) - coordinates jobs │ ├── entity/ # SubmitQueue-specific domain entities -│ ├── extension/ # SubmitQueue-specific extension impls (storage, counter, mergechecker, …) -│ └── core/ # SubmitQueue-internal shared infra (consumer wiring, request, topickey, …) -├── stovepipe/ # Stovepipe domain (single Ping-only service for now) -│ └── controller/ # Business logic (currently just Ping); entity/extension/core added as it grows +│ ├── extension/ # SubmitQueue-specific extension contracts and implementations +│ └── core/ # SubmitQueue-internal shared infra (batch, changeset, request, topickey) +├── stovepipe/ # Stovepipe domain (single service) +│ ├── controller/ # RPC and queue-stage business logic +│ ├── entity/ # Stovepipe domain entities +│ ├── extension/ # Stovepipe-specific extension contracts and implementations +│ └── core/ # Stovepipe-internal queue contracts and shared infrastructure ├── runway/ # Runway domain (single service — the domain *is* the service) │ └── controller/ # Runway service controllers (consumes the merge queues; no gateway/orchestrator split) ├── tool/ # Development and CI tooling @@ -68,7 +71,7 @@ submitqueue/ # repo root (Go module github.com/uber/submi └── doc/ # Documentation ``` -The `platform/` tree holds code reused across domains (infrastructure, shared entities, shared extension contracts). A multi-service **domain** (e.g. `submitqueue/`) keeps the same internal layout (`gateway/`, `orchestrator/`, `entity/`, `extension/`, `core/`); a domain's own `core/` (e.g. `submitqueue/core/`) holds infra shared only between that domain's services. A **single-service domain** collapses that split — the domain *is* the service, so its controllers live directly under the domain root (e.g. `runway/controller/`, `stovepipe/controller/`) with no `gateway/`/`orchestrator/` segment, and its wire contract is service-segment-free (`api/{domain}/`). `runway` is a consumer-only landing service with no gateway; `stovepipe` is currently a single Ping-only service that can grow the other layers (`entity/`, `extension/`, `core/`) as it gains real behavior. +The `platform/` tree holds code reused across domains (infrastructure, shared entities, shared extension contracts). A multi-service **domain** (e.g. `submitqueue/`) keeps the same internal layout (`gateway/`, `orchestrator/`, `entity/`, `extension/`, `core/`); a domain's own `core/` (e.g. `submitqueue/core/`) holds infra shared only between that domain's services. A **single-service domain** collapses that split — the domain *is* the service, so its controllers live directly under the domain root (e.g. `runway/controller/`, `stovepipe/controller/`) with no `gateway/`/`orchestrator/` segment, and its wire contract is service-segment-free (`api/{domain}/`). `runway` is a consumer-only landing service with no gateway. `stovepipe` exposes ingestion RPC behavior and runs its own process, build, build-signal, record, hook, and DLQ queue stages. The `api/` tree holds **published** wire contracts — those depended on from outside the owning domain. RPC contracts live at `api/{domain}/{service}/` (`proto/` for `.proto` sources, `protopb/` for committed generated Go); for a single-service domain the service segment is dropped, so the contract lives directly at `api/{domain}/` (e.g. `api/runway/{proto,protopb}/`). A service package may hold multiple `.proto` files, all generating into the same `protopb/`. External message-queue contracts live at `api/{domain}/messagequeue/` (see Message Queue Contracts below). Internal queue contracts do **not** go here — they live under `{domain}/core/messagequeue/`. @@ -83,7 +86,7 @@ Each service follows the same layout: ``` / -└── controller/ # Business logic (pure, transport-agnostic) +└── controller/ # Transport-agnostic business logic over injected interfaces ├── {method}.go # RPC controllers (e.g., land.go, ping.go) ├── {method}_test.go └── {step}/ # Queue message controllers (e.g., request/) @@ -95,22 +98,22 @@ Wire contracts for a service live separately under `api/{domain}/{service}/` (se ### Controllers -Two types, both containing pure business logic independent of infrastructure: +Two types, both containing transport-agnostic business logic. Controllers depend on behavioral interfaces for storage, queues, providers, and other infrastructure; service wiring supplies concrete implementations. -**RPC Controllers** — in `{service}/controller/`, accept protobuf types: +**RPC Controllers** — in `{service}/controller/`, normally accept and return domain entities after the service wiring maps the wire request. Legacy health-check controllers may still accept protobuf types directly: ```go -func (c *LandController) Land(ctx context.Context, req *pb.LandRequest) (*pb.LandResponse, error) +func (c *LandController) Land(ctx context.Context, req entity.LandRequest) (entity.LandResult, error) ``` **Queue Message Controllers** — in `{service}/controller/{step}/`, implement `consumer.Controller`: ```go -// Return nil to ack, error to nack. Consumer handles ack/nack automatically. +// Return nil to ack or an error to nack/reject. Call Hold and return nil to postpone. func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) error ``` -Controllers receive `consumer.Delivery` (subset interface without Ack/Nack) to enforce separation of business logic from infrastructure. +Controllers receive `consumer.Delivery` (a subset interface without Ack/Nack) to enforce separation of business logic from queue mechanics. `delivery.Hold(delayMs)` requests delayed redelivery without consuming retry budget; the controller must then return `nil`. -**Queue payloads: IDs within a boundary, full payloads across one.** When producer and consumer share a store (same service — e.g. `build`→`buildsignal`, `validate`→`mergeconflict`), put only the entity **ID** on the queue and reload from storage (the store is the source of truth, messages stay small, redelivery is idempotent). Reloading from storage is what makes the publish ordering load-bearing — see "Persist before you publish" above. When a queue **crosses a service boundary** (the consumer cannot read the producer's store — e.g. orchestrator→runway), publish the **full payload** the consumer needs, and have the **client own the correlation ID** so it can match the async result back to the work it is tracking. The queue's **owner defines the wire contract and topic keys** (in its own domain package); the other side imports them. +**Queue payloads: IDs within a boundary, full payloads across one.** When producer and consumer share a store (for example SubmitQueue's `build`→`buildsignal` flow), put only the entity **ID** on the queue and reload from storage (the store is the source of truth, messages stay small, redelivery is idempotent). Reloading from storage is what makes the publish ordering load-bearing — see "Persist before you publish" above. When a queue **crosses a service boundary** (for example SubmitQueue validation or merge handing work to Runway), publish the **full payload** the consumer needs, and have the **client own the correlation ID** so it can match the asynchronous result back to the work it is tracking. The queue's **owner defines the wire contract and topic keys** (in its own domain package); the other side imports them. ### Entities @@ -153,7 +156,7 @@ The cost of "callers loop over a small batch" is usually negligible. The cost of When in doubt, ask: *"If the next implementation were DynamoDB / Kafka / Bigtable / a remote RPC service / an in-memory map, could it satisfy this signature without contortion?"* If the answer is no, simplify the contract. -**Input contract — identity in, resolve internally.** A decision/action extension takes the orchestrator's thin reference entity at its pipeline-stage granularity — `entity.Request` (request stage) or `entity.Batch` / `[]entity.Batch` (batch stage) — never controller-pre-resolved data. It resolves the granular content it needs (changes, diffs, targets) through dependencies injected at its `Factory` (e.g. a request store, a change provider), not a global aggregator. Stores (`storage`, `changestore`) and config (`queueconfig`) are the exception — they are the resolution *targets* and stay key/value-shaped per the rule above. `conflict.Analyzer` is the reference shape; every new extension or signature change must follow it. See [doc/rfc/submitqueue/extension-contract.md](doc/rfc/submitqueue/extension-contract.md). +**Input contract — identity in, resolve internally.** A decision/action extension takes the orchestrator's thin reference entity at its pipeline-stage granularity — `entity.Request` (request stage) or `entity.Batch` / `[]entity.Batch` (batch stage) — never controller-pre-resolved data. It resolves the granular content it needs (changes, diffs, targets) through dependencies injected when the implementation is constructed (for example a `changeset.Resolver` or change provider), not through a global aggregator. Storage and configuration extensions are the resolution targets and retain key-oriented contracts. `conflict.Analyzer` is the reference shape; every new extension or signature change must follow it. ### Import Paths @@ -165,7 +168,7 @@ Paths follow the directory layout: shared packages live under `platform/` at the - Queue contracts: external `github.com/uber/submitqueue/api/{domain}/messagequeue`; internal `github.com/uber/submitqueue/{domain}/core/messagequeue` - Domain entities: `github.com/uber/submitqueue/{domain}/entity` (e.g. `.../submitqueue/entity`) - Domain extensions: `github.com/uber/submitqueue/{domain}/extension/{ext}[/{impl}]` (e.g. `.../submitqueue/extension/storage/mysql`) -- Cross-domain consumer framework: `github.com/uber/submitqueue/platform/consumer`; internal pipeline topic keys: `github.com/uber/submitqueue/{domain}/core/topickey` (external queue topic keys live with their contract package, e.g. `api/runway/messagequeue`) +- Cross-domain consumer framework: `github.com/uber/submitqueue/platform/consumer`; internal topic keys live with the owning domain contract (for example `submitqueue/core/topickey` and `stovepipe/core/messagequeue`); external queue topic keys live with their published contract (for example `api/runway/messagequeue`) - Domain-internal infra: `github.com/uber/submitqueue/{domain}/core/{pkg}` (e.g. `.../submitqueue/core/request`) - Shared entities: `github.com/uber/submitqueue/platform/base/{pkg}` (e.g. `.../platform/base/messagequeue`) - Shared extensions: `github.com/uber/submitqueue/platform/extension/{ext}[/{impl}]` (e.g. `.../platform/extension/messagequeue/mysql`) @@ -189,13 +192,15 @@ Generated proto files are committed. When modifying `.proto` files: 2. `make proto` (generates `*.pb.go`, `*_grpc.pb.go`, `*.pb.yarpc.go` into `api/{domain}/{service}/protopb/`) 3. Commit all generated files -To add a new `.proto` to a service, drop it in the service's `api/{domain}/{service}/proto/` dir, add it to that package's `srcs` in `api/{domain}/{service}/proto/BUILD.bazel` and its `exports_files`, then `make proto && make gazelle`. The codegen and `make proto` copy loop already handle multiple `.proto` files per package. +To add a new `.proto` to a service, drop it in the service's `api/{domain}/{service}/proto/` directory, export it from that directory's `BUILD.bazel`, and add its label to the corresponding `go_proto_generated_files` target in `tool/proto/BUILD.bazel`. Then run `make proto && make gazelle`. The codegen and `make proto` copy loop handle multiple `.proto` files per package. ### Message Queue Contracts -Queue payloads are defined in **proto3** (`.proto` under `proto/`, generated Go in `protopb/` as the binding) and serialized as **protobuf JSON** (protojson) so the queue keeps storing self-describing JSON. Location follows audience: external/cross-domain contracts go under `api/{domain}/messagequeue/`; internal contracts (used only within the owning domain) go under `{domain}/core/messagequeue/`. Bazel `visibility` enforces the split — internal targets are domain-scoped, `api/` targets are public. See [doc/rfc/messagequeue-contract.md](doc/rfc/messagequeue-contract.md). +New queue contracts are defined in **proto3** (`.proto` under `proto/`, generated Go in `protopb/` as the binding) and serialized as **protobuf JSON** (protojson) so the queue keeps storing self-describing JSON. Location follows audience: external/cross-domain contracts go under `api/{domain}/messagequeue/`; internal contracts (used only within the owning domain) go under `{domain}/core/messagequeue/`. Bazel `visibility` enforces the split — internal targets are domain-scoped, `api/` targets are public. -The message types are generated; the contract package adds only generic `protojson` glue — `Marshal(m)` / `Unmarshal[T](b, m)` — owning the wire conventions: `UseProtoNames` (snake_case fields), UPPER_SNAKE enum values, int64-as-string, unknown fields discarded on read (additive evolution). The topic key(s) carrying a message are declared on the message via the `topic_keys` proto option — a `google.protobuf.MessageOptions` extension defined in `api/base/messagequeue`. A topic key is a stable logical name, not a concrete wire topic; each implementer maps it to its backend's topic name, and a `TopicKeys(msg)` reflection helper reads the option back. It is contract metadata, not the hot path — publish/consume still routes on `consumer.TopicKey` + `TopicRegistry`. The contract package owns both halves: the proto payload and the `TopicKey` constants for its topic keys. A contract test round-trips the payloads and asserts every topic key is bound to exactly one message. Shared field types (`Change`, `Strategy`) are shared protos under `api/base/{change,mergestrategy}`. `api/runway/messagequeue/` is the reference example. +For proto-backed contracts, the message types are generated and the contract package adds generic `protojson` glue — `Marshal(m)` / `Unmarshal[T](b, m)` — owning the wire conventions: `UseProtoNames` (snake_case fields), UPPER_SNAKE enum values, int64-as-string, and unknown fields discarded on read (additive evolution). The topic key(s) carrying a message are declared on the message via the `topic_keys` proto option — a `google.protobuf.MessageOptions` extension defined in `api/base/messagequeue`. A topic key is a stable logical name, not a concrete wire topic; each implementer maps it to its backend's topic name, and a `TopicKeys(msg)` reflection helper reads the option back. It is contract metadata, not the hot path — publish/consume still routes on `consumer.TopicKey` + `TopicRegistry`. The contract package owns both halves: the proto payload and the `TopicKey` constants for its topic keys. A contract test round-trips the payloads and asserts every topic key is bound to exactly one message. Shared field types (`Change`, `Strategy`) are shared protos under `api/base/{change,mergestrategy}`. `api/runway/messagequeue/` and `stovepipe/core/messagequeue/` are current examples. + +SubmitQueue's internal pipeline predates the proto-backed convention. It continues to serialize domain entities with `encoding/json` and declares its logical keys in `submitqueue/core/topickey/`. Do not convert or mix these wire formats incidentally; treat migration as an explicit compatibility change. ### Naming Conventions @@ -247,10 +252,10 @@ make clean # Clean Bazel cache **Add new queue message controller:** 1. Create `{domain}/{service}/controller/{step}/` implementing `consumer.Controller` -2. Wire up in `service/{domain}/{service}/server/main.go` +2. Add it to the owning service's pipeline topology when one exists (for example `submitqueue/orchestrator/pipeline.go`); otherwise wire it in `service/{domain}/{service}/server/main.go` **Add new extension:** -1. Create the extension under `{domain}/extension/{ext}/{impl}/` (domain-specific, e.g. `submitqueue/extension/...`) or `platform/extension/{ext}/{impl}/` (shared across domains) with factory and interfaces +1. Define the interface, config, and `Factory` interface under `{domain}/extension/{ext}/` or `platform/extension/{ext}/`, and put implementation constructors under the `{impl}/` subdirectory. Keep concrete factory adapters and per-queue routing in service wiring. 2. Add `BUILD.bazel`, tests, and README.md **Add new entity:** @@ -312,7 +317,7 @@ deps = [ **Integration tests** use Docker Compose via `testutil.ComposeStack`: - Package naming: folder name as package (NOT `*_test` suffix) - Bazel: add `tags = ["integration", "requires-network"]` and `data = [...]` for every input the test reads (compose file, schema dirs, and each service's `docker_test_context` filegroup bundling its Dockerfile, configs, and Bazel-built `*_linux` binary). Tests are hermetic: never resolve the repo root — resolve inputs from runfiles via `testutil.Runfile` and stage docker build contexts with `testutil.WithBuildContext`. -- Use `testutil.NewComposeStack()` with meaningful context (e.g., `"ext-storage-mysql"`) +- Use `testutil.NewComposeStack()` with meaningful context (e.g., `"ext-submitqueue-storage-mysql"`) See [doc/howto/TESTING.md](doc/howto/TESTING.md) for full testing guide. @@ -331,7 +336,7 @@ CI runs on every PR and enforces all checks via a `required-checks` gate. **Befo ### Code Style -1. **Structured logging** — `zap.SugaredLogger` with `Debugw`/`Infow`/`Errorw(msg, key, val, ...)`. Never unstructured methods. +1. **Structured logging** — use structured Zap APIs: `zap.SugaredLogger` with `Debugw`/`Infow`/`Errorw(msg, key, val, ...)`, or core `zap.Logger` methods with typed fields. Never use unstructured formatted logging methods. 2. **Interfaces for behavior, structs for data** — use interfaces for behavioral contracts (Consumer, Controller, Storage). Use structs for data containers, configs, and registries (TopicRegistry, SubscriptionConfig). 3. **Value types over pointers** — prefer value types for structs, configs, and return values. Use `(T, bool)` to signal absence instead of `*T`. Pointers only when mutation or shared ownership is needed. 4. **Errors for failures, not control flow** — reserve `error` returns for unexpected or infrastructure failures. Use result types (structs, bools) for expected outcomes like `(Result, error)` or `(T, bool)`. Avoid sentinel errors that represent non-failure states. diff --git a/README.md b/README.md index 37791cca0..6c7fb86a7 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ Designed for large monorepos and fast-moving teams where concurrent changes can ## Repository layout -Cross-domain Go code (errors, metrics, consumer framework, HTTP helpers, shared entities, shared extension contracts) lives under [`platform/`](platform/README.md). Each product domain has its own tree (`submitqueue/`, `stovepipe/`, …) and grows into `gateway/`, `orchestrator/`, `entity/`, `extension/`, and domain-local `core/` — though a domain may start smaller (Stovepipe is currently a single Ping-only service with just `controller/`). See [AGENTS.md](AGENTS.md) for conventions and import paths. +Cross-domain Go code (errors, metrics, consumer framework, HTTP helpers, shared entities, shared extension contracts) lives under [`platform/`](platform/README.md). Each product domain has its own tree (`submitqueue/`, `stovepipe/`, …). Multi-service domains may split into `gateway/` and `orchestrator/`; single-service domains keep controllers directly under the domain root. Stovepipe currently exposes ingestion behavior and runs its own queue pipeline. See [AGENTS.md](AGENTS.md) for conventions and import paths. ## Quick Start diff --git a/submitqueue/README.md b/submitqueue/README.md index 11bcf3b38..d6cb7ca2f 100644 --- a/submitqueue/README.md +++ b/submitqueue/README.md @@ -2,10 +2,10 @@ SubmitQueue service layout: -- `gateway/` — Gateway service: entry point for land requests (`Ping`, `Land`, `Cancel` RPCs). -- `orchestrator/` — Orchestrator service: coordinates the land pipeline (batch, speculate, build, merge, conclude, ...). -- `extension/` — SubmitQueue-specific extension implementations (storage, counter, changestore, mergechecker, pusher, scorer, conflict, queueconfig, buildrunner, ...). +- `gateway/` — Gateway service: entry point for `Ping`, `Land`, `Cancel`, request-summary, request-history, and queue-listing RPCs. It also consumes request-log events and maintains the public request projections. +- `orchestrator/` — Orchestrator service: coordinates validation, dependency analysis, speculation, builds, landing, cancellation, conclusion, hooks, and DLQ reconciliation. +- `extension/` — SubmitQueue-specific extension contracts and implementations, including storage, queue configuration, change providers, validation, conflict analysis, speculation, and build runners. - `entity/` — SubmitQueue-specific domain entities. -- `core/` — Infrastructure shared across SubmitQueue's own services (gateway and orchestrator): the queue `consumer` framework, the `request` lifecycle, and topic keys. The SubmitQueue-scoped analogue of the repo-level `platform/`. +- `core/` — Infrastructure shared across SubmitQueue's own services, including request and batch lifecycle helpers, change-set resolution, and internal topic keys. -Cross-domain building blocks live outside this directory: shared entities in `platform/base/`, shared extensions in `platform/extension/`, and cross-domain infrastructure in `platform/`. +Cross-domain building blocks live outside this directory: shared entities in `platform/base/`, shared extensions in `platform/extension/`, and cross-domain infrastructure such as the consumer framework in `platform/`. diff --git a/submitqueue/core/changeset/README.md b/submitqueue/core/changeset/README.md index 253374434..4e8c9463c 100644 --- a/submitqueue/core/changeset/README.md +++ b/submitqueue/core/changeset/README.md @@ -1,17 +1,17 @@ # changeset -`changeset` resolves batch identity into the changes a batch contains. It is the single place the orchestrator walks batch → requests → changes, consolidating the resolution the build and merge controllers each performed privately. +`changeset` resolves batch identity into the changes a batch contains. Current consumers include build-runner and scorer implementations and the path-overlap conflict analyzer. The merge controller loads member requests directly because its Runway payload preserves one ordered merge step per request. ## Why it exists -A `Batch` is a thin reference entity: it carries the IDs of the requests it contains, not their changes. Decision and action extensions (the scorer, build runner, pusher, and future detail-aware conflict analyzers) are handed that identity and resolve the granular content themselves through an injected `Resolver`, rather than depending on a controller to pre-resolve and pass the data in. The resolver depends only on the two resolution-target stores — the request store (to walk a batch's contained requests) and the change store (to attach provider details) — and nothing else. +A `Batch` is a thin reference entity: it carries the IDs of the requests it contains, not their changes. Decision and action extensions such as scorers, build runners, and conflict analyzers are handed that identity and resolve the granular content themselves through an injected `Resolver`, rather than depending on a controller to pre-resolve and pass the data in. The resolver uses the queue-scoped storage aggregate to reach the request store (to walk a batch's contained requests) and the change store (to attach provider details). ## Two fidelities -The resolver offers the same walk at two levels of detail, and both preserve batch boundaries — neither flattens across batches, so a caller that wants a flat list flattens the result itself: +Both methods operate on one batch per call: -- The raw view returns each batch's contained changes as URIs only, one group per input batch, in input order. It performs no change-store read. The build stage uses it for base and head inputs; the merge stage uses it for the pusher. -- The detailed view returns a single batch's normalized, batch-level changes: one entry per claimed URI, each carrying the provider details recorded in the change store, aggregated across every request in the batch. Because the change store returns rows for every request that ever claimed a URI, the resolver selects the row owned by the requesting request. The scorer uses it, as will any analyzer that needs changed-file or line-count facts. +- The raw view returns a batch's contained changes as URIs only, in request order. It performs no change-store read. Build-runner implementations use it to construct base and head inputs. +- The detailed view returns a single batch's normalized, batch-level changes: one entry per claimed URI, each carrying the provider details recorded in the change store, aggregated across every request in the batch. Because the change store returns rows for every request that ever claimed a URI, the resolver selects the row owned by the requesting request. Scorers and detail-aware conflict analyzers use this view. ## Testing diff --git a/submitqueue/extension/conflict/README.md b/submitqueue/extension/conflict/README.md index 4813f6413..feab1f163 100644 --- a/submitqueue/extension/conflict/README.md +++ b/submitqueue/extension/conflict/README.md @@ -1,46 +1,25 @@ # Conflict -Vendor-agnostic interface for detecting conflicts between a candidate batch -and the batches already in flight. +Vendor-agnostic interface for detecting conflicts between a candidate batch and the batches already in flight. ## Interface -`Analyzer` exposes a single `Analyze` method that takes the candidate batch -and the list of in-flight batches it might conflict with. It returns the -subset of in-flight batches that conflict with the candidate, each paired -with a `ConflictType` describing the kind of conflict. An empty result means -the candidate is free to advance independently. +`Analyzer` exposes a single `Analyze` method that takes the candidate batch and the list of in-flight batches it might conflict with. It returns the subset of in-flight batches that conflict with the candidate, each paired with a `ConflictType` describing the kind of conflict. An empty result means the candidate is free to advance independently. -Callers are responsible for filtering out the candidate itself and any -terminal batches from the in-flight list before invoking the analyzer. The -analyzer itself stays free of lifecycle knowledge. A non-nil error reports -an infrastructure failure of the analysis and should be treated as -retryable by the caller. +Callers are responsible for filtering out the candidate itself and any terminal batches from the in-flight list before invoking the analyzer. The analyzer itself stays free of lifecycle-transition knowledge. A non-nil error reports that analysis could not be completed; implementations return plain errors and the configured error classifiers decide retryability. -The analyzer is intentionally pure with respect to batch state: it does not -mutate inputs, does not read storage, and may be called concurrently. Real -implementations are expected to resolve the batch contents (e.g. changed -build targets, modified files) via whichever upstream system they depend -on, and to return as much classification detail as that system supports. +The analyzer does not mutate batch inputs and may be called concurrently. Implementations resolve the batch contents they need through injected dependencies. For example, `pathoverlap` uses a `changeset.Resolver`, whose store-backed implementation reads queue-scoped request and change records. ## Implementations -- [`all/`](all/) — pessimistic stub: reports every in-flight batch as a - `ConflictTypeConservative` conflict. Useful as a worst-case baseline and - for wiring tests where speculation must serialize. -- [`none/`](none/) — optimistic stub: reports no conflicts. Useful as a - best-case baseline and for wiring tests where speculation should run all - batches in parallel. +- [`all/`](all/) — pessimistic stub: reports every in-flight batch as a `ConflictTypeConservative` conflict. Useful as a worst-case baseline and for wiring tests where speculation must serialize. +- [`fake/`](fake/) — wraps another analyzer and optionally injects configured failures for tests and example wiring. +- [`none/`](none/) — optimistic stub: reports no conflicts. Useful as a best-case baseline and for wiring tests where speculation should run all batches in parallel. +- [`pathoverlap/`](pathoverlap/) — resolves changed files and reports overlap conflicts by whole file or parent directory. ## Adding a new backend 1. Create `extension/conflict/{backend}/` with an `Analyzer` implementation. -2. Resolve each `entity.Batch` into whatever signal the backend needs - (e.g. changed build targets, files touched, dependency graphs). -3. Emit one `Conflict` per (in-flight batch, detected conflict type). Pick - the most specific `ConflictType` your backend can determine; use - `ConflictTypeConservative` only when the backend cannot prove the absence - of a conflict and falls back to a pessimistic default. Introduce a new - `ConflictType` constant when you can classify the conflict more precisely. -4. Return a plain error for transient infrastructure failures so callers - can classify and retry. +2. Resolve each `entity.Batch` into whatever signal the backend needs (e.g. changed build targets, files touched, dependency graphs). +3. Emit one `Conflict` per (in-flight batch, detected conflict type). Pick the most specific `ConflictType` your backend can determine; use `ConflictTypeConservative` only when the backend cannot prove the absence of a conflict and falls back to a pessimistic default. Introduce a new `ConflictType` constant when you can classify the conflict more precisely. +4. Return plain errors and let the consumer's configured classifiers determine retry behavior. diff --git a/submitqueue/extension/storage/README.md b/submitqueue/extension/storage/README.md index 2672851f8..df1095f0e 100644 --- a/submitqueue/extension/storage/README.md +++ b/submitqueue/extension/storage/README.md @@ -6,9 +6,7 @@ Pluggable persistence interfaces for SubmitQueue entities (requests, batches, de Storage follows the extension contract: the queue-scoped store aggregate is resolved per queue through a factory keyed by queue name, mirroring how every decision/action extension resolves its implementation. A resolved aggregate is bound to its queue — entity arguments whose queue disagrees with the binding are rejected, queue-keyed reads are implicitly scoped, and the host wiring decides which backend serves which queue (single shared backend by default). -Three read-model stores are deliberately global rather than queue-scoped, because their lookups start from identifiers that arrive without queue context (a bare request ID or change URI at the status API): the request log, the request summary, and the change-URI mapping. They are injected individually as standalone seams, following the gateway's per-store injection. The queue registry (`queueconfig`) was never part of this aggregate and stays the registry the factory sits beside. - -The classification rule: a store is queue-scoped when every read path authoritatively holds the queue before the first read, and global when any read path begins from an identifier that arrives without queue context. Entity IDs are opaque — no reader may derive the queue from an ID prefix; the queue travels explicitly on payloads and requests. +Every entity and read-model store is a member of the queue-scoped `Storage` aggregate returned by `Factory.For`. Gateway read requests therefore carry the queue explicitly before resolving request summaries, logs, URI mappings, or queue-list projections. Entity IDs remain opaque — readers do not derive the queue from an ID prefix. ## Optimistic locking contract @@ -48,7 +46,7 @@ A `Get` immediately following a successful write (`Create`/`Update`) — by the ## Key-value contract -Store interfaces are designed for the storage technology *space*, not for SQL (see the Extensions section of the repo [AGENTS.md](../../../../AGENTS.md)): every method must be satisfiable by a plain key-value backend (DynamoDB, Bigtable, an in-memory map) as cheaply as by MySQL. Concretely, a store exposes only get/put/conditional-update **by primary key**. No lookups by other attributes, no listings filtered server-side, no joins. +Store interfaces are designed for the storage technology *space*, not for SQL (see the Extensions section of the repo [AGENTS.md](../../../AGENTS.md)): every method must be satisfiable by a plain key-value backend (DynamoDB, Bigtable, an in-memory map) as cheaply as by MySQL. Contracts use point operations by complete primary key and deliberate bounded prefix/range reads where the lookup fields are leading components of the primary key. They do not require secondary-index lookups, joins, or arbitrary server-side predicates. **The smell test is the index.** If implementing a proposed store method in MySQL requires adding a secondary index (`KEY idx_*`) to the schema, the method is a query-by-attribute in disguise and the contract has left the key-value space — a KV backend would need a global secondary index or a hand-maintained index table to fake it. Treat a new `KEY` line in a schema diff as a design review flag, not a tuning detail. diff --git a/submitqueue/gateway/README.md b/submitqueue/gateway/README.md index f1f640623..4e9ec8465 100644 --- a/submitqueue/gateway/README.md +++ b/submitqueue/gateway/README.md @@ -4,21 +4,23 @@ The gateway is the RPC entry point to SubmitQueue. It accepts `Land`, `Cancel`, ## Request receipts and current summaries -`Land` creates gateway-owned receipt projections before publishing the request: +`Land` first creates an internal request summary in `accepting` state. That receipt prevents a failed publish from exposing work that was never admitted. After the start message is published, the gateway records the `accepted` log through the request materializer. -- An authoritative request summary keyed by sqid. -- One exact change-URI mapping per submitted URI. -- A queue-ordered receipt projection used by `List`. +The materializer appends the log, chooses the winning current status, and activates or repairs the public projections: -`GetRequestSummaryByID` and `GetRequestSummaryByChangeURI` read authoritative summaries. `List` reads the queue projection and may briefly lag those summaries while eventual repair converges. +- The authoritative request summary keyed by sqid. +- One change-URI mapping per submitted URI. +- The queue-ordered projection used by `List`. + +`GetRequestSummaryByID` and `GetRequestSummaryByChangeURI` read authoritative summaries. `List` reads the queue projection and may briefly lag while later log materialization repairs a partial attempt. If recording `accepted` fails after publication, `Land` still succeeds because subsequent pipeline logs can advance the internal `accepting` summary and create the public projections. ## Request log ownership The gateway owns the request log read model and is the only service that reads it. -- For statuses produced synchronously by the gateway, such as `accepted` on `Land` and `cancelling` on `Cancel`, the gateway persists the event through the shared request-log materializer before returning or publishing. +- `Land` publishes first and then attempts to materialize `accepted`; publication is its success boundary. `Cancel` materializes `cancelling` before publishing so the user's intent is visible when the RPC returns. - For statuses produced downstream, the orchestrator publishes entries to the `log` topic through `submitqueue/core/request.PublishLog`. The gateway consumes that topic and persists each entry through the same materializer. -- Orchestrator DLQ reconciliation materializes terminal repairs directly so the DLQ delivery remains unacknowledged until the log and public projections converge. +- Orchestrator DLQ reconciliation transitions durable request state and publishes terminal log entries to the same `log` topic; the gateway remains the materializer. - `GetRequestHistoryByID` and `GetRequestHistoryByChangeURI` read retained request-log rows directly. The materializer appends every audit event, selects the current authoritative winner, and repairs the queue projection. The normal orchestrator pipeline does not read or write the request-log store directly. diff --git a/submitqueue/orchestrator/README.md b/submitqueue/orchestrator/README.md index c2405ffc1..6dea30d7c 100644 --- a/submitqueue/orchestrator/README.md +++ b/submitqueue/orchestrator/README.md @@ -1,22 +1,24 @@ # SubmitQueue Orchestrator -The orchestrator runs the SubmitQueue land pipeline. It consumes the internal topics declared in `submitqueue/core/topickey/` and advances requests and batches through the stages that lead from `accepted` to a terminal state. +The orchestrator runs the SubmitQueue land pipeline. Its complete consumed-stage and publish-only topology is declared in [`pipeline.go`](pipeline.go). It consumes SubmitQueue-owned internal topics, Runway-owned result topics, and the shared hook topic. ## Pipeline stages The pipeline is queue-driven: each stage consumes one topic, advances one entity, and publishes to the next topic. - **start** — receives `LandRequest` from the gateway, persists the `Request` entity, and emits `Started`. +- **cancel** — records cancellation intent and hands affected batches to speculation for best-effort cancellation. - **validate** — checks for duplicates, resolves change metadata, and publishes a `MergeRequest` to Runway's `merge-conflict-check` topic. -- **mergeconflictsignal** — correlates the dry-run result, fails the request on conflict, or forwards it to batching. -- **batch** — groups the request into a `Batch` with its dependencies. -- **speculate** — decides which speculative paths to validate (CI) versus land directly. +- **merge-conflict-check-signal** — correlates the dry-run result, fails the request on conflict, or forwards it to batching. +- **batch** — creates an inert batch attempt and hands it to dependency analysis. +- **dependency-analysis** — enrols requests, computes dependencies, and promotes the selected batch attempt. +- **speculate** — reconciles queue-wide path state, decides outcomes, and allocates speculative builds. - **build** — triggers a CI build for a speculative path. -- **buildsignal** — records the CI result and loops back to `speculate`. +- **buildsignal** — polls or receives CI state, records the result, wakes `speculate`, and holds non-terminal deliveries until the next poll. - **merge** — publishes a committing `MergeRequest` to Runway's `runway-merge` topic. -- **mergesignal** — correlates the merge result and fans out to `conclude` and back to `speculate`. +- **merge-signal** — correlates the merge result and fans out to `conclude` and back to `speculate`. - **conclude** — maps the terminal batch state to the request states. -- **log** — persists gateway-owned request-log events published by the orchestrator. +- **submitqueue-hook** — dispatches lifecycle hook events to configured integrations. - **DLQ reconcilers** — one per primary consumed topic, driving stuck requests/batches to a conservative terminal `failed` state. -See [doc/rfc/submitqueue/workflow.md](../../doc/rfc/submitqueue/workflow.md) for the full pipeline diagram and ownership rules. +The orchestrator publishes request-log entries to `log`, but does not consume or persist them; the gateway owns that stage. It also publishes full cross-service requests to Runway's merge-conflict-check and merge topics. diff --git a/submitqueue/orchestrator/controller/README.md b/submitqueue/orchestrator/controller/README.md index c248eac8e..4fc03735a 100644 --- a/submitqueue/orchestrator/controller/README.md +++ b/submitqueue/orchestrator/controller/README.md @@ -1,94 +1,41 @@ # Controller Correctness -SubmitQueue controllers are built around eventual consistency. Controllers advance a workflow through durable state checkpoints, and every component must tolerate retries before the next checkpoint is recorded. +SubmitQueue controllers reconcile durable state under at-least-once delivery. Messages may be duplicated, delayed, or replayed after only part of an earlier attempt completed, so each controller must classify the state it loads and make its writes and fan-out safe to repeat. -The core model is: +There is no single checkpoint algorithm that applies to every stage. A controller's ordering depends on which facts a downstream consumer requires and whether a replay can reconstruct an output that was lost. -> Load durable state, reconcile it toward this controller's checkpoint, then replay the checkpoint's fanout until it is accepted. +## Reconciliation pattern -Optimistic locking protects checkpoints from concurrent writers. Failures and races are expected to be uncommon, so the system may leave harmless partial or orphaned data from attempts that never reached a checkpoint. That data can be cleaned up separately if it becomes a problem. +A queue controller normally: -## Checkpoint pattern +1. Decodes the message's thin identity or cross-service payload. +2. Resolves queue-scoped dependencies and reloads authoritative state. +3. Classifies the current state as actionable, already handled, superseded, or invalid. +4. Performs retry-safe preparation and conditional writes. +5. Replays or emits the required fan-out with stable logical identities. +6. Returns `nil` to acknowledge, an error for consumer classification, or calls `delivery.Hold(delayMs)` and returns `nil` to postpone polling work. -Each controller owns a small set of state transitions. It must classify the latest state before writing: +States beyond a controller's work are not handled uniformly. Some stages acknowledge because a downstream owner has taken over; others repair advisory records or repeat fan-out. The controller package and tests must document which behavior is correct for each state. -```text -Process(message): - entity = load latest durable state - - if state is before my checkpoint: - perform retry-safe preparation - record checkpoint with optimistic locking - if the version changed: - return ErrVersionMismatch - - if state is at my checkpoint: - replay complete fanout using stable message identities - return success - - if state is beyond or supersedes my checkpoint: - return success - - return invalid-state error -``` - -The important states are: - -| State relative to this controller | Behavior | -|---|---| -| Before checkpoint | Perform retry-safe work and record the checkpoint. | -| At checkpoint | Skip the state transition and replay the complete fanout. | -| Beyond checkpoint | A downstream controller already consumed the handoff. Acknowledge without regressing state. | -| Superseded | Cancellation, failure, or another outcome made this work unnecessary. | -| Invalid | Return an error rather than inventing a transition. | - -## Retry and redelivery - -Prefer one reconciliation pass per delivery. The consumer framework is the retry loop: - -```text -controller returns error - -> error processor classifies it - -> consumer nacks retryable errors - -> redelivery re-enters Process and reloads durable state -``` - -Controllers should not classify ordinary backend failures merely because replay would be convenient. Return the raw wrapped error and let the configured classifiers decide whether it is transient. A permanent publish or storage failure must eventually reach the DLQ rather than retry forever. - -## Persist before publishing +## Persistence and publishing -For a state transition followed by queue fanout: - -```text -persist checkpoint -publish complete fanout -ack delivery -``` +Persist any fact a downstream consumer must reload before publishing the message that exposes it. For example, a newly accepted speculation path is stored before the build-stage dispatch, and a build record is stored before its build-signal message. -The checkpoint proves that the state transition happened. It does not prove that every output was published. +When a durable transition acts as a replayable checkpoint, a controller may write it and then publish. Redelivery can observe the checkpoint and repeat the complete fan-out with stable message identities. -If a process fails after recording the checkpoint, redelivery observes the checkpoint, skips the transition, and republishes the complete fanout. Every replayed output must use the same topic, partition key, logical message ID, and payload. +Publish first when the later write would erase the only evidence that an announcement is needed. Request-status and build-status observations use this ordering in selected paths: a failed publish leaves state unchanged so redelivery observes and republishes the same event. Such exceptions must be explicit and idempotent; they are not permission to publish arbitrary downstream work before its prerequisites exist. ## Optimistic locking Optimistic locking answers whether an entity changed since it was read. It does not decide whether a lifecycle transition is valid. -A controller must write only from states it owns. For example, speculate may transition `Created` to `Speculating`; it must not load `Merging` and write it back to `Speculating`. +A controller must write only from states it owns. It computes `newVersion := oldVersion + 1`, passes both versions to storage, and updates its local copy only after the conditional write succeeds. When modifying slices or maps, use a candidate copy whose reference fields are cloned so a failed write cannot mutate the caller's original value. -Version arithmetic follows the [storage optimistic-locking contract](../../extension/storage/README.md): compute the new version in the controller and update the in-memory entity only after the write succeeds. +See the [storage optimistic-locking contract](../../extension/storage/README.md). -## Example: speculate +## Fan-out and external effects -| Batch state | Behavior | -|---|---| -| `Created` | Start speculation: publish to `build`, then record `Speculating`. | -| `Speculating` | Once dependencies resolve, publish to `merge` and record `Merging`. | -| `Merging` | Acknowledge without regressing the batch; the merge controller owns recovery. | -| `Cancelling`, terminal | The transition was superseded or another controller owns recovery. | - -Each row writes only from a state `speculate` owns; states owned by other controllers (such as `Merging`) are acknowledged, never rewritten. If a publish fails after a state transition is recorded, redelivery reloads the batch, skips the completed transition, and republishes the fanout with stable message identities. - -## External effects +Every replayed output must preserve the logical identity needed by its consumer: topic, partitioning, correlation ID, and payload semantics. Use a stable intent ID when repeats represent the same hand-off; use a distinct ID only for a deliberate repeat-until-effective repair. An external effect whose outcome was not recorded cannot be made safe by queue deduplication alone: @@ -99,14 +46,24 @@ controller fails before recording the result Such effects require a provider-supported idempotency key, a stable operation identity that can be queried, or an explicit acceptance that duplicate or orphaned work is harmless. +## Speculation example + +The speculate topic is a dirty signal for a queue, not a command to transition only the named batch. A run reloads the queue's in-flight batches, dependency outcomes, path sets, and build results; admits any batches still in `Created`; commits outcomes to a fixed point; asks the configured speculator for new proposals; persists changed path sets; and dispatches pending builds. + +A batch can advance to merge when one passed path matches all settled dependency outcomes. It can also advance while dependencies remain unsettled when passed paths cover every possible outcome of those dependencies, proving that the head passed regardless of how they finish. + +Path-set changes are persisted before build dispatch. Terminal outcomes are committed before later decisions derive from them. Selected request-log announcements are intentionally published before their corresponding state write so replay cannot lose the observation. + ## Review checklist For each controller, make these answers clear: -1. What durable checkpoint does it own? -2. Is all work before that checkpoint safe to retry? -3. How does each possible durable state classify relative to the checkpoint? -4. Can the complete fanout be reconstructed and replayed with stable message identities? -5. Which controller or DLQ path owns superseded and terminal recovery? +1. What authoritative state is reloaded? +2. Which durable states are actionable, already handled, superseded, or invalid? +3. Which writes and external calls are safe to repeat? +4. Which facts must be durable before each publish? +5. Can lost fan-out be reconstructed with the correct logical identities? +6. Which controller or DLQ path owns terminal recovery? +7. Does a non-terminal poll use `Hold` rather than consuming retry budget? -See the [consumer error contract](../../../platform/consumer/README.md), the [orchestrator workflow](../../../doc/rfc/submitqueue/workflow.md), and the [SQL queue RFC](../../../doc/rfc/sql-queue-rfc.md). +See the [consumer error contract](../../../platform/consumer/README.md) and the orchestrator's current [`pipeline.go`](../pipeline.go) topology.