Skip to content

Commit 100e9fe

Browse files
committed
fix(studio): serialize caption saves so an older body cannot win
Each save() captured its own body but nothing stopped two PUTs being in flight at once, and both raced the shared versionRef. The common ordering self-heals (the older lands first, the newer 409s, adopts the version and retries), but if the two reorder, or the older one's adopt-and-retry lands last, the older body is written and the newer edit is silently lost. It needs a slow save overlapping a fresh edit, so it is off the common path, but it is real data loss. Saves now run one at a time. A save requested while one is in flight replaces any already-queued attempt instead of stacking, so the next PUT always carries the newest body and no edit is dropped on the way. Also throw StudioSaveNetworkError rather than a plain Error on fetch rejection, so retryStudioSave's network-retry path applies to captions the way it already does in useFileManager.
1 parent ebe6704 commit 100e9fe

2 files changed

Lines changed: 104 additions & 25 deletions

File tree

packages/studio/src/captions/hooks/useCaptionSync.test.tsx

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,4 +219,56 @@ describe("useCaptionSync", () => {
219219

220220
expect(fetchMock.mock.calls.some(([, init]) => init?.method === "PUT")).toBe(true);
221221
});
222+
it("serializes overlapping saves so the newest body lands last", async () => {
223+
vi.useFakeTimers();
224+
let inFlight = 0;
225+
let maxInFlight = 0;
226+
const putBodies: string[] = [];
227+
const releases: Array<() => void> = [];
228+
229+
const fetchMock = vi.fn(async (_url: string, init?: RequestInit) => {
230+
if (init?.method !== "PUT") return new Response(null, { status: 404 });
231+
putBodies.push(String(init.body));
232+
inFlight++;
233+
maxInFlight = Math.max(maxInFlight, inFlight);
234+
await new Promise<void>((resolve) => releases.push(resolve));
235+
inFlight--;
236+
return jsonResponse({ ok: true, version: `"sha256:v${putBodies.length}"` });
237+
});
238+
vi.stubGlobal("fetch", fetchMock);
239+
240+
mountCaptionSync("proj-1");
241+
const store = useCaptionStore.getState();
242+
store.setSourceFilePath("comp.html");
243+
act(() => store.setModel(makeModel()));
244+
act(() => store.setEditMode(true));
245+
246+
// First edit: its PUT starts and then hangs, still in flight.
247+
act(() => store.setModel(makeModel({ x: 5 })));
248+
await flushDebounce();
249+
expect(putBodies).toHaveLength(1);
250+
251+
// A second edit arrives before the first PUT has come back. It must be
252+
// queued rather than raced against the in-flight one, which is what could
253+
// let the older body win and lose this edit.
254+
act(() => store.setModel(makeModel({ x: 9 })));
255+
await flushDebounce();
256+
expect(putBodies).toHaveLength(1);
257+
expect(maxInFlight).toBe(1);
258+
259+
// Releasing the first PUT lets the queued newest body go out.
260+
await act(async () => {
261+
releases.shift()?.();
262+
await vi.advanceTimersByTimeAsync(0);
263+
});
264+
265+
expect(putBodies).toHaveLength(2);
266+
expect(maxInFlight).toBe(1);
267+
expect(putBodies[1]).toContain('"x": 9');
268+
269+
await act(async () => {
270+
releases.shift()?.();
271+
await vi.advanceTimersByTimeAsync(0);
272+
});
273+
});
222274
});

packages/studio/src/captions/hooks/useCaptionSync.ts

Lines changed: 52 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import type { CaptionStyle } from "../types";
66
import { studioWriteHeaders } from "../../utils/studioFileVersion";
77
import {
88
StudioFileConflictError,
9+
StudioSaveNetworkError,
910
createStudioSaveHttpError,
1011
retryStudioSave,
1112
} from "../../utils/studioSaveDiagnostics";
@@ -91,6 +92,15 @@ export function useCaptionSync(projectId: string | null) {
9192
// before the first write), null = confirmed not to exist yet (send
9293
// If-None-Match instead of If-Match).
9394
const versionRef = useRef<string | null | undefined>(undefined);
95+
// Saves are serialized. Two PUTs in flight at once race on versionRef: the
96+
// usual ordering self-heals (the older lands first, the newer 409s, adopts
97+
// and retries), but if they reorder — or the older one's adopt-and-retry
98+
// lands last — the older body wins and the newer edit is silently lost.
99+
// Only one attempt runs at a time, and a save requested while one is in
100+
// flight replaces any already-queued attempt rather than stacking, so the
101+
// next PUT always carries the newest body.
102+
const savingRef = useRef(false);
103+
const queuedSaveRef = useRef<{ pid: string; body: string; seq: number } | null>(null);
94104

95105
const setPending = (pending: boolean) => {
96106
pendingRef.current = pending;
@@ -113,9 +123,14 @@ export function useCaptionSync(projectId: string | null) {
113123
body,
114124
});
115125
} catch (error) {
116-
throw new Error(`Failed to save ${CAPTION_OVERRIDES_PATH}: network error`, {
117-
cause: error,
118-
});
126+
// StudioSaveNetworkError, not a plain Error, so retryStudioSave's
127+
// network-retry path applies here the way it does in useFileManager.
128+
throw new StudioSaveNetworkError(
129+
`Failed to save ${CAPTION_OVERRIDES_PATH}: network error`,
130+
{
131+
cause: error,
132+
},
133+
);
119134
}
120135
if (response.status === 409) {
121136
const conflict = (await response.json().catch(() => null)) as {
@@ -165,35 +180,47 @@ export function useCaptionSync(projectId: string | null) {
165180
return;
166181
}
167182

168-
const seqAtSave = editSeqRef.current;
169-
const body = JSON.stringify(buildOverrides(state.model), null, 2);
183+
queuedSaveRef.current = {
184+
pid,
185+
body: JSON.stringify(buildOverrides(state.model), null, 2),
186+
seq: editSeqRef.current,
187+
};
188+
if (savingRef.current) return;
189+
savingRef.current = true;
170190

171-
(async () => {
191+
void (async () => {
172192
try {
173-
await putOverrides(pid, body);
174-
} catch (error) {
175-
if (error instanceof StudioFileConflictError) {
176-
// versionRef was already refreshed by putOverrides; one retry with
177-
// it is enough for a single-writer file.
178-
await putOverrides(pid, body);
179-
} else {
180-
throw error;
193+
// Drains whatever is queued, including edits that arrive mid-PUT, so
194+
// the last write to land is always the newest body.
195+
while (queuedSaveRef.current) {
196+
const attempt = queuedSaveRef.current;
197+
queuedSaveRef.current = null;
198+
try {
199+
await putOverrides(attempt.pid, attempt.body);
200+
} catch (error) {
201+
if (error instanceof StudioFileConflictError) {
202+
// versionRef was already refreshed by putOverrides; one retry
203+
// with it is enough for a single-writer file.
204+
await putOverrides(attempt.pid, attempt.body);
205+
} else {
206+
throw error;
207+
}
208+
}
209+
// A newer edit may have re-armed the debounce while this PUT was in
210+
// flight — its beforeunload/unmount flush still needs pending=true.
211+
if (editSeqRef.current === attempt.seq) setPending(false);
212+
const s = useCaptionStore.getState();
213+
if (s.syncError) s.setSyncError(null);
181214
}
182-
}
183-
})()
184-
.then(() => {
185-
// A newer edit may have re-armed the debounce while this PUT was in
186-
// flight — its beforeunload/unmount flush still needs pending=true.
187-
if (editSeqRef.current === seqAtSave) setPending(false);
188-
const s = useCaptionStore.getState();
189-
if (s.syncError) s.setSyncError(null);
190-
})
191-
.catch((error: unknown) => {
215+
} catch (error: unknown) {
192216
// Caption auto-save is a data-loss path: surface it to the user, not
193217
// just telemetry. pendingRef stays true so beforeunload still warns.
194218
trackEvent("studio_caption_autosave_failed", { error: String(error) });
195219
useCaptionStore.getState().setSyncError("Caption changes couldn't be saved");
196-
});
220+
} finally {
221+
savingRef.current = false;
222+
}
223+
})();
197224
}, [putOverrides]);
198225

199226
// Auto-save on model changes with 800ms debounce

0 commit comments

Comments
 (0)