Skip to content

Commit e2cb3b2

Browse files
waleedlatif1waleedlatif1
authored
fix(files): fix savingRef mutex integrity race after discard correction (#5554)
* fix(files): fix savingRef mutex integrity race, anchor discard un-suppress to captured target An independent 4-agent audit (beyond the bot review loop) converged on a real race in the round-11 discard-correction fix: - save()'s deferred MIN_SAVING_DISPLAY_MS status timer resolves as a macrotask well after the save's own promise (used to sequence discard's correction) has already settled as a microtask. That timer unconditionally reset savingRef/triggered a trailing resave, even after discard's correction had since claimed the save slot for its own in-flight write — letting a fresh debounced save start concurrently with the correction. Now guarded with `if (inFlightRef.current) return` before touching savingRef, so it only acts when nothing else has claimed the slot since. - The render-time un-suppress check keyed off isDirty, which a stale save's markSavedContent(next) landing after discard can transiently corrupt (overwriting savedContent with the pre-discard value while content has already been reverted), causing a spurious "genuinely new edit" signal. Now keyed off a discardTargetRef captured at discard time instead, which that corruption doesn't touch. Also hardened recovery to key its once-per-mount guard on the specific draft key rather than a bare boolean, so a hypothetical future caller that reuses a hook instance across files would still get correct recovery (today's real callers already remount per file, so this is defense in depth, not a behavior change for any current caller). * fix(files): fix discard status masking and cross-key discard suppression leak Greptile round-1 review on the follow-up fix PR caught 3 real gaps in the discard/correction flow: - The display timer's !discardedRef guard (added to stop a stale save's status update from clobbering a running correction) also silently suppressed the idle-timer reschedule, and discard()'s own correction never set a terminal status on settle — so saveStatus could stick on 'saving' forever after a successful correction, and a failed correction surfaced only via the onDiscardCorrectionFailed callback with no status change. Now the correction's own .then()/.catch() sets 'idle'/'error' once it owns the flow (only when nothing has since un-suppressed discard). - The discard-suppression un-suppress check compared content against the captured discard target, but never reset if a hook instance were reused across draftKeys — a coincidental content match with the previous file's target would keep discard permanently suppressing saves for the new file. Reset discardedRef whenever the effective draftKey changes. * fix(files): re-chain autosave after a discard correction settles Cursor Bugbot found the discard correction's finally() cleared the save mutex but never rechecked for dirty content: an edit made while the correction was in flight bailed out of the debounce effect (savingRef was held) and, since content isn't a savingRef dependency, was never rescheduled once the mutex freed — the edit could sit unsaved indefinitely. The same gap explained a related report that a failed correction which had already been superseded by a newer edit left saveStatus stuck, since nothing else was driving it forward. Fix is one line: call save() in the finally() when content is still dirty. save()'s own guards (savingRef/discardedRef/content-equality) make this safe to call unconditionally, and it naturally hands status ownership to the newer edit's own save cycle. * fix(files): key discard state to raw draftKey, make failed corrections retryable Cursor Bugbot round 3 caught 2 more real gaps: - The document-change reset added last round compared effectiveDraftKey (draftKey gated by enabled), so toggling enabled alone for the SAME document — e.g. a streaming lock — looked identical to switching files. That cleared discardedRef mid-correction, which skipped the correction's own setSaveStatus('error'/'idle') (gated on discardedRef to avoid clobbering a newer edit's status), silently stranding the hook on 'saving' with no visible retry affordance. Now keyed off the raw draftKey, which enabled toggling never touches. - After a failed correction, content already equals savedContent (that's what discard reverted to), so saveImmediately()'s retry going through save() hit its dirty-check and was a complete no-op — the error toast's Retry button did nothing. Extracted the correction logic into runCorrection(target), shared by discard() and by saveImmediately when a failed correction is pending, so retry pushes the reverted baseline again instead of bailing on a check that assumes retries are always for dirty content.
1 parent 896d15b commit e2cb3b2

2 files changed

Lines changed: 441 additions & 23 deletions

File tree

‎apps/sim/hooks/use-autosave.test.tsx‎

Lines changed: 365 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -763,6 +763,371 @@ describe('useAutosave', () => {
763763
await flush()
764764
})
765765

766+
it('does not let the original save leftover status timer clobber a still-running correction', async () => {
767+
const resolvers: Array<() => void> = []
768+
const onSave = vi.fn(
769+
() =>
770+
new Promise<void>((resolve) => {
771+
resolvers.push(resolve)
772+
})
773+
)
774+
const { handle } = renderAutosave({
775+
content: 'a',
776+
savedContent: 'a',
777+
onSave,
778+
draftKey: 'file-discard-9',
779+
})
780+
781+
// A save is in flight when the user discards.
782+
handle.rerender({ content: 'a1' })
783+
await act(async () => {
784+
vi.advanceTimersByTime(1500)
785+
})
786+
await flush()
787+
expect(onSave).toHaveBeenCalledTimes(1)
788+
act(() => handle.discard())
789+
handle.rerender({ content: 'a' })
790+
791+
// The original save resolves; the correction starts (savingRef/inFlightRef now belong to it).
792+
await act(async () => {
793+
resolvers[0]?.()
794+
})
795+
await flush()
796+
expect(onSave).toHaveBeenCalledTimes(2)
797+
798+
// The original save's own MIN_SAVING_DISPLAY_MS timer fires while the correction is still
799+
// unresolved. Without the inFlightRef guard, this used to unconditionally reset savingRef,
800+
// letting a debounced save for a new edit start concurrently with the correction.
801+
await act(async () => {
802+
vi.advanceTimersByTime(600)
803+
})
804+
await flush()
805+
806+
// A new edit made in this window must not be able to schedule a concurrent save while the
807+
// correction (still unresolved) rightfully holds the mutex.
808+
handle.rerender({ content: 'a2' })
809+
await act(async () => {
810+
vi.advanceTimersByTime(1500)
811+
})
812+
await flush()
813+
expect(onSave).toHaveBeenCalledTimes(2)
814+
815+
resolvers[1]?.()
816+
await flush()
817+
})
818+
819+
it('reaches a terminal status once the discard correction settles instead of sticking on saving', async () => {
820+
const resolvers: Array<() => void> = []
821+
const onSave = vi.fn(
822+
() =>
823+
new Promise<void>((resolve) => {
824+
resolvers.push(resolve)
825+
})
826+
)
827+
const { handle } = renderAutosave({
828+
content: 'a',
829+
savedContent: 'a',
830+
onSave,
831+
draftKey: 'file-discard-10',
832+
})
833+
834+
handle.rerender({ content: 'a1' })
835+
await act(async () => {
836+
vi.advanceTimersByTime(1500)
837+
})
838+
await flush()
839+
expect(onSave).toHaveBeenCalledTimes(1)
840+
act(() => handle.discard())
841+
handle.rerender({ content: 'a' })
842+
843+
// The original save resolves; the correction starts. Its own MIN_SAVING_DISPLAY_MS timer
844+
// must not resolve status to 'saved'/'idle' for content the user no longer sees.
845+
await act(async () => {
846+
resolvers[0]?.()
847+
})
848+
await flush()
849+
await act(async () => {
850+
vi.advanceTimersByTime(600)
851+
})
852+
await flush()
853+
expect(onSave).toHaveBeenCalledTimes(2)
854+
expect(handle.status()).toBe('saving')
855+
856+
// The correction itself settles — without this, status stayed on 'saving' forever.
857+
await act(async () => {
858+
resolvers[1]?.()
859+
})
860+
await flush()
861+
expect(handle.status()).toBe('idle')
862+
})
863+
864+
it('surfaces an error if the discard correction itself fails, rather than drifting to idle', async () => {
865+
const resolvers: Array<() => void> = []
866+
const rejecters: Array<(error: Error) => void> = []
867+
let callCount = 0
868+
const onSave = vi.fn(
869+
() =>
870+
new Promise<void>((resolve, reject) => {
871+
callCount += 1
872+
if (callCount === 1) resolvers.push(resolve)
873+
else rejecters.push(reject)
874+
})
875+
)
876+
const onDiscardCorrectionFailed = vi.fn()
877+
const { handle } = renderAutosave({
878+
content: 'a',
879+
savedContent: 'a',
880+
onSave,
881+
draftKey: 'file-discard-11',
882+
onDiscardCorrectionFailed,
883+
})
884+
885+
handle.rerender({ content: 'a1' })
886+
await act(async () => {
887+
vi.advanceTimersByTime(1500)
888+
})
889+
await flush()
890+
act(() => handle.discard())
891+
handle.rerender({ content: 'a' })
892+
893+
await act(async () => {
894+
resolvers[0]?.()
895+
})
896+
await flush()
897+
expect(onSave).toHaveBeenCalledTimes(2)
898+
899+
await act(async () => {
900+
rejecters[0]?.(new Error('network down'))
901+
})
902+
await flush()
903+
expect(onDiscardCorrectionFailed).toHaveBeenCalledTimes(1)
904+
expect(handle.status()).toBe('error')
905+
})
906+
907+
it('does not let discard suppression from a previous file block saves for a newly switched-to file', async () => {
908+
const onSave = vi.fn(async () => {})
909+
const { handle } = renderAutosave({
910+
content: 'a',
911+
savedContent: 'a',
912+
onSave,
913+
draftKey: 'file-x',
914+
})
915+
916+
handle.rerender({ content: 'a1' })
917+
act(() => handle.discard())
918+
handle.rerender({ content: 'a' })
919+
920+
// Switch to a different file whose content coincidentally equals the previous file's
921+
// discard target — without keying discard suppression to draftKey, this would stay
922+
// wrongly suppressed forever, since content would never differ from the stale target.
923+
handle.rerender({ draftKey: 'file-y', savedContent: 'z', content: 'a' })
924+
await act(async () => {
925+
vi.advanceTimersByTime(1500)
926+
})
927+
await flush()
928+
expect(onSave).toHaveBeenCalledTimes(1)
929+
})
930+
931+
it('autosaves an edit made while a discard correction was in flight, once the correction settles', async () => {
932+
const resolvers: Array<() => void> = []
933+
const onSave = vi.fn(
934+
() =>
935+
new Promise<void>((resolve) => {
936+
resolvers.push(resolve)
937+
})
938+
)
939+
const { handle } = renderAutosave({
940+
content: 'a',
941+
savedContent: 'a',
942+
onSave,
943+
draftKey: 'file-discard-12',
944+
})
945+
946+
handle.rerender({ content: 'a1' })
947+
await act(async () => {
948+
vi.advanceTimersByTime(1500)
949+
})
950+
await flush()
951+
expect(onSave).toHaveBeenCalledTimes(1)
952+
act(() => handle.discard())
953+
handle.rerender({ content: 'a' })
954+
955+
// The original save resolves; the correction starts and holds the savingRef mutex.
956+
await act(async () => {
957+
resolvers[0]?.()
958+
})
959+
await flush()
960+
expect(onSave).toHaveBeenCalledTimes(2)
961+
962+
// The user keeps typing while the correction is still in flight. The debounce effect sees
963+
// savingRef held and bails — without a re-chain once the mutex frees, this edit would never
964+
// autosave until some unrelated future change happened to fire the effect again.
965+
handle.rerender({ content: 'a2' })
966+
await act(async () => {
967+
vi.advanceTimersByTime(1500)
968+
})
969+
await flush()
970+
expect(onSave).toHaveBeenCalledTimes(2)
971+
972+
await act(async () => {
973+
resolvers[1]?.()
974+
})
975+
await flush()
976+
expect(onSave).toHaveBeenCalledTimes(3)
977+
expect(onSave).toHaveBeenLastCalledWith()
978+
})
979+
980+
it('surfaces the newer edit’s own save outcome if a discard correction fails after being superseded', async () => {
981+
const resolvers: Array<() => void> = []
982+
const rejecters: Array<(error: Error) => void> = []
983+
let callCount = 0
984+
const onSave = vi.fn(() => {
985+
callCount += 1
986+
if (callCount === 1) return new Promise<void>((resolve) => resolvers.push(resolve))
987+
if (callCount === 2) return new Promise<void>((_, reject) => rejecters.push(reject))
988+
// The third call is the superseding edit's own save — never resolved, since we only
989+
// need to observe that it was picked up at all.
990+
return new Promise<void>(() => {})
991+
})
992+
const onDiscardCorrectionFailed = vi.fn()
993+
const { handle } = renderAutosave({
994+
content: 'a',
995+
savedContent: 'a',
996+
onSave,
997+
draftKey: 'file-discard-13',
998+
onDiscardCorrectionFailed,
999+
})
1000+
1001+
handle.rerender({ content: 'a1' })
1002+
await act(async () => {
1003+
vi.advanceTimersByTime(1500)
1004+
})
1005+
await flush()
1006+
act(() => handle.discard())
1007+
handle.rerender({ content: 'a' })
1008+
1009+
await act(async () => {
1010+
resolvers[0]?.()
1011+
})
1012+
await flush()
1013+
expect(onSave).toHaveBeenCalledTimes(2)
1014+
1015+
// A newer edit supersedes the in-flight correction before it settles.
1016+
handle.rerender({ content: 'a2' })
1017+
1018+
await act(async () => {
1019+
rejecters[0]?.(new Error('network down'))
1020+
})
1021+
await flush()
1022+
1023+
// The failed correction still reports itself, but the superseding edit's own save must be
1024+
// picked up immediately instead of leaving the hook stalled on a dead mutex.
1025+
expect(onDiscardCorrectionFailed).toHaveBeenCalledTimes(1)
1026+
expect(onSave).toHaveBeenCalledTimes(3)
1027+
expect(onSave).toHaveBeenLastCalledWith()
1028+
})
1029+
1030+
it('does not lift discard suppression when only `enabled` toggles for the same document', async () => {
1031+
const resolvers: Array<() => void> = []
1032+
const rejecters: Array<(error: Error) => void> = []
1033+
const onSave = vi.fn(
1034+
() =>
1035+
new Promise<void>((resolve, reject) => {
1036+
resolvers.push(resolve)
1037+
rejecters.push(reject)
1038+
})
1039+
)
1040+
const { handle } = renderAutosave({
1041+
content: 'a',
1042+
savedContent: 'a',
1043+
onSave,
1044+
draftKey: 'file-discard-14',
1045+
})
1046+
1047+
handle.rerender({ content: 'a1' })
1048+
await act(async () => {
1049+
vi.advanceTimersByTime(1500)
1050+
})
1051+
await flush()
1052+
expect(onSave).toHaveBeenCalledTimes(1)
1053+
act(() => handle.discard())
1054+
handle.rerender({ content: 'a' })
1055+
1056+
// The correction starts once the original save settles.
1057+
await act(async () => {
1058+
resolvers[0]?.()
1059+
})
1060+
await flush()
1061+
expect(onSave).toHaveBeenCalledTimes(2)
1062+
1063+
// A streaming lock (or any other reason `enabled` flips) toggles for the SAME document
1064+
// while the correction is still in flight — this must not be mistaken for switching files
1065+
// and clear discard's ownership of saveStatus.
1066+
handle.rerender({ enabled: false })
1067+
handle.rerender({ enabled: true })
1068+
1069+
// Without keying off the raw draftKey, the `enabled` round-trip would have cleared
1070+
// discardedRef, so the correction's failure handler would skip setSaveStatus('error') and
1071+
// leave the hook stuck on 'saving' with no visible retry affordance.
1072+
await act(async () => {
1073+
rejecters[1]?.(new Error('network down'))
1074+
})
1075+
await flush()
1076+
expect(handle.status()).toBe('error')
1077+
})
1078+
1079+
it('retries a failed discard correction via saveImmediately instead of silently no-opping', async () => {
1080+
const resolvers: Array<() => void> = []
1081+
const rejecters: Array<(error: Error) => void> = []
1082+
let callCount = 0
1083+
const onSave = vi.fn(() => {
1084+
callCount += 1
1085+
if (callCount === 1) return new Promise<void>((resolve) => resolvers.push(resolve))
1086+
if (callCount === 2) return new Promise<void>((_, reject) => rejecters.push(reject))
1087+
return Promise.resolve()
1088+
})
1089+
const onDiscardCorrectionFailed = vi.fn()
1090+
const { handle } = renderAutosave({
1091+
content: 'a',
1092+
savedContent: 'a',
1093+
onSave,
1094+
draftKey: 'file-discard-15',
1095+
onDiscardCorrectionFailed,
1096+
})
1097+
1098+
handle.rerender({ content: 'a1' })
1099+
await act(async () => {
1100+
vi.advanceTimersByTime(1500)
1101+
})
1102+
await flush()
1103+
act(() => handle.discard())
1104+
handle.rerender({ content: 'a' })
1105+
1106+
await act(async () => {
1107+
resolvers[0]?.()
1108+
})
1109+
await flush()
1110+
expect(onSave).toHaveBeenCalledTimes(2)
1111+
1112+
await act(async () => {
1113+
rejecters[0]?.(new Error('network down'))
1114+
})
1115+
await flush()
1116+
expect(onDiscardCorrectionFailed).toHaveBeenCalledTimes(1)
1117+
expect(handle.status()).toBe('error')
1118+
1119+
// Content already equals savedContent (that's what discard reverted to), so a naive retry
1120+
// through save()'s dirty-check would be a silent no-op. saveImmediately must still push
1121+
// the reverted baseline again.
1122+
await act(async () => {
1123+
await handle.saveImmediately()
1124+
})
1125+
await flush()
1126+
expect(onSave).toHaveBeenCalledTimes(3)
1127+
expect(onSave).toHaveBeenLastCalledWith('a')
1128+
expect(handle.status()).toBe('idle')
1129+
})
1130+
7661131
it('serializes IndexedDB writes and deletes so a slow write cannot resurrect a discarded draft', async () => {
7671132
let resolveWrite: (() => void) | undefined
7681133
const idbKeyval = await import('idb-keyval')

0 commit comments

Comments
 (0)