Skip to content

fix(desktop): stop live-sync duplicating every message on send - #352

Merged
Kyzcreig merged 1 commit into
mainfrom
fix/desktop-optimistic-stamp-dup
Jul 15, 2026
Merged

Kyzcreig merged 1 commit into
mainfrom
fix/desktop-optimistic-stamp-dup

Conversation

@Kyzcreig

Copy link
Copy Markdown
Collaborator

Two-part fix for desktop messages rendering twice (display-only; DB clean). Server: message.complete now carries committed message_ids via _completed_turn_committed_ids(). Client: stamp predicate recognizes completed assistant rows (user-/assistant- prefix OR pending). Gated by RED→GREEN tests (6 server + 2 client). Upstream: NousResearch#64564.

The desktop renders a sent turn optimistically (client-minted ids user-<ts>/assistant-<ts>) and stamps them to the committed DB ids carried on the message.complete frame. Two defects made stamping a no-op, so the live session-sync poll re-fetched the committed rows, failed to recognize them, and appended them as DUPLICATES (every message shown twice; DB stays clean):

1. Server (tui_gateway/server.py): message.complete never included the committed ids. Add _completed_turn_committed_ids() to extract [user_id, assistant_id] for the finished turn from session history (_db_persisted_row_id) and ship them as payload.message_ids.

2. Client (use-session-changes.ts): the stamp predicate only matched user-prefixed OR pending rows, but completeAssistantMessage() clears the streamed assistant row pending flag BEFORE markTurnComplete() stamps it, so the completed assistant row (id assistant-<ts>, pending:false) was skipped and duplicated. Recognize any optimistic id (user-/assistant- prefix) OR pending.

Gated by tests that FAIL on the naive code: tests/tui_gateway/test_completed_turn_committed_ids.py (6) + two new cases in use-session-changes.test.ts proving the already-completed assistant row stamps and does not duplicate end-to-end.
@Kyzcreig
Kyzcreig enabled auto-merge (squash) July 15, 2026 22:56
@Kyzcreig
Kyzcreig merged commit d68abfb into main Jul 15, 2026
40 checks passed
@greptile-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a desktop bug where every sent message was rendered twice (UI display only, DB was clean). The root cause was a two-part race: the client's completeAssistantMessage() cleared pending on the assistant row before markTurnComplete() ran the stamp, so the stamp predicate (which required user- prefix OR pending) silently skipped the assistant row; the post-completion poll then re-appended the committed row as a duplicate. The server half adds _completed_turn_committed_ids() to emit real DB row ids on message.complete; the client half widens the stamp predicate to also recognize assistant- prefixed ids.

  • Server (tui_gateway/server.py): New _completed_turn_committed_ids() walks session history under history_lock, returns the last user row id and last assistant text row id (skipping tool-call rows), and attaches them to the message.complete payload as message_ids.
  • Client (use-session-changes.ts): stampOptimisticTranscriptRows and optimisticTranscriptIds now treat any assistant--prefixed id as an optimistic id, regardless of pending state, so the completed assistant row is correctly re-keyed to its committed integer id before the poll runs.
  • Tests: Six new Python tests cover the server helper; two new TypeScript tests cover the exact runtime ordering that triggered the bug (pending cleared before stamp) and the full end-to-end seam.

Confidence Score: 4/5

The fix is safe to merge. The duplication bug is real and reproducible and the changes target it precisely with matching tests on both sides of the stack.

The core change — widening the stamp predicate to accept assistant- prefixed ids — is correct and directly addresses the reported race. One design edge case exists in _completed_turn_committed_ids: when a user message has no _db_persisted_row_id, the function returns only the assistant id, and the client will assign that id to the user's optimistic row instead of leaving everything unstamped. The docstring describes the intent as 'no stamping' but the code delivers partial wrong-role stamping. This scenario requires a DB flush miss that should not occur in normal operation, so it is unlikely to surface in production, but it is worth verifying the ordering guarantee before the next multi-turn stress test.

tui_gateway/server.py — specifically the return value of _completed_turn_committed_ids when only one of the two expected ids is present.

Important Files Changed

Filename Overview
tui_gateway/server.py Adds _completed_turn_committed_ids() (new helper) and calls it under history_lock before _emit("message.complete", ...). The helper correctly skips tool-call rows and takes the last user/assistant text ids; a partial-list edge case (user id missing) could cause mis-aligned stamping on the client, though this is unlikely in normal operation.
apps/desktop/src/app/chat/hooks/use-session-changes.ts Widens the stamp predicate in stampOptimisticTranscriptRows and optimisticTranscriptIds to recognise assistant- prefixed ids regardless of pending state — the direct fix for the duplication bug.
tests/tui_gateway/test_completed_turn_committed_ids.py New test file with 6 unit tests covering the Python helper: normal case, tool-loop turn, empty-content tool row, missing row ids, non-int row id, and empty/invalid history. All scenarios are well-documented and correct.
apps/desktop/src/app/chat/hooks/use-session-changes.test.ts Adds two targeted TypeScript tests: one for the exact runtime ordering (pending cleared before stamp) that caused the bug, and one full end-to-end seam test verifying no duplicate is added after a post-completion poll.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant C as Desktop Client
    participant S as TUI Gateway Server
    participant DB as Database

    C->>S: User sends message (optimistic ids: user-ts, assistant-ts)
    S->>DB: Flush messages (_db_persisted_row_id stamped: 100, 101)
    Note over C: completeAssistantMessage()<br/>clears pending on assistant-ts row
    S->>S: _completed_turn_committed_ids(history) → [100, 101]
    S->>C: "message.complete {message_ids: [100, 101]}"
    Note over C: markTurnComplete() runs stampOptimisticTranscriptRows()<br/>user-ts → 100, assistant-ts → 101<br/>(assistant-ts matched by assistant- prefix, not pending)
    C->>S: "session.changes poll (since=99)"
    S->>C: "[{id:100,user},{id:101,assistant}]"
    Note over C: appendFetchedMessages: 100 and 101 already in renderedIds → NO DUPLICATES
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant C as Desktop Client
    participant S as TUI Gateway Server
    participant DB as Database

    C->>S: User sends message (optimistic ids: user-ts, assistant-ts)
    S->>DB: Flush messages (_db_persisted_row_id stamped: 100, 101)
    Note over C: completeAssistantMessage()<br/>clears pending on assistant-ts row
    S->>S: _completed_turn_committed_ids(history) → [100, 101]
    S->>C: "message.complete {message_ids: [100, 101]}"
    Note over C: markTurnComplete() runs stampOptimisticTranscriptRows()<br/>user-ts → 100, assistant-ts → 101<br/>(assistant-ts matched by assistant- prefix, not pending)
    C->>S: "session.changes poll (since=99)"
    S->>C: "[{id:100,user},{id:101,assistant}]"
    Note over C: appendFetchedMessages: 100 and 101 already in renderedIds → NO DUPLICATES
Loading

Reviews (1): Last reviewed commit: "fix(desktop): stop live-sync duplicating..." | Re-trigger Greptile

Comment thread tui_gateway/server.py
Comment on lines +5797 to +5816
user_id: int | None = None
assistant_id: int | None = None
for m in history:
if not isinstance(m, dict):
continue
role = m.get("role")
row_id = m.get("_db_persisted_row_id")
if not isinstance(row_id, int):
continue
if role == "user":
user_id = row_id
elif role == "assistant" and not m.get("tool_calls"):
# Only a visible assistant *text* row is an optimistic client row;
# an assistant(tool_calls) row renders as a tool card via the poll.
content = m.get("content")
if isinstance(content, str) and not content.strip():
continue
assistant_id = row_id

return [rid for rid in (user_id, assistant_id) if rid is not None]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Partial ID list causes misaligned stamping

When only the assistant row has a _db_persisted_row_id (e.g. user message not yet flushed), the function returns [assistant_id] instead of []. The client's stampOptimisticTranscriptRows then stamps in position order — so user-<ts> (the first optimistic row) gets assigned assistant_id, while assistant-<ts> is left with its optimistic id and later dropped by the poll. The docstring says "partial or absent list simply means no stamping this turn", but the partial [assistant_id] path causes wrong-role id assignment rather than a clean no-op.

Returning [] instead of a partial list when either required id is missing matches the stated intent and avoids the mis-stamp. The test test_missing_row_ids_are_skipped asserts [101] today, so this is a documented design choice — but the client-side consequence of receiving a one-element list was not validated there.

Kyzcreig added a commit that referenced this pull request Jul 16, 2026
…357)

* fix(desktop): ship the runtime footer as metadata, not message text

The /footer runtime footer was appended to the message.complete TEXT. That
broke the desktop two ways once #352's stamping landed:
1. DUPLICATED every message: the streamed bubble has no footer, the completed
   text does -> the streamed-vs-final dedupe in completeAssistantMessage no
   longer matches -> the completed text appends as a second bubble.
2. The footer vanished on session-sync re-hydration (DB rows carry no footer).

Fix: the server ships the footer as payload.footer on message.complete (text
stays pristine); the client carries it through completeAssistantMessage ->
ChatMessage.footer -> toRuntimeMessage metadata.custom.footer and renders it
as a muted monospace line under the bubble (RuntimeMetadataFooter), proper
chrome instead of fake message text.

Contract test pins 'footer never concatenated into text'. Desktop 1305 green,
TSC clean, tui_gateway suites 91 green.

* test: CWD-independent path + explicit find() guards (Greptile P2s)

---------

Co-authored-by: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com>
Kyzcreig added a commit that referenced this pull request Jul 16, 2026
…ep them out of the render cache (#361)

* fix(desktop): sweep zombie optimistic rows on the reconnect seam + keep them out of the render cache

A backend restart / WS reconnect can sever the message.complete stamping path
(#352): the turn's committed rows land in state.db but the completion frame
never reaches the client, so its optimistic rows (user-<ts> / assistant-
stream-<ts>) keep their client-minted ids. The id-only dedupe in
appendFetchedMessages then paints the polled committed rows as DUPLICATES
beside the zombies — and the render-cache write-through persisted the zombies
to disk, re-infecting every subsequent boot (observed live 2026-07-15:
assistant-stream-1784172446416 cached next to its committed twin 695173; DB
clean, pure render corruption; a relaunch did NOT clear it because the cache
re-seeded the zombie).

Three narrow changes:
1. appendFetchedMessages runs dropZombieOptimisticRows before appending: an
   optimistic, non-pending row whose (role, exact text) matches an incoming
   committed row is dropped in favor of the committed twin. Guardrails:
   committed rows untouchable, pending (streaming) rows exempt, empty-text
   rows exempt, each incoming row consumes at most one zombie.
2. pushTranscriptToRenderCache persists committed rows only (prefix-allowlist
   isCommittedTranscriptRow; unknown string ids fail open).
3. normalizeCachedTranscriptRows drops legacy optimistic rows already on disk
   before painting (all-optimistic file = cache miss), preserving array
   identity on the pass-through path.

Tests: 6 reconnect-seam cases in use-session-changes.test.ts (zombie drop for
user+assistant, pending exempt, no over-collapse, one-zombie-per-row budget,
role mismatch) + 4 cache-hygiene cases in render-cache-hydration.test.ts.
RED-proven: reverting the merge-loop line fails 2, reverting the cache filters
fails 4. Full desktop suite 1320/1320, tsc clean.

* fix(desktop): normalize MEDIA-tag representation in the zombie join key (Greptile #361 P2)

Committed assistant rows arrive media-rendered (assistantTextPart ->
renderMediaTags) while a streamed zombie may hold the raw MEDIA: line;
normalize both sides through the idempotent renderMediaTags so the
(role, text) join key is representation-stable. +1 test.

---------

Co-authored-by: Kyzcreig <9063726+Kyzcreig@users.noreply.github.com>
Kyzcreig added a commit that referenced this pull request Sep 25, 2026
Fork-PR audit FINAL DROP (card t_46cf57ba, lead t_03e35f0e, FINAL.md row #352).
#352 shipped payload.message_ids on message.complete so the desktop
live-sync poll could stamp optimistic rows. The livesync seam is being
reverted (#268/#272, audit/scripts_misc/revert-livesync) and the apps/
consumer is already DROP by D9; no fork consumer of message_ids remains
(git grep apps/ ui-tui/ web/: 0 hits) and upstream never emitted it.

Removes _completed_turn_committed_ids(), its call under history_lock, the
payload field, and tests/tui_gateway/test_completed_turn_committed_ids.py.

Verified: test-gate narrow run test_failed_turn_retention.py +
test_desktop_runtime_footer.py + test_server_no_duplicate_defs.py:
29 passed.
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