Skip to content

Commit b15f32d

Browse files
allocate edit session numbers monotonically across a recovery
Row 4505376427 (DiffViewProvider.ts:655). Allocation and installation were the same number: open() advanced the field and attemptRollbackRecovery() assigned the recovered record's older value into it, with only the acquisition-rejection path putting the previous value back. A settled recovery therefore left the provider holding an earlier attempt's stamp, and the next open() re-issued a number a retained record already held. Path and edit type say nothing about which attempt a record describes - a retry reopens the same file as the same kind of edit - so the settled-acquisition comparison accepted that record against the new install and rolled the newer edit back with the older attempt's baseline while reporting a successful recovery. Allocation now has its own counter that nothing but open() writes: an install wears a stamp and a recovery may wear an older one, and neither puts a number back into circulation. The guards themselves are unchanged - same fields, same comparison, same refusal - only the source of the stamp moved. The test runs two attempts at one file, rolls the first back through its record, opens the file a third time and asserts the new install keeps its edit when the second attempt record is offered back: the recovery refuses, and nothing is restored or saved out of the live edit. Control: with the previous head's file installed the third open re-issues the number - the assertion fails as expected 2 to be greater than 2 - and restoring the fix is byte-equal. Focused suite at this change: 109 passed. Local Stryker preflight before this push: not run.
1 parent 8b95a8e commit b15f32d

2 files changed

Lines changed: 77 additions & 1 deletion

File tree

‎src/integrations/editor/DiffViewProvider.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,15 @@ export class DiffViewProvider {
118118
// thing that tells two edits on one provider apart: relPath and editType say what is being edited,
119119
// not which attempt is doing it, so a retry that reopens the same file as the same kind of edit
120120
// looks exactly like the install a retained recovery record describes.
121+
// This is the install stamp, not the allocator: a recovery performs an older attempt's edit and so wears
122+
// that attempt number, which can put the number back below one a later open() already handed out.
123+
// Allocation itself lives in sessionAllocator below, which nothing but open() ever writes.
121124
private editSession = 0
125+
// Allocation counter for those stamps: advanced by open() and never assigned by anything else, so a
126+
// stamp is issued once. If the counter rewound along with a recovery's install, the next open() would
127+
// hand out a number a retained record already holds, and that record - same file, same kind of edit -
128+
// would then read as its own install and roll back an edit it has nothing to do with.
129+
private sessionAllocator = 0
122130
private taskRef: WeakRef<Task>
123131

124132
constructor(
@@ -132,7 +140,8 @@ export class DiffViewProvider {
132140
// A new edit session starts here, even when the target and the edit type are the same as the last
133141
// one: this install's baseline, directories and buffer are its own, and any recovery record captured
134142
// from an earlier install is now answerable to a different attempt.
135-
const openSession = ++this.editSession
143+
const openSession = ++this.sessionAllocator
144+
this.editSession = openSession
136145
this.relPath = relPath
137146
const fileExists = this.editType === "modify"
138147
const absolutePath = path.resolve(this.cwd, relPath)
@@ -681,6 +690,8 @@ export class DiffViewProvider {
681690
// The install is this recovery performing the edit the record describes, so it wears that record
682691
// session: the comparison afterwards asks whether this install is still what the provider answers
683692
// to, and only a stamp that a later open() moved tells it when the target and the type stayed put.
693+
// Wearing the number is not the same as putting it back into circulation: sessionAllocator keeps its
694+
// own value, so the next open() issues a stamp no retained record can already hold.
684695
this.editSession = context.editSession
685696
this.createdDirs = [...context.createdDirs]
686697
this.originalContent = context.originalContent

‎src/integrations/editor/__tests__/DiffViewProvider.spec.ts‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2986,6 +2986,71 @@ describe("DiffViewProvider", () => {
29862986
expect(diffViewProvider["originalContent"]).toBe("the retry's own baseline")
29872987
expect(diffViewProvider["activeDiffEditor"]).toBe(retryEditor)
29882988
})
2989+
it("never re-issues a session number after a recovery rewinds the install", async () => {
2990+
const relPath = "mock-recovery-target.ts"
2991+
const absolutePath = `${mockCwd}/${relPath}`
2992+
// Stubbed the way the rest of this suite stubs it: these cases are about which install a record is
2993+
// allowed to roll back, not about the tab plumbing or the acquisition timer. Each attempt gets its
2994+
// own editor so an installed buffer can be told from another attempt's.
2995+
const acquired: Array<{ editor: ReturnType<typeof makeTextEditor> }> = []
2996+
diffViewProvider["openDiffEditor"] = vi.fn().mockImplementation(async () => {
2997+
const editor = makeTextEditor({
2998+
document: makeTextDocument({
2999+
uri: makeUri(absolutePath),
3000+
getText: vi.fn().mockReturnValue("streamed, never approved"),
3001+
isDirty: true,
3002+
}),
3003+
})
3004+
acquired.push({ editor })
3005+
return editor
3006+
})
3007+
diffViewProvider["closeAllDiffViews"] = vi.fn().mockResolvedValue(undefined)
3008+
diffViewProvider["restorePreviewTabs"] = vi.fn().mockResolvedValue(undefined)
3009+
diffViewProvider["keepOrCloseEditedFile"] = vi.fn().mockResolvedValue(undefined)
3010+
diffViewProvider["restorePreStreamBuffer"] = vi.fn().mockResolvedValue(undefined)
3011+
diffViewProvider["saveBufferClean"] = vi.fn().mockResolvedValue(undefined)
3012+
diffViewProvider.editType = "modify"
3013+
3014+
// Two attempts at the same file, each record captured while its own install was the live one.
3015+
await diffViewProvider.open(relPath)
3016+
const firstRecord = recoveryContextFor(relPath, "modify")
3017+
const firstSession = firstRecord.editSession
3018+
await diffViewProvider.reset()
3019+
await diffViewProvider.open(relPath)
3020+
const secondRecord = recoveryContextFor(relPath, "modify")
3021+
const secondSession = secondRecord.editSession
3022+
await diffViewProvider.reset()
3023+
expect(secondSession).toBe(firstSession + 1)
3024+
3025+
// The first attempt rollback runs: it installs the first attempt edit and rewinds the provider to that
3026+
// attempt stamp.
3027+
await expect(diffViewProvider.attemptRollbackRecovery(firstRecord)).resolves.toBe(true)
3028+
expect(diffViewProvider["editSession"]).toBe(firstSession)
3029+
await diffViewProvider.reset()
3030+
3031+
// A third attempt on the same file. Allocation has to stay ahead of every stamp a retained record
3032+
// already holds, otherwise the second record describes this install as well as it describes its own.
3033+
await diffViewProvider.open(relPath)
3034+
const thirdSession = diffViewProvider["editSession"]
3035+
const thirdEditor = acquired[acquired.length - 1].editor
3036+
expect(thirdSession).toBeGreaterThan(secondSession)
3037+
const thirdRestore = vi.fn().mockResolvedValue(undefined)
3038+
const thirdSave = vi.fn().mockResolvedValue(undefined)
3039+
diffViewProvider["restorePreStreamBuffer"] = thirdRestore
3040+
diffViewProvider["saveBufferClean"] = thirdSave
3041+
3042+
await expect(diffViewProvider.attemptRollbackRecovery(secondRecord)).resolves.toBe(false)
3043+
3044+
// The second record belongs to an attempt that already finished its rollback question. The retry in
3045+
// front of it keeps its own edit: nothing was restored, nothing was saved, and its stamp, target and
3046+
// buffer are still what the provider answers to.
3047+
expect(thirdRestore).not.toHaveBeenCalled()
3048+
expect(thirdSave).not.toHaveBeenCalled()
3049+
expect(diffViewProvider["isEditing"]).toBe(true)
3050+
expect(diffViewProvider["relPath"]).toBe(relPath)
3051+
expect(diffViewProvider["editSession"]).toBe(thirdSession)
3052+
expect(diffViewProvider["activeDiffEditor"]).toBe(thirdEditor)
3053+
})
29893054
})
29903055
// The acquisition inside openDiffEditor() races a ten-second timer against a showTextDocument it cannot
29913056
// cancel. Both cases below are about what that leaves behind: a diff nobody is waiting for, and a diff

0 commit comments

Comments
 (0)