fix(sessions): heal missing last_read_at on read-only GET /api/sessions opens - #80047
Closed
ygd58 wants to merge 1 commit into
Closed
fix(sessions): heal missing last_read_at on read-only GET /api/sessions opens#80047ygd58 wants to merge 1 commit into
ygd58 wants to merge 1 commit into
Conversation
…ns opens Fixes NousResearch#80037. Read-only SessionDB opens skip _init_schema()/_reconcile_columns() entirely (for good reason: the healthy read path must never take a write lock or request a checkpoint). To handle this for existing databases, _open_session_db_for_profile() already runs a stale-schema probe query (_SESSION_DB_READ_PROBE_SQL) against a handful of columns dashboard read paths query; if it raises "no such column"/"no such table", it opens one writable connection (running the real migration via _reconcile_columns()) before reopening read-only. last_read_at (added for the session read/unread state feature) was never added to this probe list. So on an existing pre-feature database, opening GET /api/sessions' read-only listing connection right after an update never touched the probe's actually-checked columns (archived/pinned/active/compacted, all already present) -- the probe query itself SUCCEEDED, meaning the self-healing branch never triggered. The actual failure only surfaced deeper inside list_sessions_rich()'s own broader SELECT, by which point it's a plain 500 rather than a healable stale-schema signal. The migration only ran whenever something ELSE (e.g. creating a new session) happened to open a write connection first -- exactly matching the issue's reported symptom (empty session list until the first new session). Added last_read_at to the probe SQL, plus a maintenance comment noting any future dashboard-queried column needs the same treatment. Extended the EXISTING parametrized stale-schema healing test (test_get_sessions_heals_stale_schema_store) with a last_read_at case, reusing the established test infrastructure that already covers archived/pinned. Verified as a genuine regression by reverting the probe fix and confirming the new test case fails with the EXACT reported traceback (sqlite3.OperationalError: no such column: s.last_read_at, at the same list_sessions_rich() call site). 3/3 pass in the parametrized test; 145/146 across the full test_web_server.py file plus test_session_read_state.py (1 pre-existing unrelated skip; no regression).
alt-glitch
added
type/bug
Contributor
Author
|
#80797 (merged) supersedes this structurally -- it derives the probe from SCHEMA_SQL via _parse_schema_columns() instead of hand-maintaining a list, which correctly kills the class rather than the instance. My fix (extending the hand-written list with last_read_at) was exactly the maintenance treadmill teknium correctly flagged in the review: the next ADD COLUMN would have gone stale again. Also covers the profiles.py read-only sidebar paths my PR did not touch. Closing in favor of #80797. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #80037.
Root cause
Read-only
SessionDBopens skip_init_schema()/_reconcile_columns()entirely (the healthy read path must never take a write lock). To handle this for existing databases,_open_session_db_for_profile()already runs a stale-schema probe query against a handful of columns dashboard read paths query -- if it raises "no such column", it opens one writable connection (running the real migration) before reopening read-only.last_read_at(added for the session read/unread state feature) was never added to this probe list. So on an existing pre-feature database, the probe query itself SUCCEEDED (checking only already-present columns), meaning self-healing never triggered -- the failure only surfaced deeper insidelist_sessions_rich()'s own broader SELECT as a plain 500. The migration only ran whenever something else (e.g. creating a new session) opened a write connection first -- exactly matching the reported symptom.Fix
Added
last_read_atto the probe SQL, plus a maintenance comment for future columns.Verification
Extended the EXISTING parametrized stale-schema healing test with a
last_read_atcase. Verified as a genuine regression by reverting the fix and confirming the new test case fails with the exact reported traceback.3/3 pass in the parametrized test; 145/146 across the full
test_web_server.pyfile plustest_session_read_state.py(1 pre-existing unrelated skip; no regression).