Prune expired entries from cooldown dicts on write

[dcellison]

Summary

Add lazy pruning to the PR review and issue triage cooldown dicts so they don't grow without bound over the lifetime of the service.

The problem

_review_cooldowns and _triage_cooldowns accumulate one entry per unique (repo, number) key. Entries are added by _record_review() and _record_triage() but never removed. Each entry is small (~120 bytes) and growth is slow, but a long-running service should not have unbounded memory growth.

The fix

_prune_expired() removes entries older than the cooldown threshold. Called from both _record_* functions before adding a new entry, so the dicts never grow beyond the number of unique PRs/issues active within the cooldown window.

Changes

Change	Detail
New _prune_expired() helper	Two-pass: collect expired keys, then delete. Avoids dict-mutation-during-iteration.
_record_review() signature	Added cooldown parameter so pruning uses the actual configured value
_record_review() body	Calls _prune_expired before adding
_record_triage() body	Calls _prune_expired with _TRIAGE_COOLDOWN_SECONDS before adding
Call site (line ~553)	Passes cooldown to _record_review()

Test plan

• test_removes_old_entries - entries older than max_age are pruned
• test_retains_recent_entries - entries within max_age survive
• test_empty_dict_is_noop - pruning an empty dict does nothing
• test_record_review_prunes_before_adding - old entries removed before new one added
• test_record_triage_prunes_before_adding - same for triage
• Existing cooldown tests updated with new cooldown parameter
• All 1106 tests pass, make check clean

Fixes #156

[dcellison]

Review by Kai

PR Review: fix/cooldown-dict-growth

The implementation is correct and clean. One test issue worth fixing.

---

Warning — test_record_triage_prunes_before_adding hardcodes a magic offset

tests/test_webhook.py, new test ~line 330:

_triage_cooldowns[("repo", 10)] = time.time() - 120

This sets the entry 120 seconds in the past and asserts it gets pruned. Whether that's true depends entirely on _TRIAGE_COOLDOWN_SECONDS. If the constant is, say, 3600 (1 hour), then 120 < 3600 and the entry is not expired — the assertion assert ("repo", 10) not in _triage_cooldowns will fail.

Compare with test_record_review_prunes_before_adding, which correctly anchors to the cooldown it controls:

_review_cooldowns[("repo", 1)] = time.time() - 500  # 500 > 300 passed to _record_review
_record_review("repo", 2, 300)

Fix: import _TRIAGE_COOLDOWN_SECONDS in the test and use time.time() - (_TRIAGE_COOLDOWN_SECONDS + 60) so the test is self-consistent regardless of the constant's value.

---

Suggestions (non-blocking)

• webhook.py, _record_triage vs _record_review symmetry: _record_review now takes a cooldown parameter while _record_triage reads _TRIAGE_COOLDOWN_SECONDS directly. The asymmetry is defensible (triage has one global cooldown), but worth a brief comment explaining the intentional difference.

• _prune_expired docstring: The docstring says "Called during record operations" but this is an implementation detail that could drift. Not a correctness issue.

---

Clean items

• Two-pass delete (collect then delete) correctly avoids mutation-during-iteration.
• >= boundary in _prune_expired is consistent with < in _should_skip_review.
• No thread-safety regression — asyncio's cooperative scheduling means there's no interleaving between _prune_expired and the dict write.
• Passing cooldown through the call site at webhook.py:576 is correct.

[dcellison]

Review by Kai

PR Review: fix/cooldown-dict-growth (updated)

The prior review's warning has been addressed. The updated diff is clean.

Addressed

test_record_triage_prunes_before_adding now correctly anchors to the constant:

_triage_cooldowns[("repo", 10)] = time.time() - (_TRIAGE_COOLDOWN_SECONDS + 60)

The + 60 margin ensures the entry is always expired regardless of the constant's value. This matches the pattern from test_record_review_prunes_before_adding.

No new issues found

• _prune_expired: two-pass delete is correct, >= boundary is consistent with < in _should_skip_review.
• _record_review(cooldown: float) vs _should_skip_review(cooldown: int): harmless Python duck-typing, not a real inconsistency.
• Module-level dict mutation in tests (_review_cooldowns.clear(), _triage_cooldowns.clear()) is pre-existing test design, not introduced here.
• No security issues — pruning operates on internal state only, no external input reaches the dict keys or timestamps.

Good to merge.