MemberJunction / MemberJunction/MJ
AIPromptRunner failover: skipped candidates + asymmetric attempt accounting + failed-result mislabeled as failover success
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
## Summary
`AIPromptRunner.executeModelWithFailover` has two related bookkeeping/selection defects in the failover path. Surfaced during the review of #3542; the rewritten failover suite drives the real path and pins the current behavior, but the divergences are marked with bare comments rather than fixed.
File: `packages/AI/Prompts/src/AIPromptRunner.ts` · Tests: `packages/AI/Prompts/src/__tests__/AIPromptRunner.failover.test.ts`
## 1. Vendor-filter index handling can skip surviving candidates
When candidates are filtered by vendor during failover, the index handling can advance past a candidate that should still be tried, so a viable model/vendor is never attempted. This can turn a recoverable failure into a hard failure even though a good candidate remained in the list.
## 2. Asymmetric failover-attempt accounting
`FailoverAttempts` is recorded inconsistently depending on *how* a candidate failed:
- a **thrown fatal** failure records `1` (test `:308`)
- a **scope-blocked** failure records `1` (test `:332`)
- a **returned non-eligible** failure records `0` (test `:369`) — attempt recording only runs when `errorInfo?.canFailover` (~`AIPromptRunner.ts:3198`)
So the same "we tried a candidate and it failed" event is counted or not counted based on the failure shape, and the `FailoverAttempts` metric under-reports.
## 3. (Related, currently untested) eligible → non-eligible *returned* failure mislabels a failure as failover success
On the sequence "one eligible failure, then a non-eligible **returned** failure," production falls into the "result was successful" branch and calls `updatePromptRunWithFailoverSuccess` on a **failed** result — switching `ModelID`/`VendorID` bookkeeping to the failed candidate (~`AIPromptRunner.ts:3230`). The failover suite only ever puts the non-eligible failure first, so this path is not exercised.
## Impact
Failover can give up while a working candidate remained, the failover metrics are unreliable, and a failed run can be recorded as a successful failover against the wrong model/vendor — all of which mislead cost/reliability analysis and debugging.
## Suggested fix
- Normalize attempt accounting to record an attempt whenever a candidate is invoked and returns/throws a failure, regardless of `canFailover`.
- Audit the vendor-filter index advance so no surviving candidate is skipped.
- Only call `updatePromptRunWithFailoverSuccess` on an actually-successful result; add a regression test for the eligible-then-non-eligible-returned sequence.
## References
- PR #3542 (review finding #3, and failover-suite finding F1–F3)
Contributor guide
Research direction
Start in packages/AI/Prompts/src/AIPromptRunner.ts, especially the failover logic around lines 3198 and 3230, then run packages/AI/Prompts/src/__tests__/AIPromptRunner.failover.test.ts. Compare the existing cases around lines 308, 332, and 369 with the candidate-filtering behavior. Done means surviving candidates are tried, every failed invocation is counted, and the eligible-then-non-eligible returned-failure sequence has a regression test without recording failover success.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- backend, testing-qa
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100