fix(realtime): a committed doc mention must not 500 on a Redis outage - #217
Merged
Conversation
`document_mentions` is the dedup ledger — processDocMention() skips any (doc, mentioner, mentioned) tuple written in the last 60 seconds. The toast was published AFTER that transaction committed, with no guard, so a Redis outage threw for work that had already succeeded. The retry then found its own rows inside the dedup window and did nothing: the notice was gone for good, not merely late. Move the event onto the transactional outbox that every other durable mutation already uses. mentionedIds is settled before COMMIT, so the enqueue fits in-band and a degraded Redis only delays delivery. Also fail-open the human typing indicator. It is pure ephemera the renderer expires on its own, and a composer that POSTs one every few seconds should not be taught to hammer the error path when Redis is down — the agent-side emitters on the same channel already swallow it.
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
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.
Closes the last two gaps in PERF-H1 from the security/performance review (
Redis outages cause unbounded request waits and offline-queue growth). The structural half of that finding has already landed:redis.ts:13-22bounds the command client (maxRetriesPerRequest: 1/enableOfflineQueue: false/commandTimeout: 2s) while leaving the subscriber free to reconnect, andrealtime-outbox.tsmoved durable mutations onto a transactional outbox. Two call sites never got the treatment.1. The doc-mention toast was published after COMMIT, unguarded
processDocMention()(ws.ts) writesdocument_mentions— and anagent_logrow for mentioned agents — then committed, then did:document_mentionsis not just a record, it is the dedup ledger: the loop above it skips any(document_id, mentioner_id, mentioned_id)tuple written in the last 60 seconds. So on a Redis outage:publishthrows, the caller sees a failure for work that succeeded,freshRowscomes back empty, and it returns early.The notice isn't late — it's gone. This is exactly the "after PostgreSQL commits,
await publish()can hang / throw" shape the review flagged, on the one durable path that hadn't been converted.Fixed by enqueueing on the same transaction.
mentionedIdsis settled before COMMIT, soenqueueBroadcast(client, CH_DOC_MENTION, event)fits in-band, followed bynudgeRealtimeOutbox()after commit — the patternonboardCompany.ts:354-365,polls.ts,membership.tsandtools.tsalready use. A degraded Redis now only delays the toast; the outbox worker retries with backoff and the dedup window is never spent on a delivery that didn't happen.2. The human typing indicator 500'd on a Redis outage
POST /conversations/:id/typingpublished bare. Typing is pure ephemera the renderer expires on its own, and the composer fires one of these every few seconds while a user types — turning a Redis blip into a steady stream of 500s teaches the client to hammer the error path for something nobody needs delivered. Now.catch()-logged and still{ ok: true }, matching how the agent-side emitters on the very same channel already behave (inproc-client.ts:557-566,scheduler.ts:450).What is deliberately unchanged
Every other
publish()in the tree is either already.catch()-guarded, already inside atry, or genuinely ephemeral (doc awareness, presence, wake/control bus — all of which have a documented durable fallback).subkeepsmaxRetriesPerRequest: nullon purpose: a subscriber must reconnect indefinitely, which is the separation the review asked for.Verification
processDocMentionis now exported so the integration suite can drive it directly — the only production caller is adoc.mention.notifyWS frame, and standing up a socket to assert a durable/outbox contract would test the transport instead. The new case inrealtime-outbox.test.tsreuses the file's existingpublishFnfault-injection:document_mentionshas 1 row and the outbox holds exactly 1 pendingcumora:doc.mentionevent — proving the enqueue is in-band, not post-COMMIT{ claimed: 1, published: 0, failed: 1 }and both rows intactmentionedIdspreserved, and the ledger still holds exactly one mentionLocal (this machine has no Postgres/Redis and no usable Docker daemon, so
npm test/npm run test:integrationare covered by PR CI's service containers — same situation as #212):