Skip to content

fix(state): four session-state fixes — safe close tracking, flush-cursor class fix, row-retry, usage-PK healer - #75883

Merged
teknium1 merged 6 commits into
mainfrom
salvage/sqlite-session-state
Aug 1, 2026
Merged

fix(state): four session-state fixes — safe close tracking, flush-cursor class fix, row-retry, usage-PK healer#75883
teknium1 merged 6 commits into
mainfrom
salvage/sqlite-session-state

Conversation

@teknium1

@teknium1 teknium1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Salvage: four session-state / SQLite fixes (sqlite-session-state)

Combines four community session-state/SQLite fixes onto current main.

Fixes #75629, Fixes #73823
Closes #75699, Closes #75170, Closes #74934, Closes #73838

Credit: @trippyogi, @spfcraze, @Dannou, @RelaxJonh — all four commits carry the original authors' authorship (cherry-picked or --author).

1. TrackedConnection.close() ordering (#75699, @trippyogi) — cherry-picked as-is

hermes_cli/sqlite_safe_read.py untracked the connection before calling super().close(). A raising close (e.g. cross-thread ProgrammingError) left the FD open while has_live_connection() said false → the byte-probe advisory-lock guard could green-light a probe against a live connection (#75629). Now closes first, untracks only on success. Merge-ready; picked verbatim with the author's tests.

2. Flush-scan cursor invalidation at the marker-pop site (#75170, @spfcraze) + sibling-site class fix

The bounded flush-scan in _flush_messages_to_session_db_unlocked skips the identity-matched prefix of its previous snapshot, on the documented assumption that no code path pops _DB_PERSISTED_MARKER from a live dict in place.

  • Cherry-picked: @spfcraze's fix at the finalize_turn fill site (agent/turn_finalizer.py) — without it the filled final answer was identity-skipped and content='' persisted; /resume replayed an empty reply (the Gateway can deliver assistant response without persisting assistant row #43849/Telegram: assistant responses not persisted to session DB (model re-answers old messages) #44100 class resurfacing via the perf cursor).
  • Follow-up commit (fix-the-class, per the never-patch-predicates standing order): the micro-compaction defrag pass (agent/context_compressor.py::_defrag_rolling_summary) is the one sibling site that also pops the marker from a live dict in place (rewrites the newest MICRO marker's content). The compressor holds no agent reference, so the pop site raises _flush_scan_cursor_invalidated and the finalize_turn micro-compaction block consumes it, setting agent._db_flush_scan_prefix = None.
  • Class inventory: the only other _DB_PERSISTED_MARKER pops (context_compressor.py ~175/224) operate on fresh copies — identity-breaking by construction, so the cursor already re-scans them; deliberately left alone.

3. 'no more rows available' retry gap (port of #74934, @Dannou/Dannoob)

Under dual gateway/agent WAL contention (FTS5 trigram sync holding the write lock on large appends), the SQLite engine raises a transient no more rows available error whose class varies by build — some surface it as InterfaceError, a sibling of DatabaseError, which escaped both retry branches in _execute_write on attempt 0 and killed the turn as session_persistence_failed.

The PR's diff predates the deadline-patience rewrite (8da8a78), so its constants changes (60 retries / 300 ms jitter / 2.0 s engine timeout) were intentionally not carried over — they're superseded by the patience budget. Fresh commit (authored as Dannoob, the email on pr-74934's commit) implementing the same idea against current main:

  • extracted the jittered-sleep-within-deadline logic into a shared _sleep_before_retry helper (behavior-preserving for locked/busy),
  • message-scoped retry of 'no more rows available' from OperationalError, DatabaseError (checked before the FTS-corruption rebuild path so it isn't misrouted), and a sqlite3.Error catch-all,
  • anything else propagates untouched on attempt 0.

Tests: transient InterfaceError retried to success; unrelated InterfaceError propagates immediately; DatabaseError variant retried; exhausted patience surfaces the original error.

4. Unconditional session_model_usage PK heal (salvage of #73838, @RelaxJonh)

Installs whose state.db reached schema_version >= 22 before the task dimension landed carry a 5-column PK on session_model_usage. The reconciler ADDs task outside the key (SQLite can't ALTER a PK) and the version-gated v22 rebuild is unreachable (current_version < 22 already false) → every _record_model_usage upsert fails with an ON CONFLICT mismatch, aborting the enclosing transaction — token/cost accounting permanently dead (#73823).

Salvaged with the triage fix-ups:

  • ported to SessionSchemaMixin in hermes_state_schema.py (schema code moved out of hermes_state.py in 21c7ae8; the PR targeted the old location),
  • rebuild wrapped in a PRAGMA foreign_keys=OFF/ON window — the connection enables FKs before _init_schema, and INSERT OR IGNORE does not suppress FK violations, so a single orphaned usage row would have aborted the heal,
  • COALESCE('') on the nullable reconciler-added task (and billing columns) during the copy,
  • stale-v22+ regression tests: PK rebuilt + upsert restored on a DB whose version row is already current, orphan rows survive the FK window, healthy-DB no-op/idempotence, no _legacy_pk leftover table.

Verification

  • Full sweep green: tests/state/, tests/test_hermes_state.py (137), tests/test_sqlite_lock_safe_inspection.py, tests/agent/test_micro_compaction.py, tests/agent/test_turn_finalizer_final_response_persistence.py, tests/run_agent/test_identity_flush.py — 216+ tests passing.
  • Sabotage runs (fix reverted → new tests FAIL → fix restored) performed for every new regression test:
    • sibling-site defrag tests: 3/3 fail with the flag/finalizer wiring stashed;
    • no-more-rows tests: both retry tests fail with the hermes_state.py fix stashed (message-scoped propagation tests correctly still pass);
    • PK-heal tests: 3/4 fail with hermes_state_schema.py stashed (leftover-table test vacuously passes without the heal, as expected).
  • Note: local linked SQLite is 3.50.4 (WAL-reset-bug fallback → journal_mode=DELETE), so tests ran in DELETE mode; no WAL-only assertions were added.
  • Branch rebased onto current main (991f5f1e9e) immediately before push; full suite re-run post-rebase.

Infographic

PR infographic

@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

૮ >ﻌ< ა ci review

ran on 6795a59

ℹ️ Info

Desktop E2E visual evidence · View test artifacts · View job

3 visual diffs.

inline evidence is publishing...

@teknium1 teknium1 added area/sessions Session lifecycle, resume, persistence, history area/usage-cost Token accounting, usage reporting, billing, cost tracking labels Aug 1, 2026
@teknium1
teknium1 force-pushed the salvage/sqlite-session-state branch from 9bde241 to ae426d2 Compare August 1, 2026 05:49
trippyogi and others added 6 commits July 31, 2026 23:07
A failed close left the FD open while the byte-probe guard thought
nothing was live. Keep the registry entry until close actually works.
The bounded flush-scan in _flush_messages_to_session_db_unlocked skips
the identity-matched prefix of its previous snapshot, on the documented
assumption that no code path pops _DB_PERSISTED_MARKER from a live dict
in place. finalize_turn's pure-tool-call-tail fill is exactly that path:
it pops the marker so the filled content gets re-persisted — but the
cursor then skips the row anyway, so the delivered final response never
reaches state.db and /resume replays content="" (the #43849/#44100
class resurfacing via the perf cursor). Invalidate the cursor at the
pop site so the filled row is re-examined.
…ling site

The micro-compaction defrag pass (_defrag_rolling_summary) rewrites the
newest MICRO marker's content and pops _DB_PERSISTED_MARKER from the
LIVE dict in place — the same in-place pop class finalize_turn's fill
site was fixed for in #75170. Without invalidation the bounded
flush-scan cursor identity-skips the rewritten marker row and the
defragged rolling summary never reaches state.db (resume rehydrates a
stale summary).

The compressor holds no agent reference, so the pop site raises
_flush_scan_cursor_invalidated and the finalize_turn micro-compaction
block consumes it, setting agent._db_flush_scan_prefix = None.

The module-scope pop sites (context_compressor.py:175/224) operate on
fresh copies — identity-breaking by construction — and need no flag.

Follow-up to #75170 (fix-the-class sweep of _DB_PERSISTED_MARKER
in-place pops).
…e3.Error classes

Under dual gateway/agent WAL contention (FTS5 trigram sync holding the
write lock on large appends) the SQLite engine can raise a transient
'no more rows available' error. The exception CLASS varies with the
build — some surface it as InterfaceError, a SIBLING of DatabaseError —
so it escaped both existing retry branches in _execute_write on attempt
0 and killed the turn as session_persistence_failed even though the
identical write succeeds standalone.

Port of #74934 onto the deadline-patience rewrite (8da8a78): the
PR's attempt-counted constants (60 retries / 300ms jitter / 2.0s engine
timeout) predate that rewrite and are superseded by the patience
budget, so they are intentionally NOT carried over. Instead the check
is message-scoped and rides the existing deadline/patience loop:

- extract the jittered-sleep-within-deadline logic into a shared
  _sleep_before_retry helper (behavior-preserving for locked/busy)
- retry 'no more rows available' from OperationalError, DatabaseError
  (checked BEFORE the FTS-corruption rebuild path so it is not
  misrouted), and a message-scoped sqlite3.Error catch-all
- any other error in any class propagates untouched on attempt 0

Tests: transient InterfaceError retried to success; unrelated
InterfaceError propagates immediately; DatabaseError variant retried;
exhausted patience surfaces the original error.
…ken/cost accounting

Installs whose state.db reached schema_version >= 22 before the task
dimension was added carry a 5-column PRIMARY KEY on
session_model_usage. The column reconciler ADDs task as a bare
nullable, but SQLite cannot ALTER a primary key, and the version-gated
v22 rebuild is unreachable (current_version < 22 already false), so
the composite 6-column key never lands. Every upsert in
_record_model_usage then fails with 'ON CONFLICT clause does not match
any PRIMARY KEY or UNIQUE constraint', aborting the enclosing write
transaction — token/cost accounting permanently dead (#73823).

Add an idempotent _heal_session_model_usage_pk() modeled on
_heal_gateway_routing_pk(), run unconditionally from _init_schema on
every open. Salvaged from #73838 with fix-ups:

- ported to SessionSchemaMixin in hermes_state_schema.py (the schema
  code moved out of hermes_state.py in 21c7ae8; the PR targeted the
  old location)
- rebuild wrapped in a PRAGMA foreign_keys=OFF/ON window: the
  connection enables FKs before _init_schema and OR IGNORE does NOT
  suppress FK violations, so a single orphaned usage row (session
  pruned while accounting was broken) would have aborted the heal
- COALESCE('') on the nullable reconciler-added task column (and the
  billing columns) during the copy
- stale-v22+ regression tests: rebuilt PK + restored upsert, orphan
  rows survive the FK window, healthy-DB no-op, no legacy leftover

Fixes #73823
…lper; add contributor mappings

The rebase onto main's extracted _sleep_before_write_retry() method left
three call sites pointing at the dropped local helper; rewire them.
Also adds contributors/emails mappings (Dannou, trippyogi, spfcraze).
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 area/usage-cost Token accounting, usage reporting, billing, cost tracking

Projects

None yet

5 participants