storage: fix SQLite 3.53.4/3.54.0 CLI compatibility for init rollback and busy diagnostics (#1645)

scripts/drivers/storage/sqlite.sh

@@ -162,7 +162,32 @@ storage_init() {
   # store says so, or none of it is visible and the store still says the old
   # revision. A crash or failure in the middle cannot leave a new stamp over
   # an old schema, which is the one way the fast path above could lie.
-  agmsg_sqlite -bail "$db" "
+  #
+  # Fed to sqlite3 on STDIN, not as a command-line argument (#1645). SQLite
+  # 3.53.4 and 3.54.0's CLI only honor -bail's stop-at-first-error contract
+  # when every top-level statement's terminating `;` ends its own physical
+  # line; the exact same statements handed over as a single argv string ran
+  # a failing statement past the error anyway and still reached the
+  # PRAGMA/COMMIT below it, stamping the current revision over a schema that
+  # never finished (3.51.0 did not have this gap). The statements below
+  # already satisfy the per-line shape -- a statement may span several
+  # lines, as CREATE TABLE events does, but never shares a line with another
+  # statement -- so moving them to stdin needs no reshaping, only the
+  # transport.
+  #
+  # A heredoc redirect, not a `printf | agmsg_sqlite` pipe (review on
+  # #1645/#1654): a pipe's right-hand side is a subshell, so the escape
+  # probe's memo set there dies with it and every call needs its own
+  # agmsg_sqlite_warm -- which is also what tests/test_remote_sync.bats's
+  # #462 scan looks for within two lines above a `| agmsg_sqlite`, and a
+  # warm 100 lines above the actual pipe here read as unwarmed and failed
+  # that scan. `<<SQL ... SQL` runs in the CURRENT shell, so there is no
+  # subshell to lose the memo to and no warm call needed here at all. The
+  # delimiter is intentionally unquoted (`<<SQL`, not `<<'SQL'`) so
+  # ${_AGMSG_STORAGE_SCHEMA_REV} below still expands; nothing else in this
+  # block contains a `$` or a backtick for that same unquoted expansion to
+  # misfire on.
+  agmsg_sqlite -bail "$db" <<SQL >/dev/null 2>&1 || { echo runtime_error; return 13; }
     BEGIN IMMEDIATE;
     CREATE TABLE IF NOT EXISTS events (
       seq        INTEGER PRIMARY KEY AUTOINCREMENT,
@@ -262,7 +287,7 @@ storage_init() {
       VALUES('read_cursor_v1','1');
     PRAGMA user_version=${_AGMSG_STORAGE_SCHEMA_REV};
     COMMIT;
-  " >/dev/null 2>&1 || { echo runtime_error; return 13; }
+SQL
   echo ok
 }
 

scripts/lib/storage.sh

@@ -332,6 +332,43 @@ agmsg_sqlite_warm() {
   [ -n "$_AGMSG_ESCAPE_PROBED" ] || _agmsg_escape_flag >/dev/null
 }
 
+# Is $1 (sqlite3's full stderr, possibly several lines) a real lock-timeout
+# giveup -- never a statement whose own error text happens to MENTION
+# "database is locked", e.g. a table or column named that, or a failed
+# statement displayed verbatim in a parse error. A plain substring test
+# cannot tell the two apart (review on #1645/#1654: a `SELECT` against a
+# table literally named "database is locked" read as busy and got the
+# busy diagnostic, when it was really "no such table"). Matched instead
+# against the exact whole lines sqlite3 itself emits for SQLITE_BUSY,
+# collected by reproducing a real lock timeout on both argv and stdin input
+# and on both supported CLI generations:
+#   3.51.0, argv:  Error: in prepare, database is locked (5)
+#   3.51.0, argv:  Error: stepping, database is locked (5)
+#   3.51.0, stdin: Parse error near line N: database is locked (5)
+#   3.53.4, argv:  Parse error in Nth command line argument: database is locked (5)
+#   3.53.4, argv:  Error in Nth command line argument: database is locked
+#   3.53.4, stdin: Parse error near line N: database is locked (5)
+# Anchored start-to-end per line, with no wildcard around "database is
+# locked" itself, so a line that embeds extra text around it (the
+# counter-example above renders as "...: no such table: database is
+# locked") fails every alternative and is correctly left unclassified.
+#
+# Windows' sqlite3.exe ends stderr lines with \r\n, not \n (the same CRLF
+# behaviour as its stdout, see agmsg_sqlite()'s own CRLF comment above) --
+# left in, the trailing \r sits between "locked" and the `$` anchor and
+# every alternative misses, so a real busy timeout goes unclassified on
+# Windows (review on #1654). Stripped with a builtin substitution, not an
+# external command, matching ONLY the string this match is run against;
+# the original stderr this function was handed is still re-emitted to the
+# caller's stderr unchanged by agmsg_sqlite() itself.
+_agmsg_sqlite_err_is_busy() {
+  local _err="${1//$'\r'/}"
+  printf '%s\n' "$_err" | grep -Eq \
+    '^Error: (in prepare|stepping), database is locked( \([0-9]+\))?$'\
+'|^(Error|Parse error) in [0-9]+(st|nd|rd|th) command line argument: database is locked( \([0-9]+\))?$'\
+'|^(Error|Parse error) near line [0-9]+: database is locked( \([0-9]+\))?$'
+}
+
 agmsg_sqlite() {
   # Probe in THIS shell, not in a command substitution. `$(_agmsg_escape_flag)`
   # ran the function in a subshell, so the memo it set was discarded on exit and
@@ -380,12 +417,41 @@ agmsg_sqlite() {
   # status is sqlite3's, not sed's, without changing pipefail for the
   # calling script (same shape as _sqlite_data / _sqlite_data_stdin in
   # drivers/storage/sqlite.sh).
-  local _agmsg_sqlite_rc=0
-  (
-    set -o pipefail
-    # shellcheck disable=SC2086  # intentional split: "-escape off" → two args, or none
-    sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" | sed $'s/\r$//'
-  ) || _agmsg_sqlite_rc=$?
+  # stderr is captured into a variable, not a temp file: this function is on
+  # the hot path (every send/receive/watch.sh cycle), so mktemp+cat+rm on
+  # EVERY call -- including the common all-ok case -- would add 3 processes
+  # per call for the busy-text check below alone, the same class of per-cycle
+  # process cost #1627/#1630/#1652 were about removing, and the kind of
+  # leftover-temp-file risk #1580 just fixed. The classic fd-juggle instead:
+  # `3>&1` on the outer group saves this function's real stdout onto fd 3;
+  # inside the command substitution, `1>&3` then points the pipeline's
+  # stdout back at that real stdout (bypassing the substitution's own
+  # capture), while `2>&1` (evaluated first, while fd 1 still means the
+  # substitution's capture) sends the pipeline's stderr there instead. Net
+  # effect: stdout streams straight through to the caller as before, stderr
+  # lands in $_agmsg_sqlite_err, and no process beyond sqlite3/sed is forked.
+  local _agmsg_sqlite_rc=0 _agmsg_sqlite_err
+  {
+    _agmsg_sqlite_err=$(
+      set -o pipefail
+      # shellcheck disable=SC2086  # intentional split: "-escape off" → two args, or none
+      { sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" | sed $'s/\r$//'; } 2>&1 1>&3 3>&-
+    )
+  } 3>&1 || _agmsg_sqlite_rc=$?
+  [ -z "$_agmsg_sqlite_err" ] || printf '%s\n' "$_agmsg_sqlite_err" >&2
+  # SQLite 3.53.4/3.54.0's CLI returns the generic 1 for a lock-timeout
+  # giveup instead of 5, on the same failure 3.51.0 reports as 5 (#1645) --
+  # so rc alone can no longer tell a busy timeout from any other failure on
+  # every supported CLI. _agmsg_sqlite_err_is_busy() recognizes the exact
+  # lines sqlite3 itself emits for this, so a real lock timeout still
+  # normalizes to the historical 5 (keeping every existing rc==5 caller,
+  # including the diagnostic below, working unchanged on either CLI
+  # generation), while an ordinary error that merely mentions "database is
+  # locked" in a table/column name or an echoed statement does not.
+  if [ "$_agmsg_sqlite_rc" -ne 0 ] && [ "$_agmsg_sqlite_rc" -ne 5 ] &&
+     _agmsg_sqlite_err_is_busy "$_agmsg_sqlite_err"; then
+    _agmsg_sqlite_rc=5
+  fi
   # SQLITE_BUSY after the full timeout used to pass in silence: the caller saw
   # a non-zero it often swallowed, and the operator saw a command that hung
   # for the timeout and said nothing (#1001 -- two people diagnosed two