MemberJunction / MemberJunction/MJ

AIPromptRunner failover: skipped candidates + asymmetric attempt accounting + failed-result mislabeled as failover success

Open
#3,789 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.