Skip to content

test(migrations): prove 0002 purges tombstones and keeps live rows - #140

Merged
hsinatfootprintai merged 2 commits into
mainfrom
test/0002-purges-tombstones
Aug 15, 2026
Merged

test(migrations): prove 0002 purges tombstones and keeps live rows#140
hsinatfootprintai merged 2 commits into
mainfrom
test/0002-purges-tombstones

Conversation

@hsinatfootprintai

Copy link
Copy Markdown
Contributor

Nothing asserted that 0002 does anything

migrations/embed_test.go says so in its own header:

The tests below are therefore about COVERAGE, not about the SQL: every file in this directory has to be reachable through the embed

So 0002_purge_soft_deleted_documents shipped, was pinned by grandturks (#1004/#1014), and was applied to a deployed cluster — without a single assertion that it removes a tombstone.

How the gap surfaced

Running FootprintAI/grandturks#977's criterion 4 against ubm01 — "after #970's migration, count(*) WHERE deleted_at IS NOT NULL returns 0".

It returned 0. It had also returned 0 before the migration ran, because that database had never held a tombstone; the apply reported DELETE 0. The criterion passed while demonstrating nothing — sprint #827's failure one level down.

What this adds

A test that cannot be satisfied vacuously: it creates a tombstone, asserts it exists, then asserts the migration removed it. A require.NotZero on the fixture makes the vacuous case a hard failure rather than a silent pass — that specific failure mode is what prompted this.

The fixture is built the way the old code built it. DocumentCURD.Delete is Unscoped now (#136/#137), so it hard-deletes and cannot produce the rows this migration exists to purge. A scoped gorm delete is what the pre-#137 code did, and what wrote every tombstone in every deployed database. Reproducing that rather than INSERTing a hand-made row keeps the fixture honest: gorm sets deleted_at exactly as production did, and if that behaviour ever changes the test changes with it.

Two assertions, not one. "Tombstones are gone" is half the contract; "live rows are untouched" is the half that makes the migration safe to run — a DELETE with a broken predicate would still satisfy the first. The keeper is read back through the normal path rather than counted, so a row that survived as a tombstone would not pass.

It also asserts the tombstoned row still holds its data payload before the purge — the state #936 objected to, and the reason this is a retention question rather than a tidiness one.

A second test covers idempotence, because #970's runbook tells operators "re-running is harmless" and the deployed sites are applied by hand, so a second run is likely rather than hypothetical.

Applied from the embedded copy, not from disk — that is what a consumer pinning this module actually runs.

Verification, stated honestly

go vet passes and the tests skip cleanly under -short. They have not been executed against a real Postgres — I have no Docker locally, so CI's postgres:16-alpine service is the first thing that will actually run them.

If CI is green, that is the first time anyone has seen 0002 remove a row. Worth watching that run rather than assuming it.

Gated on testing.Short() like every other postgres-backed test here.

🤖 Generated with Claude Code

hsinhoyeh and others added 2 commits August 15, 2026 10:15
migrations/embed_test.go says it plainly in its own header - those tests are
"about COVERAGE, not about the SQL". So 0002 shipped, was pinned by grandturks
(#1004/#1014) and was applied to a deployed cluster without a single assertion
that it removes a tombstone.

The gap surfaced running FootprintAI/grandturks#977's criterion 4 - "after
#970's migration, count(*) WHERE deleted_at IS NOT NULL returns 0" - against
ubm01. It returned 0. It had also returned 0 BEFORE the migration ran, because
that database had never held a tombstone: the apply reported DELETE 0. The
criterion passed while demonstrating nothing, which is sprint #827's failure one
level down.

This is the missing half, and it cannot be satisfied vacuously: it CREATES a
tombstone, asserts it exists, then asserts the migration removed it. A
require.NotZero on the fixture makes the vacuous case a hard failure rather than
a silent pass.

THE FIXTURE IS BUILT THE WAY THE OLD CODE BUILT IT. DocumentCURD.Delete is
Unscoped now (#136/#137) and hard-deletes, so it cannot produce the rows this
migration exists to purge. A scoped gorm delete is what the pre-#137 code did
and what wrote every tombstone in every deployed database. Reproducing that
rather than INSERTing a hand-made row keeps the fixture honest - gorm sets
deleted_at exactly as production did, and if that ever changes the test changes
with it.

TWO ASSERTIONS, NOT ONE. "Tombstones are gone" is half the contract; "live rows
are untouched" is the half that makes the migration safe to run, and a DELETE
with a broken predicate would still satisfy the first. The keeper is read back
through the normal path rather than counted, so a row that survived AS A
TOMBSTONE would not pass.

It also asserts the tombstoned row still holds its `data` payload before the
purge - the state #936 objected to, and the reason this is a retention question
rather than a tidiness one.

A second test covers idempotence, because #970's runbook tells operators
"re-running is harmless" and the deployed sites are applied by hand, so a second
run is likely rather than hypothetical.

Applied from the EMBEDDED copy, not from disk: that is what a consumer pinning
this module actually runs.

Gated on testing.Short() like every other postgres-backed test here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first CI run - the first time these tests executed against a real postgres -
failed here with 'converting NULL to string is unsupported'.

ModelDocumentData marshals the caller's map under a json_value key
(documents.go:86), so data->>'imageBytes' reads a path that does not exist. The
fixture guard above it passed, so the tombstone was created correctly; only this
assertion was wrong.

Worth noting what NOT to do: weakening it to 'data IS NOT NULL' would have gone
green while no longer showing that the PAYLOAD survives, which is the whole
reason #936 treated these rows as a retention problem.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hsinatfootprintai
hsinatfootprintai merged commit 8a94836 into main Aug 15, 2026
2 checks passed
@hsinatfootprintai
hsinatfootprintai deleted the test/0002-purges-tombstones branch August 15, 2026 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants