fix(undo): capture version_id inline to avoid cascade-artifact pollution#58
Merged
Conversation
ExecuteUndoTransaction restores a set of affected files in three phases: Step 1 DELETEs rows that were created after the undo target point; Step 2 UPSERTs rows back to a history-snapshot state for edits/renames/ deletes; Step 3 inserts a new type='undo' log entry per affected file, each carrying a version_id pointer at the history row written by Step 1/Step 2's mutation. That pointer is what a future undo-of-undo dispatches on -- it must resolve to the row written by the actual mutation (tombstone for a recreate, rename/edit/delete OLD-state row for the others), not to any other row in this file_id's history. Step 3 captured that version_id by querying the history table for each file's "newest row" -- SELECT version_id FROM <history> WHERE file_id= $1 ORDER BY version_id DESC LIMIT 1 -- AFTER all Step 1/2 mutations were done. That post-hoc lookup is wrong when Step 2 touches a parent dir: the bump_parent_mtime AFTER trigger cascades on every child restore, firing the archive trigger again on the parent and writing a no-semantic-content edit row. That cascade row becomes "newest" for the parent's file_id, so Step 3 records IT as the parent's undo-log version_id instead of the parent's own restore snapshot. A subsequent undo-of-undo restores from a snapshot whose filename matches current state -- a no-op. The user-visible symptom: roll forward after an undo-to-savepoint silently drops a directory rename while correctly re-applying child renames in the same batch. Move the version_id capture inside the Step 1 (DELETE) and Step 2 (UPSERT) loops, immediately after each file's own mutation, before control returns to ExecuteUndoTransaction. At that instant the file's own snapshot is guaranteed-newest in its history (row-level BEFORE trigger, one row per UPSERT/DELETE, no other SQL between mutation and capture, PG18 per-session monotonic uuidv7). Stash the captured version_id in a per-file map; Step 3 reads from the map instead of re-querying. Correct regardless of iteration order, cascade direction, or future triggers added on the source table -- the invariant is local to each iteration rather than depending on the cumulative state of history at end-of-transaction. Adds TestSynth_UndoOfUndo_DirRenameWithChild (integration, reproduces the user's reported bug end-to-end) and TestExecuteUndoTransaction_CapturesVersionIDInline_NotPostHoc (unit, exercises the cascade with seeded triggers). Both verified to fail pre-fix and pass post-fix.
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.
ExecuteUndoTransaction restores a set of affected files in three phases: Step 1 DELETEs rows that were created after the undo target point; Step 2 UPSERTs rows back to a history-snapshot state for edits/renames/ deletes; Step 3 inserts a new type='undo' log entry per affected file, each carrying a version_id pointer at the history row written by Step 1/Step 2's mutation. That pointer is what a future undo-of-undo dispatches on -- it must resolve to the row written by the actual mutation (tombstone for a recreate, rename/edit/delete OLD-state row for the others), not to any other row in this file_id's history.
Step 3 captured that version_id by querying the history table for each file's "newest row" -- SELECT version_id FROM WHERE file_id= $1 ORDER BY version_id DESC LIMIT 1 -- AFTER all Step 1/2 mutations were done. That post-hoc lookup is wrong when Step 2 touches a parent dir: the bump_parent_mtime AFTER trigger cascades on every child restore, firing the archive trigger again on the parent and writing a no-semantic-content edit row. That cascade row becomes "newest" for the parent's file_id, so Step 3 records IT as the parent's undo-log version_id instead of the parent's own restore snapshot. A subsequent undo-of-undo restores from a snapshot whose filename matches current state -- a no-op. The user-visible symptom: roll forward after an undo-to-savepoint silently drops a directory rename while correctly re-applying child renames in the same batch.
Move the version_id capture inside the Step 1 (DELETE) and Step 2 (UPSERT) loops, immediately after each file's own mutation, before control returns to ExecuteUndoTransaction. At that instant the file's own snapshot is guaranteed-newest in its history (row-level BEFORE trigger, one row per UPSERT/DELETE, no other SQL between mutation and capture, PG18 per-session monotonic uuidv7). Stash the captured version_id in a per-file map; Step 3 reads from the map instead of re-querying.
Correct regardless of iteration order, cascade direction, or future triggers added on the source table -- the invariant is local to each iteration rather than depending on the cumulative state of history at end-of-transaction.
Adds TestSynth_UndoOfUndo_DirRenameWithChild (integration, reproduces the user's reported bug end-to-end) and TestExecuteUndoTransaction_CapturesVersionIDInline_NotPostHoc (unit, exercises the cascade with seeded triggers). Both verified to fail pre-fix and pass post-fix.