zombor/pergamum
1
0
Fork 0
Code Issues Pull requests 1 Projects Releases Packages 2 Wiki Activity Actions

fix(nav): bust book-count-by-library cache on library create + scan complete (bookshelf-k4jrb) #1213

Closed
zombor wants to merge 1 commit from bd-bookshelf-k4jrb into main
pull from: bd-bookshelf-k4jrb
merge into: zombor:main
Conversation 2 Commits 1 Files changed 7 +311 -1
zombor commented 2026-07-23 23:57:30 +00:00
Owner
Copy link

Summary

  • The sidebar book-count badge was missing for up to 1h on newly-created/populated libraries because CountBooksByLibrary results were cached with a 1h TTL via NavCache.
  • Add NavCache.Invalidate() (clears c.entry under c.mu) and WrapInt64MapWithInvalidate (returns both the caching func and an invalidate func).
  • Wire the invalidate func to the library-create handler (POST /libraries) and the scan RunScan activity so the cache is busted on the two events that change book counts.
  • appwire.Deps.InvalidateNavBookCountByLibrary carries the func through to library.Deps.InvalidateBookCountCache without cross-package coupling.

Test plan

  • NavCache.Invalidate: after prime + Invalidate, next Get calls the underlying function again without advancing the injected clock past TTL
  • WrapInt64MapWithInvalidate nil/zero-TTL cases return nil func + no-op invalidate
  • library.createHandler: InvalidateBookCountCache is called on successful create, NOT called on create error
  • make test passes (all 100% coverage maintained)
  • make lint clean on changed packages

Closes bead bookshelf-k4jrb on merge.

## Summary - The sidebar book-count badge was missing for up to 1h on newly-created/populated libraries because `CountBooksByLibrary` results were cached with a 1h TTL via `NavCache`. - Add `NavCache.Invalidate()` (clears `c.entry` under `c.mu`) and `WrapInt64MapWithInvalidate` (returns both the caching func and an invalidate func). - Wire the invalidate func to the library-create handler (`POST /libraries`) and the scan `RunScan` activity so the cache is busted on the two events that change book counts. - `appwire.Deps.InvalidateNavBookCountByLibrary` carries the func through to `library.Deps.InvalidateBookCountCache` without cross-package coupling. ## Test plan - [ ] `NavCache.Invalidate`: after prime + Invalidate, next Get calls the underlying function again without advancing the injected clock past TTL - [ ] `WrapInt64MapWithInvalidate` nil/zero-TTL cases return nil func + no-op invalidate - [ ] `library.createHandler`: `InvalidateBookCountCache` is called on successful create, NOT called on create error - [ ] `make test` passes (all 100% coverage maintained) - [ ] `make lint` clean on changed packages Closes bead bookshelf-k4jrb on merge.
fix(nav): bust book-count-by-library cache on library create + scan complete (bookshelf-k4jrb)
All checks were successful
/ E2E API (pull_request) Successful in 2m28s
Details
/ JS Unit Tests (pull_request) Successful in 1m28s
Details
/ Test Race (pull_request) Successful in 4m33s
Details
/ Coverage (pull_request) Successful in 5m7s
Details
/ Integration (pull_request) Successful in 5m28s
Details
/ Lint (pull_request) Successful in 6m38s
Details
/ E2E Browser (pull_request) Successful in 5m5s
Details
b5aeb5553a
 
Add NavCache.Invalidate() and WrapInt64MapWithInvalidate() so callers can
clear the sidebar book-count badge cache on mutating events instead of
waiting for the 1h TTL to expire.

Wire the invalidate func to:
- library createHandler (badge appears immediately after library create)
- wfScanDeps.RunScan wrapper in app.New (badge updates after scan)

Add Deps.InvalidateBookCountCache + appwire.Deps.InvalidateNavBookCountByLibrary
to carry the invalidate func from the nav-cache wiring through to the library
handler without introducing cross-package coupling.

Closes bead bookshelf-k4jrb on merge.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
zombor commented 2026-07-24 00:08:12 +00:00
Author
Owner
Copy link

Security Review — PR #1213 (nav-count cache invalidation, bd-bookshelf-k4jrb)

Scope: git diff e1e169f7...b5aeb555 — NavCache.Invalidate + WrapInt64MapWithInvalidate (internal/middleware/nav_cache.go), wiring in internal/app/app.go, the Deps.InvalidateBookCountCache field + call site in internal/library/handler.go, plus internal/library/wire.go and internal/appwire/appwire.go.

Assessment against the requested focus areas

(1) Multi-user cache correctness — no cross-user leak. CountBooksByLibrary is a global (all-libraries) map both before and after this PR; the change only swaps WrapInt64Map → WrapInt64MapWithInvalidate (identical Get semantics) and adds a global Invalidate. Per-user surfacing is unchanged: applyNavCounts (nav.go:374) copies a count onto a sidebar entry ONLY when its ID is already present in the requesting user's user-scoped libs slice, so a user still cannot see a badge for a library they cannot access. Invalidation clears the global entry → the next Get recomputes the same global map, still re-filtered per-user at apply time. No new leakage.

(2) Create-path authz unchanged. POST /libraries remains gated by g.ManipulateLibrary (routes.go:59). InvalidateBookCountCache is invoked inside createHandler only after Create succeeds (handler.go:242) and is nil-guarded. No authz surface changed.

(3) No DoS / unbounded work. Invalidate just nils the cached entry; the next render triggers at most ONE bounded CountBooksByLibraryMap fill, protected by the existing single-flight/stampede guard in Get. Not per-request work; the two triggers (admin-gated create, scan-complete) are infrequent.

(4) Thread-safety — no data race. Invalidate takes c.mu.Lock(), sets entry = nil, unlocks; Get uses the same mutex. No memory race.

(5) No secret/PII in logs. Create path logs only library_id + trace_id; audit record carries the user-supplied library name (not a secret). No tokens/keys echoed.

Findings

[MINOR] internal/middleware/nav_cache.go:54 — Invalidate can be swallowed by a concurrent in-flight fill
If Invalidate() runs while a Get fill is in-flight, the fill re-acquires the lock afterward and repopulates c.entry (nav_cache.go:90-92) with a value computed just before the invalidation — leaving one TTL cycle of stale count. Purely a benign staleness window for a badge number (self-corrects at TTL); no memory-safety or security impact. If exact freshness were ever required, invalidate could also drop the in-flight result. Not blocking.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Security Review — PR #1213 (nav-count cache invalidation, bd-bookshelf-k4jrb) Scope: `git diff e1e169f7...b5aeb555` — `NavCache.Invalidate` + `WrapInt64MapWithInvalidate` (`internal/middleware/nav_cache.go`), wiring in `internal/app/app.go`, the `Deps.InvalidateBookCountCache` field + call site in `internal/library/handler.go`, plus `internal/library/wire.go` and `internal/appwire/appwire.go`. ### Assessment against the requested focus areas **(1) Multi-user cache correctness — no cross-user leak.** `CountBooksByLibrary` is a global (all-libraries) map both before and after this PR; the change only swaps `WrapInt64Map` → `WrapInt64MapWithInvalidate` (identical `Get` semantics) and adds a global `Invalidate`. Per-user surfacing is unchanged: `applyNavCounts` (nav.go:374) copies a count onto a sidebar entry ONLY when its `ID` is already present in the requesting user's user-scoped `libs` slice, so a user still cannot see a badge for a library they cannot access. Invalidation clears the global entry → the next `Get` recomputes the same global map, still re-filtered per-user at apply time. No new leakage. **(2) Create-path authz unchanged.** `POST /libraries` remains gated by `g.ManipulateLibrary` (routes.go:59). `InvalidateBookCountCache` is invoked inside `createHandler` only after `Create` succeeds (handler.go:242) and is nil-guarded. No authz surface changed. **(3) No DoS / unbounded work.** `Invalidate` just nils the cached entry; the next render triggers at most ONE bounded `CountBooksByLibraryMap` fill, protected by the existing single-flight/stampede guard in `Get`. Not per-request work; the two triggers (admin-gated create, scan-complete) are infrequent. **(4) Thread-safety — no data race.** `Invalidate` takes `c.mu.Lock()`, sets `entry = nil`, unlocks; `Get` uses the same mutex. No memory race. **(5) No secret/PII in logs.** Create path logs only `library_id` + `trace_id`; audit record carries the user-supplied library name (not a secret). No tokens/keys echoed. ### Findings [MINOR] internal/middleware/nav_cache.go:54 — Invalidate can be swallowed by a concurrent in-flight fill If `Invalidate()` runs while a `Get` fill is in-flight, the fill re-acquires the lock afterward and repopulates `c.entry` (nav_cache.go:90-92) with a value computed just before the invalidation — leaving one TTL cycle of stale count. Purely a benign staleness window for a badge number (self-corrects at TTL); no memory-safety or security impact. If exact freshness were ever required, invalidate could also drop the in-flight result. Not blocking. REVIEW VERDICT: 0 blocker, 0 major, 1 minor
zombor commented 2026-07-24 00:09:37 +00:00
Author
Owner
Copy link

Adversarial Code Review: PR #1213 (bookshelf-k4jrb)

Verdict

✓ REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Detailed Checks

1. Mutex Safety on Invalidate() and Get() — PASS

  • Invalidate() (nav_cache.go:54–62) correctly acquires/releases the mutex before modifying c.entry
  • Get() (nav_cache.go:56–98) also holds the mutex when reading/modifying c.entry
  • No window for data races; concurrent Get and Invalidate are serialized by the mutex

2. Targeted Invalidation — PASS

  • Only CountBooksByLibrary cache is wrapped and invalidated (app.go:504)
  • Other nav caches (CountAllBooks, CountDistinctSeries, CountAuthors, etc.) remain unaffected
  • Invalidation is surgically precise

3. Invalidate Called on SUCCESS Only — PASS

  • handler.go:242–245: invalidate call is placed AFTER successful Create() and BEFORE audit logging
  • Error path (lines 213–230) returns early, never reaching the invalidate call
  • Tests verify both paths: "InvalidateBookCountCache is called on successful create" ✓ and "NOT called on create error" ✓

4. Workflow Command-Sequence Safety — PASS

  • Invalidate call is OUTSIDE workflow in HTTP handler (handler.go) — safe
  • Invalidate call inside RunScan activity (app.go:530–534) happens AFTER the original scan completes — does NOT emit new go-workflows commands, just clears a local in-process cache
  • Standalone worker (cmd/pergamum/worker.go) calls BuildScanDeps unwrapped; no wrapping occurs — comment at app.go:524–526 correctly documents this
  • Zero workflow-command-sequence violations

5. Multi-user Safety — PASS

  • CountBooksByLibrary returns a global map of all library_id → count (no user scoping)
  • This is intentional and documented (middleware/nav.go:29–33): "safe to cache globally because applyNavCounts only looks up entries for libraries already in the user's sidebar — which are themselves user-scoped"
  • Invalidating the entire cache refreshes for all users on next request; each user still sees only their accessible libraries
  • No per-user data leak

6. Black-box Testing, Ginkgo Conventions, 100% Coverage — PASS

  • handler_test.go: package library_test (black-box) ✓
  • nav_cache_test.go: package middleware_test (black-box) ✓
  • Proper Ginkgo structure: BeforeEach (setup), JustBeforeEach (invocation), It (exactly one Expect per block)
  • Tests cover:
    • Invalidate on successful create (handler_test.go:835–860)
    • Invalidate NOT called on create error (handler_test.go:866–895)
    • NavCache.Invalidate() behavior (nav_cache_test.go:328–377)
    • WrapInt64MapWithInvalidate with nil fn, zero TTL, positive TTL (nav_cache_test.go:389–505)
  • Curried DI pattern used consistently: deps = library.Deps{...} with injected functions
  • Nil-guarding: both handler and wrapper functions check for nil before invoking

7. Dependency Wiring — PASS

  • app.go creates invalidateBookCountByLibrary before workflow engine setup (line 504), ensuring it's available for both handler and scan-activity wrapper (line 505)
  • wire.go correctly assigns d.InvalidateNavBookCountByLibrary to library.Deps.InvalidateBookCountCache (line 88)
  • Handler nil-guards the callback (handler.go:242)
  • No ordering or initialization issues

Summary

The PR is clean and well-executed. Cache invalidation is correctly targeted, properly guarded against nil, placed at success-only points, and safe from workflow-command-sequence violations. Multi-user scoping is preserved. Tests are comprehensive, black-box, and follow Ginkgo conventions. Ready to merge.

## Adversarial Code Review: PR #1213 (bookshelf-k4jrb) ### Verdict **✓ REVIEW VERDICT: 0 blocker, 0 major, 0 minor** --- ### Detailed Checks #### 1. Mutex Safety on `Invalidate()` and `Get()` — PASS - `Invalidate()` (nav_cache.go:54–62) correctly acquires/releases the mutex before modifying `c.entry` - `Get()` (nav_cache.go:56–98) also holds the mutex when reading/modifying `c.entry` - No window for data races; concurrent `Get` and `Invalidate` are serialized by the mutex #### 2. Targeted Invalidation — PASS - Only `CountBooksByLibrary` cache is wrapped and invalidated (app.go:504) - Other nav caches (`CountAllBooks`, `CountDistinctSeries`, `CountAuthors`, etc.) remain unaffected - Invalidation is surgically precise #### 3. Invalidate Called on SUCCESS Only — PASS - handler.go:242–245: invalidate call is placed AFTER successful `Create()` and BEFORE audit logging - Error path (lines 213–230) returns early, never reaching the invalidate call - Tests verify both paths: "InvalidateBookCountCache is called on successful create" ✓ and "NOT called on create error" ✓ #### 4. Workflow Command-Sequence Safety — PASS - Invalidate call is OUTSIDE workflow in HTTP handler (handler.go) — safe - Invalidate call inside `RunScan` activity (app.go:530–534) happens AFTER the original scan completes — does NOT emit new go-workflows commands, just clears a local in-process cache - Standalone worker (cmd/pergamum/worker.go) calls `BuildScanDeps` unwrapped; no wrapping occurs — comment at app.go:524–526 correctly documents this - Zero workflow-command-sequence violations #### 5. Multi-user Safety — PASS - `CountBooksByLibrary` returns a global map of all library_id → count (no user scoping) - This is intentional and documented (middleware/nav.go:29–33): "safe to cache globally because applyNavCounts only looks up entries for libraries already in the user's sidebar — which are themselves user-scoped" - Invalidating the entire cache refreshes for all users on next request; each user still sees only their accessible libraries - No per-user data leak #### 6. Black-box Testing, Ginkgo Conventions, 100% Coverage — PASS - `handler_test.go`: package `library_test` (black-box) ✓ - `nav_cache_test.go`: package `middleware_test` (black-box) ✓ - Proper Ginkgo structure: `BeforeEach` (setup), `JustBeforeEach` (invocation), `It` (exactly one Expect per block) - Tests cover: - Invalidate on successful create (handler_test.go:835–860) - Invalidate NOT called on create error (handler_test.go:866–895) - NavCache.Invalidate() behavior (nav_cache_test.go:328–377) - WrapInt64MapWithInvalidate with nil fn, zero TTL, positive TTL (nav_cache_test.go:389–505) - Curried DI pattern used consistently: `deps = library.Deps{...}` with injected functions - Nil-guarding: both handler and wrapper functions check for nil before invoking #### 7. Dependency Wiring — PASS - `app.go` creates `invalidateBookCountByLibrary` before workflow engine setup (line 504), ensuring it's available for both handler and scan-activity wrapper (line 505) - `wire.go` correctly assigns `d.InvalidateNavBookCountByLibrary` to `library.Deps.InvalidateBookCountCache` (line 88) - Handler nil-guards the callback (handler.go:242) - No ordering or initialization issues --- ### Summary The PR is clean and well-executed. Cache invalidation is correctly targeted, properly guarded against nil, placed at success-only points, and safe from workflow-command-sequence violations. Multi-user scoping is preserved. Tests are comprehensive, black-box, and follow Ginkgo conventions. Ready to merge.
zombor closed this pull request 2026-07-24 18:56:37 +00:00
All checks were successful
/ E2E API (pull_request) Successful in 2m28s
Required
Details
/ JS Unit Tests (pull_request) Successful in 1m28s
Details
/ Test Race (pull_request) Successful in 4m33s
Required
Details
/ Coverage (pull_request) Successful in 5m7s
Required
Details
/ Integration (pull_request) Successful in 5m28s
Required
Details
/ Lint (pull_request) Successful in 6m38s
Required
Details
/ E2E Browser (pull_request) Successful in 5m5s
Required
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
zombor/pergamum!1213
No description provided.