governance: enabling calendar alignment preserves the open window - #6013
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughCalendar alignment now preserves usage while advancing eligible budget and rate-limit windows to current calendar boundaries. Governance handlers detect newly enabled alignment and invoke in-memory adoption for affected governance entities. ChangesCalendar alignment adoption
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GovernanceUpdate
participant GovernanceManager
participant BifrostHTTPServer
participant GovernanceStore
GovernanceUpdate->>GovernanceManager: request adoption for changed owners
GovernanceManager->>BifrostHTTPServer: AdoptCalendarAlignmentInMemory
BifrostHTTPServer->>GovernanceStore: adopt budget and rate-limit windows
GovernanceStore-->>BifrostHTTPServer: adoption result
BifrostHTTPServer-->>GovernanceManager: success or error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@transports/bifrost-http/handlers/governance.go`:
- Around line 4201-4208: Update the provider governance flow around the reload
result near the existing model-config update logic, capturing it in a variable
accessible to the adoption block. In the call to adoptCalendarAlignment, pass
the reloaded model config’s Budgets and RateLimitID, falling back to the
in-handler mc when the reload returns no entity, matching the updateTeam and
updateCustomer patterns.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c8fb3370-1232-4e72-9beb-3d8b6bd2bbb6
📒 Files selected for processing (11)
docs/features/governance/budget-and-limits.mdxframework/configstore/tables/budget.goframework/configstore/tables/budgetwindow_test.goframework/configstore/tables/ratelimit.goplugins/governance/budgetcycle_test.goplugins/governance/store.goplugins/governance/storeconcurrency_test.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.gotransports/bifrost-http/handlers/pricing_override_test.gotransports/bifrost-http/server/server.go
7cea97e to
d6f98e1
Compare
f9894d4 to
acf4899
Compare
Merge activity
|
The base branch was changed.
acf4899 to
66a0f2d
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
…ximhq#6013) ## Summary Switching calendar alignment on for an owner with accumulated usage was destructive: the reset sweep treats a window as due whenever its most recent calendar boundary is later than `LastReset`, so any window that opened before that boundary was immediately overdue and its usage was cleared on the next tick. For a monthly budget switched mid-month, that meant losing most of the month's accumulated spend with no warning. This PR introduces a forward-only adoption step that runs on the `false → true` transition. Instead of resetting, each open window's `LastReset` is moved forward to the boundary it now follows, making the window current rather than overdue. Usage is never touched. The first real reset then happens at the next boundary, exactly as if alignment had been set at creation time. ## Changes - **`TableBudget.AdoptCalendarAlignment`** — moves `LastReset` forward to `WindowStart(now)` when the window is overdue under the new rule. Returns `false` and changes nothing for unaligned budgets, sub-day durations (no calendar boundary to adopt), and windows that are already current. - **`TableRateLimit.AdoptCalendarAlignment`** — mirrors the budget logic for token and request counters independently, since they carry separate durations and `LastReset` values. - **`LocalGovernanceStore.AdoptCalendarAlignmentInMemory` / `AdoptRateLimitCalendarAlignmentInMemory`** — CAS-loop wrappers that apply adoption to the live in-memory store. The loop carries whatever usage it observes at swap time, so a concurrent spend bump is never silently dropped. - **`GovernanceHandler.adoptCalendarAlignment`** — called after the in-memory reload in `updateVirtualKey`, `updateTeam`, `updateCustomer`, and `updateProviderGovernance`, but only when `calendar_aligned` transitions from `false` to `true`. For virtual keys, adoption also covers every model config that inherits the flag. - **`BifrostHTTPServer.AdoptCalendarAlignmentInMemory`** — server-level entry point that resolves the governance store and dispatches to the per-budget and per-rate-limit adoption methods. Missing entries are not treated as errors. - **`GovernanceManager` / `ServerCallbacks` interfaces** — extended with `AdoptCalendarAlignmentInMemory` so enterprise can broadcast the adoption to cluster peers the same way usage resets are broadcast. - **Documentation** — the calendar alignment section is rewritten to describe the adoption contract: the boundary moves forward, usage is preserved, sub-day windows stay rolling, and the forward-only rule is what keeps cluster nodes in agreement. Notable design decisions: - Adoption is **forward-only**. Rewinding `LastReset` would re-open a window the cluster already agreed was spent, breaking cluster consistency. - Adoption runs **after** the in-memory reload, for the same reason the usage reset does: the reload carries cached values forward and would overwrite the change if adoption ran first. - Sub-day windows (e.g. `1h`) are explicitly excluded because they have no calendar boundary to adopt; they remain on their rolling schedule regardless of the owner's alignment flag. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [x] Docs ## How to test ```sh go test ./framework/configstore/tables/... ./plugins/governance/... ./transports/bifrost-http/handlers/... ``` Key scenarios to verify: 1. **Mid-month switch-on**: create a monthly budget with usage accumulated since before the 1st, enable `calendar_aligned`, confirm `LastReset` moves to the 1st and `CurrentUsage` is unchanged. 2. **Already-current window**: a window opened after the most recent boundary should report no adoption and keep its original `LastReset`. 3. **Sub-day window**: an hourly counter beside a monthly budget should keep its rolling anchor after the switch. 4. **Concurrent spend**: usage bumped concurrently with adoption must survive; the CAS loop must not drop any increment. 5. **Already-aligned owner**: a second write with `calendar_aligned: true` on an already-aligned owner must not call adoption, which would keep pushing the boundary forward and prevent the window from ever resetting. ## Breaking changes - [x] No ## Security considerations None. The change only affects how reset boundaries are computed in memory; no authentication, secrets, or PII are involved. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
…ximhq#6013) ## Summary Switching calendar alignment on for an owner with accumulated usage was destructive: the reset sweep treats a window as due whenever its most recent calendar boundary is later than `LastReset`, so any window that opened before that boundary was immediately overdue and its usage was cleared on the next tick. For a monthly budget switched mid-month, that meant losing most of the month's accumulated spend with no warning. This PR introduces a forward-only adoption step that runs on the `false → true` transition. Instead of resetting, each open window's `LastReset` is moved forward to the boundary it now follows, making the window current rather than overdue. Usage is never touched. The first real reset then happens at the next boundary, exactly as if alignment had been set at creation time. ## Changes - **`TableBudget.AdoptCalendarAlignment`** — moves `LastReset` forward to `WindowStart(now)` when the window is overdue under the new rule. Returns `false` and changes nothing for unaligned budgets, sub-day durations (no calendar boundary to adopt), and windows that are already current. - **`TableRateLimit.AdoptCalendarAlignment`** — mirrors the budget logic for token and request counters independently, since they carry separate durations and `LastReset` values. - **`LocalGovernanceStore.AdoptCalendarAlignmentInMemory` / `AdoptRateLimitCalendarAlignmentInMemory`** — CAS-loop wrappers that apply adoption to the live in-memory store. The loop carries whatever usage it observes at swap time, so a concurrent spend bump is never silently dropped. - **`GovernanceHandler.adoptCalendarAlignment`** — called after the in-memory reload in `updateVirtualKey`, `updateTeam`, `updateCustomer`, and `updateProviderGovernance`, but only when `calendar_aligned` transitions from `false` to `true`. For virtual keys, adoption also covers every model config that inherits the flag. - **`BifrostHTTPServer.AdoptCalendarAlignmentInMemory`** — server-level entry point that resolves the governance store and dispatches to the per-budget and per-rate-limit adoption methods. Missing entries are not treated as errors. - **`GovernanceManager` / `ServerCallbacks` interfaces** — extended with `AdoptCalendarAlignmentInMemory` so enterprise can broadcast the adoption to cluster peers the same way usage resets are broadcast. - **Documentation** — the calendar alignment section is rewritten to describe the adoption contract: the boundary moves forward, usage is preserved, sub-day windows stay rolling, and the forward-only rule is what keeps cluster nodes in agreement. Notable design decisions: - Adoption is **forward-only**. Rewinding `LastReset` would re-open a window the cluster already agreed was spent, breaking cluster consistency. - Adoption runs **after** the in-memory reload, for the same reason the usage reset does: the reload carries cached values forward and would overwrite the change if adoption ran first. - Sub-day windows (e.g. `1h`) are explicitly excluded because they have no calendar boundary to adopt; they remain on their rolling schedule regardless of the owner's alignment flag. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [x] Docs ## How to test ```sh go test ./framework/configstore/tables/... ./plugins/governance/... ./transports/bifrost-http/handlers/... ``` Key scenarios to verify: 1. **Mid-month switch-on**: create a monthly budget with usage accumulated since before the 1st, enable `calendar_aligned`, confirm `LastReset` moves to the 1st and `CurrentUsage` is unchanged. 2. **Already-current window**: a window opened after the most recent boundary should report no adoption and keep its original `LastReset`. 3. **Sub-day window**: an hourly counter beside a monthly budget should keep its rolling anchor after the switch. 4. **Concurrent spend**: usage bumped concurrently with adoption must survive; the CAS loop must not drop any increment. 5. **Already-aligned owner**: a second write with `calendar_aligned: true` on an already-aligned owner must not call adoption, which would keep pushing the boundary forward and prevent the window from ever resetting. ## Breaking changes - [x] No ## Security considerations None. The change only affects how reset boundaries are computed in memory; no authentication, secrets, or PII are involved. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
…ximhq#6013) ## Summary Switching calendar alignment on for an owner with accumulated usage was destructive: the reset sweep treats a window as due whenever its most recent calendar boundary is later than `LastReset`, so any window that opened before that boundary was immediately overdue and its usage was cleared on the next tick. For a monthly budget switched mid-month, that meant losing most of the month's accumulated spend with no warning. This PR introduces a forward-only adoption step that runs on the `false → true` transition. Instead of resetting, each open window's `LastReset` is moved forward to the boundary it now follows, making the window current rather than overdue. Usage is never touched. The first real reset then happens at the next boundary, exactly as if alignment had been set at creation time. ## Changes - **`TableBudget.AdoptCalendarAlignment`** — moves `LastReset` forward to `WindowStart(now)` when the window is overdue under the new rule. Returns `false` and changes nothing for unaligned budgets, sub-day durations (no calendar boundary to adopt), and windows that are already current. - **`TableRateLimit.AdoptCalendarAlignment`** — mirrors the budget logic for token and request counters independently, since they carry separate durations and `LastReset` values. - **`LocalGovernanceStore.AdoptCalendarAlignmentInMemory` / `AdoptRateLimitCalendarAlignmentInMemory`** — CAS-loop wrappers that apply adoption to the live in-memory store. The loop carries whatever usage it observes at swap time, so a concurrent spend bump is never silently dropped. - **`GovernanceHandler.adoptCalendarAlignment`** — called after the in-memory reload in `updateVirtualKey`, `updateTeam`, `updateCustomer`, and `updateProviderGovernance`, but only when `calendar_aligned` transitions from `false` to `true`. For virtual keys, adoption also covers every model config that inherits the flag. - **`BifrostHTTPServer.AdoptCalendarAlignmentInMemory`** — server-level entry point that resolves the governance store and dispatches to the per-budget and per-rate-limit adoption methods. Missing entries are not treated as errors. - **`GovernanceManager` / `ServerCallbacks` interfaces** — extended with `AdoptCalendarAlignmentInMemory` so enterprise can broadcast the adoption to cluster peers the same way usage resets are broadcast. - **Documentation** — the calendar alignment section is rewritten to describe the adoption contract: the boundary moves forward, usage is preserved, sub-day windows stay rolling, and the forward-only rule is what keeps cluster nodes in agreement. Notable design decisions: - Adoption is **forward-only**. Rewinding `LastReset` would re-open a window the cluster already agreed was spent, breaking cluster consistency. - Adoption runs **after** the in-memory reload, for the same reason the usage reset does: the reload carries cached values forward and would overwrite the change if adoption ran first. - Sub-day windows (e.g. `1h`) are explicitly excluded because they have no calendar boundary to adopt; they remain on their rolling schedule regardless of the owner's alignment flag. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [x] Docs ## How to test ```sh go test ./framework/configstore/tables/... ./plugins/governance/... ./transports/bifrost-http/handlers/... ``` Key scenarios to verify: 1. **Mid-month switch-on**: create a monthly budget with usage accumulated since before the 1st, enable `calendar_aligned`, confirm `LastReset` moves to the 1st and `CurrentUsage` is unchanged. 2. **Already-current window**: a window opened after the most recent boundary should report no adoption and keep its original `LastReset`. 3. **Sub-day window**: an hourly counter beside a monthly budget should keep its rolling anchor after the switch. 4. **Concurrent spend**: usage bumped concurrently with adoption must survive; the CAS loop must not drop any increment. 5. **Already-aligned owner**: a second write with `calendar_aligned: true` on an already-aligned owner must not call adoption, which would keep pushing the boundary forward and prevent the window from ever resetting. ## Breaking changes - [x] No ## Security considerations None. The change only affects how reset boundaries are computed in memory; no authentication, secrets, or PII are involved. ## Checklist - [x] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable

Summary
Switching calendar alignment on for an owner with accumulated usage was destructive: the reset sweep treats a window as due whenever its most recent calendar boundary is later than
LastReset, so any window that opened before that boundary was immediately overdue and its usage was cleared on the next tick. For a monthly budget switched mid-month, that meant losing most of the month's accumulated spend with no warning.This PR introduces a forward-only adoption step that runs on the
false → truetransition. Instead of resetting, each open window'sLastResetis moved forward to the boundary it now follows, making the window current rather than overdue. Usage is never touched. The first real reset then happens at the next boundary, exactly as if alignment had been set at creation time.Changes
TableBudget.AdoptCalendarAlignment— movesLastResetforward toWindowStart(now)when the window is overdue under the new rule. Returnsfalseand changes nothing for unaligned budgets, sub-day durations (no calendar boundary to adopt), and windows that are already current.TableRateLimit.AdoptCalendarAlignment— mirrors the budget logic for token and request counters independently, since they carry separate durations andLastResetvalues.LocalGovernanceStore.AdoptCalendarAlignmentInMemory/AdoptRateLimitCalendarAlignmentInMemory— CAS-loop wrappers that apply adoption to the live in-memory store. The loop carries whatever usage it observes at swap time, so a concurrent spend bump is never silently dropped.GovernanceHandler.adoptCalendarAlignment— called after the in-memory reload inupdateVirtualKey,updateTeam,updateCustomer, andupdateProviderGovernance, but only whencalendar_alignedtransitions fromfalsetotrue. For virtual keys, adoption also covers every model config that inherits the flag.BifrostHTTPServer.AdoptCalendarAlignmentInMemory— server-level entry point that resolves the governance store and dispatches to the per-budget and per-rate-limit adoption methods. Missing entries are not treated as errors.GovernanceManager/ServerCallbacksinterfaces — extended withAdoptCalendarAlignmentInMemoryso enterprise can broadcast the adoption to cluster peers the same way usage resets are broadcast.Notable design decisions:
LastResetwould re-open a window the cluster already agreed was spent, breaking cluster consistency.1h) are explicitly excluded because they have no calendar boundary to adopt; they remain on their rolling schedule regardless of the owner's alignment flag.Type of change
Affected areas
How to test
go test ./framework/configstore/tables/... ./plugins/governance/... ./transports/bifrost-http/handlers/...Key scenarios to verify:
calendar_aligned, confirmLastResetmoves to the 1st andCurrentUsageis unchanged.LastReset.calendar_aligned: trueon an already-aligned owner must not call adoption, which would keep pushing the boundary forward and prevent the window from ever resetting.Breaking changes
Security considerations
None. The change only affects how reset boundaries are computed in memory; no authentication, secrets, or PII are involved.
Checklist
docs/contributing/README.mdand followed the guidelines