Skip to content

fix(github): retarget the backfill-registered-repos job family from isRegistered to isInstalled - #5688

Merged
JSONbored merged 1 commit into
mainfrom
fix/backfill-isinstalled-5021
Jul 14, 2026
Merged

fix(github): retarget the backfill-registered-repos job family from isRegistered to isInstalled#5688
JSONbored merged 1 commit into
mainfrom
fix/backfill-isinstalled-5021

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • backfillRegisteredRepositories and enqueueRepositoryOpenDataBackfill (the two entry points for open-data backfill) both filtered/gated on repo.isRegistered. Any repo on the gittensor subnet's registry file got swept into backfill every 30 minutes, regardless of whether this self-host instance ever installed the app on it.
  • Live-verified on edge-nl-01: 15 of 18 gittensor-subnet repos were unrelated third-party miner repos, none installed on this instance, all still getting backfill-repo-segment jobs dispatched every cron cycle — the direct driver of the unbounded queue growth reported in the parent epic Untangle gittensor-subnet integration from core self-host review features #5016.
  • Retargets both entry points to isInstalled. The optional job-type rename (backfill-registered-reposbackfill-installed-repos) suggested in the issue is deliberately left for a separate follow-up to keep this fix small and low-risk.

A note on test fallout

Fixing the two gate checks surfaced a real, pre-existing test-isolation gap: the module-level installation-token cache (src/github/app.ts) is keyed by installationId alone, and the newly-eligible test repos needed a real installed-repo fixture, several of which ended up sharing the same hardcoded id. Without resetting that cache between tests, whichever test happened to mint a token first would silently poison every later test reusing that id — regardless of that later test's own fetch mock. A dedicated reset helper (clearInstallationTokenCacheForTest) already existed for exactly this purpose (used elsewhere in the repo) but wasn't wired into backfill.test.ts; fixed that, plus one-line install fixtures in three other test files whose repo setups only ever set isRegistered and now need isInstalled too to keep exercising the same code path they always did.

Test plan

  • Two new regression tests reproducing the live incident shape (installed-not-registered repo included, registered-not-installed repo excluded) for both entry points
  • npm run test:ci — full local gate, clean (0 failures), run twice for stability

…sRegistered to isInstalled

backfillRegisteredRepositories and enqueueRepositoryOpenDataBackfill (the
two entry points for open-data backfill) both filtered/gated on
repo.isRegistered. Any repo on the gittensor subnet's registry file got
swept into backfill every 30 minutes, regardless of whether this
self-host instance ever installed the app on it.

Live-verified on edge-nl-01: 15 of 18 gittensor-subnet repos were
unrelated third-party miner repos, none installed on this instance, all
still getting backfill-repo-segment jobs dispatched every cron cycle --
the direct driver of the unbounded queue growth reported in #5016.

Retargets both entry points to isInstalled. The optional job-type rename
(backfill-registered-repos -> backfill-installed-repos) suggested in the
issue is deliberately left for a separate follow-up to keep this fix
small and low-risk.

Also wires clearInstallationTokenCacheForTest() into backfill.test.ts's
afterEach: the module-level installation-token cache is keyed by
installationId alone, and the new isInstalled-eligible test repos all
share one hardcoded id, so without this reset whichever test first mints
a token poisons every later test reusing that id, independent of that
later test's own fetch mock. Same underlying cache also required a
one-line install call in backfill-file-hydration-scoping.test.ts,
queue.test.ts, and queue-2.test.ts, whose own repo fixtures only ever set
isRegistered and now need isInstalled too to keep exercising the same
code path they always did.

Closes #5021
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.02%. Comparing base (66e927e) to head (3d99f01).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5688      +/-   ##
==========================================
- Coverage   95.04%   95.02%   -0.03%     
==========================================
  Files         576      576              
  Lines       45977    45977              
  Branches    14724    14724              
==========================================
- Hits        43699    43688      -11     
- Misses       1525     1530       +5     
- Partials      753      759       +6     
Flag Coverage Δ
shard-1 43.56% <50.00%> (-0.43%) ⬇️
shard-2 35.84% <100.00%> (+0.11%) ⬆️
shard-3 32.40% <0.00%> (-0.06%) ⬇️
shard-4 32.94% <0.00%> (-0.12%) ⬇️
shard-5 31.51% <50.00%> (-0.26%) ⬇️
shard-6 44.61% <0.00%> (+0.20%) ⬆️

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

Files with missing lines Coverage Δ
src/github/backfill.ts 96.35% <100.00%> (-0.88%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

loopover-orb Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-14 04:05:24 UTC

5 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This is a narrow, well-scoped one-line-per-entry-point fix retargeting the backfill eligibility gate from `isRegistered` to `isInstalled` in both `backfillRegisteredRepositories` and `enqueueRepositoryOpenDataBackfill`, directly addressing the unbounded queue growth incident described. The test fallout is real and well-explained (module-level installation-token cache keyed by installationId needed resetting between tests), and two new regression tests reproduce the live incident shape (installed-not-registered included, registered-not-installed excluded). The PR correctly identifies and documents a now-unreachable dead branch (missing-token skip path) rather than silently deleting or leaving it unexplained, which is good practice.

Nits — 5 non-blocking
  • The PR notes the 'skips public repo backfill without a service token' test was removed/renamed to 'backs off fresh sync states' and leaves the corresponding source branch in src/github/backfill.ts as documented dead code rather than removing it — worth tracking as a small follow-up since dead branches erode coverage signal over time.
  • The optional job-type rename (backfill-registered-repos -> backfill-installed-repos) is explicitly deferred to a follow-up, which is reasonable for keeping this fix small, but the job type name will now be slightly misleading until that follow-up lands.
  • seedInstalledAndRegisteredRepo hardcodes installationId 123 across many tests in backfill.test.ts (and other files reuse different hardcoded ids like 456, 9401, 9402, 9002) — worth confirming there's no cross-file test-run collision risk beyond what clearInstallationTokenCacheForTest's afterEach already handles within a single file.
  • Consider filing the deferred job-type rename (backfill-registered-repos -> backfill-installed-repos) as an explicit follow-up issue referenced in this PR if not already linked.
  • Consider whether the now-unreachable missing-token skip branch in backfillRegisteredRepositories (src/github/backfill.ts:329) should be removed in the follow-up rather than kept as documented dead code, to avoid long-term coverage drift.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.

2. Maintainer requires a linked issue — Link the relevant issue (for example `Closes #123`) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
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.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 45 registered-repo PR(s), 37 merged, 330 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 45 PR(s), 330 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, Ruby, Go, JavaScript, MDX, Shell, Solidity
  • Official Gittensor activity: 45 PR(s), 330 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.
  • 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.
  • 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.
[BETA] Chat with Gittensory

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

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

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

🟩 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 added the manual-review Gittensor contributor context label Jul 14, 2026
@JSONbored
JSONbored merged commit 21d13b2 into main Jul 14, 2026
17 checks passed
@JSONbored
JSONbored deleted the fix/backfill-isinstalled-5021 branch July 14, 2026 04:06
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.

1 participant