Visibility bounces between public and private across refreshes — summary still served ccSWR (stale-serve window defeats the ~v ETag) #381
Labels
No labels
actions
bug
cli
duplicate
enhancement
fork
forum
git storage
help wanted
insights
invalid
issues
moderation
oidc
ownership transfer
packages
pr/merge protection rules
projects
pull requests
question
releases
sponsorships
tags
webhooks
wiki
wontfix
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
crueber/walhub#381
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What's wrong
After changing a repo's visibility in repo settings (
/crueber/walhub/settings), refreshing the page repeatedly makes the visibility bounce betweenpublicandprivate — owner onlyon alternating refreshes. There is no authoritative answer visible to the user — the same page shows different values from refresh to refresh.Root cause (code evidence — the #280 flip-flop class, still open on the summary surface)
The visibility value is rendered from two different sources with different cache contracts, and one of them is the stale-serve window that #280 flagged:
The header badge reads the repo summary.
visibilityBadge(s())(web/src/pages/Repo.jsx:561-566) consumes the shared summary signal, whoseVisibilityfield is projected server-side (internal/api/summary.go:124,139—repoVisibility→GetAccess, the authoritativeaccess.jsonread).But the summary is served
ccSWR—writeCached(w, r, ccSWR, etag, …)atinternal/api/summary.go:187, whereccSWR = "private, max-age=0, stale-while-revalidate=60". The visibility flip does change the ETag (etag += "~v" + visibility, :181-185 — the #345 cache-trap mitigation landed as designed), but the SWR window still permits the browser to serve the stale pre-flip body on the next refresh while revalidating in the background. That reproduces the exact bounce: refresh 1 serves the old visibility (stale) and revalidates; refresh 2 shows the new one; flip again → repeat. The ETag suffix makes the revalidation correct; it does nothing about the stale serve.The settings select is a second, divergent source. The General tab seeds its visibility select from
GET …/accessviauseDatawith a 5 s TTL (web/src/pages/Settings.jsx:93,getAccess), prefilling once (getVis() === nullgate, :112-116). The access endpoint is servedccMutable(private, no-cache— the #280 fix,internal/identity/http.go:193), so it's authoritative — but within the 5 s client TTL the select can still show the pre-PUT value after a save, and the badge (SWR) can disagree with the select (no-cache) on the same screen.Server state is correct throughout:
PutAccessCAS-writesaccess.jsonand invalidates the identity access LRU (internal/identity/access.go:201), and the summary projection reads it fresh per request (repoVisibility→GetAccess, conditional GET on the LRU version —access.go:215-224, 174-196). This is purely a client/browser cache-contract bug, the same class #280 documented for threads/social/releases — the summary surface was left onccSWRbecause its ETag covers most mutable fields, but SWR's stale-serve window defeats ETag correctness for exactly the "changed it, then refreshed" case.Fix
ccSWR's stale-serve window for the mutable projections it now carries — visibility, open counts (#319), description (#235), mirror state (#320) are all user-mutable with no ref movement. Either:private, no-cache(the #280 class — ETag/304 economics kept, stale serve gone), orno-cacheendpoint and keep the summary SWR for ref-derived data only.(a) is a one-line change with the ETag already covering every mutable field (
~d,~m,~c,~vsuffixes all present); (b) is a wire change. Recommend (a) and amend #280's systemic ticket to include the summary call site in its scope.access:{full}entry used for prefill (or invalidate it insaveVisibility(), which currently only invalidatesrepo:andaccess:— verify the key matches the onegetAccessreads; both useaccess:{full}so the invalidate is correct, but the TTL still lets a pre-save doc seed the select within 5 s of page load).Acceptance criteria
Cache-Controlno longer permits stale-serve of a pre-flip body (no-cache class or equivalent), while unchanged summaries still revalidate to 304 (ETag economics preserved).Fix ready for review: PR #383 (#383) — recommended fix (a). Summary serves private, no-cache (new per-package ccMutable); ~d/~m/~c/~v suffixes verified intact with exact-composition test; 304 economics proven by test; settings select paints the PUT echo; #280 scope closed in 07_api Decisions with the summary listed. internal/api -race green, 95.2% coverage; node unit 750 pass (2 smoke failures are the foreign :8080 instance, unrelated).
REVIEW PR #383 (fix/issue-381 @
9361185) — verified in scratch worktree /tmp/pr383 (removed afterward); main left untouched; no docker/browser/live-instance touches (note: no real-browser proof per task scoping — reasoning + tests below).(1) Cache class exact — PASS. internal/api/env.go:701 ccMutable="private, no-cache"; summary.go:192 writeCached(ccMutable). private kept (visibility-filtered reads vary per caller — comment :697-699 states why bare ccNoCache is wrong). No stale-while-revalidate on the route (only remaining SWR sites are other routes; summary's old ccSWR gone).
(2) ETag economics — PASS. All four suffixes intact (~d :161, ~m :168, ~c :174, ~v :180). New TestSummaryMutableClassAndETagEconomics (visibility345_test.go:365) proves exact composed ETag, 304 on unchanged, flip public->private => 200 with ~vprivate then 304 again, and no-cache class on both 200s. Unchanged summaries still 304 (zero body) — economics kept.
(3) Law-6 cost — PASS. Header-only change; handler probes unchanged, no new sequential store trips. no-cache vs SWR: every summary GET now revalidates (one conditional request) instead of serving stale for 60s; revalidation is a cheap 304 (ETag-covered, LRU-backed version hits). Path is off push/sync/checkpoint budgets (never called there) — stated in code :189-191 and docs. No hot-path regression.
(4) Client echo paint — PASS. Settings.jsx:163 setVis(next.visibility ?? vis) paints the PUT echo authoritatively; server PUT returns accessView incl. visibility (identity/http_invites.go:226) with ?? vis fallback. Invalidation keys match seeds: access:{full} (:93 seed / :167 invalidate), repo:{full} (:166). 5s TTL kept deliberately; ttl=0 refetch-loop rationale is sound (signal-subscribed useData effect would resubscribe-loop).
(5) Badge+select agreement — PASS. Badge reads shared summary s() (Repo.jsx:561-566); summary is now no-cache server-side and invalidated on save, select paints PUT echo synchronously — same-screen disagreement window closed (#259 lesson honored: one authoritative write result seeds both).
(6) #280 scope — PASS. 07_api.md §4 table lists the summary in the mutable-collab class; §9 counts/visibility sections amended; Decisions entry closes the systemic scope with the summary explicitly listed as fixed; 06 §3 table + Apidocs string + 12_web_ui entry all agree.
(7) Other ccSWR surfaces with the same trap — FOLLOW-UPS ONLY (out of scope, not blockers): internal/api/repos_detailed.go:142 (listing rows carry visibility+mirror flags, ccSWR, no ETag — visibility flip serves stale ≤60s) and internal/api/profile.go:200 (owner profile bio/display-name PUT-editable, ccSWR, no ETag). Suggest filing follow-up issue(s) under #280 rather than expanding this PR. Ref-derived routes (refs/resolve/tree/blob/commits/discovery/owners_activity) are correctly SWR.
(8) Gates — PASS: internal/api -race green; coverage 95.2% (>=95%); gofmt clean; go vet clean; go build ./... clean. node --test: all unit suites pass except the 2 smoke.test.js server-dependent cases (they hit the foreign live :8080 instance, 401/no hashed asset — environmental, unrelated to the touched Settings.jsx/Apidocs.jsx which carry no new headless cover by design).
No fixes pushed (nothing to fix). MERGE RECOMMENDATION: ready to merge.
Fixed by PR #383 (review clean — all 8 points pass, ETag economics proven, #280 scope closed), merged. Closing.
Still reproducing after the ccMutable conversion - follow-up ticket #391 (the bounce survives the HTTP-cache fix, so the stale value comes from save-failure handling, the 5s-TTL CAS version, or store conditional-GET staleness; instrumented diagnosis required).