Repository navigation
fix(state): log disk state files that fail to load instead of ignoring them - #7631
gaurav0107 wants to merge 2 commits into
Conversation
…g them StateManagerDisk.load_state swallowed every exception, so an existing state file that could not be read looked exactly like a missing one: get_state started from a fresh state and the next write replaced the file, with nothing in the logs. A damaged or unreadable file is now logged as a warning with the traceback. A state class that changed since the file was written is still an expected reset (as in the redis manager) and is logged at debug level. BaseState reports both as StateSchemaMismatchError, so the unpickling cause tells them apart. Closes reflex-dev#7167
[Medium impact] The PR appears safe to merge. SummaryThe disk state manager now reports failed loads without changing how it starts from fresh state.
No new actionable issues were found. No previous review threads were supplied. Reviews (2) · Last reviewed commit: "fix(state): warn on any disk state file ..." · Reviewed by Greptile |
Merging this PR will regress 1 benchmark2 warnings
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
Treat a StateSchemaMismatchError as an expected reset only when it has no chained cause. BaseState._deserialize chains every unpickling failure, so a pickle referencing a missing module is now a warning too, not only a truncated or garbled file. Also assert the token in the warning and tighten the changelog wording.
All Submissions:
Type of change
Description
StateManagerDisk.load_state()caught every exception and returnedNone, so an existing state file that could not be loaded was handled like a missing one.get_state()started from a fresh state and the next write replaced the file, and nothing was logged.This PR keeps the return value (still
None, so the app keeps working from a fresh state, as the redis manager does on a mismatch) but makes the failure visible:warningthat names the token and file path, with the traceback.StateSchemaMismatchErrorwith no chained cause): adebugrecord. This is an expected reset after editing a state class in dev, and a warning here would fire for every stale substate after each edit.BaseState._deserializealso raisesStateSchemaMismatchErrorfor unpickling failures, but chains the original error, so a mismatch counts as an expected reset only when it has no chained cause.I kept this to logging and didn't add a new exception or quarantine the file, because the issue doesn't settle the recovery behavior. If you'd prefer to raise, or to move the unreadable file aside before it gets overwritten, I can change it.
Closes #7167
Changes To Core Features:
Testing
New tests in
tests/units/istate/manager/test_disk.py. All of them fail onmain:load_statereturnsNoneand logs nothingb"not-a-pickle"and an empty file for both a plainStateTokenand aBaseStateToken, plus a pickle that references a missing module): oneWARNINGthat names the token and path and carriesexc_info, andget_statestill returns a fresh stateDEBUGrecord, no warning, andget_statereturns defaultsCommands run:
uv run pytest tests/units/istate/manager: 75 passeduv run pytest tests/units --cov --no-cov-on-fail --cov-report=: the only 3 failures also fail onmainin my environment (two dataeditor tests neednode, onereflex_benchHMR port test)uv run pre-commit run --files <changed files>(ruff format/check, codespell, pyright, ty): all pass🤖 Opened with Superhuman, an open-source contribution agent.