fix(files): fix savingRef mutex integrity race after discard correction#5554
Conversation
…press 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).
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@cursor review |
PR SummaryMedium Risk Overview Corrective server writes move into A completed save’s Adds a broad regression suite in Reviewed by Cursor Bugbot for commit 76964d4. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b0f9bf9. Configure here.
Greptile SummaryThis PR tightens autosave behavior around discard correction. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "fix(files): key discard state to raw dra..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR tightens autosave discard handling and draft recovery. The main changes are:
Confidence Score: 5/5The changed flow looks mergeable after small cleanup around discard-state edge cases.
apps/sim/hooks/use-autosave.ts Important Files Changed
Reviews (2): Last reviewed commit: "fix(files): fix savingRef mutex integrit..." | Re-trigger Greptile |
…ion 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.
|
@cursor review |
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.
|
@cursor review |
…s 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.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 76964d4. Configure here.
1 participant
Summary
save()'s deferredMIN_SAVING_DISPLAY_MSstatus timer could clobbersavingRef/trigger a concurrent resave even afterdiscard()'s correction had claimed the save slot — now guarded oninFlightRef.currentisDirty, which a stale save'smarkSavedContentlanding post-discard could transiently corrupt — now keyed off adiscardTargetRefcaptured at discard timeType of Change
Testing
does not let the original save leftover status timer clobber a still-running correction— verified it fails pre-fix (3 concurrent saves) and passes post-fixbun run type-check,bunx biome check, and the full autosave/files/knowledge vitest suite (509 tests) all passChecklist