Conversation
montehurd
force-pushed
the
per-fetch-storage-path
branch
4 times, most recently
from
September 23, 2026 23:14
1474f03 to
4ce6c37
Compare
Every fetch of an artifact wrote one path, so fetches with different URLs
or digests, which do not share a coalescing key, could overwrite each
other's bytes, serve the wrong ones, or delete them on a digest mismatch
while the other was about to open them.
storeArtifact now writes {ecosystem}/{name}/{version}/{fetch id}/{filename},
with a random id per fetch. Most ecosystems learn the digest only after
the fetch, and storage has no rename, so the id is the one rule that fits
all of them. Existing records keep their paths and stay readable. On a
file:// bucket, Delete now removes an emptied fetch directory, which
fileblob leaves behind, and no other directory, since another fetch may
be creating its own inside it.
A path a record stops pointing at is queued in pending_deletes rather
than deleted, since a request that read the record may still open it. A
loop deletes queued paths after max(1h, direct_serve_ttl), whether or not
max_size is set. UpsertArtifact and clears apply only if the record still
holds the path the caller read, so racing commits each queue the path
they replaced and a clear never orphans a newer commit. Eviction still
deletes inline, and counts space as freed only when the record still
pointed at what it deleted.
Eviction deleted an object before checking its record still pointed at it. If a refetch had moved the record, that object was queued and a request that read the record earlier could still be opening it. Eviction now clears first, skips the delete if the record moved, and queues the object when the delete fails after a clear. Reclaim queues a failed delete again, which moves it behind the rest, so objects the backend keeps refusing cannot fill every batch. Also drop the clearArtifactCache passthrough and ClearCachedArtifact's unused context, note that only tests call ArtifactPath, and document that max_size does not count objects waiting to be reclaimed.
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.
Follow-up to git-pkgs#329 for the storage path item you asked for (comment, review), built as sketched on git-pkgs/proxy#348.
Every fetch of an artifact wrote to one path. Fetches with different URLs or digests don't share a coalescing key, so they could overwrite each other's object, serve each other's bytes, or delete the object on a digest mismatch while the other fetch was about to open it (Copilot on git-pkgs/proxy#329).
{ecosystem}/{name}/{version}/{fetch id}/{filename}with a random 16-character id. I suggested the digest on Fix duplicate fetches and 502s on concurrent cache misses git-pkgs/proxy#329, but only container and Swift know it before fetching, and storage has no rename, so a digest path would mean copying every artifact after it's written. I used a per-fetch id instead. One function makes the id if you'd rather switch later.file://storage,Deletenow also removes an emptied fetch directory. fileblob leaves directories behind, and with one per fetch they would otherwise pile up. Other directories are left alone, since another fetch may be creating its own inside one.pending_deletestable instead of being deleted, since a request that read the record may still open it. A loop deletes queued paths after max(1h,direct_serve_ttl), which keeps signed URLs valid. It runs with or withoutmax_size. Queued objects don't count towardmax_size, so the cache can briefly go over the limit by whatever was replaced during the grace period.checkCache, which used to leave the bytes for the next fetch to overwrite, and Helm'sClearCachedArtifact, which used to delete them inline.UpsertArtifactonly writes if the record still holds the path it read, and retries otherwise, so racing commits each queue the path they replaced.ClearArtifactCachelikewise only clears the path the caller read, so eviction or a discard can't orphan a newer commit.Tests: two fetches with different URLs each serve their own bytes, and a digest mismatch leaves another fetch's object alone. Both tests fail on the old shared path. The rest cover the queue, reclaim, and the upsert race. CI has no Postgres, so I ran the database tests against Postgres 17 locally.