sync: /save answered 502 because Turso will not parse a bare HAVING - #385
Merged
Conversation
`/save` failed with 502 every time unless you passed --force. The cause is one missing clause, and it survived because it is invisible to the test suite. insertRevision() builds two statements. The --force path sends ifRevision: null and inserts unconditionally. The ordinary path carries a precondition and expressed it as a HAVING on an implicit single-group aggregate: INSERT INTO settings_snapshots (…) SELECT … FROM settings_snapshots WHERE user_id = ? HAVING COALESCE(MAX(revision),0) = ? SQLite treats the whole result as one group and runs that happily. Turso's parser rejects it outright: SQL string could not be parsed: near HAVING, "None": syntax error The route threw, the platform returned 502, and the CLI reported exactly what it saw. #380 added retries for "the app does not answer", which could never help: the statement is deterministically unparseable, so every attempt failed the same way. The tests could not catch it. They run against `file:` — a different engine from the deployment — and test/settings-sync.mjs does cover the precondition, with a stale ifRevision refused as 409 and a current one accepted. It passed throughout, because on SQLite the statement is valid. Adding GROUP BY user_id makes it parse on both. It cannot change the answer: the caller sets ifRevision to null when the account has no current revision, so there is always at least one row for this user to group. Verified against the real database rather than only locally — the bare form fails to parse on Turso, the grouped form parses and inserts nothing when the precondition does not hold, which is the conflict the caller reports as 409. test/sql-portability.test.mjs now fails on any HAVING in src/ with no GROUP BY. Static rather than behavioural on purpose: the behaviour is correct on the engine the tests use, so only reading the SQL can catch this. It matches the uppercase keyword with comments stripped, because "having" is also an English word and four files that contain no SQL said it in prose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan97 finding(s) HIGH/CRITICAL: 5 | MEDIUM: 41 | LOW: 51
…and 47 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
/savereturned 502 every time unless you passed--force. One missing clause, invisible to the test suite.What was happening
insertRevision()builds two statements.--forcesendsifRevision: nulland inserts unconditionally — that path always worked. The ordinary path carries a precondition, written as aHAVINGon an implicit single-group aggregate:SQLite treats the whole result as one group and runs it. Turso refuses to parse it:
The route threw, Railway returned 502, and the CLI reported what it saw.
Reproduced from the CLI:
moshcode savecould not save: the app returned 502moshcode save --forcesaved 2 files … (revision 5)#380 added retries for "the app does not answer". They could never help — the statement is deterministically unparseable, so every attempt failed identically.
Why the tests missed it
They run against
file:, a different engine from the deployment. Andtest/settings-sync.test.mjsdoes cover this path — staleifRevision→ 409, current → 200. It passed the whole time, because on SQLite the statement is valid.That is the part worth fixing beyond the one line.
The fix
GROUP BY user_idmakes it parse on both engines. It cannot change the answer: the caller setsifRevisiontonullwhen the account has no current revision (if (!current) ifRevision = null), so there is always at least one row for this user to group.Verified against the real database, not just locally:
syntax erroron TursoThe guard
test/sql-portability.test.mjsfails on anyHAVINGinsrc/without aGROUP BY. Static rather than behavioural on purpose — the behaviour is correct on the engine the tests use, so only reading the SQL can catch this class.It matches the uppercase keyword with comments stripped: "having" is an ordinary English word, and my first version flagged four files that contain no SQL at all because their comments said "rather than having…". Confirmed to fail when the
GROUP BYis removed and pass when it is present.Verification
pnpm install --ignore-workspace; they are not installed by a root install, which is why this suite does not run in CI).🤖 Generated with Claude Code