From 662192cbfa3357df434e7522f94004461a87a6c8 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 28 Jul 2026 09:54:41 +0000 Subject: [PATCH 1/2] docs(design): attribution facts as a stream, not a keyspace Three design records land together. attribution-fact-stream.md is the chosen replacement for the attribution keyspace. The audit receiver appends one batched entry per type to a per-(route, group/resource) Redis stream, every process follows only the types it watches, and the facts live in one bounded, TTL'd in-memory index. That deletes the per-key SET/GET lookup, the 150ms poll loop, and the deletecollection expander, while keeping the blocking grace window, commit order, and the one-fact-serves-every-GitTarget property. The transport sits behind a two-method seam, so a single-pod install can run attribution on an in-memory ring instead of Redis, and the same shape works unchanged once GitTargets are distributed across replicas. attribution-wait-poll-vs-push.md is the reasoning trail behind it, kept as a superseded record: six options priced against the measurement that decided between them. Audit delivery is batched by the apiserver, so the watch event arrives first by roughly the batch window and the first lookup is a near-guaranteed miss. Both the e2e wait histogram and the mutation-capture corpus already recorded this. open-asks-priority.md merges the three open backlogs into one ordered queue. Docs-only, so task lint-docs is the whole gate per AGENTS.md. It passes. Co-Authored-By: Claude Opus 5 --- docs/INDEX.md | 5 +- docs/design/attribution-fact-stream.md | 725 +++++++++++++++++++ docs/design/attribution-wait-poll-vs-push.md | 545 ++++++++++++++ docs/design/open-asks-priority.md | 346 +++++++++ 4 files changed, 1620 insertions(+), 1 deletion(-) create mode 100644 docs/design/attribution-fact-stream.md create mode 100644 docs/design/attribution-wait-poll-vs-push.md create mode 100644 docs/design/open-asks-priority.md diff --git a/docs/INDEX.md b/docs/INDEX.md index 6e9c88db..0faa1084 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -65,12 +65,15 @@ says what we support and refuse** — and then its kustomize field taxonomy, the write boundary, the orchestrator/expansion line, and how secrets are handled. -Thirteen other open items: +Fifteen other open items: | Doc | Open question | |---|---| +| [`open-asks-priority.md`](design/open-asks-priority.md) | three backlogs are open at once — the gitops-api consumer asks, the maintainer review's unbuilt block (F6, F9, F10), and the config-surface proposal (B1–B6) — and they overlap. Merges them into one ordered queue under four stated tests, and makes one design call against what was asked: **delete Option C sibling inference** rather than ship an off-switch for it, because it lets a human's edit to the repository change the operator's behaviour with nothing in status recording the move, its central guard has already failed once by cascading, and the explainability its own spec made mandatory was never built. That answers the namespace-leak ask by removal, and means `spec.placement.mode` is never built. Open: whether the removal ships with an Event on the first fall-back to canonical | | [`docs-linting.md`](design/docs-linting.md) | how to mechanize [`style-guide.md`](style-guide.md) with markdownlint-cli2 and Vale. Both are wired into `task lint`, gated on the files [`.docs-lint-scope`](../.docs-lint-scope) lists rather than the whole tree: 102 of 174 files fail markdownlint and 148 of 174 fail Vale, so the two backlogs need different gates. Open: how the scope list grows to cover the tree, the `MD013` limit, and whether `AGENTS.md` and the chart READMEs are in scope | | [`attribution-fact-identity.md`](design/attribution-fact-identity.md) | several `ClusterProvider`s may name one physical cluster, but a kube-apiserver posts audit to one route, so only one of those names is ever fed and every other one authors `unknown (attribution unresolved)`. Proposes a declared `spec.attribution.auditRoute` that partitions the facts instead of `metadata.name`, so several providers can share one cluster's facts while cloned clusters stay separate, ingestion loses its last Kubernetes read, and a misrouted provider becomes loud. Renames the key infix and the annotation-key flag to the same word | +| [`attribution-fact-stream.md`](design/attribution-fact-stream.md) | the chosen replacement for the attribution keyspace: the audit receiver appends one batched entry per type to a per-`(route, group/resource)` Redis **stream**, every process follows only the types it watches, and the facts live in one bounded, TTL'd in-memory index per process. Deletes the per-key `SET`/`GET` lookup and the poll loop; keeps the blocking grace window, commit order, and the one-fact-serves-every-GitTarget property. Argues against plain publish and subscribe (silent, undetectable loss on every restart, reconnect, and new watch, once the keys are gone) and against a per-GitTarget index. Also deletes the `deletecollection` expander: one collection fact that every removal in its (type, namespace, selector, window) scope joins, ranked below every per-object fact, which removes the response-body parsing and starts resolving the aggregated-API and metadata-only cases that degrade to committer today. Chosen partly to unblock HA: audit POST and watch shard land on different replicas by construction, and a resumable stream survives the rollouts an HA install spends its life in. Open: cap granularity, entry size, and whether a replica warms its index before taking over a watch | +| [`attribution-wait-poll-vs-push.md`](design/attribution-wait-poll-vs-push.md) | **superseded by the above, kept as the reasoning trail.** a watch event needs its author before it can be routed, and the audit fact naming that author may not have arrived yet, so `ResolveAuthor` polls Redis every 150ms for up to three seconds on the watch shard's own goroutine. Separates the wait (forced: two unordered deliveries out of one kube-apiserver, and the commit window groups by author) from the poll (a choice). Answers which of the two fires first: the watch, nearly always, because audit delivery is batched by the apiserver while the watch is streamed, so the first lookup is a near-guaranteed miss and the loop runs to completion on every attributable event. Six options priced against that, from shifting the first check to the delivery floor and a circuit breaker for an audit route that has never resolved anything, through a Redis publish and subscribe, to the reassembly-buffer design that stops blocking the watch shard. Open: whether the wait population is dominated by resolved-late (favors publish and subscribe) or never-resolved (favors the buffer), plus a proposed per-scenario timing report from the mutation-capture lab | | [`watch-and-catalog-architecture.md`](design/watch-and-catalog-architecture.md) | the target three-layer watch model — **needs a human call before building** | | [`metrics-observability-plan.md`](design/metrics-observability-plan.md) | the watch-stage metrics do not exist yet | | [`reconcile-triggering.md`](design/reconcile-triggering.md) | which controllers still fail to wake up | diff --git a/docs/design/attribution-fact-stream.md b/docs/design/attribution-fact-stream.md new file mode 100644 index 00000000..841058df --- /dev/null +++ b/docs/design/attribution-fact-stream.md @@ -0,0 +1,725 @@ +# Attribution facts as a stream, not a keyspace + +> **design**: proposed, not built. Index: [`../INDEX.md`](../INDEX.md) +> +> Supersedes the option analysis in +> [`attribution-wait-poll-vs-push.md`](attribution-wait-poll-vs-push.md), which priced six ways to +> stop polling Redis. This record picks one and specifies it: the audit receiver publishes facts, +> the watch side subscribes per type and holds them in memory, and the per-key Redis lookup is +> deleted rather than optimized. +> +> Three deliberate deviations from the brief are argued in [the push back](#the-push-back). The +> largest is the transport: a Redis **stream** rather than plain publish and subscribe. +> +> It also deletes the `deletecollection` expander outright: a collection delete becomes one fact +> that every removal in its scope joins, which is both less code and more capable than parsing a +> response body that is often not there. See [collection deletes](#collection-deletes-are-one-fact). +> +> The transport sits behind a two-method seam so a single-pod install can run attribution on an +> in-memory ring instead of Redis. See +> [the transport seam](#the-transport-seam-and-running-without-redis), which also records what the +> deleted `RedisByTypeStreamQueue` did and what it never had. +> +> It is also chosen to move toward high availability rather than away from it. See +> [what this buys for HA](#what-this-buys-for-high-availability) for which part of that problem it +> solves and which part it leaves open. + +## The decision + +Attribution facts stop being a keyspace that the watch side reads, and become a **log that the +watch side follows**. + +The audit receiver, which already decodes a whole batch of audit events per request, appends one +entry per type to a per-type stream. Every process that watches that type follows the stream and +keeps the facts in a bounded, TTL'd in-memory index. The resolver's join becomes a map lookup +against that index, and the grace window becomes a wait on an in-process signal rather than a poll +against Redis. + +`SET`, `GET`, and the three fact key shapes in +[`attribution_index.go`](../../internal/queue/attribution_index.go) go away. Redis keeps the watch +resume cursors and the command-author records, which are unrelated and unchanged. + +## What this is replacing + +Today the write side stores each fact under up to two of three key shapes, and the read side polls +for up to three seconds trying up to three keys per attempt. The measurement that motivates the +change is in the previous record: audit delivery is **batched** by the apiserver, so the watch event +arrives first by roughly `--audit-webhook-batch-max-wait`, the first lookup is a near-guaranteed +miss, and the poll loop runs to completion on essentially every attributable event. + +The keyspace was built for a lookup that was assumed to usually hit. It usually misses, and the +thing it is being asked to do is deliver a fact that has not happened yet. That is a queue's +job. + +## The push back + +### 1. Use a stream, not plain publish and subscribe + +Plain `PUBLISH` and `SUBSCRIBE` is fire-and-forget and at-most-once. A message reaches only the +subscribers connected at that instant, and there is no way to notice that one was missed. + +Dropping the keys removes the safety net that currently makes those properties tolerable. Today a +missed notification would cost latency, because the fact is still sitting in Redis under a key and +the next poll finds it. Once the keys are gone, a message lost during a reconnect is **gone**, and +the commits that needed it are authored `unknown (attribution unresolved)`, silently and +permanently. Attribution is the product's reason for existing, so trading a latency problem for a +correctness problem is the wrong trade even at low probability. + +Three concrete cases turn this from theory into routine: + +- **Every process restart.** A rollout, an upgrade, an OOM kill. A fresh subscriber's in-memory + index is empty, and pure publish and subscribe has no way to fill it. Every event in flight + during that window loses its author. +- **Every reconnect.** A dropped Redis connection loses whatever was published while it was down, + with nothing to detect afterward. +- **Every new watch.** A `WatchRule` that starts watching a type subscribes with an empty index, + and its first events find nothing. + +A Redis **stream** is the same idea with the same shape and none of those costs. It is still a queue +the receiver appends to and the watch side follows. It still replaces the keyspace. But entries +persist for a bounded window, each has an ID, and a reader resumes from the last ID it processed. +That single property turns all three cases above into a replay of the last few minutes: +[start from the TTL horizon](#starting-up-and-catching-up) and the index is warm before the first +event needs it. + +It also makes loss **detectable**, which fire-and-forget never can. If a reader falls so far behind +that the stream trimmed past its position, the gap is visible in the IDs, so it becomes a metric and +a log line instead of a quiet run of unattributed commits. + +The cost is one Redis data structure the codebase has not used before. That is a smaller price than +the three cases above. + +### 2. One index per process, not one per GitTarget + +The brief asks for "an in-memory TTL'ed set of attribution facts for every GitTarget". That would +undo the property worth keeping: a fact names a write that happened in Kubernetes, not a consumer +interested in it, so one fact already serves every `GitTarget` that needs it. Five `GitTarget`s +mirroring one `Deployment` would hold five copies of every fact, and the memory bill would scale +with a number that has nothing to do with how much is happening in the cluster. + +Instead: **one process-wide index**, keyed exactly as the fact keys are keyed today, and a +**subscription set that is the union** of the types any watch is following, reference counted so a +type is unsubscribed when the last watch on it goes away. `GitTarget`s share the index the same way +they share the keyspace today. The fan-out the brief asks for still happens, at the level where it +does useful work: the process never receives facts for a type nobody watches. + +### 3. Keep the loss bounded and visible + +Redis enforced two things for free: a TTL on every fact, and a memory ceiling with an eviction +policy on the server. Moving the index in-process moves both jobs to this codebase. + +A `deletecollection` over ten thousand objects, or a large rollout, arrives as a burst of facts. +The index therefore needs a per-type cap with oldest-first eviction, and eviction has to be +**counted and exported**, not silently absorbed. An attribution that was dropped because the index +was full must look different in the metrics from one that was never published. + +## The design + +### Stream naming + +One stream per `(audit route, group/resource)`: + +```text +gitops-reverser:author:v2:audit:route:: +``` + +The route infix stays exactly as it is today and for the same reason: an apiserver posts under one +route, several `ClusterProvider`s naming one cluster share that route and therefore share its facts, +and a fact from cluster A must never name the author of an object watched on cluster B. See +[`attribution-fact-identity.md`](attribution-fact-identity.md). + +The group/resource suffix is the new part, and it is what makes the fan-out meaningful. A process +watching only `configmaps` and `deployments` follows two streams and never receives a fact for +anything else. + +### The publish side + +[`serveEventListRequest`](../../internal/webhook/audit_handler.go#L186) already decodes one +`EventList` per request, and [`processEvent`](../../internal/webhook/audit_handler.go#L289) already +decides which events produce a usable fact. The change is to accumulate rather than write +immediately: + +1. Each accepted event is reduced to an [`AuthorFact`](../../internal/queue/attribution_index.go#L90) + as [`RecordFact`](../../internal/queue/attribution_index.go#L132) does now, with one change: a + `deletecollection` is published as **one fact describing the collection**, not expanded into one + fact per object. See [collection deletes](#collection-deletes-are-one-fact). +2. Facts are grouped by `(route, group/resource)` across the whole request. +3. One `XADD` per group, carrying that group's facts as a single entry. + +An apiserver batch of 400 events over three types becomes **three appends**, not 400 writes and 800 +key updates. The batching that causes the delivery delay is the same batching that makes the +publish cheap, which is the part of the brief that is exactly right. + +Only facts that would have been stored are published. An event with no `objectRef` or no user +produces nothing today and must produce nothing here, or waiters get woken by facts that cannot name +anyone. The "no resolvable name" rule is the one exception that changes: a name-less +`deletecollection` is now exactly the case that produces a fact, so the check becomes "no name and +not a collection verb". + +[`AuthorFact`](../../internal/queue/attribution_index.go#L90) gains two fields: the label selector +from the request URI, and the optional set of uids the collection covered, reduced from the response +body at the receiver and dropped past a size cap. It already carries the namespace, verb, and stage +timestamp that the collection join reads, and it stops needing the per-item name and namespace the +expander used to fill in. + +### Trimming is the TTL + +Each `XADD` carries `MAXLEN ~ ` so a hot type cannot grow without bound, and a periodic `XTRIM +MINID ` drops entries past the retention horizon. Stream IDs are millisecond +timestamps, so a time-based trim is a plain `MINID` bound and needs no side index. + +`--author-attribution-ttl` keeps its meaning: how long a fact remains available for a watch event to +join. It now bounds stream retention and the in-memory index together. + +### The subscribe side + +One reader goroutine per process issues a single blocking `XREAD` across every stream in the +subscription set, with a per-stream last-seen ID. + +Two things to get right: + +**No consumer groups.** They are the obvious reach and the wrong one. A consumer group distributes +entries *between* consumers, and this is a fan-out: every process that watches a type needs every +fact for that type. Each process reads independently, from its own last-ID cursor, held in memory. + +**The subscription set changes at runtime**, whenever a `WatchRule` starts or stops covering a type. +The reader blocks with a bounded `BLOCK` interval and re-issues, so a set change takes effect within +one block period rather than needing the connection torn down. + +### The in-memory index + +Entries are applied in stream order into four structures: + +| Structure | Key | Serves | +|---|---|---| +| exact | `(group/resource, uid, rv)` | `ADDED` and `MODIFIED`, the only exact-capable join | +| latest | `(group/resource, uid)` | single-object removals, whose rv never matches; last write wins | +| rv-only | `(group/resource, rv)` | the escape hatch for a fact with an rv but no uid | +| collection | `(group/resource, namespace)`, time-bounded, with an optional uid set | removals caused by a `deletecollection` | + +The first three mirror the key shapes the lookup already knows, so the join policy in +[`LookupAuthorResolution`](../../internal/queue/attribution_index.go#L382) survives unchanged. The +fourth is new and is described in [the next section](#collection-deletes-are-one-fact). + +Stream order matters for exactly one of these. The `latest` map is last-writer-wins, so entries have +to be applied in the order they were appended. A single stream is ordered, and a uid belongs to one +group/resource and therefore to one stream, so per-object order is preserved without any +cross-stream coordination. + +Each entry carries its insertion time for the TTL sweep, and each map is bounded per type with +oldest-first eviction. + +### The resolver + +[`ResolveAuthor`](../../internal/watch/author_resolver.go#L160) keeps its signature, its grace +window, and its blocking behavior. Only the middle changes: + +1. Register a waiter for this event's candidate keys. +2. Check the in-memory index. +3. If absent, block on the waiter, `ctx.Done()`, or the grace deadline. +4. On a hit, return exactly as today, including the + [outcome classification](../../internal/watch/author_resolver.go#L198) that distinguishes + not-attempted from unresolved. + +Step 1 must precede step 2. Registering after the check loses a fact applied in the gap between +them, which is the same race the poll loop currently papers over by looking again. + +The waiter signal comes from the goroutine applying stream entries. There is no Redis call anywhere +on this path: the fast case is a map read, and the waiting case is a channel receive. + +## Collection deletes are one fact + +A `deletecollection` is published as a single fact describing the collection, and every removal that +falls inside that collection joins it. The expander that reconstructs per-object facts by parsing +the response body is deleted. + +### Why the current expander is the wrong shape + +`kubectl delete configmaps --all -n team-a` produces **one** name-less audit event and **N** +independent watch events. That asymmetry is not an accident of our plumbing, it is what the two +mechanisms are: audit reports the request that was made, and the watch reports each object that +changed. The lab corpus records exactly this shape for row 9, watch times N against audit times one. + +[`RecordDeleteCollectionFacts`](../../internal/queue/attribution_index.go#L242) tries to erase the +asymmetry by rebuilding the N from the one, parsing `responseObject` into a list of per-object +identities and writing a fact for each. Everything about that is fragile: + +- **It needs a body that may not be there.** An aggregated API, a metadata-only response, or a + truncated audit event yields nothing, and the whole collection silently degrades to + committer-authored removals. The code says so itself. +- **It parses an untyped list shape** and has to accept typed lists, `v1.List`, and anything else + carrying an `items` array. +- **It writes N entries for one request.** A collection delete over ten thousand objects is ten + thousand writes for one audit event, which is also the burst most likely to overflow the index. +- **It carries its own reason code and its own test file** for a case that the join could have + handled structurally. + +### The replacement + +Publish one fact carrying what the audit event actually said: the actor, the verb, the +group/resource, the namespace, the stage timestamp, and the selector from the request URI when one +was given. Then a removal joins it by **scope** instead of by identity. + +For a removal event, the resolver tries in order: + +1. the exact `(group/resource, uid, rv)` fact, +2. the `latest` fact for that uid, +3. a **collection** fact whose scope matches and whose **uid set contains this object**, when the + collection carried a usable response body, +4. a **collection** fact whose group/resource and namespace match the object, whose selector matches + the object's labels, and whose stage timestamp is within the collection window, +5. the rv-only hatch. + +Precedence is the correctness argument. A collection fact is the weakest evidence in the table, so +it only ever names an author when nothing more specific does. An unrelated delete by another actor +during the same window is claimed by its own fact at step 2 and never reaches step 4. + +Steps 3 and 4 are the same fact matched two ways, and the split is the subject of +[the next section](#should-the-response-body-travel-with-the-fact). Step 4 alone is already more +capable than the expander, because it does not depend on the response body existing at all, so the +aggregated-API and metadata-only cases that degrade to committer today start resolving. + +### Should the response body travel with the fact? + +Carrying the body so the join can still be uid-precise is worth doing, in a reduced form. Two +questions hide inside it and they have different answers. + +**What travels** and **when it is parsed** are separate decisions. The instinct to "defer it to the +real lookup moment" is right about the second and wrong about the first. + +#### What travels: the uid set, not the body + +| Option | What is stored | Verdict | +|---|---|---| +| 1. Nothing | scope only | The necessary floor, but gives up precision that is sometimes free | +| 2. The raw response body | the full list of deleted objects | Rejected | +| 3. A reduced uid set | one fact plus the uids it covered | **Chosen** | +| 4. Per-object facts (today) | N facts for one request | Rejected, this is what is being removed | + +Option 2 fails on size. A `deletecollection` over ten thousand ConfigMaps carries a response body in +the tens of megabytes, and in this design that body would be broadcast to every subscriber of the +type, retained for the whole TTL window, and replayed into memory on every restart backfill. The +transport is a fact log, and full object bodies are not facts. + +Option 3 keeps everything the join needs and almost none of the size. The receiver already has the +body decoded, so it reduces it there to the set of uids the request covered and publishes +`{actor, scope, selector, uids}`. A uid is 36 bytes, so ten thousand of them is a few hundred +kilobytes rather than tens of megabytes, and it is still **one** entry for one audit event. This is +not the expander in disguise: write amplification stays at one, and no per-object key is created. + +The set needs a cap, and the cap is where the design gets its safety: past some size the uids are +dropped and the fact degrades to scope matching, which is exactly step 4 and is already correct. +Degradation must be counted, so "we fell back to scope" is visible rather than inferred. + +#### What that buys: over-attribution goes to zero when the body is there + +This is the strongest argument for your suggestion, and it is worth being explicit about. + +Scope matching accepts a bounded risk of naming the **wrong** human, which is worse than naming +nobody. Uid membership has no such risk: either the object was in the set the apiserver said it +deleted, or it was not. So when the body is present, the weakest tier in the table stops being weak. + +One clarification on what "exact" can mean here. The body's items carry the **pre-delete** +resourceVersion, which no watch removal event ever presents, which is why the current expander +writes only the `:last` key. So the body upgrades the join from scope to **uid membership**, not to +the `(uid, rv)` exact tier. That is still a large improvement, and it is the reason step 3 sits +above step 4 rather than replacing it. + +#### When it is parsed: at ingest, not at lookup + +Parsing at lookup sounds cheaper and is not, for two reasons. + +The lookup runs **once per removal event**, so a lazily parsed body is parsed N times for one +collection delete, unless it is memoized, at which point it is option 3 with extra steps. And the +lookup runs on the watch shard's blocking path, where the event's whole stream is stalled behind it. +Parsing a ten thousand item list belongs on the stream reader goroutine, not there. + +The one thing lazy parsing wins is a collection fact that expires with no removal ever arriving, +where the parse was wasted. A `deletecollection` that produced facts almost always produces +removals, so that saving is rare. + +What **should** stay at lookup time is the **decision**, not the parsing: try uid membership, fall +back to scope. Deferring the decision is what makes the fallback possible at all, and it is the part +of the suggestion worth keeping. + +#### Why the scope fallback is the primary path, not the corner case + +The body is least likely to be there exactly when it would be most valuable. + +[`audit-webhook-api-server-connectivity.md`](../facts/audit-webhook-api-server-connectivity.md) +recommends enabling `--audit-webhook-truncate-enabled` in production, and truncation drops +request and response bodies from oversized events. The oversized event is the ten-thousand-object +collection delete. A well-configured production cluster is therefore the one most likely to hand us +a body-less `deletecollection`, and the current expander gives up entirely in that case. + +So the ordering is deliberate: scope matching is the floor that must work on its own, and the uid +set is an opportunistic upgrade taken when the apiserver happened to send a body. + +#### Bodies stay out of the stream for everything else + +This applies to collection deletes only. For a normal write, the watch event already carries the +full object, so an audit body would duplicate it for no gain, and the fact already extracts the one +thing it needs from `responseObject`, the post-write resourceVersion +([`rvFromRawObject`](../../internal/queue/attribution_index.go#L558)). + +That question is settled in this repository rather than open. Row 15 of the lab corpus established +that an aggregated-API write produces an audit event with an **empty** body while the watch carries +the full object, and the supplementary body-enrichment proxy was retired on the strength of it +([`test/mutationlab/README.md`](../../test/mutationlab/README.md)). Re-introducing bodies into the +fact path generally would walk that decision back. + +### The selector is better evidence than the body + +The request URI's `labelSelector` is the *intent* the actor expressed. The watch event carries the +*object*. Evaluating one against the other is a better test of membership than reading back a list +the apiserver may not have sent, and it uses each stream for what it uniquely knows. This is the +difference the brief points at, and it is worth stating as the design rule: **audit says what was +asked for, the watch says what changed, and the join belongs at the point where both are in hand.** + +A collection fact with no selector matches every object of that type in the namespace, which is +correct, because that is what `--all` means. + +### Why the window can be tight + +Over-attribution is the real risk here and it deserves a plain statement: a scope match can name the +wrong human, which is worse than naming nobody. Three things bound it, and the third is the one that +makes it safe. + +Namespace and selector narrow the scope. Precedence keeps anything with its own fact out. And the +window is short, because of the deletion-as-intent rule in +[`deletecollection-attribution-expander.md`](../spec/deletecollection-attribution-expander.md): the +removal being attributed happens at **delete-request time**, when `deletionTimestamp` is set, not at +whatever later moment finalization completes. Finalizers do not stretch the window. So the fact's +own `stageTimestamp` plus a small allowance for skew and delivery is enough, and it can be far +shorter than the fact TTL. + +That last point is what makes this design safe and would not hold under the old framing, where +attribution chased the eventual removal. + +## How it fits together + +```mermaid +flowchart TB + subgraph SRC["Source cluster"] + API["kube-apiserver"] + end + + subgraph OPERATOR["gitops-reverser process"] + AH["audit_handler.go
decode EventList
group facts by type"] + RD["stream reader
one blocking XREAD
over the watched set"] + IDX[["in-memory fact index
exact · latest · rv-only
TTL'd, bounded, one per process"]] + WS1["watch shard
configmaps"] + WS2["watch shard
deployments"] + BW["branch worker
commit window"] + end + + subgraph REDIS["Redis"] + S1[("stream
route:prod-eu-1:configmaps")] + S2[("stream
route:prod-eu-1:apps/deployments")] + S3[("stream
route:prod-eu-1:secrets
nobody watches this type")] + end + + API -->|"audit POST · batched
up to batch-max-size events"| AH + AH -->|"XADD · one per type per request"| S1 + AH -->|XADD| S2 + AH -->|XADD| S3 + + S1 -->|XREAD| RD + S2 -->|XREAD| RD + S3 -.->|"never read"| RD + + RD -->|"apply in order"| IDX + IDX -.->|"signal waiters"| WS1 + IDX -.->|"signal waiters"| WS2 + + API ==>|"watch event · streamed
arrives first"| WS1 + API ==>|"watch event"| WS2 + + WS1 -->|"blocks until author or grace"| IDX + WS2 --> IDX + WS1 ==>|"routed in arrival order"| BW + WS2 ==> BW + + classDef gone fill:#f0f0f0,stroke:#999,color:#666; + classDef mem fill:#e6f7e6,stroke:#33aa33,color:#000; + class S3 gone; + class IDX mem; +``` + +The heavy arrows are the mirroring path, which is unchanged and stays serial per shard. The dotted +arrow to `secrets` is the fan-out doing its job: the stream is written, nobody follows it, and it +trims itself away. + +## What is deliberately kept + +**The blocking property.** A watch event still waits inline for its author, up to the same grace +window, on the same single-threaded shard. This is what keeps a late fact from ever rewriting a +shipped commit, and it is why the wait exists at all. + +**Commit order.** Nothing here touches the ordering chain: one goroutine per +`(GitTarget, GVR, scope)` that finishes one event before reading the next, feeding one FIFO into the +branch worker. Attribution decides what author is stamped, never when an event is routed relative to +its siblings. See +[`watch-event-ordering-and-attribution-grace.md`](../facts/watch-event-ordering-and-attribution-grace.md), +including its warning that resolving events concurrently without a sequence-numbered reassembly +buffer would let a later event overtake an earlier one on the same object. Faster resolution makes +that refactor tempting. It is still out of scope, and still wrong without the buffer. + +Order improves in one respect. An open commit window accepts one `(author, GitTarget)` pair, so +[`canAppend`](../../internal/git/branch_worker.go#L730) finalizes on any author change. Today an +event whose fact lands a moment after the grace expires ships unresolved between two resolved +siblings and splits one person's change into three commits. Resolving from a delivered fact rather +than from a race against a deadline removes most of that class. + +**Attribution stays optional.** A nil `Manager.AuthorResolver` remains configured-author mode, with +no stream reader started and no subscription taken. + +**The fan-in property.** One fact still serves every `GitTarget` that needs it, now through a shared +index instead of a shared key. + +**Facts that never resolve.** A status subresource update and a graceful pod delete produce no audit +event at all, so no wait and no transport can name their author. They still spend the grace window +and ship unresolved. That is unchanged, and it is the population that keeps +[the circuit breaker](attribution-wait-poll-vs-push.md#option-c-circuit-break-a-route-that-has-never-resolved-anything) +worth building separately. + +## Starting up and catching up + +On start, and on any reconnect, the reader begins from `MINID = now - factTTL` rather than from +`$`. The index is populated with the whole retention window before the first watch event needs it, +which is what makes a restart cost nothing. + +The last-seen ID per stream lives in memory only. It does not need to be durable: a process that +lost it also lost its watch connections and its in-memory index, so it is starting from the horizon +anyway. + +If a reader's last-seen ID is older than the stream's first surviving entry, it was trimmed past and +lost facts. That is detectable by comparing the two, and it must be reported: a counter plus a log +line naming the stream. This is the case pure publish and subscribe cannot detect at all. + +## Failure modes + +| Failure | Today | Publish and subscribe only | This design | +|---|---|---|---| +| Process restart | facts survive in Redis, re-read by key | index empty, facts lost | replay from the TTL horizon | +| Redis connection drop | next poll finds the key | messages during the gap lost, undetectably | resume from last ID, gap detected if trimmed | +| New watch on a type | reads existing keys | starts empty | replays that type's window | +| Reader falls behind | not possible, reads are on demand | silent loss | trim gap detected and counted | +| Redis unavailable | every lookup misses, all unresolved | same | same | +| Index full under a burst | Redis evicts per its policy | unbounded growth | oldest-first eviction, counted | +| Fact never published | unresolved after the grace | unresolved after the grace | unresolved after the grace | + +The last row is the one that does not move, and it is the honest limit of the whole exercise. + +## What it costs + +**Every process receives every fact for every type it watches**, whether or not it holds an object +that matches. That is the fan-out working as intended, and the brief's judgment that some unmatched +messages are acceptable is right. The volume is bounded by the audit event rate on watched types. + +**Today that cost is zero anyway.** The chart rejects `replicaCount > 1` +([`validate-replica-count.yaml`](../../charts/gitops-reverser/templates/validate-replica-count.yaml)), +so there is exactly one process, and it is both the audit receiver and the watch host. + +**Memory moves from Redis into the pod**, bounded by the caps above and observable through the +eviction counter. + +That single-replica fact raises a fair question: if one process both publishes and consumes, why go +through Redis at all? An in-process channel would work today and would be simpler. The answer is in +the next section, and it is the main reason not to take the shortcut. + +## What this buys for high availability + +HA is wanted, and this design is chosen partly to move toward it rather than away from it. Being +precise about which part of the problem it solves matters, because it is not the hard part. + +**It removes a coupling HA would otherwise create.** Under multiple replicas, the apiserver's audit +webhook posts through a Service to *whichever* replica answers, while a given object's watch shard +lives on *whichever* replica owns that `GitTarget`. Those are unrelated choices, so the fact and the +watcher that needs it routinely land in different processes. A per-type stream with independent +per-reader cursors is exactly the primitive for that: the receiving replica appends, and every +replica watching the type reads it, with no knowledge of each other. The alternative shapes are all +worse. Sticky audit routing makes the apiserver's load balancing this operator's problem, and a +shared lookup keyed per object is what is being deleted here. + +**It makes rolling updates safe, which is where HA spends its time.** A replicated deployment is +almost always mid-rollout, reconnecting, or restarting a pod. Those are precisely the three cases +where plain publish and subscribe loses facts silently, and where a resumable stream replays the +retention window instead. An HA install rolls far more often than a single-replica one, so this is +the difference between attribution being reliable and attribution being reliable except during +deploys. + +**The fan-out cost scales the right way.** A replica subscribes only to the types it hosts shards +for. If HA distributes `GitTarget`s across replicas, the subscription sets are largely disjoint, so +total fact traffic tracks the number of watched types rather than replicas times types. Fan-out +only becomes expensive if every replica watches everything, which is the topology HA exists to +avoid. + +**In-process delivery would foreclose all of that.** A channel between the receiver and the resolver +works perfectly on one replica and has to be thrown away on the second. Going through Redis now +costs a sub-millisecond round trip against a batch window measured in seconds, which is not a +trade-off worth thinking about, and Redis is already a hard dependency for the resume cursors. + +**What it does not solve.** Attribution stops being an HA blocker; it was never the main one. The +open problems live in [`ha-gittarget-distribution-plan.md`](../future/ha-gittarget-distribution-plan.md) +and are about ownership: which replica owns a `GitTarget`, how ownership moves without dropping or +duplicating a watch, and above all that commits to one `(GitProvider, branch)` stay serialized +through a single writer. Two replicas committing to one branch is the real problem, and nothing +here addresses it. What this design does is make sure that when that work happens, attribution +already works across processes instead of needing its own answer. + +## The transport seam, and running without Redis + +### What existed before, and what did not + +The memory is half right, and the half that is right is useful. + +A per-type Redis stream layer did exist. `RedisByTypeStreamQueue` in +`internal/queue/redis_bytype_queue.go` mirrored audit events into one stream per resource type, and +it already used the primitives this design needs: `XADD`, `XTRIM` with `MINID` to bound retention, +a `__index__` set so the keyspace could be enumerated without `SCAN`, and a per-type high-water mark +for ordering. A sibling file added a parallel watch-event stream. The whole audit-stream layer, +about 3,500 lines with its tests, was deleted on 2026-06-30 in commit `e25b5ebd` when watch-first +ingestion made it unnecessary. + +It is worth reading before writing this, from `git show e25b5ebd^:internal/queue/redis_bytype_queue.go`. +Not to restore: it carried an ordering model this design does not need, because facts are keyed data +rather than a sequence to replay. But the stream mechanics, the trim cursor, and the key layout were +worked out once already. + +The abstraction was `AuditEventQueue`, a **one-method interface declared at the consumer** in +`internal/webhook/audit_handler.go`, satisfied by the Redis type and by test fakes. + +**There was never a second implementation.** No in-memory queue existed at any point. The seam was +there, the alternative behind it never was. That matters for estimating this work: the interface is +not the hard part, and having had one before is not evidence that the memory-backed side is close to +free. + +The idiom is alive in the current code and should be followed rather than invented. +[`AttributionLookup`, `CursorStore`, and `AuthorResolver`](../../internal/watch/author_resolver.go) +are all narrow interfaces declared where they are consumed, with the Redis-backed type satisfying +them from the other side. + +### Redis is already optional, which makes this small + +The dependency is narrower than it looks. `--redis-addr` may be empty today, and validation only +rejects that when author attribution or the admission webhook is enabled +([`cmd/main.go`](../../cmd/main.go)). The watch cursor store is already nil-tolerant: with no store, +[`lookupTargetWatchCursor`](../../internal/watch/target_watch.go#L956) returns a miss and the watch +cold-replays on restart, which is correct and only more expensive. + +So Redis is a hard requirement for **attribution and the admission webhook**, and for nothing else. +Making attribution work without it is one seam, not a project. + +One correction belongs with that work: the doc comment on +[`RedisStore`](../../internal/queue/redis_store.go#L45) calls it "a hard dependency in every mode", +which the validation above contradicts. + +### Abstract the capability, not the client + +The seam goes around what the transport *does*, not around Redis. Two methods: + +- **publish** a batch of facts for one `(route, group/resource)`, +- **follow** a set of `(route, group/resource)` from a horizon, yielding entries in append order. + +Everything above that line has exactly one implementation and never learns which transport it has: +the four match structures, the TTL sweep and eviction, the waiter registry, the resolver, and the +collection join. Only the transport is doubled. + +The wrong seam, worth naming because it is the tempting one, is a storage interface with `get` and +`set` that Redis and a map both implement. That reintroduces the per-key lookup this design deletes, +and it would make the in-memory side a cache rather than a log. + +### The in-memory implementation is the same data structure + +This is the argument that the second implementation is honest rather than a stub. + +A Redis stream is a capped, ordered log with per-reader cursors. That is a ring buffer. The +in-memory transport is a per-`(route, group/resource)` ring with a TTL-based trim and one cursor per +follower, which is not an approximation of the Redis behavior, it is the same structure in process +memory. Replay from the horizon on start is reading the ring from its tail. Trim-gap detection is +comparing a follower's cursor against the ring's oldest surviving entry. Both fall out rather than +being simulated. + +That shared shape is what makes a single conformance suite meaningful, and running it against both +implementations is the condition for building this at all. Two transports that are only tested apart +will drift, and the drift will land in whichever one the maintainers do not run daily. + +### Guard rails + +**In-memory means one process, and that has to fail loudly.** The transport only works when the +audit receiver and the resolver are the same process. That is true today, and the chart already +refuses `replicaCount > 1`, so the gate exists. It must stay coupled: the memory transport plus more +than one replica is a configuration error, refused at startup rather than degraded into silent +attribution loss. The same applies if the audit ingress is ever split into its own Deployment. + +**Select it explicitly.** An empty `--redis-addr` currently means attribution is off. Making it also +mean "attribution over an in-memory transport" would change existing installs by inference. A +separate choice, defaulting to the Redis transport, keeps the change opt-in and keeps the +combinations checkable at startup. + +**Redis stays the default when attribution is on.** In-memory is for the single-pod install where +running a StatefulSet to name commit authors is out of proportion to the benefit. It is not the +production recommendation, and the cost is worth stating in the user-facing docs: facts do not +survive a restart, so events in flight across one lose their author. That is already true of any +restart today, which is what keeps the delta small. + +## Code inventory + +| Change | Where | +|---|---| +| Delete the fact key builders, `SET`/`GET` paths, and the SCAN-based size gauge | [`attribution_index.go`](../../internal/queue/attribution_index.go) | +| Delete the collection expander: `RecordDeleteCollectionFacts`, `storeDeleteCollectionFacts`, `deleteCollectionItems`, `deleteCollectionItem`, and their tests | [`attribution_index.go`](../../internal/queue/attribution_index.go), `attribution_index_deletecollection_test.go` | +| Retire §5 and §8 of the expander spec; §2, the deletion-as-intent render rule, is untouched and still binds | [`deletecollection-attribution-expander.md`](../spec/deletecollection-attribution-expander.md) | +| Group per request, one `XADD` per type | [`audit_handler.go`](../../internal/webhook/audit_handler.go) | +| New: the two-method transport seam, plus a Redis-stream and an in-memory implementation and one conformance suite over both | new files under [`internal/queue/`](../../internal/queue/) | +| New: subscription set, in-memory index, waiter registry, all transport-agnostic | new file under [`internal/queue/`](../../internal/queue/) | +| Add the transport selection flag and reject in-memory with more than one replica | [`cmd/main.go`](../../cmd/main.go) | +| Correct the stale "hard dependency in every mode" comment | [`redis_store.go`](../../internal/queue/redis_store.go#L45) | +| Wait on a signal instead of polling; drop `attributionPollInterval` | [`author_resolver.go`](../../internal/watch/author_resolver.go) | +| Subscribe and unsubscribe a type as watches come and go | [`target_watch.go`](../../internal/watch/target_watch.go) | +| Replace the fact-index size gauge; add eviction and trim-gap counters | [`telemetry/exporter.go`](../../internal/telemetry/exporter.go) | + +`AttributionResolutionsTotal` and `AttributionResolutionWaitSeconds` keep their names and meanings, +so the e2e reporting in [`reportAttributionStats`](../../test/e2e/e2e_suite_test.go) and every +existing dashboard keep working across the change. That continuity is deliberate: the wait +histogram is how the improvement gets measured. + +## Rollout + +The fact schema is ephemeral by construction. Facts carry a TTL measured in minutes, nothing reads +them after that, and no user data is stored in them. So there is no migration to write: the new +version stops writing keys and starts appending to streams, and the old keys expire on their own +within `--author-attribution-ttl` of the upgrade. + +The one visible effect is that events in flight across the restart lose their author, which is +already true of any restart today. + +## Open questions + +- **How large may a collection's uid set be before it is dropped?** It bounds one entry's size, the + broadcast to every subscriber, and the replay on restart. The fallback is already correct, so this + is a tuning number rather than a correctness one, but it decides how often the precise path is + taken. +- **How long is the collection window?** Short enough that an unrelated delete in the same namespace + is not claimed, long enough to cover audit batching plus clock skew. The deletion-as-intent rule + means it does not have to cover finalization, so a few multiples of the grace window is the + starting point. It should be measurable from the lab's per-scenario timing report. +- **What replaces the `exact_deletecollection_item` result label?** The match is now scope-based, so + the name is wrong. Renaming it is a visible change to an exported metric's label value, which + needs saying in the release notes even though the metric keeps its name. +- **Cap per type, or one global cap?** Per type is fairer under a burst on one noisy type. A global + cap is simpler and bounds the pod. Probably both, with the per-type cap as the primary. +- **Is one entry per type per request the right granularity?** It matches the apiserver's batching + exactly, but a single entry can then carry hundreds of facts. An entry-size ceiling that splits + oversized groups may be needed. +- **`BLOCK` interval.** It sets how quickly a subscription change takes effect and how often the + reader wakes for nothing. Likely a second, worth confirming against the observed publish rate. +- **Does the per-type stream count stay reasonable?** One `XREAD` across a few dozen streams is + ordinary. A cluster watching several hundred types would want checking before it is assumed. +- **Should the trim-gap counter feed a condition?** A reader that is losing facts is a real + degradation, and it is currently only visible as an unresolved rate. +- **Under HA, must a replica warm its index before starting a watch it has taken over?** Replaying + the type's window is cheap, but starting the watch first would lose attribution for the handover + window. Ordering the two is a small decision with a visible effect, and it belongs with the + ownership work rather than here. diff --git a/docs/design/attribution-wait-poll-vs-push.md b/docs/design/attribution-wait-poll-vs-push.md new file mode 100644 index 00000000..b156d903 --- /dev/null +++ b/docs/design/attribution-wait-poll-vs-push.md @@ -0,0 +1,545 @@ +# Waiting for an audit fact without polling for it + +> **design**: superseded. Index: [`../INDEX.md`](../INDEX.md) +> +> **The decision was taken in [`attribution-fact-stream.md`](attribution-fact-stream.md)**, which +> replaces the fact keyspace with a per-type Redis stream and an in-memory index. This record is +> kept as the reasoning trail: the six options, what each costs, and the measurements that ruled +> most of them out. Read it for why, then read the other one for what. +> +> Nothing here is shipped. The current behavior is the poll loop described under +> [what the wait costs today](#what-the-wait-costs-today); everything from +> [the options](#the-options) onward is intent. +> +> Prompted by a review question: the attribution lookup waits for a fact to appear in Redis, and +> the way it waits is a fixed-interval poll. This record separates the part of that design that is +> forced (the wait) from the part that is a choice (the poll), and lists what could replace the +> choice. +> +> The finding that drives the recommendation is in +> [which one fires first](#which-one-fires-first): audit delivery is batched by the apiserver, so +> the watch event arrives first by roughly the batch window and the first lookup is a +> near-guaranteed miss. Two measurements already in the repository support it. + +## The decision to make + +A live watch event needs a commit author before it can be routed, and the audit fact that names +that author may not have arrived yet. Waiting is unavoidable. Polling Redis every 150ms for up to +three seconds, on the watch shard's own goroutine, is one way to wait, and it is the expensive one. + +The question is whether to keep it, tune it, or replace it with a notification. + +## Why the shape of this architecture is worth keeping + +Before criticizing the wait, it is worth being explicit about what the surrounding design gets +right, because every option below is constrained by wanting to preserve it. + +**Attribution is a layer on top of the watch, not a rewrite of it.** The mirroring path is complete +without it. A watch event carries the object, the operation, and the resource identity, and it +produces a correct commit whether or not anyone ever names an author. +[`RedisStore`](../../internal/queue/redis_store.go#L45) holds the resume cursors and is a hard +dependency in every mode; +[`AttributionIndex`](../../internal/queue/attribution_index.go#L112) is built on the same connection +only when the operator asks for author attribution, and it +[knows nothing about cursors](../../internal/queue/redis_store.go#L79). Turning attribution off is +expressed by leaving `Manager.AuthorResolver` nil, at which point +[`attachAuthor`](../../internal/watch/target_watch.go#L748) returns immediately and the commit is +authored by the configured committer. There is no second code path for the unattributed case, no +feature flag threaded through the writer, and no degraded mode to test separately. The enhancement +is optional in the strong sense, and the cost of not using it is zero. + +**One fact serves every watcher that needs it.** A fact key is +`route:::object::` +([`factKeyExact`](../../internal/queue/attribution_index.go#L454)). Notice what is absent from it: +the `GitTarget`, the `WatchRule`, the branch, the folder. The key names the *write that happened in +Kubernetes*, and it says nothing about which consumer is interested. So when five `GitTarget`s +mirror the same `Deployment` into five repositories, the API server posts one audit event, +ingestion stores one fact, and all five watch shards join that same key independently and each +stamps the same author on its own commit. Adding a sixth `GitTarget` adds no work on the write side +at all. + +That property is what makes the audit route the correct partition rather than the provider name, +which is the whole argument of +[`attribution-fact-identity.md`](attribution-fact-identity.md): several `ClusterProvider`s naming +one physical cluster deliberately share one route so they share its facts. Fan-in on a shared key +is what the schema is for. + +Both properties survive every option below, and the push option in +[option D](#option-d-publish-from-the-audit-receiver) actively exploits the second +one: a single notification on a shared key wakes every waiter on it, for the same reason a single +`GET` on that key serves every reader. + +## Why a wait is unavoidable + +The author fact and the watch event are two independent deliveries out of the same kube-apiserver. +One travels the audit webhook backend as an HTTP POST to +[`audit_handler.go`](../../internal/webhook/audit_handler.go#L312); the other travels the watch +stream into [`routeLiveTargetWatchEvent`](../../internal/watch/target_watch.go#L692). Nothing +orders them against each other, and the watch event frequently wins. + +The wait also cannot be deferred until after routing, which is the obvious alternative. The branch +worker groups events into a commit window keyed by author: +[`canAppend`](../../internal/git/branch_worker.go#L730) force-finalizes the open window as soon as +the incoming author differs from the window's. An event routed with the author still unknown would +either split the window immediately or need its author patched in afterward, and patching it means +rewriting a commit that may already be pushed. Waiting briefly before shipping is what makes "a late +audit arrival must not rewrite a shipped commit" enforceable, which is exactly what the constant +says it is for +([`DefaultAttributionGraceWindow`](../../internal/watch/author_resolver.go#L21)). + +So the three-second grace stays. Only the mechanism inside it is open. + +Ordering between events is a separate question and it is already settled in +[`watch-event-ordering-and-attribution-grace.md`](../facts/watch-event-ordering-and-attribution-grace.md): +the wait cannot reorder the events of one object or one type, because each +`(GitTarget, GVR, scope)` watch is a single goroutine that finishes one event before reading the +next. Read that record before touching option F, which is the non-blocking variant it sketches. + +## Which one fires first + +The watch event, nearly always, and the margin is not small. This is structural rather than +incidental, and it is the single most important input to the options below. + +The watch is a streamed push: the apiserver writes the event to an open connection as soon as the +write commits. The audit event is **batched**. `--audit-webhook-mode=batch` is what the +[attribution setup guide](../attribution-setup-guide.md) tells operators to configure, and batch +mode holds events until either `--audit-webhook-batch-max-size` events have accumulated or +`--audit-webhook-batch-max-wait` elapses. The batching parameters belong to the apiserver, not to +this operator, so the delay is not ours to remove. + +The consequence for the resolver: **the first `GET` in `ResolveAuthor` is a near-guaranteed miss**. +The poll loop is not an exceptional path taken under load, it is the normal path taken by every +attributable event. That inverts the usual instinct about a retry loop, where the first attempt +usually succeeds and the retries are the rare case. + +[`configuration.md`](../configuration.md) already records the operational half of this as a warning +on `--author-attribution-grace`: the apiserver's `--audit-webhook-batch-max-wait` delays every fact +by up to that much, so a grace at or below it loses actors systematically. Worth checking against +the upstream default for that flag, which is considerably larger than the one-second value the e2e +cluster uses, and larger than this operator's three-second default grace. There is no Kubernetes +checkout under `external-sources/` to source-verify it here, so treat it as a number to confirm +before it is written down as a fact. + +## What has already been measured + +Two measurements exist in the repository today, and they agree. + +**The e2e suite reports the wait distribution on every run.** +[`reportAttributionStats`](../../test/e2e/e2e_suite_test.go) queries +`gitopsreverser_attribution_resolutions_total` by `result` and prints the +`gitopsreverser_attribution_resolution_wait_seconds` histogram, split into resolved and absent +because the two populations answer different questions. It also prints how many resolutions +succeeded only because e2e widens the grace past the three-second default, which is a direct +measure of how much headroom the flag buys. + +The finding is written into the comment above it: the e2e cluster runs +`--audit-webhook-batch-max-wait=1s` ([`start-cluster.sh`](../../test/e2e/cluster/start-cluster.sh)), +and a healthy run puts most waits in the 0.5 to 2 second range. Fact delivery is bounded below by +the batch window, exactly as the section above predicts. + +**The corpus records which events have no audit event at all.** Several rows of the mutation-capture +lab corpus are documented silences rather than gaps: a status subresource update produces two watch +events and **no** audit event, and a graceful pod delete produces watch `MODIFIED` plus `DELETED` +and **no** audit event ([`test/mutationlab/README.md`](../../test/mutationlab/README.md), rows 5 and +7). Those events can never resolve no matter how long the resolver waits. They are a structural +population that burns the entire grace window and then ships unresolved, which is a stronger version +of the problem [option C](#option-c-circuit-break-a-route-that-has-never-resolved-anything) +addresses per route. + +## What the mutation-capture lab can add + +The lab is the right instrument for the part the Prometheus histogram cannot answer, and it needs +only a small addition. + +It already has what matters. Every recorder stamps `ObservedAt: time.Now()` on its record +([`record.go`](../../internal/mutationlab/record.go), and the four recorders under +[`internal/mutationlab/recorder/`](../../internal/mutationlab/recorder/)), all in one process on one +clock, and every record carries an `ObjectKey` with the uid and resourceVersion that correlate the +watch event with the audit event for the same write. The skew this whole design is about is +therefore already captured on every lab run. It is discarded on the way to the corpus, because the +normalizer replaces timestamps with `` so the golden files stay deterministic. + +So the addition is a **separate, non-golden timing report**: per scenario, the delta between the +audit record's `ObservedAt` and the watch record's `ObservedAt` for the same `(uid, rv)`. It must +not enter `corpus/`, which exists precisely to be byte-stable across runs. + +What that buys over the aggregate histogram: + +- **Per-scenario skew instead of one blended distribution.** A create, a server-side apply, and a + finalizer delete may sit in different places relative to the batch window, and the Prometheus + histogram blends them. +- **The sign of the skew, confirmed per scenario** rather than inferred. If any scenario ever shows + the audit record arriving first, that is worth knowing, and no current measurement would show it. +- **The structural silences priced.** Rows 5 and 7 have no audit record to subtract, so they appear + as "no fact, ever" rather than as a large delta. That distinction is exactly the one + `AttributionAbsent` cannot make today, since an aged-out fact and a fact that never existed are + indistinguishable by design. +- **A regression signal on a Kubernetes upgrade.** The corpus is already the behavioral changelog + for a version bump. A timing report alongside it would show a batching or delivery change in the + same review. + +The honest limits: the lab and the product deliver differently enough that the absolute numbers do +not transfer. The lab records at handler entry rather than after ingestion, it runs on the e2e +cluster's deliberately aggressive batch profile (which +[`audit-webhook-api-server-connectivity.md`](../facts/audit-webhook-api-server-connectivity.md) +states is a test feedback optimization and not production advice), and it does not write to Redis at +all. It answers "which one fires first, and by roughly how much, per scenario". It does not predict +a production install's numbers, and the flag values below should still come from the e2e histogram +and from real installs. + +## What the wait costs today + +[`ResolveAuthor`](../../internal/watch/author_resolver.go#L160) loops: look up, and if the result is +`AttributionAbsent`, sleep `attributionPollInterval` (150ms) and look up again until the grace +expires. + +Each iteration is not one Redis round trip. +[`LookupAuthorResolution`](../../internal/queue/attribution_index.go#L382) tries up to three keys: +the immutable exact key, the `:last` pointer for removals, and the type-scoped rv-only hatch. So a +single event that never resolves costs roughly 20 wakeups and 40 to 60 `GET`s, and an event whose +fact lands mid-grace pays an average of 75ms of pure poll-interval latency on top of however long +the fact took to arrive. + +The larger cost is where the loop runs. +[`attachAuthor`](../../internal/watch/target_watch.go#L729) is called inline in the shard's event +loop, before `RouteToGitTargetEventStream` and before +[`recordTargetWatchCursor`](../../internal/watch/target_watch.go#L689). The grace is not a +background wait. It head-of-line blocks that entire `(GitTarget, GVR, namespace)` shard, so a burst +of 50 unattributed events on one shard serializes into 150 seconds during which the shard reads +nothing from its watch channel. + +```mermaid +flowchart TB + subgraph SRC["Source cluster · one kube-apiserver"] + API["Kubernetes API
write to ConfigMap web
uid U · rv 101"] + end + + subgraph OP["Operator"] + AH["audit_handler.go
RecordFact"] + WS["target_watch.go
watch shard goroutine"] + AR["author_resolver.go
ResolveAuthor"] + BW["branch_worker.go
commit window keyed by author"] + end + + FACT[("route:prod-eu-1:configmaps
:object:U:101
author alice")] + + API -->|"audit POST · path 1 · BATCHED
held up to --audit-webhook-batch-max-wait"| AH + AH -->|SET with TTL| FACT + API -->|"watch event · path 2 · STREAMED
arrives first, nearly always"| WS + WS -->|"blocks the shard here"| AR + AR -.->|"GET, absent"| FACT + AR -.->|"sleep 150ms, GET again, up to 20 times"| FACT + AR -->|"author, or unresolved after 3s"| WS + WS --> BW + + OTHER["four more GitTargets
watching the same object"] -.->|"the same key, no extra write"| FACT + + classDef cost fill:#ffe6e6,stroke:#cc3333,color:#000; + classDef good fill:#e6f7e6,stroke:#33aa33,color:#000; + class AR cost; + class OTHER,FACT good; +``` + +The red box is the only thing under discussion. The green path is the fan-in property from +[the section above](#why-the-shape-of-this-architecture-is-worth-keeping), and it is not a problem +to solve. + +Two distinct problems hide in the red box, and they need separating because most options address +only one: + +1. **Wasted round trips and added latency** while waiting for a fact that will arrive. +2. **Head-of-line blocking** for the full three seconds when the fact will never arrive. + +## The options + +### Option A: measure, then leave it alone + +The resolver already emits everything needed to price this problem: +[`recordAttributionResolution`](../../internal/watch/author_resolver.go#L213) records +`AttributionResolutionsTotal` broken down by `result` and `AttributionResolutionWaitSeconds` as a +histogram, and the e2e suite already reads both. + +This was the natural first option before the batching floor was written down. It is much weaker now. +"The poll loop rarely runs a second iteration" was the outcome that would have justified stopping +here, and the measured 0.5 to 2 second waits rule it out: with audit delivery batched, roughly every +attributable event runs the loop to completion. + +What is still worth measuring is the *split*, which decides between the options rather than against +them: how much of the population resolves late (option D territory) against how much never resolves +at all (option C and the structural silences). + +### Option B: shift the first re-check to the delivery floor + +The original form of this option was a decaying backoff starting fast, on the theory that a fact +that is going to arrive arrives almost immediately. The batching floor says otherwise, so a +sequence like 25ms, 50ms, 100ms spends its cheapest attempts in the window where a fact provably +cannot have arrived yet. + +The corrected shape is the opposite: **wait before looking, then look often**. Skip the first +re-check until roughly the observed delivery floor, then poll on a short interval through the +window where facts actually land. + +The floor is not configured in this operator and cannot be read from the apiserver, so it has to be +learned. `routeAttributionHealth` already holds per-route state and is the natural place to keep a +cheap running estimate of that route's resolved-wait percentile, which the resolver then uses as +its first-check delay. A route whose facts land at 1.2 seconds stops paying for eight useless +lookups per event. + +Cost: still small, and contained inside the resolver. It is no longer the obvious first move it +looked like, because a learned first-check delay is most of the way to admitting that a +notification would be better. + +### Option C: circuit-break a route that has never resolved anything + +[`routeAttributionHealth`](../../internal/watch/author_resolver.go#L108) already tracks, per audit +route, whether attribution has ever resolved and how many events have gone unresolved since. Today +it is used for exactly one thing: logging a warning once, in +[`warnIfRouteNeverResolves`](../../internal/watch/author_resolver.go#L237). + +The same signal can gate the wait. Once a route crosses the unresolved streak threshold having never +resolved a single event, collapse its grace to a single lookup. A route nobody posts to currently +costs three seconds of shard blocking and dozens of `GET`s per event indefinitely; this turns it +into one `GET`, while the existing warning keeps telling the operator what to fix. + +The recovery rule matters: the streak counter is already cleared on any resolution +([`observe`](../../internal/watch/author_resolver.go#L119)), so a route that starts working restores +its full grace on the first fact it resolves. The breaker must therefore keep taking one real lookup +per event rather than skipping the lookup entirely, which it does. + +This is the only cheap option that fixes head-of-line blocking, and it fixes it exactly in the case +where the blocking is unbounded rather than incidental. + +The corpus silences argue for a second breaker beside it. A status subresource update and a graceful +pod delete produce no audit event at all, so those events pay the full grace on a route that is +otherwise healthy, which the per-route breaker will never trip on. A per-`(route, resource, +operation)` variant of the same counter would catch them. That is a larger change and it needs the +lab's per-scenario report to size it, which is one concrete reason to build the report first. + +### Option D: publish from the audit receiver + +Turn the wait from a poll into a push, driven by the code that receives the audit events. + +1. [`writeFactKeys`](../../internal/queue/attribution_index.go#L201) pipelines a `PUBLISH` of each + written key onto a per-route channel alongside its `SET`. Pipelined, so the write side pays no + extra round trip. See [where to publish from](#where-to-publish-from) for the better placement. +2. The resolver process holds one long-lived subscription per audit route. Routes are bounded by + the number of `ClusterProvider`s, so this is a handful of connections for the whole process. The + subscriber fans out into a `key -> waiters` registry. +3. `ResolveAuthor` registers its waiters **before** performing the existing lookup, then selects on + the notification, `ctx.Done()`, and the grace deadline. +4. A coarse re-check stays as a safety net, for example one at the deadline. + +Point 3 is the entire correctness argument. Registering after the lookup loses any fact published +in the gap between the two, which is precisely the window that matters. Point 4 is the other half: +Redis pub/sub is at-most-once and drops messages on reconnect or failover, so the fallback ensures a +lost notification degrades to today's behavior rather than to a hang. + +The transport is available without changing anything else. +[`NewRedisStore`](../../internal/queue/redis_store.go#L56) builds a plain `redis.NewClient`, not a +cluster client, so ordinary `PUBLISH` and `SUBSCRIBE` apply with no shard-routing caveat. Writer and +reader are frequently different pods (the audit POST arrives through a Service while the watch shard +lives wherever it lives), which is why an in-process channel cannot substitute. + +This option composes with the fan-in property: one publish on a shared key wakes all five waiting +`GitTarget`s at once, for the same reason one `GET` served all five before. + +Cost: a subscriber lifecycle to own and reconnect, a waiter registry, and a new dependency on a +Redis feature the deployment has not used so far. Benefit: round trips per unresolved event drop +from dozens to roughly zero, and resolution latency for a late fact drops from 75ms of poll jitter +to about one round trip. + +The batching floor is what promotes this option. When the loop was assumed to exit on its first or +second lookup, a subscriber lifecycle was a lot of machinery for a rare path. With delivery batched, +every attributable event runs the loop for most of a second or more, so the dozens of saved round +trips are the steady state rather than the exception. This is where the wasted work sits. + +It does **not** fix head-of-line blocking. An event whose fact never arrives still holds its shard +for the full grace, and it still holds it for the delivery latency when the fact does arrive. + +#### Why the batching floor also makes the push reliable + +Publish and subscribe has one structural weakness: it is at-most-once, and a message only reaches +subscribers that are subscribed **at the moment it is published**. A late subscriber gets nothing. +That is normally the reason to be careful with it. + +Here that weakness is mostly canceled by the ordering measured in +[which one fires first](#which-one-fires-first). The watch event arrives first, by roughly the batch +window, and the resolver blocks inline the moment it arrives. So by the time the audit POST is +received and the fact is published, **the waiter is already waiting**. The one arrival order that +would defeat the push is the one the apiserver's batching makes rare. + +This is the argument that promotes the push from "a caching trick" to the natural shape of the +system. The push is aimed at exactly the window the measurements say the events land in. + +The residual cases are real and are what Redis stays for: an audit event that overtakes its watch +event anyway, a watch shard that starts after the fact was published, a replica whose subscription +was reconnecting, and a `GitTarget` that begins watching a type mid-grace. Each of those is a +late-join, and a late-join is precisely what a TTL'd key in Redis serves. + +#### Carry the fact, not a pointer to it + +The step above publishes the *key* and has the woken resolver `GET` it. Publishing the +[`AuthorFact`](../../internal/queue/attribution_index.go#L90) itself is better, and it is barely +more work. + +- **The woken resolver needs no Redis read at all.** The message carries the author, display name, + and email, which is everything [`userInfoForResolution`](../../internal/watch/author_resolver.go#L198) + reads. In the common case the join costs zero round trips rather than one. +- **It removes an ordering constraint on the write side.** A notify-only publish must happen + strictly *after* the `SET`, or the woken reader races back to a key that is not there yet. A + payload-carrying publish has no read-after-write dependency, so it can be issued as early as the + receiver likes. +- **A fact is small.** The struct is a dozen short string fields, and only accepted, mutating, + author-bearing events produce one. + +The cost is that a message is broadcast to every subscriber on the route, so every replica receives +every fact whether or not it is waiting for that object, where today each replica reads only the +keys it wants. The comparison is broadcast volume (audit event rate times replica count) against +saved lookups (watch event rate times polls per event). With polls per event in the high single +digits and replica counts small, the push still wins comfortably. It stops winning at a high replica +count, which is the number to watch if this is built. + +#### Where to publish from + +Two placements, and the receiver is the better one. + +[`writeFactKeys`](../../internal/queue/attribution_index.go#L201) knows exactly which keys it wrote, +which makes it the obvious home for a notify-only publish. But it sees one fact at a time, so a +`deletecollection` expanding into N facts +([`storeDeleteCollectionFacts`](../../internal/queue/attribution_index.go#L283)) becomes N publishes. + +The receiver sees the whole batch. One audit POST carries an `EventList` of up to +`--audit-webhook-batch-max-size` events, decoded once in +[`serveEventListRequest`](../../internal/webhook/audit_handler.go#L186) and looped through +[`processEvent`](../../internal/webhook/audit_handler.go#L289). Publishing there means **one message +per POST carrying every fact from that batch**, which collapses a batch of 400 events into a single +publish instead of 400. The batching that causes the delay is the same batching that makes the +notification cheap. + +The constraint on that placement is that the receiver must publish only what +[`RecordFact`](../../internal/queue/attribution_index.go#L132) stored. That function drops +events with no `objectRef`, no resolvable name, or no user, and expands a `deletecollection` into +per-item facts. Publishing the raw `EventList` would notify waiters about facts that do not exist +and cannot name anyone. So `RecordFact` needs to return what it wrote, and the receiver accumulates +those across the batch and publishes once. That return value is the only real change to existing +code. + +### Option E: keyspace notifications instead of an explicit publish + +Redis can emit `__keyevent@__:set` notifications by itself, giving option D's push without +touching the write path. + +Rejected as the primary mechanism, for three reasons. It requires `notify-keyspace-events` to be +enabled server side, which a managed Redis or Valkey may not permit and which the chart does not +control. It is unfiltered: a process-wide subscription receives every `SET` on the connection, +including watch cursors and command-author records, to be discarded by prefix on the client. And it +can only ever announce a key, never a fact, so it forfeits the zero-lookup join that +[carrying the payload](#carry-the-fact-not-a-pointer-to-it) buys. + +Publishing from the receiver is the same idea with none of those three costs, because the receiver +already holds the decoded events and knows which facts it stored. Keyspace notifications are worth +keeping in mind only if the write side ever needs to stay on an older version than the read side. + +### Option F: move the wait off the shard's critical path + +Buffer the event, let the shard continue reading its watch channel, and resolve attribution in a +separate stage. + +This is not a new idea and it should not be redesigned from scratch. The final section of +[`watch-event-ordering-and-attribution-grace.md`](../facts/watch-event-ordering-and-attribution-grace.md) +already specifies it: a per-`(GVR, scope)` ordered pipeline that starts the lookup immediately and +holds an event until its grace expires **or** every earlier event on that watch has shipped, using a +sequence-numbered reassembly buffer. That record also carries the warning that makes the design +non-optional rather than a nicety: parallelizing attribution per event *without* the reassembly +buffer lets a fast-matched later event overtake a slow earlier one on the same object, which breaks +same-object ordering and can let an older mutation win. + +That record deferred it explicitly, "until measurements say otherwise". The batching floor is the +measurement. The head-of-line stall is not an occasional cost paid by unattributable events, it is +paid by every attributable event, for the length of the apiserver's batch window, on a +single-threaded shard. Under the upstream default `--audit-webhook-batch-max-wait` rather than the +e2e cluster's one second, that is a serious throughput ceiling on a busy type. + +The cursor is recorded after routing +([`processLiveTargetWatchEvent`](../../internal/watch/target_watch.go#L671)), so the reassembly +buffer has to preserve that too: a cursor may only advance for an event that has shipped, +otherwise a restart skips the events still held in the buffer. + +Still the largest change here, and still not the first thing to build. It is now the structural fix +rather than a curiosity, so it should be repriced once the split from option A is known. + +## What the push does to commit order + +Nothing, and that is the point. It then improves commit fidelity as a side effect. + +Ordering is not a property of the attribution mechanism. It is a property of the watch shard, which +is one goroutine per `(GitTarget, GVR, scope)` that finishes one event, wait included, before it +reads the next one from the channel, and of the single FIFO `eventQueue` the branch worker drains. +That chain is what +[`watch-event-ordering-and-attribution-grace.md`](../facts/watch-event-ordering-and-attribution-grace.md) +establishes, and none of options B through E touches any part of it. Attribution decides *what +author is stamped* on an event. It never decides *when the event is routed relative to its +siblings*. A push shortens a wait that happens inside an already-serialized loop. + +The side effect is a real improvement, and it works in favor of commits that read like the sequence +of changes that happened. + +An open commit window accepts one `(author, GitTarget)` pair at a time, so +[`canAppend`](../../internal/git/branch_worker.go#L730) finalizes the window whenever the incoming +author differs. Today an event whose fact arrives a moment after the grace expires ships as +attribution-unresolved while the event on either side of it resolves to a real actor. That single +timeout splits one window into three commits, and the split carries no meaning: the three events +were one person's change, and the middle commit is attributed to nobody because a batch was a few +hundred milliseconds late. Resolving from a push instead of from a race against a deadline removes +most of that class. Same order, fewer commits, and each one attributed to the actor who caused it. + +**The one change that would break order** is worth stating plainly next to a design that makes it +tempting. Once waits become short, resolving several events concurrently to reclaim the shard looks +free. It is not: a fast-matched later event would overtake a slow earlier one on the same object, +and the older mutation would win in Git. That is the explicit warning in the ordering record, and it +is why [option F](#option-f-move-the-wait-off-the-shards-critical-path) is specified with a +sequence-numbered reassembly buffer rather than as plain concurrency. Push the notification, keep +the shard serial. + +## Comparison + +| Option | Fixes round trips | Fixes latency | Fixes shard blocking | Size | +|---|---|---|---|---| +| A: measure only | no | no | no | none | +| B: first check at the delivery floor | mostly | no | no | one function | +| C: route circuit breaker | for a dead route | for a dead route | for a dead route | small | +| D: publish from the receiver | yes, to zero | yes | no | moderate, new subsystem | +| E: keyspace notifications | to one lookup | yes | no | moderate, plus a config dependency | +| F: reassembly buffer off the hot path | no | no | yes | large | + +Note what the second column now says about B. Shifting the first check to the delivery floor saves +lookups but cannot make a fact arrive sooner, so it does not improve latency. Only a notification +does, and only F removes the stall. + +## Recommended sequence + +1. **Build the lab timing report.** It is the smallest piece of work here, it produces the + per-scenario number that prices everything else, and it turns a Kubernetes upgrade's timing + change into something the corpus review would catch. +2. **Read the e2e histogram for the split**, resolved-late against never-resolved. Those two + populations point at different options and the aggregate total does not separate them. +3. **C, on its own.** Small, uses state that already exists, and it is the only cheap fix for the + unbounded case. Ship it regardless of what the numbers say. +4. **Then D or F, decided by the split.** A large resolved-late population makes D the answer: + most of that wait is real delivery latency and a notification collects it at the earliest + possible moment. A large never-resolved population, especially concentrated in the structural + silences, makes F the answer: no notification is ever coming for those events, so the only + remaining fix is to stop blocking the shard while waiting for them. +5. **B as a fallback**, if neither D nor F is affordable soon. It reduces the waste without + addressing either underlying problem. + +D and F are complementary rather than alternatives, and building D first is the better order. D +shrinks the wait for every event that has a fact, which shrinks the population F has to hold in its +buffer, and it leaves the shard serial while it does so. F then handles what is left, which is the +events that have no fact at all. + +The revision that matters relative to the first draft of this record: the batching floor moves the +work from "tune the loop" toward "stop looping", and it makes F structural rather than optional. +Both conclusions came from measurements that already existed in the repository. The receiver-side +publish then falls out of the same finding, because the arrival order that makes the poll wasteful +is the arrival order that makes the push land on a waiter that is already there. diff --git a/docs/design/open-asks-priority.md b/docs/design/open-asks-priority.md new file mode 100644 index 00000000..01e51b31 --- /dev/null +++ b/docs/design/open-asks-priority.md @@ -0,0 +1,346 @@ +# What to build next: the open asks, ordered + +> **design**: a priority call, not a plan of record. Nothing here binds until scheduled. +> Index: [`../INDEX.md`](../INDEX.md) +> Date: 2026-07-28. Written against `v0.40.1` / `main`. +> +> Three backlogs are open at once and they overlap: the gitops-api consumer asks (revision 11, +> 2026-07-28), the maintainer review's unbuilt block in +> [`flux-maintainer-review-status-and-config-model.md`](../future/flux-maintainer-review-status-and-config-model.md) +> (F6, F9, F10), and the config-surface proposal in +> [`config-surface-for-a-structured-repository.md`](../future/config-surface-for-a-structured-repository.md) +> (B1–B6). This page merges them into one queue and says where we deliberately do **not** do +> what was asked. +> +> One of the queue's entries is already specified rather than merely wanted: +> [`attribution-fact-stream.md`](attribution-fact-stream.md) picks and details the replacement for +> the attribution keyspace. It is ranked here like everything else, and it changes how the +> highest-priority consumer ask should be built. See +> [attribution facts as a stream](#attribution-facts-as-a-stream-tier-2-and-it-answers-15s-hard-part). + +## The ordering rule + +Four tests, applied in order. They are what produced the queue in +[the queue](#the-queue), and they are the part worth arguing with: + +1. **Does it stop the product being silently wrong?** Attribution is the product. A + misconfiguration that mirrors perfectly and authors every commit `unknown` outranks + everything, because nothing surfaces it until the commits already exist. +2. **Does it get cheaper by being done now?** Every spec-field change is free while we are + `v1alpha3` and the consumer count is one. gitops-api pins us three ways (image, Go module, + `require` line), so the breaking wave costs one coordinated bump today and N tomorrow. +3. **Is a deletion available instead of a switch?** A feature that needs an off-switch is a + feature with a design problem. Removing it is smaller than the enum that guards it, and it + removes the enum too. +4. **Does it make the output legible?** This product's entire output is a Git diff. Things that + make the diff unreadable are not cosmetic here, whatever they would be elsewhere. + +Test 3 is why this document exists rather than a straight re-ranking of the asks, and it lands +on sibling inference. It has a second instance, arrived at independently: +[`attribution-fact-stream.md`](attribution-fact-stream.md) deletes the fact keyspace and the +`deletecollection` expander rather than optimizing either, and ends up with less code doing more. +Two of these in one quarter is a pattern worth naming: the parts of this system that hurt are the +parts that reconstruct something from state they do not own. + +--- + +## The one real design call: delete sibling inference, do not switch it off + +The config-surface proposal's **B3** offers `spec.placement.mode: Infer|Declared|Strict`: an +enum that lets a user turn inference off. This document argues the opposite: **remove Option C's +cohort ladder entirely**, keep the kustomize-root fallback, and ship no enum at all. + +### What inference is, precisely + +[`gittarget-new-file-placement-rules.md`](../spec/gittarget-new-file-placement-rules.md) Option C. +It fires **only** for a resource that has no document in Git yet; everything already written is +match-first and never moves. For that narrow case it finds the largest cohort of similar existing +documents (step 1: same type + namespace; step 2: same type, any namespace) and puts the new +document in that cohort's directory, appending to a bundle if the cohort is bundled. Roughly +`resolveInferred` through `allSameDir` in +[`internal/manifestanalyzer/placement.go`](../../internal/manifestanalyzer/placement.go): about a +third of that file, plus its tests. + +### Why it should go + +**It makes a human's edit to the repository change the operator's behavior, with no Kubernetes +object changing and nothing in status recording the move.** That is the finding already written +down as C3 in the config-surface doc, and it is the whole objection in one line. Delete enough of +one namespace's ConfigMaps from a bundle and the bundle stops being namespace-agnostic, so the +next new ConfigMap takes a different path. The GitTarget did not change. Nobody approved it. No +condition mentions it. + +**Its central guard has already failed once, and it failed by cascading.** The +namespace-agnosticism check was, for a period, vacuous on the singleton branch: one directory +holding one namespace satisfied "all the same directory" trivially, so a new namespace's object +was appended into the first namespace's file, which then genuinely spanned two namespaces, which +legitimized the bundle for every later object, which collapsed a whole type into one file. Fixed +on `feat/watchrule-source-namespace-pr4`, and the fix is right. The lesson is not "that bug is +gone"; it is that a rule inferred from mutable state has failure modes that *feed themselves*, +and no amount of care makes the class safe. + +**The explainability it requires was declared mandatory and never built.** The spec's own P8 says +the scan/dry-run output **must** state, per new resource, the chosen path plus the cohort and +ladder step that produced it: "without that, 'why did it land there?' is unanswerable". There is +no such trace, no status surface, and the placement skips that do exist are a log line and a +counter. We shipped the inference and not the thing that made it defensible. + +**What it actually buys is smaller than the demo suggests.** "Point me at an existing repo and it +just works" is carried overwhelmingly by *match-first*, not by inference: every resource that +already has a document is edited exactly where it lives, forever. Inference only fires for a type +or namespace the target has never written. That is a rare event, and it is precisely the event +where a wrong guess is invisible, because there is no prior file to compare against. + +**And the case it cannot handle is the case people have.** P4 in the spec: a custom +per-namespace layout cannot be extended to an unseen namespace, because inference refuses to +reverse-engineer the path segment. So the layout most likely to be hand-authored is exactly the +one inference drops to canonical. The user has to declare it anyway. + +### What we keep + +**The kustomize-root fallback stays.** It is not inference: it is a structural fact. If the +scanned subtree is governed by exactly one supported kustomization, a new file that is not +reachable from that root is not merely oddly placed, it is *unreachable*; it would never be +rendered. Placing it beside that root and adding the `resources:` entry follows from there being +one root, not from picking the largest matching cohort. Deleting it would reintroduce the bug it +was added to fix. + +**`placement.byType` + `placement.default` stay.** The B2 map is already shipped and is the +declared answer for every layout inference cannot reach. + +**Match-first stays**, untouched. This changes nothing about existing documents. + +### What follows from it + +- **Ask #10 dissolves.** The reported symptom (two tenants' `infra` objects, one landing in the + other's directory, so `git log -- tenants/tenant-acmelive/` shows another tenant's history) is + not a missing namespace term in the cohort key. Adding one would be `mode: Declared` reached by + a slower route: every namespace-segmented layout would fall to canonical anyway, which is what + deleting the ladder does directly. We answer #10 with the deletion, not with the patch. +- **B3's enum is never built.** There is nothing left to switch off, so the API surface shrinks + by an enum instead of growing by one. This is the trade worth naming: B3 costs a permanent + three-valued field in a CRD; deletion costs a one-time behavior change. +- **`status.layout` (B2) becomes more valuable, not less.** With inference gone, what remains + invisible is what the operator *understood* about the folder. That is a projection of facts the + analyzer already computes, and it is honest in a way an inference never was. + +### The cost, stated plainly + +A brownfield repo with a bundle layout that today gets a new type appended to the bundle will, +after this, get a canonical path unless the user writes one `byType` line. That is a real +regression for the demo and a real gain for predictability. Cold-start repos are unchanged: +inference already fell to canonical there. It is a behavior change for existing targets, so it +needs a `docs/UPGRADING.md` entry, and it is cheapest now, while the user count makes "one +`byType` line" a sentence in a release note rather than a migration. + +**Open, and worth deciding before writing code:** whether the deletion lands with a +`PlacementFellBackToCanonical`-style Event on the first new type per target, so the user learns +they need a `byType` line at the moment it matters rather than by noticing a file. + +--- + +## The queue + +| # | Ask | Source | Tier | +|---|---|---|---| +| 22 | `ReasonRefusedStructural` doc says "permanent"; refusal-detail stem; `Actor` reachability | gitops-api | **0** | +| 15 | A declared `auditRoute` with zero facts must say so | gitops-api | **1** | +| n/a | Delete sibling inference (answers #10) | this doc | **1** | +| n/a | Attribution facts become a stream; the keyspace and the expander are deleted | [`attribution-fact-stream.md`](attribution-fact-stream.md) | **2** | +| F6 | `spec.suspend`, `spec.interval`, `requestedAt` | maintainer review | **2** | +| 5 | `CommitRequest.spec.author`, SAR-guarded | gitops-api (#220) | **2** | +| B4 | `commitWindow` / `commit.message` move to GitTarget | config surface | **2** | +| B1 | `GitTarget.spec.mode: Observe\|Write` | config surface | **2** | +| 6 | Movable destination via `status.observedDestination` | gitops-api (#220) | **2** | +| F10 | CommitRequest TTL / ownerRef + the `delete` verb | maintainer review | **2** | +| 11 | One encoder for `[CREATE]` and `[UPDATE]` bodies | gitops-api | **3** | +| B2 | `GitTarget.status.layout` | config surface | **3** | +| F9 | The `scope: Namespaced` status-write envtest | maintainer review | **3** | +| B6 | The `default` ClusterProvider not-found message | config surface | **3** | +| 10 | Namespace-aware sibling inference *as asked* | gitops-api | **declined** | +| B3 | `spec.placement.mode` enum | config surface | **declined** | + +### Tier 0: correct what is false, this week + +**#22 is three separate things and all three claims check out against `main` today.** They are +worth doing immediately because each one is a sentence and each one is currently misleading a +consumer that reads our source as the contract. + +- **The doc comment.** `ReasonRefusedStructural` is documented as "the permanent support + boundary" in [`pkg/manifestanalyzer/repo.go`](../../pkg/manifestanalyzer/repo.go), while + `refusedStructuralReason` in + [`internal/manifestanalyzer/scan_repo.go`](../../internal/manifestanalyzer/scan_repo.go) sets + `Solvable` from `classifyKustomizeFeatures` and can return `true`. The code is right; the + comment is a year-old summary of a boundary that has since acquired a `Solvable` field. Delete + "permanent" and point the reader at `Solvable`. The consumer wrote `Permanent = true` off that + sentence and shipped "can never be synced" for a folder whose author had a typo to fix. + Telling a user "never" when the answer is "not yet" is the worse of the two lies. +- **The refusal-detail stem, and we should go further than asked.** `refusedStructuralDetail` + builds one stem ("kustomization uses unsupported feature(s): …") for both branches, so a + solvable refusal is described as an unsupported feature. The parenthetical from + `kustomizationDecodeError` is the useful half. The ask is a second stem for the solvable + branch; the better shape is to derive the stem from the same classification that sets + `Solvable`, so the two cannot drift the way the doc comment did. One function, two stems, one + input. +- **The `Actor` question, answered: yes, it is structural.** `AcceptancePolicy.InScope` is + assigned in exactly two places in the tree, both tests. `folderScanPolicy` leaves it nil and + `ScanRepo` never sets it, and `mappingRefusal`'s `IssueOutOfScope` is gated on it being + non-nil, so neither `ScanFolder` nor `ScanRepo` can emit `ActorPlatformOperator`. A consumer's + platform-operator branch is dead code today. The right fix is not only the doc line they asked + for: `Actor`'s doc should state which scan kinds can produce which values, so the guarantee is + written where it is read rather than reconstructed from a nil check three files away. + +### Tier 1: silent wrongness + +**#15: a declared audit route that has received zero facts.** This is the highest-value item on +the page, because it is the only one where the failure is invisible *and* the thing it breaks is +the product's reason to exist. If the write route and the read route diverge, mirroring stays +perfect and every commit is authored `unknown (attribution unresolved)`. The consumer's own +mitigation (an alert plus a test) fires *after* a wrong commit exists. + +Design notes, going slightly beyond the ask: + +- It must **not** be a `Stalled`, and must not make `Ready` false. Mirroring genuinely works; a + kstatus consumer that reads `Failed` here would be wrong. This is a separate condition on + ClusterProvider (`AuditRouteReceiving`, or similar) plus an Event. +- It must start `Unknown`, not `False`. A route that has existed for four seconds has legitimately + received nothing. `False` after a grace window, with the window named in the message. +- It should carry the two facts we already hold and the user cannot see: when a fact last arrived + on this route, and how many have. Zero-with-a-timestamp is the whole signal. +- The Event recorder landed with F7, so the Event is nearly free. + +**Do not wait for the stream work to build it, and do not build it twice.** The obvious trap is to +defer #15 until the transport changes, because the stream design makes the signal so much easier +to produce. The right move is the opposite: ship the condition now, and define it in terms that +both transports can answer, which is "how many facts has this route contributed, and when was the +last one". Today that is a counter incremented where +[`RecordFact`](../../internal/queue/attribution_index.go) writes; after the change it is the same +counter incremented where the receiver appends. The condition never learns which transport it has, +which is the same seam rule the stream design argues for one level down. + +Where `auditRoute` came from is [`attribution-fact-identity.md`](attribution-fact-identity.md). +The related open question about *how the watch waits* is no longer open in the way it was: the six +options in [`attribution-wait-poll-vs-push.md`](attribution-wait-poll-vs-push.md) are superseded by +[`attribution-fact-stream.md`](attribution-fact-stream.md), which picks one and specifies it. + +**The inference deletion** sits in this tier for the reason argued above: repo state changing +operator behavior invisibly is the same class of defect as an audit route that silently resolves +nothing. + +### Tier 2: the breaking wave, all at once, while `v1alpha3` + +These all add or change a spec field. Doing them as one `feat(api)!` sequence costs the consumer +one coordinated bump; doing them one at a time costs six. + +#### Attribution facts as a stream: Tier 2, and it answers #15's hard part + +[`attribution-fact-stream.md`](attribution-fact-stream.md) is the only item in this queue that +arrives already specified, and it is the largest. It is in Tier 2 rather than Tier 1 for an honest +reason: today's keyspace is *slow*, not *wrong*. The poll loop runs to completion on essentially +every attributable event, which is waste, and waste does not outrank a silent misconfiguration. + +What lifts it above the rest of Tier 2 is that it retires three things at once instead of adding a +fourth. The per-key `SET`/`GET` and the poll loop go; the `deletecollection` expander, which +rebuilds N per-object facts by parsing a response body that truncation removes exactly when the +collection is large, goes with them; and a collection delete becomes one fact that removals join by +scope, which resolves the aggregated-API and truncated cases that degrade to committer-authored +today. That is test 3 again, and it is why this ranks above the spec-field work rather than beside +it. + +Three things it settles that this queue had left open: + +- **#15's signal gets cheaper and more honest.** A per-`(route, group/resource)` stream makes "this + route has contributed nothing" a directly observable fact rather than a statistic we choose to + keep. The design already proposes a trim-gap counter and asks, in its own open questions, whether + that counter should feed a condition. It should, and it should feed *the same* condition #15 + creates: a route that is losing facts and a route that never had any are the same user-visible + failure (commits authored `unknown`), and they should not be two unrelated surfaces. +- **The circuit breaker keeps its separate justification.** The stream design is explicit that a + status subresource update and a graceful pod delete produce no audit event at all, so no + transport can name their author. That population is why "a route that has never resolved + anything" is a distinct signal from "this event did not resolve", and it is what #15 is really + asking us to expose. +- **It is a prerequisite for HA, not a detour from it.** Under multiple replicas the audit POST and + the watch shard land on different replicas by construction. A per-type stream with independent + cursors is the primitive for that; an in-process channel would work today and have to be thrown + away on the second replica. The ownership problem in + [`ha-gittarget-distribution-plan.md`](../future/ha-gittarget-distribution-plan.md) is untouched + by it, and remains the real blocker. + +The sequencing that follows: **#15 first** (small, and specified above so it survives the +transport change), **then the stream work**, and the trim-gap counter joins #15's condition when it +lands rather than arriving as a second one. + +The one thing to decide before code, beyond that document's own open questions: whether the +in-memory transport ships in the first cut at all. It is argued well, and the conformance-suite +condition is right, but it is a second implementation of the piece that carries attribution +correctness, in service of an install shape (single pod, attribution on, no Redis) we have not been +asked for. Shipping the seam and one implementation is the smaller first commit. + +**F6: `spec.suspend` first.** The maintainer review's bottom line stands: this controller writes +to a Git repository and there is no way to make it stop that is not deleting the object. +`spec.interval` on GitProvider (a real `ls-remote` per pass, hardcoded at 5 min, no jitter) and +the `requestedAt` annotation ride along. + +**#5: `CommitRequest.spec.author`, SAR-guarded.** Two arguments, and the second is the one that +cannot be worked around: attribution needs an audit webhook, which a hosted control plane will +not give you; and *audit cannot attribute a finalized delete at all*: the human's delete is +recorded as an update setting `deletionTimestamp`, and the real delete names whichever controller +cleared the last finalizer. Most operator-managed CRs have finalizers. No audit stream carries +that answer, so no amount of work on the audit path fixes it. The `#220` shape (honored only +against an admission record carrying an authorized verdict, fail-closed independent of the +webhook's `failurePolicy`) is the right one. + +**B4, B1, #6, F10** as written in their source documents. #6 is explicitly a lower priority than +when it was filed: the consumer downgraded it themselves, because branch and folder are now +chosen once per repository on an object that exists because the user picked that repository. +It rides the wave because it is in the wave, not because it is urgent. + +### Tier 3: legibility + +**#11: one encoder, and we should not accept the "cosmetic" label.** The consumer filed this as +cosmetic. For a product whose entire output is a Git diff, a create that renders list items at +2-space indent and an update that renders them at 4 means every install rewrites all 19 +`repositories` lines to carry one changed field. That does not make the diff ugly; it makes it +*unreadable*, which defeats the mirror. The two paths reach different encoders: `contract.go` +and `render.go` go through JSON→YAML, `manifestedit/patch.go` uses `yaml.v3` with its own indent, +which is the shape that produces it. Unverified since v0.35.0, so step one is a test that pins +create and update output byte-for-byte against each other; the fix follows from wherever that +fails. + +**B2: `status.layout`.** Highest value per line of code in the config-surface doc, and more so +after the inference deletion. `ambiguousDocuments` in particular is a correctness-relevant +fallback that is currently a debug-level store diagnostic. + +**F9, B6** as filed. F9 is one envtest; B6 is one error message that will otherwise be the most +likely first-run support ticket. + +### Declined, with the reason + +- **#10 as asked**: superseded by the deletion. See above; we agree the behavior is wrong and + disagree about the fix. +- **B3 (`spec.placement.mode`)**: superseded by the deletion. An off-switch for a feature we are + removing is a permanent API field bought to solve a temporary problem. +- **`spec.expect.layout`**: unchanged from the config-surface doc: publish the observation (B2) + before inventing the assertion. + +--- + +## What this commits us to + +1. A `feat(api)!` sequence for Tier 2, landed together, with one `docs/UPGRADING.md` entry. +2. A behavior change (inference removal) that needs its own UPGRADING entry and a decision on the + fall-back-to-canonical Event. +3. Rewriting Option C's sections in + [`gittarget-new-file-placement-rules.md`](../spec/gittarget-new-file-placement-rules.md). That + document binds the code, so the ladder cannot be deleted from one and left in the other. The + kustomize-root fallback keeps its section; P1–P10 become history rather than live risks. +4. Telling the gitops-api team which two of their asks we are answering differently, before they + build against the shapes they proposed. +5. Specifying #15's condition in transport-neutral terms *before* the stream work starts, so it is + not built twice, and folding the stream design's trim-gap counter into that same condition + rather than adding a second surface. +6. Retiring §5 and §8 of + [`deletecollection-attribution-expander.md`](../spec/deletecollection-attribution-expander.md) + when the expander goes, and keeping §2's deletion-as-intent rule, which the collection join + depends on. That spec binds the code, like the placement one. From 98cb282385e2a5bbd48c754ac6a0518c8372ab17 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 28 Jul 2026 11:16:10 +0000 Subject: [PATCH 2/2] docs(design): answer the review on the fact-stream record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings from the review, three of them corrections and three that the record left implicit. The index keys now carry the audit route. The streams are partitioned by (route, group/resource) and the whole cross-cluster argument rests on that dimension, but the four in-memory structures were keyed on the type alone, which would pool two clusters' facts in one process-wide map and let a watch event on cluster B take an author from cluster A. The rv-only hatch is where that bites hardest, since a resourceVersion is not unique across clusters. The route travels through the waiter keys and the collection scope match too, and a cross-route test is named as the thing that proves it. "No user data is stored in them" was wrong: a fact names an actor and carries the username, display name, and email. There is now a section saying what is held, for how long, where, and who can read it. XREAD takes a concrete position per stream, so the catch-up section says how the TTL horizon becomes one — "-0" — instead of implying an XREAD MINID that does not exist. MINID stays where it belongs, on the trim. The three that were implicit: a retried audit POST may append the same batch twice and that is safe, because a fact is keyed data rather than a position in a sequence, so no XADD idempotency mode is needed; the startup validation has to require Redis only for the Redis transport and the admission webhook, or the in-memory mode is unreachable by construction; and a mixed-version rollout is one process replacing one process, because the chart refuses a second replica, with the dual-write answer noted for when HA makes the window real. Co-Authored-By: Claude Opus 5 (1M context) --- docs/design/attribution-fact-stream.md | 92 +++++++++++++++++++++++--- 1 file changed, 81 insertions(+), 11 deletions(-) diff --git a/docs/design/attribution-fact-stream.md b/docs/design/attribution-fact-stream.md index 841058df..605af54a 100644 --- a/docs/design/attribution-fact-stream.md +++ b/docs/design/attribution-fact-stream.md @@ -162,6 +162,20 @@ body at the receiver and dropped past a size cap. It already carries the namespa timestamp that the collection join reads, and it stops needing the per-item name and namespace the expander used to fill in. +#### A retried audit POST may append twice, and that is safe + +The API server retries a webhook delivery it did not get an acknowledgement for, so the same batch +can be appended more than once, under a fresh stream ID each time. No deduplication is needed for +that, because a fact is **keyed data rather than a position in a sequence**: the second copy carries +the same author under the same `(uid, rv)`, the `latest` map is last-writer-wins over identical +content, and a waiter woken twice resolves to the same name. The duplicate costs one entry's worth +of retention and nothing else. + +This is the property the [transport seam](#the-transport-seam-and-running-without-redis) is built +on, and it is what separates this from the high-water-mark ordering the deleted per-type stream +layer needed. It also means the design does not depend on `XADD`'s idempotency options, which would +put a floor under the Redis and Valkey versions this operator supports in exchange for nothing. + ### Trimming is the TTL Each `XADD` carries `MAXLEN ~ ` so a hot type cannot grow without bound, and a periodic `XTRIM @@ -192,15 +206,27 @@ Entries are applied in stream order into four structures: | Structure | Key | Serves | |---|---|---| -| exact | `(group/resource, uid, rv)` | `ADDED` and `MODIFIED`, the only exact-capable join | -| latest | `(group/resource, uid)` | single-object removals, whose rv never matches; last write wins | -| rv-only | `(group/resource, rv)` | the escape hatch for a fact with an rv but no uid | -| collection | `(group/resource, namespace)`, time-bounded, with an optional uid set | removals caused by a `deletecollection` | +| exact | `(route, group/resource, uid, rv)` | `ADDED` and `MODIFIED`, the only exact-capable join | +| latest | `(route, group/resource, uid)` | single-object removals, whose rv never matches; last write wins | +| rv-only | `(route, group/resource, rv)` | the escape hatch for a fact with an rv but no uid | +| collection | `(route, group/resource, namespace)`, time-bounded, with an optional uid set | removals caused by a `deletecollection` | The first three mirror the key shapes the lookup already knows, so the join policy in [`LookupAuthorResolution`](../../internal/queue/attribution_index.go#L382) survives unchanged. The fourth is new and is described in [the next section](#collection-deletes-are-one-fact). +**The route leads every key, and it has to.** The index is one per process while the streams are one +per `(route, group/resource)`, so an index keyed on the type alone would pool two clusters' facts in +one map and hand a watch event on cluster B an author from cluster A. The rv-only hatch is where +that bites hardest, because a resourceVersion is opaque and not unique across clusters, and the +collection tier is where it bites most quietly, because a namespace name says nothing about which +cluster it is in. The v1 fact keys already carry the route for this reason +([`attribution-fact-identity.md`](attribution-fact-identity.md)), and the same dimension has to +travel through the waiter candidate keys and the collection scope match, not only the four maps +above. A test that stores identical `(group/resource, uid, rv)` facts under two routes and resolves +each from its own is the one that proves it, and it belongs with the index rather than the +transport: the transport already partitions by route because the stream name does. + Stream order matters for exactly one of these. The `latest` map is last-writer-wins, so entries have to be applied in the order they were appended. A single stream is ordered, and a uid belongs to one group/resource and therefore to one stream, so per-object order is preserved without any @@ -485,9 +511,13 @@ worth building separately. ## Starting up and catching up -On start, and on any reconnect, the reader begins from `MINID = now - factTTL` rather than from -`$`. The index is populated with the whole retention window before the first watch event needs it, -which is what makes a restart cost nothing. +On start, and on any reconnect, the reader begins from the retention horizon rather than from `$`. +`XREAD` takes one concrete position per stream, so the horizon is rendered as the stream ID +`-0`: stream IDs are millisecond timestamps, so a point in time +is directly a position, and the read resumes from the entry after it. (`MINID` is an `XTRIM` strategy and +never appears in a read; the trim that enforces the same horizon is described in +[trimming is the TTL](#trimming-is-the-ttl).) The index is populated with the whole retention window +before the first watch event needs it, which is what makes a restart cost nothing. The last-seen ID per stream lives in memory only. It does not need to be durable: a process that lost it also lost its watch connections and its in-memory index, so it is starting from the horizon @@ -611,6 +641,14 @@ cold-replays on restart, which is correct and only more expensive. So Redis is a hard requirement for **attribution and the admission webhook**, and for nothing else. Making attribution work without it is one seam, not a project. +That requirement narrows once the transport is selectable, and the startup validation has to narrow +with it or the in-memory mode is unreachable: an empty `--redis-addr` becomes an error only when the +**Redis** transport is selected, or when the admission webhook is enabled, and the combination of +the in-memory transport with an empty address becomes a supported configuration rather than a +rejected one. The flag, the validation in [`cmd/main.go`](../../cmd/main.go), and +[`configuration.md`](../configuration.md) move together in that change, because a flag whose +validation and documentation disagree is how a mode ends up unreachable in the first place. + One correction belongs with that work: the doc comment on [`RedisStore`](../../internal/queue/redis_store.go#L45) calls it "a hard dependency in every mode", which the validation above contradicts. @@ -687,14 +725,46 @@ histogram is how the improvement gets measured. ## Rollout -The fact schema is ephemeral by construction. Facts carry a TTL measured in minutes, nothing reads -them after that, and no user data is stored in them. So there is no migration to write: the new -version stops writing keys and starts appending to streams, and the old keys expire on their own -within `--author-attribution-ttl` of the upgrade. +The fact schema is ephemeral by construction. Facts carry a TTL measured in minutes and nothing +reads them after that, so there is no migration to write: the new version stops writing keys and +starts appending to streams, and the old keys expire on their own within `--author-attribution-ttl` +of the upgrade. The one visible effect is that events in flight across the restart lose their author, which is already true of any restart today. +### What a fact holds about a person + +A fact names an actor, so it carries personal data and should be described as such: the username, +and the display name and email when the API server supplied them, alongside the object identity, +verb, and stage timestamp. That is the same content the v1 fact keys hold today, taken from the +audit event's `user` field, and moving it from a key to a stream entry changes where it lives rather +than what it is. + +Retention moves the same way. A fact is held for `--author-attribution-ttl` (ten minutes by default) +in the Redis stream and, once read, in the process's in-memory index, and the trim and the TTL sweep +drop it after that. Nothing writes it to Git: the commit carries the author's +name and email as commit metadata, which is what the actor already published by making the change. +Access is whoever can read the Redis keyspace and the pod's memory, which is why the keyspace is +namespaced per install ([`--redis-key-prefix`](../configuration.md)) and why an install that +declines to store any of it can leave attribution off and commit as the configured author. + +### A rolling upgrade is one process, not two formats + +Old writers produce keys and new writers produce streams, and neither reads the other, so an install +running both at once would lose attribution for whatever the wrong half handled. It cannot run both +at once: the chart rejects `replicaCount > 1` +([`validate-replica-count.yaml`](../../charts/gitops-reverser/templates/validate-replica-count.yaml)), +so a single process is replaced by a single process, and the window is one pod restart in which +in-flight events lose their author: the same cost as any restart today, bounded by the +[TTL-horizon replay](#starting-up-and-catching-up) to the events in flight at that moment. + +Under the [HA topology](#what-this-buys-for-high-availability), where replicas overlap during a +rollout, that stops being true and a mixed-version window becomes real. The answer belongs with the +ownership work rather than here, and it is cheap when it arrives: a new writer can append to the +stream and write the v1 keys for one release, so an old reader keeps resolving. Taking that on now +would build a compatibility path for a topology the chart refuses to start. + ## Open questions - **How large may a collection's uid set be before it is dropped?** It bounds one entry's size, the