Skip to content

revert(orb): drop the redundant cloud token-cache; keep the timeout bumps - #1640

Merged
JSONbored merged 1 commit into
mainfrom
fix/orb-broker-cleanup
Jun 28, 2026
Merged

revert(orb): drop the redundant cloud token-cache; keep the timeout bumps#1640
JSONbored merged 1 commit into
mainfrom
fix/orb-broker-cleanup

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Reverts the cloud-side brokered-token cache added in #1634. It turned out to be the wrong layer: the self-host engine already caches brokered tokens in Redis (redis-token-cache.ts, wired in server.ts — confirmed live), which is strictly better — no broker round-trip on a hit, survives restarts, and mints from GitHub only ~hourly. The cloud cache was therefore redundant, did a wasteful per-call D1 write, and its read never hit in prod (it always fell through to a fresh mint — benign, but dead weight).

brokerOrbToken is restored to mint-on-call (the engine's Redis cache keeps that ~hourly, so no throttling). Kept the genuinely useful parts of #1634: BROKER_TIMEOUT_MS=25s (engine, broker-client.ts) and createOrbInstallationToken's 25s mint timeout (app-auth.ts) — those tolerate a slow cold mint and are unrelated to the cache.

The migration-0081 cached_token_json column is left in place: it's already applied to prod and an applied migration can't be removed (contiguity), so the now-unused column simply stays (harmless).

Scope

  • Conventional Commit title; focused (revert broker.ts + its test; keep the timeout bumps).
  • No site//CNAME/Pages; follows CONTRIBUTING.md.
  • Small self-evident revert (no separate issue needed).

Validation

  • git diff --check · actionlint · db:migrations:check · typecheck
  • test:coveragebroker.ts restored to its pre-fix(orb): cache brokered installation tokens to stop GitHub token-mint throttling #1634 form (covered by the existing 9 broker tests: valid mint, invalid/revoked enrollment, ineligible install + the endpoint flows); the cache tests are removed with the cache.
  • test:workers · build:mcp · test:mcp-pack · ui:* · npm audit --audit-level=moderate

If any required check was skipped, explain why:

  • No new migration: the 0081 column is already applied + left unused (can't drop an applied migration without a gap).

Safety

  • No secrets/wallets/trust-scores exposed.
  • Auth/eligibility gate unchanged (enrollment-secret validation + install eligibility still run per exchange); negative-path tests unchanged + green.
  • No public GitHub text change.

Notes

…umps

The self-host engine already caches brokered tokens in Redis (redis-token-cache.ts,
wired in server.ts) — the right layer (no broker round-trip on a hit, survives
restarts, mints ~hourly). #1634's cloud-side cache in brokerOrbToken was therefore
redundant, did a wasteful per-call D1 write, and its read never hit in prod
(always fell through to a fresh mint). Restore brokerOrbToken to mint-on-call.

KEEP the useful parts of #1634: BROKER_TIMEOUT_MS=25s (broker-client.ts) +
createOrbInstallationToken's 25s mint timeout (app-auth.ts). The migration-0081
cached_token_json column stays (already applied; an applied migration can't be
dropped without a gap) — now an unused, harmless column.
@dosubot dosubot Bot added the size:M label Jun 28, 2026
@loopover-orb

loopover-orb Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review — held for maintainer review

2 files · 1 AI reviewers · no blockers · readiness 66/100 · CI pending · blocked

⏸️ Held for maintainer review

Review summary
Focused, clean revert of the D1-backed cloud token cache introduced in #1634. `brokerOrbToken` is restored to mint-on-call with a single `last_token_at` update; `readCachedOrbToken`, `cacheOrbToken`, `touchLastToken`, and the `decryptSecret`/`encryptSecret` imports are all removed without residue. The rationale is sound — the self-host Redis cache is strictly better (no broker round-trip on a hit, survives restarts, mints ~hourly), and the D1 cache never hit in prod. Leaving `cached_token_json` in the applied migration is the only correct choice given contiguity. The nine surviving broker tests cover every reachable branch of the restored function.

Signal Result Evidence
Code review ✅ No blockers 1 reviewers, synthesized
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Review load ✅ 20/20 Readiness component derived from cached public PR metadata and labels; size label size:M.
Validation evidence ❌ 5/25 Cached preflight status is hold.
Open PR queue ❌ 3/10 35 open PR(s), 17 likely reviewable, 18 unlinked.
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 80 PR(s), 291 issue(s).
Gate result ✅ Passing No configured blocker found.
Nits — 5 non-blocking
  • `TOKEN_ENCRYPTION_SECRET` was the exclusive consumer of `cacheOrbToken`/`readCachedOrbToken` in `broker.ts`; if no other module references it, scrub it from `Env`, wrangler.toml bindings, and test helpers to prevent a dead env var accumulating in the security surface (`src/orb/broker.ts`).
  • The `touchLastToken` helper's intent — always stamp `last_token_at` for the audit trail regardless of cache state — is now implicit; a one-line comment on the inlined `UPDATE` at `broker.ts:63` would preserve that context for the next reader.
  • Audit `TOKEN_ENCRYPTION_SECRET` across the full codebase (a quick `grep -r TOKEN_ENCRYPTION_SECRET src/`) — if `broker.ts` was its only consumer, remove it from `Env`, wrangler.toml, and `brokerEnv` in the test helper; ghost env vars expand the misconfiguration surface silently.
  • Queue a future housekeeping migration to `DROP COLUMN cached_token_json` from `orb_enrollments` once you're confident the revert sticks — unused nullable columns are low-risk now but quietly inflate row sizes and confuse schema readers six months later.
  • Code changes lack test evidence — Add focused regression tests or explain why existing coverage is sufficient.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 80 PR(s), 291 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Fix blocker.
  • Expect slower review.
  • Refresh registry data or choose a registered active repo.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

Focused, clean revert of the D1-backed cloud token cache introduced in #1634. `brokerOrbToken` is restored to mint-on-call with a single `last_token_at` update; `readCachedOrbToken`, `cacheOrbToken`, `touchLastToken`, and the `decryptSecret`/`encryptSecret` imports are all removed without residue. The rationale is sound — the self-host Redis cache is strictly better (no broker round-trip on a hit, survives restarts, mints ~hourly), and the D1 cache never hit in prod. Leaving `cached_token_json` in the applied migration is the only correct choice given contiguity. The nine surviving broker tests cover every reachable branch of the restored function.

Nits (4)

  • `TOKEN_ENCRYPTION_SECRET` was the exclusive consumer of `cacheOrbToken`/`readCachedOrbToken` in `broker.ts`; if no other module references it, scrub it from `Env`, wrangler.toml bindings, and test helpers to prevent a dead env var accumulating in the security surface (`src/orb/broker.ts`).
  • The `touchLastToken` helper's intent — always stamp `last_token_at` for the audit trail regardless of cache state — is now implicit; a one-line comment on the inlined `UPDATE` at `broker.ts:63` would preserve that context for the next reader.
  • Audit `TOKEN_ENCRYPTION_SECRET` across the full codebase (a quick `grep -r TOKEN_ENCRYPTION_SECRET src/`) — if `broker.ts` was its only consumer, remove it from `Env`, wrangler.toml, and `brokerEnv` in the test helper; ghost env vars expand the misconfiguration surface silently.
  • Queue a future housekeeping migration to `DROP COLUMN cached_token_json` from `orb_enrollments` once you're confident the revert sticks — unused nullable columns are low-risk now but quietly inflate row sizes and confuse schema readers six months later.

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory 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 gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jun 28, 2026
@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@298014a). Learn more about missing BASE report.
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1640   +/-   ##
=======================================
  Coverage        ?   95.53%           
=======================================
  Files           ?      204           
  Lines           ?    22093           
  Branches        ?     7981           
=======================================
  Hits            ?    21106           
  Misses          ?      412           
  Partials        ?      575           
Files with missing lines Coverage Δ
src/orb/broker.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit 9ab9f36 into main Jun 28, 2026
37 of 41 checks passed
@JSONbored
JSONbored deleted the fix/orb-broker-cleanup branch June 28, 2026 03:27
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.

1 participant