From be39118a8bd9748b31e511c9bcc11c97b0b0905b Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 13 Oct 2026 13:18:57 +0200 Subject: [PATCH] test: fix flaky run() compaction test and hang-on-failure The time-bucketed scheduling test polled restore() while run() was still performing its first sync. On a slow runner the replica was still empty, so restore() threw NoMatchingBackupError and the helper rethrew it instead of retrying. Because ac.abort() only ran on the success path, the failed test left run() looping forever and the process never exited, which hung the release workflow. - waitForRestoredRows retries transient restore failures until its deadline and reports the last error on timeout - always abort and await run() in finally (compaction and leasing tests) - release workflow: skip tests (CI covers them) and add a job timeout Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0162xGMZU6wtdADEwqhqNqnY --- .github/workflows/release.yml | 11 ++- test/integration/compaction-retention.test.ts | 43 ++++++---- .../integration/compaction-scheduling.test.ts | 31 ++++--- test/integration/leasing.test.ts | 80 ++++++++++--------- 4 files changed, 98 insertions(+), 67 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 33218b3..670b71b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1,8 +1,9 @@ name: Publish release -# Manually triggered release. Bumps every workspace package to the same -# version, pushes the bump commit, publishes to npm via OIDC trusted -# publishing (no NPM_TOKEN) and creates the GitHub release. +# Manually triggered release. Tests are not run here: they run in CI on every +# push to main. Bumps every workspace package to the same version, pushes the +# bump commit, publishes to npm via OIDC trusted publishing (no NPM_TOKEN) and +# creates the GitHub release. # # Publishing is only possible from the `main` environment, which is # restricted to the `main` branch; the npm trusted publisher configuration @@ -24,6 +25,7 @@ jobs: release-npm: if: ${{ github.ref == 'refs/heads/main' }} runs-on: ubuntu-latest + timeout-minutes: 15 environment: main permissions: id-token: write @@ -51,9 +53,6 @@ jobs: - name: Lint (ESLint + neostandard, type-stripping guard) run: pnpm lint - - name: Test - run: pnpm test:all - - name: Bump version id: bump env: diff --git a/test/integration/compaction-retention.test.ts b/test/integration/compaction-retention.test.ts index 741f326..5610abb 100644 --- a/test/integration/compaction-retention.test.ts +++ b/test/integration/compaction-retention.test.ts @@ -147,14 +147,14 @@ test('run() schedules compaction and retention while replicating', async () => { const ac = new AbortController(); const outPath = join(makeTempDir('restore-'), 'w.db'); + const runPromise = replica.run({ + intervalMs: 10, + compactionIntervalMs: 40, + retentionIntervalMs: 80, + retention: { snapshotRetain: 1, l0RetentionMs: 0 }, + signal: ac.signal, + }); try { - const runPromise = replica.run({ - intervalMs: 10, - compactionIntervalMs: 40, - retentionIntervalMs: 80, - retention: { snapshotRetain: 1, l0RetentionMs: 0 }, - signal: ac.signal, - }); // New writes land as additional L0 files over the next several ticks. for (let i = 0; i < 5; i++) { @@ -191,6 +191,10 @@ test('run() schedules compaction and retention while replicating', async () => { 'the live-head level-0 file is retained', ); } finally { + // Always stop the run loop, or a failed assertion leaves it (and the test + // process) running forever. + ac.abort(); + await runPromise; replica.close(); t.close(); } @@ -204,17 +208,24 @@ async function waitForRestoredRows( timeoutMs: number, ): Promise { const deadline = Date.now() + timeoutMs; + let last = 'no restore attempted'; for (;;) { - await replica.restore({ outputPath: outPath }); - const d = new DatabaseSync(outPath); - const n = (d.prepare('SELECT count(*) AS n FROM t').get() as { n: number }).n; - const ic = (d.prepare('PRAGMA integrity_check').get() as { integrity_check: string }) - .integrity_check; - d.close(); - if (n === expected && ic === 'ok') return; - if (Date.now() > deadline) { - throw new Error(`timed out waiting for ${expected} rows (got ${n})`); + // The run loop is live: before the first sync lands the replica is empty + // (NoMatchingBackupError), and compaction/retention may change the file set + // mid-restore. Treat any failure as "not there yet" and retry. + try { + await replica.restore({ outputPath: outPath }); + const d = new DatabaseSync(outPath); + const n = (d.prepare('SELECT count(*) AS n FROM t').get() as { n: number }).n; + const ic = (d.prepare('PRAGMA integrity_check').get() as { integrity_check: string }) + .integrity_check; + d.close(); + if (n === expected && ic === 'ok') return; + last = `got ${n} rows, integrity_check=${ic}`; + } catch (err) { + last = `restore failed: ${(err as Error).message}`; } + if (Date.now() > deadline) throw new Error(`timed out waiting for ${expected} rows (${last})`); await sleep(150); } } diff --git a/test/integration/compaction-scheduling.test.ts b/test/integration/compaction-scheduling.test.ts index bd01a67..3f4d5c8 100644 --- a/test/integration/compaction-scheduling.test.ts +++ b/test/integration/compaction-scheduling.test.ts @@ -146,8 +146,8 @@ test('run() compacts per level using time-bucketed scheduling', async () => { { level: 3, intervalMs: 120 }, ]); + const runPromise = replica.run({ intervalMs: 10, compactionLevels: levels, signal: ac.signal }); try { - const runPromise = replica.run({ intervalMs: 10, compactionLevels: levels, signal: ac.signal }); for (let i = 0; i < 5; i++) { t.db.exec("INSERT INTO t (name) VALUES ('x')"); @@ -174,6 +174,10 @@ test('run() compacts per level using time-bucketed scheduling', async () => { for (let level = 1; level <= 3; level++) compacted += (await countLevel(client, level)).length; assert.ok(compacted >= 1, 'per-level scheduling produced a higher-level file'); } finally { + // Always stop the run loop, or a failed assertion leaves it (and the test + // process) running forever. + ac.abort(); + await runPromise; replica.close(); t.close(); } @@ -186,15 +190,24 @@ async function waitForRestoredRows( timeoutMs: number, ): Promise { const deadline = Date.now() + timeoutMs; + let last = 'no restore attempted'; for (;;) { - await replica.restore({ outputPath: outPath }); - const d = new DatabaseSync(outPath); - const n = (d.prepare('SELECT count(*) AS n FROM t').get() as { n: number }).n; - const ic = (d.prepare('PRAGMA integrity_check').get() as { integrity_check: string }) - .integrity_check; - d.close(); - if (n === expected && ic === 'ok') return; - if (Date.now() > deadline) throw new Error(`timed out waiting for ${expected} rows (got ${n})`); + // The run loop is live: before the first sync lands the replica is empty + // (NoMatchingBackupError), and compaction/retention may change the file set + // mid-restore. Treat any failure as "not there yet" and retry. + try { + await replica.restore({ outputPath: outPath }); + const d = new DatabaseSync(outPath); + const n = (d.prepare('SELECT count(*) AS n FROM t').get() as { n: number }).n; + const ic = (d.prepare('PRAGMA integrity_check').get() as { integrity_check: string }) + .integrity_check; + d.close(); + if (n === expected && ic === 'ok') return; + last = `got ${n} rows, integrity_check=${ic}`; + } catch (err) { + last = `restore failed: ${(err as Error).message}`; + } + if (Date.now() > deadline) throw new Error(`timed out waiting for ${expected} rows (${last})`); await sleep(100); } } diff --git a/test/integration/leasing.test.ts b/test/integration/leasing.test.ts index d7a93b5..cd27aea 100644 --- a/test/integration/leasing.test.ts +++ b/test/integration/leasing.test.ts @@ -130,28 +130,34 @@ describe('Replica.run() with a leaser (primary/follower)', () => { // A starts first → becomes primary. const acA = new AbortController(); const runA = replicaA.run({ intervalMs: 20, signal: acA.signal }); - await waitFor(() => replicaA.isPrimary()); - assert.ok(replicaA.isPrimary()); - - // B starts while A holds the lease → follower. const acB = new AbortController(); - const runB = replicaB.run({ intervalMs: 20, signal: acB.signal }); - await waitFor(() => !replicaB.isPrimary(), 2000); - assert.ok(!replicaB.isPrimary()); - - // A follower must not be able to write directly. - await assert.rejects(replicaB.sync(), ErrLeaseNotHeld); - - // Stop the primary → it releases the lease → B becomes primary. - acA.abort(); - await runA; - await waitFor(() => replicaB.isPrimary(), 3000); - assert.ok(replicaB.isPrimary()); - - acB.abort(); - await runB; - db.close(); - await rm(dir, { recursive: true, force: true }); + let runB: Promise = Promise.resolve(); + try { + await waitFor(() => replicaA.isPrimary()); + assert.ok(replicaA.isPrimary()); + + // B starts while A holds the lease → follower. + runB = replicaB.run({ intervalMs: 20, signal: acB.signal }); + await waitFor(() => !replicaB.isPrimary(), 2000); + assert.ok(!replicaB.isPrimary()); + + // A follower must not be able to write directly. + await assert.rejects(replicaB.sync(), ErrLeaseNotHeld); + + // Stop the primary → it releases the lease → B becomes primary. + acA.abort(); + await runA; + await waitFor(() => replicaB.isPrimary(), 3000); + assert.ok(replicaB.isPrimary()); + } finally { + // Always stop both loops (and their lease timers), or a failed assertion + // leaves them running and the test process never exits. + acA.abort(); + acB.abort(); + await Promise.allSettled([runA, runB]); + db.close(); + await rm(dir, { recursive: true, force: true }); + } }); test('dedicated renewal timer keeps the lease alive with a large loop interval', async () => { @@ -168,21 +174,23 @@ describe('Replica.run() with a leaser (primary/follower)', () => { const ac = new AbortController(); const runPromise = replica.run({ intervalMs: 5000, signal: ac.signal }); - await waitFor(() => replica.isPrimary()); - - // Wait well past the 300ms TTL. If renewal were tied to the loop (5s), the - // lock would have expired. With the timer it stays current. - await new Promise((r) => setTimeout(r, 700)); - const lock = JSON.parse( - await (await import('node:fs/promises')).readFile(nodePath.join(dir, 'lock.json'), 'utf8'), - ) as { expires_at: string }; - assert.ok(Date.parse(lock.expires_at) > Date.now(), 'lease should have been renewed'); - assert.ok(replica.isPrimary()); - - ac.abort(); - await runPromise; - db.close(); - await rm(dir, { recursive: true, force: true }); + try { + await waitFor(() => replica.isPrimary()); + + // Wait well past the 300ms TTL. If renewal were tied to the loop (5s), the + // lock would have expired. With the timer it stays current. + await new Promise((r) => setTimeout(r, 700)); + const lock = JSON.parse( + await (await import('node:fs/promises')).readFile(nodePath.join(dir, 'lock.json'), 'utf8'), + ) as { expires_at: string }; + assert.ok(Date.parse(lock.expires_at) > Date.now(), 'lease should have been renewed'); + assert.ok(replica.isPrimary()); + } finally { + ac.abort(); + await runPromise; + db.close(); + await rm(dir, { recursive: true, force: true }); + } }); });