fix(client): close() shuts the MSP child down cleanly and resolves when it is gone - #12
Closed
Freshair129 wants to merge 1 commit into
Closed
Freshair129 wants to merge 1 commit into
Freshair129 wants to merge 1 commit into
Conversation
…en it is gone createMspStdioCaller's close() called child.kill() and returned immediately. A killed child never closes its SQLite connection, so its write-ahead log is left behind with file handles still open for a moment — and anything reopening that database in the window races a process that is still dying. Eight integration tests and four security tests do exactly that: close the caller, then read the journal or an entity row straight from the file. close() now ends the child's stdin first. The server reads stdin through readline, so ending it lets the process finish on its own and SQLite close and checkpoint the log properly; kill() stays as a one-second fallback for a child that will not leave. close() returns a promise that resolves once the child has actually exited, and the twelve tests that touch the database afterwards now await it. Not a breaking change: close() returned undefined before and the promise is equally ignorable, so a caller that does not care keeps working unchanged. Why now: on better-sqlite3 11 this race is invisible — SQLite tolerates it. On 13 (the upgrade that removes the Node 24 teardown abort, #11) the same sequence fails with SQLITE_IOERR_TRUNCATE. A probe confirmed it is genuinely a race and not a regression in that upgrade: the identical sequence passes three times and fails the fourth, with no wait either way. Verified against better-sqlite3 13 locally (installed for the run, not committed — the dependency move belongs to #11): the three files that failed there now pass 21/21, and two consecutive full runs on Node 24 are 202/202 with security 45/45 twice. That is the first fully green local suite in this work stream. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Superseded by #7, which fixed the same race on main first and more thoroughly: it awaits the child's real exit at 56 call sites across the suite, including the afterEach cleanups, where this PR awaited only the 12 sites that reopen the database immediately. One difference, deliberately not carried over: this PR ended the child's stdin first so the server could finish on its own and SQLite could close and checkpoint its write-ahead log, keeping kill() as a one-second fallback. #7 kills immediately and awaits the exit. Both close the race — awaiting the exit is the part that matters — and main is green on Windows (205/205 twice, security 45/45), so there is nothing left to fix here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prerequisite for #11. Merge this first, and #11 lands green on Windows as well as on CI.
The defect
createMspStdioCaller'sclose()calledchild.kill()and returned immediately. A killed child never closes its SQLite connection, so its write-ahead log is left behind with file handles still open for a moment — and anything that reopens that database in the window races a process that is still dying.Eight integration tests and four security tests do exactly that: close the caller, then read the journal or an entity row straight from the file.
The fix
close()now ends the child's stdin first. The server reads stdin through readline, so ending it lets the process finish on its own and SQLite close and checkpoint its log properly.kill()stays as a one-second fallback for a child that will not leave.close()returns a promise that resolves once the child has actually exited, and the twelve tests that touch the database afterwardsawaitit.Not a breaking change.
close()returnedundefinedbefore and the promise is equally ignorable, so a caller that does not care keeps working unchanged. (Unlike #10, which was breaking and said so.)Why this surfaced now
On
better-sqlite311 the race is invisible — SQLite tolerates it. On 13, the upgrade in #11 that removes the Node 24 teardown abort, the same sequence fails withSQLITE_IOERR_TRUNCATE.I probed it rather than assuming: the identical sequence — spawn, call,
close(), reopen — passes three times and fails the fourth, with no wait in either case. A race, not a regression in the upgrade.Verification
Against
better-sqlite313 locally — installed for the run, not committed, since the dependency move belongs to #11:That is the first fully green local suite in this work stream. On this branch as it stands (better-sqlite3 11), CI is the check that matters, and it is running.
Suggested order
better-sqlite313 + Node 24 is green locally too, not only on CI🤖 Generated with Claude Code