Address review feedback on the readiness step:
- Read the PID safely (`cat app.pid 2>/dev/null || true`) and treat a
missing/empty PID as "not running", so a failed nohup no longer produces a
confusing `kill` usage error and misleading message.
- Guard the diagnostic dump behind a helper so a missing app.log prints
"(app.log not found)" instead of erroring.
- The exact 24.18.1 pin comes from the Dockerfiles; .nvmrc only pins the 24
major. Corrected the comments to say so rather than citing .nvmrc as the
source of the patch version.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The `test` job intermittently fails at "Wait for app availability" because
`npm run dev` crashes on startup:
node::RemoveEnvironmentCleanupHook(...) at ../src/api/hooks.cc:142
Assertion failed: (env) != nullptr
Statement::~Statement() [better-sqlite3/build/Release/better_sqlite3.node]
Node >= 24.19.0 added node::ObjectWrap cleanup hooks that are incompatible
with NAN-style native addons such as better-sqlite3 (11.9.1, this project's
pinned version), tripping that assertion on teardown. It is intermittent (a
race during native cleanup), so the job fails on some runs and passes on
others.
The app ships on Node 24.18.1 (both Dockerfiles and .nvmrc), which predates
the breaking change, so production is unaffected. Only CI hit it, because
`setup-node` with `node-version: '24'` floats to the latest 24.x (24.20.0 at
time of failure). Pinning CI to 24.18.1 makes it test the version that
actually ships and avoids the regression.
Also capture `npm run dev` output to app.log and fail fast (printing it) if
the process exits early, so a future startup failure is diagnosable instead
of surfacing only as "App failed to start" after a 25s wait.
(Longer term, upgrading better-sqlite3 to v12+ restores Node 24.19+ support;
pinning the runtime is the minimal, prod-matching fix.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owen
miloschwartz
Fred KISSIE
dependabot[bot]
Parikshith