Restore displaced and current models when a model load fails - #257
Merged
Conversation
ModelReader.read_model for serializers 4-7 builds the new model in the live system: parse_dir creates a model (making it current) and RenameParser renames it to its saved name immediately during parsing, displacing a same-named existing model to <name>_BAK<n>. On a failed read, only the half-built model was closed: the user's model stayed stranded under its backup name, and cur_model() was left None even though the previously current model still existed, so the next current-model-dependent API call silently created a brand-new model. Snapshot the system's models and current model before reading and restore both in the except path after closing the half-built model (modelx/serialize/reader_state.py, shared by serializers 4-6; serializer 7 inherits 6). Successful reads are unchanged: the displaced model intentionally keeps its backup name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
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
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.
What changed
ModelReader.read_model(serializers 4-7) now restores the system's pre-existing models when a load fails partway:RenameParserruns atAT_PARSEand renames the half-built model to its saved name immediately during parsing, displacing a same-named existing model to<name>_BAK<n>. Previously, a failed read closed only the half-built model, leaving the user's model stranded under the backup name with nothing under the original name.parse_dir'smx.new_model()makes the half-built model current, andclose_modelnullscurrentmodelinstead of restoring the prior one — so after any failed read,mx.cur_model()wasNoneand the next current-model-dependent call (e.g.mx.defcells,mx.new_space) silently created a brand-new model.The fix is shared: a new
modelx/serialize/reader_state.pydefinesSystemStateSnapshot, which recordssystem.modelsandsystem.currentmodelbefore each read; theexceptpaths of serializer 4/5/6 (7 inherits 6) callstate.restore()after closing the half-built model and rolling back the IOManager journal (#256). Restoration is guarded by identity checks so it only renames back a model this read displaced, and only when the original name is free.Successful reads are unchanged: the displaced model intentionally keeps its
_BAKname (covered by the existingtest_restore_model_leave_old).Tests
New
modelx/tests/serialize/test_read_error_recovery.py(16 tests, versions 4-7). The failure is produced without mocks by truncating a space file mid-statement in a copy of the compat fixtures, so the load aborts after the displacing rename — the worst-case failure point. Scenarios: displaced model restored by identity under its original name, priorcur_model()restored, no-prior-model reads left untouched, and the success path still displacing to_BAK.Without the fix, 8 of the 16 fail (both defects, all four versions); with it, the full suite is green (1144 passed, 6 skipped).
Notes for review
1ab0bcf), which restructured the sameexceptblocks;state.restore()runs after that fix'sclose()+rollback_journalblock, beforeraise.🤖 Generated with Claude Code