fix(proxy): default DB path to home dir and auto-create parents#232
fix(proxy): default DB path to home dir and auto-create parents#232maciej-konczal wants to merge 1 commit into
Conversation
|
@maciej-konczal is attempting to deploy a commit to the Finna Team on Vercel. A member of the Team first needs to authorize it. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Greptile SummaryThis PR fixes the
Confidence Score: 4/5Safe to merge — the core fix is a minimal, well-scoped change to db.ts, and the new dev-script flag is backwards-compatible. The implementation is correct and the test covers the primary behaviour. The only gap is that the test never closes the database handle before resetting modules and deleting the temp directory; adding more test cases without addressing this will accumulate open file descriptors and could break cleanup on non-Linux platforms. tests/proxy/db.test.ts — database handle should be closed before vi.resetModules() and tmpDir cleanup to keep the pattern safe as the test suite grows. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[getDb called] --> B{db singleton\ninitialized?}
B -- yes --> G[return db]
B -- no --> C[resolve DB_PATH\nPROXY_DB_PATH env\nor ~/matrixos/proxy.db]
C --> D[mkdirSync dirname DB_PATH\nrecursive: true]
D --> E[new Database DB_PATH]
E --> F[pragma WAL + foreign_keys\nmigrate schema]
F --> G
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
tests/proxy/db.test.ts:24-29
**Open database handle not closed before cleanup**
`getDb()` initialises a module-level singleton that keeps `better-sqlite3`'s database file open. The test never calls `db.close()` (or equivalent) before `vi.resetModules()` and `fs.rmSync()` run in `afterEach`. On Linux the unlink succeeds because the OS allows deleting open files, but the old module's `Database` instance keeps holding the file descriptor until it is garbage-collected. As more `it` blocks are added (e.g. for `insertUsage`, `checkQuota`), each reset accumulates a stale, unclosed handle — which can trigger warnings from `better-sqlite3` and would break the cleanup entirely on Windows. The module should export a `closeDb()` helper (or the test should grab the return value of `getDb()` and call `.close()` directly) before the module is reset.
Reviews (1): Last reviewed commit: "fix(proxy): default DB path to home dir ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| it("creates the parent directory if it does not exist", async () => { | ||
| const { getDb } = await import("../../packages/proxy/src/db.js"); | ||
| expect(() => getDb()).not.toThrow(); | ||
| expect(fs.existsSync(dbPath)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
getDb() initialises a module-level singleton that keeps better-sqlite3's database file open. The test never calls db.close() (or equivalent) before vi.resetModules() and fs.rmSync() run in afterEach. On Linux the unlink succeeds because the OS allows deleting open files, but the old module's Database instance keeps holding the file descriptor until it is garbage-collected. As more it blocks are added (e.g. for insertUsage, checkQuota), each reset accumulates a stale, unclosed handle — which can trigger warnings from better-sqlite3 and would break the cleanup entirely on Windows. The module should export a closeDb() helper (or the test should grab the return value of getDb() and call .close() directly) before the module is reset.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/proxy/db.test.ts
Line: 24-29
Comment:
**Open database handle not closed before cleanup**
`getDb()` initialises a module-level singleton that keeps `better-sqlite3`'s database file open. The test never calls `db.close()` (or equivalent) before `vi.resetModules()` and `fs.rmSync()` run in `afterEach`. On Linux the unlink succeeds because the OS allows deleting open files, but the old module's `Database` instance keeps holding the file descriptor until it is garbage-collected. As more `it` blocks are added (e.g. for `insertUsage`, `checkQuota`), each reset accumulates a stale, unclosed handle — which can trigger warnings from `better-sqlite3` and would break the cleanup entirely on Windows. The module should export a `closeDb()` helper (or the test should grab the return value of `getDb()` and call `.close()` directly) before the module is reset.
How can I resolve this? If you propose a fix, please make it concise.
|
thanks for the pr @maciej-konczal . we have moved away from sqlite, to postgres only. sqlite has been the old db and soem coding agents (mainly claude) still makes mistake. we should move completely to codex. The proper fix is to migrate the proxy persistence tables (api_usage, user_quotas) to Postgres/Kysely using DATABASE_URL or the existing platform/owner Postgres wiring, then remove better-sqlite3 from @matrix-os/proxy. |
|
Closing and #235 provided |
2 participants
Running bun run dev I got: " TypeError: Cannot open database because the directory does not exist
at getDb (packages/proxy/src/db.ts:9:10)"