From 096e16409cbebe6bcdaf730d10faf4ec2173fd73 Mon Sep 17 00:00:00 2001 From: "Michael J. Arcan" Date: Fri, 3 Jul 2026 11:21:08 +0200 Subject: [PATCH] fix(#1741): wrap test-DB insert loops in a single transaction (#1819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Fixes #1741 `TestBoundedLoad_OldestLoadedSet` (and any test building a 5000-row fixture) hung/timed out, blocking reliable `go test ./cmd/server` and CI. ## Root cause The four test-DB builders in `cmd/server/bounded_load_test.go` (`createTestDBAt`, `createTestDBWithObs`, `createTestDBWithAgedPackets`) inserted rows in a loop with no `BEGIN`/`COMMIT`. With the pure-Go `modernc.org/sqlite` driver every `Exec` auto-commits → one fsync per row → ~2N fsyncs for N transmissions (tx + obs). At `numTx=5000` that's ~10k fsyncs and the fixture blows past the test timeout. Sibling tests with `numTx<=3000` happened to stay under the timeout, so only the 5000-row cases visibly hung. ## Fix Wrap each insert loop in a single `BEGIN`/`COMMIT` so the whole fixture build becomes one commit. Fixtures now finish in well under a second regardless of `numTx`; the tests' actual assertions (`oldestLoaded` set, newest-first ordering, bounded load) are exercised instead of the timeout masking them. Also made the prepared-statement `Exec` calls check their error (previously discarded) so a failed insert surfaces instead of silently leaving the DB short. No production code changed — test infrastructure only. ## Verified - `TestBoundedLoad_OldestLoadedSet`: **0.18s** (was: 30s timeout / FAIL). - Full `TestBoundedLoad*` + retention group: passes in ~1.2s. - `go test ./...` in `cmd/server`: exit 0 (no longer blocks on this test). Co-authored-by: Waydroid Builder Co-authored-by: Claude --- cmd/server/bounded_load_test.go | 47 ++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/cmd/server/bounded_load_test.go b/cmd/server/bounded_load_test.go index db425dde..6da1d458 100644 --- a/cmd/server/bounded_load_test.go +++ b/cmd/server/bounded_load_test.go @@ -170,6 +170,13 @@ func createTestDBWithAgedPackets(t *testing.T, numRecent, numOld int) string { now := time.Now().UTC() id := 1 + // Single transaction for all inserts — see createTestDBAt for the rationale + // (modernc.org/sqlite auto-commit per Exec fsyncs per row). numOld+numRecent + // is small here today, but wrapping keeps the fixture robust if callers scale + // it up, and is consistent with the other builders. + if _, err := conn.Exec("BEGIN"); err != nil { + t.Fatalf("test DB BEGIN: %v", err) + } // Insert old packets (48 hours ago) for i := 0; i < numOld; i++ { oldT := now.Add(-48 * time.Hour).Add(time.Duration(i) * time.Second) @@ -188,6 +195,9 @@ func createTestDBWithAgedPackets(t *testing.T, numRecent, numOld int) string { conn.Exec("INSERT INTO observations VALUES (?,?,?,?,?,?,?,?,?,?,?)", id, id, "obs1", "Obs1", "RX", -10.0, -80.0, 5, `[]`, newT.Unix(), "") id++ } + if _, err := conn.Exec("COMMIT"); err != nil { + t.Fatalf("test DB COMMIT: %v", err) + } return dbPath } @@ -342,11 +352,27 @@ func createTestDBAt(tb testing.TB, dbPath string, numTx int) { defer obsStmt.Close() base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + // Wrap the inserts in a single transaction. Without this, modernc.org/sqlite + // (pure-Go driver) auto-commits every Exec → one fsync per row → ~2N fsyncs + // for N transmissions (tx + obs). At numTx=5000 that is ~10k fsyncs and the + // fixture blows past the test timeout (the #1741 hang). A single + // BEGIN/COMMIT makes the whole build one commit, finishing in well under a + // second regardless of numTx. + if _, err := conn.Exec("BEGIN"); err != nil { + tb.Fatalf("test DB BEGIN: %v", err) + } for i := 1; i <= numTx; i++ { ts := base.Add(time.Duration(i) * time.Minute).Format(time.RFC3339) hash := fmt.Sprintf("h%04d", i) - txStmt.Exec(i, "aabb", hash, ts, 0, 4, 1, fmt.Sprintf(`{"pubKey":"pk%04d"}`, i)) - obsStmt.Exec(i, i, "obs1", "Obs1", "RX", -10.0, -80.0, 5, `["aa","bb"]`, ts) + if _, err := txStmt.Exec(i, "aabb", hash, ts, 0, 4, 1, fmt.Sprintf(`{"pubKey":"pk%04d"}`, i)); err != nil { + tb.Fatalf("test DB insert transmission %d: %v", i, err) + } + if _, err := obsStmt.Exec(i, i, "obs1", "Obs1", "RX", -10.0, -80.0, 5, `["aa","bb"]`, ts); err != nil { + tb.Fatalf("test DB insert observation %d: %v", i, err) + } + } + if _, err := conn.Exec("COMMIT"); err != nil { + tb.Fatalf("test DB COMMIT: %v", err) } } @@ -396,16 +422,29 @@ func createTestDBWithObs(tb testing.TB, dbPath string, numTx int) { obsNames := []string{"Alpha", "Bravo", "Charlie", "Delta", "Echo"} obsID := 1 base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) + // Single transaction for all inserts — see createTestDBAt for the rationale + // (modernc.org/sqlite auto-commit per Exec would fsync per row; at numTx=30000 + // the benchmarks would otherwise stall for minutes). One BEGIN/COMMIT. + if _, err := conn.Exec("BEGIN"); err != nil { + tb.Fatalf("test DB BEGIN: %v", err) + } for i := 1; i <= numTx; i++ { ts := base.Add(time.Duration(i) * time.Minute).Format(time.RFC3339) hash := fmt.Sprintf("h%06d", i) - txStmt.Exec(i, "aabb", hash, ts, 0, 4, 1, fmt.Sprintf(`{"pubKey":"pk%06d"}`, i)) + if _, err := txStmt.Exec(i, "aabb", hash, ts, 0, 4, 1, fmt.Sprintf(`{"pubKey":"pk%06d"}`, i)); err != nil { + tb.Fatalf("test DB insert transmission %d: %v", i, err) + } nObs := (i % 5) + 1 // 1–5 observations per transmission for j := 0; j < nObs; j++ { snr := -5.0 + float64(j)*2.5 rssi := -90.0 + float64(j)*5.0 - obsStmt.Exec(obsID, i, observers[j], obsNames[j], "RX", snr, rssi, 5-j, `["aa","bb"]`, ts) + if _, err := obsStmt.Exec(obsID, i, observers[j], obsNames[j], "RX", snr, rssi, 5-j, `["aa","bb"]`, ts); err != nil { + tb.Fatalf("test DB insert observation %d: %v", obsID, err) + } obsID++ } } + if _, err := conn.Exec("COMMIT"); err != nil { + tb.Fatalf("test DB COMMIT: %v", err) + } }