fix(selfhost): fail open with a metric on a token-cache Redis error - #6364
Conversation
redis-token-cache.ts's get() had no try/catch around redis.get() itself, so a connection failure threw uncaught with no observability signal — unlike its two sibling wrappers (redis-response-cache.ts throws with a metric, redis-cache.ts fails open silently). Its actual caller (github/app.ts's readCachedToken -> createInstallationToken) has no try/catch of its own, so an uncaught rejection here would hard-fail GitHub App token minting on every Redis hiccup instead of costing one extra real mint. Fail open (return null, same contract as redis-cache.ts) is the correct behavior for this caller, but record a metric so the failure is never silently invisible, matching redis-response-cache.ts's observability discipline. Closes JSONbored#6288
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6364 +/- ##
=======================================
Coverage 95.60% 95.60%
=======================================
Files 599 599
Lines 47221 47224 +3
Branches 15024 15024
=======================================
+ Hits 45147 45150 +3
Misses 1290 1290
Partials 784 784
Flags with carried forward coverage won't be shown. Click here to find out more.
|
loopover-orb
Bot
added
the
gittensor:bug
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-16 05:13:43 UTC
Review summary Nits — 4 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk 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.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 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.
|
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
1 participant
Closes #6288
Summary
redis-token-cache.ts'sget()had notry/catcharoundredis.get()itself, so a Redis connection failure threw uncaught with no observability signal — unlike its two sibling wrappers (redis-response-cache.tsthrows with aresult:"error"metric,redis-cache.tsfails open silently with no metric).github/app.ts'sreadCachedToken→createInstallationTokenhas notry/catchof its own around this call, so an uncaught rejection here would hard-fail GitHub App token minting on every Redis hiccup instead of just costing one extra real mint. Fail open (returnnull, same contract asredis-cache.ts) is the correct behavior for this specific caller — but unlikeredis-cache.ts, this fix also records aresult:"error"metric so the failure is never silently invisible, matchingredis-response-cache.ts's observability discipline.set()and the other two Redis wrapper files are untouched — no inconsistency was found in their behavior.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typecheck— roottsc --noEmitreliably OOMs on this shared sandbox regardless of what changed (reproduced repeatedly this session on a clean checkout). Scoped coverage (npx vitest run ... --coverage --coverage.include="src/selfhost/redis-token-cache.ts") confirms 100% statements/branches/functions/lines on the changed file, and the full dedicated test file plusgithub-app.test.ts(98 tests total) pass.npm run test:coverage— not run repo-wide (same OOM risk). Scoped coverage on the changed file is 100% branches (6/6).test/unit/selfhost-redis-token-cache.test.ts(7 tests) andtest/unit/github-app.test.ts(91 tests, covering the caller side) both pass.npm run test:workers— N/A, no Worker-facing behavior changed beyond this file's own contract, which is now more resilient (fail-open), not less.npm run build:mcp/npm run test:mcp-pack— N/A, no@loopover/mcpchanges.npm run ui:openapi:check/ui:lint/ui:typecheck/ui:build— N/A, noapps/loopover-uichanges.npm audit --audit-level=moderate— 0 vulnerabilities.mainwith the fix stashed) before confirming it passes with the fix applied.If any required check was skipped, explain why:
npm run typecheck/npm run test:coverage: reliably OOMs on this shared sandbox under memory pressure from concurrent sessions, independent of the diff. Substituted with scoped 100% coverage on the changed file plus the full caller-side test suite (98 tests, all passing).Safety
UI Evidencesection below with screenshots. — N/A, no visible UI change.CHANGELOG.mduntouched.Notes
set()in the same file also lacks atry/catch, but the issue explicitly scopes to theget()path (redis-token-cache.ts:22-27) and instructs not to touch the other files "unless a genuine inconsistency in THEIR behavior is also found" — kept this PR focused on the describedget()gap rather than expanding scope.