fix(document): apply transactions atomically#10275
Open
kirangadhave wants to merge 2 commits into
Open
Conversation
Invalid cell references could surface as raw `KeyError`s, while a failure after earlier changes had been applied could leave the document partially mutated. Validate targets and anchors in batch order, then stage changes on an isolated copy so failed transactions preserve cells and versions.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
No issues found across 2 files
Architecture diagram
sequenceDiagram
participant Client as Notebook Client
participant API as Document.apply(tx)
participant Validate as _validate()
participant Staged as Staged NotebookDocument
participant Live as Live State
Note over Client,Live: Transaction Application Flow
Client->>API: apply(tx)
Note over API,Validate: Validation Phase (NEW: comprehensive checks)
API->>Validate: _validate(tx.changes, self._cells)
Validate->>Validate: Build live_ids from current cells
Validate->>Validate: Track claimed_ids, deleted, moved, updated
loop For each change
alt CreateCell
Validate->>Validate: Check cell_id not in claimed_ids
Validate->>Validate: Check anchor (before/after) exists in live_ids
Validate->>Validate: Add cell_id to live_ids and claimed_ids
else DeleteCell
Validate->>Validate: Check cell_id not already deleted or moved
Validate->>Validate: Check cell_id exists in live_ids
Validate->>Validate: Remove cell_id from live_ids
else MoveCell
Validate->>Validate: Check cell_id not deleted
Validate->>Validate: Check cell_id exists in live_ids
Validate->>Validate: Check anchor exists in live_ids and not self
Validate->>Validate: Check anchor not referencing itself
else SetCode/SetName/SetCellConfig
Validate->>Validate: Check cell_id not deleted
Validate->>Validate: Check cell_id exists in live_ids
end
end
alt Validation fails
Validate-->>API: Raise ValueError
API-->>Client: Raise ValueError (document unchanged)
else All valid
Validate-->>API: OK
Note over API,Staged: Staging Phase (NEW: isolated copy)
API->>Staged: Create deep copy of current cells
Staged-->>API: Staged copy
Note over API,Staged: Apply Phase (NEW: staged only)
loop For each change
API->>Staged: _apply_change(change)
end
alt Application fails (e.g., RuntimeError)
Staged-->>API: Exception
API-->>Client: Raise exception (document unchanged)
else All changes applied
API->>API: next_version = self._version + 1
API->>API: Create return Transaction with next_version
Note over API,Live: Commit Phase (NEW: atomic swap)
API->>Live: Replace self._cells with staged._cells
API->>Live: Set self._version = next_version
Live-->>API: Complete
API-->>Client: Return applied Transaction
end
end
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes NotebookDocument.apply() transactional/atomic: it validates targets/anchors up front, stages changes, and only commits the new state if the full transaction succeeds—preventing partial mutation on invalid references or unexpected apply-time failures.
Changes:
- Added batch-order validation for missing targets/anchors and intra-transaction conflicts, raising
ValueError(instead of surfacing rawKeyError). - Implemented staging + commit in
NotebookDocument.apply()to ensure atomic document updates. - Added regression tests covering missing targets/anchors, self-anchored moves, create-then-reference within a batch, and rollback on failures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
marimo/_messaging/notebook/document.py |
Adds stricter validation and stages transactions so document state updates commit atomically. |
tests/_messaging/notebook/test_document.py |
Expands validation and atomicity test coverage to prevent regressions. |
Atomic staging deep-cloned every cell and config, making common single-cell edits allocate in proportion to notebook size. Shallow-copy the ordered list and clone only cells that are updated so failed transactions remain isolated without full-document allocation overhead.
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.
📝 Summary
Make
NotebookDocument.apply()atomic when validation or change application fails. Previously, invalid cell references could surface as rawKeyErrors, and an unexpected failure after earlier changes had been applied could leave cells or versions partially mutated.Validate targets and anchors in batch order, then apply changes to an isolated copy and replace live state only after the complete transaction succeeds. This preserves create-then-reference behavior within a batch while ensuring failed transactions leave the document unchanged.
Add focused regression coverage for missing targets and anchors, self-anchored moves, earlier-created anchors, validation rollback, and unexpected execution failures.
Closes MO-6621