Skip to content

fix(discovery-index): bound TtlCache by a max-entry cap enforced on set - #10135

Closed
bitfathers94 wants to merge 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-10029
Closed

fix(discovery-index): bound TtlCache by a max-entry cap enforced on set#10135
bitfathers94 wants to merge 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-10029

Conversation

@bitfathers94

Copy link
Copy Markdown
Contributor

fix(discovery-index): bound TtlCache by a max-entry cap enforced on set

Without a cap, a key that is never re-read (e.g. a scope no caller repeats,
or a soft-claim that's never released) stays resident in the long-lived
module-level caches for the whole process lifetime. set() now drops
already-expired entries first, then evicts the oldest-inserted entry until
the store is back at or under the cap.

Closes #10029

Without a cap, a key that is never re-read (e.g. a scope no caller repeats,
or a soft-claim that's never released) stays resident in the long-lived
module-level caches for the whole process lifetime. set() now drops
already-expired entries first, then evicts the oldest-inserted entry until
the store is back at or under the cap.
@bitfathers94
bitfathers94 requested a review from JSONbored as a code owner July 31, 2026 08:41
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - fixes required

Review updated: 2026-07-31 08:51:23 UTC

3 files · 1 AI reviewer · no blockers · CI failing · unstable

🛑 Suggested Action - Fix Blockers

Review summary
This adds an explicit max-entry cap to TtlCache, enforced in set(): drop expired entries first, then evict oldest-inserted (Map insertion order) until under cap, and wires the default cap into all three long-lived cache instances in server.ts. The eviction logic is correct — evictForCapacity only fires when inserting a genuinely new key at-or-over capacity, and the two-phase (expired-first, then LRU-by-insertion) approach matches the well-designed regression/eviction tests. This closes the stated issue (#10029: unbounded key growth for never-re-read keys) and is a narrow, well-tested fix.

Nits — 3 non-blocking
  • test/unit/discovery-index/cache.test.ts's 'server.ts wiring' block asserts caps are wired via regex-matching the raw source text of server.ts rather than exercising real behavior — it'll pass even if the caps were silently no-ops due to a signature mismatch, and will break on harmless reformatting; a behavioral test (e.g. via a spy/mock cap) would be more robust.
  • packages/discovery-index/src/server.ts:29-32 passes DEFAULT_CACHE_MAX_ENTRIES explicitly at every call site even though it's already the constructor default — slightly redundant, though arguably intentional for self-documentation/explicitness.
  • Consider making the server.ts wiring test call the actual createApp/exported app rather than string-matching source, to catch real wiring regressions instead of textual ones.

CI checks failing

  • codecov/patch — 91.66% of diff hit (target 99.00%)

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #10029
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ⚠️ 20/25 Preflight is ready, but the PR body does not name the validation run.
Contributor workload ✅ 10/10 Author activity: 98 registered-repo PR(s), 67 merged, 3 issue(s).
Contributor context ✅ Confirmed Gittensor contributor bitfathers94; Gittensor profile; 98 PR(s), 3 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The diff adds a constructor-supplied max-entry cap with an exported default constant, enforces it in set() by first dropping expired entries then evicting oldest-inserted ones via the Map's own order (no timers), passes explicit caps at all three server.ts call sites while leaving SoftClaimStore's constructor signature untouched, and adds tests covering cap enforcement, expired-first eviction, ove

Review context
  • Author: bitfathers94
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: TypeScript
  • Official Gittensor activity: 98 PR(s), 3 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add validation command/output.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: hold · clause: success
  • config: 0c4e229052c411c4727e58b4de71ad77572489e4a3eb9e64b4caf6d1c9aec481 · pack: oss-anti-slop · ci: failed
  • note: diverted by the randomized close-audit holdout (calibration: randomized ε-holdout on would-close PRs with propensity logging #8831) — the deterministic pipeline would otherwise have closed this PR
  • record: 71024497fc175a41fc5715334d610dda5642a1c09cda0a257e31e2781bae4c04 (schema v6, head 67a27b8)

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 31, 2026
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.84%. Comparing base (1d2b142) to head (67a27b8).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
packages/discovery-index/src/cache.ts 91.66% 0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (91.66%) is below the target coverage (99.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10135   +/-   ##
=======================================
  Coverage   79.83%   79.84%           
=======================================
  Files         282      283    +1     
  Lines       58695    58723   +28     
  Branches     6909     6917    +8     
=======================================
+ Hits        46862    46889   +27     
  Misses      11544    11544           
- Partials      289      290    +1     
Flag Coverage Δ
backend 96.42% <91.66%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/discovery-index/src/cache.ts 96.42% <91.66%> (ø)

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 31, 2026

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing CI, closing.

@JSONbored JSONbored closed this Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

discovery-index(cache): bound TtlCache so a never-re-read key is actually freed instead of living for the process lifetime

2 participants