Skip to content

fix(desktop): keep fresh pin/unpin actions from being undone by pin sync - #76304

Closed
Yoki-cmd wants to merge 1 commit into
NousResearch:mainfrom
Yoki-cmd:fix/desktop-pin-sync-race
Closed

fix(desktop): keep fresh pin/unpin actions from being undone by pin sync#76304
Yoki-cmd wants to merge 1 commit into
NousResearch:mainfrom
Yoki-cmd:fix/desktop-pin-sync-race

Conversation

@Yoki-cmd

@Yoki-cmd Yoki-cmd commented Aug 1, 2026

Copy link
Copy Markdown

Symptom

On the desktop app, three user-visible failures in the pinned-sessions feature, all in apps/desktop/src/store/session-pin-sync.ts:

  1. Pinning a session silently fails — the session never moves into the pinned section (or moves and is instantly removed).
  2. Unpinning silently fails — a pinned session stays pinned no matter how many times the user clicks 取消置顶.
  3. Pinning a second session right after the first undoes the first — the earlier pin is dropped from the pinned section.

Root cause

reconcile() runs the pull pass (pullRemotePins) before the push pass, and unconfirmed only guards writes that are already in flight. Each fresh local write (pin or unpin) has a window where the sidebar row still carries the pre-write server value and the PATCH has not been sent or acked:

  • Window 1 (fresh pin): pinSessionset → reconcile runs synchronously; the row still says pinned=false and the PATCH hasn't been sent, so the pull pass reads the new local pin as stale and removes it.
  • Window 2 (fresh unpin): the row still says pinned=true, so the pull pass re-adopts the pin the user just removed.
  • Window 3 (post-ack): writePin clears the unconfirmed guard on the PATCH ack, but the row keeps its old pinned value until the WS row update lands. The next reconcile (e.g. the user pinning a second session) then reads the stale row as the server disagreeing and removes the confirmed pin.

Fix

  • Track the pinned set as of the last reconcile (lastSeen). Ids added since then are fresh pins — register them in pending before the pull pass and skip the pull's undo branch for them. Ids removed since then are fresh unpins — skip the pull's adopt branch for them (freshlyUnpinned).
  • On the PATCH ack, refresh the row's pinned field in $sessions so the pull pass never reads our own confirmed write as stale.
  • Boot pins (seeded from localStorage at the first reconcile) stay subject to server authority in both directions.

The pre-existing pull pass behavior — adopting remote pins, dropping stale local pins the server reports unpinned, deferring pins until their row loads — is unchanged and still covered by the existing suite.

Tests

  • apps/desktop/src/store/session-pin-sync.test.ts: added 3 tests reproducing each fresh-write window (stale pinned=false page before the pin PATCH is sent; stale pinned=true page before the unpin PATCH is sent; sequential pin-pin with the first row still stale). All 14 tests in the file pass; the existing 11 remote-pull tests are untouched.
  • Full src/store suite: 597 passed.

@Yoki-cmd
Yoki-cmd force-pushed the fix/desktop-pin-sync-race branch from baca5a5 to 1ee86be Compare August 1, 2026 17:11
@teknium1

teknium1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for the focused regression coverage. The premise is confirmed on current main: reconcile() calls pullRemotePins() before local push bookkeeping at apps/desktop/src/store/session-pin-sync.ts:102, while the pull can mutate local pins from stale row state at lines 84-91. This change fences fresh local additions/removals before that pull (apps/desktop/src/store/session-pin-sync.ts:128-149 in the PR) and refreshes the matching cached row after an acknowledged PATCH (lines 52-64).

A concurrent maintainer-authored alternative exists in #75973 (9cc3f515629dcf8287ba7ffb237014dbb24d4a29), but it is not on current main; this PR remains a valid fix candidate rather than an implemented-on-main duplicate.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state needs-decision Awaiting maintainer decision before any implementation labels Aug 1, 2026
@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Aug 1, 2026
@jamesconnors

Copy link
Copy Markdown

Independent reproduction and validation from a macOS ARM64 Hermes setup using Desktop as a thin client against a remote gateway:

  • User-visible symptom matched this PR exactly: both the session-row context-menu Pin action and Shift-click reached the shared onPin() callback, but the pin disappeared immediately.
  • On current main, a loaded row with pinned: false is pulled before the local push pass, so the optimistic pin is removed before PATCH pinned=true starts.
  • I independently wrote the same production-shaped regression first (row(..., { pinned: false })) and observed it fail with expected [] to include 'optimistic'.

I then checked out this PR at 1ee86bea0 in an isolated worktree, installed its exact lockfile dependencies with Node v22.23.1, and ran:

  • npm test -- --project ui src/store/session-pin-sync.test.ts14/14 passed
  • npm run test:ui3,234/3,234 passed
  • npm run typecheck — passed
  • npx eslint src/store/session-pin-sync.ts src/store/session-pin-sync.test.ts — passed
  • git diff --check HEAD^ HEAD — passed

The additional post-ack/sequential-pin case is important. A simpler push-before-pull reorder fixes the first click but can still lose a confirmed pin after unconfirmed clears and before the cached row catches up; this PR explicitly covers that window.

One non-blocking documentation nit: the module header still says the whole local pin set is re-asserted at boot (session-pin-sync.ts lines 10–12), while the new lastSeen === null behavior and PR description intentionally make the first server row authoritative. Updating that comment would keep the stated contract aligned with the implementation.

I did not install this branch on the user's live thin client; the evidence above is an independent code-path reproduction plus clean focused/full-suite validation.

Three race windows in session-pin-sync could undo the user's pin state,
each with a user-visible symptom:

1. Pinning a session silently failed: reconcile() runs the pull pass
   synchronously while the sidebar row still carries pinned=false (the
   PATCH has not been sent), so the fresh local pin was read as stale
   and removed before the write even left.
2. Unpinning silently failed: the stale pinned=true row made the pull
   pass re-adopt the pin the user had just removed.
3. Pinning a second session right after the first undid the first: the
   PATCH ack clears the unconfirmed guard, but the row keeps its old
   pinned value until the WS row update lands, so the next reconcile
   read the stale row as the server disagreeing and removed the pin.

Fix: track ids added/removed since the last reconcile (lastSeen diff) and
hold them out of the pull pass until their PATCH is written (pending /
freshlyUnpinned guards); refresh the row's pinned field on the PATCH ack
so the pull pass never reads our own confirmed write as stale. Pins
present at boot stay subject to server authority in both directions.

Adds tests for each fresh-write window; the existing remote-pull suite
is unchanged and still green.
@Yoki-cmd
Yoki-cmd force-pushed the fix/desktop-pin-sync-race branch from 1ee86be to a82a223 Compare August 1, 2026 23:27
@Yoki-cmd

Yoki-cmd commented Aug 1, 2026

Copy link
Copy Markdown
Author

Thanks for the independent reproduction and the thorough validation — great to have the macOS/remote-gateway path confirmed, and glad the post-ack window test held up.

The doc nit is addressed: the module header now states that the boot re-assertion applies except where a session row already carries an explicit pinned value, which is authoritative from the first reconcile (pushed as a82a223f).

@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists and removed P3 Low — cosmetic, nice to have labels Aug 1, 2026
@OutThisLife

Copy link
Copy Markdown
Collaborator

Superseded by #80711. Same pin-sync race (fresh pin/unpin undone by the next reconcile).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

5 participants