Conversation
fileblob stores blob metadata in an ".attrs" file per object and rewrites it with os.Create, truncating in place outside the atomic rename that protects the blob. A read overlapping a write decodes a partial file and fails with "opening reader: EOF", served as a 502. One writer against four readers on a single key failed 408 of 2000 reads. cacheMetadataBlob is most exposed to it, rewriting a key on every refresh while readers are served from it. Nothing in the proxy reads what the sidecar holds. gocloud.dev/blob is imported only by internal/storage, Store sets no ContentType, and Attributes is used only for Size, which comes from os.Stat. A missing sidecar already defaults cleanly, so "metadata=skip" removes the hazard rather than locking around it, and saves a write per store.
metadata=skip stops fileblob rewriting sidecars but does not delete ones already on disk, so a sidecar left partial by an interrupted write now fails every read of its key for good. Before, a later store repaired it by rewriting. Store therefore removes the sidecar for the key it writes. Removal is atomic where the rewrite was not, so a concurrent reader gets the whole old file or nothing. Delete already removes sidecars, so the two paths drain a cache between them. Deriving that path is necessary because fileblob's key escaping is unexported. It is the identity for a plain key and parts from one only for keys that are not valid local paths, which is what filepath.Localize rejects. That also keeps the removal inside the cache directory: without it a key holding ".." resolves outside. The clearing test runs one key per storage path the proxy builds, seeded through a bucket that still writes sidecars so the path under test is fileblob's own.
This was referenced Sep 8, 2026
andrew
requested review from
andrew
and
a lite review from Copilot
and removed request for
andrew
September 13, 2026 16:01
There was a problem hiding this comment.
🟡 Changes recommended
Address the Windows cleanup gap and propagate writer errors in the concurrency test.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR prevents .attrs sidecar corruption during concurrent fileblob access and cleans up legacy sidecars.
Changes:
- Enables
metadata=skipfor local storage. - Removes legacy sidecars during writes.
- Adds cleanup and concurrency regression tests.
File summaries
| File | Summary |
|---|---|
internal/storage/blob.go |
Configures metadata skipping and sidecar cleanup. Critical (2 votes): Windows key localization can leave corrupted legacy sidecars in place. |
internal/storage/blob_test.go |
Tests sidecar behavior and concurrency. Moderate (2 votes): writer errors are ignored, allowing the test to pass without successful writes. |
Review details
Suppressed comments (2)
internal/storage/blob.go:146
- This unconditional removal can delete a real cached object: for a valid key
x, the legacy sidecar path is the same filesystem path used by the object keyx.attrs. Storingxafterx.attrshas been cached removesx.attrsbefore writing the new blob, so subsequent reads of that unrelated object return not found. The cleanup needs a migration strategy that can distinguish a legacy sidecar from a legitimate object at the same path.
if sidecar := b.legacySidecarPath(path); sidecar != "" {
_ = os.Remove(sidecar)
internal/storage/blob.go:147
- Ignoring
os.Removeerrors letsStorereturn success even when a stale sidecar could not be removed (for example, due to permissions or platform file-sharing rules). Withmetadata=skip, that sidecar remains the only metadata source and future reads can still fail with the same decode error. Please ignore only not-exist and propagate other removal errors, or otherwise make failed cleanup observable.
if sidecar := b.legacySidecarPath(path); sidecar != "" {
_ = os.Remove(sidecar)
}
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+133
to
+135
| rel, err := filepath.Localize(key) | ||
| if err != nil { | ||
| return "" |
Comment on lines
+360
to
+362
| if _, _, err := b.Store(ctx, key, strings.NewReader(payload)); err != nil { | ||
| return | ||
| } |
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.
fileblobkeeps per-object metadata in an.attrssidecar and rewrites it withos.Create, truncating in place outside the atomic rename that protects the blob. A read overlapping a write to one key decodes a partial file and fails withopening reader: EOF, served as a 502. One writer against four readers on a single key failed 408 of 2000 reads.Nothing in the proxy reads what the sidecar holds (
gocloud.dev/blobis imported only byinternal/storage,Storesets noContentType, and the onlyAttributesfield read isSize), andgetAttrsalready defaults a missing one, sometadata=skipremoves the hazard instead of locking around it. That is the first commit, one line.The second is separable.
metadata=skipstops rewriting sidecars without deleting them, so one already left partial by an interrupted write would be stuck for good, where a later store used to repair it.Storetherefore clears the sidecar for the key it writes. Deriving that path usesfilepath.Localize, which also keeps the removal inside the cache directory. Drop this commit in favour of documenting afind <cache> -name '*.attrs' -deletesweep if that is preferred.Each commit carries the tests that prove it, and each fails without its own change. Green on ubuntu, macOS and Windows.