chore: schedule FDv2 recovery orchestration after fallback TTL expires - #1857
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 50a8250. Configure here.
| } | ||
| } finally { | ||
| conditions.close(); | ||
| fdv2RecoveryCondition?.close(); |
There was a problem hiding this comment.
FDv1 failure cancels FDv2 recovery
Medium Severity
The FDv2 recovery timer is scoped to the FDv1 synchronizer inner loop and closed in finally on every exit. An FDv1 terminal_error (for example non-recoverable 4xx from FDv1PollingSynchronizer) blocks FDv1, cancels the scheduled recovery, and leaves no available synchronizers, so the SDK never retries FDv2 after the server-directed TTL.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 50a8250. Configure here.
|
|
||
| it('uses a default recovery interval (not zero) when fdv1FallbackTtlMs is absent (regression: SDK-2617 §8.3.2)', async () => { | ||
| // §8.3.2: when x-ld-fd-fallback-ttl header is absent, the SDK MUST use a default | ||
| // retry interval of 1 hour. Absence must NOT suppress recovery (unlike TTL=0). |
There was a problem hiding this comment.
Internal spec section in comments
Low Severity
The new test name and comment reference opaque internal section §8.3.2. That violates the rule against internal specification identifiers in code comments; the surrounding text already explains the required absent-TTL behavior without the section marker.
Triggered by learned rule: Do not reference internal specification documents in code comments
Reviewed by Cursor Bugbot for commit 50a8250. Configure here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Summary
Wires the FDv2-recovery primitives from the previous PR in this stack into
FDv2DataSource's synchronizer run loop: after falling back to FDv1, a recovery condition races alongside the existing fallback/recovery conditions, using the server-supplied TTL (or a caller-configurable override, or a 1-hour default if absent -- a TTL of0means indefinite, no recovery scheduled).On firing,
SourceManager.fdv2Recovery()re-enables FDv2 and the loop naturally attempts a fresh FDv2 connection -- if the server still signals fallback on that attempt, fallback re-engages from the new result. Also guards against the FDv1 synchronizer's own results re-triggering the fallback machinery. Ships with regression tests covering TTL present/absent/zero and the no-re-trigger guard.Note
Overview
After the SDK falls back to FDv1,
FDv2DataSourcenow schedules a return to FDv2 by racing an FDv2 recovery timer alongside the existing fallback/recovery conditions. When the timer fires,SourceManager.fdv2Recovery()re-enables FDv2 synchronizers so the loop can try FDv2 again (a new server fallback directive can re-engage FDv1).TTL resolution follows server
fdv1FallbackTtlMswhen present, then optional configfdv2RecoveryTimeoutMs, thenDEFAULT_FDV2_RECOVERY_TIMEOUT_MS(1 hour).TTL = 0means indefinite FDv1 with no recovery timer; missing TTL is not treated like zero.handleFdv1Fallbackno longer re-runs fallback when the active synchronizer is already the FDv1 fallback slot, so FDv1 results that still carry the fallback flag do not loop fallback machinery.Regression tests cover TTL expiry recovery, indefinite TTL, absent TTL defaulting to recovery, and the FDv1 no-re-trigger guard (SDK-2617).
Reviewed by Cursor Bugbot for commit 50a8250. Bugbot is set up for automated code reviews on this repo. Configure here.