Offline retry queue for userscript writes #5

2026-07-27T17:47:29+07:00

sulthan commented

2026-07-27 17:47:29 +07:00

The bug

Every mutation in the userscript is optimistic: it writes state.list and the mangabm:cache copy, re-renders, then PUTs. If the PUT fails, nothing rolls back and nothing retries. The cache now asserts something the server has never heard of, until the next successful GET /bookmarks silently overwrites it.

Walked end to end: phone loses signal mid-read, user taps Archive, the card moves to Archived and looks saved. apiPut throws. Local state is not rolled back. Signal returns, a later navigation calls refresh() → apiGet() → setList(), which replaces state.list wholesale. The series is back in All. No toast, no explanation, minutes later.

Four of the five call sites already toasted a promise of a retry that did not exist. This makes the existing copy honest rather than adding a new promise.

The shape

Markers, not payloads. Every mutation already builds and PUTs the whole desired row, and state.list (mirrored into mangabm:cache) already is the desired state. So the queue stores only {key, op, sendStatus, attempts} in localStorage under mangabm:queue; the body is read from state.byKey at send time. That collapses four hard questions at once:

Ordering — one entry per key, so two writes to the same series cannot replay out of order.
Coalescing — archive-then-unarchive is not two writes, it is "the cache now says reading". Nothing to merge.
DELETE after PUT — the delete entry replaces the put entry, so a replay cannot resurrect the row.
Staleness — no snapshot can drift from the cache, because there is no snapshot.

One write path. pushBookmark / pushDelete are the only way a user-facing mutation reaches the API — not a fallback bolted onto each catch. That distinction is the whole point; see below.

Drain triggers, all cheap when the queue is empty (drain() returns on its first line): head of refresh(), onNavigate (drain only, not a full refresh — Asura is client-routed and an extra GET per route change is not wanted), a window online listener, and tapping the pending chip.

Visibility. A ⟳ N pending chip in the existing #nav row, hidden entirely when the queue is empty. Silent convergence in the happy path; honest the moment something is stuck.

Failure classes: a 400 drops the entry and says so; a 401 aborts the whole pass and keeps the queue intact (fixing the token fixes everything); a 404 on DELETE is treated as success; network errors and 5xx retry to a cap of 10 attempts. Every dropped write is announced — a queue that fails permanently and says nothing is the same class of bug being fixed.

The sticky-`sendStatus` hole this closes

An empty status on the wire means "keep the stored bucket" server-side. A queue bolted onto each mutation's catch has a hole:

Offline. User archives X → entry {X, put, sendStatus: true} is queued.
Signal returns. No drain trigger has fired yet.
User reads a chapter of X → syncUpsert PUTs with sendStatus: false → succeeds → upsertLocal(saved) adopts a server row that still says reading.
The archive is gone from local state, and the pending entry now replays a row that no longer carries the intent. Silent un-archive.

Routing every write through pushBookmark — which ORs in any pending sendStatus and only ever widens it, never narrows it — is what closes that. A replayed progress write still omits status; a replayed archive still carries it.

refresh() drains before it fetches, then overlayPending() re-applies anything still pending over the fetched list before setList replaces state.byKey, so the card the user just changed never flaps back.

applyLatestChapterIfChanged deliberately stays out of the queue: it is background information the user never asked for, the server-side poller learns the same fact independently, and backgroundRefreshLatest already retries on a 4h throttle. Queueing it would let a stale local latest_chapter overwrite a fresher poller value on replay.

Fixes from the final review (commits 6-8)

The whole-branch review found one Critical and two Important defects that only appear across commit boundaries:

Critical — the un-archive hole reopened through the latest-chapter exclusion. applyLatestChapterIfChanged PUTs without sendStatus, so the server strips status, returns the stored reading, and upsertLocal(saved) writes that over a pending archive. onNavigate runs maybeCaptureLatestOnSeriesPage() before drain() with no await between them, so this was deterministic on any series-page visit, not a race — and the drain then sent status:"reading" explicitly, making it permanent. Fixed with a single if (queueGet(bm.key)) return; guard: the write stays unqueued as designed, it just no longer adopts a server row while a write is pending. latest_chapter still reaches the server via the drain, carrying the correct bucket.
Important — draining guarded drain-vs-drain but not drain-vs-mutation. A tap during an in-flight same-key PUT started a second concurrent write; whichever response landed second won, and the loser's queueDrop could delete the entry the tap had just parked. Fixed with per-key in-flight tracking: a write for a key already in flight defers (parks a queue entry, sends nothing, returns false so the caller still toasts), and the landing flight suppresses its own upsertLocal/queueDrop when superseded — including on its failure path, so a failing flight cannot clobber a parked op:"delete" and resurrect a removed bookmark.
Important — drain() returned undefined while already draining, so refresh()'s await drain() was a silent no-op and could adopt a pre-write list, flapping the card at boot. It now returns the in-flight promise. The empty-queue fast path is unchanged and still an immediate return.

Also: render() moved out of pushBookmark's try (a render throw was re-queueing an already-successful write), and unawaited drain() rejections are swallowed.

The backend is untouched

No file under backend/ is in this diff. The updated_at rule and the empty-status keep rule stay solely in Store.Upsert; nothing client-side duplicates or works around them. A replayed PUT is an ordinary late write under the project's existing last-write-wins model. Regression check: go test ./... is ok, CGO_ENABLED=0 go build ./... succeeds.

Two accepted losses, marked with ponytail: comments at the replay site: a latest_chapter the poller learned while the client was offline can be overwritten by the client's older value (self-healing on the poller's next cooldown), and read progress made on another device between the failed write and the replay can be overwritten (single-user deployment).

Verification status — read this before merging

node --check passes and every commit was reviewed, but the 15-row manual DevTools checklist has NOT been run. There is no test infrastructure for the userscript, and by design it gains none here — a pasted copy of the logic in a scratch node script would drift from the real file the moment either changed. The author is shipping to prod and verifying there.

The rows most worth checking first, because each maps to a specific defect the review caught:

Offline → archive X → online → open X's series page and nothing else → X must stay Archived. (the Critical above; nothing else exercises it)
Slow 3G → queue a write → tap the pending chip → immediately archive the same series → final state must match the last tap.
Queue a write → reload on a slow link → the card must not flap back during init.
Offline → archive X, then remove X → one entry, op:"delete" → after reconnecting, X must not reappear.
Empty queue → navigate for a minute → no chip and no extra network requests from onNavigate.

Known limitations, deliberately not fixed here

A 400 drops the entry and toasts, but local state keeps asserting the lost change until the next successful GET.
A 401 on a live tap toasts "will sync when online" rather than the auth message; the user learns the truth on the next drain.
Two same-origin tabs clobber each other's mangabm:queue — the queue is read once at boot and each save writes the whole array. Same idiom as the pre-existing saveCache; low risk on mobile Bromite.
A pending favourite floats to the top of the list until it syncs, then settles back. Consistent with how optimistic writes already behaved.

🤖 Generated with Claude Code

## The bug Every mutation in the userscript is optimistic: it writes `state.list` and the `mangabm:cache` copy, re-renders, then PUTs. **If the PUT fails, nothing rolls back and nothing retries.** The cache now asserts something the server has never heard of, until the next successful `GET /bookmarks` silently overwrites it. Walked end to end: phone loses signal mid-read, user taps **Archive**, the card moves to Archived and looks saved. `apiPut` throws. Local state is not rolled back. Signal returns, a later navigation calls `refresh()` → `apiGet()` → `setList()`, which replaces `state.list` wholesale. The series is back in All. No toast, no explanation, minutes later. Four of the five call sites already toasted a promise of a retry that did not exist. This makes the existing copy honest rather than adding a new promise. ## The shape **Markers, not payloads.** Every mutation already builds and PUTs the *whole* desired row, and `state.list` (mirrored into `mangabm:cache`) already *is* the desired state. So the queue stores only `{key, op, sendStatus, attempts}` in `localStorage` under `mangabm:queue`; the body is read from `state.byKey` at send time. That collapses four hard questions at once: - **Ordering** — one entry per key, so two writes to the same series cannot replay out of order. - **Coalescing** — archive-then-unarchive is not two writes, it is "the cache now says `reading`". Nothing to merge. - **DELETE after PUT** — the delete entry *replaces* the put entry, so a replay cannot resurrect the row. - **Staleness** — no snapshot can drift from the cache, because there is no snapshot. **One write path.** `pushBookmark` / `pushDelete` are the only way a user-facing mutation reaches the API — not a fallback bolted onto each `catch`. That distinction is the whole point; see below. **Drain triggers**, all cheap when the queue is empty (`drain()` returns on its first line): head of `refresh()`, `onNavigate` (drain only, *not* a full refresh — Asura is client-routed and an extra GET per route change is not wanted), a `window` `online` listener, and tapping the pending chip. **Visibility.** A `⟳ N pending` chip in the existing `#nav` row, hidden entirely when the queue is empty. Silent convergence in the happy path; honest the moment something is stuck. **Failure classes:** a `400` drops the entry and says so; a `401` aborts the whole pass and keeps the queue intact (fixing the token fixes everything); a `404` on DELETE is treated as success; network errors and `5xx` retry to a cap of 10 attempts. Every dropped write is announced — a queue that fails permanently and says nothing is the same class of bug being fixed. ## The sticky-`sendStatus` hole this closes An empty `status` on the wire means "keep the stored bucket" server-side. A queue bolted onto each mutation's `catch` has a hole: 1. Offline. User archives X → entry `{X, put, sendStatus: true}` is queued. 2. Signal returns. No drain trigger has fired yet. 3. User reads a chapter of X → `syncUpsert` PUTs with `sendStatus: false` → **succeeds** → `upsertLocal(saved)` adopts a server row that still says `reading`. 4. The archive is gone from local state, and the pending entry now replays a row that no longer carries the intent. Silent un-archive. Routing every write through `pushBookmark` — which ORs in any pending `sendStatus` and only ever *widens* it, never narrows it — is what closes that. A replayed progress write still omits `status`; a replayed archive still carries it. `refresh()` drains before it fetches, then `overlayPending()` re-applies anything still pending over the fetched list before `setList` replaces `state.byKey`, so the card the user just changed never flaps back. `applyLatestChapterIfChanged` deliberately stays **out** of the queue: it is background information the user never asked for, the server-side poller learns the same fact independently, and `backgroundRefreshLatest` already retries on a 4h throttle. Queueing it would let a stale local `latest_chapter` overwrite a fresher poller value on replay. ## Fixes from the final review (commits 6-8) The whole-branch review found one Critical and two Important defects that only appear across commit boundaries: - **Critical — the un-archive hole reopened through the latest-chapter exclusion.** `applyLatestChapterIfChanged` PUTs without `sendStatus`, so the server strips `status`, returns the stored `reading`, and `upsertLocal(saved)` writes that over a pending archive. `onNavigate` runs `maybeCaptureLatestOnSeriesPage()` *before* `drain()` with no await between them, so this was deterministic on any series-page visit, not a race — and the drain then sent `status:"reading"` explicitly, making it permanent. Fixed with a single `if (queueGet(bm.key)) return;` guard: the write stays unqueued as designed, it just no longer adopts a server row while a write is pending. `latest_chapter` still reaches the server via the drain, carrying the correct bucket. - **Important — `draining` guarded drain-vs-drain but not drain-vs-mutation.** A tap during an in-flight same-key PUT started a second concurrent write; whichever response landed second won, and the loser's `queueDrop` could delete the entry the tap had just parked. Fixed with per-key in-flight tracking: a write for a key already in flight defers (parks a queue entry, sends nothing, returns `false` so the caller still toasts), and the landing flight suppresses its own `upsertLocal`/`queueDrop` when superseded — including on its failure path, so a failing flight cannot clobber a parked `op:"delete"` and resurrect a removed bookmark. - **Important — `drain()` returned `undefined` while already draining**, so `refresh()`'s `await drain()` was a silent no-op and could adopt a pre-write list, flapping the card at boot. It now returns the in-flight promise. The empty-queue fast path is unchanged and still an immediate return. Also: `render()` moved out of `pushBookmark`'s `try` (a render throw was re-queueing an already-successful write), and unawaited `drain()` rejections are swallowed. ## The backend is untouched No file under `backend/` is in this diff. The `updated_at` rule and the empty-status keep rule stay solely in `Store.Upsert`; nothing client-side duplicates or works around them. A replayed PUT is an ordinary late write under the project's existing last-write-wins model. Regression check: `go test ./...` is `ok`, `CGO_ENABLED=0 go build ./...` succeeds. Two accepted losses, marked with `ponytail:` comments at the replay site: a `latest_chapter` the poller learned while the client was offline can be overwritten by the client's older value (self-healing on the poller's next cooldown), and read progress made on another device between the failed write and the replay can be overwritten (single-user deployment). ## Verification status — read this before merging `node --check` passes and every commit was reviewed, but **the 15-row manual DevTools checklist has NOT been run.** There is no test infrastructure for the userscript, and by design it gains none here — a pasted copy of the logic in a scratch node script would drift from the real file the moment either changed. The author is shipping to prod and verifying there. The rows most worth checking first, because each maps to a specific defect the review caught: - Offline → archive X → online → open X's **series page** and nothing else → X must stay Archived. *(the Critical above; nothing else exercises it)* - Slow 3G → queue a write → tap the pending chip → immediately archive the same series → final state must match the last tap. - Queue a write → reload on a slow link → the card must not flap back during `init`. - Offline → archive X, then remove X → one entry, `op:"delete"` → after reconnecting, X must not reappear. - Empty queue → navigate for a minute → no chip and **no extra network requests** from `onNavigate`. ## Known limitations, deliberately not fixed here - A `400` drops the entry and toasts, but local state keeps asserting the lost change until the next successful `GET`. - A `401` on a live tap toasts "will sync when online" rather than the auth message; the user learns the truth on the next drain. - Two same-origin tabs clobber each other's `mangabm:queue` — the queue is read once at boot and each save writes the whole array. Same idiom as the pre-existing `saveCache`; low risk on mobile Bromite. - A pending favourite floats to the top of the list until it syncs, then settles back. Consistent with how optimistic writes already behaved. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

sulthan added 8 commits 2026-07-27 17:47:30 +07:00

feat(userscript): retry queue storage and status-carrying API errors bbe04b1e81

feat(userscript): route every write through the retry queue 0615617ebe

feat(userscript): drain the retry queue on navigate and on reconnect 4b863d5d2e

feat(userscript): keep pending writes over a refreshed list 6eb29902b1

feat(userscript): show pending writes in the panel nav 51bf123cab

docs: record the userscript retry queue a9ddf3d804

fix(userscript): close the un-archive hole and the drain interleaving races cfcac48df2

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

fix(userscript): don't let a failing in-flight write clobber a deferred entry 66eb3d2a98

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

sulthan merged commit 6dce9fc481 into main

2026-07-27 17:52:17 +07:00

sulthan deleted branch feat/offline-retry-queue

2026-07-31 01:02:13 +07:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sulthan/mangaBookmark#5