Skip to content

fix(desktop): serialize session persistence - #745

Open
imMamdouhaboammar wants to merge 1 commit into
lsdefine:mainfrom
imMamdouhaboammar:fix/desktop-session-persistence-race-upstream
Open

fix(desktop): serialize session persistence#745
imMamdouhaboammar wants to merge 1 commit into
lsdefine:mainfrom
imMamdouhaboammar:fix/desktop-session-persistence-race-upstream

Conversation

@imMamdouhaboammar

Copy link
Copy Markdown

Problem

Desktop session persistence used one deterministic temp path per session (<sid>.json.tmp) while releasing self.lock before disk I/O. Two threads persisting the same session could therefore write/replace the same temp file concurrently. A separate deletion race also allowed a late worker persist to recreate a session JSON after the session had been deleted from manager state.

Root cause

_persist_session() only held self.lock while building the data snapshot, then performed tmp.write_text() and os.replace() unlocked. _delete_session_file() also performed its filesystem operation without the same lock, and _persist_session() did not verify that the Session object was still registered before writing.

Fix

  • keep the existing RLock held across the session snapshot, temp write, and atomic replace
  • skip persistence when self.sessions.get(s.id) is not s, preventing stale workers from recreating deleted sessions
  • perform session-file deletion under the same lock

This preserves the existing per-session JSON format and atomic replace strategy.

Regression coverage

  • concurrent persists of the same session cannot overlap writes to <sid>.json.tmp
  • a late persist of a deleted Session object cannot recreate its JSON file
  • the test harness loads desktop_bridge.py only inside the unittest class lifecycle, uses an isolated temporary GA_ROOT, restores sys.argv and sys.modules, and does not leave import-time global state behind

Verification

TDD was performed on a separate validation branch:

  1. Regression-only GitHub Actions run 31164526542 failed both targeted cases on current main
    • concurrent same-session temp writes overlapped and produced an actual FileNotFoundError from os.replace
    • a late _persist_session() recreated the file after delete_session()
  2. The minimal locking/registration fix passed run 31164598932: compile, both deterministic concurrency regressions, and git diff --check
  3. Review feedback identified test-global-state leakage. The harness was refactored without changing production code, and validation run 31165250013 passed compile, both regressions, and git diff --check
  4. The contribution branch was then rebuilt directly from current main as one clean commit containing the production fix plus the isolated regression test

Scope

  • 1 production file modified
  • 1 focused concurrency regression-test file added
  • 1 clean commit on top of current main
  • no dependencies or runtime configuration changes

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