fix: heal session_model_usage PK unconditionally to restore token/cost accounting (#73823)

[RelaxJonh]

Summary

Fixes #73823

Installs whose state.db reached schema_version >= 22 before the task dimension was added to session_model_usage carry a 5-column PRIMARY KEY. The existing migration is gated behind if current_version < 22: which is unreachable once the version bumps — so the PK rebuild never runs, and every token/cost write fails permanently and silently.

Root cause

1. SCHEMA_SQL declares the 6-column PK (session_id, model, billing_provider, billing_base_url, billing_mode, task) — only applied to new databases.
2. The migration at line 3782 that rebuilds the table with the correct PK is gated behind if current_version < 22:.
3. _reconcile_columns() ADDs a bare nullable task column via ALTER TABLE, but SQLite cannot ALTER a primary key.
4. Result: the task column exists but is NOT in the PK. Every upsert in _record_model_usage() fails with "ON CONFLICT clause does not match any PRIMARY KEY or UNIQUE constraint", aborting the enclosing write transaction and zeroing all token/cost data.

Fix

Add an idempotent _heal_session_model_usage_pk() method modeled on the existing _heal_gateway_routing_pk() pattern. It:

• Runs unconditionally on every database open (not version-gated)
• Detects the legacy 5-column PK via PRAGMA table_info (checks if task is in the PK)
• Rebuilds the table with the correct 6-column composite key
• Uses INSERT OR IGNORE to avoid IntegrityError on any theoretical row collision
• Is a no-op on healthy databases where task is already in the PK

Changes

• hermes_state.py: Added _heal_session_model_usage_pk() method + call in _init_schema()

Test plan

• Verify syntax passes
• On healthy DB: healer is a no-op (task already in PK)
• On affected DB (5-column PK + schema_version >= 22): table rebuilt, token accounting resumes
• Idempotent: running twice doesn't crash

[ly6751]

Code Review: PR #73838 — heal session_model_usage PK unconditionally

Bug Analysis - Correctly Identified

The session_model_usage table requires a 6-column PK (session_id, model, billing_provider, billing_base_url, billing_mode, task) for its upsert in _record_model_usage(). For databases that reached schema_version >= 22 before the v22 migration was deployed, the existing version-gated migration (if current_version < 22:) never runs. _reconcile_columns() adds the task column (ALTER TABLE) but cannot change the PK so task exists as a bare column outside the PK and every ON CONFLICT on the 6-column key fails silently.

Fix Assessment

1. Pattern correctness: Follows the exact same structure as _heal_gateway_routing_pk() — PRAGMA table_info detection, early return if healthy, rename, recreate, INSERT OR IGNORE, drop legacy, rebuild indexes. Established pattern eliminates risk.

2. Detection logic: SELECT COUNT(*) FROM pragma_table_info(...) WHERE name = 'task' AND pk > 0 correctly distinguishes "column exists AND is in PK" (healthy, no-op) from "column exists but NOT in PK" (affected, rebuild).

3. Data safety: INSERT OR IGNORE prevents IntegrityError on theoretical PK collisions. Old rows assigned task = '' (correct for pre-task-dimension usage).

4. Idempotency: After one rebuild, task is in the PK and subsequent opens are no-ops.

5. Error handling: try/except at every SQLite call. Table-not-found, concurrent-failure, and other edge cases all degrade gracefully.

6. Index completeness: Both idx_session_model_usage_session and idx_session_model_usage_model are recreated after the rebuild.

7. Calling site: Inserted in _init_schema() after _reconcile_columns() and _heal_gateway_routing_pk() — correct ordering.

Merge Recommendation

Approve — correct, safe, and follows established patterns.

[MatheusBazarinRibeiro]

Whoa, that was fast — thank you! 🙌 And the shape is spot on: unconditional healer modeled on _heal_gateway_routing_pk, right down to the _legacy_pk rename. That's exactly what worked when I tested it locally.

One thing I hit though, and I'd much rather flag it before this merges than after 👇

Foreign keys need to be off for the copy — INSERT OR IGNORE won't save you

session_model_usage.session_id has an FK to sessions with ON DELETE CASCADE, and real databases accumulate orphan rows — rows whose session_id no longer has a matching sessions row.

The catch: SQLite's OR IGNORE conflict clause covers UNIQUE / NOT NULL / CHECK / PRIMARY KEY violations — but not foreign-key violations. Those raise no matter what the conflict clause says.

So on any connection that sets PRAGMA foreign_keys=ON before _init_schema, the copy hits an IntegrityError on the first orphan row, the rebuild aborts, and the heal silently never completes. Which is... the same failure mode this PR is fixing, just moved somewhere else 😅

What worked for me: wrap the rebuild in a single transaction with foreign_keys temporarily OFF, restoring the pragma after. The toggle has to sit outside the BEGIN/COMMIT — it's a no-op inside an open transaction.

How I confirmed it

Throwaway DB seeded to the exact broken shape (5-column PK + an ALTER-added nullable task), with one orphan row present:

	result
FK enforcement on	copy raised, table left unhealed ❌
FK-off window	both rows survived, 6-column PK in place, both indexes recreated, no _legacy_pk residue ✅

And enforcement really was restored afterwards — a follow-up orphan insert correctly raised IntegrityError.

Two smaller notes

Call site ordering. Worth double-checking it lands after _reconcile_columns(), since that's what re-ADDs the bare nullable task column. Healer running first on an unreconciled DB might be keying on a column that isn't there yet.

Detection has to be cheap. On installs where the PK is already correct, the detect path is the whole story — it needs to be a genuine no-op, not a rebuild. On a multi-GB state.db an accidental rebuild-every-open would be extremely noticeable 😬

---

Happy to test your branch against a database that's already in the broken state if that helps — that's the annoying case to synthesize and I've got one sitting right here.

Nice work getting this out so quickly 👏

[teknium1]

Thanks for isolating a real stale-schema path. Current main still gates the v22 table rebuild at hermes_state_schema.py:503, while the live writer uses a six-column conflict target at hermes_state.py:4353-4361.

Problems

• The copy at PR hermes_state.py:3646 uses INSERT OR IGNORE, but SessionDB enables foreign keys before _init_schema() (hermes_state.py:1916). SQLite does not apply OR IGNORE to foreign-key violations. The PR has no FK-off/restore window and catches only OperationalError at PR hermes_state.py:3669, so an orphaned legacy row can abort the rebuild as noted in the existing discussion.
• The schema code moved to hermes_state_schema.py in 21c7ae8563; this patch needs a thoughtful port to SessionSchemaMixin rather than the old monolith location.
• No regression test covers the v22+-marked stale-PK state. Existing coverage sets the version to 21 (tests/hermes_state/test_aux_usage_accounting.py:101-139).

Suggested changes

• Port the healer into hermes_state_schema.py, make the rebuild atomic while temporarily managing FK enforcement, and add stale-v22+, orphan-row, healthy no-op, and repeat-open regressions.

Automated hermes-sweeper review.

[teknium1]

INSERT OR IGNORE does not suppress FOREIGN KEY violations. SessionDB enables PRAGMA foreign_keys=ON before _init_schema() on current main, so an orphaned legacy session_id aborts this copy; disable and restore FK enforcement outside an atomic rebuild transaction, and add an orphan-row regression.

[teknium1]

Merged via salvage PR #75883 (#75883) — your unconditional _heal_session_model_usage_pk() approach landed, authored under your name, ported to the current SessionSchemaMixin location with an FK-off/restore window around the rebuild and stale-v22+ regression tests. Fixes #73823 — token/cost accounting now heals on installs stuck at schema v22+. Thanks!