Issue view: reactions intermittently vanish/return across refreshes (SWR stale serve); milestone/label ids shown when caches resolve late

crueber commented

2026-09-09 21:48:07 +00:00

Owner

What's wrong

Two related intermittent staleness symptoms on the issue view (/:owner/:name/issues/{num}):

Reactions flip-flop across refreshes. Add a reaction, refresh — sometimes it's gone; refresh again — it's back. Remove it, same dance in reverse. The data IS persisted (it comes back), so this is a read-path/cache problem, not a write loss.
Milestones/labels sometimes render as raw ids instead of titles/names. Event text and header chips sometimes show the milestone id / label id rather than the display name, healing on a later render.

Root cause analysis (code evidence)

Symptom 1 — the SWR window serves stale issue threads.

The issue GET is served with ccSWR + a Version-keyed ETag: internal/issues/http.go:459 — writeCached(w, r, ccSWR, "v"+strconv.Itoa(view.Thread.Version), …), where ccSWR = "private, max-age=0, stale-while-revalidate=60" (http.go:175).
max-age=0 + stale-while-revalidate=60 means the browser MAY answer a refresh from its cache (stale) while revalidating in the background. So: mutate → refresh → browser serves the pre-mutation thread (reaction missing) and revalidates → refresh again → now-fresh copy (reaction present). Exactly the reported flip-flop, and intermittent by nature because the stale serve is a race against the revalidation completing.
The ETag itself is fine — Thread.Version is bumped on every mutation under CAS (service.go:657-662 for reactions; the whole mutate path in appendEvent, store.go:104-140, is a versioned CAS put). Server-side state is correct; the browser is being allowed to show stale.
Note listEvents (the events tail) is served writeJSON with no cache class (http.go:478) — so the reaction event may appear in the events list while the thread summary (ReactionSummary, cached-stale) says otherwise — an internal inconsistency the UI can surface.

Symptom 2 — id-vs-name display races the 30 s side-caches.

The issue page resolves milestone ids to titles through the page-owned milestones:{o}/{r} cache (Issue.jsx:56, TTL.milestones = 30_000 in web/src/lib/collab.js:17; labels the same at :16). The thread payload stores milestone/label ids, and milestoneTitle() (web/src/lib/milestones.js:15-19) documents the fallback: "an id missing from the repo set … renders as the bare id — the same self-heal stance as unknown labels."
The thread fetch and the milestones/labels fetches are independent useData entries. On a cold cache (or right after the 30 s TTL expires), the thread can render before the milestone/label sets arrive → ids shown. When the side-cache resolves, a later render shows names — matching "only sometimes using the name, rather than the id."
Additionally the server serves thread + labels + milestones as three separate objects with separate cache classes — nothing synchronizes them, so any client-side staleness mix produces id/name or present/absent skew.

Fix direction (for implementer)

Symptom 1 (primary): decide the freshness contract for issue threads and make the cache class honor it:

Option A (recommended): drop SWR for the thread GET — serve no-store (or max-age=0 without the stale window) so refresh always revalidates before paint. The ETag still makes revalidation cheap (304 when unchanged). Threads are interactive, mutation-heavy state; a 60 s stale window is the wrong trade for this route. The §6 TTL table can keep 30 s TTLs for the list endpoints where staleness is cosmetic.
Option B: keep SWR but have the UI revalidate-on-focus/refetch after its own mutations (invalidate the useData entry on reaction add/remove). This fixes the post-mutation case but not deep-link refreshes; weaker guarantee.
Also reconcile listEvents vs getIssue cache classes so the events tail and thread summary can't disagree (same class, or the UI derives the summary from events).

Symptom 2: make the id→name resolution non-racing:

Gate the affected renders on the side-caches being loaded (render the chip/event text after milestones:{o}/{r} and labels:{o}/{r} settle — they're already page-owned useData entries), or
Have the thread payload carry denormalized display names (title/name snapshot at mutation time) so the ids-only fallback becomes a true edge case (deleted milestone), not a race outcome. The store keeps ids as the source of truth; display names ride along.

Acceptance criteria

Add a reaction → refresh immediately → the reaction is visible on the first refresh (no stale-window serve); same for removal.
The thread GET's cache class cannot serve a pre-mutation body after a mutation completes (verify response Cache-Control and ETag behavior with a hard refresh).
Events tail and thread summary never disagree on a single page render (same freshness class or derived summary).
Milestone/label chips and event text show titles/names on first paint of a cold-cache page load (no id flash), while deleted/unknown ids still fall back to the bare id.
ETag/304 economics preserved: unchanged threads still revalidate cheaply.
Tests: cache-class assertion on the thread GET; a UI-level test that the id→name render waits for the side-caches (or renders denormalized names).

## What's wrong Two related intermittent staleness symptoms on the issue view (`/:owner/:name/issues/{num}`): 1. **Reactions flip-flop across refreshes.** Add a reaction, refresh — sometimes it's gone; refresh again — it's back. Remove it, same dance in reverse. The data IS persisted (it comes back), so this is a read-path/cache problem, not a write loss. 2. **Milestones/labels sometimes render as raw ids instead of titles/names.** Event text and header chips sometimes show the milestone id / label id rather than the display name, healing on a later render. ## Root cause analysis (code evidence) **Symptom 1 — the SWR window serves stale issue threads.** - The issue GET is served with `ccSWR` + a Version-keyed ETag: `internal/issues/http.go:459` — `writeCached(w, r, ccSWR, "v"+strconv.Itoa(view.Thread.Version), …)`, where `ccSWR = "private, max-age=0, stale-while-revalidate=60"` (`http.go:175`). - `max-age=0` + `stale-while-revalidate=60` means the browser MAY answer a refresh from its cache (stale) while revalidating in the background. So: mutate → refresh → browser serves the pre-mutation thread (reaction missing) and revalidates → refresh again → now-fresh copy (reaction present). Exactly the reported flip-flop, and intermittent by nature because the stale serve is a race against the revalidation completing. - The ETag itself is fine — `Thread.Version` is bumped on every mutation under CAS (`service.go:657-662` for reactions; the whole mutate path in `appendEvent`, `store.go:104-140`, is a versioned CAS put). Server-side state is correct; the browser is being *allowed* to show stale. - Note `listEvents` (the events tail) is served `writeJSON` with no cache class (`http.go:478`) — so the reaction event may appear in the events list while the thread summary (ReactionSummary, cached-stale) says otherwise — an internal inconsistency the UI can surface. **Symptom 2 — id-vs-name display races the 30 s side-caches.** - The issue page resolves milestone ids to titles through the page-owned `milestones:{o}/{r}` cache (`Issue.jsx:56`, `TTL.milestones = 30_000` in `web/src/lib/collab.js:17`; labels the same at :16). The thread payload stores milestone/label **ids**, and `milestoneTitle()` (`web/src/lib/milestones.js:15-19`) documents the fallback: *"an id missing from the repo set … renders as the bare id — the same self-heal stance as unknown labels."* - The thread fetch and the milestones/labels fetches are independent `useData` entries. On a cold cache (or right after the 30 s TTL expires), the thread can render before the milestone/label sets arrive → ids shown. When the side-cache resolves, a later render shows names — matching "only sometimes using the name, rather than the id." - Additionally the **server serves thread + labels + milestones as three separate objects with separate cache classes** — nothing synchronizes them, so any client-side staleness mix produces id/name or present/absent skew. ## Fix direction (for implementer) **Symptom 1 (primary):** decide the freshness contract for issue threads and make the cache class honor it: - Option A (recommended): drop SWR for the thread GET — serve `no-store` (or `max-age=0` without the stale window) so refresh always revalidates before paint. The ETag still makes revalidation cheap (304 when unchanged). Threads are interactive, mutation-heavy state; a 60 s stale window is the wrong trade for this route. The §6 TTL table can keep 30 s TTLs for the *list* endpoints where staleness is cosmetic. - Option B: keep SWR but have the UI revalidate-on-focus/refetch after its own mutations (invalidate the `useData` entry on reaction add/remove). This fixes the post-mutation case but not deep-link refreshes; weaker guarantee. - Also reconcile `listEvents` vs `getIssue` cache classes so the events tail and thread summary can't disagree (same class, or the UI derives the summary from events). **Symptom 2:** make the id→name resolution non-racing: - Gate the affected renders on the side-caches being loaded (render the chip/event text after `milestones:{o}/{r}` and `labels:{o}/{r}` settle — they're already page-owned `useData` entries), or - Have the thread payload carry denormalized display names (title/name snapshot at mutation time) so the ids-only fallback becomes a true edge case (deleted milestone), not a race outcome. The store keeps ids as the source of truth; display names ride along. ## Acceptance criteria - [ ] Add a reaction → refresh immediately → the reaction is visible on the first refresh (no stale-window serve); same for removal. - [ ] The thread GET's cache class cannot serve a pre-mutation body after a mutation completes (verify response `Cache-Control` and ETag behavior with a hard refresh). - [ ] Events tail and thread summary never disagree on a single page render (same freshness class or derived summary). - [ ] Milestone/label chips and event text show titles/names on first paint of a cold-cache page load (no id flash), while deleted/unknown ids still fall back to the bare id. - [ ] ETag/304 economics preserved: unchanged threads still revalidate cheaply. - [ ] Tests: cache-class assertion on the thread GET; a UI-level test that the id→name render waits for the side-caches (or renders denormalized names).

crueber referenced this issue from a commit

2026-09-10 02:15:30 +00:00

Fix #259: thread GET revalidates every read; milestone renders wait on side-caches

crueber referenced this issue from a pull request that will close it,

2026-09-10 02:15:44 +00:00

Fix #259: stale thread/label reads #266

crueber commented

2026-09-10 02:15:47 +00:00

Author

Owner

Fixed by #266 (branch fix/issue-259): thread GET now private, no-cache + version ETag (no more SWR stale window), and milestone renders wait on the side-caches instead of flashing bare ids. Details + test evidence in the PR.

Fixed by #266 (branch fix/issue-259): thread GET now `private, no-cache` + version ETag (no more SWR stale window), and milestone renders wait on the side-caches instead of flashing bare ids. Details + test evidence in the PR.

crueber commented

2026-09-10 02:27:10 +00:00

Author

Owner

Review of PR #266 (fix/issue-259, bd61f18) — verified in detached scratch worktree /tmp/pr266 (removed afterward); main worktree untouched (still clean on main, only pre-existing untracked .opencode/). No browser used per brief — tests + reasoning only.

no-cache-vs-no-store call — correct. internal/issues/http.go:183 ccThread='private, no-cache' replaces the 60s SWR window on the thread GET only (http.go:467). writeCached (http.go:134-154) preserves the version-keyed ETag + If-None-Match→304 path unchanged, and the new tests assert it both ways: stale ETag after AddComment→200 with advanced ETag + 3 events, fresh ETag→304 (http_test.go:239-275). List endpoints still writeJSON/no-store, thread-list SWR untouched — ccSWR symbol is gone repo-wide except the correct immutable attachment class (attachments.go:450). No new requests anywhere.

Law-6 cost — no hot-path regression. Old SWR had max-age=0 so it already revalidated every read (one conditional GET); no-cache keeps exactly that cost (304-when-unchanged, tested) minus the stale-serve. Issue threads are collaboration-layer, outside the push≤5 / warm-refs-1 / checkpoint-4 sim budgets, and the change is header-only server-side; the frontend gating waits on existing side-caches (zero new fetches). Cost claim in the PR ('one conditional GET either way') is honest.

Summary/tail consistency — verified. Thread = no-cache + 'v' ETag; tail (listEvents via writeJSON, http.go:127) = no-store, with a new assertion (http_test.go:287-291). Neither class serves stale without revalidation, so the flip-flop source (stale summary vs fresh tail) is closed.

Milestone pending contract — sound. milestoneDisplay (milestones.js:21-30) returns {pending:true} on undefined, bare-id + unknown:true on loaded-but-missing, null on null; sidebar renders '…' placeholder (Issue.jsx:522-535); event rows render honest generic 'changed the milestone' on undefined (issue-events.js:60) with bare-id self-heal preserved for deleted ids — both paths covered by new/updated node tests. Generic interim is acceptable: it asserts no direction the event can't prove. Label-chips claim verified: names ride the thread payload (Thread.Labels []string, model.go:74) and LabelChip (LabelPicker.jsx:32-40) always renders the name — only the color dot resolves late, so no gate was needed.

Backend bodies unchanged (diff is const-value + comments only); no new deps (go.mod/web manifests untouched); docs accurate (02_issues.md table + Concurrency + Decisions, same change per law 12).

Tests: go test -race ./internal/issues/... ok; coverage 96.1% (≥95 gate holds); node --test web/test/unit/*.test.js 527/527 pass (note: scratch worktree needed web/node_modules symlinked in — untracked dir, first full run failed only on missing 'marked' import, unrelated to PR); gofmt clean; go vet clean. Vite/esbuild builds not re-run (JS changes are logic-only, no new imports).

No fixes needed — nothing to push. MERGE RECOMMENDATION: ready to merge.

Review of PR #266 (fix/issue-259, bd61f18) — verified in detached scratch worktree /tmp/pr266 (removed afterward); main worktree untouched (still clean on main, only pre-existing untracked .opencode/). No browser used per brief — tests + reasoning only. **no-cache-vs-no-store call — correct.** internal/issues/http.go:183 ccThread='private, no-cache' replaces the 60s SWR window on the thread GET only (http.go:467). writeCached (http.go:134-154) preserves the version-keyed ETag + If-None-Match→304 path unchanged, and the new tests assert it both ways: stale ETag after AddComment→200 with advanced ETag + 3 events, fresh ETag→304 (http_test.go:239-275). List endpoints still writeJSON/no-store, thread-list SWR untouched — ccSWR symbol is gone repo-wide except the correct immutable attachment class (attachments.go:450). No new requests anywhere. **Law-6 cost — no hot-path regression.** Old SWR had max-age=0 so it already revalidated every read (one conditional GET); no-cache keeps exactly that cost (304-when-unchanged, tested) minus the stale-serve. Issue threads are collaboration-layer, outside the push≤5 / warm-refs-1 / checkpoint-4 sim budgets, and the change is header-only server-side; the frontend gating waits on existing side-caches (zero new fetches). Cost claim in the PR ('one conditional GET either way') is honest. **Summary/tail consistency — verified.** Thread = no-cache + 'v<version>' ETag; tail (listEvents via writeJSON, http.go:127) = no-store, with a new assertion (http_test.go:287-291). Neither class serves stale without revalidation, so the flip-flop source (stale summary vs fresh tail) is closed. **Milestone pending contract — sound.** milestoneDisplay (milestones.js:21-30) returns {pending:true} on undefined, bare-id + unknown:true on loaded-but-missing, null on null; sidebar renders '…' placeholder (Issue.jsx:522-535); event rows render honest generic 'changed the milestone' on undefined (issue-events.js:60) with bare-id self-heal preserved for deleted ids — both paths covered by new/updated node tests. Generic interim is acceptable: it asserts no direction the event can't prove. Label-chips claim verified: names ride the thread payload (Thread.Labels []string, model.go:74) and LabelChip (LabelPicker.jsx:32-40) always renders the name — only the color dot resolves late, so no gate was needed. **Backend bodies unchanged** (diff is const-value + comments only); **no new deps** (go.mod/web manifests untouched); **docs accurate** (02_issues.md table + Concurrency + Decisions, same change per law 12). **Tests:** go test -race ./internal/issues/... ok; coverage 96.1% (≥95 gate holds); node --test web/test/unit/*.test.js 527/527 pass (note: scratch worktree needed web/node_modules symlinked in — untracked dir, first full run failed only on missing 'marked' import, unrelated to PR); gofmt clean; go vet clean. Vite/esbuild builds not re-run (JS changes are logic-only, no new imports). No fixes needed — nothing to push. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-10 02:28:05 +00:00

crueber commented

2026-09-10 02:28:05 +00:00

Author

Owner

Fixed by PR #266 (review: no-cache keeps 304 economics, milestone contract sound, no hot-path regression; 96.1% + 527/527), merged. Closing.

crueber referenced this issue from a commit

2026-09-10 02:28:06 +00:00

Merge pull request 'Fix #259: stale thread/label reads' (#266) from fix/issue-259 into main

crueber referenced this issue

2026-09-10 11:25:58 +00:00