docs(design): attribution facts as a stream, not a keyspace - #283
Merged
Conversation
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 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR updates the design index and adds design records for prioritized open asks, stream-based attribution facts, and alternatives to polling during attribution waits. ChangesAttribution design documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/attribution-fact-stream.md`:
- Around line 603-612: Update the Redis requirement documentation and
corresponding flag/validation/configuration descriptions so empty --redis-addr
is allowed for attribution when the selected transport is in-memory, while Redis
remains required for the Redis transport and admission webhook; keep all three
surfaces consistent.
- Around line 690-693: Update the fact-schema retention section around the
ephemeral-facts explanation to accurately identify the stored author, display
name, and email fields, and describe their retention in Redis streams and
in-memory indexes under the configured attribution TTL. Replace the “no user
data” and automatic-expiry-only claims with the applicable privacy and access
controls, keeping the migration behavior description consistent.
- Around line 695-696: Update the mixed-version rollout guidance around the
restart behavior to explicitly define compatibility when switching from keys to
streams. Require dual-read/dual-write, an atomic cutover, or a drain-based
rollout so old and new writers/readers cannot ignore each other’s events, and
reconcile the stated rolling-update safety claim with this requirement.
- Around line 191-202: Update the attribution index key construction and
matching around LookupAuthorResolution to include route alongside each existing
key shape: exact, latest, rv-only, and collection. Propagate route through
waiter candidate keys and collection-delete matching so facts from different
routes cannot collide, and add a test proving identical resource, UID, and RV
values remain isolated across routes.
- Around line 486-498: The “Starting up and catching up” section must define a
concrete Redis stream ID derived from now - factTTL, such as <unix-ms>-0, for
the initial per-stream XREAD position. Separate this catch-up backfill contract
from the XTRIM MINID trimming-gap detection, and remove wording that could imply
XREAD accepts MINID.
- Around line 137-149: Update the batching design around serveEventListRequest,
processEvent, and RecordFact to define a stable audit-event or request-batch
identity and deduplicate facts before each grouped XADD. Alternatively, use a
supported idempotent XADD mechanism that returns the existing stream entry for
retries; ensure retried audit POSTs do not create duplicate stream entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 34c5a9e9-5336-428c-8abf-995016871ac8
📒 Files selected for processing (4)
docs/INDEX.mddocs/design/attribution-fact-stream.mddocs/design/attribution-wait-poll-vs-push.mddocs/design/open-asks-priority.md
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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 — "<unix-millis>-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) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/attribution-fact-stream.md`:
- Around line 754-766: Update the HA rollout discussion near “what this buys for
high availability” to make compatibility an explicit rollout requirement:
prevent reader upgrades until old writers are drained or replaced, or require
dual-read support or dual-writing before deploying new readers. Clarify that
upgraded writers alone do not protect the old-writer/new-reader phase, and
remove the implication that compatibility can be deferred until later.
- Around line 165-177: Update the “A retried audit POST may append twice, and
that is safe” section to acknowledge that duplicate batches consume MAXLEN
capacity and can evict unrelated facts before factTTL expires. Document
retention as bounded by both factTTL and the stream entry cap, including
cap-sizing guidance and early-eviction behavior, and remove or qualify claims
that duplicates cost only one entry’s worth and are harmless.
- Around line 514-519: Update the reader startup/reconnect behavior description
to use the in-memory last-seen cursor for reconnects, falling back to the
factTTL retention horizon only after a process restart with no cursor. Clarify
that XREAD with id-x returns only IDs greater than id-x, so the horizon rendered
as <now - factTTL in unix milliseconds>-0 skips an entry exactly at the
boundary, while MINID retains entries at or above its threshold.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c84c1f1f-3be4-46df-8c69-c3150d2b4e54
📒 Files selected for processing (1)
docs/design/attribution-fact-stream.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Three design records. No code changes.
attribution-fact-stream.md— the decisionAttribution facts stop being a keyspace the watch side reads and become a log it follows. 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 facts live in one bounded, TTL'd in-memory index.SET,GET, and the three fact key shapes are deleted.It also:
deletecollectionexpander. One collection fact that every removal in its(type, namespace, selector, window)scope joins, ranked below every per-object fact, with an opportunistic uid set when the apiserver sent a body. That removes the response-body parsing and starts resolving the aggregated-API and metadata-only cases that degrade to committer today.Keeps: the blocking grace window, commit order, attribution being optional, and one-fact-serves-every-GitTarget.
attribution-wait-poll-vs-push.md— the reasoning trailMarked superseded, kept for the argument. 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
--audit-webhook-batch-max-waitand the first lookup is a near-guaranteed miss. The poll loop is the normal path, not the exceptional one.Both measurements already existed in this repo.
reportAttributionStatsin the e2e suite prints the wait histogram (0.5–2s withbatch-max-wait=1s), and the mutation-capture corpus records which scenarios produce no audit event at all.open-asks-priority.mdMerges the three open backlogs into one ordered queue.
Why not plain publish and subscribe
It is at-most-once and reaches only current subscribers. That is tolerable today because a missed message costs latency: the fact is still under a key. Once the keys are gone, a message lost during a reconnect is gone, and those commits are authored
unknown (attribution unresolved)silently and permanently. Three routine cases make it not hypothetical: every process restart, every reconnect, and every new watch on a type all start with an empty index and no way to backfill. A stream replays from the TTL horizon and makes loss detectable.Validation
Docs-only, so
task lint-docsis the whole gate per AGENTS.md. It passes: doccheck resolves every reference, markdownlint is clean, Vale reports 0 errors.Follow-up
Implementation lands separately on
feat/attribution-fact-stream, split per the sequence in the design record.🤖 Generated with Claude Code
Summary by CodeRabbit