chore(datastore): remove a Maven file and its attachment together

Stacked on chore(datastore): count every table that refere... (!1450 - merged) • Hayley Swimelar • 19.3, merges after it.

Why

A Maven file is the one Maven artifact whose delete can finish inside the request. Its subtree is the row itself, so nothing waits on the lifecycle purger that the package and version deletes lean on. Nothing in the tree can remove one yet, so the Maven arm of DELETE .../files/{file_id} has no datastore call to make.

MavenFileDeleter.DeleteFile is that call: one transaction that hard-deletes a maven_files row and the blob_storage_attachments row it points at. The attachment has to go with it, because ADR-010 collects a blob once it has zero attachments, so an attachment left behind after its file row is gone holds its blob out of reach of every later pass.

Step 15 of the S17 Phase 4 plan. Step 22 lands the handler that calls this.

What

Two DELETE conjuncts look redundant and are not. Both are pinned against the emitted SQL, because dropping either is invisible to a happy-path test.

maven_version_id IS NOT NULL sits beside a maven_version_id = $3 equality that already drops NULL rows. The column is nullable and the row carries a second foreign key straight to maven_packages, so the package-level maven-metadata.xml rows and their checksum siblings hang off the package rather than any version. Stating the predicate means relaxing the equality cannot silently expose them, the same reason listMavenFilesByVersionStmt and findMavenFileByIDStmt state it.

soft_deleted_at IS NULL mirrors MavenFileStore.FindMavenFileByID's addressability rule, and it is stated ahead of the first writer that marks a maven_files row. No production writer marks one today: MavenFileStore.UpsertMutableFile overwrites the row in place through ON CONFLICT ... DO UPDATE and never sets soft_deleted_at. The conjunct is what keeps the read and write halves agreeing on which rows are addressable once a marking writer does land.

Statement order carries the rest: the file row is deleted first, so the reverse-reference check behind this MR's parent no longer counts the row being deleted. Reversing the two would strand the attachment on every call.

The diff is 1,202 lines against the parent branch, over the 500-line guardrail. 243 of that is production code; the plan's Research Findings budgets the integration-fixture tax for these steps in as many words.

Test plan

go test -race -tags=integration -run 'MavenFileDeleter|DeleteMavenFile' ./internal/datastore/ -count=1
go test ./internal/datastore/ -count=1
golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 ./internal/datastore/

14 integration tests pass under -race. CI lint cannot see //go:build integration files, so the tagged lint run above is their only gate.

Two negatives are pinned as tests rather than left to prose, because a wrong implementation passes without them: a file delete leaves an emptied version and package live (Maven has no orphan rule and no packument), and an attachment shared by another row survives the delete.

Spec coverage

Criterion	Covered by
A file DELETE removes the row and its attachment in the request	TestMavenFileDeleter_DeleteFile_RemovesRowAndAttachment (pair gone, CAS blob survives)
A marked but unreaped artifact still holds its attachment	..._UnaddressableTargets/a_marked_file_row
A Maven file's subtree is the file row only	..._RemovesRowAndAttachment (sibling, other version's file, version-less row, version, package all survive)
Version-less rows stay unaddressable by id	..._UnaddressableTargets/a_version-less_package-level_row, TestDeleteMavenFileStmt_SQL
An id from another repository or namespace is not reachable	..._UnaddressableTargets (another namespace, another version), ..._DoesNotVerifyTheRepositoryChain
No 409 path: every refusal is one indistinguishable miss	..._UnaddressableTargets, ..._IsIdempotent
Deleting a Maven package's last version leaves the package	..._KeepsEmptiedParentsLive (file-level analogue)
Tenant isolation on the partition key	..._UnaddressableTargets, namespace conjunct in TestDeleteMavenFileStmt_SQL
Interrupted reap resumes; one deletion event per delete; format-mismatch 404	Not here: the S20-A purger, and Steps 22 and 38

Context for LLM agents

Design rationale

DeleteFile returns (deleted bool, err error), converting a no-match into deleted=false with a nil error rather than a sentinel. This mirrors ContainerManifestDeleter.DeleteManifestByTag, which documents the same conversion, and the plan pins the same shape on the sibling npm dist-tag composer. Step 22's handler needs that split to choose between 404 and 202. It is a not-found signal, not a count: the accounting emit (Step 38) owns widening the return later.

The composer is version-keyed and joins no parent chain, so a file under a marked version or package still deletes when the file row itself is live. That is deliberate. The caller resolves the chain, and duplicating the resolve here would leave the purge unable to remove a marked subtree's files through this path.

The attachment removal reads its two columns from DELETE ... RETURNING, which is the only chance to read them, and no separate SELECT ... FOR UPDATE precedes it. The DELETE takes the row lock itself, and the statement is addressed by the full primary key, so it matches at most one row and scans into a single struct: the multi-row grouping hazard that bites a set-based sibling composer cannot arise here.

Rejected alternatives

An EXPLAIN partition-pruning pin was declined in the first review pass on flake grounds, then reversed and added in the second: this repository's planner-nondeterminism history is on access-method pins, not partition-count pins, and the package already holds roughly 39 green pruning pins.

Filtering the parents' soft_deleted_at was rejected. The composer is version-keyed and joins no parent chain, so a file under a marked version or package still deletes when the file row itself is live, and the caller resolves the chain instead. The Maven reap does not route through this composer either way.

Non-goals

No route, handler, or event: Step 22 wires the Maven arm, Step 38 the accounting emit. No counter is moved, because the counters are S22's unmerged schema. No blob storage is reclaimed, which is S28's. SoftDeleteMavenPackage and SoftDeleteMavenVersion are not on this branch and are not referenced.

Follow-ups this MR does not fix

MavenFileStore.UpsertMutableFile's overwrite swaps blob_storage_attachment_id and nothing frees the displaced attachment, so each mutable overwrite strands one and ADR-010 then never collects its blob. Pre-existing on main, surfaced by this branch's review, and worth an issue of its own.

Database Review Evidence

No migrations in this MR. One new statement.

Queries

Note

Plans are from EXPLAIN (ANALYZE, BUFFERS) against an ephemeral PostgreSQL 17.10 container (matching GL_PG_CURR_VERSION in .gitlab-ci-other-versions.yml), with synthesized seed data rolled back per query and the container torn down at the end of the run. Numbers reflect moderate cardinality and do not capture production-scale effects. Expand each row's details for the seed shape, rendered SQL, bound args, and raw plan.

Method	Plan node	Index	Rows (plan / actual)	Cost	Time	Buffers (hit / read)	Partitions
datastore.MavenFileDeleter.DeleteFile	Delete → Index Scan	maven_files_p33_pkey	1 / 1	8.30	0.076ms	5 / 0	1 of 64

datastore.MavenFileDeleter.DeleteFile

Summary: The plan matches the method's intent. The namespace literal prunes to one of 64 hash partitions, the (id, namespace_id) primary key drives the row lookup, and the three narrowing conjuncts apply as a filter on that single row. Estimate matches reality (1 / 1), execution is 0.076ms against 5,200 rows in the target partition, and no buffer reads leave shared memory. No anomalies.

Seed shape: namespaces=1, repositories=1, maven_repositories=1, maven_packages=1, maven_versions=1, blob_storage_blobs=5200, blob_storage_attachments=5200, maven_files=5200 (5000 version-scoped, 200 version-less)

The 200 version-less rows are the package-level maven-metadata.xml shape the statement must refuse to address. They are live siblings in the same partition, so the predicate has to discriminate against them rather than the seed being silent about them.

Rendered SQL:

DELETE FROM public.maven_files
WHERE ((((maven_files.namespace_id = $1::uuid) AND (maven_files.id = $2::uuid)) AND (maven_files.maven_version_id = $3::uuid)) AND (maven_files.maven_version_id IS NOT NULL)) AND (maven_files.soft_deleted_at IS NULL)
RETURNING maven_files.blob_storage_attachment_id AS "maven_files.blob_storage_attachment_id",
          maven_files.blob_sha256 AS "maven_files.blob_sha256";

Bound args: [$1 = 0246025a-f2da-4b01-8530-385a4f7c3048 (namespace), $2 = 1dea8195-188b-49e4-8efb-4a306ccb6124 (file id), $3 = 233f71cf-76d7-4d68-919a-46328ac2168d (version id)]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Delete on maven_files  (cost=0.28..8.30 rows=1 width=10) (actual time=0.013..0.014 rows=1 loops=1)
   Delete on maven_files_p33 maven_files_1
   Buffers: shared hit=5
   ->  Index Scan using maven_files_p33_pkey on maven_files_p33 maven_files_1  (cost=0.28..8.30 rows=1 width=10) (actual time=0.007..0.008 rows=1 loops=1)
         Index Cond: ((id = '1dea8195-188b-49e4-8efb-4a306ccb6124'::uuid) AND (namespace_id = '0246025a-f2da-4b01-8530-385a4f7c3048'::uuid))
         Filter: ((maven_version_id IS NOT NULL) AND (soft_deleted_at IS NULL) AND (maven_version_id = '233f71cf-76d7-4d68-919a-46328ac2168d'::uuid))
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=271
 Planning Time: 0.406 ms
 Execution Time: 0.076 ms

Timings: planning 0.406ms, execution 0.076ms, total 0.482ms.

Query notes:

• Seed size changes the plan node, and the 50-row default is the misleading one. At the 1-plus-49-siblings sizing, the same statement plans as Seq Scan on maven_files_p48 with Rows Removed by Filter: 49, which reads like a missing-index finding and is not one: a 3-page table is cheaper to scan than to descend. The 5,200-row run above is the representative measurement. Partition pruning is unaffected either way, since both runs touch exactly one partition, so the pruning evidence does not depend on the cardinality choice.
• The attachment removal costs planning, not execution, and this MR is its second caller. BlobStorageAttachmentStore.DeleteIfUnreferenced is the parent MR's statement and unchanged here, but the composer calls it once per file delete. Measured on this path, after the file row is deleted in the same transaction: all nine NOT EXISTS clauses take Index Scans, each pruned to a single partition, execution 1.2ms to 1.8ms, and planning 5.4ms warm against 13.4ms on a cold catalog. Nothing amortizes the planning, because the pool runs the simple protocol with no server-side prepared statements, which the statement's own doc comment states. One such plan per file delete is the cost this path pays, and nine foreign-key validation triggers fire after the delete at roughly 0.12ms to 0.25ms each.

Related to #313 (closed)