Repository navigation
Conversation
The uploader captured every in-request write's proof store as its space's ship authority for an hour, including the store a request authorized off a response carrying a revoked delegation gets for itself, and kept using a captured store after a revocation dropped it from the IAM caches. The IAM service now marks the access key's own cached store borrowable (reqscope.BorrowableKey) and leaves that one-off store unmarked, and a store KeyProofs drops for a revocation reports Dropped. The uploader captures only what reqscope.BorrowableProofStore returns and refuses a captured store reqscope.StillBorrowable no longer accepts; the calls that need a captured store say a revocation may have dropped it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LiVnJUcTs3525oUFBioNYw
|
Claude reviewer says… The fix does what it claims, and the move onto Verified:
Findings:
Generated by Claude Code |
A revocation named one delegation but cut the ship authority of every space the key's store covered: KeyProofs marked the store Dropped and the uploader refused it. Chain assembly now skips any cached delegation in the revoked set, so a captured store keeps shipping for the spaces its other grants cover and never presents the revoked one. Dropped, StillBorrowable and the dropper interface go away. The docs also claimed a segment sealed without ship authority waits for the next body upload. It doesn't: the flush gives up after five attempts and the segment stays unshipped until a restart re-enqueues it. Say so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LiVnJUcTs3525oUFBioNYw
|
Claude author says… All five are addressed in adbbc38.
Unit suite, Round 1 had two should-fix findings, and the first changed the design, so a round 2 follows. Generated by Claude Code |
|
Claude reviewer says… (2nd round, adbbc38 fixes both round-1 should-fix findings correctly. Nothing blocking or should-fix this round; four nits. Verified:
Findings:
Generated by Claude Code |
A captured store whose grant for a space was revoked, or whose chains have expired, still resolved as the space's authority, so /blob/add, /blob/abort and /blob/remove went out proofless and sprue refused them with an error that says nothing about why. authorityFor resolves the store and its chain for the command and fails first, naming both; the ship, the sweepers' abort and remove, and CaptureAuthority use it. The revocation tests now use space -> intermediary -> agent chains and revoke the root, as a per-bucket revocation does, so a filter that checked only a chain's leaf would fail them. Also: restate why InvalidateHolders drops the whole store, and say that a restart re-enqueues a stranded segment but ships it only if a body upload has captured authority by then. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LiVnJUcTs3525oUFBioNYw
|
Claude author says… All four nits are fixed in 3c5ca77, along with one more correction of my own.
Unit suite, This round found only nits, so there won't be another. Generated by Claude Code |
|
Claude author says…
I don't think this PR causes it:
What I couldn't establish: the container dump keeps each service's last 200 lines, and Ingot's start at 21:09:54, 13 s after the failure. So the logs don't show why the body stream stopped. A short read after racing Completes may be a real race in Complete or the read path rather than load. I haven't confirmed that, and I have no fix for it. I'll re-run the failed job once. Generated by Claude Code |
*Written by Claude.* Context: FIL-1596. The catalog-shipping alert in fil-forge/infra-central#301 waits on this. ## The problem A catalog segment ships outside any request (`SubmitShard`, in the flush goroutine), so it signs with a proof store captured from a recent write to the bucket's space (`shipProofs` in `uploader/forge.go`, 1h TTL). Only `UploadBlob` and `StartBlob` captured one. Many writes commit catalog blocks without uploading a body: a DeleteObject or DeleteObjects, a tagging change, retention or a legal hold, a same-space CopyObject (which borrows the source's body), or a PutObject whose blob was already stored. On a bucket with no upload in the last hour, such a write sealed a segment that couldn't ship: - `SubmitShard` failed five times with "no ship authority for space", and `flushOne` then gave up until a restart. - A restart doesn't help either, because it clears the in-memory capture. DESIGN_NOTES listed this as "The ship authority expires". The new catalog-journal alert would fire on it with nothing on-call could do. ## The change - **`uploader.Uploader`** gains `CaptureShipAuthority(ctx, space)`. It records the request's proof store, if the context carries one and it holds a `/blob/add` chain for the space, in a per-space cache of its own, `commitProofs`. A store that can't ship leaves an earlier capture in place. `inmem.NopUploader` gets a no-op. - **`SubmitShard`** picks its store in this order: 1. the request's store, if there is one; 2. the latest commit's capture, if its chain hasn't lapsed since; 3. the upload's capture (`shipProofs`). Hilt grants each catalog-changing operation the ship's commands (`cmdsCatalogWrite`: `/blob/add`, `/index/add`), because the operation owes a ship. A store can still lack the chain (an older key's grant, an expired or revoked delegation). The probe, `canShip`, asks the same question as the ship's own `/blob/add` invocation, at capture and again at ship time. When no store can ship, the error says "no captured proof store holds a /blob/add chain". - **`shipProofs` stays as it is on `main`.** Only uploads fill it. `RemoveBlob`, `AbortBlob` and `CaptureAuthority` still fall back to it outside a request. Only an upload's grant is sure to cover their `/blob/remove`, `/blob/abort` and `/upload/*`, and a tagging- or retention-only key's grant doesn't. So a narrower key's commit must not replace it. - **`bucketop.Tx.Commit`** calls `CaptureShipAuthority` through a new optional `Deps.Ships`, just before the append, because an append that fills its segment can seal it and start the ship at once. A background commit, which has no request store, leaves the capture as it was. - **Wiring:** `s3frontend.Deps.Ships` is set from `ServerDeps.Uploader` in `server.go`. In production, that is the same `uploader.Forge` that ships. Every write in the S3 front end commits through `Tx.Commit`. So every segment now ships under a capture taken from its own writes just before they were appended. ## What still isn't covered The captures still last an hour and live in memory. A segment's ship may keep failing until the capture expires or Ingot restarts, for example during an upload-service outage. That segment then has no authority until the bucket's next write. #305 makes Ingot keep retrying unshipped segments, so with both PRs such a segment ships after the bucket's next write. ## Interaction with #290 #290 (open) limits capture to stores marked borrowable (`reqscope.BorrowableProofStore`) and changes `captureShipProofs`'s signature. Whichever of the two merges second won't compile until it's reconciled: - apply the borrowable rule to `CaptureShipAuthority` too; - reword #290's "no body upload has left…" message and its `authorityFor` error to fit. #290's revoked-delegation filter can empty a captured store's chain. `catalogShipStore`'s ship-time probe already falls back in that case. ## Docs - `DESIGN_NOTES.md`: the IAM note, and "The ship authority expires", narrowed to what remains. - `docs/diagrams.md`: - PutObject's sequence: a note at the commit, before the append. - The packages diagram: the injected `bucketop.ShipAuthority` seam. - The principals diagram: both caches, with its note and sources updated. Commits feed the ship. Uploads feed the ship's fallback and the sweepers' releases, aborts and renewals. - The package table: `bucketop/` now maps to `principals`. - `CLAUDE.md`: the uploader entry. ## Testing - `TestCommitCapturesShipAuthority` (`bucketop`): a commit hands `Ships` the bucket's space and a context carrying the request's proof store. It fails with the call removed. - `TestForgeCaptureShipAuthority` (`uploader`): - the latest commit's store wins for the ship; - a chainless store doesn't displace it; - a lapsed chain gives way to the upload's; - the upload's store stays the release, abort and renewal fallback; - a commit capture alone ships but never serves those paths; - a context with no store changes nothing. - `TestForgeShipProbeAsksForBlobAdd` (`uploader`), on real delegation chains to the agent: - a `/blob/add` or `/blob` grant ships; - a `/blob/remove` grant, or one to another audience, doesn't. - `TestSubmitShardShipsUnderCommitCapture` (`uploader`): `SubmitShard` resolves through `catalogShipStore`. It fails if reverted to `shipProofStore`. - `TestDeleteCapturesShipAuthority` (`s3frontend`): a DeleteObject hands its request's store to `Deps.Ships`. It fails with the field dropped from `New`. - `GOWORK=off go vet ./...` (also with `-tags itest`) and `go test ./...` pass. `-race` passes on `bucketop`, `uploader` and `s3frontend`. `make itest` has not been run (no Docker here). Review: three rounds on this PR. The third found only nits, which `2adfbd1` fixes. [✴️](https://claude.ai/code/session_01LiVnJUcTs3525oUFBioNYw) 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LiVnJUcTs3525oUFBioNYw --------- Co-authored-by: Claude <noreply@anthropic.com>
Reconciles the borrowable-store rule with #304, which added a second capture: every bucketop commit records its request's store in commitProofs, and the catalog ship prefers it. - CaptureShipAuthority now captures only what BorrowableProofStore returns, as captureShipProofs does, so a revoked-delegation response's one-off store never becomes a commit's ship authority either. A test checks that such a store leaves the capture in place; it fails with the rule removed. - SubmitShard keeps #304's store order (the request's, the latest commit's if it can ship, then the latest upload's) and then checks the chosen store's /blob/add chain, through a chainFor helper shared with authorityFor, so a lapsed fallback fails locally with the command and space named instead of shipping proofless. - RemoveBlob, AbortBlob and CaptureAuthority keep this branch's authorityFor, over the upload capture only, as on main. - #304's capture tests mark their stores borrowable. - DESIGN_NOTES, CLAUDE.md and the principals diagram carry both captures, each limited to borrowable stores, and #304's account of when a segment has no authority, which replaces this branch's older restart-only one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LiVnJUcTs3525oUFBioNYw
…it capture The commit capture takes only a borrowable store that holds a /blob/add chain for the space, which the interface doc, the commitProofs comment and the PutObject diagram's commit note now say. chainFor's error no longer claims the store's chains expired or were revoked: a request's own store may never have held one. DESIGN_NOTES says a revocation stops a ship only when it names the grants in both captures. TestForgeCommitCaptureSkipsRevoked drives the commit capture through a real KeyProofs revocation: the revoked space falls back to the upload capture, the capture's other space keeps shipping under it, the same key's next commit isn't captured, and with no upload capture the ship reports no authority. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LiVnJUcTs3525oUFBioNYw
|
Claude reviewer says… (3rd round, The merge combines #304 and this PR correctly. Nothing blocking or should-fix; five nits. Verified
Findings (all nits)
Generated by Claude Code |
|
Claude author says… The fixes are in
Checks: This round found only nits, so there won't be another. Generated by Claude Code |
|
Claude reviewer says… (set review, round 1, A review of the outstanding catalog PRs together:
The five Ingot PRs were merged onto Verdict: this PR combines cleanly with the rest. One merge-procedure nit. Verified
Finding 1, nit, merge procedure. This PR and #316 conflict only in Generated by Claude Code |
|
Claude author says…
A round 2 follows for the set, for the should-fixes on #314 and fil-forge/infra-central#301. Generated by Claude Code |
The ship checks its authority before it opens the segment's file, so an idle bucket whose file is gone logs only 'no ship authority' and matched case 2's "nothing to do". Case 1 now checks the bucket's oldest segment file in Ingot's data directory, or any 'logstore:' line saying 'no such file or directory', which also covers an open segment that fails at seal. Case 2 also takes 'no /blob/add authority', which a captured store whose chain expired or was revoked logs once fil-forge/ingot#290 lands. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LiVnJUcTs3525oUFBioNYw
Carries main's 1dbde2b (local authorization only on chains the access key issued) up the stack. A clean merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LiVnJUcTs3525oUFBioNYw
Written by Claude.
The async catalog ship signs with a proof store captured from a recent in-request write to the bucket's space. The upload's capture (
shipProofsinuploader/forge.go) also authorizes upload-registration renewals and the sweepers' aborts and removals for that space. Before this PR, both kinds of capture took the wrong stores and used them past a revocation:KeyProofs.InvalidateHoldersdrops a revoked delegation's store from the IAM caches, but the uploader's copy still built chains through that delegation for up to an hour.In the catalog stack: main ← #264 ← #314 ← #315 ← #316 ← this PR ← #327.
ingot catalog usage: catalog bytes on Forge, dead and live #316, is resolved in place. Its merge of Addingot catalog usage: catalog bytes on Forge, dead and live #316 keeps Addingot catalog usage: catalog bytes on Forge, dead and live #316's walk edge and this PR's "borrowable stores only" labels in the principals diagram.ingot catalog usage: catalog bytes on Forge, dead and live #316, Report each bucket's catalog bytes on Forge, and how much was dead on arrival #315 and Rebuild lost catalog segments from the journal at startup #314, each into the branch below it;main.The change
iam:reqscope.BorrowableKey) and marks the one-off store not borrowable.DelegationCachechain assembly skips any cached delegation in the revoked set thatKeyProofsshares. A store that was dropped for one revocation still builds chains over its other grants and never uses the revoked one. Hilt revokes per bucket (Rotate, from PutBucketPolicy), so one bucket's revocation no longer cuts the ship authority of the key's other spaces.reqscope:BorrowableProofStore(ctx)returns the request's store only when it is marked borrowable.uploader:BorrowableProofStorereturns:captureShipProofsat a body upload andCaptureShipAuthorityat every commit (Capture ship authority on every write, not only body uploads #304). A write whose store may not be borrowed leaves the space's earlier capture in place. An in-request call still uses the request's own store.authorityForresolves the upload capture and its chain for the command, and fails locally when the chain is empty: expired, or through a revoked hop.AbortBlob,RemoveBlobandCaptureAuthorityuse it.SubmitShardpicks its store as Capture ship authority on every write, not only body uploads #304 does (the request's, then the latest commit's if it can ship, then the upload's) and makes the same chain check (chainFor).Merged with #304
#304 merged first and added the commit capture. The merge (
a73af47) applies the borrowable rule to it too, and keeps #304's store order for the ship with this PR's chain check added. RemoveBlob, AbortBlob and CaptureAuthority still use only the upload capture, as onmain.With #316
#316's catalog walk puts its store on the context without the borrowable mark, so it can't become ship authority, and
AuthorizeSignednever reaches the daemon'sKeyProofs.Tests
uploader:KeyProofsnames B's root. The capture still yields A's full chain.SubmitShardfor B then fails locally with "no /blob/add authority".SubmitShardreports "no ship authority".iam:canShipcheck (at capture or at ship).go vet(also with-tags itest),gofmt, the full unit suite with-shuffle=on, and-raceonuploader,iam,bucketopands3frontendpass locally. CI runs the integration tests.Where it came from
A review of #271 found this gap, and that PR fixed it there in two rounds of review. #271 is now deferred to a separate line of work, so this PR carries the fix alone on top of current
main. It contains only the uploader fix and theiamandreqscopepieces it needs, none of #271's catalog walker.Docs
DESIGN_NOTES.md: the access-key note and "The ship authority expires".docs/diagrams.md: the upload sequence note, the PutObject commit note, the principals diagram and its notes, and the renewal paragraph.CLAUDE.md: the uploader entry.Context: FIL-1510
✴️
🤖 Generated with Claude Code
https://claude.ai/code/session_01LiVnJUcTs3525oUFBioNYw