Skip to content

fix(selfhost): fail open on a Redis write failure in the installation-token cache - #7035

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
galuis116:fix/redis-token-cache-set-fail-open
Jul 17, 2026
Merged

fix(selfhost): fail open on a Redis write failure in the installation-token cache#7035
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
galuis116:fix/redis-token-cache-set-fail-open

Conversation

@galuis116

Copy link
Copy Markdown
Contributor

Summary

  • src/selfhost/redis-token-cache.ts's get() wraps its redis.get call in try/catch with an explicit fail-open contract (the caller, github/app.ts's createInstallationToken, has no try/catch of its own, so an uncaught error would hard-fail GitHub App token minting on every Redis hiccup). set() — the sibling write call — had no try/catch at all.
  • Confirmed the caller genuinely has no surrounding try/catch: writeCachedToken (src/github/app.ts) calls externalTokenStore.set(...) directly with no guard, right after createInstallationToken successfully obtains a fresh token from GitHub. A transient Redis write failure at that point threw uncaught and turned an otherwise-successful mint into a hard failure.
  • Wraps set()'s redis.set(...) call in try/catch, matching get()'s exact fail-open contract: on error, record a metric via the existing recordTokenCacheMetric("error") and return without rethrowing. No change to get() or the TTL-flooring logic.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves.

Closes #6999

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck — the whole-repo tsc --noEmit reliably OOMs on this shared, memory-constrained sandbox regardless of what changed; relied on npm run build --workspace @loopover/engine (passed) plus the full targeted vitest run below as the local proxy, and CI's isolated runner for the authoritative tsc --noEmit.
  • npm run test:coverage — full targeted run of both affected test files: test/unit/selfhost-redis-token-cache.test.ts (7 tests, incl. the new set() fail-open regression) and test/unit/github-app.test.ts (93 tests, incl. a new end-to-end regression through the real createRedisTokenCache + createInstallationToken) — 100/100 passing.
  • npm run test:workers / npm run build:mcp / npm run test:mcp-pack (not applicable — no Worker binding/queue or MCP packaging changes)
  • npm run ui:openapi:check (no API/schema changes)
  • npm run ui:lint — 0 errors (ran as a sanity check; this PR touches no apps/loopover-ui/** files)
  • npm audit --audit-level=moderate (no dependency changes)
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • The whole-repo npm run typecheck OOMs on this specific sandbox under current memory pressure, so it was not run standalone; npm run build --workspace @loopover/engine plus the full targeted test run above stand in as the local proxy, and CI's isolated runner performs the real tsc --noEmit. npm run test:workers and the MCP packaging checks have no surface to exercise for a change scoped to one try/catch in one self-host module.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed. The new regression tests assert the cached token value ("sensitive-value") never reaches the Prometheus metrics text.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. This directly touches GitHub App installation-token caching; both the unit-level fail-open path and the end-to-end createInstallationToken success-despite-cache-failure path are tested.
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — internal self-host caching behavior, no external-facing API/MCP surface change.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI changes.)
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots. (N/A — backend-only change, no UI surface.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

Notes

  • A prior attempt at this same change (identical code) was auto-closed by CI due to an infrastructure flake unrelated to this diff: the self-host "build + boot smoke test" workflow hit no space left on device during the Docker image export step on the CI runner. codecov/patch, codecov/project, and all 6 validate-tests shards passed cleanly on that same run. Re-opening with the identical, unmodified code.

…-token cache

set() had no try/catch around its redis.set call, unlike get()'s explicit
fail-open contract. The token is already successfully minted from GitHub
before set() is called, and the caller (createInstallationToken) has no
try/catch of its own, so a transient Redis write failure was turning an
otherwise-successful mint into a hard failure instead of just costing one
extra real mint next time.
@galuis116
galuis116 requested a review from JSONbored as a code owner July 17, 2026 18:54
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.74%. Comparing base (a45625f) to head (c356dc7).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7035   +/-   ##
=======================================
  Coverage   93.74%   93.74%           
=======================================
  Files         692      692           
  Lines       68721    68723    +2     
  Branches    18760    18760           
=======================================
+ Hits        64424    64426    +2     
  Misses       3302     3302           
  Partials      995      995           
Flag Coverage Δ
shard-1 43.36% <0.00%> (-0.45%) ⬇️
shard-2 36.91% <0.00%> (+0.05%) ⬆️
shard-3 33.16% <0.00%> (-0.21%) ⬇️
shard-4 34.41% <0.00%> (+0.49%) ⬆️
shard-5 31.73% <100.00%> (-0.49%) ⬇️
shard-6 46.02% <100.00%> (+0.28%) ⬆️

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

Files with missing lines Coverage Δ
src/selfhost/redis-token-cache.ts 100.00% <100.00%> (ø)

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

loopover-orb Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-17 19:13:14 UTC

3 files · 1 AI reviewer · no blockers · readiness 95/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR wraps the previously unguarded redis.set() call in redis-token-cache.ts with a try/catch that fails open and records an error metric, matching the existing contract already used by get(). The fix is correctly scoped, matches the described bug (an uncaught write failure would otherwise hard-fail an already-successful token mint), and both unit and integration-level regression tests are added that exercise the real createRedisTokenCache/createInstallationToken path rather than a fabricated scenario. The change is small, focused, and closes issue #6999 as required.

Nits — 5 non-blocking
  • The flagged 'generic_secret_assignment' in test/unit/github-app.test.ts:2734 is just the literal string "minted-despite-cache-failure" used as a mock token value in a test fixture, not a real secret — worth a quick glance to confirm but not blocking.
  • The inline comments in redis-token-cache.ts (both get() and set()) are fairly long block comments explaining the fail-open rationale; consider trimming to a single line since the contract is now consistent across both methods.
  • Consider extracting the shared try/catch-and-record-metric pattern between get() and set() into a small helper if a third cache method is ever added, though with only two call sites this is not necessary now.
  • The new regression test in selfhost-redis-token-cache.test.ts explicitly asserts store.has(...) is false and that the metrics output does not leak the sensitive value — good defensive assertion, keep this pattern for future cache tests.
  • Possible secret-shaped assignment in the diff (generic_secret_assignment) — Verify the value is not a real credential.

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 #6999
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 ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 1962 registered-repo PR(s), 1288 merged, 52 issue(s).
Contributor context ✅ Confirmed Gittensor contributor galuis116; Gittensor profile; 1962 PR(s), 52 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff wraps set()'s redis.set call in try/catch, calls recordTokenCacheMetric("error") on failure, and returns without rethrowing, exactly mirroring get()'s existing fail-open contract, while leaving get() and TTL logic untouched.

Review context
  • Author: galuis116
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 1962 PR(s), 52 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
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 &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; 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.

🟩 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

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 9cddd4e into JSONbored:main Jul 17, 2026
16 checks passed
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

redis-token-cache.ts's set() lacks the fail-open handling its own get() requires

1 participant