fix: resolve absolute DB path to prevent SQLITE_CANTOPEN on CWD change#32
Conversation
mkreyman
left a comment
There was a problem hiding this comment.
Nice work — the core fix is solid. A few things to address before merge:
Must fix
1. Three tests still pass MCP_DB_PATH which the server never reads
Lines 70, 190, 268 in server-initialization.test.ts pass MCP_DB_PATH: dbPath, but src/index.ts only reads DATA_DIR. These tests don't actually control the DB path — post-merge they'll silently write to ~/mcp-data/memory-keeper/ during test runs. Should be DATA_DIR: tempDir like the second test you already fixed.
2. No error handling on fs.mkdirSync
If mkdirSync fails (e.g., permission denied), the server crashes with a raw Node.js stack trace before reaching the DatabaseManager. Since the whole point of this PR is to prevent cryptic startup crashes, a try/catch with an actionable message would be consistent:
try {
fs.mkdirSync(dataDir, { recursive: true });
} catch (err) {
console.error(
`[memory-keeper] FATAL: Cannot create data directory "${dataDir}": ${(err as NodeJS.ErrnoException).message}\n` +
`Set DATA_DIR to a writable location or create the directory manually.`
);
process.exit(1);
}Should fix
3. Version reference says "v0.13.0+" but this will ship as 0.12.x
The Upgrading section header says ### Database path change (v0.13.0+) — should match whichever version this actually releases under.
Happy to push fixup commits for these if you'd like, or let me know if you'd prefer to address them yourself.
|
Thanks for the offer to keep working the issue. I haven't done any open source contributing for a while and it's been fun to get back into it. I also use real-world scenarios like this with my JavaScript students — I stress that they need to work in a real developer environment with real tools, so being able to point to my own PRs as examples makes that lesson land better. The try/catch on I'll address all three items. |
mkreyman
left a comment
There was a problem hiding this comment.
All three items addressed cleanly. Thanks for the thorough work, @cynthiateeters!
|
You're very welcome 😎, @mkreyman |
Summary
Fixes #31.
The server opened
context.dbusing a bare relative path, so SQLite resolved it against the process's CWD at startup. Users runningnode dist/index.jsdirectly (as the README instructed) bypassed thebin/mcp-memory-keeperwrapper that guards against this, causingSQLITE_CANTOPENwhenever the parent process started with a different CWD.Changes
src/index.ts$DATA_DIR/context.db(withpath.resolve()to guard against a relativeDATA_DIR) or~/mcp-data/memory-keeper/context.dbas the default fallbackmkdirSync({ recursive: true })if it does not existstderrwarning with the exactcpcommand when acontext.dbis found in CWD but the server is opening from a different path (migration warning — no auto-copy per discussion in Server crashes with SQLITE_CANTOPEN when parent process CWD changes #31)README.mdbin/mcp-memory-keeperinstead ofnode dist/index.jsTests
server-initialization.test.ts: passesDATA_DIR: tempDirin spawn env so the test reflects the new absolute-path behaviorgit-integration.test.ts: updated hardcoded'master'branch assertion to'main'(pre-existing failure unrelated to this fix)Test plan
DATA_DIR=/tmp/test node dist/index.js→ DB created at/tmp/test/context.db✅HOME=/tmp/fake-home node dist/index.js→ DB created at/tmp/fake-home/mcp-data/memory-keeper/context.db✅DATA_DIR=./relative node dist/index.js→ resolves to absolute path ✅cpcommand when legacycontext.dbdetected in CWD ✅bin/mcp-memory-keeperwrapper continues to work correctly ✅