fix(oci): fence the blob link against a repository tombstone

What this delivers

datastore.ContainerBlobLinker.LinkBlob now re-verifies the destination repository as the first statement of its own transaction. Two container write paths change. The blob finalize (chunked PUT and single-request POST, both with ?digest=) and the cross-repository blob mount now answer 404 NAME_UNKNOWN at the repository tier, in place of a false 201.

Three more behaviour changes ride with the fence. handleMount arms its own connection write deadline, so a mount that parks on the fence can still deliver its answer. The two blob 500 arms get their own log line for a link that outran its bound. The section ## The mount's own write deadline, and two new log lines covers those two. The third is the review answer described under ## A departed client's link failure is a 499: both link arms now answer 499 with no envelope, at Warn, once the request context is already canceled.

The cause. LinkBlob opened its transaction and went straight to its two inserts. No statement between the request's own resolve and that commit re-read the repositories row. A repository tombstoned inside that window took a committed blob_storage_attachments row and a committed container_blobs row. The handler then answered 201, and the purge removed both rows with nothing reaching the client.

The window starts at the write path's own resolve, not at the authorization middleware. internal/authz/middleware.go:369-375 already answers 404 on the same soft_deleted_at predicate, for every request that arrives after the tombstone is visible. Only the request that raced the tombstone got the false 201. This change makes the racing request agree with the non-racing ones.

It does add one new refusal. At the base commit bc7d0617f the linker took no repositories lock at all: internal/datastore/container_blob_linker.go there matches reVerifyRepository, FOR SHARE and ErrRepositoryConcurrentlyDeleted zero times. A link that meets row-lock contention past blobLinkTimeout now answers 500, where before it answered 201. The section ## One outcome past the bound is a 500, not a 404 describes that outcome and what it costs a client.

What this does not deliver

Issue 1122 names six hosted write paths. This merge request is part 1 of five, and it fences two of the six. Four paths remain, each in its own successor merge request:

  • the blob upload initiate, in part 2
  • the npm dist-tag PUT, in part 3
  • the npm deprecate, in part 4
  • the management-API container tag upsert, in part 5

Part 5 also removes the #1122 tracking pointers that this merge request leaves standing in the OpenAPI description (api/openapi/v1.yaml:334) and in the Bruno file (api/bruno/management-api/repositories/delete-repository.bru:65).

The mount's own write deadline, and two new log lines

The mount arms its own write deadline. handleMount armed none, so the only budget was the server.timeouts.write instant that Go starts when the request headers finish being read. The fence can hold the request for blobLinkTimeout, which is 10 seconds, after a footprint read bounded at counterEmitTimeout, which is 5 seconds. At the 10-second default in config.example.yaml:14 the connection expired before the handler could write. mountWriteTimeout is counterEmitTimeout + blobLinkTimeout + 5s, which is 20 seconds, and setMountWriteDeadline arms it (internal/format/oci/upload.go:492-503). A connection that refuses the deadline answers 500 at internal/format/oci/upload.go:528-534.

MountBlob's failure mapping moved out of handleMount into writeMountLinkError (internal/format/oci/upload.go:614-629), because handleMount reached 78 lines against the funlen limit of 70 in .golangci.yaml:459-460. The 404 and 500 arms are unchanged by that move.

The arm does not reuse setUploadDeadlines, for two measured reasons. That helper's doc is 27 counted lines and the helper is unexported (internal/format/oci/upload.go:1947-1976), so scripts/ci/check-comment-caps.sh charges cap 1 the moment a diff touches it. Its doc also says "for an upload body" and "keeps the three paths uniform", so a fourth caller makes both claims false, and guardrail 21 then needs the edit the caps gate refuses. Its budget is container.upload_read_timeout as well, one hour by default (internal/config/container.go:25) and sized for a 50 GB body, which is not a bound a bodyless mount takes.

Two new log lines. boundedLinkMessage (internal/format/oci/upload.go:605-612) picks the message for a link 500. The mount arm logs oci mount: link outran its own bound; answering 500 at internal/format/oci/upload.go:626-628, and the finalize arm logs oci upload: finalize link outran its own bound; answering 500 at :1495-1497. Before this, each arm shared one message with every storage fault and every other datastore error. errLinkOutranItsBound and linkFailure (internal/format/oci/store.go:185-195) tag the expiry of the link's own deadline, and not a caller cancellation.

Tests for these two changes. TestMountArmsWriteDeadlinePastTheLinkBound (internal/format/oci/mount_test.go:1418-1442), TestMountSetWriteDeadlineFailureReturns500 (internal/format/oci/mount_test.go:1444-1468) and TestLinkFailure_TagsOnlyTheBoundsOwnExpiry (internal/format/oci/store_test.go:1047-1103). The mount tests moved to the deadline-capable recorder the upload suite already carries (internal/format/oci/upload_deadline_test.go:54), because http.NewResponseController fails against a plain httptest.ResponseRecorder.

Test coverage

Source: the issue card's acceptance list, not a spec. docs/specs/S20-a-lifecycle-closed-beta.md's ## Error Cases row records the window as accepted for closed beta, so no S20-A criterion was unsatisfied on main. All 17 test names below were verified present on the branch.

# Item Tests
1 npm dist-tag PUT on the race answers 404 Out of scope — part 3
2 npm deprecate on the race answers 404 Out of scope — part 4
3 blob finalize answers 404 NAME_UNKNOWN, no container_blobs row TestUploadFinalize_RepositoryTombstonedMidLinkReturns404NameUnknown (chunked PUT and single-request POST), TestCompleteUploadRefusesTombstonedRepository, TestBlobFinalizeLivenessRace_RepositoryTombstonedMidLink, TestContainerBlobLinker_LinkBlob_RepositoryGate/a_tombstoned_repository_refuses_and_writes_nothing
4 mount with tombstoned destination answers 404; mount with tombstoned source keeps its 202 TestMountDestinationTombstonedMidLinkReturns404, TestBlobMountLivenessRace_DestinationTombstonedMidLink, TestBlobMountLivenessRace_TombstonedSourceKeepsIts202
5 management-API tag upsert answers 404 Out of scope — part 5
6 items hold when the tombstone commits while the transaction is open TestBlobFinalizeLivenessRace_RepositoryTombstonedMidLink, TestBlobMountLivenessRace_DestinationTombstonedMidLink, TestContainerBlobLinker_RepositoryTombstonedMidTransaction — each stages an uncommitted tombstone and asserts the fence parked on its row lock
7 live-repository behaviour unchanged TestContainerBlobLinker_LinkBlob_RepositoryGate live docker and oci subtests, TestContainerBlobLinker_LinksAndIsIdempotent, TestUploadFinalize_LinksUnderTheResolvedRepository, TestMountLinksUnderTheDestinationRepository, plus every pre-existing test of both packages
8 a parked FOR SHARE answers within lifecycle.TombstoneTimeout TestCompleteUpload_BoundsTheLinkAtTombstoneTimeout, TestMountBlob_BoundsTheLinkAtTombstoneTimeout
9 no fixed path takes FOR SHARE on a repositories row and then updates it Not asserted by a test. LinkBlob's transaction writes blob_storage_attachments and container_blobs only, so the shape is absent by inspection, as the card states
10 a miss reports ErrRepositoryConcurrentlyDeleted, not ErrNotFound the tombstone subtest (require.NotErrorIs on ErrNotFound), TestCompleteUpload_SurfacesRepositoryConcurrentlyDeleted, TestMountBlob_SurfacesRepositoryConcurrentlyDeleted
11 blob upload initiate answers 404 Out of scope — part 2

Three obligations outside the card's list, also covered:

Source Obligation Tests
docs/specs/S09-authorization.md, #### Denial mapping and its "Read deny masks" criterion the masked 404 is byte-identical to the same route's genuine not-found the two unit 404 tests compare the whole entry against each route's own unknown-name baseline; the two race tests compare code, message and the whole detail map modulo the requested name
card constraint C1 the re-verification is the transaction's first statement TestContainerBlobLinker_LinkBlob_RepositoryGate/the_refusal_runs_before_the_attachment_insert
the widened seam each call site threads its own destination repository id TestCompleteUpload_LinksUnderTheDestinationRepository, TestMountBlob_LinksUnderTheDestinationRepository, TestUploadFinalize_LinksUnderTheResolvedRepository, TestMountLinksUnderTheDestinationRepository

The three tests named in ## The mount's own write deadline, and two new log lines are not in this table. The table came from the test author before that arm existed.

Scenario catalog

docs/testing/e2e/oci.md gains no new scenario. The e2e.oci.setup.delete-repository row at line 44 is widened instead, because the same DELETE ?destructive=true scenario now covers two more write paths. The row records six things:

  • The finalize and the mount answer 404 NAME_UNKNOWN after the tombstone, in place of 201.
  • A link whose own wait outruns its deadline answers 500 in place of that 404, and 499 with no body when the client has already disconnected.
  • The rolled-back transaction leaves no container_blobs row and no attachment row for the raced digest.
  • A finalize's committed blob_storage_blobs row survives as an orphan, the same way the manifest payload's row does.
  • A refused mount leaves the destination container_images row that its image upsert committed before the link, and the repository purge removes that row with the rest.
  • A mount that takes the fallback arm still answers 202, whether the source or the destination is tombstoned.

Diff size

1796 added lines is past the 500 reviewable lines at which docs/dev/development-model.md asks for a split or a justification. The split exists, and this is part 1 of it. Figures below are git diff --numstat origin/main...HEAD at e9920866c.

Group Files Added Removed
Production Go 4 217 117
Specs and contracts 6 120 33
Tests 20 1459 71
Total 30 1796 221

Production Go: internal/format/oci/upload.go (+138/-54), internal/datastore/container_blob_linker.go (+41/-38), internal/format/oci/store.go (+36/-23), internal/lifecycle/tombstone.go (+2/-2).

Specs and contracts: docs/specs/S12-container-oci-hosted.md (+82/-13), docs/specs/S20-a-lifecycle-closed-beta.md (+13/-5), api/openapi/v1.yaml (+12/-7), api/bruno/management-api/repositories/delete-repository.bru (+11/-6), docs/dev/configuration-reference.md (+1/-1), docs/testing/e2e/oci.md (+1/-1).

Tests: 20 files, of which internal/format/oci/blob_finalize_liveness_race_test.go is new (+367). The four next largest are store_test.go (+290), mount_test.go (+246/-40), internal/format/oci/upload_finalize_test.go (+207) and internal/datastore/container_blob_linker_integration_test.go (+205).

Why the split is what it is. Tests are 1459 of the 1796 added lines, which is 81 percent. No cut of issue 1122 puts a part under 500 reviewable lines. Each write path needs a staged-race test, a datastore integration suite and a handler arm. That floor alone is 500 to 600 lines.

A further cut of this part is not available either. internal/format/oci/store.go:282 and :489 are the only two call sites of the blobLinker seam. A change to the seam breaks whichever call site is not updated in the same commit. So part 1 is the smallest unit that compiles and fences either path. Each part is one coherent unit, and each part's description carries this split by file group.

Merge order

This merge request targets main. !2268 merged on 2026-09-04, so the dependency it carried is discharged. It supplied datastore.ErrRepositoryConcurrentlyDeleted, ContainerRepositoryStore.ReVerifyRepositoryAlive, NpmRepositoryStore.ReVerifyRepositoryAlive, and the two query_names.go constants those two methods register. main now holds all of them, through the squash commit cc507c94a. MavenRepositoryStore.ReVerifyRepositoryAlive was already on main and is not part of this dependency.

Two documentation lines overlap with other open merge requests. No pipeline reports either one, so both are stated here for whoever lands second.

  • !2052 rewrites the same docs/testing/e2e/oci.md row at line 44 and marks the scenario implemented. The resolution is a union: keep !2052's Status column and keep this row's Expected outcome. A second lander that takes one side whole drops the other half.
  • !2302 merged as the squash commit 71cdc5b1c, which is what put that Maven-publish row on main. The restack onto main resolved that adjacency, so this overlap is discharged.

Two corrections to apply to the squash commit message

The project squash-merges, so the body of 62a379fa3 becomes the main commit message. Two of its sentences are wrong, and the commit was not amended.

  1. The body says "It supplies ErrRepositoryConcurrentlyDeleted and both ReVerifyRepositoryAlive methods, none of which exist on main." Three such methods exist, and the Maven one is on origin/main at internal/datastore/maven_repositories.go:248. The corrected sentence is: "It supplies ErrRepositoryConcurrentlyDeleted and the npm and container ReVerifyRepositoryAlive methods, neither of which exists on main."
  2. The body says "including S12's mount status table". S12 carries no heading of that name. The heading is **Mount's status codes:** at docs/specs/S12-container-oci-hosted.md:780.

Why the commit was not amended. An amend needs a force-push on a merge request that the AppSec bot and the Reviewer Roulette have already read, and a force-push moves the anchors their threads point at. The restack onto main has since force-pushed the branch, so that commit is now 62a379fa3 and the anchors have already moved. The prose reaches main only through the squash message, so the corrections stay recorded here. Apply both corrections to the squash message at merge time.

One outcome past the bound is a 500, not a 404

blobLinkTimeout is lifecycle.TombstoneTimeout, which is 10 seconds. A link that parks on the repositories row lock past that bound answers 500 INTERNAL, not 404. The reason is the error chain: jet: timeout: context deadline exceeded carries no ErrRepositoryConcurrentlyDeleted, so it falls past the new 404 arm.

Measured on the wire, on both arms. The finalize was driven to the bound and answered 500 in 10.037 s. That run used a rig whose server.timeouts.write is 30 s (.claude/skills/run-artifact-registry/driver.sh:477), so it measured the finalize alone and says nothing about a parked mount. The mount was then driven to the bound on a rig set to the 10 s default. Before the write-deadline arm, curl exited 52 with an empty reply and HTTP status 000 at 10.021 s, while the server logged status:500 — recorded, never delivered. After the arm, curl exited 0 and received 500 with the INTERNAL envelope at 10.022 s. An unparked control on the same binary answered 201 in 0.024 s.

This is why acceptance item 8 was taken in scope. Without the bound, the fence's row-lock wait has no limit at all. Both siblings on !2268 arm the same bound, as publishCommitTimeout and manifestPersistTimeout. They are not merged: git grep on origin/main finds neither symbol.

What a client pays for a link that parks past the bound. session.Commit runs before the link and consumes the upload session. So the 500 arrives with the session already gone: the retry PUT gets 404 BLOB_UPLOAD_UNKNOWN, the pre-upload HEAD misses, and the client re-uploads the whole layer. internal/format/oci/blob_finalize_liveness_race_test.go:241-252 pins the residue behind this — no container_blobs row, no attachment row, and the committed blob_storage_blobs row still present.

The operator handle for a refused link is the log line, not the 404 count. writeRepositoryConcurrentlyDeleted (internal/format/oci/upload.go:1824-1846) writes the same 404 and NAME_UNKNOWN envelope that a route-parse miss and setErrorFromResolve write. No metric separates the three, so a dashboard built on the status code cannot tell them apart. The dedicated INFO line is the only signal that names the cause, and it carries the namespace id and the container repository id.

Contention on the fenced repositories row

A push meets the fence once per layer, not once per push. The manifest-push fence on !2268 takes the repositories row once per manifest push. This fence takes the same row once per blob link. So a ten-layer docker push meets the wait eleven times where it met it once before. The practical worst case is the counter drain, whose transaction holds FOR NO KEY UPDATE on the same row across Redis round trips with no context deadline. The drain's chunk size defaults to 500 (internal/config/storageaccounting.go:19) and its hard cap is 850 (internal/config/storageaccounting.go:92).

A change that reduces this exposure belongs on the drain, and no open merge request touches the drain. This merge request proposes no change to blobLinkTimeout.

The park count is unbounded, and this merge request accepts that for closed beta. A parked LinkBlob holds one pooled database connection for up to 10 seconds, where before it held one for two fast inserts. Nothing caps how many requests park on one row at the same time. S20-A's ## Error Cases table carries a new row for that park, and the acceptance is tracked in #1169. Accepting matches two records the project already carries. docs/specs/S20-a-lifecycle-closed-beta.md:1003 already accepts an unbounded park on this same repositories row for closed beta. That acceptance is tracked in #902. Merged main ships the same shape with no duration bound at all: MavenRepositoryStore.ReVerifyRepositoryAlive takes a bare FOR SHARE (internal/datastore/maven_repositories.go:284 on origin/main), called from internal/format/maven/upload.go:690 inside the request's own transaction, and neither that file nor its handler calls context.WithTimeout.

One contingency, flagged rather than swallowed. The deployed pool size is not readable from this tree, and pgxpool's own default is max(4, runtime.NumCPU()). If the deployed MaxConns sits at or near that floor of 4, this recommendation should be re-taken, because four parallel layers into a drain-locked repository then hold the whole pool. The settlement is one scrape of gitlab_artifact_registry_database_connection_pool_max_size on a running pod.

The mount route is only partly fenced, by design

mountFallback reaches CreateSession, which the LinkBlob fence does not cover. A tombstoned destination that takes the fallback arm still opens an upload session and still answers 202. Part 2 fences the image write and covers that arm. No comment and no test here claims the mount route is fully fenced. The S20-A ## Error Cases row, the S20-A acceptance criterion on the mount destination, and the docs/testing/e2e/oci.md row all state this residue.

Spec and contract amendments

  • docs/specs/S20-a-lifecycle-closed-beta.md (+9/-5) — the ## Error Cases row moves the finalize and the mount out of the set that reaches the window, and records the fallback-arm residue. A second ## Error Cases row is new: each blob-link park is bounded at blobLinkTimeout and the number of parks is not, accepted for closed beta and tracked in #1169. The acceptance criterion on the mount destination is scoped to the image tier, so a tombstoned destination repository stays the separate question the ## Error Cases row answers. That criterion also names which arm the repository-tier 404 reaches: ContainerBlobLinker.LinkBlob is the mount's only fenced write, so the mountFallback arm still answers 202 until the initiate is re-verified too. Three sentences citing S12's 404 ban narrow it from any mount failure to a source-side one, which is what S12 now says.
  • docs/specs/S12-container-oci-hosted.md (+68/-12) — the table under the **Mount's status codes:** heading at S12:780 gains the tombstoned-destination 404 and a server-fault 500. The finalize and mount sequence diagrams and the mount SQL example gain the fence. The sentence that read "No other status codes are valid for mount responses" narrows to the source-side business outcomes it was really about.
  • api/openapi/v1.yaml (+12/-7) and its Bruno mirror api/bruno/management-api/repositories/delete-repository.bru (+11/-6) — the DELETE ?destructive=true description records the same two outcomes.
  • docs/dev/configuration-reference.md (+1/-1) — the server.timeouts.write row named five in-code re-armers of the write deadline. The mount is a sixth, and this merge request is what made it one, so the row now names it and gives its value the way the management-API repository delete's is given. The configuration-reference guardrail is not triggered, because no file under internal/config/** and no config.example.yaml changes here.

The 499 review answer amends four of the same documents again: the S12 mount status-code table and the sentence that made an infrastructure fault a 500, the S12 finalize handler-transaction bullet and its Error cases table, the S20-A blob-link ## Error Cases row, and the e2e.oci.setup.delete-repository row. No OpenAPI or Bruno change rides with it, because docs/dev/api-style.md keeps 499 out of every OpenAPI document.

S12 was already stale on main, for two reasons that this change did not create. A tombstoned destination already answers 404 from the authorization middleware, before handleMount runs at all. A mount already answers 500 from three arms of handleMount's own body, and this merge request's write-deadline arm makes four. The amendment shows that derivation rather than asserting it.

The S12 amendment and the two upload.go mount-contract comment corrections are the same finding on two surfaces. They were reconciled together and deliberately. A spec amendment that adds a status code, shipped beside comments in the same diff that still assert a 201/202-only mount, contradicts itself inside one merge request.

One over-general claim survives, and is disclosed rather than corrected

Three sweeps ran over every added line for every, only, all, never, both, each and none, and each hit was checked against the code rather than against the sentence beside it. Four instances were found and corrected, two of them written by the same pass that was correcting the others. One survives.

The equivalence claim for the fallback 202 still stands unqualified. S12:794 says the source-side outcomes take the 202 fallback "byte-identically", the status table at S12:786-792 states the same equivalence, and S20-a:1015 and docs/testing/e2e/oci.md:44 repeat it in other words. The session UUID differs per request, so two such responses are not byte-identical. mountFallback's own body comment already carries the qualifier (internal/format/oci/upload.go:795-799), and so does internal/format/oci/blob_finalize_liveness_race_test.go:366. Widening that qualifier into the three specs and the catalog is a fresh finding, not part of this fix. That finding is filed as #1190, with every coordinate re-derived at head 60fc9a3ef, the two unqualified claims upload.go already carries on origin/main, and the correction that O22 answered the test assertion rather than the mountFallback doc comment.

Comment caps: two treatments taken, four blocks declined

scripts/ci/check-comment-caps.sh charges every line of a comment block that a diff touches, so a one-line correction inside an over-cap block forces the whole block to the cap. docs/dev/go-style.md gives five ordered outcomes for that case, and each block was taken on its own.

Outcome 1 selected the treatment for the ContainerBlobLinker type doc. Its trigger is that the claim fits one line once the concept has a name, and every noun here was lifted rather than coined. That block went from 26 counted lines to 3 and kept all six of its claims (internal/datastore/container_blob_linker.go:15-17).

Outcome 2 selected the treatment for CompleteUpload. Its trigger is that a site with room is empty, so the sentinel went on the signature's closing line at internal/format/oci/store.go:231.

Four blocks keep a stale enumeration, and docs/dev/go-style.md:277 requires this list.

Block What is stale in it Where the fence is stated instead
internal/format/oci/store.go:201-204 and :209-211, inside the CompleteUpload doc at :197-231 :201-204 gives a two-insert account of the transaction that omits the fence; :209-211 names the two sentinels the caller maps, not the third this change adds the trailing comment at store.go:231, and the parameter comments at store.go:173-174
internal/format/oci/store.go:150-157, inside the blobLinker seam doc at :143-168 a numbered two-insert account of the transaction the parameter comments at store.go:173-174
internal/format/oci/store.go:412-413 and :421-422, inside the MountBlob doc at :407-435 the same two-insert account, plus "MountBlob performs only the existence check and the link", which no longer accounts for the re-verification the trailing comment at store.go:446, the body comment at store.go:486-487, and store.go:173-174
internal/datastore/container_blob_linker.go:43-44, the LinkBlob doc "the blob_storage_attachments + container_blobs inserts" the parameter comments at container_blob_linker.go:58-59, and the type doc at :15-17

Compressing store.go:197-231 trades that doc's rendered API contracts for two omissions, inside a fix merge request. Each of those contracts is restated in the package or enforced in code, but in body comments and at call sites that go doc does not render. go doc also strips the zero-cost site at :231. That fact therefore reaches a reader of the source, and not a reader of the rendered package documentation.

This rests on a reading, and the reading is reversible. store.go:209-211 opens by naming expectedDigest and attributes each mapping to Commit, and the body block above the session.Commit call makes the same two-sentinel claim. On that evidence the paragraph is scoped to Commit rather than a complete caller mapping. A reviewer who reads store.go:209-211 as a complete caller mapping should ask for the compression instead. That change is one compress and it moves no behaviour.

Two hook deviations during development, both accepted and explained

The branch is ten commits above 11234c958. Both deviations below happened on intermediate commits that the phase-5 squash removed, so git log on this branch shows neither. The nine commits added after that squash each ran the full hook chain with no bypass of any kind. They are disclosed because the process is what a reviewer is entitled to weigh, not because anything on the branch still carries them.

One --no-verify, on the test-first authorship commit. That commit carries tests which fail by design before the implementation lands, so the go-test hook refuses it. This is the project's documented test-first carve-out, and the branch uses it exactly once.

One SKIP=comment-caps, on the commit that added the fence. This is not --no-verify: every other hook ran on that commit and passed. The comment-caps hook runs scripts/ci/check-comment-caps.sh --base origin/main, not --cached, so it measures the whole branch diff rather than the staged hunks. The test commit had spent the branch's one --no-verify and so never met that gate. That left 29 over-cap blocks, which blocked a commit that did not create them. The next commit compressed all 29 blocks.

The check exits 0 from that point onward, no later commit used any bypass, and the squashed commit passes it too.

One note on the base this hook measures. On a stacked branch, --base origin/main also measures !2268's diff. The exit 0 is still a true statement about this branch's own blocks, because !2268 is caps-clean.

Lint ran at golangci-lint 2.13.2

Every golangci-lint run on this branch used version 2.13.2, which mise provides and which .tool-versions pins as 2.13. AGENTS.md guardrail 7 names 2.12.2, so the version that guardrail states did not run. The golangci-lint on this machine's PATH is 2.12.2 and cannot load the current .golangci.yaml, which enables exhaustruct_v5.

Verified at runtime

The service was built and driven for real, and the mid-request race was staged through the fence's own lock rather than simulated. Three properties make that staging work:

  • An uncommitted UPDATE repositories SET soft_deleted_at holds FOR NO KEY UPDATE on the row.
  • Both of the request's resolves are plain SELECTs, so each one still sees the row live.
  • The fence's FOR SHARE OF repositories parks until the holder of the lock commits.

Every exercise passed:

  • Finalize, live repository: 201 with Location and Docker-Content-Digest. Raced: parked, then 404 NAME_UNKNOWN. Both framings ran, PUT ?digest= and POST ?digest=.
  • Mount, live: 201, with one shared blob_storage_blobs row. Destination raced: parked, then 404, and the dead destination took no container_blobs row.
  • Mount with a tombstoned source: 202 with a normal session, and a PUT through that session returned 201. This is the arm a careless fence breaks, and it holds on the wire.
  • Six negative source cases (absent, unknown, malformed, cross-namespace from=, malformed ?mount=, digest absent): all 202 with a session.
  • DELETE ?destructive=true: 202, tombstone stamped, purge job running. Also 400 with no parameter, and 409 under destructive=false.
  • Residue after a refused finalize: zero container_blobs rows, zero blob_storage_attachments rows, one blob_storage_blobs row. That row is the predicted reclamation orphan.
  • All three raced 404 bodies are character-for-character the route's own unknown-name envelope with the name substituted. That is the S09 masking criterion observed rather than argued.

A later run measured the parked mount on a 10 s rig, before and after the write-deadline arm. The section ## One outcome past the bound is a 500, not a 404 carries those figures.

Conformance ran in CI, not locally

Conformance is a guardrail for OCI protocol work, and it did not run on the development machine. mise run conformance depends on db:setup, which publishes port 5432, and the system PostgreSQL instance already holds that port. Calling scripts/conformance/run.sh directly needs a DSN from the local environment file, which the agent safety net blocks from reading. Treat the pipeline job conformance:oci:s3-garage as the gate for this merge request.

Open review threads on !2268 that reach this branch

Three of !2268's six unanswered review threads bear on this code. The exposure is bounded and it is named here so a reviewer can read it rather than find it.

Thread What it asks What it costs this branch
641cf0e1 SET LOCAL lock_timeout in place of a transaction-scoped deadline about 60 added lines of production code, which is under 5 percent of the diff, plus the test lines named below
1d1667a3 a reword of the constants' trailing comment 1 line. This branch's wording is no longer !2268's
1c85713d the Maven arm adopts the sentinel, or the sentinel's doc names only the arms that produce it 0 lines today, but this branch adds a third producer

On thread 1d1667a3, the divergence is deliberate. blobLinkTimeout's trailing comment at internal/format/oci/store.go:183 reads "past it the handler answers 500, not the 404". !2268's own answer to that thread reads "past it the answer is 500". On the mount arm the handler can record a 500 the client never receives, unless the write deadline this branch arms covers the wait, so this branch's wording states what the handler does and not what the client gets.

The sites that depend on thread 641cf0e1 are these, and nothing else moves:

  • internal/format/oci/store.go:182-183 — the blobLinkTimeout constant and its doc.
  • internal/format/oci/store.go:281 and :488 — the two context.WithTimeout wrappings. The seam's own calls are at :282 and :489.
  • internal/format/oci/upload.go:492-493mountWriteTimeout, which adds blobLinkTimeout into the mount's write budget.
  • internal/format/oci/store.go:185-195 and internal/format/oci/upload.go:605-612errLinkOutranItsBound, linkFailure and boundedLinkMessage, which all key on that deadline expiring.
  • internal/lifecycle/tombstone.go:54-56 — the sentence that names the bound.
  • internal/format/oci/store_test.go:923-947, :1001-1024 and :1047-1103, and internal/format/oci/mount_test.go:1418-1442 — 131 test lines together.
  • api/openapi/v1.yaml, api/bruno/management-api/repositories/delete-repository.bru, and one clause inside docs/specs/S20-a-lifecycle-closed-beta.md:1006.

The rest is untouched under every outcome:

  • the fence itself and the reVerifyRepository helper
  • the sentinel mapping
  • the widened seam and both of its call sites
  • the two 404 NAME_UNKNOWN arms
  • all but 131 of the 1284 added test lines, the 367-line race file included

An adjacent gap found during this work

Issue #1161 records a separate defect on the mount source lookup. ContainerBlobStore.FindBlobInRepository filters container_images.soft_deleted_at but never joins repositories, so a blob in a tombstoned source repository is still found and the mount answers 201 instead of 202. This defect is pre-existing, adjacent and out of scope here. Issue 1122 fences each write path on its own destination repository, and no part of that work reads the source repository's row. The docs/testing/e2e/oci.md row at line 44 qualifies its source claim to "tombstoned before the mount resolves it". Issue #1161 is where the other reading is recorded.

Database Review Evidence

Note

Collected with the db-review-prep skill, query mode. Migration mode did not run, because the branch changes no file under internal/datastore/migrations/sql/. The SQL below is the exact text that .Sql() returns for each jet chain. It was rendered before this branch's last rebase, and all three chains are unchanged at 04d105f5a. No EXPLAIN output accompanies it. The skill plans each statement against an ephemeral PostgreSQL 17 container. This session's permission system refused both docker run and a direct psql connection. The index findings therefore come from the DDL, not from a plan. The Query notes block states what that limits.

What the change does to the transaction

datastore.ContainerBlobLinker.LinkBlob ran two inserts in one transaction. It now runs a repository-liveness re-verification as the transaction's first statement, and holds that row lock to the commit. The statement is ContainerRepositoryStore.ReVerifyRepositoryAlive (internal/datastore/container_repository.go:224), which came from !2268 (merged). This branch adds a new call site for it, inside a transaction that took no repositories lock before. Two call sites reach it: internal/format/oci/store.go:282 (blob finalize) and :489 (cross-repository mount).

Order Statement Lock it takes On
1 ReVerifyRepositoryAlive FOR SHARE one repositories row
2 blob_storage_attachments insert row insert, plus FOR KEY SHARE on the FK parents namespaces, blob_storage_blobs
3 container_blobs insert, ON CONFLICT DO NOTHING row insert, plus FOR KEY SHARE on the FK parents namespaces, container_images, blob_storage_attachments
3b conflict lookup, on a re-push of the same digest only none
4 COMMIT releases all

FOR SHARE OF repositories is qualified, so container_repositories stays unlocked even though the FROM list names it.

Queries

Statement Kind Predicate or target Index that serves it Partitions Rows
ContainerRepositoryStore.ReVerifyRepositoryAlive SELECT … FOR SHARE OF repositories container_repositories.namespace_id = $1 AND container_repositories.id = $2, joined to repositories on (id, namespace_id) pk_container_repositories (id, namespace_id), then pk_repositories (id, namespace_id) 1/64 and 1/64 1
BlobStorageAttachmentStore.Create INSERT … RETURNING no conflict target — the insert is unguarded by design FK probes hit unique_blob_storage_blobs_on_namespace_id_and_sha256 and pk_namespaces (id) 1/64 1
ContainerBlobStore.Create INSERT … ON CONFLICT DO NOTHING RETURNING ON CONFLICT (namespace_id, container_image_id, digest) unique_container_blobs_ns_id_ci_id_digest (namespace_id, container_image_id, digest), exact column match 1/64 1
ContainerBlobStore.Create conflict lookup SELECT … LIMIT 1 namespace_id = $1 AND container_image_id = $2 AND digest = $3 the same unique index, in leading order 1/64 1

Every FK parent of the two inserts is indexed on the referenced columns: pk_container_images (id, namespace_id), pk_blob_storage_attachments (id, namespace_id, sha256), unique_blob_storage_blobs_on_namespace_id_and_sha256, and pk_namespaces (id). Each of these was read from its own CREATE statement in internal/datastore/migrations/sql/.

Nothing here scales with repository count or blob count. Each statement is keyed by a full primary key or unique index and touches at most one row. The one quantity that grows is contention on that single repositories row, which rises with the concurrent write rate into one repository.

ContainerRepositoryStore.ReVerifyRepositoryAlive
SELECT container_repositories.id AS "container_repositories.id"
FROM public.container_repositories
     INNER JOIN public.repositories ON ((repositories.id = container_repositories.repository_id) AND (repositories.namespace_id = container_repositories.namespace_id))
WHERE ((((container_repositories.namespace_id = $1::uuid) AND (container_repositories.id = $2::uuid)) AND (repositories.format IN ($3, $4))) AND (repositories.kind = $5)) AND (repositories.soft_deleted_at IS NULL)
LIMIT $6
FOR SHARE OF repositories;

Bound args: [namespace_id, container_repository_id, 0, 3, 0, 1] — formats docker and oci, kind hosted, limit 1.

format, kind and soft_deleted_at are filters on the row that the primary-key probe already found. The partial (namespace_id, format) and (namespace_id, kind) indexes serve list queries, not this probe. LIMIT 1 is redundant against a unique key, and it is harmless.

BlobStorageAttachmentStore.Create
INSERT INTO public.blob_storage_attachments (namespace_id, sha256)
VALUES ($1::uuid, $2::bytea)
RETURNING blob_storage_attachments.id AS "blob_storage_attachments.id",
          blob_storage_attachments.namespace_id AS "blob_storage_attachments.namespace_id",
          blob_storage_attachments.sha256 AS "blob_storage_attachments.sha256";
ContainerBlobStore.Create and its conflict lookup
INSERT INTO public.container_blobs (id, namespace_id, container_image_id, blob_storage_attachment_id, digest, blob_sha256)
VALUES ($1::uuid, $2::uuid, $3::uuid, $4, $5::bytea, $6::bytea)
ON CONFLICT (namespace_id, container_image_id, digest) DO NOTHING
RETURNING container_blobs.id AS "container_blobs.id", ;

The lookup below runs only when the insert above returns no row.

SELECT container_blobs.id AS "container_blobs.id", 
FROM public.container_blobs
WHERE ((container_blobs.namespace_id = $1::uuid) AND (container_blobs.container_image_id = $2::uuid)) AND (container_blobs.digest = $3::bytea)
LIMIT $4;

Lock ordering

A sweep of every row-lock site in the repository found no deadlock-capable opposite-order path. The paths that could invert the order do not:

  • Purge and reap. RepositoryReaper.Reap (internal/datastore/lifecycle_reap_repository.go:297-340) reaps one page of artifacts, or finalizes, never both in one transaction. The artifact chunk reads repositories with a plain unlocked SELECT (readRepositoryReapDispatch, :387-392) and then deletes the manifest, tag, blob and attachment rows. It takes no repositories row lock at all. The finalize chunk locks only the repositories row, after unlocked reads.
  • Tombstone. Tombstoner.tombstoneTx (internal/lifecycle/tombstone.go:113-164) updates repositories and inserts a river_job row. It touches no blob table.
  • Unlink and manifest delete. ContainerBlobUnlinker.UnlinkBlob and ContainerManifestDeleter.deleteByDigest run no repositories statement.
  • Every other repositories writer runs one statement on the pool, not inside a transaction that also wrote a blob table.

One statement-order inversion does exist, and it is pre-existing. internal/format/maven/upload.go:573-586 inserts into blob_storage_attachments before upsertFileRow reaches ReVerifyRepositoryAlive at :690. It cannot close a cycle, because the attachment insert takes no repositories lock, and a Maven repository is never the same row as a container repository.

Lock duration and the bound

The repositories row is held from the transaction's first statement to the commit. Two statements run inside that window, or three on a re-push of the same digest. Nothing else runs there. The storage-layer session.Commit that lands the blob_storage_blobs row runs before LinkBlob is called. The membership read carries its own deadline and also runs before the link (internal/format/oci/store.go:274-277). The counter emits are dispatched after LinkBlob returns.

So the window holds no object-storage call and no other application work.

The bound is blobLinkTimeout = lifecycle.TombstoneTimeout, which is 10 * time.Second. It is a Go context deadline that covers BeginTx, the lock wait, both inserts and the commit. The service sets no lock_timeout and no statement_timeout, so this deadline is the only bound on the wait.

Query notes:

  • A link parked past the bound answers 500, not 404. Both OCI arms key on datastore.ErrRepositoryConcurrentlyDeleted. A deadline expiry returns context.DeadlineExceeded, which carries no sentinel, so it falls through to the CodeInternal default at internal/format/oci/upload.go:625-628 and :1494-1497. The refusal that the fence exists to produce is correct. The timeout on the same lock is the arm that reports a server fault for a wait that is expected under drain contention.
  • FOR SHARE and the counter drain's FOR NO KEY UPDATE conflict, so they queue on the same row. lockRepoScopesStmt (internal/datastore/counter_drain.go:695-715) says that FOR NO KEY UPDATE "leaves foreign keys referencing the row unblocked". That holds for the FOR KEY SHARE a foreign-key check takes, and it does not extend to FOR SHARE. The drain's transaction (internal/accounting/chunk_worker.go:403-428) also holds those locks across Redis round trips, with no context deadline. It is the practical worst case for the new wait, and blobLinkTimeout's own comment names it. This interaction is the subject of an open review thread on !2268 (merged) and is recorded here, not resolved.
  • The one shape the codebase forbids is absent here. internal/datastore/repository_parent_gate.go:158-161 names taking the share lock and then writing the same row. LinkBlob never writes the repositories row it locks.
  • No plan was measured. Partition pruning on the repositories side of the re-verification depends on the planner propagating container_repositories.namespace_id = $1 across the join equality. The predicate is present and the propagation is ordinary equivalence-class behavior, but this run did not observe it. One option is to re-run the skill's query mode where docker run is permitted. The lock-ordering finding does not depend on a plan: it rests on reading every row-lock site in the tree.

Review note 3788417617 asked for it and it is built here. Both link arms — finalizeAndRespond's final 500 and writeMountLinkError — now call writeLinkIfClientClosed first. It keys on the bare request context, stamps ev.httpStatus with the package's StatusClientClosedRequest, writes the bare header with no envelope, and logs at Warn.

One departure from the note's own wording: the failure logs at Warn rather than not at all. The bare context test also catches a session.Commit, UpsertContainerImage or digest-decode fault that only coincides with the hangup, because net/http cancels the request context on a hangup after a complete body. S12:623 and merge request !2223 (merged) keep that class as a logged fault, so the Warn line is what preserves its record while still taking it off the ERROR stream.

uploadEvent.emit is unchanged. It derives outcome from a status at or past 400, so a 499 books outcome=error, which is what pushEvent.emit books for the same status. pushEvent's only 499-specific rule excludes the status from manifestPushFailuresTotal, and uploadEvent.emit increments no counter.

The bare return in handleMount's canceled-source arm is untouched. It writes no header at all, which TestMountCanceledContextAbandonsWithoutWriting pins, and copying it onto the link arms would leave ev.httpStatus at zero.

Four handler tests cover the two arms and both sides of the guard: TestUploadPUT_LinkErrorAfterClientHangupReturns499, TestUploadPUT_LinkErrorWithLiveClientStays500, TestMountLinkErrorAfterClientHangupReturns499 and TestMountLinkErrorWithLiveClientStays500.

Seventeen stale S12 line citations are declined, not overlooked

internal/format/oci/upload.go carries 17 comment sites naming a line number inside docs/specs/S12-container-oci-hosted.md, 18 numbers in all, and not one of them resolves to the text it claims. docs/dev/go-style.md's ### Reference only what does not rot forbids the form.

They are not this merge request's to fix, and this is go-style.md's fifth ratchet outcome taken deliberately. All 18 were correct when written, in commit 692eac148, and all 18 were already wrong at this branch's merge base; this branch moves two of the numbers onto different wrong lines and falsifies none of them. Retiring them is expensive rather than cosmetic: scripts/ci/check-comment-caps.sh charges every line of a block the diff touches, and the blocks these sit in include a 7-line and a 14-line block under names capped at 1 line, so the sweep forces a compression pass across most of the mount half of the file's comments.

The sweep is filed as #1189, with the coordinates, the re-derivation command, and the three sibling citations in mount_test.go and mount_integration_test.go that belong to the same rule.

Related to #1122

What remains: the blob upload initiate, the npm dist-tag PUT, the npm deprecate, and the management-API container tag upsert, in parts 2 to 5.

This is a bot message 🤖 — /smurfit

Edited by Pawel Rozlach

Merge request reports

Loading
Loading