fix: drop stale autosaves after newer foreground writes#9239
fix: drop stale autosaves after newer foreground writes#9239mscolnick merged 2 commits intomarimo-team:mainfrom
Conversation
Queued code-mode autosaves currently capture only cells, so a snapshot created before a rename or explicit save can later write into the newer file state. This change tags queued autosaves with the filename and foreground-write generation they were created against, then drops them if a foreground write has already superseded that state. Constraint: Keep the fix narrow and preserve FIFO autosave ordering relative to other autosaves Rejected: Route all foreground saves through SerialTaskRunner | broader runtime/save semantic change than needed for the validated bug Confidence: high Scope-risk: moderate Reversibility: clean Directive: Any future autosave work should preserve the invariant that foreground writes supersede older queued snapshots Tested: uv run --group test pytest tests/_session/extensions/test_notification_listener_autosave.py -q Tested: uv run --group test pytest tests/_server/test_file_manager.py -k 'save_from_cells_persists_cells or save_from_cells_preserves_layout_file or save_and_save_from_cells_serialize_under_lock' -q Tested: uv run ruff check marimo/_session/extensions/extensions.py marimo/_session/notebook/file_manager.py tests/_session/extensions/test_notification_listener_autosave.py Not-tested: Browser-level marimo-pair/code-mode end-to-end flow
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
for more information, see https://pre-commit.ci
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
There was a problem hiding this comment.
Pull request overview
Fixes a stale queued autosave path where an older code-mode autosave snapshot could overwrite newer notebook content after a rename or explicit foreground save.
Changes:
- Add an autosave “target token” (filename + foreground-write generation) to detect stale queued autosaves.
- Capture and pass the token when queueing autosave work; drop autosaves that no longer match current filename/generation.
- Add a regression test covering rename + foreground save ordering vs queued autosave.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
marimo/_session/notebook/file_manager.py |
Introduces autosave generation tracking, capture API, and stale-autosave dropping checks inside save_from_cells. |
marimo/_session/extensions/extensions.py |
Captures autosave target before enqueueing autosave work and passes expected filename/generation to save_from_cells. |
tests/_session/extensions/test_notification_listener_autosave.py |
Updates autosave snapshot tests for new save_from_cells signature and adds a regression test for stale autosave ordering. |
Comments suppressed due to low confidence (1)
marimo/_session/notebook/file_manager.py:478
save_from_cellscan now intentionally return an empty string when the autosave is dropped due toexpected_generation/expected_filenamemismatch, but the docstring still implies it only raises on unnamed notebooks or write failures and otherwise persists. Please update the docstring/return contract to reflect the new “dropped autosave” behavior (and what callers should expect).
"""Persist the notebook from a snapshot of document cells.
Used by the server-side auto-save path for ``code_mode``
mutations. Unlike ``save()``, this takes cells directly — the
caller is responsible for snapshotting ``session.document.cells``
on a thread where the document is quiescent.
Raises:
HTTPException: If the notebook is unnamed or the write fails
"""
| with self._save_lock: | ||
| if self._is_same_path(new_path): | ||
| return new_path.name | ||
|
|
||
| self._assert_path_does_not_exist(new_path) | ||
| self._invalidate_autosaves() | ||
|
|
There was a problem hiding this comment.
_invalidate_autosaves() is called before performing the rename/write. If storage.rename() or _save_file() raises, the foreground operation didn’t actually supersede anything, but already-queued autosaves will now be treated as stale and dropped. Consider invalidating only after the rename + _save_file() succeed (still under _save_lock) so failed renames don’t disable pending autosaves.
| with self._save_lock: | ||
| self.app.update_config(config) | ||
| if self._filename is not None: | ||
| self._invalidate_autosaves() | ||
| return self._save_file( |
There was a problem hiding this comment.
Autosaves are invalidated before _save_file() runs. If _save_file() fails, pending autosaves will be dropped even though no successful foreground write occurred. Consider incrementing the autosave generation only after _save_file() succeeds (while still holding _save_lock).
| if request.persist: | ||
| self._invalidate_autosaves() | ||
| return self._save_file( | ||
| filename_path, | ||
| notebook=self.app.to_ir(), | ||
| persist=request.persist, | ||
| ) |
There was a problem hiding this comment.
Autosaves are invalidated before _save_file() executes. If the foreground save fails (e.g., storage write error), queued autosaves will be considered stale and skipped even though the newer content never reached disk. Consider invalidating only after _save_file() succeeds (still under _save_lock).
| if request.persist: | |
| self._invalidate_autosaves() | |
| return self._save_file( | |
| filename_path, | |
| notebook=self.app.to_ir(), | |
| persist=request.persist, | |
| ) | |
| result = self._save_file( | |
| filename_path, | |
| notebook=self.app.to_ir(), | |
| persist=request.persist, | |
| ) | |
| if request.persist: | |
| self._invalidate_autosaves() | |
| return result |
This pull request was authored by a coding agent.
📝 Summary
Closes #9227.
This prevents a queued code-mode autosave from overwriting newer notebook content after a rename or explicit foreground save.
🔍 Description of Changes
Queued autosaves currently capture only a cells snapshot. If a rename or foreground save happens before that queued work runs, the autosave can still write into the newer file state and revert the notebook to older content.
Root Cause
_maybe_autosave()currently queues onlycells_snapshot.save_from_cells()writes to whateverself._filenameis when the queued work eventually runs.Changes
AppFileManagermade of:This keeps the fix narrow: foreground writes still run synchronously, and FIFO autosave ordering relative to other autosaves is preserved.
Tests
📋 Checklist