From 01a9dae49e4457ab99323c766b6a83dc94e46057 Mon Sep 17 00:00:00 2001 From: BaaRai <130491490+BaaRai@users.noreply.github.com> Date: Fri, 13 Nov 2026 23:46:42 +0100 Subject: [PATCH] test(db): make profile and company-scope tests repeatable on one database TestPoolProfilesOrder, TestProfileRetryRoundTrip and TestProfileRetryClampedOnSave closed the pool with defer d.Close(), which runs before t.Cleanup, so the row-deleting cleanup hit a closed pool and its error was discarded. The fixed profile names then collided with the llm_profiles name unique key on the next run. Close via t.Cleanup registered first, give the profiles per-run names, and report cleanup errors. TestLLMRetryPolicyRoundTrip had the same ordering and now restores the setting before the pool closes. TestCompanyScopeAttribution derived its names from MAX(assets.id) and never removed its company, so the next pass found the old company holding the same fixed 198.51.100.0/24 scope and the in-CIDR IP was attributed to it. Use a time plus random suffix and a per-run /24 from 198.18.0.0/15, delete the company (its scope cascades) after the assets, and report cleanup errors. --- db/company_scope_test.go | 34 ++++++++++++++++++++++++++-------- db/config_test.go | 28 ++++++++++++++++++++++------ db/llmretry_test.go | 26 +++++++++++++++++--------- 3 files changed, 65 insertions(+), 23 deletions(-) diff --git a/db/company_scope_test.go b/db/company_scope_test.go index 11c8ca4..32ddd02 100644 --- a/db/company_scope_test.go +++ b/db/company_scope_test.go @@ -2,6 +2,7 @@ package db import ( "fmt" + "math/rand/v2" "testing" "time" ) @@ -331,19 +332,36 @@ func TestCompanyScopeAttribution(t *testing.T) { d.Close() t.Fatalf("startMax: %v", err) } + // Remove everything the test creates, or the next pass on the same database + // finds this company's scope and attributes its assets to it. Assets first + // (they reference the company), then the company; its scope rows cascade. + var cid int64 t.Cleanup(func() { - _, _ = d.Exec(`DELETE FROM assets WHERE id > $1`, startMax) - d.Close() + defer d.Close() + if _, err := d.Exec(`DELETE FROM assets WHERE id > $1`, startMax); err != nil { + t.Errorf("cleanup assets: %v", err) + } + if cid != 0 { + if _, err := d.Exec(`DELETE FROM companies WHERE id = $1`, cid); err != nil { + t.Errorf("cleanup company %d: %v", cid, err) + } + } }) - uniq := startMax + 1 - root := fmt.Sprintf("scopetest%d.com", uniq) + // Unique per run: MAX(assets.id) comes back to the same value after cleanup, + // so it cannot tell one pass from the next. The in-scope /24 is picked per + // run from 198.18.0.0/15 so a scope left by an interrupted run cannot claim + // this run's IP. + uniq := fmt.Sprintf("%d%d", time.Now().UnixNano(), rand.Uint32()) + n := rand.IntN(512) + cidr := fmt.Sprintf("198.%d.%d.0/24", 18+n/256, n%256) + root := fmt.Sprintf("scopetest%s.com", uniq) sub := "api." + root - ipIn := "198.51.100.9" + ipIn := fmt.Sprintf("198.%d.%d.9", 18+n/256, n%256) ipOut := "203.0.113.9" - outDomain := fmt.Sprintf("other%d.net", uniq) + outDomain := fmt.Sprintf("other%s.net", uniq) - cid, _, err := cs.UpsertCompany(fmt.Sprintf("ScopeCo %d", uniq), "") + cid, _, err = cs.UpsertCompany(fmt.Sprintf("ScopeCo %s", uniq), "") if err != nil { t.Fatalf("UpsertCompany: %v", err) } @@ -359,7 +377,7 @@ func TestCompanyScopeAttribution(t *testing.T) { t.Fatalf("pre-scope asset should be unattributed, got %v", *preCompanyID) } - cs.AddScope(cid, []string{root, "198.51.100.0/24"}, "unit test") + cs.AddScope(cid, []string{root, cidr}, "unit test") mustCid := func(id int64, want int64, label string) { var cID *int64 diff --git a/db/config_test.go b/db/config_test.go index f0e93a3..a9376fa 100644 --- a/db/config_test.go +++ b/db/config_test.go @@ -4,7 +4,9 @@ import ( "context" "encoding/json" "errors" + "fmt" "testing" + "time" ) // TestPoolProfilesOrder pins the failover chain query: keyless profiles can't @@ -17,17 +19,22 @@ func TestPoolProfilesOrder(t *testing.T) { if err != nil { t.Skipf("postgres unavailable (%v) — skipping", err) } - defer d.Close() + // Close via Cleanup, registered FIRST so it runs LAST: a plain defer would + // close the pool before the row-deleting cleanups below run. + t.Cleanup(func() { d.Close() }) + // llm_profiles.name is unique, so a fixed name collides with a row left on + // the same database by an earlier run. + sfx := fmt.Sprintf("-%d", time.Now().UnixNano()) mk := func(name string, priority int, exclude bool, key string) int64 { id, err := d.SaveProfile(&LLMProfile{ - Name: name, Format: "openai", Model: "m", APIKey: key, + Name: name + sfx, Format: "openai", Model: "m", APIKey: key, Priority: priority, PoolExclude: exclude, }) if err != nil { t.Fatal(err) } - t.Cleanup(func() { d.Exec(`DELETE FROM llm_profiles WHERE id=$1`, id) }) + t.Cleanup(func() { deleteTestProfile(t, d, id) }) return id } lo := mk("t-pool-lo", 1, false, "k1") @@ -42,11 +49,11 @@ func TestPoolProfilesOrder(t *testing.T) { var got []int64 for _, p := range chain { switch p.Name { - case "t-pool-lo", "t-pool-hi": + case "t-pool-lo" + sfx, "t-pool-hi" + sfx: got = append(got, p.ID) - case "t-pool-excluded": + case "t-pool-excluded" + sfx: t.Fatal("pool_exclude profile entered the failover chain") - case "t-pool-nokey": + case "t-pool-nokey" + sfx: t.Fatal("keyless profile entered the failover chain") } } @@ -55,6 +62,15 @@ func TestPoolProfilesOrder(t *testing.T) { } } +// deleteTestProfile removes a profile a test created and reports a failed delete +// instead of silently leaving the row for the next run. +func deleteTestProfile(t *testing.T, d *DB, id int64) { + t.Helper() + if _, err := d.Exec(`DELETE FROM llm_profiles WHERE id=$1`, id); err != nil { + t.Errorf("cleanup profile %d: %v", id, err) + } +} + func TestDeleteProfileContextHonorsCancellation(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() diff --git a/db/llmretry_test.go b/db/llmretry_test.go index 5633b3f..ba4dfe4 100644 --- a/db/llmretry_test.go +++ b/db/llmretry_test.go @@ -1,6 +1,7 @@ package db import ( + "fmt" "testing" "time" ) @@ -14,7 +15,8 @@ func TestProfileRetryRoundTrip(t *testing.T) { if err != nil { t.Skipf("postgres unavailable (%v) — skipping", err) } - defer d.Close() + // Registered FIRST so it runs LAST, after the row cleanup below. + t.Cleanup(func() { d.Close() }) want := RetryOverride{ Connect: RetryRule{Attempts: 5, IntervalMS: 2000}, @@ -22,12 +24,12 @@ func TestProfileRetryRoundTrip(t *testing.T) { Stream: RetryRule{IntervalMS: 250}, } id, err := d.SaveProfile(&LLMProfile{ - Name: "t-retry-roundtrip", Format: "openai", Model: "m", APIKey: "k", Retry: want, + Name: fmt.Sprintf("t-retry-roundtrip-%d", time.Now().UnixNano()), Format: "openai", Model: "m", APIKey: "k", Retry: want, }) if err != nil { t.Fatal(err) } - t.Cleanup(func() { d.Exec(`DELETE FROM llm_profiles WHERE id=$1`, id) }) + t.Cleanup(func() { deleteTestProfile(t, d, id) }) got, err := d.ProfileByID(id) if err != nil || got == nil { @@ -71,16 +73,17 @@ func TestProfileRetryClampedOnSave(t *testing.T) { if err != nil { t.Skipf("postgres unavailable (%v) — skipping", err) } - defer d.Close() + // Registered FIRST so it runs LAST, after the row cleanup below. + t.Cleanup(func() { d.Close() }) id, err := d.SaveProfile(&LLMProfile{ - Name: "t-retry-clamp", Format: "openai", Model: "m", APIKey: "k", + Name: fmt.Sprintf("t-retry-clamp-%d", time.Now().UnixNano()), Format: "openai", Model: "m", APIKey: "k", Retry: RetryOverride{Connect: RetryRule{Attempts: -99, IntervalMS: -5}}, }) if err != nil { t.Fatal(err) } - t.Cleanup(func() { d.Exec(`DELETE FROM llm_profiles WHERE id=$1`, id) }) + t.Cleanup(func() { deleteTestProfile(t, d, id) }) got, err := d.ProfileByID(id) if err != nil || got == nil { t.Fatalf("ProfileByID: %v", err) @@ -97,17 +100,22 @@ func TestLLMRetryPolicyRoundTrip(t *testing.T) { if err != nil { t.Skipf("postgres unavailable (%v) — skipping", err) } - defer d.Close() + // Registered FIRST so it runs LAST, after the restore below. + t.Cleanup(func() { d.Close() }) before, hadBefore, err := d.GetSetting(settingLLMRetryPolicy) if err != nil { t.Fatal(err) } t.Cleanup(func() { + var err error if hadBefore { - d.SetSetting(settingLLMRetryPolicy, before) + err = d.SetSetting(settingLLMRetryPolicy, before) } else { - d.Exec(`DELETE FROM settings WHERE key=$1`, settingLLMRetryPolicy) + _, err = d.Exec(`DELETE FROM settings WHERE key=$1`, settingLLMRetryPolicy) + } + if err != nil { + t.Errorf("restore %s: %v", settingLLMRetryPolicy, err) } })