Skip to content

fix(desktop): preserve concurrent session pin intent - #74444

Open
TomassonJW wants to merge 3 commits into
NousResearch:mainfrom
TomassonJW:fix/desktop-session-pins-current-20260730
Open

fix(desktop): preserve concurrent session pin intent#74444
TomassonJW wants to merge 3 commits into
NousResearch:mainfrom
TomassonJW:fix/desktop-session-pins-current-20260730

Conversation

@TomassonJW

@TomassonJW TomassonJW commented Jul 29, 2026

Copy link
Copy Markdown

Summary

  • preserve optimistic pin and unpin intent while session snapshots are stale
  • order concurrent pin writes so older PATCH responses cannot clear newer intent
  • retry the latest failed pin or unpin on the next reconciliation without a hot loop
  • cover both Shift-click and the Pin/Unpin menu through their shared synchronization path

Root cause

Both Desktop entry points update the same local pin store. The session watcher could then adopt an explicit but stale backend value before the PATCH was observed, or after an older PATCH resolved out of order. A failed latest unpin was also discarded, allowing the next backend refresh to re-pin the row.

Test plan

  • npm run test:ui --workspace apps/desktop -- src/store/session-pin-sync.test.ts (18 tests)
  • npm run test:ui --workspace apps/desktop (2,983 tests)
  • npm run typecheck --workspace apps/desktop
  • npm run lint --workspace apps/desktop (0 errors; existing warnings only)
  • npm run build --workspace apps/desktop
  • staged-content secret scan and git diff --check

Notes

The change is limited to the session pin synchronizer and its behavior tests. It does not alter Gateway lifecycle or restart behavior.

@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 labels Jul 29, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for covering the stale-page race and the superseded-promise cleanup path. The premise is confirmed on current main: pullRemotePins() runs before the local push pass in apps/desktop/src/store/session-pin-sync.ts:102, so a stale row can reverse a click before writePin() sets unconfirmed.

Problems

  • apps/desktop/src/store/session-pin-sync.ts:43-45 still dispatches every PATCH immediately. The generation only prevents an older promise from clearing the newer local guard; it does not order backend mutations. The server applies each PATCH directly (hermes_cli/web_routers/sessions.py:675-678) with no generation precondition, so two toggles can be applied out of order and leave the durable pin value opposite to the latest click.

Suggested changes

  • Serialize or coalesce writes per session id, retaining the latest desired value until the previous PATCH settles.
  • Add a test asserting that a second toggle does not send its PATCH until the first request settles, covering durable-write order rather than only response/guard order.

Automated hermes-sweeper review.


function profileFor(pinId: string): null | string | undefined {
return $sessions.get().find(row => sessionMatchesStoredId(row, pinId))?.profile
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This generation only orders local cleanup. setSessionPinnedRemote() is still invoked immediately for each toggle, while the backend applies each PATCH as it arrives (hermes_cli/web_routers/sessions.py:675-678), so rapid toggles can persist in reverse order. Please serialize or coalesce writes per id and retain the stale-read guard through the final queued write.

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/*) P3 Low — cosmetic, nice to have 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.

3 participants