fix: harden turso test cleanup against Windows file-lock flakes - #262
fix: harden turso test cleanup against Windows file-lock flakes#262petra-dot wants to merge 2 commits into
Conversation
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
left a comment
There was a problem hiding this comment.
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
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:
-
maxAttemptsis off by one (sqlite-handle-release.ts:L42-L58). Withprocess.platforminjected aswin32, an operation that always throwsEBUSYandmaxAttempts=2is 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 tomaxRetries. Add a deterministic unit test for exact call counts; the default production budget must remain unchanged. -
Cleanup suppresses every error, not only exhausted Windows file locks (turso-test-utils.ts:L11-L18).
withSqliteFileLockRetrycorrectly rethrows non-lock and non-Windows failures, but the outer catch converts all of them to warnings. I verified thatcleanupTursoTestDirectory("\\0")resolves successfully afterrmSyncthrowsERR_INVALID_ARG_VALUE. That can make the suite green when the path, helper contract, or teardown code is actually broken. Continue only for exhaustedEBUSY/EPERM/EACCESon 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.
|
Addressed both points in 1. 2. Cleanup warns only on exhausted Windows locks ( Deterministic tests (
Platform injection uses Verification: 29 tests pass across the new file and the four focused turso/migration suites, typecheck passes, Prettier clean. |
Windows test flake hardening for the turso/migration tests
Pre-existing Windows flake: under parallel
bun testload, turso migration tests intermittently fail with hook or body timeouts. Root causes and fixes:1. afterEach hook timeout (10s)
cleanupTursoTestDirectorydrove the fullwithSqliteFileLockRetrybudget (~6.4s of delays + GC passes) then threw on EBUSY, blowing the hook.Fix: added an optional
maxAttemptstowithSqliteFileLockRetry(prod callers unchanged — default keeps the full budget).cleanupTursoTestDirectorynow uses 2 attempts inside a try/catch: best-effort hygiene with aconsole.warn, never a suite failure. Each test uses a uniquemkdtempdir, 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'srenameSync(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.tsandturso-migrate-dims-preflight.test.tsnow usebun:sqlitewith explicitfinalize()beforedb.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) andturso-legacy-migrator(7 tests) get a shared 15s timeout via a localmigrationTestwrapper; the dims-preflight test and the embedding-warming import test inmemory-portability-toolget a direct 15s timeout.Verification
configparallel flake).