Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions scripts/check-inbox.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
43 changes: 38 additions & 5 deletions scripts/drivers/storage/sqlite.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}

Expand Down
23 changes: 20 additions & 3 deletions scripts/drivers/types/codex/watch-once.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
30 changes: 23 additions & 7 deletions scripts/inbox.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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):"
Expand Down
107 changes: 98 additions & 9 deletions scripts/lib/storage.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
55 changes: 44 additions & 11 deletions scripts/watch.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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=()
Expand Down
Loading
Loading