CGY-3852 fix: re-buffer sendMessage on disconnect-before-ack (CGY-3852)

[Vijayakrishna Venkatesan (vj-venkatesan)]

Success criteria

• If the connection drops between sendMessage's "are we connected" check and the endpoint actually processing the message (e.g. a backend rolling restart mid-flight), the message must be re-buffered and resent on the next successful reconnect instead of being silently lost while the UI already shows it as sent.
• Against an endpoint that doesn't ack processInput at all, no message may ever be duplicated — re-buffering only kicks in once this endpoint has been positively observed to ack a message.
• switchSession()/disconnect() must not leak an old session's buffered message into a newly started session.
• emitWithAck: false must keep the exact previous fire-and-forget behavior.
• npm test must be reliable on a cold cache (no test framework existed before this PR — it's added from scratch).
• The published npm package must not ship test files.

How to test

1. npm install && npm test — 5/5 Jest tests pass, including src/socket-client.test.ts's SocketClient#sendMessage suite covering the ack-gating, disconnect-before-ack re-buffer, no-ack-endpoint safety, and emitWithAck: false fallback.
2. npm run build — clean, no TypeScript errors.
3. npm pack --dry-run — tarball no longer contains .test.js/.test.d.ts or jest.config.js.

Root cause

SocketClient.sendMessage() did a bare socket.emit("processInput", payload) with no ack, no timeout, no retry whenever it believed it was connected. If the connection dropped in the window between that check and the server actually processing the message, the message was silently lost with no recovery path — see the root-cause writeup on CGY-3852.

This PR:

• Uses socket.io's ack-with-timeout emit (socket.timeout(ms).emit(event, payload, cb)) when emitWithAck (existing default) is true.
• Re-buffers on a disconnect-before-ack only once this endpoint has been observed to successfully ack at least one message (hasConfirmedAck), so a non-acking endpoint can never receive a duplicate — the very first message on a connection is not protected (same as today's status quo for that case), but every message after the first confirmed ack is.
• Clears messageBuffer in switchSession() so an old session's in-flight message can't replay into a new one.
• Fixes a pre-existing, unrelated setInterval typing collision that made a from-scratch npm test unreliable on a cold cache.
• Excludes test sources from the published npm package.

Security

• Possible injection vector
• Authentication/Access controls touched
• Sensitive Data could be exposed
• XSS
• Logging/Monitoring touched
• Exchanges data with external systems
• No security implications

Additional considerations

• This PR might have performance implications — outbound sendMessage calls now wait on an ack (default 10s timeout) instead of being pure fire-and-forget; this is the intended trade-off (delivery confirmation vs. silent loss).

Semver note: this changes default runtime behavior for consumers who don't override emitWithAck (the existing default, true) — recommend at least a minor version bump with a changelog entry calling out the new default outbound ack, the at-least-once resend semantic, and emitWithAck: false as the opt-out.

🤖 Generated with a debugging/planning/multi-agent-implementation session using Claude Code.

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[Copilot]

Pull request overview

Updates SocketClient.sendMessage() to use socket.io’s ack-with-timeout mechanism (when emitWithAck is enabled) and re-buffer/resend messages when the connection drops before an ack arrives, plus introduces a new Jest test suite and packaging safeguards to keep test/tooling files out of the published npm tarball.

Changes:

• Add ack-with-timeout emission + disconnect-before-ack re-buffering logic gated by first successful ack observation.
• Add Jest + ts-jest test harness and a SocketClient#sendMessage test suite.
• Prevent tests/tooling from being compiled/published (tsconfig exclude + .npmignore updates).

Reviewed changes

Copilot reviewed 5 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tsconfig.json	Excludes src/**/*.test.ts from tsc output to avoid generating/publishing test artifacts.
src/socket-client.ts	Implements ack-based send + conditional re-buffer-on-disconnect and clears buffer on switchSession().
src/socket-client.test.ts	Adds Jest tests for sendMessage ack gating, resend-on-disconnect, and emitWithAck: false behavior.
src/interfaces/options.ts	Clarifies emitWithAck semantics (both server→client and client→server acknowledgement behavior).
package.json	Adds jest test script and Jest/ts-jest dev dependencies.
package-lock.json	Locks newly added Jest/ts-jest dependency tree.
jest.config.js	Configures Jest to run TS tests via ts-jest.
.npmignore	Excludes Jest config from published package.
.gitignore	Ignores .worktrees/ and normalizes existing patterns.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Dmitrii Ostasevich (kwinto)]

Review — verified by running the code

I installed this branch, ran npm test (5/5 pass), and wrote two probe specs against the branch to check success criteria that the suite does not cover. Good, careful work: the disconnect() → re-buffer → switchSession() ordering is subtle and correct (socket.io-client's _clearAcks() fires pending .timeout() acks synchronously inside disconnect(), so clearing the buffer after this.disconnect() really is required — the comment earns its place). Adding a test framework from scratch is a clear win.

Three substantive findings, one of which I think is blocking.

1. 🔴 This introduces a new silent message-loss path — the exact failure class the ticket is about

switchSession() now unconditionally does this.messageBuffer = []. The comment justifies it in terms of an in-flight message re-buffered by the disconnect() above, but the clear is not scoped to that — it also discards messages the user typed while already offline, which were buffered by the normal (pre-existing) buffering path and which the UI has already rendered as sent.

Probe against this branch:

PROBE P1 delivered = []
✓ P1: a message buffered while offline is SILENTLY DROPPED by switchSession (21 ms)

Reachable from Webchat today: "Start new conversation" (handleStartConversation) and "Delete conversation" both dispatch switchSession() on their own. Sequence: connection drops → user types → message buffered, shown as sent → user hits "Start new conversation" → message vanishes with no delivery and no UI signal.

Before this PR that message would have been flushed into the new session — wrong session, but delivered. So this trades "delivered to the wrong session" for "silently dropped", inside a PR whose success criteria open with "must … be resent … instead of being silently lost while the UI already shows it as sent".

Suggestion: only drop what disconnect() just re-buffered, rather than the whole queue — e.g. snapshot messageBuffer.length before this.disconnect() and truncate back to it, or have the ack error path push to a separate inFlightRequeue list that switchSession() clears while leaving user-buffered messages intact. If dropping everything really is intended, it needs to be an explicit, documented product decision, and ideally surfaced to the consumer rather than silent.

2. 🟠 hasConfirmedAck is per-client, not per-connection — and it is weakest in exactly the scenario this PR targets

The field doc says it is deliberately never reset (a property of the endpoint). But the rationale comment inside sendMessage says "once we've seen this endpoint successfully ack a message at least once on this connection", and both new test names say "on this connection". The code does not do that:

PROBE P2 resent on connection #3 = ["second"]
✓ P2: hasConfirmedAck is per-CLIENT, not per-connection (survives reconnect) (2 ms)

An ack observed on connection #1 unlocks re-buffering on connection #2, whose endpoint never acked anything. That matters because the headline scenario here is "a backend rolling restart mid-flight" — precisely when you can be moved onto a node that behaves differently from the one that acked. In that window the duplicate-suppression gate is open on no evidence, which is the duplicate the gate exists to prevent.

Either reset it in connect() (making the comments and test names true), or keep it sticky and fix the comment + both test names so they stop asserting a guarantee the code does not provide. I'd lean toward per-connection: it is strictly safer and costs at most one unprotected message per connection.

3. 🟡 emitWithAck now governs two independent directions

The option doc is honest about the widening, but it couples two unrelated decisions onto one flag. A consumer who sets emitWithAck: false today does so to change what the backend does (it is forwarded as a connect query param); they now also silently lose the new outbound protection. Conversely there is no way to keep inbound acking while opting out of the new 10s outbound wait. A separate option (ackOutboundMessages, defaulting to emitWithAck) would keep both knobs independent and make the semver story much easier.

Smaller points

• Untested success criterion. "switchSession()/disconnect() must not leak an old session's buffered message into a newly started session" has no test — which is how finding 1 slipped through. Worth covering both directions: leak, and non-loss of legitimately buffered messages.
• At-least-once ⇒ duplicates, with no idempotency key. Endpoint processes the message, ack is lost in the drop → resend → the flow runs twice. Fine for chit-chat, not for "book the flight". The description flags the semantic, but a client-generated message id that the endpoint could dedupe on would turn at-least-once into effectively-once and de-risk the whole change.
• PROCESS_INPUT_ACK_TIMEOUT_MS = 10000 is hardcoded and, stylistically, a static readonly declared mid-class between methods; consider making it an Options field next to interval / reconnectionLimit.
• .npmignore still has no trailing newline (both edited files end mid-line).

Relationship to Cognigy/Webchat#313 — these do not overlap

Both PRs are CGY-3852 but fix different failure modes, and it is worth being explicit in both descriptions:

• This PR: message lost after a successful send, in the disconnect-before-ack window.
• Webchat#313: no reconnect is ever attempted when the first connection attempt fails.

Importantly, this PR does not fix the upstream defect behind Webchat#313: setupReconnectInterval() is still only reachable from inside socket.on("connect") (line 352), so a first connect() that rejects still leaves no retry machinery at all — no interval, and the buffered message sits in messageBuffer forever. That is the one change here that would have fixed the problem in every consumer instead of just Webchat. Worth a follow-up ticket, or folding into this PR while the area is already open.

[Dmitrii Ostasevich (kwinto)]

Correction to my review above — wrong symbol cited, finding unchanged.

In finding 1 I wrote that the drop path is reachable from "Start new conversation (handleStartConversation)". That function name is wrong: WebchatUI.handleStartConversation only calls onSetShowHomeScreen(false) / onSetShowChatOptionsScreen(false) / onShowChatScreen() and never touches switchSession. Apologies — I should have checked before naming it.

The finding itself stands; here are the call sites I actually verified on Webchat@main, all of which dispatch switchSession() with no accompanying sendMessage, so anything sitting in messageBuffer is discarded:

Call site	Trigger
ConversationsList.tsx:86 (handleStartButtonClick)	"Start new conversation" in the previous-conversations list
DeleteConversation.tsx:48	Delete the current conversation
DeleteAllConversations.tsx:30	Delete all conversations
Webchat.tsx:213 (endSession)	Public embedding API — webchat.endSession()

The last one is worth highlighting more than the one I originally cited: endSession() is public API that integrators call from their own page code, and it is documented as "switch to a new session and clear the messages in the current session" — nothing there implies "and silently discard an undelivered message the user already saw as sent".

For completeness, the one place that is safe is WebchatUI.tsx:1116 (handleSendActionButtonMessage), because it dispatches onSwitchSession() immediately before onSendMessage(), so the clear happens first and the message is buffered afterwards. That ordering is currently load-bearing but unenforced — which is really an argument for scoping the clear to just the re-buffered in-flight message, rather than relying on dispatch order at every call site.

[Dmitrii Ostasevich (kwinto)]

Amending my own suggestion for finding 1 — and I've now validated the replacement.

My original suggestion (preserve messages buffered before the switch) was wrong: it would deliver a session-A message into session B, which directly violates your own success criterion "switchSession()/disconnect() must not leak an old session's buffered message into a newly started session". Scratch that — your drop is the right call.

The actual defect is narrower than I framed it: not that the message is dropped, but that it is dropped silently while the consumer has already rendered it as sent. Keep the drop; just make it observable.

I implemented this on your branch and verified it — tsc clean, and all 5 of your existing tests still pass (8/8 with the 3 I added):

         this.disconnect();
 
         // Any message still buffered at this point belongs to the session
         // we're leaving (disconnect() may have just re-buffered an
         // in-flight message via the ack-callback error path above). Drop
         // it here so flushMessageBuffer() doesn't replay it into the new
         // session once it connects.
+        //
+        // These messages were never delivered, and a consumer such as the
+        // Webchat has already rendered them as sent, so tell it they are
+        // gone instead of dropping them silently.
+        const undelivered = this.messageBuffer;
         this.messageBuffer = [];
+
+        if (undelivered.length > 0) {
+            this.emit("messagesDiscarded", {
+                reason: "session-switched",
+                messages: undelivered,
+            });
+        }

Tests I used (happy to open them as a PR against your branch if useful):

✓ still does not leak an old session's messages into the new session (32 ms)
✓ tells the consumer which messages were discarded, instead of dropping them silently (3 ms)
✓ stays quiet when there is nothing undelivered to report (1 ms)

Why this is worth the extra ~8 lines:

• It closes the third untested success criterion under the same ticket, using the mechanism the class already has (EventEmitter), with no behaviour change for consumers that don't subscribe — so it stays additive for semver.
• It gives Webchat somewhere to hang a "not delivered" affordance later, rather than baking a silent-loss path into the client that no consumer can even detect.
• It is reachable from the public webchat.endSession() API, so integrators can hit it from their own page code with no way to know a message vanished.

Naming is a straw man — messagesDiscarded / reason: "session-switched" just mirrors the existing socket/error { type } shape; rename freely.

[Dmitrii Ostasevich (kwinto)]

Status check: this PR is unchanged since my review (head e110e9b, last commit 26 Aug), while its sibling Cognigy/Webchat#313 has since landed fixes for everything raised there and is green. Flagging so this one doesn't get forgotten as the "done" half of CGY-3852 — the three findings above are still open, in particular:

1. 🔴 the message silently dropped by switchSession() (validated messagesDiscarded patch posted above, 8/8 tests passing)
2. hasConfirmedAck being per-client rather than per-connection, contradicting both new test names
3. no coverage for the buffer-clear success criterion

Also worth noting: the author of #313 filed CGY-37178 for the setupReconnectInterval() root cause — that fix belongs in this repo, not Webchat. setupReconnectInterval() is still only reachable from inside socket.on("connect") (L352), so a first connect() that fails installs no retry machinery at all and the buffered message is stranded permanently, for every consumer. Neither PR addresses it. Worth deciding whether it rides along here or gets its own PR.

---

On the open Copilot thread about the ack callback observing a different socket instance — I looked into this, and I don't think it's reachable, so it shouldn't block:

I read socket.io-client@4.7.5 (build/cjs/socket.js) directly. onclose() (L467) sets connected = false, emits "disconnect", then calls _clearAcks() (L480), which invokes pending .timeout() acks with new Error("socket has been disconnected") — synchronously, inside the close handler. So socket A's acks are all settled before any reconnect can assign a new socket to this.socket, and the callback always observes A. The only way the callback runs while a newer socket is connected is the 10s timeout path, and that requires A to still be reporting connected (so no reconnect would have been triggered in the first place).

The same reading is also why the disconnect()-before-clear ordering in switchSession() is correct and load-bearing, which is worth a comment in the code so nobody "tidies" it later.

For completeness, I also chased and disproved a related concern: _clearAcks skips acks whose packet is still in sendBuffer, which would strand them. Not reachable here — sendBuffer.push only happens when !connected, and sendMessage never emits in that state.