Skip to content

Give each fetch its own storage path - #13

Open
montehurd wants to merge 2 commits into
mainfrom
per-fetch-storage-path
Open

montehurd wants to merge 2 commits into
mainfrom
per-fetch-storage-path

Conversation

@montehurd

@montehurd montehurd commented Sep 23, 2026 •

Copy link
Copy Markdown
Owner

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).

  • Each fetch writes {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.
  • Existing records keep their paths and stay readable, so no objects need migrating.
  • On file:// storage, Delete now 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.
  • When a record stops pointing at a path, the path goes into a new pending_deletes table 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 without max_size. Queued objects don't count toward max_size, so the cache can briefly go over the limit by whatever was replaced during the grace period.
  • Besides the stale discard from Discard stale cache entries under the coalescing key git-pkgs/proxy#348, this covers two clears the sketch missed: the integrity failure clears in checkCache, which used to leave the bytes for the next fetch to overwrite, and Helm's ClearCachedArtifact, which used to delete them inline.
  • UpsertArtifact only writes if the record still holds the path it read, and retries otherwise, so racing commits each queue the path they replaced. ClearArtifactCache likewise only clears the path the caller read, so eviction or a discard can't orphan a newer commit.
  • Eviction still deletes inline, as before, and now counts space as freed only when the record still pointed at what it deleted. Digest mismatch and scan block also still delete inline, but now only the fetch's own object, which nothing else can open.

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.

@montehurd
montehurd force-pushed the per-fetch-storage-path branch 4 times, most recently from 1474f03 to 4ce6c37 Compare September 23, 2026 23:14
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant