refactor(failover): unify feature naming on "failover"#451
Conversation
The model-failover feature was referred to as both "failover" and "fallback", with two packages (internal/failover for the store/service and internal/fallback for the resolver). Unify on "failover" — the precise term for switching to a backup model when the primary fails, and the one already used by the DB tables (failover_rules), admin handler, audit-log snapshot, and gateway attempt kind. "fallback" stays reserved for its unrelated generic uses (provider-type defaults, CONFIGURED_PROVIDER_MODELS_MODE=fallback, pricing/registry fallbacks). - Merge internal/fallback (resolver) into internal/failover — one package. - Rename feature identifiers Fallback*->Failover* across config, gateway (fallback.go->failover.go), server, app, admin, core, workflows, auditlog, and live (types, funcs, the FailoverResolver interface, the workflow FailoverEnabled() toggle, the WithFailoverUsed context flag). - Config surface: FailoverConfig, the `failover:` YAML block, FAILOVER_MANUAL_RULES_PATH + FAILOVER_MODE env vars, config/failover.go, config/failover.example.json, .env.template. - Workflow feature flag features.fallback -> features.failover (Go, dashboard JS/HTML, generated docs, e2e scenario). - Regenerate swagger/openapi; update feature docs. Kept as "fallback" deliberately: the rule data fields primary_model / fallback_models (DB columns + JSON API + dashboard labels) describe the target role and pair naturally with primary_model; renaming would force a needless SQLite/Postgres/Mongo migration and API break while the feature identity (routes, table, package, types) is already failover. The feature is fresh (#444), so env vars / config key / the serialized features.failover flag are renamed outright rather than aliased. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR renames the “fallback” concept to “failover” across configuration, workflow feature flags, routing/execution, server wiring, admin workflow management, audit logging, generated API docs, and supporting tests and documentation. ChangesFallback to Failover Rename
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Failover *bool `json:"failover,omitempty" bson:"failover,omitempty"` | ||
| } | ||
|
|
||
| func (f FeatureFlags) canonicalize() FeatureFlags { | ||
| if f.Budget == nil { | ||
| budgetEnabled := true | ||
| f.Budget = &budgetEnabled | ||
| } | ||
| if f.Fallback == nil { | ||
| fallbackEnabled := true | ||
| f.Fallback = &fallbackEnabled | ||
| if f.Failover == nil { | ||
| failoverEnabled := true | ||
| f.Failover = &failoverEnabled | ||
| } |
There was a problem hiding this comment.
Any workflow stored in DB/BSON with "fallback": false will deserialize into the new struct with Failover == nil (the old key is simply ignored). canonicalize() then defaults nil to &true, silently re-enabling failover for a workflow that had it explicitly disabled. The PR acknowledges this as a deliberate breaking change for a "fresh" feature, but the breakage is asymmetric: stored false silently flips to true, while stored true is equally silent but produces the same result — so the only affected users are those who opted out. A backward-compatible read of the old key (e.g. a custom UnmarshalJSON that also checks "fallback") or a one-off DB migration would prevent unintended silent re-enablement on upgrade.
| ModelAuthorizer RequestModelAuthorizer // Optional: request-scoped concrete model access controller | ||
| WorkflowPolicyResolver RequestWorkflowPolicyResolver // Optional: persisted workflow resolver used during workflow resolution | ||
| FallbackResolver RequestFallbackResolver // Optional: translated-route fallback resolver | ||
| FailoverResolver RequestFailoverResolver // Optional: translated-route fallback resolver |
There was a problem hiding this comment.
| FailoverResolver RequestFailoverResolver // Optional: translated-route fallback resolver | |
| FailoverResolver RequestFailoverResolver // Optional: translated-route failover resolver |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/admin/handler_failover.go (1)
191-216: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPass the loaded failover config into the resolver.
config.FailoverConfig{Enabled: true}strips the configured disabled-model state beforeSuggestFailoversruns.internal/failover/resolver.goonly suppresses suggestions throughcfg.Disabled, so/admin/failover/generatecan still propose rules for models thatfailover.disabled_modelshas explicitly turned off. Please build this resolver from the same failover config the runtime uses instead of a synthetic enabled-only config.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/admin/handler_failover.go` around lines 191 - 216, The failover resolver is being built with a synthetic always-enabled config, which drops the loaded disabled-model state before SuggestFailovers runs. Update the resolver construction in the admin failover handler to use the same loaded failover config that the runtime uses, so Resolver.SuggestFailovers can respect cfg.Disabled and avoid proposing rules for models marked in failover.disabled_models. Keep the change localized around the resolver setup in the handler and the failover.NewResolverWithRuleProvider call.internal/admin/handler_workflows_test.go (1)
424-452: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAssert the positive
failovervalue, not just the key presence.Line 424 proves the response includes a
failoverkey, but this test never proves the renamed field survives astruein eithereffective_featuresorbody.Payload.Features. A regression that always serializedfailover: falsewould still pass here. As per coding guidelines,**/*_test.go: Add or update tests for behavior changes, and ensure tests cover request translation, response normalization, error handling, default configuration, and provider-specific parameter mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/admin/handler_workflows_test.go` around lines 424 - 452, The workflow response test only checks that the failover field exists, not that it remains enabled; update the assertions around effectiveFeatures and workflows.View in handler_workflows_test to verify failover is true in both the normalized response map and body.Payload.Features. Use the existing symbols effectiveFeatures, body.Payload.Features, and workflows.View to locate the checks and add a positive assertion for failover alongside the current usage/audit/guardrails validations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/gomodel/docs/docs.go`:
- Around line 4613-4620: The request schemas for admin.deleteFailoverRuleRequest
(and the matching object at the other referenced location) currently expose
primary_model as optional even though the handler requires it. Update the schema
definitions in docs.go to mark primary_model as required with required:
["primary_model"], then regenerate the OpenAPI output so docs/openapi.json stays
in sync.
In `@internal/admin/dashboard/templates/page-workflows.html`:
- Line 130: The checkbox bound to workflowForm.features.failover is still
unlabeled, so update the workflow form markup to associate it with a visible
label by either wrapping the input in a label element or giving the input a
unique id and matching it with a for attribute; keep the change localized to the
failover control in the page-workflows template.
In `@internal/auditlog/auditlog.go`:
- Line 159: The `WorkflowFeaturesSnapshot` field rename from `fallback` to
`failover` breaks reads of existing audit records because older documents still
store `workflow_features.fallback`. Add a backward-compatible deserialization
path in `auditlog.go` so `Failover` is populated from either legacy `fallback`
or the new key, or perform a migration/backfill before changing the BSON/JSON
tags. Keep the fix localized around `WorkflowFeaturesSnapshot` and its audit log
read/write handling so historical entries still surface the flag correctly.
In `@internal/gateway/refactor_findings_test.go`:
- Around line 217-220: The test setup for stream failover is seeding only
"fallback", so the primary path in streamFailoverProvider is still resolving to
nil instead of an empty stream. Update
TestStreamResponsesFallsBackAfterEmptyPrimaryStream to explicitly seed "primary"
in streamsByModel with an empty io.ReadCloser so the test exercises the
empty-stream fallback behavior in streamFailoverProvider.streamForModel rather
than the nil-stream path.
In `@tests/e2e/release-e2e-scenarios.md`:
- Line 1348: Update the release e2e read-back check for the workflow payload to
assert the renamed failover feature alongside cache. In the scenario that posts
`workflow_payload.features` for the managed-key release flow, extend the
existing validation so it explicitly verifies both
`workflow_payload.features.failover` and `effective_features.failover` using the
same read-back step identifiers used for the current cache assertion. This will
ensure the scenario fails if the server drops, renames, or misnormalizes the new
failover key.
---
Outside diff comments:
In `@internal/admin/handler_failover.go`:
- Around line 191-216: The failover resolver is being built with a synthetic
always-enabled config, which drops the loaded disabled-model state before
SuggestFailovers runs. Update the resolver construction in the admin failover
handler to use the same loaded failover config that the runtime uses, so
Resolver.SuggestFailovers can respect cfg.Disabled and avoid proposing rules for
models marked in failover.disabled_models. Keep the change localized around the
resolver setup in the handler and the failover.NewResolverWithRuleProvider call.
In `@internal/admin/handler_workflows_test.go`:
- Around line 424-452: The workflow response test only checks that the failover
field exists, not that it remains enabled; update the assertions around
effectiveFeatures and workflows.View in handler_workflows_test to verify
failover is true in both the normalized response map and body.Payload.Features.
Use the existing symbols effectiveFeatures, body.Payload.Features, and
workflows.View to locate the checks and add a positive assertion for failover
alongside the current usage/audit/guardrails validations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: de5bc026-9610-48a1-97d8-e83cd035e976
📒 Files selected for processing (58)
.env.templatecmd/gomodel/docs/docs.goconfig/config.example.yamlconfig/config.goconfig/config_test.goconfig/failover.example.jsonconfig/failover.godocs/advanced/guardrails.mdxdocs/advanced/resilience.mdxdocs/advanced/workflows.mdxdocs/dev/possible-refactoring.mddocs/features/budgets.mdxdocs/features/failover.mdxdocs/features/user-path.mdxdocs/openapi.jsoninternal/admin/dashboard/static/js/modules/workflows-layout.test.cjsinternal/admin/dashboard/static/js/modules/workflows.jsinternal/admin/dashboard/static/js/modules/workflows.test.cjsinternal/admin/dashboard/templates/page-workflows.htmlinternal/admin/handler_failover.gointernal/admin/handler_failover_test.gointernal/admin/handler_workflows_test.gointernal/app/app.gointernal/app/app_test.gointernal/auditlog/auditlog.gointernal/auditlog/auditlog_test.gointernal/auditlog/middleware.gointernal/core/context.gointernal/core/workflow.gointernal/failover/factory.gointernal/failover/resolver.gointernal/failover/resolver_test.gointernal/failover/service.gointernal/failover/service_test.gointernal/gateway/failover.gointernal/gateway/failover_test.gointernal/gateway/inference_execute.gointernal/gateway/inference_orchestrator.gointernal/gateway/inference_orchestrator_test.gointernal/gateway/interfaces.gointernal/gateway/refactor_findings_test.gointernal/live/broker_test.gointernal/responsecache/handle_request_test.gointernal/responsecache/simple.gointernal/server/failover_test.gointernal/server/handlers.gointernal/server/http.gointernal/server/internal_chat_completion_executor.gointernal/server/internal_chat_completion_executor_test.gointernal/server/messages_handler.gointernal/server/request_model_resolution.gointernal/server/translated_inference_service.gointernal/workflows/compiler_test.gointernal/workflows/types.gointernal/workflows/types_test.gotests/e2e/release-e2e-scenarios.mdtests/integration/workflows_guardrails_test.gotools/seed-demo-data.sh
| "admin.deleteFailoverRuleRequest": { | ||
| "type": "object", | ||
| "properties": { | ||
| "primary_model": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add required validation for primary_model.
Both request schemas publish primary_model as optional, but the handler rejects empty values. That breaks the published contract and can generate invalid clients. Add required: ["primary_model"] to both objects, then regenerate docs/openapi.json.
Also applies to: 4796-4812
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/gomodel/docs/docs.go` around lines 4613 - 4620, The request schemas for
admin.deleteFailoverRuleRequest (and the matching object at the other referenced
location) currently expose primary_model as optional even though the handler
requires it. Update the schema definitions in docs.go to mark primary_model as
required with required: ["primary_model"], then regenerate the OpenAPI output so
docs/openapi.json stays in sync.
| </label> | ||
| <label class="workflow-feature-toggle" x-show="workflowFailoverVisible()"> | ||
| <input type="checkbox" x-model="workflowForm.features.fallback"> | ||
| <input type="checkbox" x-model="workflowForm.features.failover"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Associate this checkbox with a label.
Line 130 is still flagged as an unlabeled form control, which weakens keyboard/screen-reader usability and shrinks the click target. Wrap it in a <label> or add a matching id/for pair.
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 130-130: No matching [ label ] tag found.
(input-requires-label)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/admin/dashboard/templates/page-workflows.html` at line 130, The
checkbox bound to workflowForm.features.failover is still unlabeled, so update
the workflow form markup to associate it with a visible label by either wrapping
the input in a label element or giving the input a unique id and matching it
with a for attribute; keep the change localized to the failover control in the
page-workflows template.
Source: Linters/SAST tools
| Budget bool `json:"budget" bson:"budget"` | ||
| Guardrails bool `json:"guardrails" bson:"guardrails"` | ||
| Fallback bool `json:"fallback" bson:"fallback"` | ||
| Failover bool `json:"failover" bson:"failover"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the legacy fallback audit field on reads.
Line 159 changes the persisted JSON/BSON key from fallback to failover. Existing audit documents written with workflow_features.fallback will now deserialize with Failover=false, so historical entries lose that flag in live preview and any other readers of WorkflowFeaturesSnapshot. Please add a backward-compatible read path or a data migration/backfill before switching the storage tags.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auditlog/auditlog.go` at line 159, The `WorkflowFeaturesSnapshot`
field rename from `fallback` to `failover` breaks reads of existing audit
records because older documents still store `workflow_features.fallback`. Add a
backward-compatible deserialization path in `auditlog.go` so `Failover` is
populated from either legacy `fallback` or the new key, or perform a
migration/backfill before changing the BSON/JSON tags. Keep the fix localized
around `WorkflowFeaturesSnapshot` and its audit log read/write handling so
historical entries still surface the flag correctly.
| provider := &streamFailoverProvider{ | ||
| streamsByModel: map[string]io.ReadCloser{ | ||
| "fallback": io.NopCloser(strings.NewReader("data: {}\n\n")), | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Model the primary attempt as an empty stream, not a nil stream.
Line 292 returns nil for "primary" because the map only seeds "fallback", so this test is no longer exercising the “empty primary stream” path named by TestStreamResponsesFallsBackAfterEmptyPrimaryStream. Seed "primary" with an empty io.ReadCloser; otherwise a regression in empty-stream detection can slip by while a nil-stream path still passes.
Suggested change
provider := &streamFailoverProvider{
streamsByModel: map[string]io.ReadCloser{
+ "primary": io.NopCloser(strings.NewReader("")),
"fallback": io.NopCloser(strings.NewReader("data: {}\n\n")),
},
}As per coding guidelines, **/*_test.go: Add or update tests for behavior changes, and ensure tests cover request translation, response normalization, error handling, default configuration, and provider-specific parameter mapping.
Also applies to: 290-292
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/gateway/refactor_findings_test.go` around lines 217 - 220, The test
setup for stream failover is seeding only "fallback", so the primary path in
streamFailoverProvider is still resolving to nil instead of an empty stream.
Update TestStreamResponsesFallsBackAfterEmptyPrimaryStream to explicitly seed
"primary" in streamsByModel with an empty io.ReadCloser so the test exercises
the empty-stream fallback behavior in streamFailoverProvider.streamForModel
rather than the nil-stream path.
Source: Coding guidelines
Follow-up to the fallback->failover rename, from PR review: - server/http.go: fix a `FailoverResolver` field comment that still said "fallback resolver". - gateway/failover.go: rename the failover-callback closure param `fallback` -> `failoverFn` in executeWithFailoverResponse (it names the failover path, unlike the generic default-value `fallback` params which stay). - admin/handler_workflows_test.go + tests/e2e/release-e2e-scenarios.md: assert the positive `failover` value (not just key presence) in the effective-features projection and payload, so a regression that dropped or misnormalized the renamed key is caught. Deliberately not changed (out of scope / by design): - No backward-compat shim for persisted `features.fallback` or the audit `workflow_features.fallback` key — an accepted breaking change for a brand-new feature (#444). - The admin `/failover/generate` resolver already respects `failover.disabled_models` via the rule provider's Disabled(), so the synthetic `FailoverConfig{Enabled: true}` is not a bug. - The workflow failover checkbox is wrapped in a `<label>` like every other toggle; the "unlabeled control" lint is a false positive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the reviews. Addressed in
Deliberately not changed:
|
2 participants
What & why
The model-failover feature was referred to as both failover and fallback, and lived in two packages (
internal/failoverfor the store/service,internal/fallbackfor the resolver). This unifies the feature on failover — the precise term for switching to a backup model when the primary fails, and the one already used by the DB tables (failover_rules), the admin handler, the audit-log snapshot, and the gateway attempt kind.fallbackstays reserved for its unrelated generic uses (provider-type defaults,CONFIGURED_PROVIDER_MODELS_MODE=fallback, pricing/registry fallbacks, generic default-value params).Changes
internal/failover/package (resolver moved in;internal/fallback/removed).Fallback*→Failover*across config, gateway (fallback.go→failover.go), server, app, admin,core,workflows,auditlog,live— types, functions, theFailoverResolverinterface, the workflowFailoverEnabled()toggle, and theWithFailoverUsed/GetFailoverUsedcontext flag.FailoverConfig, thefailover:YAML block,FAILOVER_MANUAL_RULES_PATH+FAILOVER_MODEenv vars,config/failover.go,config/failover.example.json,.env.template.features.fallback→features.failover(Go, dashboard JS/HTML, generated docs, e2e scenario) — the flag set is now internally consistent: cache/audit/usage/budget/guardrails/failover..mdxprose.Deliberately kept as
fallbackprimary_model/fallback_models(DB columns, JSON API, dashboard labels). They describe the target role ("the models you fall back to"), pair naturally withprimary_model, and renaming would force a needless SQLite/Postgres/Mongo migration + API break while the feature identity (routes/admin/failover, tablefailover_rules, package, types) is alreadyfailover.User-visible / breaking
The failover feature is fresh (#444), so its env vars, config key, and the serialized
features.failoverworkflow flag are renamed outright rather than aliased:fallback:(config.yaml block)failover:FALLBACK_MANUAL_RULES_PATHFAILOVER_MANUAL_RULES_PATHFEATURE_FALLBACK_MODE(deprecated)FAILOVER_MODEfeatures.fallbackfeatures.failoverconfig/fallback.example.jsonconfig/failover.example.jsonThe admin failover API (
/admin/failover,primary_model/fallback_models) is unchanged.Testing
gofmtclean ·go build -tags=swagger ./...· fullgo test ./...· tagged tests compiletest-race,lint, dashboard tests,mint validate) pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation