fix(api): pass connected set as array literal in sync-presence#4483
Conversation
The reconciler was passing the JS string array straight into the
drizzle sql template, which expanded it as N positional params:
`ANY(($1, $2, ..., $57)::text[])`. That's a row-cast, not an array
construction, so every QStash tick was 502ing with "Reconcile write
failed" and no rows ever flipped. Verified in prod via vercel logs
showing the exact expanded SQL.
Fix: build a Postgres array literal (`{a,b,c}`) and pass it as a
single text parameter. Routing keys are `${uuid}:${32-char-hex}`
which never contain literal `,`, `{`, `}`, `"`, or `\`, so the
unquoted form is safe.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesPresence Sync Array Literal
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Greptile SummaryThis PR fixes the reconciler's 502 errors by replacing a broken Drizzle array interpolation with a single Postgres array-literal parameter. The root cause was that
Confidence Score: 4/5The fix correctly resolves the production 502 and is safe to merge; the only open question is whether Redis key format is enforced upstream. The change is small and targeted, directly addressing a confirmed production breakage. The array-literal approach is a standard Postgres workaround for parameterized array inputs and behaves correctly under Drizzle's binding model. The one gap is that there is no in-code assertion that Redis values match the uuid:hex format before they are embedded in the literal — a format drift in directory.ts would produce a quiet 502 rather than a clear validation error. That is a hardening concern, not a blocker. The single changed file, apps/api/src/app/api/hosts/jobs/sync-presence/route.ts, deserves a second look around line 79 where the array literal is constructed without validating element format.
|
| Filename | Overview |
|---|---|
| apps/api/src/app/api/hosts/jobs/sync-presence/route.ts | Fixes the Drizzle array-expansion bug by building a Postgres array literal and passing it as a single parameter; the fix is correct and not SQL-injectable, though it adds an implicit dependency on Redis key format that is not validated in code. |
Sequence Diagram
sequenceDiagram
participant QStash
participant Route as sync-presence route
participant Redis
participant Postgres as Postgres (Neon)
QStash->>Route: POST (signed)
Route->>Route: verify QStash signature
Route->>Redis: ZRANGEBYSCORE relay:tunnel-ttl [now, +inf]
Redis-->>Route: connected string[]
alt "connected.length === 0"
Route-->>QStash: "200 { skipped: true }"
else "connected.length > 0"
Route->>Route: "build connectedArrayLiteral = "{a,b,c}""
Note over Route,Postgres: Before fix: drizzle expanded array into ($1,$2,...)::text[] (row-cast, invalid). After fix: passes literal as single $1
Route->>Postgres: UPDATE v2_hosts ... ANY($1::text[])
Postgres-->>Route: RETURNING rows with new is_online
Route-->>QStash: "200 { connected, flippedOn, flippedOff }"
end
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/api/src/app/api/hosts/jobs/sync-presence/route.ts:79
**Unvalidated Redis values flow into Postgres array literal**
The fix is not a SQL-injection risk — Drizzle parameterizes `${connectedArrayLiteral}` as a single `$1` binding — but it creates a silent failure path: if any value returned by `redis.zrange` ever contains `{`, `}`, `,`, or whitespace (e.g., after a key-format change in `directory.ts` or due to Redis data corruption), Postgres will reject the `$1::text[]` cast and the reconciler will 502. Because the guard at line 63 only checks length, not shape, a single malformed key silences the whole run. Adding a format check before building the literal would make that failure mode explicit and diagnosable.
Reviews (1): Last reviewed commit: "fix(api): pass connected set as array li..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // rather than letting drizzle expand the JS array into N placeholders | ||
| // (`($1, $2, ...)::text[]` is a row-cast, not an array). Routing keys are | ||
| // `${uuid}:${32-char-hex}` so the unquoted `{a,b,c}` literal is safe. | ||
| const connectedArrayLiteral = `{${connected.join(",")}}`; |
There was a problem hiding this comment.
The fix is not a SQL-injection risk — Drizzle parameterizes ${connectedArrayLiteral} as a single $1 binding — but it creates a silent failure path: if any value returned by redis.zrange ever contains {, }, ,, or whitespace (e.g., after a key-format change in directory.ts or due to Redis data corruption), Postgres will reject the $1::text[] cast and the reconciler will 502. Because the guard at line 63 only checks length, not shape, a single malformed key silences the whole run. Adding a format check before building the literal would make that failure mode explicit and diagnosable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/api/src/app/api/hosts/jobs/sync-presence/route.ts
Line: 79
Comment:
**Unvalidated Redis values flow into Postgres array literal**
The fix is not a SQL-injection risk — Drizzle parameterizes `${connectedArrayLiteral}` as a single `$1` binding — but it creates a silent failure path: if any value returned by `redis.zrange` ever contains `{`, `}`, `,`, or whitespace (e.g., after a key-format change in `directory.ts` or due to Redis data corruption), Postgres will reject the `$1::text[]` cast and the reconciler will 502. Because the guard at line 63 only checks length, not shape, a single malformed key silences the whole run. Adding a format check before building the literal would make that failure mode explicit and diagnosable.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
The reconciler from #4476 has been 502-ing every QStash tick since merge. Verified via Vercel logs — the actual error:
```
Error: Failed query: ... ANY(($1, $2, $3, ..., $57)::text[])
```
Drizzle's `sql` template was expanding the JS `connected: string[]` as N positional placeholders, producing a Postgres row-constructor cast to text[] rather than an array. That's not valid SQL syntax for membership testing.
Fix
Build a single Postgres array literal (`{a,b,c}`) from the connected set and pass it as one text parameter. The routing keys are `${uuid}:${32-char-hex}` — they contain only `[0-9a-f-]` plus a single `:`, no characters that would conflict with the unquoted array literal format, so no per-element quoting needed.
Test plan
Notes
Summary by cubic
Fix the sync-presence job by passing the connected set as a single Postgres array literal (
{a,b,c}) instead of lettingdrizzleexpand the JS array into N placeholders that became a row-cast and caused 502s. Membership checks now work, so hosts flip online/offline as expected.Written for commit 4f515d2. Summary will update on new commits.
Summary by CodeRabbit