fix(test): harden preload cleanup against Windows EBUSY#14895
fix(test): harden preload cleanup against Windows EBUSY#14895Hona merged 1 commit intoanomalyco:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens test teardown on Windows by explicitly closing the SQLite client used during tests and replacing one-shot temp directory removal with a retrying cleanup that mitigates EBUSY from lingering WAL handles.
Changes:
- Add
Database.close()to explicitly close the underlyingbun:sqlitehandle and reset the lazy client. - Update test preload teardown to GC + retry
fs.rm()cleanup to avoid flaky WindowsEBUSYfailures. - Add inline rationale explaining why GC + retry is required on Windows.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/opencode/test/preload.ts | Switch teardown to async cleanup with GC + retry loop and close DB before removing temp dir. |
| packages/opencode/src/storage/db.ts | Track the underlying SQLite handle and expose Database.close() for explicit teardown/reset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const busy = (error: unknown) => | ||
| typeof error === "object" && error !== null && "code" in error && error.code === "EBUSY" | ||
| const rm = async (left: number): Promise<void> => { | ||
| Bun.gc(true) | ||
| await Bun.sleep(100) | ||
| return fs.rm(dir, { recursive: true, force: true }).catch((error) => { | ||
| if (!busy(error)) throw error | ||
| if (left <= 1) throw error | ||
| return rm(left - 1) | ||
| }) | ||
| } | ||
|
|
||
| // Windows can keep SQLite WAL handles alive until GC finalizers run, so we | ||
| // force GC and retry teardown to avoid flaky EBUSY in test cleanup. | ||
| await rm(30) |
There was a problem hiding this comment.
The cleanup loop always forces a full GC and sleeps 100ms even on non-Windows platforms (where EBUSY is unlikely). This adds a fixed delay and extra GC pressure to every test run; consider gating the GC+retry logic behind process.platform === "win32" (or similar) and using a single fs.rm on other platforms.
| const busy = (error: unknown) => | |
| typeof error === "object" && error !== null && "code" in error && error.code === "EBUSY" | |
| const rm = async (left: number): Promise<void> => { | |
| Bun.gc(true) | |
| await Bun.sleep(100) | |
| return fs.rm(dir, { recursive: true, force: true }).catch((error) => { | |
| if (!busy(error)) throw error | |
| if (left <= 1) throw error | |
| return rm(left - 1) | |
| }) | |
| } | |
| // Windows can keep SQLite WAL handles alive until GC finalizers run, so we | |
| // force GC and retry teardown to avoid flaky EBUSY in test cleanup. | |
| await rm(30) | |
| if (process.platform === "win32") { | |
| const busy = (error: unknown) => | |
| typeof error === "object" && error !== null && "code" in error && (error as { code?: unknown }).code === "EBUSY" | |
| const rm = async (left: number): Promise<void> => { | |
| Bun.gc(true) | |
| await Bun.sleep(100) | |
| return fs.rm(dir, { recursive: true, force: true }).catch((error) => { | |
| if (!busy(error)) throw error | |
| if (left <= 1) throw error | |
| return rm(left - 1) | |
| }) | |
| } | |
| // Windows can keep SQLite WAL handles alive until GC finalizers run, so we | |
| // force GC and retry teardown to avoid flaky EBUSY in test cleanup. | |
| await rm(30) | |
| } else { | |
| // On non-Windows platforms, EBUSY is unlikely; a single rm is sufficient. | |
| await fs.rm(dir, { recursive: true, force: true }) | |
| } |
| export const Client = lazy(() => { | ||
| log.info("opening database", { path: path.join(Global.Path.data, "opencode.db") }) | ||
|
|
||
| const sqlite = new BunDatabase(path.join(Global.Path.data, "opencode.db"), { create: true }) | ||
| state.sqlite = sqlite | ||
|
|
There was a problem hiding this comment.
state.sqlite is assigned immediately after creating the BunDatabase, but Client initialization can still throw later (e.g., during migrations). If an error is thrown, lazy() will retry on the next call and state.sqlite may get overwritten, leaking the original open handle and making close() behavior ambiguous. Consider only setting state.sqlite after successful initialization, or ensuring the created handle is closed/cleared on initialization failure.
Summary