Prevent recurring native module ABI failures - #2575
Conversation
|
@slopcop review |
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I am reviewing this pull request for security, code quality, performance, architecture, and end-to-end behavior. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This change protects bb when two Node versions share one native database file. It separates hard links before checks. It also repeats the repair before each managed server start.
I found two restart lifecycle problems.
-
P1: The repeated check can use a cached add-on. The launcher keeps the first
better-sqlite3module in memory. A later install can replace the disk file. The launcher then tests the cached module, while the new server loads the bad file. This result can restore the restart loop. Use a fresh child process for every initial check. -
P2: A repair can block shutdown. The repair uses synchronous child processes for downloads and source builds. These calls block the launcher signal handler. After 15 seconds,
bb stopcan kill the launcher and leave child processes alive. Use the existing asynchronous process-group helper.
The security review found no issue. The architecture review found no duplicate repair path. The callback also keeps repository repair policy outside the published launcher, which is the correct boundary.
The focused tests passed: 73 bb-app tests and 121 scripts tests. Both package typechecks passed. The worktree passed git diff --check.
I started the dev server from this branch. The health endpoint responded successfully. Doobie loaded the Settings page without HTTP error responses or browser errors.
The repair idea is sound, but the restart path still has these two failure cases. I left line comments with the details.
|
@slopcop review |
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, duplication, and end-to-end behavior. |
| } = {}) { | ||
| // Each check needs a fresh module cache. The process group also lets the | ||
| // launcher stop a blocked download or source build during shutdown. | ||
| const result = await runBuildProcess({ |
There was a problem hiding this comment.
🚨 slopcop/review — Bound repeated native repair attempts
If this preflight fails during a supervised restart, restartManagedProcess retries it every second without a limit. Each attempt can repeat a prebuilt download and a full node-gyp rebuild. A persistent network or compiler failure can consume CPU and network resources until the user stops bb. Please stop after a repair failure, or add a bounded retry count with a longer delay.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary
This pull request protects each checkout from shared native binaries. It verifies native modules in a fresh process before every managed server start.
Finding
I found one P2 resource issue. A persistent repair failure enters the existing one-second restart loop. Every attempt can repeat a prebuilt download and a full source build. This can use CPU and network resources until the user stops bb. See the inline comment for the requested change.
Other review areas
- Security: I found no new security issue.
- Quality: The change has clear boundaries and focused tests.
- Architecture: The repository has one repair implementation. I found no duplicate repair logic.
- Refactor: A small helper could combine three process-result checks. I do not require that change.
- Performance: The successful preflight has low overhead. The unlimited failure loop is the only performance issue.
Checks
- The
bb-appsuite passed 73 tests. - The
@bb/scriptssuite passed 123 tests. - Both package type checks passed through Turbo.
git diff --check origin/main...HEADpassed.- The production-like worktree start reached ready state.
- A forced server exit produced a new server process.
- Doobie loaded the optimized app with HTTP 200 after the restart.
1 participant
Human comments
What was wrong
Node 22 and Node 24 could use the same
better_sqlite3.nodeinode through pnpm hard links. The prior fix detached the file only after verification found an ABI mismatch. A valid ABI 137 file could therefore stay shared. A later Node 22 install changed that inode to ABI 127. The managed Node 24 server then failed each restart because the launcher checked native modules only before supervision began.What changed
How you verified
pnpm exec turbo run test --filter=@bb/scripts --forcepassed 123 tests.pnpm exec turbo run test --filter=bb-app --forcepassed 73 tests.pnpm exec turbo run typecheck --filter=@bb/scripts --filter=bb-app --forcepassed.pnpm exec turbo run test --filter=@bb/scripts --force -- --run test/start-bb.test.mjspassed four focused tests.Fixes: no linked issue.