perf(gateway): single-row routing UPSERT fast path for metadata-only saves - #64169
perf(gateway): single-row routing UPSERT fast path for metadata-only saves#64169Soju06 wants to merge 1 commit into
Conversation
e7c9829 to
6ad2433
Compare
7cdd1c7 to
bb7e695
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating a real per-turn routing persistence cost. Current main still serializes the full routing index on the healthy lookup path (gateway/session.py:2021-2022) and in update_session (gateway/session.py:2051-2065).
Problems
- The new fast-path guard can let an older metadata snapshot overwrite a newer one.
_save_entryreads_routing_generationbut does not advance it, while the guard only skips when_persisted_routing_generation > snap_gen(PRgateway/session.py:1353). Two concurrent fast saves can therefore sharesnap_gen: after the newer UPSERT completes, the delayed older UPSERT still passes the guard and replacesentry_json.save_gateway_routing_entryis an unconditional conflict update (hermes_state.py:1909-1914). This regresses the ordering contract introduced byb196ce80cand currently implemented ingateway/session.py:1241-1275.
Suggested changes
- Give metadata-only writes a durable ordering mechanism (for example, per-entry revisions), and add a deterministic reverse-completion test for two same-key fast saves plus the full-save interaction.
Automated hermes-sweeper review.
| save_lock = threading.Lock() | ||
| self._save_lock = save_lock | ||
| try: | ||
| with save_lock: |
There was a problem hiding this comment.
snap_gen is only read, never advanced for fast-path mutations. Two same-key fast saves can therefore share it; if the newer UPSERT wins first, a delayed older snapshot still passes this strict > check and overwrites the newer entry_json. Please add an ordering mechanism for per-entry writes and a forced reverse-completion regression test.
bb7e695 to
ed481e6
Compare
…saves The steady-state turn only bumps updated_at/last_prompt_tokens on one routing entry, but persisted it through the full index rewrite twice per turn (get_or_create_session's healthy-path bump + update_session): every entry re-serialized, DELETE+INSERT of every gateway_routing row, and a multi-MB sessions.json dump+fsync — ~50ms p50 at ~1,100 routing keys in production, out of ~175ms total per-turn gateway persistence. Metadata-only saves now UPSERT the single row via the existing HermesDB.save_gateway_routing_entry (<1ms). Structural transitions (create/recover/reset/switch/prune, compression-tip heals) keep the full rewrite, which also refreshes the legacy sessions.json mirror. Correctness: each fast save allocates a per-entry revision from the routing generation counter under _lock, so fast and full snapshots are totally ordered by number. Under _save_lock the UPSERT is skipped when a newer full snapshot or a newer fast save of the same key has already persisted, and a delayed full rewrite folds in fast records serialized after its snapshot before writing — an older snapshot can never overwrite a newer one, in either direction. update_session snapshots peer fields under _lock so a concurrent reset cannot record a torn peer row; no DB or a failed UPSERT falls back to the full rewrite so DB-less installs keep sessions.json durable every turn.
ed481e6 to
b6087a3
Compare
|
Good catch — the fast path could indeed let a delayed older metadata UPSERT land over a newer one, since two same-key fast saves shared snap_gen and the guard only compared against full-snapshot generations. Fixed by giving fast saves a durable per-entry ordering: each _save_entry now allocates a revision from the same routing generation counter under the session lock at serialize time, so fast and full snapshots are totally ordered by number. Under the save lock the UPSERT is skipped when a newer full snapshot or a newer fast save of the same key has already persisted, and the reverse interaction is handled in _persist_routing_data — a delayed full rewrite folds in fast records serialized after its snapshot before writing (state.db and the sessions.json mirror), so an older snapshot can never overwrite a newer one in either direction. The fast path stays allocation-light: one counter increment plus a dict slot that reuses the already-serialized entry_json. Added the requested deterministic tests (a lock-gate wrapper parks a writer between its serialize point and its durable write): reverse-order completion of two same-key fast saves keeps the newer entry_json; a delayed older full rewrite preserves a later fast save; a delayed fast save skips after a newer full rewrite. The first two fail on the previous implementation. Rebased onto current main. |
|
@teknium1 Gentle ping — all points from the review here have been addressed (summary in the comment above), the branch is rebased on current main, and CI is green. Ready for another look whenever convenient. |
|
Thanks @Soju06 — excellent work on the risky part of this PR class: the shared-counter revision ordering and the fold-in logic both held up under independent interleaving analysis, and the field-completeness audit confirmed no data loss (same to_dict serializer both paths). Verified (16/16 new tests, mutation-checked, seam audit clean) and salvaged into #76916 with your authorship preserved via cherry-pick, plus one small follow-up extracting a single allocator for the shared counter so the two bump sites can't drift. Closing in favor of the salvage. |
Review follow-up on the #64169 salvage: _save_entry duplicated _snapshot_routing_locked's counter-bump line verbatim. The stale-write protection is a total order over ONE counter — extract _next_routing_generation_locked() so the two allocation sites can't drift apart silently.
Review follow-up on the NousResearch#64169 salvage: _save_entry duplicated _snapshot_routing_locked's counter-bump line verbatim. The stale-write protection is a total order over ONE counter — extract _next_routing_generation_locked() so the two allocation sites can't drift apart silently.
Review follow-up on the NousResearch#64169 salvage: _save_entry duplicated _snapshot_routing_locked's counter-bump line verbatim. The stale-write protection is a total order over ONE counter — extract _next_routing_generation_locked() so the two allocation sites can't drift apart silently.
Review follow-up on the NousResearch#64169 salvage: _save_entry duplicated _snapshot_routing_locked's counter-bump line verbatim. The stale-write protection is a total order over ONE counter — extract _next_routing_generation_locked() so the two allocation sites can't drift apart silently.
Problem
On a production deployment with ~1,100 gateway routing keys, per-turn gateway persistence (session resolve + post-turn save) measured ~175ms p50. Profiling attributed ~50ms of that to the routing-index save: every
SessionEntryre-serialized, a fullDELETE+INSERTof everygateway_routingrow in state.db, and a multi-MBsessions.jsondump+fsync — and the steady-state turn pays it twice (once inget_or_create_session's healthy-pathupdated_atbump, once inupdate_session), even though both writes only changeupdated_at/last_prompt_tokenson a single entry.Change
Add
SessionStore._save_entry(session_key): persist one routing entry via the existingHermesDB.save_gateway_routing_entryUPSERT instead of rewriting the whole index. Only the two metadata-only call sites use it:get_or_create_session's healthy path (existing entry, no reset/recover/heal — just theupdated_atbump)update_session(updated_at/last_prompt_tokens)Structural transitions — create, recover, reset, session switch, prune, and compression-tip heals (anything that changes the key → session_id mapping) — keep the full-rewrite path, which also refreshes the legacy
sessions.jsonmirror. Between structural saves the mirror can lag in metadata only; state.db is the primary durable store for routing, so restart rebinding is unaffected.update_sessionalso now runs its SQLite write and peer-record update outside_lock, so the commit never blocks concurrent routing lookups.Correctness notes
_locktogether with the current routing generation. Under_save_lock, the UPSERT is skipped when a full snapshot taken after our serialize point has already persisted — that snapshot necessarily contains a same-or-newer copy of the key, so writing ours would regress it. A full snapshot older than our serialize point that lands after us can only regress that key's metadata by one racing turn (the next turn rewrites it), never the session_id: session_id changes always carry a newer generation and win via the existing guard in_persist_routing_data.update_sessionsnapshotssession_id/origin/display_namewhile still holding_lock, so a concurrent reset that rewrites the entry between lock release and the peer record cannot record a mix of old and new fields._heal_compression_tip_locked's return value now gates the fast path — a heal rewritesentry.session_idand must reach thesessions.jsonmirror too.sessions.json— their primary store — durable every turn.Tests
New
tests/gateway/test_routing_save_fast_path.py(13 tests): changed values always land in state.db; restart rebinding works when thesessions.jsonmirror lagged fast-path writes (or was deleted); compression heals andforce_newtransitions still rewrite the mirror; no-DB and failed-UPSERT fallbacks; peer fields snapshotted under_lock; generation-guard skip/proceed ordering, including restart rebinding after a skipped idempotent write.python -m pytest tests/gateway: 9,124 tests, no regressions vs a clean checkout ofmainin the same environment (the same set of environment-dependent failures — Telegram/PTB and Feishu SDK version drift, path-completion fixtures — fails identically before and after this change).Measured impact
Production deployment of this change at ~1,100 routing keys: metadata-only routing saves dropped from ~50ms (full index rewrite +
sessions.jsondump+fsync, twice per turn) to <1ms per single-row UPSERT. Structural transitions are unchanged.🤖 Generated with Claude Code