Skip to content

fix(control-plane): give the fake tenant registry KV's last-writer-wins installation index - #9630

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
rsnetworkinginc:fix-9613
Jul 29, 2026
Merged

fix(control-plane): give the fake tenant registry KV's last-writer-wins installation index#9630
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
rsnetworkinginc:fix-9613

Conversation

@rsnetworkinginc

Copy link
Copy Markdown
Contributor

Closes #9613

Summary

fix(control-plane): give the fake tenant registry KV's last-writer-wins installation index

createFakeTenantRegistry.getByOrbInstallationId was [...records.values()].find(...) — a linear scan in Map insertion order, i.e. first-inserted-wins — while createKvTenantRegistry resolves through its installation:${id} secondary index, which upsert repoints on every write: last-writer-wins, with #9143's compare-and-delete rule. The two implementations agree only while at most one record carries a given installation ID, and the #9143 comment block in this same file documents that assumption as false by design (isRecreatableState lets a failed/torn-down tenant's claim be taken over by a new tenant). For that exact scenario the fake resolved 555 to the stale "old" tenant while KV resolves it to "newcomer" — and the fake is what http-app.test.ts injects, so the installation_already_claimed 409 and webhook-routing assertions ran against inverted semantics.

Root cause

The fake derived the answer by scanning records at read time instead of maintaining the write-time installation index the real implementation's semantics come from.

Fix

  • The fake now keeps its own installationIndex: Map<number, string> (installation → primary key) inside the closure, maintained on every upsert exactly like createKvTenantRegistry's index maintenance: the new claim always repoints the index (last writer wins), and a cleared/changed claim removes the stale entry only if it still points at the upserting record's own key (control-plane: eight pre-deploy defects, headlined by two tenants sharing one Neon database (and one customer's teardown deleting another's) #9143's compare-and-delete — a late upsert of a stale record can never rip out another tenant's live pointer).
  • getByOrbInstallationId resolves through the index; no scan.
  • upsert, get, and list otherwise unchanged, including list()'s existing sort. TenantRegistry interface, createKvTenantRegistry, and http-app.ts untouched.
  • The fake's header comment now describes the index (no longer "a plain linear scan").

Diff: control-plane/src/tenant-registry.ts (+27/−5) and control-plane/test/tenant-registry.test.ts (+42) only.

Tests (RED → GREEN)

Against the unfixed source, the required named regression test fails (the fake resolves the duplicated ID to the stale "old" tenant):

not ok 232 - createFakeTenantRegistry: an unrelated tenant's installation-index entry SURVIVES a stale record's later re-creation/teardown upsert (#9143)
# tests 234
# pass 233
# fail 1

After the fix:

# tests 234
# pass 234
# fail 0
  • The control-plane: eight pre-deploy defects, headlined by two tenants sharing one Neon database (and one customer's teardown deleting another's) #9143 fake-side scenario: upsert ("old","orb","failed") with orbInstallationId: 555, then ("newcomer","orb","active") with 555getByOrbInstallationId(555) resolves to "newcomer"; then upsert ("old","orb","torn down") with no orbInstallationId → still "newcomer" (compare-and-delete keeps the pointer).
  • Re-linking 555 → 777 returns undefined for 555 and the tenant for 777 (mirrors the existing KV test), plus a same-ID re-upsert leaves the pointer in place (the unchanged-claim arm).
  • Unlinking (upsert without an ID) clears the tenant's own stale entry.
  • The existing fake test ("getByOrbInstallationId finds a tenant by installation ID, undefined for an unclaimed one") passes unmodified, as does the whole pre-existing suite.

Coverage

Control-plane files are graded by the control-plane Codecov flag; I ran CI's exact recipe (node --experimental-strip-types scripts/control-plane-coverage.ts, c8 over the built dist remapped to control-plane/src/**):

Statements   : 100% ( 2295/2295 )
Branches     : 100% ( 476/476 )
Functions    : 100% ( 109/109 )
Lines        : 100% ( 2295/2295 )

Cross-referencing the lcov against this PR's diff hunks: control-plane/src/tenant-registry.ts — changed lines 22, uncovered changed lines none, partial branches on changed lines none; whole file has zero uncovered lines or branches. Both arms of every new conditional are exercised: upsert with an ID / without / changed / unchanged, and the compare-and-delete branch in both the "index still points at me" (delete) and "index points at someone else" (keep) cases.

Validation commands (repo root)

git diff --check                                                    # clean
npm run control-plane:test                                          # 234 tests, 234 pass, 0 fail
node --experimental-strip-types scripts/control-plane-coverage.ts   # 100% statements/branches/functions/lines
npm --prefix control-plane run cf:typecheck                         # clean

…ns installation index

createFakeTenantRegistry answered getByOrbInstallationId with a linear
scan of its records Map in insertion order (first-inserted-wins), while
createKvTenantRegistry resolves through its installation:${id}
secondary index (last-writer-wins with JSONbored#9143's compare-and-delete
rule). For the duplicated-installation scenario JSONbored#9143 documents as real
-- a failed tenant's claim taken over by a new active tenant -- the two
implementations returned OPPOSITE tenants, so http-app.test.ts (which
injects the fake) asserted inverted webhook-routing and
installation_already_claimed 409 semantics.

Give the fake its own in-memory Map<number, string> installation ->
primary-key index, maintained on every upsert exactly like the KV
path: last writer wins, stale pointers removed on a cleared/changed
claim only when they still point at the upserting record's own key.
upsert/get/list are otherwise unchanged, including list()'s sort, and
the header comment now describes the index instead of a linear scan.

Closes JSONbored#9613
@loopover-orb

loopover-orb Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-29 01:54:23 UTC

2 files · 1 AI reviewer · no blockers · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
The fake registry's getByOrbInstallationId is switched from a first-inserted-wins linear scan to a last-writer-wins secondary index, mirroring createKvTenantRegistry's own upsert/index maintenance including the #9143 compare-and-delete guard. The index maintenance logic (delete-then-set, guarded by pointer identity) is a correct line-for-line mirror of the KV implementation, and the new tests exercise the exact duplicated-installation and re-link/unlink scenarios the bug affected. This closes #9613 as a narrow, well-scoped correctness fix with no changes to the public interface or other implementations.

Nits — 4 non-blocking

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 #9613
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 ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 52 registered-repo PR(s), 27 merged, 3 issue(s).
Contributor context ✅ Confirmed Gittensor contributor rsnetworkinginc; Gittensor profile; 52 PR(s), 3 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The fake now maintains an installationIndex Map updated on upsert with last-writer-wins semantics and #9143's compare-and-delete guard, getByOrbInstallationId resolves through that index instead of scanning, and the header comment is updated; new tests cover the #9143 scenario and the installation-change case as required.

Review context
  • Author: rsnetworkinginc
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is registered but has no active allocation in the current snapshot.
  • Public profile languages: not available
  • Official Gittensor activity: 52 PR(s), 3 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Await review-lane availability.
  • Then work through the remaining 2 steps in the Signals table above.
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

@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.26%. Comparing base (8e3e4f5) to head (ec9ad1d).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9630      +/-   ##
==========================================
- Coverage   90.08%   89.26%   -0.83%     
==========================================
  Files         889      889              
  Lines      112024   112041      +17     
  Branches    26587    26590       +3     
==========================================
- Hits       100921   100014     -907     
- Misses       9773    10939    +1166     
+ Partials     1330     1088     -242     
Flag Coverage Δ
backend 94.07% <ø> (-1.50%) ⬇️
control-plane 100.00% <100.00%> (ø)

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

Files with missing lines Coverage Δ
control-plane/src/tenant-registry.ts 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

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

@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 a402b9f into JSONbored:main Jul 29, 2026
8 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.

control-plane(registry): the fake registry resolves a duplicated installation ID differently from KV

1 participant