Skip to content

fix(#244,#245,#246): make concurrent + offline message delivery reliable#279

Merged
TortoiseWolfe merged 1 commit into
mainfrom
fix/messaging-reliability-trio
Jul 13, 2026
Merged

fix(#244,#245,#246): make concurrent + offline message delivery reliable#279
TortoiseWolfe merged 1 commit into
mainfrom
fix/messaging-reliability-trio

Conversation

@TortoiseWolfe

Copy link
Copy Markdown
Owner

Summary

Fixes the messaging-reliability trio — #244, #245, #246 — the last three of the 10-bug GDPR/messaging Gap-Audit cluster. This PR closes the whole cluster (prior: #276+#277 security-critical, #278 GDPR trio).

They share one root — a non-atomic sequence-number trigger — and interact, so they ship together. A 3-agent adversarial design review reproduced the race on the live backend (12/30 concurrent inserts failed) and caught several defects in the initial designs, all fixed here.

The three fixes

#244 — Concurrent sends collide and get dropped

The assign_sequence_number() BEFORE-INSERT trigger did a non-atomic SELECT MAX(sequence_number)+1, so two concurrent inserts into one conversation got the same number and the loser hit unique_sequence (23505).

  • A transaction-scoped advisory lock keyed on the conversation (pg_advisory_xact_lock(int4,int4) carved from the UUID) makes MAX+1 atomic per conversation.
  • Verified live: 30 concurrent inserts → 0 collisions, sequence 1..30 gap-free (was 12/30 failing).

#245 — Offline messages delivered multiple times

The offline flush did a plain insert with no dedupe key, so every reconnect re-inserted a fresh row.

  • Add messages.client_generated_id (the stable UUID the client already mints at enqueue) + a plain unique index (NULLs distinct, so live sends coexist; a partial index would break the ON CONFLICT inference — 42P10). Flush becomes upsert(onConflict, ignoreDuplicates) + .maybeSingle(), treating a null-no-error (DO NOTHING) as "already delivered."
  • Verified live: replayed flush → exactly one row; NULL live sends not deduped.

#246 — Offline message lost on a transient conflict

The offline flush treated any error (incl. a transient 23505) as a permanent failure after 5 retries.

  • Give it the live path's discrimination: a 23505 is retried in-loop with a fresh sequence read (rare now that Messages in a busy conversation are dropped under concurrent sends #244 is atomic — the belt-and-suspenders the review required); only a genuinely non-idempotent error defers to the next sync.
  • The online send path is untouched and safe (never sets client_generated_id → stays NULL → excluded from dedupe → .single() never hits a skip).

Verification

🤖 Generated with Claude Code

The three messaging-reliability bugs share one root — a non-atomic
sequence-number trigger — and interact, so they're fixed together. A 3-agent
adversarial design review reproduced the race on the live backend (12/30
concurrent inserts failed) and caught several defects in the initial designs,
all addressed here.

#244 Concurrent sends collide and get dropped: the BEFORE INSERT trigger
assign_sequence_number() did a non-atomic SELECT MAX(sequence_number)+1, so two
concurrent inserts into one conversation computed the same next_seq and the loser
violated unique_sequence (23505). A transaction-scoped advisory lock keyed on the
conversation (pg_advisory_xact_lock(int4,int4) carved from the UUID) makes the
MAX+1 atomic per conversation — the second insert blocks until the first commits,
then reads the correct MAX. Verified live: 30 concurrent inserts → 0 collisions,
sequence 1..30 gap-free (was 12/30 failures).

#245 Offline messages delivered multiple times: the offline flush did a plain
insert with no server-visible dedupe key, so every reconnect (online /
visibility / focus / poll / mount, or a flush that inserted server-side but died
before writing synced:1) re-inserted a fresh row. Add messages.client_generated_id
(the stable UUID the client already mints at enqueue) + a unique index; the flush
becomes upsert(onConflict=client_generated_id, ignoreDuplicates) so a replay is a
no-op. A PLAIN (not partial) unique index — NULLs are distinct, so live sends
that leave it NULL coexist, while a partial index would break the ON CONFLICT
inference (42P10). The insert now uses .maybeSingle() and treats a null-no-error
result (ON CONFLICT DO NOTHING) as "already delivered → mark synced". Verified
live: replayed flush → exactly one row; NULL live sends not deduped.

#246 Offline message lost on a transient conflict: the offline flush wrapped ANY
insertError (including a transient unique_sequence 23505) in a generic error and
burned a retry → after 5 it was permanently 'failed'. Give the flush the live
path's discrimination: a 23505 is retried in-loop with a fresh sequence read
(rare now that #244 is atomic, but the belt-and-suspenders the adversarial review
required); only a genuinely non-idempotent error falls through to the
retry-on-next-sync path. The online send path is untouched and safe — it never
sets client_generated_id (stays NULL, excluded from the dedupe), so its .single()
never hits a DO-NOTHING skip.

Verification: RLS DB-contract tests (tests/rls/messaging-reliability) prove #244
gap-free under real concurrency and #245 exactly-once + NULL coexistence live.
Full 71-test RLS suite + 295 messaging/crypto unit tests green; type-check + lint
clean. Schema applied live via the Management API (idempotent, monolithic
migration); no edge-function changes.

This closes the last of the 10-bug GDPR/messaging Gap-Audit cluster.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants