feat(router): add per-session beta routing toggle - #1006
Conversation
|
Claude finished @Symbiomancer's task —— View job Posted advisory comment-length review with 7 inline suggestions. |
workweave-bot
left a comment
workweave-bot
left a comment
There was a problem hiding this comment.
Advisory only — comment-length nits. Won't block merge.
![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 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 97307fc. Configure here.
 T-Rex LogsWhat T-Rex did
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| preference, enabled, err := s.sessionStrategyStore.Get(ctx, installationID, sessionKey) | ||
| if err != nil { | ||
| return fmt.Errorf("read beta routing preference: %w", err) | ||
| } | ||
| if enabled && preference.Strategy != router.StrategyHMMBeta { | ||
| return fmt.Errorf("unsupported persisted session routing strategy %q", preference.Strategy) | ||
| } | ||
| if !enabled && !s.PolicyStrategyAvailable(router.StrategyHMMBeta) { | ||
| return writeBetaCommandResponse(w, env, betaUnavailable, inputTokens) | ||
| } | ||
|
|
||
| message := betaEnabledMessage | ||
| previousStrategy := router.StrategyFromContext(ctx) | ||
| if enabled { | ||
| if err := s.sessionStrategyStore.Clear(context.Background(), installationID, sessionKey); err != nil { | ||
| return fmt.Errorf("disable beta routing: %w", err) | ||
| } | ||
| previousStrategy = router.StrategyHMMBeta | ||
| message = betaDisabledMessage | ||
| } else { | ||
| if err := s.sessionStrategyStore.Set(context.Background(), sessionstrategy.Preference{ | ||
| InstallationID: installationID, | ||
| SessionKey: sessionKey, | ||
| Strategy: router.StrategyHMMBeta, | ||
| }); err != nil { | ||
| return fmt.Errorf("enable beta routing: %w", err) | ||
| } |
There was a problem hiding this comment.
Two overlapping /beta requests for the same session can both read a missing preference, both call Set, and both return “Beta enabled.” Two serialized toggles from disabled should instead enable and then disable beta, leaving it disabled and returning an enabled acknowledgement followed by a disabled one. Make the per-session read-modify-write atomic—preferably in the persistence layer with a transaction or lock that returns the resulting state—so it remains correct across multiple router instances.
Artifacts
Focused Go harness that overlaps two beta toggles after the same preference read
- The in-package test source installs a barrier after both Get calls snapshot the disabled state and then invokes the real beta handler twice, with the takeaway that the exact concurrent path is exercised.
Race-enabled first concurrent beta-toggle reproduction
- The first race-enabled execution records two Gets, two Sets, zero Clears, final enabled state, and two enabled acknowledgements, with the takeaway that the collapse occurs in an executed handler path.
Race-enabled repeated concurrent beta-toggle reproduction
- Twenty race-enabled executions repeat the same two-Set, zero-Clear result without a race-detector report, with the takeaway that the lost-toggle interleaving is deterministic under the controlled overlap.
Sequential beta toggle comparison and concurrent reproduction
- The comparison run shows the harness collapsing concurrent toggles while the existing sequential test logs enabled then disabled, with the takeaway that only the non-serialized concurrent path violates toggle semantics.
2 participants
Summary
Validation
No model artifact, deployment pin, or infrastructure configuration is included in this public repository PR.