ccSWR stale-serve trap on repos_detailed (visibility+mirror flags, no ETag)

crueber commented

2026-09-12 15:34:53 +00:00

Owner

Follow-up flagged by the #381 review (PR #383 findings). internal/api/repos_detailed.go:142 serves visibility+mirror flags under ccSWR with no ETag — the same stale-serve trap class #381 fixed on the summary. Either move to ccMutable (no-cache + ETag) per the #381 pattern or document why SWR is safe there.

crueber referenced this issue from a commit

2026-09-12 15:45:35 +00:00

Fix #384: detailed route off SWR (ccMutable + content ETag, 07_api Decisions)

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

2026-09-12 15:45:36 +00:00

Fix #384: detailed route off SWR #387

crueber commented

2026-09-12 15:45:39 +00:00

Author

Owner

Fix open as PR #387: #387 — detailed route moved to ccMutable + content ETag (200->304->flip->200->304 for visibility and mirror), -race + 95.3% cover, docs updated. Not merging per instructions.

Fix open as PR #387: https://git.packden.us/crueber/walhub/pulls/387 — detailed route moved to ccMutable + content ETag (200->304->flip->200->304 for visibility and mirror), -race + 95.3% cover, docs updated. Not merging per instructions.

crueber commented

2026-09-12 15:47:46 +00:00

Author

Owner

Review of PR #387 (fix/issue-384, commit a5a6f84) — verified in scratch worktree /tmp/pr387 (removed afterward), main worktree untouched/clean. No browser (tests + reasoning only, per task).

(1) Mutable projections — COMPLETE. detailedETag hashes json.Marshal(out) AFTER fillMirrorFlags + fillVisibilityFlags (repos_detailed.go:142-154), i.e. the entire rendered rows: Name (row-set changes from visibility filtering), all catalog fields (size_bytes, object_count, head_seq, updated_at, last_commit_sha/time, last_push_at), Mirror, MirrorUpstream, Visibility, plus row order (JSON array order). No mutable field can escape the hash by construction; the doc-comment enumeration (repos_detailed.go:159-165) matches the RepoSizeRow struct field-for-field.

(2) ETag sound — YES. json.Marshal of []RepoSizeRow is deterministic (fixed struct field order, no maps/floats, no time.Now in path; out is make()-built so never nil -> [] not null). FNV-1a is non-randomized. Query params are reflected (FilterSort runs before hashing, :129-154); owner-scoping is per-URL so cross-owner hash equality is harmless. writeCached quotes the bare d and does weak/any-of If-None-Match matching (env.go:742-754) — consistent with the summary pattern. Non-blocking nit: 32-bit hash — accidental-collision 304 risk is negligible for a listing validator, no change requested.

(3) Class exact — YES. ccMutable = private, no-cache (env.go:701, reused from #383 — correctly no new constant). Test asserts exact equality plus explicit stale-while-revalidate absence (repos_detailed_test.go:270-273). private retained, which the visibility-filtered reads need.

(4) Economics proven — YES. TestDetailedMutableClassAndETagEconomics: 200 -> 304 -> visibility flip -> 200 -> 304 -> mirror sidecar add -> 200 -> 304, incl. MirrorUpstream URL assertion (:308-324) and exact-class re-pin after flip (:299).

(5) Law 6 — CLEAN. detailedETag is pure CPU over already-rendered rows; zero new store calls; catalog + sidecar/visibility probes unchanged; read-only listing off the push/sync/checkpoint budgets.

(6) Gate results (scratch worktree): gofmt clean; go vet clean; go test ./internal/api/ -race green (4.6s); coverage 95.3% >= 95% (detailedETag 100%); go build ./... clean. Docs accurate: 07_api §4 class row, §8 class line, and Decisions bullet all updated in the same change (law 12).

#381 pattern fidelity — FAITHFUL with justified deltas: ccMutable reuse + economics-test shape mirror #383; content-hash replaces ~suffix with sound rationale (N rows/N tips, documented in code + Decisions). #383 extras correctly N/A: 06 §3 table never tabulated this route, Apidocs.jsx never listed it, no client behavior changes (server headers only).

Sibling observation (not a blocker, not in #384 scope): GET /api/v1/owners/detailed (owners_activity.go:215) stays ccSWR, but its rows (name/is_org/repo_count/activity) carry no visibility/mirror badge projections, so the #384 stale-badge trap does not apply the same way. owner profile ccSWR is already tracked as #385.

No fixes pushed — nothing fix-worthy found. MERGE RECOMMENDATION: ready to merge.

Review of PR #387 (fix/issue-384, commit a5a6f84) — verified in scratch worktree /tmp/pr387 (removed afterward), main worktree untouched/clean. No browser (tests + reasoning only, per task). (1) Mutable projections — COMPLETE. detailedETag hashes json.Marshal(out) AFTER fillMirrorFlags + fillVisibilityFlags (repos_detailed.go:142-154), i.e. the entire rendered rows: Name (row-set changes from visibility filtering), all catalog fields (size_bytes, object_count, head_seq, updated_at, last_commit_sha/time, last_push_at), Mirror, MirrorUpstream, Visibility, plus row order (JSON array order). No mutable field can escape the hash by construction; the doc-comment enumeration (repos_detailed.go:159-165) matches the RepoSizeRow struct field-for-field. (2) ETag sound — YES. json.Marshal of []RepoSizeRow is deterministic (fixed struct field order, no maps/floats, no time.Now in path; out is make()-built so never nil -> [] not null). FNV-1a is non-randomized. Query params are reflected (FilterSort runs before hashing, :129-154); owner-scoping is per-URL so cross-owner hash equality is harmless. writeCached quotes the bare d<hex> and does weak/any-of If-None-Match matching (env.go:742-754) — consistent with the summary pattern. Non-blocking nit: 32-bit hash — accidental-collision 304 risk is negligible for a listing validator, no change requested. (3) Class exact — YES. ccMutable = private, no-cache (env.go:701, reused from #383 — correctly no new constant). Test asserts exact equality plus explicit stale-while-revalidate absence (repos_detailed_test.go:270-273). private retained, which the visibility-filtered reads need. (4) Economics proven — YES. TestDetailedMutableClassAndETagEconomics: 200 -> 304 -> visibility flip -> 200 -> 304 -> mirror sidecar add -> 200 -> 304, incl. MirrorUpstream URL assertion (:308-324) and exact-class re-pin after flip (:299). (5) Law 6 — CLEAN. detailedETag is pure CPU over already-rendered rows; zero new store calls; catalog + sidecar/visibility probes unchanged; read-only listing off the push/sync/checkpoint budgets. (6) Gate results (scratch worktree): gofmt clean; go vet clean; go test ./internal/api/ -race green (4.6s); coverage 95.3% >= 95% (detailedETag 100%); go build ./... clean. Docs accurate: 07_api §4 class row, §8 class line, and Decisions bullet all updated in the same change (law 12). #381 pattern fidelity — FAITHFUL with justified deltas: ccMutable reuse + economics-test shape mirror #383; content-hash replaces ~suffix with sound rationale (N rows/N tips, documented in code + Decisions). #383 extras correctly N/A: 06 §3 table never tabulated this route, Apidocs.jsx never listed it, no client behavior changes (server headers only). Sibling observation (not a blocker, not in #384 scope): GET /api/v1/owners/detailed (owners_activity.go:215) stays ccSWR, but its rows (name/is_org/repo_count/activity) carry no visibility/mirror badge projections, so the #384 stale-badge trap does not apply the same way. owner profile ccSWR is already tracked as #385. No fixes pushed — nothing fix-worthy found. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

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

crueber referenced this issue from a commit

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

Merge pull request 'Fix #384: detailed route off SWR' (#387) from fix/issue-384 into main

crueber commented

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

Author

Owner

Fixed by PR #387 (review clean; ETag covers all mutable projections by construction, economics proven, #381 pattern faithful), merged. Closing.