fix(undo): correct undo-of-undo for DELETE via history tombstone trigger#53
Merged
Conversation
…tone The archive trigger on history-enabled workspaces' source tables previously fired only on UPDATE or DELETE, so INSERTs were invisible to the history table. That asymmetry breaks undo-of-undo for DELETE operations: the undo of a DELETE is an INSERT, which produced no fresh history row, leaving the undo log entry with no way to distinguish its "restore-to-prior-state" target from its "I just restored that state" semantic. Reversing such an entry no-ops instead of re-deleting. Extend the trigger to fire on BEFORE INSERT OR UPDATE OR DELETE. The INSERT branch writes a tombstone history row from NEW.* with operation='create' -- a self-describing record that the row was reached via INSERT, distinguishable from edit/rename/delete states. The history.operation CHECK constraint is extended to allow 'create' (no 'undo' value: history captures physical state transitions, while 'undo' is a meta-op realized as one of the four physical operations). For new workspaces, GenerateHistorySQL emits the updated trigger directly. For workspaces that haven't yet run the relational-directories migration, the existing migration regenerates the trigger from GenerateHistorySQL and picks up the new DDL automatically. One visible side effect: .history/<file>/ now lists one extra version (the create tombstone) for every file. Tests that assert specific version counts have been updated.
ExecuteUndoSingle and ExecuteUndo previously treated every type=undo log
entry uniformly: "restore from version_id." This was correct for undo of
edit/rename/delete (the version_id captured the right state), but wrong
for undo of a CREATE -- where the undo entry's version_id points to the
delete-history row with the same content as the current restored state,
making "restore" a no-op when the correct semantic is to re-delete.
Two coordinated fixes:
1. ExecuteUndoTransaction now captures version_id for the DELETE branch
too. Previously the DELETE branch (used when undoing a CREATE) wrote
undo log entries without version_id, leaving them unanalyzable by the
new dispatcher. With the BEFORE-DELETE trigger already firing during
the undo's DELETE, a fresh history row exists; the function now reads
its version_id and stores it on the new undo entry.
2. The case "undo" branch in both ExecuteUndoSingle and ExecuteUndo now
looks up the history row's operation column via QueryHistoryOperation
(new query, added to LogWriter interface) and dispatches by it:
- operation='create' (tombstone): the undo entry restored a deleted
row; reverse it by re-deleting.
- operation='edit'/'rename'/'delete': the existing "restore from
version_id" path produces the right state; keep it.
Legacy undo log entries from before the tombstone trigger landed have
version_ids pointing to old delete-history rows (operation='delete').
Their dispatch falls back to the existing restore behavior with a
logged warning. After the relational-directories migration regenerates
the trigger and new operations are performed, the legacy entries are
no longer relevant.
…types TestSynth_UndoOfUndo_DeleteRestores reproduces the user-reported scenario directly: create a file, delete it, undo the delete (file comes back), undo the undo (file must be gone). Without the polarity fix this would leave the file alive; the test confirms it's correctly re-deleted. TestSynth_UndoOfUndo_AllOpTypes covers the polarity round-trip for the other three op types -- create, edit, rename -- each running forward-op -> undo -> undo-of-undo and asserting the state matches post-forward-op. All four op types now round-trip correctly. TestSynth_History_IncludesCreateEvent confirms the BEFORE-INSERT trigger writes a tombstone history row: a fresh create yields one history version, an edit adds a second, and so on. This is the visible side effect of the tombstone change at the .history/<file>/ listing surface. TestSynth_HistoryMultipleVersions had to be bumped from 3 to 4 versions to account for the create tombstone -- the test creates a file and edits it 3 times, which under the new trigger produces 4 history rows (1 create + 3 pre-update captures) instead of 3.
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
1 participant
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
operation='create'tombstone row. This gives undo log entries a self-describing history row regardless of which forward op they reversed.case "undo"inExecuteUndoSingle/ExecuteUndoby readinghistory.operationfor the entry'sversion_id:create-> re-DELETE;edit/rename/delete-> existing restore path.version_idfor the DELETE branch inExecuteUndoTransactionso that undo-of-CREATE entries are subsequently undoable (previously they were emitted without a version_id, leaving the new dispatcher blind).Bug
Real-world undo-of-undo over an atomic-rename Write triplet (
create temp,delete target,rename temp -> target) landed in an inconsistent state and surfaced as macOS NFS "RPC struct is bad". Two coupled issues:case "undo"was treated as "restore from version_id." Correct for undo of edit/rename/delete (a fresh BEFORE-trigger history row captures the right state). Wrong for undo of DELETE: the undo's reversing INSERT fired no trigger, so the only available history row was the one written by the original DELETE -- "restore from that" is a no-op when the correct semantic is to re-delete.ExecuteUndoTransaction's DELETE branch (the path used when undoing a CREATE) inserted undo log entries with no version_id at all, making them unanalyzable by any future dispatcher.Fix
synth.GenerateHistorySQLnow emitsBEFORE INSERT OR UPDATE OR DELETE; the INSERT branch writes a tombstone history row fromNEW.*withoperation='create'. The history CHECK constraint allows'create'(no'undo'-- history captures physical state transitions; undo is realized as one of the four physical ops).QueryHistoryOperation(ctx, schema, history, version_id)returns the operation column for O(1) on the PK. Thecase "undo"branches in bothExecuteUndoandExecuteUndoSingleswitch on it.Migration
No new named migration.
addParentPointerMigrationalready regenerates the trigger viaGenerateHistorySQL, so workspaces that have not yet run the post-0.6 migration pick up the new DDL automatically. Fresh workspaces get it directly. Per testimony, no demo workspace has run any post-0.6 migration yet.Caveats
.history/<file>/now lists one extra version (the create tombstone) per file.TestSynth_HistoryMultipleVersionswas bumped from 3 to 4 to match.