From 20743b66f79ddf7ecc326296d54b2d019faaf4ba Mon Sep 17 00:00:00 2001 From: hsinhoyeh Date: Sat, 15 Aug 2026 10:15:34 +0800 Subject: [PATCH 1/2] test(migrations): prove 0002 purges tombstones and keeps live rows 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) --- pkg/storage/documents/purge_migration_test.go | 165 ++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 pkg/storage/documents/purge_migration_test.go diff --git a/pkg/storage/documents/purge_migration_test.go b/pkg/storage/documents/purge_migration_test.go new file mode 100644 index 0000000..e0133e6 --- /dev/null +++ b/pkg/storage/documents/purge_migration_test.go @@ -0,0 +1,165 @@ +package storagedocuments + +import ( + "context" + "testing" + + "github.com/sdinsure/agent/pkg/logger" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/footprintai/restcol/migrations" + appmodeldocuments "github.com/footprintai/restcol/pkg/models/documents" + storagecollections "github.com/footprintai/restcol/pkg/storage/collections" + storageprojects "github.com/footprintai/restcol/pkg/storage/projects" + storagetestutils "github.com/footprintai/restcol/pkg/storage/testutil" +) + +// TestMigration0002PurgesTombstonesAndKeepsLiveRows proves what +// 0002_purge_soft_deleted_documents claims, which until now nothing did. +// +// # WHY THIS WAS MISSING, AND WHY IT MATTERED +// +// migrations/embed_test.go says so in its own header: those tests are "about +// COVERAGE, not about the SQL" - they assert every file is reachable through +// the embed, not that any of them does anything. 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 when FootprintAI/grandturks#977's criterion 4 - "after +// #970's migration, count(*) WHERE deleted_at IS NOT NULL returns 0" - was run +// against ubm01. It returned 0. It had also returned 0 BEFORE the migration ran, +// because that database had never held a tombstone: `DELETE 0`. The criterion +// passed while demonstrating nothing, which is the exact failure sprint #827 +// closed on, one level down. +// +// This test is the missing half. It cannot be satisfied vacuously: it CREATES a +// tombstone, asserts it is there, then asserts the migration removed it. +// +// # THE TOMBSTONE IS MADE THE WAY THE OLD CODE MADE 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 it is what wrote every tombstone in every +// deployed database. Reproducing it here rather than INSERTing a hand-made row +// keeps the fixture honest: gorm sets deleted_at exactly as it did in +// production, and if that behaviour ever changes this test changes with it. +// +// # THE SECOND ASSERTION IS THE ONE THAT PROTECTS DATA +// +// "Tombstones are gone" is half the contract. "Live rows are untouched" is the +// half that makes the migration safe to run, and a `DELETE FROM ... WHERE +// deleted_at IS NOT NULL` with a broken predicate would still satisfy the +// first. Both are asserted, and the keeper is read back through the normal path +// rather than counted, so a row that survived as a tombstone would not pass. +func TestMigration0002PurgesTombstonesAndKeepsLiveRows(t *testing.T) { + if testing.Short() { + t.Skip("requires a local postgres; skipping under -short") + } + + ctx := context.Background() + postgrescli, err := storagetestutils.NewTestPostgresCli(logger.NewLogger(false)) + require.NoError(t, err) + + regularProject, _, err := storageprojects.TestProjectSuite(postgrescli) + require.NoError(t, err) + + docCURD := NewDocumentCURD(postgrescli) + require.NoError(t, docCURD.AutoMigrate()) + + collectionsCURD := storagecollections.NewCollectionCURD(postgrescli) + require.NoError(t, collectionsCURD.AutoMigrate()) + + collection, err := storagecollections.TestCollectionSuite(postgrescli, regularProject) + require.NoError(t, err) + require.NoError(t, collectionsCURD.Write(ctx, "", collection)) + + newDoc := func(payload map[string]interface{}) *appmodeldocuments.ModelDocument { + return &appmodeldocuments.ModelDocument{ + ID: appmodeldocuments.NewDocumentID(), + Data: appmodeldocuments.NewModelDocumentData(payload), + ModelCollectionID: collection.ID, + ModelProjectID: regularProject.ID, + } + } + + keeper := newDoc(map[string]interface{}{"keep": "this row must survive"}) + require.NoError(t, docCURD.Write(ctx, "", keeper)) + + // The payload names what is actually at stake: for taskWorker-sinked + // documents the `data` jsonb holds base64 image bytes, which is why #936 + // raised retention over these rows rather than tidiness. + doomed := newDoc(map[string]interface{}{"imageBytes": "iVBORw0KGgoAAAANSUhEUg=="}) + require.NoError(t, docCURD.Write(ctx, "", doomed)) + + // A SCOPED delete - what the code did before #137 - leaves the row with + // deleted_at set and its data intact. + require.NoError(t, postgrescli.GormDB(). + Where("id = ?", doomed.ID.String()). + Delete(&appmodeldocuments.ModelDocument{}).Error) + + countTombstones := func() int64 { + var n int64 + require.NoError(t, postgrescli.GormDB().Unscoped(). + Model(&appmodeldocuments.ModelDocument{}). + Where("deleted_at IS NOT NULL"). + Count(&n).Error) + return n + } + + // Guard the fixture itself. If this is 0 the migration below would "pass" + // against nothing, which is precisely the vacuous result that prompted the + // test - so it is a hard failure, not an assumption. + require.NotZero(t, countTombstones(), + "fixture did not produce a tombstone; the rest of this test would be vacuous") + + // The row is unreachable through the API and still on disk - the state + // #936 objected to. + var doomedData string + require.NoError(t, postgrescli.GormDB().Unscoped(). + Model(&appmodeldocuments.ModelDocument{}). + Where("id = ?", doomed.ID.String()). + Select("data->>'imageBytes'").Scan(&doomedData).Error) + assert.Equal(t, "iVBORw0KGgoAAAANSUhEUg==", doomedData, + "the tombstoned row should still hold its payload before the purge") + + // Apply the migration from the EMBEDDED copy, not from disk - that is what + // a consumer pinning this module actually runs (#1004). + up, err := migrations.EmbeddedMigrations.ReadFile("0002_purge_soft_deleted_documents.up.sql") + require.NoError(t, err, "the migration must be reachable through the embed") + require.NoError(t, postgrescli.GormDB().Exec(string(up)).Error) + + assert.Zero(t, countTombstones(), + "0002 left a soft-deleted row behind - the migration does not do what it says") + + // And the live row is still readable through the normal path. Counting + // would not be enough: a keeper that survived AS A TOMBSTONE would still be + // counted, and would still be broken. + found, err := docCURD.Get(ctx, "", regularProject.ID, collection.ID, keeper.ID) + require.NoError(t, err) + assert.Equal(t, keeper.ID, found.ID, + "0002 removed a LIVE document - its predicate is wrong and it is unsafe to run") +} + +// TestMigration0002IsIdempotent - #970's runbook tells operators "re-running is +// harmless", and that claim is load-bearing: the deployed sites are applied by +// hand, so a second run is likely rather than hypothetical. +func TestMigration0002IsIdempotent(t *testing.T) { + if testing.Short() { + t.Skip("requires a local postgres; skipping under -short") + } + + postgrescli, err := storagetestutils.NewTestPostgresCli(logger.NewLogger(false)) + require.NoError(t, err) + + docCURD := NewDocumentCURD(postgrescli) + require.NoError(t, docCURD.AutoMigrate()) + + up, err := migrations.EmbeddedMigrations.ReadFile("0002_purge_soft_deleted_documents.up.sql") + require.NoError(t, err) + + for i := 0; i < 3; i++ { + require.NoErrorf(t, postgrescli.GormDB().Exec(string(up)).Error, + "run %d of 0002 failed; the runbook promises re-running is harmless", i+1) + } +} From 119915e84e2a65e71d49aa4979aa8b2ae0440aa3 Mon Sep 17 00:00:00 2001 From: hsinhoyeh Date: Sat, 15 Aug 2026 10:22:09 +0800 Subject: [PATCH 2/2] fix(test): read the jsonb payload at data->'json_value'->>'imageBytes' 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) --- pkg/storage/documents/purge_migration_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/storage/documents/purge_migration_test.go b/pkg/storage/documents/purge_migration_test.go index e0133e6..18f8b88 100644 --- a/pkg/storage/documents/purge_migration_test.go +++ b/pkg/storage/documents/purge_migration_test.go @@ -115,11 +115,18 @@ func TestMigration0002PurgesTombstonesAndKeepsLiveRows(t *testing.T) { // The row is unreachable through the API and still on disk - the state // #936 objected to. + // + // The jsonb path is data->'json_value'->>'imageBytes', not + // data->>'imageBytes': ModelDocumentData marshals the caller's map under a + // `json_value` key (documents.go:86). Reading the wrong path returns NULL, + // which is how this assertion first failed - and it would have been easy to + // "fix" by weakening it to a NOT NULL check on the whole column, which + // would no longer show that the PAYLOAD survives. var doomedData string require.NoError(t, postgrescli.GormDB().Unscoped(). Model(&appmodeldocuments.ModelDocument{}). Where("id = ?", doomed.ID.String()). - Select("data->>'imageBytes'").Scan(&doomedData).Error) + Select("data->'json_value'->>'imageBytes'").Scan(&doomedData).Error) assert.Equal(t, "iVBORw0KGgoAAAANSUhEUg==", doomedData, "the tombstoned row should still hold its payload before the purge")