From 7d88ec5184e7bafafbab9823ea52226f45f7ef84 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Mon, 2 Nov 2026 17:39:29 -0500 Subject: [PATCH] fix(tests): test-created devices and fixture dirs now meet the cleanup contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test suites that attach devices to the SHARED admin (testuser, the live dev account) escaped the harness sweeps whenever their names lacked "test": createContractDeviceNamed named its devices by surface type ("koreader" — the web progress push adopts the device name as sync source, so conflict tests drive a second named source without a separate account), the reinstall regression test uses the web flow's real "Reinstall Device" label, and opds_test.go's folders test named its device "OPDS Folders Device". None had self-cleanup either, so every suite run left 3 more rows on the dev account's device grid — 476 by the time anyone looked. TestDeviceSetup.CreateDevice also never cleanup-registered, so any CURRENT or future helper-created device was one rename away from the same leak. Changes: - TestDeviceSetup.CreateDevice self-deletes its row at test end (t.Cleanup): callers owe nothing to name hygiene any more. - createContractDeviceNamed self-deletes likewise (contract-% prefix). - device_test.go: core via cleanupDeviceByIdentifier on every device it attaches to the admin ("Reinstall Device" by unique identifier — the NAME is user-facing-labeled, sweeps must never torch it). - Harness setup-time admin-device sweep gains the unambiguous harness patterns (dev_contract_* tokens, contract-%/opds-%/repro-reinstall-% identifiers) as the drift net; ran once with the rebuilt tests image and that sweep alone already purged the existing 476 rows. - metadata_write_test.go now globs ALL test-metadata-write-* dirs in its RemoveAll instead of only its own pid's — killed runs left root- owned dirs on the user's uploads bind mount (7 found, deleted). audits: collections/saved-filters/libraries/sync_queue are clean; the only "test"-named media rows left are scans of the user's own on-disk test files (Test/, Bookhoard Test/, embedded " test" in two real books' metadata) — content, not suite debris. Verified: filtered runs (contract/OPDS/reinstall/pairing/metadata) green, zero device residue after the run, uploads mount clean. --- cmd/server/tests/contract_pins_test.go | 8 +++++- cmd/server/tests/device_test.go | 11 ++++++++ cmd/server/tests/metadata_write_test.go | 10 +++++++- cmd/server/tests/test_helpers_test.go | 34 +++++++++++++++++++++++-- docs/contributing/development.md | 9 +++++++ 5 files changed, 68 insertions(+), 4 deletions(-) diff --git a/cmd/server/tests/contract_pins_test.go b/cmd/server/tests/contract_pins_test.go index 58cf355e..e839aa37 100644 --- a/cmd/server/tests/contract_pins_test.go +++ b/cmd/server/tests/contract_pins_test.go @@ -168,7 +168,13 @@ func createContractDeviceNamed(t *testing.T, setup *TestServerSetup, name, devic DeviceMetadata: []byte("{}"), }) require.NoError(t, err, "Should create sync device") - _ = device + // Self-delete at test end: the contract helper attaches to the + // SHARED admin (the web progress push adopts the device name, so + // conflict tests drive a second named source without a separate + // account) and names it for its surface — "koreader" passes every + // name-based sweep, so without this the row survived forever (294 + // of them ended up on the dev account's device grid). + t.Cleanup(func() { _ = setup.DB.DeleteDevice(context.Background(), device.ID) }) return token } diff --git a/cmd/server/tests/device_test.go b/cmd/server/tests/device_test.go index f251429f..31b3beab 100644 --- a/cmd/server/tests/device_test.go +++ b/cmd/server/tests/device_test.go @@ -19,6 +19,9 @@ import ( func TestDeviceRegistrationFlow(t *testing.T) { setup := setupTestServer(t) + // Approved through the shared admin's session; the row must not + // outlive the test (name hygiene alone should never carry it). + cleanupDeviceByIdentifier(t, setup, "kindle-test-hw-id-12345") // Step 1: Initiate device registration regRequest := map[string]interface{}{ @@ -208,6 +211,7 @@ func TestDeviceAuthentication(t *testing.T) { // Create a device directly in the database userID := getTestUserID(t, setup.DB) + cleanupDeviceByIdentifier(t, setup, "test-device-123") deviceToken := fmt.Sprintf("dev_%s", uuid.New().String()) _, err := setup.DB.CreateDevice(context.Background(), database.CreateDeviceParams{ @@ -279,6 +283,10 @@ func TestApproveDeviceReapprovalRotatesToken(t *testing.T) { userID := getTestUserID(t, setup.DB) identifier := fmt.Sprintf("repro-reinstall-%s", uuid.New().String()) + // Self-delete: the row is named "Reinstall Device" — the web + // reinstall flow's own label — so the name-based sweeps can never + // safely touch it. The identifier is this test's fixture alone. + cleanupDeviceByIdentifier(t, setup, identifier) registerAndApprove := func() string { regRequest := map[string]interface{}{ @@ -352,6 +360,9 @@ func TestApproveDeviceReapprovalRotatesToken(t *testing.T) { func TestApproveDeviceRegistration(t *testing.T) { setup := setupTestServer(t) + // Approved through the shared admin's session; the row must not + // outlive the test (name hygiene alone should never carry it). + cleanupDeviceByIdentifier(t, setup, "test-approval-12345") regRequest := map[string]interface{}{ "device_name": "Test Device for Approval", diff --git a/cmd/server/tests/metadata_write_test.go b/cmd/server/tests/metadata_write_test.go index 7e0641e6..9faaa992 100644 --- a/cmd/server/tests/metadata_write_test.go +++ b/cmd/server/tests/metadata_write_test.go @@ -438,7 +438,15 @@ func TestMetadataWriteToFile(t *testing.T) { assert.Equal(t, "Comic Series", series) }) + // Sweep ALL generations of the fixture dirs, not just this + // process's: a killed run never reaches its RemoveAll, and the stale + // root-owned dirs then sit on the user's bind mount forever (seven + // of them were found in uploads/ in Nov 2026, one per past run — + // each surviving run's cleanup skipped them by pid). + matches, _ := filepath.Glob(filepath.Join(uploadsDir, "test-metadata-write-*")) t.Cleanup(func() { - _ = os.RemoveAll(filepath.Join(uploadsDir, fmt.Sprintf("test-metadata-write-%d", os.Getpid()))) + for _, dir := range matches { + _ = os.RemoveAll(dir) + } }) } diff --git a/cmd/server/tests/test_helpers_test.go b/cmd/server/tests/test_helpers_test.go index 68c6fe4e..a1179906 100644 --- a/cmd/server/tests/test_helpers_test.go +++ b/cmd/server/tests/test_helpers_test.go @@ -96,6 +96,21 @@ type TestServerSetup struct { closed bool } +// cleanupDeviceByIdentifier deletes any devices rows carrying the given +// identifier at test end. API-register tests need this instead of a +// token-based delete: the status poll is single-use, so a row created +// through register+approve changes tokens mid-test — the stable +// identity is UNIQUE(device_identifier). Use for every device a test +// attaches to the SHARED admin (testuser); See the DOCTRINE comment in +// setupTestServer — rows that survive their test pile up on the live +// dev account's device grid. +func cleanupDeviceByIdentifier(t *testing.T, setup *TestServerSetup, identifier string) { + t.Cleanup(func() { + _, _ = setup.DBPool.Exec(context.Background(), + "DELETE FROM devices WHERE device_identifier = $1", identifier) + }) +} + // Close cleans up all resources in the correct order func (s *TestServerSetup) Close() error { s.mu.Lock() @@ -481,6 +496,13 @@ func (s *TestDeviceSetup) CreateDevice(t *testing.T, deviceName, deviceType, dev }) require.NoError(t, err, "Should create device") + // Devices created through this helper self-delete at test end, so + // callers owe nothing to name hygiene: setupDeviceTest attaches to + // the SHARED admin, which the harness deliberately never deletes — + // unqualified rows would pile up on the live dev account forever + // (the "OPDS Folders Device" graveyard was exactly this gap). + t.Cleanup(func() { _ = s.DB.DeleteDevice(context.Background(), device.ID) }) + deviceUUID, err := uuid.FromBytes(device.ID.Bytes[0:16]) require.NoError(t, err, "Should parse device ID") @@ -809,9 +831,17 @@ func setupTestServer(t *testing.T) *TestServerSetup { // Same passes for test DEVICES (identifier/name containing "test") // and test SAVED FILTERS (name containing "test"): both accumulated // across runs and poisoned later runs (duplicate identifiers, 409s, - // non-empty "initially empty" assertions). + // non-empty "initially empty" assertions). The second half of the + // device predicate catches test suites that attach to the shared + // admin with surface-specific names ("koreader" from the contract + // pins, "OPDS Folders Device", "Reinstall Device") — every suite + // now also self-deletes its rows, this is the drift safety net: + // dev_contract_% auth tokens, contract-% / opds-folders-% / + // repro-reinstall-% identifiers are unambiguously harness-owned. _, derr := dbPool.Exec(ctx, - "DELETE FROM devices WHERE user_id = $1 AND (device_identifier ILIKE '%test%' OR device_name ILIKE '%test%')", + "DELETE FROM devices WHERE user_id = $1 AND (device_identifier ILIKE '%test%' OR device_name ILIKE '%test%' "+ + "OR device_identifier ILIKE 'contract-%' OR device_identifier ILIKE 'opds-folders-%' OR device_identifier ILIKE 'repro-reinstall-%' "+ + "OR auth_token LIKE 'dev_contract_%')", adminUUID) if derr != nil { t.Logf("device cleanup failed: %v", derr) diff --git a/docs/contributing/development.md b/docs/contributing/development.md index 3bcfa061..e17c1d3b 100644 --- a/docs/contributing/development.md +++ b/docs/contributing/development.md @@ -287,6 +287,15 @@ Test-created data rules (harness cleanup depends on them): users use `@tests.bookhoard.internal` / `@example.com` emails, libraries and collections carry "test"-prefixed names, and cleanups delete test libraries (cascades handle the rest) rather than table-wide deletes. +Devices attach to the per-run regular user when it doesn't matter, or +self-delete via `TestDeviceSetup.CreateDevice` / `createContractDevice` +/ `cleanupDeviceByIdentifier` when they attach to the shared admin — +names lacking "test" (device-type names like "koreader", the +"Reinstall Device" label) escaped the name sweeps for weeks and piled +476 rows onto the live dev account; every suite now cleans its own +rows at test end, with the harness sweep's identifier/token patterns +(`contract-%`, `opds-folders-%`, `repro-reinstall-%`, +`dev_contract_*`) as the drift net. ### Media Fixtures (audio/books)