Skip to content

perf(session): defer non-critical writes in SessionStore._save() - #23275

Closed
devsart95 wants to merge 1 commit into
NousResearch:mainfrom
devsart95:perf/session-deferred-save
Closed

perf(session): defer non-critical writes in SessionStore._save()#23275
devsart95 wants to merge 1 commit into
NousResearch:mainfrom
devsart95:perf/session-deferred-save

Conversation

@devsart95

Copy link
Copy Markdown

Description

SessionStore._save() serializes the entire _entries dict to sessions.json every time anything changes (17 call sites). For high-frequency metadata updates (e.g. updated_at, last_prompt_tokens), this is wasteful I/O.

This PR introduces _save_deferred() for non-critical writes that can be coalesced, while keeping critical state changes (suspend, resume_pending, switch, reset, prune) as immediate _save().

Changes

  • Added _pending_save: bool flag to SessionStore
  • Added _save_deferred() — sets flag without I/O (must be called under lock)
  • Added flush_pending_save() — flushes to disk if flag is set
  • get_or_create_session (no-reset path) and update_session now use _save_deferred()
  • All critical call sites remain immediate _save()
  • flush_pending_save() called after _ensure_loaded_locked() to persist any deferred writes left pending before an unclean shutdown

Design decisions

  • No format change to sessions.json
  • No data migration needed
  • No change to gateway/run.py call sites (all are already critical/frontier operations)
  • Existing tests for critical paths (resume_pending, suspend, switch) are unaffected

X: @rojassartorio
Co-authored-by: Rojas Sartorio rojassartorio@users.noreply.github.com

Add _save_deferred() for high-frequency metadata updates (e.g.
updated_at, last_prompt_tokens) that can be coalesced instead of
serializing the entire sessions.json index on every change.

- get_or_create_session (no-reset path): _save_deferred()
- update_session: _save_deferred()
- All other call sites (suspend, resume_pending, switch, reset,
  prune, new entry): remain immediate _save()

Add flush_pending_save() on load to persist any deferred writes
that may have been left pending before an unclean shutdown.

Co-authored-by: Rojas Sartorio <rojassartorio@users.noreply.github.com>
@alt-glitch alt-glitch added type/perf Performance improvement or optimization P3 Low — cosmetic, nice to have comp/gateway Gateway runner, session dispatch, delivery labels May 10, 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 targeting a real per-turn persistence cost. Current main needs this reworked rather than applied directly.

Problems

  • The added _pending_save is only in memory. The sole added flush_pending_save() call is during initial loading, after a new store initializes the flag to False; no timer, shutdown hook, or later lifecycle call flushes mutations made by _save_deferred() (gateway/session.py, PR diff lines 675–757). Metadata can therefore remain unpersisted for the entire process and be lost on crash.
  • The PR targets the old JSON-only persistence path. Current SessionStore._save() replaces the complete gateway_routing scope in SQLite and writes sessions.json only as a compatibility mirror (gateway/session.py:1181-1215; 94205a113).

Suggested changes

  • Rebase the design on the current state.db-plus-mirror persistence path and define a bounded, concurrency-safe flush lifecycle.
  • Add durability/coalescing tests for that lifecycle and the immediate routing-transition paths.

Automated hermes-sweeper review.

Comment thread gateway/session.py
print(f"[gateway] Warning: Failed to load sessions: {e}")

self._loaded = True
# Flush any deferred writes from a previous run that may not have

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.

_pending_save is initialized to False for every new SessionStore, so this initial-load call cannot flush deferred work from a prior process. The diff adds no timer, shutdown hook, or later lifecycle call to flush work scheduled by _save_deferred(), leaving metadata non-durable until an unrelated immediate _save() happens.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 13, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Closing as superseded on main. The two call sites this PR defers (the get_or_create updated_at touch and the update_session token/updated_at write) are now routed through SessionStore._save_entry — a single-row UPSERT into the state.db gateway_routing table (gateway/session.py:1590-1661), sub-millisecond and immediately durable, with generation-numbered stale-write protection. sessions.json is a legacy mirror rather than the hot path, so deferring its writes no longer buys anything, and the deferred flush-on-next-load window this PR introduced would trade away durability that main's mechanism keeps. The sibling write-amplification problem on the SQLite transcript side (your companion #23254) was real and is salvaged in #77619 with authorship preserved. Thank you for both PRs — the pair correctly identified per-turn persistence as the hot spot.

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/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants