Skip to content

fix: harden turso test cleanup against Windows file-lock flakes - #262

Open
petra-dot wants to merge 2 commits into
tickernelz:mainfrom
petra-dot:fix/turso-test-cleanup-timeout
Open

fix: harden turso test cleanup against Windows file-lock flakes#262
petra-dot wants to merge 2 commits into
tickernelz:mainfrom
petra-dot:fix/turso-test-cleanup-timeout

Conversation

@petra-dot

Copy link
Copy Markdown
Contributor

Windows test flake hardening for the turso/migration tests

Pre-existing Windows flake: under parallel bun test load, turso migration tests intermittently fail with hook or body timeouts. Root causes and fixes:

1. afterEach hook timeout (10s)

cleanupTursoTestDirectory drove the full withSqliteFileLockRetry budget (~6.4s of delays + GC passes) then threw on EBUSY, blowing the hook.
Fix: added an optional maxAttempts to withSqliteFileLockRetry (prod callers unchanged — default keeps the full budget). cleanupTursoTestDirectory now uses 2 attempts inside a try/catch: best-effort hygiene with a console.warn, never a suite failure. Each test uses a unique mkdtemp dir, so a leftover is harmless garbage.

2. legacy-migrator body EBUSY on the shard rename

The fixture DBs were created with @libsql/client. client.close() leaves prepared-statement handles alive until GC (tursodatabase/libsql-js#228), so under load the migration's renameSync (already guarded by the prod retry) could hit EBUSY long enough to exhaust the ~6.4s budget and abort.
Fix: fixtures in turso-legacy-migrator.test.ts and turso-migrate-dims-preflight.test.ts now use bun:sqlite with explicit finalize() before db.close() — the file handle is released deterministically, no GC dependency.

3. 5s body timeouts on migration-heavy tests under load

Real file swaps of live SQLite files (plus CPU-heavy embedding warmup in the portability import path) legitimately exceed the 5s default on a loaded box.
Fix: shard-path-migrate (9 tests) and turso-legacy-migrator (7 tests) get a shared 15s timeout via a local migrationTest wrapper; the dims-preflight test and the embedding-warming import test in memory-portability-tool get a direct 15s timeout.

Verification

  • Two consecutive full-suite runs: all turso/migration tests green; only the pre-existing env artifacts remain (onnxruntime symlink EPERM on Windows, config parallel flake).
  • Typecheck + prettier clean.

Three flake classes hit the turso/migration tests on loaded Windows:

- afterEach hook timeout: cleanupTursoTestDirectory drove the full 8-attempt file-lock retry (6.4s of delays) then threw. Bounded to 2 attempts and made best-effort (warn only) via a new optional maxAttempts on withSqliteFileLockRetry; prod callers keep the full budget unchanged.
- legacy-migrator body EBUSY on the shard rename: fixture DBs created with @libsql/client left prepared-statement handles alive past close() until GC (tursodatabase/libsql-js#228), blocking the migration's rename under load. Fixtures now use bun:sqlite with explicit finalize() before close(), which releases the file handle deterministically.
- 5s body timeouts on migration-heavy tests under parallel load: shard-path-migrate (9 tests) and turso-legacy-migrator (7 tests) get a shared 15s timeout; the embedding-warming import test in memory-portability-tool and the dims-preflight test get 15s.

Verified: two consecutive full-suite runs green for all turso/migration tests (only the pre-existing onnxruntime symlink + config parallel env artifacts remain).

@lindixu6-hash lindixu6-hash left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intent: make the Windows Turso/migration tests deterministic by releasing fixture handles, bounding cleanup retries, and widening only migration-heavy test budgets.

flowchart LR
  A[close Turso handles] --> B[remove unique temp dir]
  B --> C{Windows retryable lock?}
  C -->|yes, budget remains| D[GC + bounded retry]
  D --> B
  C -->|yes, exhausted| E[warn and continue]
  C -->|no / programming error| F[fail the test]
  style D fill:#bbdefb,color:#0d47a1
  style E fill:#fff3e0,color:#e65100
  style F fill:#ffcdd2,color:#b71c1c
Loading

The fixture switch to bun:sqlite, targeted timeouts, 21 focused tests, typecheck, Prettier, and the six-platform package-smoke matrix all look consistent with that intent. Two failure-sensitive gaps still need changes:

  1. maxAttempts is off by one (sqlite-handle-release.ts:L42-L58). With process.platform injected as win32, an operation that always throws EBUSY and maxAttempts=2 is invoked 3 times. The stop test is applied after attempt index 2 has already entered the operation. Use an unambiguous total-attempt contract (attempt + 1 >= maxAttempts), validate a positive integer, or rename the parameter to maxRetries. Add a deterministic unit test for exact call counts; the default production budget must remain unchanged.

  2. Cleanup suppresses every error, not only exhausted Windows file locks (turso-test-utils.ts:L11-L18). withSqliteFileLockRetry correctly rethrows non-lock and non-Windows failures, but the outer catch converts all of them to warnings. I verified that cleanupTursoTestDirectory("\\0") resolves successfully after rmSync throws ERR_INVALID_ARG_VALUE. That can make the suite green when the path, helper contract, or teardown code is actually broken. Continue only for exhausted EBUSY/EPERM/EACCES on Windows; rethrow everything else. Add tests proving both the bounded-lock warning path and non-lock propagation.

Independent exact-head verification on c3bfc1e90258bc76e6bdd728b259ef35e42a5a44:

  • four focused files: 21 pass, 0 fail, 118 assertions (the command then reported only the local sandbox's temp-directory cleanup restriction);
  • typecheck: pass;
  • Prettier on all changed files: pass;
  • upstream run 32291730839: Ubuntu, Windows, macOS 15/26 Intel, and macOS 15/26 Apple Silicon all pass.

Please keep the deterministic bun:sqlite fixture finalization, but make the retry/error contract testable rather than relying on full-suite success to cover these branches.

Rename maxAttempts to maxRetries: the operation now runs exactly
maxRetries + 1 times (initial attempt plus retries), validated as a
non-negative integer. Previously the stop check ran after entering the
operation, so maxAttempts=2 invoked it 3 times.

Scope cleanupTursoTestDirectory's warning to exhausted Windows lock
codes only (EBUSY/EPERM/EACCES); rethrow everything else so broken
paths or teardown bugs surface instead of silently passing.

Add deterministic unit tests for exact retry call counts and both
cleanup paths.
@petra-dot

Copy link
Copy Markdown
Contributor Author

Addressed both points in f57c6ec.

1. maxRetries exact-call contract (src/services/turso/sqlite-handle-release.ts)
Renamed maxAttempts to maxRetries and validated it as a non-negative integer. The stop check now runs before retrying, so the operation executes exactly maxRetries + 1 times. The default is still FILE_LOCK_RETRY_DELAYS_MS.length (8), which keeps the production budget bit-identical to the pre-parameter behavior: 9 calls, all 8 delays. All production callers pass no second argument and are untouched.

2. Cleanup warns only on exhausted Windows locks (tests/turso-test-utils.ts)
The catch now routes through isExhaustedWindowsLock(error): warn + continue only when process.platform === "win32" and the error code is in RETRYABLE_FILE_LOCK_CODES (exported so the set lives in one place). A Windows lock error can only escape the retry with its budget exhausted, so that is exactly the sole warning path; every other error rethrows. cleanupTursoTestDirectory("\0") now rejects with ERR_INVALID_ARG_VALUE instead of silently passing.

Deterministic tests (tests/sqlite-handle-release.test.ts, 8 new)

  • maxRetries=2 + persistent EBUSY on forced-win32 platform → exactly 3 calls
  • maxRetries=0 → exactly 1 call
  • non-Windows platform → no retry, 1 call
  • non-lock error → no retry, 1 call
  • invalid maxRetries (-1, 1.5) → TypeError
  • eventual success after one EBUSY → 2 calls, returns value
  • cleanupTursoTestDirectory("\0") → rethrows (non-lock propagation)
  • exhausted Windows lock (open bun:sqlite handle blocking rmSync) → warns, does not throw; guarded with it.skipIf(process.platform !== "win32")

Platform injection uses Object.defineProperty(process, "platform", { value, configurable: true }) restored in afterAll, so the exact-count assertions are deterministic on all six CI platforms.

Verification: 29 tests pass across the new file and the four focused turso/migration suites, typecheck passes, Prettier clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants