diff --git a/scripts/check-inbox.sh b/scripts/check-inbox.sh index b14939137..64795abe3 100755 --- a/scripts/check-inbox.sh +++ b/scripts/check-inbox.sh @@ -283,13 +283,30 @@ for team in "${TEAM_LIST[@]}"; do # _sqlite_sync_lit_into in sqlite-sync.sh, which documents the same hazard. _AGMSG_SQ="'" _arr="[$(printf '%s' "$UNREAD_JSONL" | paste -sd, -)]" - agmsg_sqlite ':memory:' " - SELECT json_extract(value,'\$.from') || char(31) || - replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) || - json_extract(value,'\$.at') || char(31) || - json_extract(value,'\$.id') - FROM json_each('${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}'); - " + # #777: this team's unread backlog grows with every message sent to it, so + # interpolating it into ONE argv element eventually exceeds the OS's + # per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 bytes; smaller still + # on Windows/macOS) and `agmsg_sqlite` fails with "Argument list too + # long" -- every single poll, since the backlog that triggered it never + # shrinks on its own. Pass the statement on stdin instead, mirroring + # drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, #882) and + # history.sh/inbox.sh: printf is a bash builtin, so writing a large value + # to a temp file never execs and can hit neither that ceiling nor argv's + # at all. The temp file is scoped to THIS subshell -- its EXIT trap fires + # when the subshell itself exits (success, `exit 98`/`exit 13` above, or a + # signal), never touching the outer script's own traps. + _agmsg_ci_sql=$(mktemp "${TMPDIR:-/tmp}/agmsg-checkinbox-rows.XXXXXX") || exit 13 + trap 'rm -f "$_agmsg_ci_sql"' EXIT HUP INT TERM + { + printf "%s\n" "SELECT json_extract(value,'\$.from') || char(31) ||" + printf "%s\n" " replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.at') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.id')" + printf "FROM json_each('" + printf '%s' "${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}" + printf "');\n" + } > "$_agmsg_ci_sql" + agmsg_sqlite ':memory:' < "$_agmsg_ci_sql" ) _rc=$? set -e diff --git a/scripts/drivers/storage/sqlite.sh b/scripts/drivers/storage/sqlite.sh index 47bda31e1..763ee0083 100755 --- a/scripts/drivers/storage/sqlite.sh +++ b/scripts/drivers/storage/sqlite.sh @@ -378,9 +378,36 @@ storage_read_cursor_consume() { WHERE e.type='message_sent' AND e.team='$tl' AND e.id='$(_sqlite_lit "$id")' AND e.legacy_id IS NOT NULL);" done - agmsg_sqlite "$db" "BEGIN IMMEDIATE; - $sql - INSERT OR IGNORE INTO read_cursors(team,agent,local_position) + # #777 ("Not measured" section): $sql gains one INSERT/UPDATE block per + # delivered id, and the whole "BEGIN IMMEDIATE; ...; COMMIT;" statement + # used to be handed to `agmsg_sqlite` as ONE argv element. Measured on + # Windows: 97 ids built a 38,897-byte statement and CreateProcess refused + # it outright (that ceiling is 32,767 characters -- well under Linux's own + # MAX_ARG_STRLEN=131,072 bytes) -- and the failure was masked further, + # surfacing only as this function's ordinary runtime_error/13 return, never + # as a visible "Argument list too long". Same fix as history.sh / + # inbox.sh / check-inbox.sh / watch.sh / watch-once.sh, and + # drivers/storage/sqlite-sync.sh's own #882 fix: write the statement to a + # temp file with printf (a bash builtin, so it never execs) and feed + # `agmsg_sqlite` the statement on stdin instead. + # + # No trap here, on purpose: this is a SHARED LIBRARY FUNCTION, called every + # poll from watch.sh's long-lived loop, which installs its own permanent + # `trap cleanup EXIT` / `trap 'exit 0' INT TERM HUP` once near the top of + # that process. A trap set and then cleared in here (bash traps do not + # stack) would replace watch.sh's for the rest of its life the first time + # this function ever ran -- the exact mistake this same #777 pass caught + # and avoided in watch.sh's own ROWS-fetch fix a few lines above this one + # in the call chain. The temp file is removed explicitly on every path + # instead; the one path that leaks it (a signal landing mid-call) is left + # for the OS's own temp-directory cleanup, same trade-off already accepted + # there. + local sql_file + sql_file=$(mktemp "${TMPDIR:-/tmp}/agmsg-cursor-consume.XXXXXX" 2>/dev/null) || { echo runtime_error; return 13; } + { + printf '%s\n' "BEGIN IMMEDIATE;" + printf '%s\n' "$sql" + printf '%s\n' " INSERT OR IGNORE INTO read_cursors(team,agent,local_position) VALUES('$tl','$al',0); UPDATE read_cursors SET local_position=MAX(local_position,COALESCE(( SELECT MIN(e.seq)-1 FROM events e @@ -390,8 +417,14 @@ storage_read_cursor_consume() { AND NOT EXISTS(SELECT 1 FROM events r WHERE r.type='message_read' AND r.team=e.team AND r.agent='$al' AND r.msg_id=e.id) ),MIN($target,$(_sqlite_highwater)))) - WHERE team='$tl' AND agent='$al'; - COMMIT;" >/dev/null 2>&1 || { echo runtime_error; return 13; } + WHERE team='$tl' AND agent='$al';" + printf '%s\n' "COMMIT;" + } > "$sql_file" + if ! agmsg_sqlite "$db" < "$sql_file" >/dev/null 2>&1; then + rm -f "$sql_file" + echo runtime_error; return 13 + fi + rm -f "$sql_file" echo ok } diff --git a/scripts/drivers/types/codex/watch-once.sh b/scripts/drivers/types/codex/watch-once.sh index 415c2b86b..1c23f9d06 100755 --- a/scripts/drivers/types/codex/watch-once.sh +++ b/scripts/drivers/types/codex/watch-once.sh @@ -127,9 +127,26 @@ while true; do u="$(storage_list_unread "$_team" "$_agent" 2>/dev/null || true)" [ -n "$u" ] || continue uarr="[$(printf '%s' "$u" | paste -sd, -)]" - ids="$(agmsg_sqlite ':memory:' " - SELECT json_extract(value,'\$.id') FROM json_each('$(printf '%s' "$uarr" | sed "s/'/''/g")'); - " 2>/dev/null || true)" + # #777: this pair's unread backlog grows with every message sent to it, + # so interpolating it into ONE argv element eventually exceeds the OS's + # per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 bytes; smaller + # still on Windows/macOS) and `agmsg_sqlite` fails with "Argument list + # too long" -- every single poll, since the backlog that triggered it + # never shrinks on its own (this script never marks anything read; see + # the file header). Pass the statement on stdin instead, mirroring + # drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, #882) and + # history.sh/inbox.sh: printf is a bash builtin, so writing a large + # value to a temp file never execs and can hit neither that ceiling nor + # argv's at all. `|| continue` on mktemp failure matches the existing + # per-pair `continue` a few lines above: one pair's storage error must + # not end the whole subscription's poll. + _agmsg_wo_sql=$(mktemp "${TMPDIR:-/tmp}/agmsg-watchonce-ids.XXXXXX" 2>/dev/null) || continue + trap 'rm -f "$_agmsg_wo_sql"' EXIT HUP INT TERM + printf "%s\n" "SELECT json_extract(value,'\$.id') FROM json_each('$(printf '%s' "$uarr" | sed "s/'/''/g")');" \ + > "$_agmsg_wo_sql" + ids="$(agmsg_sqlite ':memory:' < "$_agmsg_wo_sql" 2>/dev/null || true)" + rm -f "$_agmsg_wo_sql" + trap - EXIT HUP INT TERM [ -n "$ids" ] || continue count=$(( count + $(printf '%s\n' "$ids" | grep -c .) )) all_ids="$all_ids$ids"$'\n' diff --git a/scripts/inbox.sh b/scripts/inbox.sh index c44a12259..8a887bcff 100755 --- a/scripts/inbox.sh +++ b/scripts/inbox.sh @@ -48,13 +48,29 @@ fi # _sqlite_sync_lit_into in sqlite-sync.sh, which documents the same hazard. _AGMSG_SQ="'" _arr="[$(printf '%s' "$UNREAD_JSONL" | paste -sd, -)]" -ROWS=$(agmsg_sqlite ':memory:' " - SELECT json_extract(value,'\$.from') || char(31) || - replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) || - json_extract(value,'\$.at') || char(31) || - json_extract(value,'\$.id') - FROM json_each('${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}'); -") +# #777: an agent's unread backlog grows with every message sent to it, so +# interpolating it into ONE argv element eventually exceeds the OS's +# per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 bytes; smaller still on +# Windows/macOS) and `agmsg_sqlite` fails with "Argument list too long" -- +# every single call, since the backlog that triggered it never shrinks on +# its own. Pass the statement on stdin instead, mirroring +# drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, #882) and +# history.sh: printf is a bash builtin, so writing a large value to a temp +# file never execs and can hit neither that ceiling nor argv's at all. +_agmsg_inbox_sql=$(mktemp "${TMPDIR:-/tmp}/agmsg-inbox-rows.XXXXXX") || exit 13 +trap 'rm -f "$_agmsg_inbox_sql"' EXIT HUP INT TERM +{ + printf "%s\n" "SELECT json_extract(value,'\$.from') || char(31) ||" + printf "%s\n" " replace(replace(json_extract(value,'\$.body'), char(10), '\n'), char(9), '\t') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.at') || char(31) ||" + printf "%s\n" " json_extract(value,'\$.id')" + printf "FROM json_each('" + printf '%s' "${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}" + printf "');\n" +} > "$_agmsg_inbox_sql" +ROWS=$(agmsg_sqlite ':memory:' < "$_agmsg_inbox_sql") +rm -f "$_agmsg_inbox_sql" +trap - EXIT HUP INT TERM COUNT=$(printf '%s\n' "$ROWS" | wc -l | tr -d ' ') echo "$COUNT new message(s):" diff --git a/scripts/lib/storage.sh b/scripts/lib/storage.sh index 6283f22ed..1c5e35b65 100644 --- a/scripts/lib/storage.sh +++ b/scripts/lib/storage.sh @@ -253,9 +253,48 @@ agmsg_sqlite() { _agmsg_sqlite_recording "$@" return fi - # shellcheck disable=SC2086 # intentional split: "-escape off" → two args, or none + # Windows' sqlite3.exe (measured: 3.53.4) ends each row of a multi-row + # result with \r\n, not \n -- confirmed by piping a three-row SELECT + # through `od -c` on real Windows hardware. This is independent of the + # `-escape` probe above (#102/#143: that is sqlite3 >= 3.50's own caret- + # notation rendering, fixed by `-escape off`, and reproduces on Linux too + # -- this CRLF ending does not reproduce here). HYPOTHESIS (unverified): + # the Windows C runtime's stdio text-mode translation rewrites sqlite3's + # own LF terminators to CRLF on the way out; what is actually confirmed is + # only the \r\n on the wire, not this mechanism. + # + # `ROWS=$(agmsg_sqlite ...)` strips only the trailing newline of the WHOLE + # captured output (bash command substitution), so every row but the last + # keeps a \r stuck to its final field -- typically an id, since every + # multi-field row built by this codebase's callers puts id/cursor/at last + # and body earlier (never in scope for this fix, but worth naming: it is + # why this hazard has not already shown up as corrupted message bodies). + # `IFS=$'\x1f' read` does not split on \r, so that \r rides along into + # the field value. Reported and measured on real Windows hardware: a + # 100-message backlog lost 99 of 100 mark-as-read updates in one + # inbox.sh run, because storage_mark_read_batch's ids no longer matched + # any real msg_id. + # + # The fix normalizes ONLY a \r immediately before the line-ending \n -- + # not every \r in the stream. `tr -d '\r'` (used by _sqlite_data / + # _sqlite_data_stdin in drivers/storage/sqlite.sh, wrapping calls to THIS + # function) would also be correct for THIS symptom, but it deletes every + # \r anywhere in the output, including one that is a message body's own + # content (char(13) is not replaced the way char(10) already is in every + # row-building SELECT in this codebase) -- so it is not used here. `sed`'s + # `$` anchor matches only end-of-line, so a \r elsewhere in a row + # (mid-body) is left untouched. + # + # Wrapped in a subshell with its own `set -o pipefail` so the pipeline's + # 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 - sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" || _agmsg_sqlite_rc=$? + ( + 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=$? # 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 @@ -285,16 +324,66 @@ agmsg_sqlite() { # failed, so "the operation failed and the last statement was busy" names it. # # stderr is captured to classify it and re-emitted unchanged, so a caller that -# reads or silences it sees what it saw before; stdout is the data stream and -# is not touched; the exit status is passed through. Written as an `if` so a -# caller running under `set -e` is not exited by the assignment itself. +# reads or silences it sees what it saw before; stdout is the data stream, now +# passed through the same trailing-CR normalization as agmsg_sqlite()'s own +# non-recording path above (Windows' sqlite3.exe row-separator \r\n; see that +# comment for the full writeup -- this path bypasses it entirely via the early +# `return` above, so it needs its own copy of the fix, not a call into it: this +# function's stdout/stderr routing exists for a different purpose, classifying +# ok/busy/failed for the sync driver adapter, and folding the two together +# would tangle two independent concerns). The exit status is still passed +# through, unaffected either way. +# +# The original fd-3 passthrough trick (sqlite3's own fd 1 repointed at +# whatever fd 1 was outside this function, with no process in between) cannot +# survive inserting `sed`: stdout now goes through an actual pipe, so a temp +# file replaces the `err=$(...)` capture for stderr, and the exit status comes +# from `${PIPESTATUS[0]}` (sqlite3's, not sed's) rather than the substitution's +# own `$?`. Stderr is still read back whole and re-emitted verbatim afterward, +# so a caller that reads or silences it sees the same bytes as before. +# +# The pipeline is wrapped in an `if`, same as the original, and for the same +# reason: this is a plain function call, not a subshell, so it runs in the +# CALLING script's own shell -- and several callers set both `-e` and +# `-o pipefail`. A command tested by `if` is exempt from `set -e` on a +# non-zero exit (POSIX), so the pipeline cannot abort the caller here +# regardless of its pipefail setting. +# +# `${PIPESTATUS[0]}` (sqlite3's exit status, not sed's) is read in BOTH +# branches, not once after the `if` -- and specifically not guarded with +# `|| true` the way the CRLF fix above is, because `|| true` is not safe +# here. `PIPESTATUS` is overwritten by the NEXT command this shell +# executes, of any kind, including a trivial one: `pipeline || true` runs +# `true` whenever the pipeline's own exit status is non-zero, and reading +# `${PIPESTATUS[0]}` after that reads back `true`'s status (0), not +# sqlite3's. The CRLF fix's own `|| true` above is fine BECAUSE that call +# site never reads PIPESTATUS at all. This one silently turned every +# failure here into rc=0 whenever pipefail was already active in the +# caller -- and only there: storage-sync-driver.sh sets `-o pipefail` +# itself, so a plain `bash -c` probe without it stayed green while the +# real busy-timeout contract test (test_remote_sync.bats, "a store +# another writer holds is busy") got 0 where it expected 11. Reading +# PIPESTATUS inside the `if`'s own branches, before anything else runs, +# is what keeps it correct either way. _agmsg_sqlite_recording() { - local err rc + local err rc errfile + # A mktemp failure degrades stderr capture to /dev/null rather than failing + # the operation outright: worse diagnostics (an unclassifiable error reads + # as "failed", never as "busy"), not worse correctness, and the same + # "environment problem, not a bad input" class of failure the busy/failed + # distinction exists to tell apart from an ordinary refusal. + errfile=$(mktemp "${TMPDIR:-/tmp}/agmsg-sqlite-recording-err.XXXXXX" 2>/dev/null) || errfile=/dev/null # shellcheck disable=SC2086 # same intentional split as above - if { err=$(sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" 2>&1 >&3 3>&-); } 3>&1; then - rc=0 + if sqlite3 $_AGMSG_ESCAPE_FLAG -cmd ".timeout ${AGMSG_BUSY_TIMEOUT:-5000}" "$@" 2>"$errfile" | sed $'s/\r$//'; then + rc=${PIPESTATUS[0]} + else + rc=${PIPESTATUS[0]} + fi + if [ "$errfile" = /dev/null ]; then + err="" else - rc=$? + err="$(cat "$errfile" 2>/dev/null)" + rm -f "$errfile" fi [ -z "$err" ] || printf '%s\n' "$err" >&2 if [ "$rc" -eq 0 ]; then diff --git a/scripts/watch.sh b/scripts/watch.sh index a3c59f484..00d633e15 100755 --- a/scripts/watch.sh +++ b/scripts/watch.sh @@ -698,17 +698,50 @@ while true; do # _sqlite_sync_lit_into in sqlite-sync.sh, which documents the same hazard. _AGMSG_SQ="'" _arr="[$(printf '%s' "$OUT" | paste -sd, -)]" - ROWS="$(agmsg_sqlite ':memory:' " - SELECT COALESCE(json_extract(value,'\$.type'),'') || char(31) || - COALESCE(json_extract(value,'\$.id'),'') || char(31) || - COALESCE(json_extract(value,'\$.at'),'') || char(31) || - COALESCE(json_extract(value,'\$.team'),'') || char(31) || - COALESCE(json_extract(value,'\$.from'),'') || char(31) || - COALESCE(json_extract(value,'\$.to'),'') || char(31) || - replace(replace(replace(COALESCE(json_extract(value,'\$.body'),''), char(13), ''), char(10), '\\n'), char(9), '\t') || char(31) || - COALESCE(json_extract(value,'\$.cursor'),'') - FROM json_each('${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}'); - " 2>/dev/null || true)" + # #777: this pair's undelivered backlog grows independently of anything + # this loop bounds, so interpolating it into ONE argv element eventually + # exceeds the OS's per-argument ceiling (Linux MAX_ARG_STRLEN=131,072 + # bytes; smaller still on Windows/macOS) and `agmsg_sqlite` fails with + # "Argument list too long" -- every single poll, because the failure + # below was already swallowed by `|| true` and the read cursor is only + # advanced from FINAL_CURSOR/DELIVERED_IDS further down, so a silently + # empty ROWS here left the cursor stuck forever, repeating the same + # failure on every future poll. Pass the statement on stdin instead, + # mirroring drivers/storage/sqlite-sync.sh:1301 (`_sqlite_data_stdin`, + # #882) and history.sh/inbox.sh: printf is a bash builtin, so writing a + # large value to a temp file never execs and can hit neither that ceiling + # nor argv's at all. + # + # No trap here: this script installs `trap cleanup EXIT` and + # `trap 'exit 0' INT TERM HUP` once, near the top (bash traps do not + # stack -- the last one set wins), and this runs inside that same + # process's long-lived polling loop, once per pair per interval. Adding a + # loop-local trap here would silently replace those for the rest of the + # process's life. The temp file is removed explicitly on every path + # instead; the one path that leaks it (a signal landing between mktemp + # and the following rm) is caught by the pre-existing INT/TERM/HUP + # handler tearing down the whole process, same as any other in-flight + # work here. + _agmsg_watch_sql="$(mktemp "${TMPDIR:-/tmp}/agmsg-watch-rows.XXXXXX" 2>/dev/null || true)" + if [ -n "$_agmsg_watch_sql" ]; then + { + printf "%s\n" "SELECT COALESCE(json_extract(value,'\$.type'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.id'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.at'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.team'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.from'),'') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.to'),'') || char(31) ||" + printf "%s\n" " replace(replace(replace(COALESCE(json_extract(value,'\$.body'),''), char(13), ''), char(10), '\\n'), char(9), '\t') || char(31) ||" + printf "%s\n" " COALESCE(json_extract(value,'\$.cursor'),'')" + printf "FROM json_each('" + printf '%s' "${_arr//$_AGMSG_SQ/$_AGMSG_SQ$_AGMSG_SQ}" + printf "');\n" + } > "$_agmsg_watch_sql" + ROWS="$(agmsg_sqlite ':memory:' < "$_agmsg_watch_sql" 2>/dev/null || true)" + rm -f "$_agmsg_watch_sql" + else + ROWS="" + fi FINAL_CURSOR="" DELIVERED_IDS=() diff --git a/tests/test_helper.bash b/tests/test_helper.bash index a9c481ab6..38916b858 100644 --- a/tests/test_helper.bash +++ b/tests/test_helper.bash @@ -405,3 +405,31 @@ spawn_decoy_with_cmdline() { bash "$decoy" "$path" 3>&- & DECOY_PID=$! } + +# Sends messages of ~ bytes each from to on +# , via storage_send directly rather than send.sh's own CLI (#777 +# argv-length regressions in inbox.sh/check-inbox.sh/watch.sh/watch-once.sh). +# +# A plain bash FUNCTION CALL, not a subprocess: `storage_send "$team" ... +# "$body"` hands the body to sqlite3 through the same escaped-argv path +# production code uses for a single INSERT (which is not itself in scope -- +# no test here builds a body anywhere near that ceiling), but building the +# backlog this way never has to exec anything with the WHOLE backlog as one +# argument, which is exactly the shape production code used to get wrong +# on read. Bodies are tagged "$label-$i-" so a caller can assert both +# ends of the run (index 0 and count-1) are actually present in what the +# script under test displayed, not just that its exit status was 0. +bulk_send_direct() { + local team="$1" from="$2" to="$3" count="$4" bodylen="$5" label="$6" \ + i=0 pad + pad="$(head -c "$bodylen" /dev/zero | tr '\0' 'x')" + ( + # shellcheck disable=SC1090 + source "$SCRIPTS/lib/storage.sh" + agmsg_storage_load + while [ "$i" -lt "$count" ]; do + storage_send "$team" "$from" "$to" "${label}-${i}-${pad}" >/dev/null + i=$((i + 1)) + done + ) +} diff --git a/tests/test_inbox.bats b/tests/test_inbox.bats index ede115c6d..ecf3f7d22 100644 --- a/tests/test_inbox.bats +++ b/tests/test_inbox.bats @@ -409,3 +409,42 @@ _codex_proj() { grep -q 'additive' <<<"$output" [ "$(pair_unread_count ctm alice)" -eq 0 ] } + +# --- argv-length regression (#777) --------------------------------------- +# +# Both inbox.sh and check-inbox.sh used to embed the whole unread backlog into +# ONE argv element for `sqlite3 ':memory:' ""`. 100 messages of +# ~2000 bytes each is about 200,000 bytes of body alone, well past Linux's +# MAX_ARG_STRLEN (131,072 bytes -- measured directly in this same suite's +# environment, and documented in scripts/history.sh; the ceiling is smaller +# still on Windows: 32,767 characters). Before the fix this failed every +# single run with "Argument list too long" -- the backlog that triggered it +# never shrinks on its own, so it never recovered. + +@test "inbox: a backlog large enough to exceed the OS argv ceiling still displays and marks read (#777)" { + bulk_send_direct testteam bob alice 100 2000 BIG + + run bash "$SCRIPTS/inbox.sh" testteam alice + [ "$status" -eq 0 ] + grep -qF -- "100 new message(s):" <<< "$output" + grep -qF -- "BIG-0-" <<< "$output" + grep -qF -- "BIG-99-" <<< "$output" + [ "$(unread_count alice)" -eq 0 ] +} + +@test "check-inbox: a backlog large enough to exceed the OS argv ceiling still delivers and marks read (#777)" { + bulk_send_direct testteam bob alice 100 2000 CIBIG + + # Not delivered_to_operator() here: that helper embeds the WHOLE payload + # into its own single-shot json_valid('$esc') probe (an sqlite3 argv + # element again, just on the test side), so a body this size would trip + # the identical #777 ceiling one layer up and fail for a reason that has + # nothing to do with check-inbox.sh. Reading raw stdout directly, the way + # "multiple identities poll only the first agent's exact team rows" above + # already does, keeps this test pinned on the script under test. + run bash -c "echo '{}' | bash '$SCRIPTS/check-inbox.sh' claude-code /tmp/project-a" + [ "$status" -eq 0 ] + grep -qF -- "CIBIG-0-" <<< "$output" + grep -qF -- "CIBIG-99-" <<< "$output" + [ "$(unread_count alice)" -eq 0 ] +} diff --git a/tests/test_sqlite_crlf.bats b/tests/test_sqlite_crlf.bats new file mode 100644 index 000000000..ca3ec9c39 --- /dev/null +++ b/tests/test_sqlite_crlf.bats @@ -0,0 +1,258 @@ +#!/usr/bin/env bats + +# Regression coverage for a Windows-only sqlite3.exe behavior that this +# machine (Linux) cannot reproduce on its own: multi-row SELECT output +# separates rows with \r\n, not \n. Measured on real Windows hardware +# (sqlite3.exe 3.53.4) and reported against inbox.sh's mark-as-read step -- +# see scripts/lib/storage.sh's agmsg_sqlite() for the full writeup. This +# file exercises the fix with a PATH-shimmed sqlite3 stub that reproduces +# the \r\n row separator deterministically on Linux, the same "wrapper +# script ahead of the real binary on PATH" technique test_watch_once.bats +# already uses for its slow-awk shim. +# +# No issue number yet -- this was found independently of #777 while +# verifying #777's own fix on Windows hardware, and has not been filed +# upstream. + +load test_helper + +setup() { + setup_test_env +} + +teardown() { + teardown_test_env +} + +# A wrapper named `sqlite3`, placed ahead of the real one on PATH, that +# behaves exactly like the real binary except every line of its stdout +# gets a synthetic \r appended right before the \n -- simulating the \r\n +# Windows' sqlite3.exe emits at each row boundary. +# +# The real binary's path is resolved HERE, before this directory is ever +# prepended to PATH, and baked into the wrapper as a literal exec target. +# Mirrors test_watch_once.bats's `_slow_startup_path` awk shim exactly for +# this reason: a lookup done INSIDE the wrapper, after PATH already +# includes this directory, would resolve back to the wrapper itself. +_stub_sqlite3_crlf() { + local dir="$BATS_TEST_TMPDIR/crlfbin" real + real="$(command -v sqlite3)" + mkdir -p "$dir" + cat > "$dir/sqlite3" </dev/null + bash "$SCRIPTS/join.sh" crlfteam bob claude-code /tmp/project-crlf >/dev/null + local n + for n in $(seq 1 20); do + bash "$SCRIPTS/send.sh" crlfteam bob alice "CRLF-$n" >/dev/null + done + + PATH="$stub:$PATH" + run bash "$SCRIPTS/inbox.sh" crlfteam alice + [ "$status" -eq 0 ] + grep -qF -- "20 new message(s):" <<< "$output" + for n in $(seq 1 20); do + [[ "$output" == *"CRLF-$n"* ]] + done + + # The real symptom: with the pre-fix agmsg_sqlite, only the LAST id in a + # multi-row unread scan kept a clean (unmangled) trailing field, so all + # but one mark-as-read update silently matched no real msg_id and every + # other message stayed unread. Fixed, the whole backlog clears. + local left + # `grep -c .` exits 1 when the count is 0 (no matching lines) -- correct + # and expected here, but bats runs test bodies under `set -e`, so without + # `|| true` that exit status would abort the test right at this + # assignment before the assertion below ever ran. + left="$(bash -c ' + source "'"$SCRIPTS"'/lib/storage.sh" + agmsg_storage_load + storage_list_unread crlfteam alice + ' | grep -c . || true)" + [ "$left" -eq 0 ] +} + +# --- AGMSG_SQLITE_OUTCOME_FILE (recording) path ------------------------- +# +# agmsg_sqlite() takes a completely different branch when +# AGMSG_SQLITE_OUTCOME_FILE is set (_agmsg_sqlite_recording -- only the +# sync driver adapter sets this, scripts/internal/storage-sync-driver.sh), +# so the fix above does not automatically cover it: it needs, and got, its +# own copy of the same trailing-CR normalization. A codex review of the +# first version of this fix caught the gap. Reproduces via the same +# _stub_sqlite3_crlf, plus a second, fully synthetic stub +# (_stub_sqlite3_fixed) for deterministically exercising the ok/busy/failed +# classification without depending on real SQLITE_BUSY lock-contention +# timing. + +@test "agmsg_sqlite (recording path): strips only the trailing CR, same as the non-recording path" { + local stub outfile + stub="$(_stub_sqlite3_crlf)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'SELECT 1; SELECT 2; SELECT 3;'" + [ "$status" -eq 0 ] + [ "$output" = $'1\n2\n3' ] + [ "$(cat "$outfile")" = ok ] +} + +@test "agmsg_sqlite (recording path): preserves a genuine mid-body CR under the same CRLF stub" { + local stub outfile + stub="$(_stub_sqlite3_crlf)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' \"SELECT 'x' || char(31) || 'a' || char(13) || 'b' || char(31) || 'id123';\"" + [ "$status" -eq 0 ] + [ "$output" = $'x\x1fa\rb\x1fid123' ] + [ "$(cat "$outfile")" = ok ] +} + +# A stub that ignores the real database entirely and just emits FIXED +# stdout/stderr/exit-code content read back from two plain files -- for +# testing _agmsg_sqlite_recording's own ok/busy/failed classification and +# exit-status passthrough in isolation from any real SQL execution or lock +# timing. The desired bytes are written to files by the CALLER (ordinary +# $'...' quoting there, no heredoc-embedding hazards) rather than baked into +# the generated script's own text. +_stub_sqlite3_fixed() { + local dir="$BATS_TEST_TMPDIR/fixedbin" stdout_file="$1" stderr_file="$2" exitcode="$3" + mkdir -p "$dir" + cat > "$dir/sqlite3" <&2 +exit $exitcode +EOF + chmod +x "$dir/sqlite3" + printf '%s' "$dir" +} + +@test "agmsg_sqlite (recording path): ok classification, outcome file, and exit code are unchanged" { + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf 'row1\nrow2\n' > "$stdout_file" + printf '' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 0)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 0 ] + [ "$output" = $'row1\nrow2' ] + [ "$(cat "$outfile")" = ok ] +} + +@test "agmsg_sqlite (recording path): busy classification, outcome file, exit code, and verbatim stderr are unchanged" { + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf '' > "$stdout_file" + printf 'Error: database is locked\n' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 5)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + # --separate-stderr (bats-core, same idiom test_remote_sync.bats already + # uses) so stdout and stderr can be asserted apart -- the sed fix touches + # stdout only, so the classification text must arrive on $stderr + # byte-for-byte, not just be classified correctly. + run --separate-stderr bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 5 ] + [ "$(cat "$outfile")" = busy ] + [[ "$stderr" == *"Error: database is locked"* ]] +} + +@test "agmsg_sqlite (recording path): failed classification, outcome file, exit code, and verbatim stderr are unchanged" { + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf '' > "$stdout_file" + printf 'Error: near "not": syntax error\n' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 1)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run --separate-stderr bash -c "source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 1 ] + [ "$(cat "$outfile")" = failed ] + [[ "$stderr" == *"syntax error"* ]] +} + +@test "agmsg_sqlite (recording path): busy classification survives a caller with -e/pipefail already on (storage-sync-driver.sh's own setting)" { + # _agmsg_sqlite_recording is a plain function call, not a subshell -- it + # runs IN the calling script's own shell, inheriting whatever `set -e` / + # `set -o pipefail` that shell already has. storage-sync-driver.sh, the + # ONLY real caller that ever sets AGMSG_SQLITE_OUTCOME_FILE, has + # `set -euo pipefail` at its own top, so this is the actual condition in + # production, not a hypothetical. + # + # A prior version of this fix guarded the pipeline with `pipeline || + # true` to keep a pipefail-inheriting caller's `set -e` from aborting + # right there. That guard is safe on its own, but this call site ALSO + # reads `${PIPESTATUS[0]}` afterward -- and PIPESTATUS is overwritten by + # the very next command this shell runs, of any kind. With pipefail on, + # the pipeline's own exit status became sqlite3's non-zero one, which is + # exactly when `|| true` runs `true` -- so `${PIPESTATUS[0]}` was read + # back as `true`'s (0), not sqlite3's, and every busy/failed call quietly + # became "ok". Only a caller with pipefail already on triggers `|| true` + # in the first place, which is why the tests above -- run from a plain + # `bash -c` with no pipefail -- stayed green through this: they never + # replicated the one caller shape that actually breaks it. Caught for + # real by test_remote_sync.bats's busy-timeout contract test, which does + # go through the real adapter and hence its `-o pipefail`. + local stub outfile stdout_file stderr_file + stdout_file="$BATS_TEST_TMPDIR/out.txt"; stderr_file="$BATS_TEST_TMPDIR/err.txt" + printf '' > "$stdout_file" + printf 'Error: database is locked\n' > "$stderr_file" + stub="$(_stub_sqlite3_fixed "$stdout_file" "$stderr_file" 5)" + outfile="$BATS_TEST_TMPDIR/outcome" + PATH="$stub:$PATH" + run --separate-stderr bash -c "set -euo pipefail; source '$SCRIPTS/lib/storage.sh'; AGMSG_SQLITE_OUTCOME_FILE='$outfile' agmsg_sqlite ':memory:' 'irrelevant, the stub ignores it;'" + [ "$status" -eq 5 ] + [ "$(cat "$outfile")" = busy ] + [[ "$stderr" == *"Error: database is locked"* ]] +} diff --git a/tests/test_watch.bats b/tests/test_watch.bats index b50f830ab..b4168caa2 100644 --- a/tests/test_watch.bats +++ b/tests/test_watch.bats @@ -944,6 +944,53 @@ _record_handover_events() { done } +# --- argv-length regression (#777) -------------------------------------- +# +# watch.sh used to embed the whole page of `storage_watch_after` rows into +# ONE argv element for `sqlite3 ':memory:' ""`, and its failure +# was swallowed by a trailing `2>/dev/null || true` -- so ROWS silently +# became empty, FINAL_CURSOR never got set, and the read cursor never +# advanced. The same backlog would then fail identically on every following +# poll: not a one-off skip, a stall. +# +# 100 messages of ~2000 bytes each is about 200,000 bytes of body alone, +# well past Linux's MAX_ARG_STRLEN (131,072 bytes; smaller still on +# Windows/macOS). Sent BEFORE the watcher starts, so its very first poll has +# to scan and embed the entire backlog in one statement -- the shape the bug +# needed, rather than many small pages that would each stay under the +# ceiling on their own. +@test "watch: a backlog large enough to exceed the OS argv ceiling still delivers and advances the cursor (#777)" { + skip_on_windows "watcher background launch under Git Bash (#182)" + local sid="sess-argv-backlog" + local out="$TEST_SKILL_DIR/argv-backlog.log" + + bulk_send_direct team bob alice 100 2000 WBIG + + AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "$sid" "$PROJ" claude-code >"$out" 2>/dev/null 3>&- 4>&- & + local w=$! + _wait_for_file_contains "$out" "WBIG-99-" || { kill "$w" 2>/dev/null || true; false; } + + # Cursor advancement is a SEPARATE step that runs after every row in this + # poll has already been printed (storage_read_cursor_consume, embedding all + # 100 delivered ids in its own statement) -- killing the watcher the instant + # the last line lands, the way the plain burst test (#245) does, races that + # step under this much data. Poll for it instead, same as "watch: restart + # delivers messages that arrived while the watcher was down" above. + local i cursor + for i in $(seq 1 100); do + cursor=$(_read_cursor team alice 2>/dev/null || echo 0) + [ "${cursor:-0}" -gt 0 ] && break + sleep 0.1 + done + kill "$w" 2>/dev/null || true + wait "$w" 2>/dev/null || true + + grep -q "WBIG-0-" "$out" + grep -q "WBIG-99-" "$out" + # Not stuck: the store-owned cursor moved past where it started (0). + [ "${cursor:-0}" -gt 0 ] +} + @test "watch: empty session_id gets a generated fallback instead of a Usage error (#236)" { local out="$BATS_TEST_TMPDIR/empty-sid.out" AGMSG_WATCH_INTERVAL=1 bash "$SCRIPTS/watch.sh" "" "$PROJ" claude-code alice >"$out" 2>&1 3>&- 4>&- & diff --git a/tests/test_watch_once.bats b/tests/test_watch_once.bats index 2ce8a9532..bba75cb8f 100644 --- a/tests/test_watch_once.bats +++ b/tests/test_watch_once.bats @@ -105,6 +105,27 @@ _assert_startup_was_delayed() { [[ "$output" =~ "hello pending" ]] } +# --- argv-length regression (#777) -------------------------------------- +# +# This pair's unread ids used to be embedded into ONE argv element for +# `sqlite3 ':memory:' ""`, with the failure swallowed by a +# trailing `2>/dev/null || true` -- so `ids` silently became empty and the +# `[ -n "$ids" ] || continue` a few lines later skipped the whole team every +# single poll, never marking anything read (this script never does) and +# never reporting it pending either. +# +# 100 messages of ~2000 bytes each is about 200,000 bytes of body alone, +# well past Linux's MAX_ARG_STRLEN (131,072 bytes; smaller still on +# Windows/macOS). +@test "watch-once: a backlog large enough to exceed the OS argv ceiling still reports pending (#777)" { + bulk_send_direct team bob alice 100 2000 WOBIG + + run bash "$TYPES/codex/watch-once.sh" "$PROJ" codex --name alice --team team --timeout 2 --interval 1 + [ "$status" -eq 0 ] + grep -qF -- "status=pending" <<< "$output" + [[ "$output" =~ "count=100" ]] +} + @test "watch-once: ignores messages already read by inbox.sh" { bash "$SCRIPTS/send.sh" team bob alice "read already" >/dev/null bash "$SCRIPTS/inbox.sh" team alice >/dev/null