The following discussions from !1980 should be addressed: - [ ] @GitLabDuo started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1980#note_3739900041): (+1 comment) > This test creates a `container_manifest_relationships` row directly via `NewContainerManifestRelationshipStore().Create(...)` but registers no `t.Cleanup` for it. The delete under test here is the *refusal* path (`DeleteManifestByDigest` refuses because a live parent exists), so this relationship row and the two manifests it links survive to the end of the test. > > If `seedManifestRelFixtures`'s own cleanup deletes the `container_manifests` rows directly (the pattern used elsewhere in this file, e.g. `cleanupDeleteCountersContainerRows`), that delete will hit a foreign-key violation, since `fk_cmr_child_container_manifest_id_container_manifests` (and its parent-side counterpart) declare no `ON DELETE` action — this is exactly the ordering hazard `linkReapIndex` in `lifecycle_reap_container_integration_test.go` explicitly guards against with its own `t.Cleanup` that removes the relationship row before its manifests. > > Worth registering a `t.Cleanup` here that deletes this relationship row ahead of the fixture's own teardown, mirroring `linkReapIndex`'s handling. > ```suggestion:-0+6 > relationship, _, err := NewContainerManifestRelationshipStore().Create(ctx, db, CreateContainerManifestRelationshipInput{ > NamespaceID: seed.NS, > ContainerImageID: seed.ImageID, > ParentContainerManifestID: seed.ParentID, > ChildContainerManifestID: seed.ChildID, > }) > require.NoError(t, err, "linking the parent index to the child") > > //nolint:contextcheck // cleanup runs after the test context is canceled > t.Cleanup(func() { > _, _ = db.ExecContext(context.Background(), > `DELETE FROM container_manifest_relationships WHERE namespace_id = $1 AND id = $2`, > seed.NS, relationship.ID) > }) > ``` - [ ] @vespian_gl started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1980#note_3740831532): > non-blocking: this doc names a transposition hazard that the replaced signature did not have. > > The two counts on the old return list are `tagsDeleted int` and `freedSizeBytes int64`. The types differ, so the compiler rejects a transposed `:=` destructure at every production consumption site. `DeleteManifestResult.TagsDeleted` is `int` (`internal/format/oci/manifest_delete.go:81`) and `deleteCounters.freedSizeBytes` is `int64` (`internal/managementapi/artifact_write.go:221`). > > The cited precedent rests on the opposite fact. `NpmVersionUnpublishOutcome`'s doc (`internal/datastore/npm_version_unpublish_deleter.go:91-94`) says `PackageDeleted` and `VersionDeleted` "are same-type booleans whose transposition across a bare return list would compile silently". > > There is also no site at which the transposition can be written. All four destructure sites on `main` bind one of the two integers to `_`: `internal/format/oci/manifest_delete.go:212`, `internal/managementapi/bulk_container_worker.go:988` and `:1040`, and `internal/managementapi/container_manifest_delete.go:104`. > > The stronger reason is already in commit `6ba53c40f`, in the description, and in issue #906: the silent positional **drop**, which no type check catches. You should state that hazard here instead, and keep the `NpmVersionUnpublishOutcome` citation for the named-fields remedy only. The doc is 3 lines against the 3-line exported cap, so a same-length reword fits. The text is branch-new, so no `main`-owned rationale is at risk here, unlike the `cascadeLayer` deviation your description records. > > _This is a bot message 🤖 — /mergeable_ - [ ] @vespian_gl started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1980#note_3740833728): > non-blocking: this comment names a row that the freed-size probe never reads. > > The premise about the shadow's triggers is right, but the conclusion "has to land on both rows" does not follow. `repoFreedBlobSizeTx` reads the size only from the shadow (`internal/datastore/repo_blob_references.go:88-89`), and `containerRepoStillReferencesBlobStmt` projects no size column. Nothing on this path reads `blob_storage_blobs.size`. > > A check confirms it. With the `UPDATE blob_storage_blobs` statement removed, `TestContainerManifestDeleter_DeleteManifestByDigest_SuccessReportsWholeOutcome` still passes against a real Postgres. > > The sentence is an instruction for a future edit, so a reader who follows it will believe the base table takes part in the freed-size report. You should say that the shadow row is the only row the probe reads. Keep the base-table `UPDATE` if you want fixture consistency, and describe it as that. The block is 2 lines against the 2-line `_test.go` cap, so a same-length reword fits. > > _This is a bot message 🤖 — /mergeable_ - [ ] @vespian_gl started a [discussion](https://gitlab.com/gitlab-org/ops/artifact-registry/-/merge_requests/1980#note_3740835149): > non-blocking: this file also changes in open merge request !1982, and the two conflict. > > `git merge-tree --write-tree` against !1982's branch reports one conflict, in `internal/format/oci/manifest_delete.go`, and no other. !1982 deletes `NewManifestHandlerWithDelete` and rewrites the file-header block. This branch rewrites the `manifestDeleteTxRunner` doc directly below it and reshapes the seam. > > The two changes are compatible. The resolution keeps !1982's removal of `NewManifestHandlerWithDelete` together with this branch's `manifestDeleteTxRunner` doc and its `datastore.DeleteManifestOutcome` signature. Nothing records that today, so whoever merges second has to derive it. > > `AGENTS.md` asks for this check before an MR opens, and says "an overlap conflicts whichever lands second, and no pipeline reports it". !1982's own description names its conflict with !1895 and says who resolves it. > > This asks for one line in the description, not a code change. I checked this branch's other 11 files against the open merge requests too. !1912 and !1916 also touch `internal/datastore/lifecycle_reap_container.go`, and both merge cleanly with this head. > > _This is a bot message 🤖 — /mergeable_