refactor: stream store records as events - #9
Draft
pyropy wants to merge 1 commit into
Draft
Conversation
Stream now yields a store.Event sum type instead of a RevocationRecord, so revocation and principal invalidation records can share one poll, cursor, settle window and dedup map. Event carries a Kind and one of Revocation or PrincipalRevocation, with Cause and RecordedAt accessors for the fields the stream loop needs. Nothing produces principal events yet. The memory backend keeps its map for Get and streams from an append-only log, so every stored record is streamed in the order it was stored. The postgres backend wraps the rows it already reads. The firehose handler switches on the event kind and refuses kinds it has no wire format for. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
pyropy
added this pull request to stack #12
September 10, 2026 16:19
pyropy
force-pushed
the
srdjan/feat/iam-store-events
branch
from
September 11, 2026 12:40
55565a9 to
73ca334
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues affect retry idempotency and stream deduplication.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refactors revocation streaming to use typed events across storage backends and firehose handling.
Changes:
- Adds event types, constructors, and accessors.
- Updates PostgreSQL and memory streaming.
- Adapts firehose dispatch and tests.
File summaries
| File | Summary |
|---|---|
pkg/store/store.go |
Defines the event-based store API. |
pkg/store/store_test.go |
Tests event helpers. |
pkg/store/postgres/store.go |
Streams PostgreSQL records as events; moderate deduplication concern remains (1 vote). |
pkg/store/postgres/store_test.go |
Updates PostgreSQL stream tests. |
pkg/store/memory/store.go |
Adds append-only event logging; moderate retry-idempotency concern (2 votes) and a nit for additional coverage (1 vote). |
pkg/store/memory/store_test.go |
Updates memory stream tests. |
pkg/fx/app.go |
Dispatches firehose events by kind. |
pkg/fx/app_test.go |
Adapts the firehose test store. |
Review details
Suppressed comments (2)
pkg/store/memory/store.go:80
- The append-only log is what makes the new re-revocation behavior observable, but the memory-store tests only add distinct delegations. If this is accidentally changed back to keeping only the latest entry per delegation, the current suite still passes. Please add a test that adds two different revocation invocations for the same delegation before starting the stream and asserts that both events are delivered (while
Getstill returns the latest record).
s.log = append(s.log, logEntry{event: event, seq: s.nextSeq})
pkg/store/postgres/store.go:144
Eventexposes a pointer to the record, so the callback can mutaterec.Revocation.RecordedAtbefore this line runs. That lets a consumer change the timestamp stored inseen(for example to zero), causing the row to be pruned from deduplication and delivered again on a later poll. Capturerec.RecordedAt()before invokingyield, or yield a protected copy, and use the captured value here.
seen[link] = rec.RecordedAt()
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| s.records = make(map[cid.Cid]store.RevocationRecord) | ||
| } | ||
| s.records[record.Revoke] = record | ||
| s.appendLocked(store.RevocationEvent(record)) |
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
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.
Description
RevocationStore.Streamyields astore.Event, a sum of a revocation record and a principal revocation record, so both kinds share one poll, cursor, settle window and dedup map. Nothing produces the second kind yet. Part of the Forge S3 tenant IAM work (RFC).Change log
store.Event,EventKind,PrincipalRevocationRecordand constructors;Streamreturns eventsEvent; stream-events tests on both backends🤖 Generated with Claude Code