fix(history): pass the history and unread-marker SQL on stdin (#777) - #899
Conversation
…e#777) history.sh builds two statements by interpolating a JSON array into the SQL text and passing the whole string as one argv element: :41 the display slice, via json_each('<HIST_JSONL as a JSON array>') :64 the recipient's unread backlog, via json_each('<unread list as a JSON array>') Both eventually exceed the per-argument length ceiling. Measured on Linux, not only Windows: the limit that fails is MAX_ARG_STRLEN (one argument), which is 32 * PAGE_SIZE = 131,072 here -- NOT ARG_MAX (argv+envp total), which is 2,097,152 and is never approached. Boundary measured with a plain external binary: 131,071 OK, 131,072 E2BIG. On a live team the unread statement measures 125,945 bytes, i.e. 96% of that ceiling, so one more ordinary message tips it over. Past it, history exits 126 with "sqlite3: Argument list too long" from lib/storage.sh:237. The unread one (:64) bites first and is the more surprising of the two, because it is NOT bounded by the display limit: `history.sh <team> "" 3` fails the same way, since storage_list_unread returns the recipient's whole backlog regardless of how many rows the caller asked to display. From the operator's side, asking for fewer messages does not help at any value. Write each statement to a temp file and feed it to sqlite3 on stdin, which is what drivers/storage/sqlite-sync.sh:1082 already does for the sync-apply batch. printf is a bash builtin, so passing the large value through it does not exec and cannot hit the limit. Temp files are removed via trap on EXIT HUP INT TERM, following the idiom already used in export.sh:90. Verified on a team where the unpatched script exits 126: patched exits 0 and prints the requested rows. Existing bats suites that exercise history.sh (test_messaging, test_storage, test_storage_contract, test_bin_agmsg, test_binding_mode, test_migrate_team_store, test_legacy_mirror) pass: 133 tests, 0 failures.
|
Thank you for this — and for splitting it the way you did. Naming the unread marker as the The code path has no blocking finding. One thing to fix before this lands, and it is only in The comment in the unread branch names the wrong limit. It says the statement "eventually Your PR body and commit message already say this correctly. It is the comment that kept the Worth stating precisely there, because the two limits behave differently and the difference For what it is worth, the same class was just fixed on the sync side and landed in |
The comment recorded ARG_MAX. The measurement in this PR's own body puts the failing statement at 125,945 bytes, which is well inside ARG_MAX (2,097,152 on the machine measured) -- what it exceeds is MAX_ARG_STRLEN, the ceiling on a single argument, 131,072 bytes on Linux. The two are not interchangeable and the difference decides the repair: splitting one long statement into several shorter arguments satisfies MAX_ARG_STRLEN while leaving ARG_MAX untouched. A reader who has the wrong one in mind reaches for the wrong fix. Also worth having in the comment: MAX_ARG_STRLEN has no getconf key, so the limit that bites is the one the tools will not show you. That is why the wrong one is easy to reach for. The body and the commit message already had this right; only the comment kept the earlier reading. Author's commit is unchanged.
|
I pushed the comment correction to your branch rather than asking you to round-trip for one What the added commit changes, and nothing else: It also picks up something worth having in the file: Same on the other platforms, for what it is worth: the Windows ceiling is the documented CI should re-run on the new head. Nothing else from the review is outstanding — the code |
|
Thank you — and please keep your commit on the branch. Your wording is better than mine, and
That is exactly how I got it wrong, twice, before measuring the boundary directly. Having it On the red shard
Run locally on Linux against this branch's head: $ bats tests/test_watch.bats
21 tests, 0 failures (RAW_EXIT=0)The other three macOS shards and every Linux shard passed. So from here it reads as either a Scope, unchangedStill the two |
Fixes the two
history.shcall sites listed in #777 (history.sh:47andhistory.sh:65inthe issue's numbering) by passing the SQL on stdin instead of as one argv element.
Why these two first
They are the pair that makes
historyunusable rather than merely slow, and one of them isnot bounded by anything the operator can control:
:64-65(unread marker) — buildsjson_each('<the recipient's entire unread backlog>'). This set is independent of the display limit, sohistory.sh <team> "" 3fails exactly like
history.sh <team>. From the operator's side, asking for fewer messagesdoes not help at any value.
:41(display slice) — same construction over the displayed rows. Bounded by thelimit, but a single long body can carry it past the ceiling on its own, so it is fixed in
the same commit.
Measured on Linux, not only Windows
This issue is scoped in its title to Windows/Git Bash. It reproduces on Linux, and the
headroom there is much smaller than
getconf ARG_MAXsuggests — the limit that fails is theper-argument one:
ARG_MAX(argv+envp total)getconf ARG_MAXMAX_ARG_STRLEN(one argument)32 * PAGE_SIZE)Boundary, measured with a plain external binary:
On a live team (2,079+ messages, ~31 MB store) the unread statement measures 125,945
bytes — 96% of that ceiling, i.e. about 5 KB of headroom remains. One sufficiently large
message, or a handful of ordinary ones, takes it over, and
historythen stops workingentirely. So on Linux this is reached at roughly 128 KB of
accumulated unread payload for one recipient, which is a normal team where a participant has
not read for a while — not an unusually large one.
Before/after in the same installation, same team:
Approach
Mirrors what
drivers/storage/sqlite-sync.sh:1082already does for the sync-apply batch:write the statement to a temp file, feed it to
sqlite3on stdin.printfis a bashbuiltin, so passing the large value through it does not exec and cannot hit the limit.
Temp files are removed via
trap ... EXIT HUP INT TERM, following the idiom already usedin
export.sh:90, so a failingsqlite3does not leave one behind.No behaviour change otherwise — same SQL, same output, same escaping (
sed "s/'/''/g"iskept and applied identically).
Tests
Existing suites, unmodified, on Linux with bats 1.13.0:
Those are the suites that exercise
history.sh(grep -l history.sh tests/*.bats).Which numbers are measured, which are not
the unpatched-vs-patched exit codes; the bats results.
threshold appears in the issue thread for the pull path; it is not used here.)
--limit ~200idea floated in the issue thread forremote pullis acandidate to test, not a recommendation, and is not part of this change.
Note on the commit message: an earlier revision of this branch attributed the failure to
ARG_MAX. That was wrong and is corrected both in the current commit message and inthe issue thread.
Not covered here
The other five call sites named in #777 (
check-inbox.sh:241,inbox.sh:51,remote.sh:786,watch.sh:704,drivers/types/codex/watch-once.sh:131) and the pull-applysite reported separately in the issue thread (
sqlite-sync.sh:1094-1112) are untouched. Happyto extend this PR to any of them if you would rather land them together — I kept the scope to
the two that were blocking day-to-day
historyuse here.Disclosure
Investigated and drafted by Claude via Claude Code, prepared with the user's authorization,
following the same convention as the rest of #777.