#280 - Systemic: SWR stale-serve window on all mutable collab state (reactions, stars, watches, releases, identity) causes refresh flip-flop — needs a third cache class - crueber/walhub

crueber commented

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

Owner

Supersedes #259 (same root cause, wider blast radius, permanent fix proposed). #259 scoped this to the issue view; the same stale-serve behavior exists on stars/watches (/{o}/{r}/api/social), releases, pulls, and identity surfaces. Closing #259 in favor of this one is appropriate.

The systemic problem

Mutating collaborative state — reactions, stars, watches, release edits, issue patches — then refreshing can serve the pre-mutation state, and the next refresh shows the post-mutation state. It is intermittent by construction and affects every surface served with the SWR cache class:

Surface	ETag basis	File
Issue/PR thread (reactions, comments, patches)	`v<Thread.Version>`	`internal/issues/http.go:459`
Social (stars, watches, forks)	store version token	`internal/social/http.go:248`
Release single + list	store version	`internal/releases/http.go:355,371,380`
Pull view	`HeadLive`	`internal/pulls/http.go:493`
Identity (profile, orgs, teams, invites)	doc version	`internal/identity/http.go` (7 sites)

All use ccSWR = "private, max-age=0, stale-while-revalidate=60" (api/env.go:582, duplicated per package in issues/http.go:175, social/http.go:185).

Mechanics of the flip-flop: stale-while-revalidate=60 permits the browser to answer a refresh from its cache — the stale pre-mutation body — while revalidating in the background. Refresh 1 paints old state (reaction "gone", star "off"); the background revalidation lands; refresh 2 paints new state. Server state is correct at all times (every mutation is a versioned CAS put and the ETag is version-keyed) — the header contract is what licenses the staleness.

Why the client makes it worse: the SPA's useData layer (web/src/lib/data.js) has its own TTL revalidation (5–30 s by surface), and mutations invalidate only the mutation's own cache entry — a hard refresh re-requests through the browser cache, where SWR serves stale before the client TTL logic even runs.

Where the design went wrong

The repo's own spec drew the line in the right place and the implementation crossed it:

docs/go/07_api.md §4 ("the central design rule") defines exactly two cache classes, both for git content: sha-addressed → immutable; ref-dependent (name-addressed git views) → SWR-60 + sha ETag. The rationale is sound: git content changes only via refs, so a ref-state ETag makes SWR safe.
Collaborative state is not ref-dependent — threads, social counters, releases, and identity docs mutate independently of any ref move. Their specs then borrowed the class anyway: docs/features/07_releases_stars.md:235 explicitly says "single release / list / social GETs are ref-dependent class (SWR 60 s + ETag version token)" and docs/features/02_issues.md:258 says "GET-by-num is ref-class SWR keyed on the header version." That was the wrong call: a version-keyed ETag gives correct revalidation, but SWR's stale-serve window is a correctness concession that git content can afford (refs move rarely, stale ≈ seconds old) and mutable collab state cannot (the user just clicked the button).

Proposed permanent solution

Principle: the SWR stale-serve window is for content whose staleness is bounded by ref movement, not for user-mutable state. Any GET whose resource can change via a direct user action must not serve stale.

Introduce a third cache class — ccNoCache = "private, no-cache" — for mutation-sensitive GETs: Cache-Control: private, no-cache + the existing version ETag. no-cache requires revalidation before every use (the browser still caches; the ETag still makes unchanged responses 304 with zero body), so the economics stay cheap — you lose only the stale-serve window, which is exactly the bug. This is a one-line class + per-site swap; writeCached already implements the If-None-Match → 304 path.
Apply it to every version-keyed collab GET: issues/PRs thread + events, social, releases single/list/latest, pulls view, identity profile/org/team/invite surfaces (the 20 call sites in the table above). Leave git-content routes (summary/refs/resolve/tree/blob/commits) on SWR per §4 — those are correct as spec'd.
Fix the spec where it encodes the wrong call: 07_releases_stars.md:235 and 02_issues.md:258 change "ref-dependent class (SWR 60 s)" to the new class; add a sentence to 07_api.md §4 defining the third class and the rule for when it applies (mutability, not addressability, decides). Note 07_api.md §4 currently says "the two cache classes (the central design rule)" — this amendment touches a stated design rule, which is why this issue is flagged for the maintainer rather than just implemented.
Client-side belt-and-suspenders: on any successful mutation, invalidate() the affected useData entries and force a browser revalidation for the mutated resource (the SDK mutation helpers can issue the follow-up GET with If-None-Match stripped — a conditional GET the browser must forward under no-cache). This covers same-view updates without a reload; the header contract covers the reload case.
Audit for stragglers: a contract test asserting that any handler whose ETag is derived from a store/doc version (not a git sha) serves no-cache, not SWR — the same test proposed in #272's registry work would be a natural home.

Laws impact (decision requested)

Not a dependency-law issue (no new modules).
It amends the §9.2/§4 "two cache classes" design rule in 07_api.md — adding a third class for mutable collab state. The rule's intent ("SWR is safe because staleness is bounded by ref movement") is preserved; the amendment narrows where the class applies. Per repo law 12 (existing keys never change meaning), git-content routes keep byte-identical headers; only collab GETs change.
Alternative if the two-class rule is sacred: drop SWR to max-age=0 only (no stale window) for collab GETs — same user-visible fix, keeps the class count at two, but loses the ETag-mandated revalidation semantics clarity of no-cache. Both are one-line header changes; the difference is documentation framing.

Acceptance criteria

Add reaction / star / watch / release-edit → hard refresh → post-mutation state on the first refresh, every time (the flip-flop is gone).
Unchanged collab resources still revalidate to 304 (no bandwidth regression; verify If-None-Match handling on the new class).
Git-content routes (summary, refs, resolve, tree, blob, commits by name) still serve SWR-60 unchanged (byte-identical headers).
07_api.md §4 and the two feature specs document the class assignment rule; the per-package duplicated ccSWR constants are consolidated or reference the shared definition.
Mutation helpers invalidate + revalidate the client cache so same-view UI updates without reload (existing behavior preserved, now backed by the header contract).
Contract test: version-ETag GETs serve no-cache; sha-ETag GETs serve SWR/immutable as spec'd.

**Supersedes #259** (same root cause, wider blast radius, permanent fix proposed). #259 scoped this to the issue view; the same stale-serve behavior exists on stars/watches (`/{o}/{r}/api/social`), releases, pulls, and identity surfaces. Closing #259 in favor of this one is appropriate. ## The systemic problem Mutating collaborative state — reactions, stars, watches, release edits, issue patches — then refreshing can serve the **pre-mutation state**, and the next refresh shows the post-mutation state. It is intermittent by construction and affects every surface served with the SWR cache class: | Surface | ETag basis | File | |---|---|---| | Issue/PR thread (reactions, comments, patches) | `v<Thread.Version>` | `internal/issues/http.go:459` | | Social (stars, watches, forks) | store version token | `internal/social/http.go:248` | | Release single + list | store version | `internal/releases/http.go:355,371,380` | | Pull view | `HeadLive` | `internal/pulls/http.go:493` | | Identity (profile, orgs, teams, invites) | doc version | `internal/identity/http.go` (7 sites) | All use `ccSWR = "private, max-age=0, stale-while-revalidate=60"` (`api/env.go:582`, duplicated per package in `issues/http.go:175`, `social/http.go:185`). **Mechanics of the flip-flop:** `stale-while-revalidate=60` permits the browser to answer a refresh from its cache — the stale pre-mutation body — while revalidating in the background. Refresh 1 paints old state (reaction "gone", star "off"); the background revalidation lands; refresh 2 paints new state. Server state is correct at all times (every mutation is a versioned CAS put and the ETag *is* version-keyed) — the header contract is what licenses the staleness. **Why the client makes it worse:** the SPA's `useData` layer (`web/src/lib/data.js`) has its own TTL revalidation (5–30 s by surface), and mutations invalidate only the mutation's own cache entry — a hard refresh re-requests through the browser cache, where SWR serves stale *before* the client TTL logic even runs. ## Where the design went wrong The repo's own spec drew the line in the right place and the implementation crossed it: - `docs/go/07_api.md` §4 ("the central design rule") defines exactly **two cache classes**, both for **git content**: sha-addressed → immutable; **ref-dependent** (name-addressed git views) → SWR-60 + sha ETag. The rationale is sound: git content changes only via refs, so a ref-state ETag makes SWR safe. - Collaborative state is *not* ref-dependent — threads, social counters, releases, and identity docs mutate independently of any ref move. Their specs then borrowed the class anyway: `docs/features/07_releases_stars.md:235` explicitly says "single release / list / social GETs are ref-dependent class (SWR 60 s + ETag version token)" and `docs/features/02_issues.md:258` says "GET-by-num is ref-class SWR keyed on the header version." That was the wrong call: a version-keyed ETag gives correct **revalidation**, but SWR's stale-serve window is a **correctness** concession that git content can afford (refs move rarely, stale ≈ seconds old) and mutable collab state cannot (the user just clicked the button). ## Proposed permanent solution **Principle: the SWR stale-serve window is for content whose staleness is bounded by ref movement, not for user-mutable state. Any GET whose resource can change via a direct user action must not serve stale.** 1. **Introduce a third cache class** — `ccNoCache = "private, no-cache"` — for mutation-sensitive GETs: `Cache-Control: private, no-cache` + the existing version ETag. `no-cache` *requires* revalidation before every use (the browser still caches; the ETag still makes unchanged responses 304 with zero body), so the economics stay cheap — you lose only the stale-serve window, which is exactly the bug. This is a one-line class + per-site swap; `writeCached` already implements the If-None-Match → 304 path. 2. **Apply it to every version-keyed collab GET**: issues/PRs thread + events, social, releases single/list/latest, pulls view, identity profile/org/team/invite surfaces (the 20 call sites in the table above). Leave git-content routes (summary/refs/resolve/tree/blob/commits) on SWR per §4 — those are correct as spec'd. 3. **Fix the spec where it encodes the wrong call**: `07_releases_stars.md:235` and `02_issues.md:258` change "ref-dependent class (SWR 60 s)" to the new class; add a sentence to `07_api.md` §4 defining the third class and the rule for when it applies (mutability, not addressability, decides). Note `07_api.md` §4 currently says "the two cache classes (the central design rule)" — this amendment touches a stated design rule, which is why this issue is flagged for the maintainer rather than just implemented. 4. **Client-side belt-and-suspenders:** on any successful mutation, `invalidate()` the affected `useData` entries **and** force a browser revalidation for the mutated resource (the SDK mutation helpers can issue the follow-up GET with `If-None-Match` stripped — a conditional GET the browser must forward under `no-cache`). This covers same-view updates without a reload; the header contract covers the reload case. 5. **Audit for stragglers:** a contract test asserting that any handler whose ETag is derived from a store/doc version (not a git sha) serves `no-cache`, not SWR — the same test proposed in #272's registry work would be a natural home. ## Laws impact (decision requested) - Not a dependency-law issue (no new modules). - It **amends the §9.2/§4 "two cache classes" design rule** in `07_api.md` — adding a third class for mutable collab state. The rule's intent ("SWR is safe because staleness is bounded by ref movement") is preserved; the amendment narrows where the class applies. Per repo law 12 (existing keys never change meaning), git-content routes keep byte-identical headers; only collab GETs change. - Alternative if the two-class rule is sacred: drop SWR to `max-age=0` only (no stale window) for collab GETs — same user-visible fix, keeps the class count at two, but loses the ETag-mandated revalidation semantics clarity of `no-cache`. Both are one-line header changes; the difference is documentation framing. ## Acceptance criteria - [ ] Add reaction / star / watch / release-edit → hard refresh → post-mutation state on the **first** refresh, every time (the flip-flop is gone). - [ ] Unchanged collab resources still revalidate to 304 (no bandwidth regression; verify `If-None-Match` handling on the new class). - [ ] Git-content routes (summary, refs, resolve, tree, blob, commits by name) still serve SWR-60 unchanged (byte-identical headers). - [ ] `07_api.md` §4 and the two feature specs document the class assignment rule; the per-package duplicated `ccSWR` constants are consolidated or reference the shared definition. - [ ] Mutation helpers invalidate + revalidate the client cache so same-view UI updates without reload (existing behavior preserved, now backed by the header contract). - [ ] Contract test: version-ETag GETs serve `no-cache`; sha-ETag GETs serve SWR/immutable as spec'd.

crueber referenced this issue

2026-09-10 11:26:20 +00:00

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

crueber referenced this issue

2026-09-10 11:26:46 +00:00

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

crueber referenced this issue from a commit

2026-09-10 15:26:17 +00:00

Fix #280: third cache class (private, no-cache) for mutable collab state

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

2026-09-10 15:26:19 +00:00

Fix #280: third cache class for mutable state #292

crueber commented

2026-09-10 15:26:22 +00:00

Author

Owner

Fix PR: #292 (branch fix/issue-280) — third cache class private, no-cache applied across social, releases, pulls view, and identity with version-keyed ETags intact; git-content routes untouched. Table-driven httptest per surface, coverage ≥95% held, -race green. Not merged — review requested.

Fix PR: #292 (branch fix/issue-280) — third cache class `private, no-cache` applied across social, releases, pulls view, and identity with version-keyed ETags intact; git-content routes untouched. Table-driven httptest per surface, coverage ≥95% held, -race green. Not merged — review requested.

crueber referenced this issue from a commit

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

Fix #280: autodraft is tokenless, not version-keyed (doc precision)

crueber commented

2026-09-10 15:31:17 +00:00

Author

Owner

Review of PR #292 (fix/issue-280, third cache class). Verified in scratch worktree; docs-only follow-up pushed as 1084462.

pullETag completeness (load-bearing question) — COMPLETE, no gaps. internal/pulls/http.go:515 pullETag folds HeadLive + BaseLive + Thread.Version + PR.Version + Mergeable(State+ComputedAt). Enumerated against PullView (service.go:486): every view input is covered — (a) thread fields incl. title/state/labels/participants/comment_count/review_summary: every writer bumps Thread.Version (pulls title/state/comment/merge/force-push/subscribe in service.go:918,934,1006 merge.go:267 mergeable.go:230; issues label/assignee paths; review refreshSummary review/service.go:276); (b) PR fields incl. body/merged/draft/head SHA: all writers go through savePR which bumps PR.Version (service.go:132); (c) mergeable conflicts/rebaseable/merge-base are pure functions of (baseSHA,headSHA), both folded, and state flips always land with a fresh ComputedAt; (d) HeadRefOk is deterministic in (pr.Head.SHA, HeadLive); (e) events window: every append bumps Thread.Version, pagination params are per-URL cache keys so unfolded after_seq/n is safe; (f) check results are not rendered in the view (merge-time gate only) — nothing to fold. Same-second recompute edge is immaterial since conflicts cannot change without a sha change.

Other checks — all pass: getDiff stays SWR (http.go:550, sound: patch bytes change only via ref movement; 60s staleness is the class semantics); tokenless lists (orgs/teams lists, member single, autodraft) take ccMutable without ETag = always 200, small bodies, fine for collab pages; per-package ccMutable duplication matches the writeCached/matchETag precedent, no shared import, acceptable; stableViewETag (http_test.go:108) is test-only, zero production impact, and the old-304-test race fix is genuine (background mergeable recompute moved ComputedAt between first-read and 304 assert); law-6 cost model holds (headers-only change, zero new store round trips, git hot paths untouched; per-read revalidation cost is correct for user-driven collab pages); coverage pulls 97.7 / social 99.5 / releases 99.8 / identity 97.2 (all >=95); gofmt + go vet clean; no new non-stdlib imports (only stdlib strconv in identity test); doc entries accurate (07_api §4 third class + Decisions, 03_pull_requests, 01_identity_permissions, 07_releases_stars, 08_ui_sdk — the diff-row ETag correction to shipped 'SWR, no ETag' is correct).

Fixed directly: 07_releases_stars.md:323 grouped tokenless autodraft with 'version-keyed GETs' — reworded, pushed 1084462 (docs-only, no retest needed).

Tests: go test -race -count=1 on all four touched packages — all ok. Backend-only change; no browser per instructions.

MERGE RECOMMENDATION: ready to merge.

Review of PR #292 (fix/issue-280, third cache class). Verified in scratch worktree; docs-only follow-up pushed as 1084462. pullETag completeness (load-bearing question) — COMPLETE, no gaps. internal/pulls/http.go:515 pullETag folds HeadLive + BaseLive + Thread.Version + PR.Version + Mergeable(State+ComputedAt). Enumerated against PullView (service.go:486): every view input is covered — (a) thread fields incl. title/state/labels/participants/comment_count/review_summary: every writer bumps Thread.Version (pulls title/state/comment/merge/force-push/subscribe in service.go:918,934,1006 merge.go:267 mergeable.go:230; issues label/assignee paths; review refreshSummary review/service.go:276); (b) PR fields incl. body/merged/draft/head SHA: all writers go through savePR which bumps PR.Version (service.go:132); (c) mergeable conflicts/rebaseable/merge-base are pure functions of (baseSHA,headSHA), both folded, and state flips always land with a fresh ComputedAt; (d) HeadRefOk is deterministic in (pr.Head.SHA, HeadLive); (e) events window: every append bumps Thread.Version, pagination params are per-URL cache keys so unfolded after_seq/n is safe; (f) check results are not rendered in the view (merge-time gate only) — nothing to fold. Same-second recompute edge is immaterial since conflicts cannot change without a sha change. Other checks — all pass: getDiff stays SWR (http.go:550, sound: patch bytes change only via ref movement; 60s staleness is the class semantics); tokenless lists (orgs/teams lists, member single, autodraft) take ccMutable without ETag = always 200, small bodies, fine for collab pages; per-package ccMutable duplication matches the writeCached/matchETag precedent, no shared import, acceptable; stableViewETag (http_test.go:108) is test-only, zero production impact, and the old-304-test race fix is genuine (background mergeable recompute moved ComputedAt between first-read and 304 assert); law-6 cost model holds (headers-only change, zero new store round trips, git hot paths untouched; per-read revalidation cost is correct for user-driven collab pages); coverage pulls 97.7 / social 99.5 / releases 99.8 / identity 97.2 (all >=95); gofmt + go vet clean; no new non-stdlib imports (only stdlib strconv in identity test); doc entries accurate (07_api §4 third class + Decisions, 03_pull_requests, 01_identity_permissions, 07_releases_stars, 08_ui_sdk — the diff-row ETag correction to shipped 'SWR, no ETag' is correct). Fixed directly: 07_releases_stars.md:323 grouped tokenless autodraft with 'version-keyed GETs' — reworded, pushed 1084462 (docs-only, no retest needed). Tests: go test -race -count=1 on all four touched packages — all ok. Backend-only change; no browser per instructions. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-10 15:31:50 +00:00

crueber commented

2026-09-10 15:31:50 +00:00

Author

Owner

Fixed by PR #292 incl. review doc fix (third cache class everywhere mutable; pullETag complete; all gates green), merged. Closing.

crueber referenced this issue from a commit

2026-09-10 15:31:51 +00:00

Merge pull request 'Fix #280: third cache class for mutable state' (#292) from fix/issue-280 into main

crueber added the

git storage

label

2026-09-10 22:26:50 +00:00

crueber added this to the v1 milestone

2026-09-10 22:27:07 +00:00

crueber referenced this issue

2026-09-11 11:27:52 +00:00

Repo tab bar: open-count badges next to Issues and Pulls #319

crueber referenced this issue from a commit

2026-09-11 12:25:38 +00:00

Fix #319: open-count badges on Issues/Pulls tabs (07 §9.1, 08 §4)

crueber referenced this issue

2026-09-11 12:25:43 +00:00

Fix #319: open-count badges #322

crueber referenced this issue

2026-09-11 12:26:09 +00:00

Repo tab bar: open-count badges next to Issues and Pulls #319

crueber referenced this issue

2026-09-11 13:34:01 +00:00

Repo tab bar: open-count badges next to Issues and Pulls #319

crueber referenced this issue

2026-09-11 15:43:44 +00:00

Audit: laws & design-spec conformance across the codebase — findings documented as comments on this ticket #331

crueber referenced this issue

2026-09-11 17:22:05 +00:00

Audit: laws & design-spec conformance across the codebase — findings documented as comments on this ticket #331

crueber referenced this issue

2026-09-12 15:21:48 +00:00

Visibility bounces between public and private across refreshes — summary still served ccSWR (stale-serve window defeats the ~v ETag) #381

crueber referenced this issue

2026-09-12 15:26:25 +00:00

NEW LAW + systemic guard: cache-class is decided by MUTABILITY, not addressability — shared policy definition, contract test, and an AGENTS.md law so stale-serve flip-flops (#259/#280/#381) cannot recur #382

crueber commented

2026-09-12 15:27:05 +00:00

Author

Owner

Superseded by #382, which elevates this to a systemic fix: shared cache-policy definition, a route-enumeration contract test, a new AGENTS.md law (mutability decides the cache class), and the summary conversion from #381. Closing in favor of #382.