Skip to content

fix(control-plane): isolate one AMS tenant's wake failure from the rest of the tick - #10202

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-10063
Jul 31, 2026
Merged

fix(control-plane): isolate one AMS tenant's wake failure from the rest of the tick#10202
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
bitfathers94:fix/issue-10063

Conversation

@bitfathers94

Copy link
Copy Markdown
Contributor

fix(control-plane): isolate one AMS tenant's wake failure from the rest of the tick

A rejection from stub.start(), pollForExitCode's getState(), or either registry.upsert() call inside
wakeDueAmsTenants aborted the whole tick, leaving every remaining due tenant untouched and the failing
tenant's lastRunAt/lastExitCode never written. Wrap each tenant's wake in a try/catch, add an explicit
AmsWakeResult.failed field, and still attempt (and catch a second failure of) the lastRunAt write when
a tenant's wake throws.

Closes #10063

…st of the tick

A rejection from stub.start(), pollForExitCode's getState(), or either registry.upsert() call inside
wakeDueAmsTenants aborted the whole tick, leaving every remaining due tenant untouched and the failing
tenant's lastRunAt/lastExitCode never written. Wrap each tenant's wake in a try/catch, add an explicit
AmsWakeResult.failed field, and still attempt (and catch a second failure of) the lastRunAt write when
a tenant's wake throws.
@bitfathers94
bitfathers94 requested a review from JSONbored as a code owner July 31, 2026 12:23
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-31 12:46:40 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR wraps each tenant's wake sequence in ams-wake.ts (control-plane/src/ams-wake.ts:139-186) in a try/catch, adds a `failed` field to AmsWakeResult, and attempts a best-effort lastRunAt write (with its own nested catch) when a tenant's wake throws. The isolation is correctly scoped per-loop-iteration inside the existing sequential for-loop, so a rejection from start(), getState(), or either upsert() now only aborts that one tenant's cycle instead of the whole tick, and the claimed nextDueAt from the first upsert survives even if later steps fail (ensuring retry next cycle rather than starving other tenants). The accompanying tests exercise the real failure paths (throwing start/getState stubs, a persistently-failing upsert wrapper) rather than fabricated states, including the nested catch-of-catch branch and interaction with maxTenantsPerTick.

Nits — 4 non-blocking
  • control-plane/src/ams-wake.ts:169-176 — the failure-path upsert omits `lastExitCode`, which leaves a prior successful run's `lastExitCode` untouched on the record after a later failed run; worth confirming this is the intended semantics (last known exit code persists) vs. clearing it.
  • control-plane/test/ams-wake.test.ts — `throwingGetStateStub`'s local `getStateCalls` closure variable is incremented but never asserted on in the corresponding test; either assert on it or drop the tracking to avoid dead state.
  • Consider logging (or otherwise surfacing) the caught error in both catch blocks of control-plane/src/ams-wake.ts:169-186 — right now a failed tenant's actual error message is swallowed entirely and only `failed: true` survives, which will make production debugging of *why* a tenant failed harder than necessary.
  • The double-catch retry in control-plane/src/ams-wake.ts:171-181 re-sends the full `record` spread; consider a short comment or assertion clarifying that `amsSchedule.lastExitCode` is deliberately omitted here (ties to the nit above).

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 #10063
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 ⚠️ 20/25 Preflight is ready, but the PR body does not name the validation run.
Contributor workload ✅ 10/10 Author activity: 130 registered-repo PR(s), 92 merged, 3 issue(s).
Contributor context ✅ Confirmed Gittensor contributor bitfathers94; Gittensor profile; 130 PR(s), 3 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Linked issue satisfaction

Addressed
The diff wraps the per-tenant body in try/catch as required, adds an explicit `failed` field to AmsWakeResult (leaving exitCode/timedOut untouched), attempts (and separately catches a failure of) the lastRunAt upsert on the failure path, keeps processing sequential and the slot-claim-before-poll ordering intact, and still counts a failed tenant toward maxTenantsPerTick. Tests cover start()/getStat

Review context
  • Author: bitfathers94
  • 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: 130 PR(s), 3 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add validation command/output.
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 <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> 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.

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

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.35%. Comparing base (3884485) to head (5ec2a10).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10202      +/-   ##
==========================================
- Coverage   92.21%   91.35%   -0.87%     
==========================================
  Files         934      934              
  Lines      114178   114206      +28     
  Branches    27593    27598       +5     
==========================================
- Hits       105294   104328     -966     
- Misses       7582     8771    +1189     
+ Partials     1302     1107     -195     
Flag Coverage Δ
backend 94.13% <ø> (-1.55%) ⬇️
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/ams-wake.ts 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

@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 cc9d102 into JSONbored:main Jul 31, 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(ams-wake): isolate one tenant's wake failure from the rest of the tick

1 participant