Repository navigation
Conversation
Bucket policy writes are unconditional, so nothing computes or compares a policy entity tag any more. StatementCID, which orders a policy's statements, stays. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pyropy
added a commit
to fil-forge/hilt
that referenced
this pull request
Oct 29, 2026
Pins fil-forge/libforge#89 (c068c96); repin to libforge main once it merges. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pyropy
added a commit
to fil-forge/ingot
that referenced
this pull request
Oct 29, 2026
Pins fil-forge/libforge#89 (c068c96); repin to libforge main once it merges. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@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. 🎉 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". |
Peeja
added a commit
to fil-forge/hilt
that referenced
this pull request
Nov 3, 2026
…112) *Written by Claude.* Part of [FIL-1646](https://linear.app/filecoin-foundation/issue/FIL-1646). Replaces #111, whose stuck-lock diagnosis was wrong. **Overlaps #110.** That PR makes the same change, scoping a rotation's rewrite to the written bucket with `ReplaceBySubject`, as one part of a larger redesign that drops policy ETags, preconditions and locks. That redesign also depends on fil-forge/libforge#89, fil-forge/ingot#320, fil-one/fil-one#948 and an RFC. This PR is meant to land first as the narrow fix for the cause, independent of that redesign. #110 would then rebase onto it, keeping either `ReplaceOver` or its own `ReplaceBySubject`. ## Problem Since 2026-10-29, every `PutBucketPolicy` on staging eu-central-3 fails with "the policy is being changed concurrently" (409 OperationAborted at the gateway). That keeps fil-one's policies e2e suite red, and the suite gates fil-one's production deploys. A policy write rotates the stored delegations of every key bound to a principal whose actions changed. `Rotate` did that through `delegations.Replace`, which locks the keys, then reads, decodes, deletes and re-inserts **everything they hold, over every bucket**, to change their grants over one. A write's cost therefore grew with the principal's keys × the buckets they reach × the commands per grant. Once it passed `grant.BatchTimeout` (8s), every write for that principal failed, and `writeError` reports the deadline as a concurrent change. Evidence from staging: - Ingot's `s3_PutBucketPolicy` spans (Tempo, 5-minute buckets) on 2026-10-29: p90 was about 1.7–2.0s at 12:25 UTC, 3.7s at 13:40, 6.6s at 14:10, and pinned at 8.0–8.4s from about 14:40. A latency climb over two hours points to growth, not a held lock. - Hilt's request log for the failing writes shows `latency` of about 8.02s next to the concurrent-change error, which matches `BatchTimeout` rather than the 10s `LockTimeout`. - fil-one's default bucket policy names the bucket's creator, so every bucket the e2e user creates is one more bucket their keys reach. Hilt logged about 2,300 keys created for that user's principal since 2026-10-15, and 20,000–50,000 `published revocation` lines per 6-hour window during 2026-10-26 to 10-29. That those keys and buckets are left over from e2e runs is an inference; I haven't counted the live rows. ## Change - `delegation.Store` gains `ReplaceOver`: `Replace` narrowed to the delegations over one subject. It takes the same audience locks, so the two serialize. It reads and deletes only the rows over the subject, which the `(audience, subject, command)` index serves, and leaves the rest in place. It refuses an undefined subject, and a replacement over another subject (`CheckReplacementOver`). Memory and Postgres share the implementation with `Replace` behind a private `replace`. Postgres uses two query texts rather than an optional-subject predicate, so each is planned for its index. - `Rotate` uses `ReplaceOver`, so a write costs what the keys hold over the bucket being written. Revocations and the stored result are unchanged. `Revoke` (principal removal) still replaces whole sets, as it removes everything. - `Rotate` logs each rotation's key count and duration: Info on success, Warn with the error on any failure, including one while loading the keys or the tenant's key. A rotation with no keys logs nothing. Today the handler reports a deadline only as a concurrent change, which is why this was hard to see. ## What this doesn't fix - A write still does per-key work: one key lookup and one set of issued delegations for each key of each affected principal. A principal with thousands of keys would still be slow, just much less so. - The leftover keys and buckets in the staging e2e tenant are still there. Cleaning them up is a separate question for whoever owns that tenant. ## Observability - Logs: `prepared delegation rotation` / `rotating delegations failed` with `tenant`, `bucket`, `keys`, `took` and `error`. Success reads "prepared" because a policy write commits after the rotation returns. - This feature's health reads best as a number: the p90 latency of bucket policy writes (Ingot's `s3_PutBucketPolicy` span, or Hilt's own once its traces are exported), against the 8s `BatchTimeout`. Part of [FIL-1143](https://linear.app/filecoin-foundation/issue/FIL-1143). An alert at around 4s would have flagged this a day before it failed. ## Checks - `go build ./... && go vet ./... && go test ./...` pass locally, and `gofmt` is clean. - Docker wasn't available here, so the new store-suite cases ran against the memory backend only. CI's Linux job requires Docker, so the Postgres backend runs there. - The new rotator tests fail against the old `Rotate`, and the duplicate-audience store test fails against the memory store before its fix. [✴️](https://claude.ai/code/session_01DhEPPzQsYKXVZ2yGjcRWff) https://claude.ai/code/session_01DhEPPzQsYKXVZ2yGjcRWff --- _Generated by [Claude Code](https://claude.ai/code)_ --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Srdjan <stankovic.srdjo@gmail.com>
Contributor
Author
|
Closing: the staging performance issues behind FIL-1646 have been resolved, so we're keeping bucket policy locks and ETag preconditions. The branch stays in case we revisit. |
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.
Bucket policy writes become unconditional, so nothing computes or compares a policy entity tag.
bucket.ETagand its tests;etag.gois renamedcid.go.StatementCID, which orders a policy's statements.PolicyOKdoc reference to the tag.Hilt and Ingot pin this commit in their PRs.
References: FIL-1646; RFC fil-one/RFC#38; Hilt fil-forge/hilt#110; Ingot fil-forge/ingot#320.
🤖 Generated with Claude Code