Repository navigation
Conversation
9507aca to
bf74330
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical PostgreSQL streaming and firehose compatibility issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds principal invalidation persistence and unified streaming across memory and PostgreSQL backends.
Changes:
- Extends the store API with
AddPrincipalRevocation. - Adds PostgreSQL schema, storage, and unified polling.
- Adds backend and validation tests.
File summaries
| File | Summary |
|---|---|
pkg/store/store.go |
Extends the store interface. |
pkg/store/postgres/store.go |
Persists and streams principal invalidations. |
pkg/store/postgres/store_test.go |
Tests PostgreSQL behavior. |
pkg/store/postgres/migrations/sql/00002_principal_invalidation.sql |
Adds the invalidation table and index. |
pkg/store/memory/store.go |
Adds in-memory support. |
pkg/store/memory/store_test.go |
Tests in-memory behavior. |
pkg/fx/app_test.go |
Updates the test store implementation. |
Review details
Suppressed comments (1)
pkg/store/postgres/store.go:208
- The UNION now permits the same invocation CID to exist once in each table: the primary keys are table-local and both
Addmethods accept the same invocation. However, the sharedseenmap below is keyed only byrec.Cause().Link(), so recording both effects with one invocation silently drops the second event even thoughStreampromises all records. Key deduplication by record kind plus cause, and prune the same composite key.
`SELECT kind, cause, revoked_delegation, path_witness, tenant, principal, recorded_at
- Files reviewed: 7/7 changed files
- Comments generated: 4
- 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.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| record.RecordedAt = time.Now() // under the lock, as in Add | ||
| s.appendLocked(store.PrincipalRevocationEvent(record)) |
| FROM principal_invalidation | ||
| WHERE recorded_at >= $1 | ||
| ) AS record | ||
| ORDER BY recorded_at, id`, |
There was a problem hiding this comment.
Pushing back: the query is valid PostgreSQL. ORDER BY may reference any column of the FROM item, and record is a derived table that projects id; the projected-columns rule applies to SELECT DISTINCT and to a top-level UNION, and the UNION ALL here is inside the derived table. The Postgres store suite runs this exact query on every stream test, and the integration test streams both record kinds through it.
| yield(store.Event{}, fmt.Errorf("decoding principal invalidation: %w", err)) | ||
| return | ||
| } | ||
| event = store.PrincipalRevocationEvent(record) |
| // AddPrincipalRevocation adds a principal invalidation record to the store. | ||
| // The invalidation is the invocation that voids every proof cached for the | ||
| // principal's keys; tenant must be defined and principal must not be empty. | ||
| AddPrincipalRevocation(ctx context.Context, invalidation ucan.Invocation, tenant did.DID, principal string) error |
There was a problem hiding this comment.
Pushing back on this one and on the two sibling comments (memory/store.go:98, postgres/store.go:262). This PR is one layer of a stack: it adds the record and the stream event, the next PR (#11) adds the wire representation and the writer for it, and the route that creates a principal revocation record arrives in #13. Within this PR nothing can produce such a record, so the writer's refusal of a kind it has no format for is unreachable here and is the correct intermediate state for a reviewer reading the PR on its own.
On the suppressed note about one invocation CID appearing in both tables: an invocation carries one command, and /ucan/revoke and /principal/invalidate are different commands stored by different routes, so a single invocation cannot land in both tables. Deduplicating by cause alone is exact.
RevocationStore gains AddPrincipalRevocation, which records that every proof cached for a principal's keys is void. Both backends reject an undefined tenant or an empty principal and otherwise mirror Add. Postgres stores the records in a new principal_invalidation table (migration 00002) and reads both tables in one UNION ALL ordered by recorded_at and id, so the stream keeps one now() horizon, one cursor and one dedup map across both kinds. The memory backend appends to the same log Stream already reads, which dedups a retried invalidation by cause link as it does for revocations. Get is unaffected: it reads only the revocation table. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bf74330 to
04d3973
Compare
|
Superseded by the per-key marker design in fil-one/RFC#30 (f61f474): no principal invalidation record; markers are revoked through the existing /ucan/revoke, and the client's nonce option is #15. Branch kept. |
Description
RevocationStoregainsAddPrincipalRevocation, recording that every proof a gateway cached for a principal's keys is void. Postgres reads both tables in oneUNION ALLordered byrecorded_at, keeping a single horizon, cursor and dedup map.GET /revocation/:cidis unchanged. Part of the Forge S3 tenant IAM work (RFC).Change log
AddPrincipalRevocationon the interface and both backendsprincipal_invalidationtable and index🤖 Generated with Claude Code