Skip to content

fix(messaging): stop discarding the user's message when a send fails - #98

Open
TortoiseWolfe wants to merge 3 commits into
mainfrom
fix/send-failure-discards-message
Open

fix(messaging): stop discarding the user's message when a send fails#98
TortoiseWolfe wants to merge 3 commits into
mainfrom
fix/send-failure-discards-message

Conversation

@TortoiseWolfe

Copy link
Copy Markdown
Owner

The last deterministic E2E failure (offline-queue.spec.ts, shard msg-3/4, 100% of runs). It is a real product bug, not a test bug.

A message that fails to send because of a network-layer fault — while the browser still reports navigator.onLine === true — is silently thrown away.

From the failing CI trace (run 30845792628):

[sendMessage] INSERT failed {error: Failed to send message: TypeError: Failed to fetch}

Zero offline-queue-updated events. The failure screenshot shows the thread reading "No messages yet. Send the first one!" with the composer cleared. The user's typed text is gone — not queued, not recoverable, not even an error banner.

Root cause: two sibling predicates disagreed

matched a thrown TypeError matched an error object
isTransientFetchError
isNetworkError

supabase-js never throws on a failed fetch — postgrest-js catches it and returns { error: { message: "TypeError: Failed to fetch", … }, status: 0 }.

So the INSERT gets an error object. isTransientFetchError recognises it and enters the RLS-retry loop; the retry aborts too, and since that error has no .code and isn't an RLS error, the send throws ConnectionError ~2s in. isNetworkError then sees a ConnectionError — not a TypeError — with onLine still true, returns false, and the offline queue, which exists for exactly this case, is never reached. messages/page.tsx:458-460 then deletes the optimistic bubble.

isNetworkError now delegates to isTransientFetchError, so the two cannot drift apart again.

Also fixes a pre-existing browser gap

Both predicates only looked for 'fetch'/'network'/'failed to fetch'. The engines word it differently:

Chromium  "TypeError: Failed to fetch"
Firefox   "TypeError: NetworkError when attempting to fetch resource."
WebKit    "TypeError: Load failed"        ← matched NEITHER

On WebKit isTransientFetchError already returned false today, skipping the retry path entirely. Both branches now share one looksLikeFetchFailure covering all three. Without this the webkit shard would have stayed red.

Test corrections in the same file

  • Renamed from "should show failed status after max retries". There is no failed-status UI — MessageBubble has no status handling, DecryptedMessage has no status field. What it actually asserts is that the message survives.
  • expect(interceptCount).toBeGreaterThanOrEqual(1) was vacuous — the route pattern also catches the GET from the 10s polling refetch, so the counter climbed even when no send occurred. Counts POSTs now.
  • The 35s "wait for retries to exhaust" was fiction. Retries never exhaust on this path; the outcome is settled ~2s after the click. Reduced to 10s, removing ~25s of dead time per run per browser.

Verification

Diagnosis was adversarially reviewed by three independent agents across control-flow, supabase-client and UI/test-validity lenses. All three failed to refute it. Their corrections are incorporated: the throw site is the first inner retry (not retry exhaustion), the WebKit wording gap, and the vacuous assertion.

One reviewer confirmed the chain against the actual CI trace rather than statically — including the 2000 ms gap matching the retry loop's Math.min(2000 * 2^0, 16000) backoff.

Local: type-check clean, lint 0 errors, 388 test files pass.

Follow-up worth doing

postgrest-js sets status: 0 on every fetch-layer failure in every engine — a stronger signal than matching message text. Using it means capturing the status at the INSERT site rather than re-deriving intent from a stringified wrapper two frames later. Not done here because it changes the send path's shape; the string predicate is now centralised in one function so a fourth engine wording is a one-line change.

Refs #66, #76.

🤖 Generated with Claude Code

TortoiseWolfe and others added 3 commits August 3, 2026 17:08
offline-queue.spec.ts's last test failed on 100% of CI runs. It is a real
product bug, not a test bug: a message that fails to send because of a
network-layer fault, while the browser still reports navigator.onLine === true,
is silently thrown away.

From the failing CI trace (run 30845792628): console shows
`[sendMessage] INSERT failed {error: Failed to send message: TypeError: Failed
to fetch}`, there are zero `offline-queue-updated` events, and the screenshot
shows the thread reading "No messages yet. Send the first one!" with the
composer cleared. The user's typed text is gone — not queued, not recoverable,
and without even an error banner.

Root cause: two sibling predicates disagreed.

  isTransientFetchError  matched a thrown TypeError AND an object whose
                         .message names a fetch failure
  isNetworkError         matched ONLY a thrown TypeError

supabase-js never throws on a failed fetch — postgrest-js catches it and
returns `{ error: { message: "TypeError: Failed to fetch", ... }, status: 0 }`
(PostgrestBuilder). So the INSERT gets an error OBJECT; isTransientFetchError
recognises it and enters the RLS-retry loop; the retry aborts too, and because
that error has no `.code` and is not an RLS error the send throws
ConnectionError about two seconds in. isNetworkError then sees a
ConnectionError — not a TypeError — with onLine still true, returns false, and
the offline queue (which exists for exactly this case) is never reached.
messages/page.tsx then deletes the optimistic bubble in its catch.

isNetworkError now delegates to isTransientFetchError so the two cannot drift
apart again.

Also fixes a browser gap that predates this and would have kept webkit red:
both predicates only looked for 'fetch'/'network'/'failed to fetch', but the
three engines word it differently —

  Chromium  "TypeError: Failed to fetch"
  Firefox   "TypeError: NetworkError when attempting to fetch resource."
  WebKit    "TypeError: Load failed"        <- matched NEITHER

so on webkit isTransientFetchError already returned false today, skipping the
retry path entirely. Both branches now share one `looksLikeFetchFailure`
predicate covering all three.

Test corrections in the same file:
- Renamed from "should show failed status after max retries". There is no
  failed-status UI — MessageBubble has no status handling and DecryptedMessage
  has no status field. What it actually asserts is that the message survives.
- `expect(interceptCount).toBeGreaterThanOrEqual(1)` was vacuous: the route
  pattern also catches the GET from the 10s polling refetch, so the counter
  climbed even if the send never happened. Counts POSTs now.
- The 35s "wait for retries to exhaust" was fiction — retries never exhaust on
  this path, the outcome is settled ~2s after the click. Reduced to 10s,
  removing ~25s of dead time per run per browser.

Follow-up worth doing: postgrest-js sets `status: 0` on every fetch-layer
failure in every engine, which is a stronger signal than matching message
text. Using it means capturing the status at the INSERT site rather than
re-deriving intent from a stringified wrapper two frames later.

Diagnosis adversarially verified by three independent reviewers, all of which
failed to refute it; their corrections (throw site, webkit wording, vacuous
assertion) are incorporated above.

Refs #66, #76

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The looksLikeFetchFailure extraction left the original doc comment orphaned
above the new helper, describing the wrong function.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#98 fixed the INSERT path. Investigating whether a better fix had been left on
the table found two more places a send silently destroys the user's text, and
one hole in the fix itself.

## The predicate was correct only by coincidence

Broadening the match to 'fetch'/'network'/'load failed' is safe TODAY only
because no schema identifier happens to contain those words. Nothing enforces
that. A migration adding a `check_network_*` constraint would turn a hard
validation error into queue-and-forget — which shows NO error banner at all,
leaves the bubble looking sent, and spins useOfflineQueue's 3s poll forever
because failed rows never clear queueCount.

Gated on the structural signal instead: a PostgREST/Postgres rejection always
carries a code (SQLSTATE or PGRST***), a fetch failure carries code: '' and
status: 0. Non-empty code now means "the server answered and refused us",
whatever the message says.

## ...and the gate had a hole, found by writing the test for it

The queue decision does not see the raw error — it sees
ConnectionError("Failed to send message: " + original.message), which
concatenates the database's own text into the wrapper and drops its code. So
the gate protected the retry decision and not the queue decision: a constraint
named for a network would still have been queued.

The send path now passes the original as `cause` (ConnectionError already
accepted one), and the predicate follows `cause` so it judges the original
rather than the wrapper's assembled string.

## Two more loss paths

sendMessage nests trys: outer :238, inner :356, queue decision :510. Only
errors raised inside the inner try ever reach the queue. Both of these are
raised before it:

- The conversation lookup reported a network fault as
  ValidationError('Conversation not found') — a confident, wrong diagnosis —
  and the page deleted the bubble. Now distinguishes the two.
- getUserPublicKey's ConnectionError was rewrapped into a generic
  EncryptionError, losing the cause. ConnectionError now passes through the
  outer catch untouched.

Neither is queued, deliberately: at those points the message has NOT been
encrypted yet (encryption needs the recipient key one of them is fetching), so
queuing would write plaintext to IndexedDB — the exposure in
GHSA-94f4-7f3v-g4g5. Fixing a UX bug by widening a live security finding is
the wrong trade.

## What actually saves the text

MessageInput clears optimistically on send, which is right — waiting for a
round trip makes the composer feel broken. But when the send then failed, the
words existed nowhere. It now accepts `restoreDraft` ({content, token}; the
token lets the same text restore twice) and puts it back, only into an empty
box so live typing is never clobbered. Wired through ChatWindow from the send
catch in messages/page.tsx.

## Tests: this had zero coverage

message-service.test.ts had no occurrence of 'Failed to fetch', 'NetworkError'
or 'ConnectionError' in 1044 lines. The one queue test flips navigator.onLine,
which returns early and never reaches these predicates — the bug was invisible
to the entire suite.

Adds four, driven through the public API with the real postgrest envelope
(status: 0, code: ''; every pre-existing mock omits status): one per engine
wording, plus a DB rejection whose text contains "network" asserting it is NOT
queued.

Mutation-checked, both directions:
- remove the code gate  -> the DB-rejection test fails
- remove 'load failed'  -> exactly 1 of 3 engine tests fails (WebKit), the
                           other two still pass

Refs #66, #76

Co-Authored-By: Claude Opus 5 (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.

1 participant