From c19cb706e8a374bb60fabb679f94060c602a3f95 Mon Sep 17 00:00:00 2001
From: Parikshith <parikshith@example.com>
Date: Tue, 8 Sep 2026 16:28:35 +0530
Subject: [PATCH] fix(ci): harden app-readiness PID check; clarify
 version-source comment

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>
---
 .github/workflows/linting.yml |  5 +++--
 .github/workflows/test.yml    | 17 ++++++++++++-----
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/.github/workflows/linting.yml b/.github/workflows/linting.yml
index 55e872dee..4bd0d2a90 100644
--- a/.github/workflows/linting.yml
+++ b/.github/workflows/linting.yml
@@ -26,8 +26,9 @@ jobs:
             - name: Set up Node.js
               uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
               with:
-                # Match the version the app ships on (Dockerfile / .nvmrc)
-                # rather than floating to the latest 24.x.
+                # Match the version the app ships on (Dockerfile /
+                # Dockerfile.dev both use 24.18.1) rather than floating
+                # to the latest 24.x.
                 node-version: '24.18.1'
 
             - name: Install dependencies
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index f9fbf6fa7..e53630826 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -19,7 +19,8 @@ jobs:
       - name: Install Node
         uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
         with:
-          # Pin to the version the app actually ships on (Dockerfile / .nvmrc).
+          # Pin to the version the app actually ships on (Dockerfile /
+          # Dockerfile.dev both use 24.18.1; .nvmrc pins the 24 major).
           # A bare '24' floats to the latest 24.x; Node >= 24.19.0 added
           # node::ObjectWrap cleanup hooks that crash better-sqlite3 on teardown
           # ("Assertion failed: (env) != nullptr" in RemoveEnvironmentCleanupHook),
@@ -54,22 +55,28 @@ jobs:
 
       - name: Wait for app availability
         run: |
+          print_log() {
+            if [ -f app.log ]; then cat app.log; else echo "(app.log not found)"; fi
+          }
           for i in {1..5}; do
             if curl --silent --fail http://localhost:3002/auth/login; then
               echo "App is up"
               exit 0
             fi
-            # Fail fast (and show why) if the process already exited.
-            if ! kill -0 "$(cat app.pid)" 2>/dev/null; then
+            # Fail fast (and show why) if the process is gone. Read the PID
+            # safely: if it's missing/empty (e.g. nohup never started), treat
+            # that as "not running" instead of passing a bad arg to kill.
+            pid="$(cat app.pid 2>/dev/null || true)"
+            if [ -z "$pid" ] || ! kill -0 "$pid" 2>/dev/null; then
               echo "App process exited before becoming available. Output:"
-              cat app.log
+              print_log
               exit 1
             fi
             echo "Waiting for the app... attempt $i"
             sleep 5
           done
           echo "App failed to start. Output:"
-          cat app.log
+          print_log
           exit 1
 
   build-sqlite: