From cfcac48df2d8ba6922d0b65c2dec48309edfa1ec Mon Sep 17 00:00:00 2001 From: Sulthan Zaki Date: Mon, 27 Jul 2026 17:33:37 +0700 Subject: [PATCH] fix(userscript): close the un-archive hole and the drain interleaving races Co-Authored-By: Claude Opus 5 --- userscript/manga-bookmark.user.js | 107 +++++++++++++++++++++++------- 1 file changed, 82 insertions(+), 25 deletions(-) diff --git a/userscript/manga-bookmark.user.js b/userscript/manga-bookmark.user.js index efc9be8..5cae976 100644 --- a/userscript/manga-bookmark.user.js +++ b/userscript/manga-bookmark.user.js @@ -416,6 +416,17 @@ } } + // Parks a write without counting it as a failure. Used when the key is already + // on the wire: the write is owed, but nothing went wrong, so it must not spend + // one of the ten attempts. + function queueDefer(key, op, sendStatus) { + const e = queueGet(key) || queuePush({ key: key, op: op, sendStatus: false, attempts: 0 }); + e.op = op; // a delete replaces a put, and a put replaces a delete + e.sendStatus = e.sendStatus || sendStatus; // sticky: an archive intent is never dropped + saveQueue(queue); + return e; + } + // Records a failed write and classifies why it failed. A 400 is a payload the // server will never accept, so it is dropped now instead of being retried ten // times; a 401 is the wrong token, so the entry is kept untouched and the @@ -427,9 +438,7 @@ toast("Couldn't sync " + titleFor(key) + " — change lost", true); return; } - const e = queueGet(key) || queuePush({ key: key, op: op, sendStatus: false, attempts: 0 }); - e.op = op; // a delete replaces a put, and a put replaces a delete - e.sendStatus = e.sendStatus || sendStatus; // sticky: an archive intent is never dropped + const e = queueDefer(key, op, sendStatus); if (status === 401) { authFailed = true; } else if (++e.attempts >= QUEUE_MAX_ATTEMPTS) { @@ -440,29 +449,52 @@ saveQueue(queue); } - let draining = false; + // Keys with a request on the wire right now, mapped to "another write arrived + // while this one was flying". A second write to the same key is never sent + // concurrently — the two responses would race to own the row — it is parked in + // the queue instead and a later drain replays it. The flight already in the + // air must then leave that entry alone and not adopt its own now-stale + // response, or it would undo the write the user just made. + const inFlight = new Map(); + + let draining = null; // the pass in progress, so a second caller awaits it // Replays everything owed. Cheap in the normal case — it returns on the first // line when the queue is empty, which is why it can hang off navigation. A // 401 stops the whole pass: the token is wrong, so the next entry would fail // the same way, and the queue is left intact so fixing the token fixes it. - async function drain() { - if (draining || queue.length === 0) return; - draining = true; - authFailed = false; - try { - for (const e of queue.slice()) { - if (e.op === "delete") await pushDelete(e.key); - else await pushBookmark(e.key, e.sendStatus); - if (authFailed) { - toast("Sync auth failed — check the token", true); - break; + // + // Returns the in-flight pass when one is already running, so refresh()'s + // `await drain()` really does wait for what we owe instead of racing a drain + // that onNavigate or the online listener started unawaited. + function drain() { + if (queue.length === 0) return Promise.resolve(); + if (draining) return draining; + const pass = (async () => { + authFailed = false; + try { + for (const e of queue.slice()) { + if (e.op === "delete") await pushDelete(e.key); + else await pushBookmark(e.key, e.sendStatus); + if (authFailed) { + toast("Sync auth failed — check the token", true); + break; + } } + } finally { + render(); } - } finally { - draining = false; - } - render(); + })(); + // Swallowed, not surfaced: drain is called unawaited from onNavigate and the + // online listener, and an uncaught rejection on a page we do not control is + // a console error nobody can act on. Every real sync failure is already + // toasted and queued by pushBookmark/pushDelete. + draining = pass + .catch(() => {}) + .finally(() => { + draining = null; + }); + return draining; } // Keys the server has not heard about yet must survive a fetched list, or the @@ -503,29 +535,51 @@ queueDrop(key); // removed locally in the meantime — nothing left to send return true; } + if (inFlight.has(key)) { + queueDefer(key, "put", withStatus); // see inFlight — parked, not sent + inFlight.set(key, true); // supersedes the flight already in the air + return false; + } + inFlight.set(key, false); + let ok = false; try { // ponytail: last-write-wins, so a replay can overwrite a newer server // value (a poller-written latest_chapter, or progress from another // device). Single user, self-healing on the next poll — revisit only if // this ever runs multi-user. - upsertLocal(await apiPut(key, bm, { sendStatus: withStatus })); - queueDrop(key); - render(); - return true; + const saved = await apiPut(key, bm, { sendStatus: withStatus }); + if (!inFlight.get(key)) { + upsertLocal(saved); + queueDrop(key); + } + ok = true; } catch (e) { queueEnqueue(key, "put", withStatus, e); - return false; + } finally { + inFlight.delete(key); } + // Outside the try: a throw in render() is a UI bug, not a write failure, and + // must not re-queue a write that already landed. + if (ok) render(); + return ok; } async function pushDelete(key) { + if (inFlight.has(key)) { + queueDefer(key, "delete", false); // see inFlight — parked, not sent + inFlight.set(key, true); // supersedes the flight already in the air + return false; + } + inFlight.set(key, false); try { await apiDelete(key); - queueDrop(key); + if (!inFlight.get(key)) queueDrop(key); return true; } catch (e) { queueEnqueue(key, "delete", false, e); return false; + } finally { + inFlight.delete(key); } } @@ -635,6 +689,9 @@ }); upsertLocal(bm); render(); + // A queued write owns this row; the drain sends latest_chapter + // with it, carrying the correct bucket. + if (queueGet(bm.key)) return; try { const saved = await apiPut(bm.key, bm); upsertLocal(saved);