fix(trace): serialize V2GitStore storer access under StorerMu#52
Merged
Conversation
Adds a jj-style operation log (cli/oplog), the only record of trace's own ref mutations — previously the sole safety net was git's own reflog, which isn't queryable via trace and never covers the shadow-branch SetReference rewrites rewind/cleanup perform directly (those don't touch HEAD). Entries are stored on the existing trace/checkpoints/v1 orphan branch alongside checkpoints, so they sync the same way via the already-working push/fetch/merge path. 'trace undo' reverts the most recent recorded operation; 'trace log' lists the operation history.
runUndo restored only the git ref via SetReference for every operation type. That's correct for rewind/cleanup (shadow-branch-only rewrites that never touch HEAD), but wrong for reset-hard: git reset --hard moves the ref, index, and working tree together, so undoing it via SetReference alone left files on disk in the post-reset state while the branch pointer claimed otherwise. Now reuses performGitResetHard for OpResetHard entries, the same code path the original operation used.
go-git's filesystem storer is not safe for concurrent read+write, even across separate Repository instances sharing the same .git directory. The v1 GitStore path already wraps every operation in StorerMu; the v2 path touched the storer directly and never acquired it. Lock every public V2GitStore method that reads or writes the storer. Because several public methods call other public methods under their own lock, extract private *Locked helpers and route internal callers through them to avoid deadlock (WriteCommitted->WriteCommittedWithSessionIndex, rotateGeneration->NextGenerationNumber->ListArchivedGenerations, ReadGenerationFromRef->ReadGeneration, ReadSessionContentByID/GetSessionLog-> ReadCommitted, and the private rotateCurrentIfNeeded->rotateGeneration/ CountCheckpointsInTree paths). The pending-rotation path is intentionally left unlocked: it uses a separate .lock file (withPendingFullGenerationPublicationLock) and never touches the git storer.
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
Wraps every public V2GitStore method that touches the git storer in StorerMu, mirroring the v1 GitStore convention. Required private *Locked impl extraction for methods involved in intra-class public-to-public calls to avoid deadlocking the non-reemd mutex (caught 2 deadlocks via the test suite during development: inside rotateCurrentIfNeeded and fullRefSearchOrder -> ListArchivedGenerations).
Why
The v1 path holds StorerMu at ~30 sites because go-git storer is not concurrency-safe. V2GitStore never acquired it, risking git object corruption in the core checkpoint feature. Audit finding.
Verification