chore(datastore): versions leg on both remote arms (S20-A plan: 20a/24)

What this delivers

NpmRemotePackageReaper.Reap and MavenRemotePackageReaper.Reap now remove the version-type rows under the cache package they receive. Each new leg draws from the one budget the Reaper contract bounds. The parent row still goes on the same call as a short page, so the per-call ceiling stays at 2*limit.

npm grows two legs. The first leg removes the cache files under each cache version. The second leg removes the npm_remote_versions rows. The files leg cannot be an npmAttachmentHolder, so it reaches through npm_remote_versions; index_npm_remote_versions_on_ns_id_pkg_id and index_npm_remote_files_on_ns_id_ver_id are what answer its two equalities, and neither is partial, so a state-blind page can use both. Maven grows one leg over maven_remote_versions, and its file leg loses the MavenRemoteVersionID.IS_NULL() conjunct. index_maven_remote_files_on_ns_id_pkg_id already serves the wider predicate.

Both arms now report Components for the version-type rows they remove. npm_remote_versions and maven_remote_versions are two of the six tables ADR-007 sums for a namespace. The npm arm now reports Drained on its parent-taking branch, as its Maven twin already did. This change adds three query_names constants, with one use each.

This step adds no scan and registers no level. Nothing dispatches these two arms outside the walk of RepositoryReaper until Step 20b merges. No spec file changes here.

The cause was a caller contract, not a missing walk

Neither arm reached any version-type row. The npm arm walked only npm_remote_metadata_files. The child predicate of the Maven arm carried MavenRemoteVersionID.IS_NULL(). A cache package that still held a cache version therefore failed on its foreign key and returned ErrReapParentPinned on every attempt. No re-queue cleared that state, because nothing in either arm removed the pinning row.

Issue 728 describes the missing work as a net-new downward walk from the parent tombstone. It calls that walk the larger half of the work. In merged code the work is a versions leg on two arms that already exist, and it is the smaller half. This description carries the correction.

Coverage

Rows come from the plan's Acceptance bullets for Step 20a, not from a spec. No spec is amended by this step.

Plan: docs/plans/2026-08-11-s20a-lifecycle-closed-beta.md (Step 20a)

# Criterion Arm Tests State
AC-1 Each arm drains a subtree that fits the budget in one call, parent row included, and reports Drained npm _DrainsTheVersionsBeneathThePackage new, fails on ErrReapParentPinned
AC-1 maven "a cache version row beneath the package is drained with it" rewritten, fails on ErrReapParentPinned
AC-2 A wider subtree drains across calls, with the per-call ceiling asserted at the two limits straddling the parent-taking branch npm _StraddlesTheParentTakingBranch new, fails on ErrReapParentPinned
AC-2 maven "the cache package row goes on the call that has budget left" new, fails on ErrReapParentPinned
AC-3 Every version-type row a leg removes is counted in Components, and the package row is not npm _DrainsTheVersionsBeneathThePackage; _RemovesTheMarkedPackageAndItsCache pins the zero for a version-less subtree new; existing
AC-3 maven "a cache version row beneath the package is drained with it"; "the totals carry no component, no size and no repository" pins the zero rewritten; existing
AC-4 Each new leg's delete is EXPLAIN-asserted index-backed, with no sequential scan and no post-scan sort both none NOT COVERED: needs the statement builders this commit may not write; see the handoff note below
AC-5 Each arm still refuses with ErrReapParentPinned on a cause its legs do not clear npm _RefusesOnACacheRowAShortPageLeftBehind new, passes before and after: it replaces the sentinel coverage AC-1's npm rewrite removes
AC-5 maven "a sibling chunk's committed page shortens this one" on both arms existing, untouched
AC-6 A repository purge over each remote family reaps the same rows and reports the same totals as before both TestRepositoryReaper_Reap_WalksEveryFormatAndKindArm, plus every other case of both remote suites existing, green before and required green after
AC-7 Both arms still report no bytes and name no repository npm _DrainsTheVersionsBeneathThePackage asserts SizeBytes zero and RepositoryID uuid.Nil new
AC-7 maven "a cache version row beneath the package is drained with it" asserts the whole ReapTotals struct rewritten

The table above is the test author's, and it travels unedited. The AC-4 cell no longer describes the branch head, and three Maven cells quote a title shorter than the merged one, so this section states each correction.

AC-4 is covered now. The row reads NOT COVERED and points at a handoff note that lives outside this merge request. TestRemoteCachePackageReapPages_ReadTheirPackageIndexes closes it, at internal/datastore/lifecycle_explain_integration_test.go:652. It runs four subtests over index_npm_remote_versions_on_ns_id_pkg_id, index_npm_remote_files_on_ns_id_ver_id, index_maven_remote_files_on_ns_id_pkg_id and index_maven_remote_versions_on_ns_id_pkg_id. Each subtest asserts a single partition, no Seq Scan on a partition, and no Sort. The test sits at the end of the file, clear of the insertion point that !2040 uses.

Three Maven cells quote a title shorter than the merged one. The AC-3 row names "the totals carry no component, no size and no repository", and the merged subtest is "with no cache version the totals carry no component, no size and no repository", at the same line. The condition in the longer title is what makes the zero true, so it states the same fact more exactly. The AC-2 row names "the cache package row goes on the call that has budget left", and the merged subtest is "the cache package row goes on the call that has budget left, and no call outruns the ceiling". The AC-5 row names "a sibling chunk's committed page shortens this one", which is the prefix two subtests share, one per arm, and the row's own "on both arms" says so. Each quoted title is a substring of the merged one, so a search on any of them reaches the subtest. The two titles this change set writes have stood since its first commit, 7c8e3c980: git show 7c8e3c980:internal/datastore/lifecycle_reap_maven_remote_integration_test.go | grep 't.Run(' prints both.

Runtime evidence

A local run of the service drove a repository purge over a remote npm repository and over a remote Maven repository. Both purges reached drained. The new statements ran on the production route, inside the process, against a real database.

gitlab_artifact_registry_database_queries_total carried no reap counter before the two deletes. After both deletes it carried these counts:

  • npm_remote_versions_delete_reap_package_page: 3
  • npm_remote_files_delete_reap_package_page: 3
  • maven_remote_versions_delete_reap_package_page: 2

Each count is one call per cache package row, which is what the shape of the arm predicts. The new legs removed no rows, and that result is what makes this merge behavior-preserving. The walk of RepositoryReaper places the version level before the package level, so the version arms took every row first. driver.sh smoke reported 72 passed and 0 failed.

The base binary was not built and not run, so no A/B measurement stands behind AC-6. The evidence for AC-6 is the green suites plus two code facts. reapRepositoryLevelPage sums only Rows and Components from a sub-reaper and never reads its Drained. The walk orders versions before packages on both remote arms.

e2e scenario catalogs

No scenario is added, and no scenario moves.

e2e.npm.remote.evict-package at docs/testing/e2e/npm.md:183 says the evicted rows stay in the database and that no sweep reaches them. That row still holds, because this change registers no scan level.

Two further rows touch remote eviction. e2e.maven.lifecycle.management-delete-storage-counters describes the bulk delete job evicting cached remote Maven rows. e2e.npm.remote.url-change-evicts is a second eviction scenario in the npm catalog. Neither row moves. The Maven row attributes the remote correction to reconciliation rather than to the purger, so no purge-side assertion in it changes.

e2e.npm.setup.delete-repository and e2e.maven.setup.delete-repository are the catalog rows for the one live path this change reaches. Both assert only externally visible outcomes. The route answers 400 without destructive and 202 under destructive=true. Reads fail after the delete, and a create at the same name answers 409 until removal finishes. The local run reproduced those outcomes, so neither row moves.

Reviewable LOC, and why a split does not help

Guardrail 18 asks for a split or a justification past 500 reviewable lines. The diff is 1509 added and 468 removed lines over 13 files. Command: git diff --numstat c9d11b1e0..HEAD, at head 73511cecb.

The base is the merge base c9d11b1e0, and this branch sits on origin/main's ancestry. !2037 merged at 2026-09-01T09:37:01Z as squash commit 144f06431, and git merge-base --is-ancestor 144f06431 73511cecb exits 0. So the diff carries this branch's own 13 files, and the plan and spec files of !2037 are on main rather than in it. Neither docs/plans/2026-08-11-s20a-lifecycle-closed-beta.md nor docs/specs/S20-a-lifecycle-closed-beta.md appears in git diff --name-only c9d11b1e0..HEAD.

Each file in the table below carries its own added and removed pair.

Group Files Added Removed
Production internal/datastore/blob_storage_attachments.go 1/51, internal/datastore/lifecycle_reap_maven_remote.go 78/72, internal/datastore/lifecycle_reap_npm.go 5/59, internal/datastore/lifecycle_reap_npm_remote.go 176/88, internal/datastore/lifecycle_scan.go 3/20, internal/datastore/query_names.go 3/0 266 290
Tests internal/datastore/lifecycle_explain_integration_test.go 228/0, internal/datastore/lifecycle_reap_maven_remote_integration_test.go 219/52, internal/datastore/lifecycle_reap_maven_remote_test.go 39/0, internal/datastore/lifecycle_reap_npm_remote_integration_test.go 555/98, internal/datastore/lifecycle_reap_npm_remote_test.go 33/0 1074 150
Docs docs/dev/storage-accounting.md 167/25, internal/datastore/lifecycle_reap_npm.md 2/3 169 28
Total 13 files 1509 468

The production change is 266 added lines over six files, and a reviewer reads that part in one sitting. The tests and docs/dev/storage-accounting.md exist to explain those 266 lines. internal/datastore/lifecycle_reap_npm.md is 2 added and 3 removed lines, and it repoints one clause at that document. The two arms must move together, because the plan grades them as one pair and both share the same document sections. The comment-caps repair touches the same comment blocks as the production change, so a separate merge request cannot carry it. A split therefore produces smaller merge requests that each need the others to be reviewable.

Merge order

!2037 carried the plan this step implements, and it merged into main at 2026-09-01T09:37:01Z as squash commit 144f06431. That order is therefore satisfied rather than pending, and GitLab retargeted this merge request from the plan branch to main.

This branch has since been rebased onto origin/main, so the diff carries the 13 files the ## Reviewable LOC table lists. docs/plans/2026-08-11-s20a-lifecycle-closed-beta.md and docs/specs/S20-a-lifecycle-closed-beta.md reach this branch through 144f06431 on main rather than through the diff.

Step 20b's merge request does not open until this one merges. If a level over npm_remote_packages or maven_remote_packages runs before this step merges, the package DELETE aborts every chunk with ErrReapParentPinned. That stall is level-scoped, and the sweep re-queues it forever. Once this step merges, both arms drain their versions in the same call, and no level can dispatch a parent that a version pins.

Accepted smells

The plan names two, and both hold in the merged code.

The schedule. "this step lands a leg that no live path exercises until Step 20b merges." RepositoryReaper's walk orders npmRemoteReapWalk and mavenRemoteReapWalk children before parents, so on the one production route that reaches these arms today every version is already gone when the package reaper runs and the new legs find nothing. One interleaving is the exception: a cache fill already in flight when the repository was tombstoned can commit a version row that the leg drains, where the same chunk aborts with ErrReapParentPinned today. The end state is the same on both sides, so what the exception moves is what one chunk removes rather than what the purge leaves behind. That is what makes the merge behavior-preserving and the review bounded by the code the step adds, and it is also what makes the step unverifiable end to end on main: only its own suites exercise the legs there.

The seam. "the shared budget crosses every leg boundary as a bare int, with no type separating a remaining budget from the limit the Reaper interface hands in, and the drained gate becomes three conditions rather than one boolean." "Nothing type-checks that a leg charges what it removed, and the symptom of a leg that forgets is a ceiling breach no unit test reaches."

This is the one a reviewer of this diff has to check by hand. reapNpmRemotePackageChildren subtracts from budget after each leg and returns cacheDrained && filesDrained && versionsDrained && budget >= 1. MavenRemotePackageReaper.Reap holds filesDrained, versionsDrained and budget < 1 as its own three conditions. Every subtraction and every conjunct is a place the compiler does not help.

A third smell the plan does not name: this change set and !2123 both write docs/dev/storage-accounting.md. Three of this change set's edits to that file landed after the first version of this description, one line each. Two of them sit inside ### Remote npm cache reapers: the file-leg sentence in b8f13e17d, and the child-substitution repair in a84b0a983. !2123 writes that same section, so whichever of !2162 and !2123 merges second resolves the conflict by hand. That is an accepted cost rather than an oversight. The third edit is the Maven file-leg sentence in b8f13e17d, inside ### Remote Maven cache reapers, and no open merge request writes that section. !2040 and !2202 each end a hunk on that section's header and its blank line, as trailing context, with their changed lines stopping about three lines above the header. So that sentence is likely to auto-merge today, and it is the first thing a rebase of either merge request breaks.

Notes for the reviewer

A merged assertion moved that the plan does not argue. TestNpmRemotePackageReaper_Reap_IsIdempotent asserted ReapTotals{} on a duplicate delivery. That assertion pinned the npm package arm never setting Drained at all. It now asserts ReapTotals{Drained: true}, still as a whole-struct compare, so Rows and Components stay pinned at zero beside it. This brings the npm package arm onto the shape that three of the four duplicate-delivery sites hold. The fourth site is TestNpmRemoteVersionReaper_Reap_IsIdempotent, which keeps ReapTotals{} and is correct there, because NpmRemoteVersionReaper.Reap assigns Drained on no path.

The large comment-compression hunk is the fix for a latent CI failure, not tidying. scripts/ci/check-comment-caps.sh --base origin/main already failed at the pre-existing HEAD, independent of this change. An unmodified internal/datastore/lifecycle_reap_maven_remote_integration_test.go drew 8 of the 45 failures. Its CI twin is lint:comment-caps, so that failure was on course to take the pipeline red on a commit that was already pushed. Counting // lines across the Go half of the diff, the change removes 370 and adds 180. The two counts are git diff c9d11b1e0..HEAD -- '*.go' | grep -cE '^-[[:space:]]*//' and the same pipe with '^\+[[:space:]]*//', at head 73511cecb. The reasoning moves into docs/dev/storage-accounting.md, under ### Remote Maven cache reapers, ### Remote npm cache reapers and ### Hosted npm reapers.

internal/datastore/blob_storage_attachments.go is net -50 lines for the same reason. Its merged 52-line releaseNpmAttachments doc met a cap of 1. The text moved into the document and was corrected there, rather than gutted in place.

The comment-line pair in 7c8e3c980's commit body is wrong, and the commit is not amended. That body says "213 comment lines were removed and 44 added, comments only", and no scope reproduces that pair. git show 7c8e3c980 -- '*.go' | grep -cE '^-[[:space:]]*//' and the same pipe with '^\+[[:space:]]*//' return 259 removed and 138 added. Narrowed to internal/datastore/lifecycle_reap_maven_remote.go, those two commands return 67 removed and 23 added. Across the whole branch from c9d11b1e0 the pair is the 370 and 180 above. Every count in this paragraph was measured at head 73511cecb. That body is not amended, and this description carries the correction instead.

One comment block is left as it stands, and this paragraph is the disclosure docs/dev/go-style.md asks for. The block is internal/datastore/lifecycle_reap_npm_remote.go:162-168, inside the if err != nil of the npm remote version delete. Its closing clause reads "Every wrap this file adds follows it", and the file now also carries a leg whose wrap is plain, so the sentence reads two ways. git grep -n 'fmt.Errorf\|mapReapParentDeleteError' 73511cecb -- internal/datastore/lifecycle_reap_npm_remote.go returns three mapReapParentDeleteError wraps and two plain fmt.Errorf wraps, where the same command at c9d11b1e0 returns two mapReapParentDeleteError wraps and no plain wrap. The block itself is untouched: git diff -U0 c9d11b1e0 73511cecb -- internal/datastore/lifecycle_reap_npm_remote.go puts the file's hunks at new lines 3-4, 9, 178-180, 196-197 and later, so :162-168 falls between hunks and is byte-identical to origin/main. Editing one sentence inside the block charges every line of the block against the caps gate, and this change set did not falsify the sentence by editing it. docs/dev/go-style.md:271 gives that outcome, leaving the block alone and saying so here, and it asks for the block and the reason "since an undisclosed decline reads as an oversight".

Three commits answer guardrail 17 of AGENTS.md, and each landed after the first version of this description. That guardrail says a comment must describe the code as the same change leaves it, and agree with its call site.

b435e9c1e repairs the doc comment on reapNpmTags in internal/datastore/lifecycle_reap_npm.go. That comment cited deleteNpmAttachmentHolderBatch as the statement of what the short count is evidence of and what arbitrates it. Commit 2f79a06c4, earlier on this branch, made that citation false when it compressed the cited doc to one line. A repoint was not available. scripts/ci/check-comment-caps.sh gives an unexported doc a cap of one line, and it joins the whole 16-line run once the diff touches any of it. A one-line change reports the block at 14 lines, and deleting only the false segment reports it at 10. Both are past the cap of one line. So the block is compressed to one line and its pointer, and the reasoning moves to docs/dev/storage-accounting.md under ### Hosted npm reapers. The relocated prose is stronger than what it replaced. It names fk_npm_tags_npm_version_id_npm_versions on the version arm and fk_npm_tags_npm_package_id_npm_packages on the package arm, where the Go doc said only "what arbitrates it". Both constraint names appear in internal/datastore/migrations/sql/20260612120400_create_npm_tags.sql and in internal/datastore/migrations/structure.sql.

dc569a149 repoints the same false citation at internal/datastore/lifecycle_reap_npm.md.

b6a616767 adds the guard test that ## What a step MR does not write into the plan describes below.

TestNpmRemotePackageReaper_Reap_FreesAnAttachmentBothCacheLegsHold is a regression pin, not a fix for a defect. A review reported the two attachment releases of the npm package arm as a latent ordering hazard. Measurement disproved it. Each leg's release runs after that leg's own delete, so the leg that removes an attachment's last reference is the leg whose release frees it. Its three budget cases pin the freed count and the surviving rows under a shared attachment, at a budget that takes the whole subtree, at one that splits it, and at one that leaves the parent for a later call. They do not pin where either release sits. The chunk frees the attachment exactly once whichever of the two releases takes the last reference, so the test fails if the cache-file leg's release is removed and passes if that release moves later. Commit 73511cecb rewrites the assertion message in TestNpmRemoteReapers_StatementOrder that claimed otherwise, and states the gap it leaves: no case in this file pins where the second blob_storage_attachments release sits. docs/dev/storage-accounting.md writes those six positions out for this arm, so a reordering that preserves every path falsifies the document with the suite green. Closing that gap is #1068, which states that the npm remote cache package arm's lock order is not this merge request's scope.

The doc block of TestNpmRemoteReapers_StatementOrder stays broader than its package subtest reaches, and this paragraph is the disclosure. That block sits at internal/datastore/lifecycle_reap_npm_remote_integration_test.go:1710, and it says nothing else in the file can see the order. The package subtest sees three of the arm's five statements, and its two -1 assertions record which two it does not. Narrowing the block in place is not available: scripts/ci/check-comment-caps.sh gives a _test.go comment block a cap of two lines and charges the whole run, and the block is 19 lines. An edit therefore costs a compression of all 19, which would delete the suite-observability and two-mutation analyses the block carries. That is the same trade #915 leaves undecided, so the block is left alone and said so here.

Two ptr(x) sites became new(x), and 22 neighbours did not. The two converted sites are the two cache version seeds this branch added. The 22 remaining ptr(...) calls in the same file predate the base of this branch. Converting them puts unrelated churn in a diff that is already past the reviewable line.

Two of the four EXPLAIN index assertions depend on the fixture's shape, and a guard on that shape is what holds them. The npm cache file page subtest of TestRemoteCachePackageReapPages_ReadTheirPackageIndexes asserts two indexes, and each half of that pair fails under a different nearby fixture. At 100 versions in a 638-row namespace, the versions half is answered on the primary key with the package id as a filter. At 100 versions in a 5100-row namespace, the files half moves to the namespace's blob_sha256 index. The relation is not monotone in fixture size. The nested select leaves the planner a Nested Loop, a Hash Join and a Merge Join, and each of the three reads different indexes.

Commit 329ab6f34 guards the fixture instead of loosening the assertion. The test now asserts four seeded counts immediately before the EXPLAIN transaction, and a comment at that assertion names the dependence. A drift therefore fails at the guard with the count, rather than inside a plan assertion. This was proven rather than argued. A move of targetFilesPerVersion from 2 to 3 stops the run at the guard, which reports 9 against 6, before any subtest opens.

Both alternatives were rejected for stated reasons. A drop of the index names removes the only assertion in the tree that requires index_npm_remote_files_on_ns_id_ver_id to be read. Silent acceptance leaves a test that passes only at the size it was written against. Such a test is a flake that waits for an unrelated change, and the person who hits it will not have these measurements.

The other three subtests are single-table two-column equalities. They read their one asserted index in every configuration measured, so they carry no guard. That asymmetry is deliberate.

Every stamp in this description is re-derived at head 73511cecb, on merge base c9d11b1e0, except the five figures the last paragraph of this section names. Four rebases have now orphaned the branch SHAs this description cited, and one round rewrote a commit body, so every branch SHA here has moved four times. The latest rebase moved this branch onto c9d11b1e0, 28 commits ahead of the merge base it sat on before. git range-diff --creation-factor=100 reports 22 of the 23 commits byte-identical across it. The twenty-third is the first commit, and its only content difference is the alignment column of internal/datastore/query_names.go, which main narrowed when it added a comment above the same block. So no figure changed meaning because of a SHA move.

Each command above was re-run at this head. One output moved on this rebase, and the new value stands in place of the old one: the two docs/dev/storage-accounting.md greps return :2266 and :2277 where they returned :2172 and :2183. The rebase brought in main commits that write that file above those lines. Every other command returned the figure printed beside it: 13 files at 1509 added and 468 removed, the whole-branch comment-line pair at 370 removed and 180 added, 259 and 138 for the first commit alone, 67 and 23 narrowed to internal/datastore/lifecycle_reap_maven_remote.go, enable_seqscan at 3 on the base and 5 at head with 64 across 28 files repository-wide, and LIMIT( at 2, 1 and 0 on the base against 2, 2 and 2 at head. One claim in this description carries no SHA and cannot be re-run at head: the scripts/ci/check-comment-caps.sh --base origin/main failure count of 45 under ## Notes for the reviewer describes the tree before this branch's compression commit, so it is a record of a past state rather than a command that reproduces.

Five figures elsewhere in this description are stale, and this paragraph carries their measured values instead of editing them in place. Each was already stale at the previous head f65c70865, so no rebase of this branch moved any of them. TestRemoteCachePackageReapPages_ReadTheirPackageIndexes sits at internal/datastore/lifecycle_explain_integration_test.go:675, not :652. !2040 merged as bf328105f and inserted its own test above it, so the AC-4 note's conclusion still holds: that test is the last one in the file. e2e.npm.remote.evict-package sits at docs/testing/e2e/npm.md:190, not :183. The releaseNpmAttachments doc that met a cap of 1 is 51 comment lines, not 52, counted at internal/datastore/blob_storage_attachments.go:877-927 on the merge base. !2040, !2123 and !2202 all merged on 2026-09-02, and all three are ancestors of the merge base the previous head sat on. So the third bullet of ## Accepted smells reads them as open when they are not, and docs/dev/storage-accounting.md auto-merged on this rebase rather than needing the hand resolution that bullet predicts.

GitLab Duo's review is recorded here as carrying no finding. Note 3769123980 reads "I finished my review and found nothing to comment on", and it raises no ask against the change set. It is recorded rather than replied to, because the thread is a digest with no question in it.

What the database review measured

A database review ran EXPLAIN over the four new page statements and posted its evidence as a note on this merge request. This section records four of its results, so that a reviewer who reaches the same observation finds the answer instead of filing it.

SET LOCAL enable_seqscan = OFF is what carries the no-Seq Scan assertion. Without that setting, all four plans put a Seq Scan on the outer arm at 5100 rows. The setting is the suite's own convention, and this change did not introduce it. grep -c enable_seqscan internal/datastore/lifecycle_explain_integration_test.go returns 3 at the merge base c9d11b1e0 and 5 at head 73511cecb. One occurrence on each side is a doc-comment mention, and the head runs the statement in two EXPLAIN helpers where the base runs it in one. Repository-wide at the same head, grep -rn enable_seqscan --include='*.go' . returns 64 occurrences across 28 files. A change to the convention is a question for the whole suite, and it is outside this step.

The page bound does not reach the outer arm. All four statements take the shape DELETE ... WHERE id IN (SELECT ... LIMIT n). The planner answers the outer IN with a Hash Semi Join over a Seq Scan of the whole namespace partition. At 205,100 rows the Seq Scan node's actual time runs to 18.539 ms of a 29.439 ms execution, to match 100 ids. So the per-page cost grows with the partition rather than with the page.

This shape predates the branch rather than arriving with it. deleteNpmAttachmentHolderBatch in internal/datastore/lifecycle_reap_npm.go and reapMavenRemoteFilePageStmt in internal/datastore/lifecycle_reap_maven_remote.go both carry it on origin/main. grep -c 'LIMIT(' returns 2, 1 and 0 for lifecycle_reap_npm.go, lifecycle_reap_maven_remote.go and lifecycle_reap_npm_remote.go at the merge base c9d11b1e0, against 2, 2 and 2 at head 73511cecb. This change therefore adds three statements of that shape. No free alternative is available either: under enable_seqscan = OFF the primary-key plan costs 8390.80 against 8242.12, a 2% margin.

The foreign-key trigger dominates the two version legs. Three consecutive runs of the npm version page at limit=50, in one fresh session, gave trigger times of 73.730 ms, 6.461 ms and 6.473 ms. That is about 67 ms once per connection, and then 0.129 ms per removed version row. On the second run the trigger is 6.461 ms of a 7.621 ms execution, so it is about 85% of a warm call at that limit. At a 1000-row page the same rate gives about 130 ms of trigger time. This is a property of the foreign keys rather than of this change. git diff --name-only 8edc18ef1..HEAD -- internal/datastore/migrations/ returns no files.

One warning of the review was withdrawn on measurement. The concern was that an index with the same leading column pair can be chosen in place of the index a page needs. Read against the live index catalog, that hazard cannot fire. Every same-leading-pair sibling is partial on soft_deleted_at, and the maven_remote_files one is partial on maven_remote_version_id IS NULL as well. None of the four statements carries a soft_deleted_at predicate, so none of those siblings is usable.

The warning's own claim needed a correction too. It said that each of the four tables carries ..._size_bytes, ..._created_at_id and ..._last_downloaded_at siblings on the same leading pair. That holds for the two versions tables only. npm_remote_files and maven_remote_files carry no such sibling. Their residual competitors lead with namespace_id and a different second column, _blob_sha256 and _bsa_id, which serve the namespace equality alone.

What a step MR does not write into the plan

Guardrail 4 of AGENTS.md says that a step merge request does not edit the plan file, the Status table included. !2037 merged, so the plan file is on main and this branch reaches it: git merge-base --is-ancestor 144f06431 73511cecb exits 0. The rule still holds, because the Status table has a single writer: parallel step merge requests collide wherever their rows are adjacent lines. So the widening this change set needs is recorded on the plan's standing docs(plans) merge request rather than committed here. The rest of this section states every other file where this branch and the Step 20a Files entry differ. It also carries two notes on the text of the plan.

The change set touches two files the Step 20a Files entry does not name, and !2264 adds both. git diff --name-only c9d11b1e0..HEAD lists 13 files, at head 73511cecb. Eleven of them are in that entry on main today. The other two are internal/datastore/lifecycle_scan.go and internal/datastore/lifecycle_reap_maven_remote_test.go.

!2264 (merged) is the standing docs(plans) writer for this plan section, and it adds both paths to the Files entry and internal/datastore/lifecycle_reap_maven_remote_test.go to the Tests entry, alongside the Status-row fill it already carries. While !2264 (merged) is open, Step 20a on main names neither path; once it merges, both appear. !2264 (merged) is independent of this merge request, and either one can merge first.

An earlier version of this section, and the body of the commit that adds internal/datastore/lifecycle_reap_maven_remote_test.go, said the widening came through !2190. !2190 merged carrying internal/datastore/lifecycle_reap_npm.go and internal/datastore/lifecycle_reap_npm.md, and it carried neither of these two. Review note 3780459940 measured that, and the commit body is rewritten to name !2264 (merged) instead.

Commit 2f79a06c4 corrects the doc comment on deleteNpmAttachmentHolderBatch in internal/datastore/lifecycle_reap_npm.go. The comment said that no delete before releaseNpmAttachments touches any of the four tables its guard correlates on. From that it concluded that the freed count is the same wherever the release runs. reapNpmRemotePackageChildren is the first caller with two attachment-freeing legs. One leg's npm_remote_metadata_files delete now runs before the other leg's release, which makes that claim false at this head. Guardrail 17 of AGENTS.md is the rule: a comment must describe the code as the same change leaves it, and agree with its call site. The corrected reasoning is in docs/dev/storage-accounting.md under ### Hosted npm reapers, because scripts/ci/check-comment-caps.sh gives an unexported doc a cap of one line. Commits b435e9c1e and dc569a149 correct two more citations of that same compressed doc. dc569a149 is what brings internal/datastore/lifecycle_reap_npm.md into the change set, and ## Notes for the reviewer states both commits above.

internal/datastore/queries_test.go is named by the entry and stays untouched, and that is not a gap. The entry asks for rawSQLTimedFunctions entries "where a statement is raw". No statement this step adds is raw. reapNpmRemotePackageVersionPageStmt, reapNpmRemotePackageVersionFilePageStmt and reapMavenRemotePackageVersionPageStmt each return the go-jet type pg.DeleteStatement, and the three call sites reach the database through execAffected or instrumentQuery. rawSQLTimedFunctions pins the functions that hand the driver a string instead, so none of the three belongs in it.

internal/datastore/lifecycle_reap_npm_remote_test.go is named by the entry, and commit b6a616767 closes the gap it left open. The Tests bullet of the entry asks for "the argument guards on the new statements", and no earlier commit on this branch satisfied it. TestNpmRemotePackagePages_SpentBudget now covers the early return that deleteNpmRemotePackageVersionPage and deleteNpmRemotePackageVersionFilePage take on a budget below one. That commit moves the branch toward the plan rather than away from it. /validate-issue graded the acceptance of this step 7 of 7 while the gap was open. That grade speaks for the Acceptance bullets, and not for the file-by-file Tests bullet.

The Step 20a Scope line anchors no longer resolve. The Scope cites internal/datastore/lifecycle_scan.go:173-178 and :199-208. Both resolved exactly on the cut of !2037 this branch was written against, where they carried the Reaper seam's three-condition ceiling and its parent-deferral shape. That seam has since moved into docs/dev/storage-accounting.md under ### The Reaper seam, and at 73511cecb the two ranges hold the lifecycle-scan argument error variables and tombstoneEligibleExpr's doc comment instead. git grep -n 'Three conditions put one call' 73511cecb returns docs/dev/storage-accounting.md:2266, and git grep -n 'A parent row is never deleted in the same call as a filled page' 73511cecb returns docs/dev/storage-accounting.md:2277.

The per-call bound. The plan's Acceptance says that no call reports Rows above 2*limit - 1. That claim is wrong read per call. 2*limit - 1 binds the parent-taking branch alone, and every call is held to 2*limit. Both arms' tests assert the correct pair, and three merged sources agree on it.

Related to #728

Issue 728 names three scan levels and a downward walk over npm_remote_packages, container_remote_images and maven_remote_packages. This change set delivers the versions leg on the npm and Maven remote arms. It registers no scan level, and it does not reach container_remote_images. The remainder is Step 20b of docs/plans/2026-08-11-s20a-lifecycle-closed-beta.md, whose Scope reads "Three tombstone-discovery scans and the three purge levels that dispatch them, over npm_remote_packages, maven_remote_packages and container_remote_images". The link form is Related to rather than Closes because this merge request delivers the downward walk and not the scan levels. Issue 728 stays open for the remainder.

This is a bot message 🤖 — /smurfit

Edited by Pawel Rozlach

Merge request reports

Loading
Loading