feat: Add --wait-db-ms for --exec - #153
Conversation
|
|
Test is failing. Node 16 may be too old; can we require Node 20 or better? |
There was a problem hiding this comment.
Pull request overview
Adds a new CLI option to make mddb --exec safer to run alongside a background mddb --watch process by waiting for the SQLite DB file to become “idle” before executing a script.
Changes:
- Adds
--wait-db-ms <ms>parsing to the CLI--execpath and waits untilmarkdown.dbhas been unchanged for the specified duration. - Documents the new option in the README with usage examples.
- Adds a CLI integration test covering the new waiting behavior.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/bin/index.js |
Adds --wait-db-ms argument parsing and an async “wait until db file is stable” loop before running exec scripts. |
src/tests/cli.spec.ts |
Adds an integration test intended to verify the new wait behavior for --exec. |
README.md |
Documents --wait-db-ms under the --exec section with an example invocation. |
| const start = Date.now(); | ||
| const result = spawnSync( | ||
| process.execPath, | ||
| [cliPath, "--exec", "--wait-db-ms", "200", tmpScript, sampleDir], | ||
| { | ||
| encoding: "utf8", | ||
| cwd: tmpDir, | ||
| } | ||
| ); | ||
| const duration = Date.now() - start; | ||
|
|
||
| expect(result.status).toBe(0); | ||
| expect(result.stdout.trim()).toBe("ok"); | ||
| expect(duration).toBeGreaterThanOrEqual(150); | ||
| } finally { |
There was a problem hiding this comment.
The new duration assertion can pass even if --wait-db-ms is not actually waiting, since spawnSync + Node startup overhead may exceed 150ms on slower machines/CI. Consider making the test more deterministic (e.g., use a larger wait like 400–500ms and assert close to that value, or otherwise assert on observable behavior that only occurs after the wait).
|
@knu you are right - let's get to node 20. implementing that now. do you want to check again once that is merged? |
|
@knu we now have node 20. any updates? |
3fbedaf to
90cc30d
Compare
|
@rufuspollock Thanks! Rebased onto main (now on Node 20). All tests pass locally with the latest base. @copilot Fixed in 90cc30d: the test now compares the waited run against a baseline run (no --wait-db-ms) and asserts the delta is >= waitMs - 100, which factors out spawn/startup overhead. |
c0e1680 to
1cbd3c3
Compare
|
I'm fixing existing problems... |
|
@rufuspollock Done! I have no idea what's going on with Node 16 & 18 builds, nor whether they still matter. |
|
@rufuspollock Done! I have no idea what's going on with Node 16 & 18 builds, nor whether they still matter. |
Add --wait-db-ms for --exec to wait until markdown.db has been idle for the specified duration before running.
Per PR review, comparing the waited run to a baseline run (without --wait-db-ms) makes the assertion robust to spawn/startup overhead that could otherwise let the previous threshold pass without the wait actually elapsing.
9c7e056 to
5f457df
Compare
rufuspollock
changed the title
When running
mddb --watchin the background, the database may be in the middle of an update when you execute a query. This can lead to unexpected or incomplete results. The new--wait-db-msoption allows you to wait for the database to settle (i.e., wait until no writes have occurred for the specified duration) before executing the query, ensuring consistent results.