From 62e28ce784abda6bfeaabb41234692601accc2ca Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 19 Jul 2026 21:17:36 -0700 Subject: [PATCH] fix(editor): give self-move stamps per-registration expiries + retract tokens The refcount model used a single shared expiry scalar per role that only grew via max() and reset at refs=0, so releasing the max-contributing registration left survivors inheriting an over-extended window (and an expired registration could be resurrected by a later stamp on a key the opposite role kept resident). Both over-suppress genuine changes. Model each stamp as an independent registration carrying its own expiry (a list per role). recordSelfMove returns a ticket; clearSelfMove retracts exactly that registration. A role is live while any of its registrations is unexpired, so concurrent stamps, failed-move clears, and expiry are all precise. Wrapper/helper thread the tickets through. Adds regressions for the over-extension and resurrection cases. Co-authored-by: Orca --- .../editor/editor-self-move-registry.test.ts | 51 ++++-- .../editor/editor-self-move-registry.ts | 151 +++++++++--------- .../record-self-move-for-open-tabs.test.ts | 4 +- .../src/lib/record-self-move-for-open-tabs.ts | 21 ++- .../src/lib/rename-open-editor-tabs-path.ts | 4 +- 5 files changed, 131 insertions(+), 100 deletions(-) diff --git a/src/renderer/src/components/editor/editor-self-move-registry.test.ts b/src/renderer/src/components/editor/editor-self-move-registry.test.ts index c591d20e42a..7ec0cd58cc9 100644 --- a/src/renderer/src/components/editor/editor-self-move-registry.test.ts +++ b/src/renderer/src/components/editor/editor-self-move-registry.test.ts @@ -71,8 +71,8 @@ describe('editor-self-move-registry', () => { }) it('clears the roles a move stamped when the rename fails', () => { - recordSelfMove('/repo/old.md', '/repo/new.md') - clearSelfMove('/repo/old.md', '/repo/new.md') + const ticket = recordSelfMove('/repo/old.md', '/repo/new.md') + clearSelfMove(ticket) expect(isRecentSelfMoveSource('/repo/old.md')).toBe(false) expect(isRecentSelfMoveTarget('/repo/new.md')).toBe(false) @@ -84,9 +84,9 @@ describe('editor-self-move-registry', () => { // second fails (destination now exists) and clears. The successful move's // target stamp must survive so its delayed echo is still recognized. recordSelfMove('/one/report.md', '/dest/report.md') // move A - recordSelfMove('/two/report.md', '/dest/report.md') // move B (will fail) + const ticketB = recordSelfMove('/two/report.md', '/dest/report.md') // move B (fails) - clearSelfMove('/two/report.md', '/dest/report.md') // B's failure clear + clearSelfMove(ticketB) expect(isRecentSelfMoveTarget('/dest/report.md')).toBe(true) // B's own source is gone; A's source is untouched. @@ -94,12 +94,41 @@ describe('editor-self-move-registry', () => { expect(isRecentSelfMoveSource('/one/report.md')).toBe(true) }) - it('clear fully releases a target once every registration is cleared', () => { - recordSelfMove('/one/report.md', '/dest/report.md') - recordSelfMove('/two/report.md', '/dest/report.md') + it('clearing a stamp does not over-extend a concurrent stamp’s expiry', () => { + // Move A stamps /dest at t=0 (expires 750). Move B stamps the same target at + // t=700 (expires 1450) and fails at t=710. Clearing B must not leave /dest + // live past A's own 750ms promise — a single shared scalar expiry would. + recordSelfMove('/one/report.md', '/dest/report.md', undefined, 750) // A @ t=0 + vi.advanceTimersByTime(700) + const ticketB = recordSelfMove('/two/report.md', '/dest/report.md', undefined, 750) // B @ t=700 + vi.advanceTimersByTime(10) + clearSelfMove(ticketB) // B fails @ t=710 - clearSelfMove('/one/report.md', '/dest/report.md') - clearSelfMove('/two/report.md', '/dest/report.md') + vi.advanceTimersByTime(50) // t=760, past A's 750 promise + expect(isRecentSelfMoveTarget('/dest/report.md')).toBe(false) + }) + + it('does not resurrect an expired registration when a later stamp reuses the key', () => { + // A→P (target) expires, but a reverse move keeps P's map entry resident as a + // source. A later failed move on P must not revive the expired target refs. + recordSelfMove('/x.md', '/p.md', undefined, 750) // P target, expires 750 + recordSelfMove('/p.md', '/q.md', undefined, 3000) // P source, keeps entry alive + vi.advanceTimersByTime(1000) // P's target registration has expired + + const failed = recordSelfMove('/y.md', '/p.md', undefined, 750) // P target again + clearSelfMove(failed) + + expect(isRecentSelfMoveTarget('/p.md')).toBe(false) + // The still-live reverse-move source is untouched. + expect(isRecentSelfMoveSource('/p.md')).toBe(true) + }) + + it('clear fully releases a target once every registration is cleared', () => { + const ticketA = recordSelfMove('/one/report.md', '/dest/report.md') + const ticketB = recordSelfMove('/two/report.md', '/dest/report.md') + + clearSelfMove(ticketA) + clearSelfMove(ticketB) expect(isRecentSelfMoveTarget('/dest/report.md')).toBe(false) }) @@ -108,9 +137,9 @@ describe('editor-self-move-registry', () => { // Move A → B, then C → A. Clearing the failed C → A must not drop A's source // role from the successful A → B move. recordSelfMove('/repo/A.md', '/repo/B.md') - recordSelfMove('/repo/C.md', '/repo/A.md') + const ticketC = recordSelfMove('/repo/C.md', '/repo/A.md') - clearSelfMove('/repo/C.md', '/repo/A.md') + clearSelfMove(ticketC) expect(isRecentSelfMoveSource('/repo/A.md')).toBe(true) expect(isRecentSelfMoveTarget('/repo/A.md')).toBe(false) diff --git a/src/renderer/src/components/editor/editor-self-move-registry.ts b/src/renderer/src/components/editor/editor-self-move-registry.ts index 4080dda1346..c7f2ac09838 100644 --- a/src/renderer/src/components/editor/editor-self-move-registry.ts +++ b/src/renderer/src/components/editor/editor-self-move-registry.ts @@ -23,22 +23,28 @@ export const SELF_MOVE_REMOTE_TTL_MS = 3000 // valve — TTL pruning is the real bound, since stamps self-expire within seconds. const SELF_MOVE_MAX_STAMPS = 1024 -// Why: a path can hold BOTH roles at once — e.g. move A→B then immediately undo -// B→A leaves A a live source (of the first move, whose delayed watcher echo may -// still be in flight) and a live target (of the undo) — and it can hold the SAME -// role from two concurrent moves (drag two files named report.md into one dir → -// both stamp that dir's report.md as a target). So each role is reference -// counted: a role is live while any un-cleared registration remains and its -// expiry is in the future. Refcounting lets a FAILED move's clear drop only its -// own registration instead of erasing a concurrent successful move's stamp. -type RoleState = { - refs: number - expiresAt: number +// Why: each stamp is one independent registration carrying its own expiry, held +// as a list per role. This keeps concurrent registrations of the same path+role +// (drag two files named report.md into one dir → both stamp that dir's +// report.md as a target) fully separate, so: +// - a role is live while ANY of its registrations is unexpired, +// - `clearSelfMove` retracts EXACTLY the registration a failed move added (by +// its ticket's expiry), never a concurrent move's, +// - an expired registration can't be "resurrected" by a later stamp on the +// same key, because liveness is computed per registration, not from a shared +// scalar. (A single shared expiry would over-extend the suppression window +// when the max-contributing registration is cleared.) +type SelfMoveStamp = { + source: number[] + target: number[] } -type SelfMoveStamp = { - source: RoleState - target: RoleState +/** A retraction handle for one recorded move — see {@link clearSelfMove}. */ +export type SelfMoveTicket = { + fromPath: string + toPath: string + runtimeEnvironmentId: string | null + expiresAt: number } const stamps = new Map() @@ -47,15 +53,19 @@ function selfMoveKey(absolutePath: string, runtimeEnvironmentId?: string | null) return `${runtimeEnvironmentId?.trim() || 'client'}::${normalizeAbsolutePathForComparison(absolutePath)}` } -function isRoleLive(state: RoleState, now: number): boolean { - return state.refs > 0 && now <= state.expiresAt +function dropExpiredAndMaybeDelete(key: string, stamp: SelfMoveStamp, now: number): boolean { + stamp.source = stamp.source.filter((expiresAt) => now <= expiresAt) + stamp.target = stamp.target.filter((expiresAt) => now <= expiresAt) + if (stamp.source.length === 0 && stamp.target.length === 0) { + stamps.delete(key) + return true + } + return false } function pruneExpiredSelfMoves(now: number): void { for (const [key, stamp] of stamps) { - if (!isRoleLive(stamp.source, now) && !isRoleLive(stamp.target, now)) { - stamps.delete(key) - } + dropExpiredAndMaybeDelete(key, stamp, now) } } @@ -69,25 +79,39 @@ function enforceSelfMoveStampLimit(): void { } } -function emptyStamp(): SelfMoveStamp { - return { source: { refs: 0, expiresAt: 0 }, target: { refs: 0, expiresAt: 0 } } -} - -function stampRole( +function pushRegistration( absolutePath: string, role: 'source' | 'target', runtimeEnvironmentId: string | null | undefined, expiresAt: number ): void { const key = selfMoveKey(absolutePath, runtimeEnvironmentId) - const next = stamps.get(key) ?? emptyStamp() - const state = next[role] - state.refs += 1 - state.expiresAt = Math.max(state.expiresAt, expiresAt) + const stamp = stamps.get(key) ?? { source: [], target: [] } + stamp[role].push(expiresAt) // Why: delete+set keeps insertion recency fresh so the cap evicts genuinely // old stamps first. stamps.delete(key) - stamps.set(key, next) + stamps.set(key, stamp) +} + +function removeRegistration( + absolutePath: string, + role: 'source' | 'target', + runtimeEnvironmentId: string | null | undefined, + expiresAt: number +): void { + const key = selfMoveKey(absolutePath, runtimeEnvironmentId) + const stamp = stamps.get(key) + if (!stamp) { + return + } + const index = stamp[role].indexOf(expiresAt) + if (index >= 0) { + stamp[role].splice(index, 1) + } + if (stamp.source.length === 0 && stamp.target.length === 0) { + stamps.delete(key) + } } /** @@ -95,20 +119,33 @@ function stampRole( * delete(old)+create(new) echo as self-initiated instead of an external change. * Both endpoints are stamped: the source so the delete does not tombstone the * tab, the target so the create does not raise a changed-on-disk banner on the - * (re-homed, still-dirty) tab. + * (re-homed, still-dirty) tab. Returns a ticket to retract this exact stamp with + * {@link clearSelfMove} if the move turns out not to happen. */ export function recordSelfMove( fromPath: string, toPath: string, runtimeEnvironmentId?: string | null, ttlMs: number = SELF_MOVE_TTL_MS -): void { +): SelfMoveTicket { const now = Date.now() pruneExpiredSelfMoves(now) const expiresAt = now + ttlMs - stampRole(fromPath, 'source', runtimeEnvironmentId, expiresAt) - stampRole(toPath, 'target', runtimeEnvironmentId, expiresAt) + pushRegistration(fromPath, 'source', runtimeEnvironmentId, expiresAt) + pushRegistration(toPath, 'target', runtimeEnvironmentId, expiresAt) enforceSelfMoveStampLimit() + return { fromPath, toPath, runtimeEnvironmentId: runtimeEnvironmentId ?? null, expiresAt } +} + +/** + * Retracts exactly the registration a {@link recordSelfMove} added, so a rename + * that FAILED after stamping does not keep suppressing genuine watcher events + * for the paths it never moved. A concurrent move's registration on the same + * path+role is untouched. + */ +export function clearSelfMove(ticket: SelfMoveTicket): void { + removeRegistration(ticket.fromPath, 'source', ticket.runtimeEnvironmentId, ticket.expiresAt) + removeRegistration(ticket.toPath, 'target', ticket.runtimeEnvironmentId, ticket.expiresAt) } function hasLiveRole( @@ -122,12 +159,10 @@ function hasLiveRole( return false } const now = Date.now() - const live = isRoleLive(stamp[role], now) - // Why: drop the whole entry once neither role is live so expired stamps don't - // linger until the next write's prune. - if (!isRoleLive(stamp.source, now) && !isRoleLive(stamp.target, now)) { - stamps.delete(key) - } + const live = stamp[role].some((expiresAt) => now <= expiresAt) + // Why: drop expired registrations on read so they can't be counted live and + // don't linger until the next write's prune. + dropExpiredAndMaybeDelete(key, stamp, now) return live } @@ -147,44 +182,6 @@ export function isRecentSelfMoveSource( return hasLiveRole(absolutePath, 'source', runtimeEnvironmentId) } -function clearRole( - absolutePath: string, - role: 'source' | 'target', - runtimeEnvironmentId: string | null | undefined -): void { - const key = selfMoveKey(absolutePath, runtimeEnvironmentId) - const stamp = stamps.get(key) - if (!stamp) { - return - } - const state = stamp[role] - // Why: drop only THIS move's registration (one ref). A concurrent move that - // stamped the same role keeps the role live via its own ref. - state.refs = Math.max(0, state.refs - 1) - if (state.refs === 0) { - state.expiresAt = 0 - } - const now = Date.now() - if (!isRoleLive(stamp.source, now) && !isRoleLive(stamp.target, now)) { - stamps.delete(key) - } -} - -/** - * Drops one registration of the roles a move stamped, so a rename that FAILED - * after stamping does not keep suppressing genuine watcher events for the paths - * it never actually moved. Reference counted, so a concurrent move that stamped - * the same role keeps that role live. - */ -export function clearSelfMove( - fromPath: string, - toPath: string, - runtimeEnvironmentId?: string | null -): void { - clearRole(fromPath, 'source', runtimeEnvironmentId) - clearRole(toPath, 'target', runtimeEnvironmentId) -} - export function __clearSelfMoveRegistryForTests(): void { stamps.clear() } diff --git a/src/renderer/src/lib/record-self-move-for-open-tabs.test.ts b/src/renderer/src/lib/record-self-move-for-open-tabs.test.ts index 711f38fd32d..acbed4c91ce 100644 --- a/src/renderer/src/lib/record-self-move-for-open-tabs.test.ts +++ b/src/renderer/src/lib/record-self-move-for-open-tabs.test.ts @@ -75,8 +75,8 @@ describe('recordSelfMoveForOpenTabs', () => { { suppressActiveRuntimeFallback: true } ) - recordSelfMoveForOpenTabs('/repo/docs/readme.md', '/repo/notes/readme.md') - clearSelfMoveForOpenTabs('/repo/docs/readme.md', '/repo/notes/readme.md') + const tickets = recordSelfMoveForOpenTabs('/repo/docs/readme.md', '/repo/notes/readme.md') + clearSelfMoveForOpenTabs(tickets) expect(isRecentSelfMoveSource('/repo/docs/readme.md')).toBe(false) expect(isRecentSelfMoveTarget('/repo/notes/readme.md')).toBe(false) diff --git a/src/renderer/src/lib/record-self-move-for-open-tabs.ts b/src/renderer/src/lib/record-self-move-for-open-tabs.ts index ebd345a64d7..df4e8bc3c8e 100644 --- a/src/renderer/src/lib/record-self-move-for-open-tabs.ts +++ b/src/renderer/src/lib/record-self-move-for-open-tabs.ts @@ -3,7 +3,8 @@ import { getConnectionIdForFile } from '@/lib/connection-context' import { clearSelfMove, recordSelfMove, - SELF_MOVE_REMOTE_TTL_MS + SELF_MOVE_REMOTE_TTL_MS, + type SelfMoveTicket } from '@/components/editor/editor-self-move-registry' import { isPathInsideOrEqual } from './remap-open-editor-tabs-for-path-change' import type { OpenFile } from '@/store/slices/editor' @@ -49,15 +50,19 @@ function collectOpenTabMoves(fromPath: string, toPath: string): OpenTabMove[] { * before/after stamps and the failure clear. This is exported for that wrapper * (and tests); call it directly only when you own the rename lifecycle. */ -export function recordSelfMoveForOpenTabs(fromPath: string, toPath: string): void { - for (const move of collectOpenTabMoves(fromPath, toPath)) { +export function recordSelfMoveForOpenTabs(fromPath: string, toPath: string): SelfMoveTicket[] { + return collectOpenTabMoves(fromPath, toPath).map((move) => recordSelfMove(move.file.filePath, move.newFilePath, move.runtimeOwner, move.ttlMs) - } + ) } -/** Undoes {@link recordSelfMoveForOpenTabs} for a move that did not happen. */ -export function clearSelfMoveForOpenTabs(fromPath: string, toPath: string): void { - for (const move of collectOpenTabMoves(fromPath, toPath)) { - clearSelfMove(move.file.filePath, move.newFilePath, move.runtimeOwner) +/** + * Retracts the exact registrations from {@link recordSelfMoveForOpenTabs} for a + * move that did not happen. Takes the tickets that call returned so a concurrent + * move's stamp on a shared path is never dropped. + */ +export function clearSelfMoveForOpenTabs(tickets: SelfMoveTicket[]): void { + for (const ticket of tickets) { + clearSelfMove(ticket) } } diff --git a/src/renderer/src/lib/rename-open-editor-tabs-path.ts b/src/renderer/src/lib/rename-open-editor-tabs-path.ts index c0cd2f622ab..fe55a783c8b 100644 --- a/src/renderer/src/lib/rename-open-editor-tabs-path.ts +++ b/src/renderer/src/lib/rename-open-editor-tabs-path.ts @@ -23,11 +23,11 @@ export async function renameOpenTabsPathOnDisk( fromPath: string, toPath: string ): Promise { - recordSelfMoveForOpenTabs(fromPath, toPath) + const pendingTickets = recordSelfMoveForOpenTabs(fromPath, toPath) try { await renameRuntimePath(context, fromPath, toPath) } catch (err) { - clearSelfMoveForOpenTabs(fromPath, toPath) + clearSelfMoveForOpenTabs(pendingTickets) throw err } recordSelfMoveForOpenTabs(fromPath, toPath)