Repository navigation
feat(store): add the principal store and share-locked reads - #59
Conversation
bce3c35 to
7695d4b
Compare
7695d4b to
e80703f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Tenant deletion tests do not seed or verify principal removal, leaving the new cascade untested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds principal persistence, PostgreSQL share-locked reads, and tenant-deletion cleanup for forthcoming IAM support.
Changes:
- Adds memory and PostgreSQL principal stores with migration.
- Adds read-lock options and transactional deletion callbacks.
- Wires stores through FX and cascades principal deletion with tenants.
File summaries
| File | Description |
|---|---|
pkg/store/store.go |
Read-lock configuration |
pkg/store/principal/principal.go |
Principal store contract |
pkg/store/principal/principal_test.go |
Backend and locking tests |
pkg/store/principal/postgres/store.go |
PostgreSQL implementation |
pkg/store/principal/memory/store.go |
In-memory implementation |
pkg/migrations/sql/00006_principal.sql |
Principal schema migration |
pkg/fx/store/postgres/provider.go |
PostgreSQL FX wiring |
pkg/fx/store/memory/provider.go |
Memory FX wiring |
pkg/api/tenants_test.go |
Updated API fixtures |
pkg/api/service/tenant/service.go |
Principal deletion cascade |
pkg/api/service/tenant/service_test.go |
Updated service fixtures |
Review details
- Files reviewed: 11/11 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.
e391679 to
735265f
Compare
735265f to
ebcbae7
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ebcbae7 to
97210ea
Compare
571b766 to
363e721
Compare
363e721 to
b27a061
Compare
fb39c69 to
9314782
Compare
alanshaw
left a comment
There was a problem hiding this comment.
Please address review comments, but otherwise this seems fine.
| type ReadOption func(cfg *ReadConfig) | ||
|
|
||
| // WithShareLock reads with [ReadConfig.Share]. | ||
| func WithShareLock() ReadOption { |
There was a problem hiding this comment.
I wonder if this is important enough that all access key and principal read should just have this applied anyway. i.e. it isn't an option it's mandatory. I think we'd always rather wait for a pending write to complete right?
There was a problem hiding this comment.
I would prefer to leave it optional as this lock is raised for reads, but not necessarily all reads using it.
Adds pkg/store/principal with memory and Postgres backends behind one suite, and migration 00006_principal.sql with the RFC's principal table. Delete locks the row FOR UPDATE, runs a caller-supplied beforeCommit callback while holding it, then deletes and commits. A callback error rolls the transaction back and leaves the row in place; deleting an absent row is a no-op that skips the callback. Adds store.ReadOption and store.WithLock(store.LockShare). The Postgres Get honours it as SELECT ... FOR SHARE, so a read waits for an in-flight Delete of the same row to commit or roll back. The memory backend already serializes under its mutex and ignores the option. Wires the store into both fx store modules and has tenant deletion remove the tenant's principals before the tenant row, which they reference with ON DELETE RESTRICT. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The comment claimed the memory backend "has no waiting reader to serve", which is wrong: a share-locked Get is exactly that reader, and it does wait on a removal. The real reason is the deadlock. A policy write calls Lock while holding the bucket's policy, and a removal holds removals while its callback rewrites that same policy, so each would wait on what the other holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
store.WithShareLock replaces the LockMode variadic, following the PaginationOption pattern. A read with no option takes no lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…k fn Store.Delete marked the row removed while DeleteByTenant deletes rows, so the two read as the same operation. Tombstone says what the write does. The callback is fn: "before commit" names a transaction, and the memory backend has none. The contract is unchanged: fn runs inside the write, before it takes effect, and its error leaves the principal live. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Lock read as taking a lock the caller gives back. WithLock says the lock is held for fn and released when fn returns. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
9314782 to
56ef319
Compare
…callback name The memory store calls it fn; on Postgres the callback does run before the commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sets every staging pin in `terraform/envs/staging/apps/terraform.tfvars` to what `terraform/envs/dev/apps/terraform.tfvars` pins at ccf4093. Merging this is the promotion: Check and deploy applies `terraform/envs/staging/apps` on the merge commit, with no further confirmation. dev passed its smoke test on that commit in https://github.com/fil-forge/infra-central/actions/runs/38577102150. This pull request is rebuilt each time dev passes its smoke test after a push to `main`, and closed once staging matches dev. To hold a service back, push to `bot/promote-staging` by hand: the bot leaves the branch alone while this pull request is open. ## hilt `sha256:80076f3` → `sha256:5b6d022` What fil-forge/hilt brings over staging's pin, `a026d16..42dd070`: - feat(s3perm): add the policy vocabulary (fil-forge/hilt#58) (89d4ccf) - feat(store): add the principal store and share-locked reads (fil-forge/hilt#59) (434839e) - feat(bucketpolicy): add the bucket policy and its evaluation (fil-forge/hilt#60) (b30465e) - feat(store): add the bucket policy store with compare-and-set writes (fil-forge/hilt#61) (1ff52cb) - fix(store): bound every Postgres lock wait (fil-forge/hilt#84) (98d7a12) - feat(api): bind access keys to principals (fil-forge/hilt#64) (894624d) - feat(grant): rotate and revoke principal-bound key delegations (fil-forge/hilt#65) (e372bcd) - feat(api): add principals and principal-bound access keys (fil-forge/hilt#66) (880f780) - feat(api): add bucket policies and the principal access reads (fil-forge/hilt#67) (f366100) - feat(rpc): authorize principal-bound keys against bucket policies (fil-forge/hilt#68) (9e93a34) - feat(rpc): answer /s3/bucket/info for a principal-bound key (fil-forge/hilt#85) (b7ed334) - test(itest): cover the IAM model end to end and document it (fil-forge/hilt#69) (fa1512f) - feat(api): revoke every key's delegations before removing a tenant (fil-forge/hilt#80) (70f9380) - feat(rpc): read and write bucket policies over /s3/bucket/policy (fil-forge/hilt#89) (6a675f4) - feat(bucketpolicy): make the bucket policy an AWS bucket policy document (fil-forge/hilt#96) (42dd070)
Sets every staging pin in `terraform/envs/staging/apps/terraform.tfvars` to what `terraform/envs/dev/apps/terraform.tfvars` pins at 8f6b78d. Merging this is the promotion: Check and deploy applies `terraform/envs/staging/apps` on the merge commit, with no further confirmation. dev passed its smoke test on that commit in https://github.com/fil-forge/infra-central/actions/runs/38581685131. This pull request is rebuilt each time dev passes its smoke test after a push to `main`, and closed once staging matches dev. To hold a service back, push to `bot/promote-staging` by hand: the bot leaves the branch alone while this pull request is open. ## hilt `sha256:80076f3` → `sha256:5b6d022` What fil-forge/hilt brings over staging's pin, `a026d16..42dd070`: - feat(s3perm): add the policy vocabulary (fil-forge/hilt#58) (89d4ccf) - feat(store): add the principal store and share-locked reads (fil-forge/hilt#59) (434839e) - feat(bucketpolicy): add the bucket policy and its evaluation (fil-forge/hilt#60) (b30465e) - feat(store): add the bucket policy store with compare-and-set writes (fil-forge/hilt#61) (1ff52cb) - fix(store): bound every Postgres lock wait (fil-forge/hilt#84) (98d7a12) - feat(api): bind access keys to principals (fil-forge/hilt#64) (894624d) - feat(grant): rotate and revoke principal-bound key delegations (fil-forge/hilt#65) (e372bcd) - feat(api): add principals and principal-bound access keys (fil-forge/hilt#66) (880f780) - feat(api): add bucket policies and the principal access reads (fil-forge/hilt#67) (f366100) - feat(rpc): authorize principal-bound keys against bucket policies (fil-forge/hilt#68) (9e93a34) - feat(rpc): answer /s3/bucket/info for a principal-bound key (fil-forge/hilt#85) (b7ed334) - test(itest): cover the IAM model end to end and document it (fil-forge/hilt#69) (fa1512f) - feat(api): revoke every key's delegations before removing a tenant (fil-forge/hilt#80) (70f9380) - feat(rpc): read and write bucket policies over /s3/bucket/policy (fil-forge/hilt#89) (6a675f4) - feat(bucketpolicy): make the bucket policy an AWS bucket policy document (fil-forge/hilt#96) (42dd070)
Sets every staging pin in `terraform/envs/staging/apps/terraform.tfvars` to what `terraform/envs/dev/apps/terraform.tfvars` pins at d236fcd. Merging this is the promotion: Check and deploy applies `terraform/envs/staging/apps` on the merge commit, with no further confirmation. dev passed its smoke test on that commit in https://github.com/fil-forge/infra-central/actions/runs/38583928528. This pull request is rebuilt each time dev passes its smoke test after a push to `main`, and closed once staging matches dev. To hold a service back, push to `bot/promote-staging` by hand: the bot leaves the branch alone while this pull request is open. ## hilt `sha256:80076f3` → `sha256:5b6d022` What fil-forge/hilt brings over staging's pin, `a026d16..42dd070`: - feat(s3perm): add the policy vocabulary (fil-forge/hilt#58) (89d4ccf) - feat(store): add the principal store and share-locked reads (fil-forge/hilt#59) (434839e) - feat(bucketpolicy): add the bucket policy and its evaluation (fil-forge/hilt#60) (b30465e) - feat(store): add the bucket policy store with compare-and-set writes (fil-forge/hilt#61) (1ff52cb) - fix(store): bound every Postgres lock wait (fil-forge/hilt#84) (98d7a12) - feat(api): bind access keys to principals (fil-forge/hilt#64) (894624d) - feat(grant): rotate and revoke principal-bound key delegations (fil-forge/hilt#65) (e372bcd) - feat(api): add principals and principal-bound access keys (fil-forge/hilt#66) (880f780) - feat(api): add bucket policies and the principal access reads (fil-forge/hilt#67) (f366100) - feat(rpc): authorize principal-bound keys against bucket policies (fil-forge/hilt#68) (9e93a34) - feat(rpc): answer /s3/bucket/info for a principal-bound key (fil-forge/hilt#85) (b7ed334) - test(itest): cover the IAM model end to end and document it (fil-forge/hilt#69) (fa1512f) - feat(api): revoke every key's delegations before removing a tenant (fil-forge/hilt#80) (70f9380) - feat(rpc): read and write bucket policies over /s3/bucket/policy (fil-forge/hilt#89) (6a675f4) - feat(bucketpolicy): make the bucket policy an AWS bucket policy document (fil-forge/hilt#96) (42dd070)
Sets every staging pin in `terraform/envs/staging/apps/terraform.tfvars` to what `terraform/envs/dev/apps/terraform.tfvars` pins at d236fcd. Merging this is the promotion: Check and deploy applies `terraform/envs/staging/apps` on the merge commit, with no further confirmation. dev passed its smoke test on that commit in https://github.com/fil-forge/infra-central/actions/runs/38583928528. This pull request is rebuilt each time dev passes its smoke test after a push to `main`, and closed once staging matches dev. To hold a service back, push to `bot/promote-staging` by hand: the bot leaves the branch alone while this pull request is open. ## hilt `sha256:80076f3` → `sha256:5b6d022` What fil-forge/hilt brings over staging's pin, `a026d16..42dd070`: - feat(s3perm): add the policy vocabulary (fil-forge/hilt#58) (89d4ccf) - feat(store): add the principal store and share-locked reads (fil-forge/hilt#59) (434839e) - feat(bucketpolicy): add the bucket policy and its evaluation (fil-forge/hilt#60) (b30465e) - feat(store): add the bucket policy store with compare-and-set writes (fil-forge/hilt#61) (1ff52cb) - fix(store): bound every Postgres lock wait (fil-forge/hilt#84) (98d7a12) - feat(api): bind access keys to principals (fil-forge/hilt#64) (894624d) - feat(grant): rotate and revoke principal-bound key delegations (fil-forge/hilt#65) (e372bcd) - feat(api): add principals and principal-bound access keys (fil-forge/hilt#66) (880f780) - feat(api): add bucket policies and the principal access reads (fil-forge/hilt#67) (f366100) - feat(rpc): authorize principal-bound keys against bucket policies (fil-forge/hilt#68) (9e93a34) - feat(rpc): answer /s3/bucket/info for a principal-bound key (fil-forge/hilt#85) (b7ed334) - test(itest): cover the IAM model end to end and document it (fil-forge/hilt#69) (fa1512f) - feat(api): revoke every key's delegations before removing a tenant (fil-forge/hilt#80) (70f9380) - feat(rpc): read and write bucket policies over /s3/bucket/policy (fil-forge/hilt#89) (6a675f4) - feat(bucketpolicy): make the bucket policy an AWS bucket policy document (fil-forge/hilt#96) (42dd070) Co-authored-by: fil-forge-bot[bot] <318653112+fil-forge-bot[bot]@users.noreply.github.com>
Sets every prod pin in `terraform/envs/prod/apps/terraform.tfvars` to what `terraform/envs/staging/apps/terraform.tfvars` pins at 6cdeb78. Merging this is the promotion: Check and deploy applies `terraform/envs/prod/apps` on the merge commit, with no further confirmation. staging passed its smoke test on that commit in https://github.com/fil-forge/infra-central/actions/runs/38589211473. This pull request is rebuilt each time staging passes its smoke test after a push to `main`, and closed once prod matches staging. To hold a service back, push to `bot/promote-prod` by hand: the bot leaves the branch alone while this pull request is open. ## hilt `sha256:80076f3` → `sha256:5b6d022` What fil-forge/hilt brings over prod's pin, `a026d16..42dd070`: - feat(s3perm): add the policy vocabulary (fil-forge/hilt#58) (89d4ccf) - feat(store): add the principal store and share-locked reads (fil-forge/hilt#59) (434839e) - feat(bucketpolicy): add the bucket policy and its evaluation (fil-forge/hilt#60) (b30465e) - feat(store): add the bucket policy store with compare-and-set writes (fil-forge/hilt#61) (1ff52cb) - fix(store): bound every Postgres lock wait (fil-forge/hilt#84) (98d7a12) - feat(api): bind access keys to principals (fil-forge/hilt#64) (894624d) - feat(grant): rotate and revoke principal-bound key delegations (fil-forge/hilt#65) (e372bcd) - feat(api): add principals and principal-bound access keys (fil-forge/hilt#66) (880f780) - feat(api): add bucket policies and the principal access reads (fil-forge/hilt#67) (f366100) - feat(rpc): authorize principal-bound keys against bucket policies (fil-forge/hilt#68) (9e93a34) - feat(rpc): answer /s3/bucket/info for a principal-bound key (fil-forge/hilt#85) (b7ed334) - test(itest): cover the IAM model end to end and document it (fil-forge/hilt#69) (fa1512f) - feat(api): revoke every key's delegations before removing a tenant (fil-forge/hilt#80) (70f9380) - feat(rpc): read and write bucket policies over /s3/bucket/policy (fil-forge/hilt#89) (6a675f4) - feat(bucketpolicy): make the bucket policy an AWS bucket policy document (fil-forge/hilt#96) (42dd070)
Sets every prod pin in `terraform/envs/prod/apps/terraform.tfvars` to what `terraform/envs/staging/apps/terraform.tfvars` pins at 6cdeb78. Merging this is the promotion: Check and deploy applies `terraform/envs/prod/apps` on the merge commit, with no further confirmation. staging passed its smoke test on that commit in https://github.com/fil-forge/infra-central/actions/runs/38589211473. This pull request is rebuilt each time staging passes its smoke test after a push to `main`, and closed once prod matches staging. To hold a service back, push to `bot/promote-prod` by hand: the bot leaves the branch alone while this pull request is open. ## hilt `sha256:80076f3` → `sha256:5b6d022` What fil-forge/hilt brings over prod's pin, `a026d16..42dd070`: - feat(s3perm): add the policy vocabulary (fil-forge/hilt#58) (89d4ccf) - feat(store): add the principal store and share-locked reads (fil-forge/hilt#59) (434839e) - feat(bucketpolicy): add the bucket policy and its evaluation (fil-forge/hilt#60) (b30465e) - feat(store): add the bucket policy store with compare-and-set writes (fil-forge/hilt#61) (1ff52cb) - fix(store): bound every Postgres lock wait (fil-forge/hilt#84) (98d7a12) - feat(api): bind access keys to principals (fil-forge/hilt#64) (894624d) - feat(grant): rotate and revoke principal-bound key delegations (fil-forge/hilt#65) (e372bcd) - feat(api): add principals and principal-bound access keys (fil-forge/hilt#66) (880f780) - feat(api): add bucket policies and the principal access reads (fil-forge/hilt#67) (f366100) - feat(rpc): authorize principal-bound keys against bucket policies (fil-forge/hilt#68) (9e93a34) - feat(rpc): answer /s3/bucket/info for a principal-bound key (fil-forge/hilt#85) (b7ed334) - test(itest): cover the IAM model end to end and document it (fil-forge/hilt#69) (fa1512f) - feat(api): revoke every key's delegations before removing a tenant (fil-forge/hilt#80) (70f9380) - feat(rpc): read and write bucket policies over /s3/bucket/policy (fil-forge/hilt#89) (6a675f4) - feat(bucketpolicy): make the bucket policy an AWS bucket policy document (fil-forge/hilt#96) (42dd070) Co-authored-by: fil-forge-bot[bot] <318653112+fil-forge-bot[bot]@users.noreply.github.com>
Adds the principal store, where a principal is a console user of a tenant. Share-locked reads wait for an in-flight removal, so callers see settled state.
pkg/store/principal:Add,Get,ListByTenant,Deletewith a pre-commit callback,Lockstore.WithShareLockread option; migration00006_principal.sqlPostgresOrSkipandRequireWaitsForWritertest helpers🤖 Generated with Claude Code