[minor-8] `repoBus.ring` never evicted — unbounded memory #93

New issue

Closed

opened 2026-09-05 02:40:26 +00:00 by crueber · 3 comments

crueber commented

2026-09-05 02:40:26 +00:00

Owner

[minor-8] `repoBus.ring` never evicted — unbounded memory

internal/notify/stream.go:152-159: b.ring[f.Repo] grows one 64-frame entry per repo ever published, never deleted (subscriber cleanup exists; ring cleanup doesn't). Unbounded memory growth over instance lifetime.

Fix

Bound it: evict-on-last-subscriber, LRU cap, or TTL — pick the shape matching the codebase (document the choice). Regression test (publish to N repos, unsubscribe all, assert ring shrinks / stays capped). Coverage gate holds.

Acceptance criteria

Ring memory bounded regardless of repo count / instance lifetime; test green -race.

# [minor-8] `repoBus.ring` never evicted — unbounded memory `internal/notify/stream.go:152-159`: `b.ring[f.Repo]` grows one 64-frame entry per repo ever published, never deleted (subscriber cleanup exists; ring cleanup doesn't). Unbounded memory growth over instance lifetime. ## Fix Bound it: evict-on-last-subscriber, LRU cap, or TTL — pick the shape matching the codebase (document the choice). Regression test (publish to N repos, unsubscribe all, assert ring shrinks / stays capped). Coverage gate holds. ## Acceptance criteria - [ ] Ring memory bounded regardless of repo count / instance lifetime; test green `-race`.

crueber referenced this issue from a commit

2026-09-05 03:02:15 +00:00

Fix #93: bound repoBus ring (evict-on-last-subscriber + 256-repo LRU cap)

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

2026-09-05 03:02:17 +00:00

Fix #93: bound repoBus ring #102

crueber commented

2026-09-05 03:02:20 +00:00

Author

Owner

Fixed by #102 (branch fix/issue-93): evict-on-last-subscriber + 256-repo LRU cap on repoBus.ring. Tests green -race, coverage 97.3%. Ready for review — not merging per workflow.

crueber referenced this issue from a commit

2026-09-05 03:04:21 +00:00

Review fix: single-shot repoBus eviction (issue #93)

crueber commented

2026-09-05 03:04:26 +00:00

Author

Owner

PR #102 review (fix/issue-93, bound repoBus ring) — verified in scratch worktree at 6974431 + review fix ebe4c08.

EVICTION CORRECTNESS (pass): unsubscribe drops ring+last alongside subs on last-subscriber-out (stream.go:288-294) — mirrors the subs discipline; LRU cap enforced on every publish (stream.go:194), publish-with-no-subscribers path included (unconditional ring append before the cap check). Eviction deletes ring/last map entries only (stream.go:249-250) — subs entries and live channels never touched; a present subscriber always gets its frames via the post-eviction fan-out (stream.go:197+). All-subscribed overflow falls back to evicting oldest-subscribed replay while its channel stays open — covered by TestRepoRingEvictAllSubscribedKeepsLive.

RECENCY/LOCKS (pass): last/clk ride the pre-existing b.mu everywhere (publish, subscribe bump, unsubscribe delete, evict). No new locks, no timers/goroutines, single mutex so no lock-order issue. ### Concurrency subsection present per law 3.

CAP (sane): 256 repos x 64 frames = 16,384 frames worst case (~2-6MB for RepoFrame-sized structs) — sane in-process bound, stated in code + doc.

POST-EVICTION ATTACH (pass): subscribe on missing ring returns empty replay + live channel (stream.go:276 nil-map lookup, no error); sole production caller collab.go:90 ranges over recent (empty = no-op) then serves live tail. Timeline/API remains the durable backfill — doc states this.

CLOSE SAFETY (pass): close(ch) exists only in the unsubscribe once-path (stream.go:297); evicted rings are never ranged over or closed elsewhere (grep: ring touched only in publish/subscribe/unsubscribe/evict/ringCount, all mu-held).

TESTS GENUINE (pass): pre-fix tree has deletes only for subs maps, never ring (confirmed via 4d36584:stream.go) — the ringCount==0 and ringCount==256 assertions target exactly the missing behavior. New tests: evict-on-unsub, LRU cap + oldest-first + newest-retained, prefers-idle, all-subscribed-keeps-live (incl. deterministic live-delivery check).

SMALL FIX PUSHED (ebe4c08): publish loop 'for len(ring)>max' -> 'if' + comment (stream.go:194). One publish adds at most one repo key so a single eviction always restores the bound; removes the theoretical lock-held spin if ring/last ever diverged (evictLRULocked no-ops on !found). Behavior-identical.

VERIFY (post-fix, /tmp/pr102): gofmt clean; go vet clean; go test -race ./internal/notify/... ok; TestRepoRing* -race -count=10 ok; coverage 97.3% (>=95% gate). Doc entry (06 Decisions) accurate: pairs both mechanisms, notes live-tail nuance + worst case.

RECOMMENDATION: ready to merge.

PR #102 review (fix/issue-93, bound repoBus ring) — verified in scratch worktree at 6974431 + review fix ebe4c08. EVICTION CORRECTNESS (pass): unsubscribe drops ring+last alongside subs on last-subscriber-out (stream.go:288-294) — mirrors the subs discipline; LRU cap enforced on every publish (stream.go:194), publish-with-no-subscribers path included (unconditional ring append before the cap check). Eviction deletes ring/last map entries only (stream.go:249-250) — subs entries and live channels never touched; a present subscriber always gets its frames via the post-eviction fan-out (stream.go:197+). All-subscribed overflow falls back to evicting oldest-subscribed replay while its channel stays open — covered by TestRepoRingEvictAllSubscribedKeepsLive. RECENCY/LOCKS (pass): last/clk ride the pre-existing b.mu everywhere (publish, subscribe bump, unsubscribe delete, evict). No new locks, no timers/goroutines, single mutex so no lock-order issue. ### Concurrency subsection present per law 3. CAP (sane): 256 repos x 64 frames = 16,384 frames worst case (~2-6MB for RepoFrame-sized structs) — sane in-process bound, stated in code + doc. POST-EVICTION ATTACH (pass): subscribe on missing ring returns empty replay + live channel (stream.go:276 nil-map lookup, no error); sole production caller collab.go:90 ranges over recent (empty = no-op) then serves live tail. Timeline/API remains the durable backfill — doc states this. CLOSE SAFETY (pass): close(ch) exists only in the unsubscribe once-path (stream.go:297); evicted rings are never ranged over or closed elsewhere (grep: ring touched only in publish/subscribe/unsubscribe/evict/ringCount, all mu-held). TESTS GENUINE (pass): pre-fix tree has deletes only for subs maps, never ring (confirmed via 4d36584:stream.go) — the ringCount==0 and ringCount==256 assertions target exactly the missing behavior. New tests: evict-on-unsub, LRU cap + oldest-first + newest-retained, prefers-idle, all-subscribed-keeps-live (incl. deterministic live-delivery check). SMALL FIX PUSHED (ebe4c08): publish loop 'for len(ring)>max' -> 'if' + comment (stream.go:194). One publish adds at most one repo key so a single eviction always restores the bound; removes the theoretical lock-held spin if ring/last ever diverged (evictLRULocked no-ops on !found). Behavior-identical. VERIFY (post-fix, /tmp/pr102): gofmt clean; go vet clean; go test -race ./internal/notify/... ok; TestRepoRing* -race -count=10 ok; coverage 97.3% (>=95% gate). Doc entry (06 Decisions) accurate: pairs both mechanisms, notes live-tail nuance + worst case. RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 03:04:52 +00:00

crueber referenced this issue from a commit

2026-09-05 03:04:52 +00:00

Merge pull request 'Fix #93: bound repoBus ring' (#102) from fix/issue-93 into main

crueber commented

2026-09-05 03:04:52 +00:00

Author

Owner

Fixed by PR #102 incl. review eviction-loop fixup (ring bounded 256 repos, evict-on-unsubscribe; 97.3% coverage), merged. Closing.

crueber added the

webhooks