Skip to content

fix(desktop): stop retry/restore from truncating unpersisted failed turns - #86623

Closed
JoaoMarcos44 wants to merge 1 commit into
NousResearch:mainfrom
JoaoMarcos44:fix/desktop-retry-failed-turn-ordinal-86573
Closed

fix(desktop): stop retry/restore from truncating unpersisted failed turns#86623
JoaoMarcos44 wants to merge 1 commit into
NousResearch:mainfrom
JoaoMarcos44:fix/desktop-retry-failed-turn-ordinal-86573

Conversation

@JoaoMarcos44

Copy link
Copy Markdown
Contributor

What does this PR do?

planReload (Retry) and planRestore (restore-to-message) computed a truncate ordinal unconditionally, even when the target turn's user message never reached the gateway (e.g. a request that died on an HTTP 429 before the turn was ever persisted). With no durable row id to anchor it, that request travels the legacy truncate_before_user_ordinal-only path, whose address space can drift from the gateway's after compaction/rebuilds — silently truncating the wrong turn.

Root cause

planEdit already guards this exact case with an isFailedTurn check (nextMessage?.role === 'assistant' && Boolean(nextMessage.error)) that resubmits plainly instead of requesting a truncation it can't safely address. planReload and planRestore are the two remaining ReloadPlan/RestorePlan builders that never got the same guard — this PR applies the identical, already-proven pattern to both.

Why a client-side fix, not just a gateway guard

#86605 addresses this from the gateway side: it fails closed (4004) whenever an ordinal-only truncation request targets a session that has any durable row ids in its history. That's a reasonable defense-in-depth net, but on its own it does not fix the actual bug — it just turns "silently truncates the wrong turn" into "the Retry button now hard-fails" for the exact retry-after-failure scenario the issue describes, since planReload/planRestore (pre-fix) genuinely do emit ordinal-only requests for unpersisted failed turns. That PR's own "ruled out" section states failed-turn planning "does not request truncation," which this repro shows is not accurate for planReload/planRestore prior to this fix.

This PR fixes the producer instead: the client simply stops asking the gateway to truncate by position when it has no durable identity to offer, exactly like planEdit already does. Retry and Restore keep working after a failed turn; they resubmit plainly with no truncation at all, matching the safe, already-shipped behavior of Edit. This is complementary to (not a duplicate of) any gateway-side hardening.

Changes Made

  • apps/desktop/src/app/session/hooks/use-prompt-actions/rewind.ts
    • ReloadPlan.truncateOrdinal and RestorePlan.truncateOrdinal widened to number | undefined.
    • planReload and planRestore now null out truncateOrdinal / truncateMessageId / truncateRowId when the target turn's assistant response carries an error (the same isFailedTurn heuristic planEdit already uses).
  • apps/desktop/src/app/session/hooks/use-prompt-actions/rewind.test.ts
    • New planReload and planRestore describe blocks covering the normal (persisted) case and the failed-turn case.

Related Issue

Fixes #86573

Test plan

  • Added unit tests for planReload and planRestore covering both the persisted and failed-turn cases (rewind.test.ts).
  • Manual: trigger a 429/failed turn on Desktop, click Retry, verify no truncation is requested and the turn resubmits cleanly.

Diagram

%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#00f0ff', 'mainBkg': '#0a0a16', 'primaryTextColor': '#ffffff', 'primaryBorderColor': '#ff007f', 'lineColor': '#00f0ff'}}}%%
graph TD
    A[User clicks Retry / Restore] --> B{Target turn's assistant reply has error?}
    B -->|No - persisted turn| C[Attach durable row id + ordinal]
    B -->|Yes - never reached gateway| D[isFailedTurn guard]
    D --> E[Send plain resubmit - no truncation params]
    C --> F[prompt.submit with truncate_before_row_id]
    E --> G[Gateway: no truncation requested - safe]
    F --> H[Gateway: durable target - safe]
Loading

…urns

planReload and planRestore computed a truncate ordinal unconditionally,
even when the target turn's user message never reached the gateway
(e.g. a request that died on a 429). With no durable row id to anchor
it, that ordinal travels the legacy ordinal-only path, whose address
space can drift from the gateway's after compaction/rebuilds -
silently truncating the wrong turn (NousResearch#86573).

planEdit already guards this exact case with an isFailedTurn check
that resubmits plainly instead of requesting a truncation it can't
safely address. Apply the same guard to planReload (Retry) and
planRestore (restore-to-message), the two remaining callers that
build ReloadPlan/RestorePlan without it.

Fixes NousResearch#86573
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 15, 2026
teknium1 added a commit that referenced this pull request Aug 15, 2026
…w ids by content

Client half of #87059. The gateway now fails ordinal-only truncation
closed for durable sessions (#87150), which turned the mis-aimed cut into
a visible edit-resend error for any bubble without a bound rowId (edit
after an interrupted turn, unstamped resume). Make the Desktop always
produce a durable address or degrade safely:

- runRewindSubmit: when a truncation request lacks a durable address,
  resolve the target's row id by exact content against session.history
  (which ships row_id per persisted row). Resolution is
  exact-or-nothing: a unique text match wins; ambiguity is accepted only
  when the target is provably the newest persisted turn (the
  edit-after-interrupt shape). Anything else degrades to a PLAIN
  resubmit — never a guessed cut. The client ordinal is dropped either
  way (its space can diverge from the gateway's — the #87059 root).
- planReload/planRestore: degrade failed turns to a plain resubmit
  (extends the #86623 pattern to regenerate/restore) and carry the
  turn's persisted sourceText as the content key.
- rebindSurvivorRowIds: iterate the same failed-turn-aware ordinal
  space as the truncate math.
- session-tile-actions: reload goes through the shared runRewindSubmit
  primitive instead of a raw prompt.submit, so the tile surface gets the
  same discipline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Desktop retry of a failed turn silently truncates history via ordinal-only path — row-id guard (#83785) does not cover it

2 participants