Add autosplit detector core#1097
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (1fa4210):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughautosplitデテクタの公開契約、ルート状態管理、ウィンドウ評価、候補制限、P50分割キー選定を追加した。包括的な評価テストと、M3のPR分割および進捗を更新する設計文書も追加した。 ChangesAutosplit検出コア
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Evaluate
participant DetectorState
participant Result
Caller->>Evaluate: ConfigとInputを渡す
Evaluate->>DetectorState: ルート状態を取得・更新
Evaluate->>Evaluate: ウィンドウを集計し候補を選択
Evaluate->>Result: DecisionとEventを生成
Result-->>Caller: 評価結果を返す
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![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
🤖 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 `@distribution/autosplit/detector.go`:
- Around line 245-248: Update the invalid-window branch in the detector flow to
clear the confidence and latestHot state for every active route before
returning. Preserve the existing SkipReasonInvalidWindow event, ensuring invalid
windows break consecutive hot-window promotion and fail closed.
- Around line 269-275:
クールダウン中の履歴ウィンドウが期限後に評価されても昇格材料へ加算されないよう、`status.CooldownUntil` と現在時刻だけでなく
`window.Column.At` も期限後か検証し、期限前の列は `latestHot`
や連続超過判定から破棄してください。クールダウン中に複数列を蓄積し、期限後にまとめて処理しても再昇格しない回帰テストを追加してください。
- Around line 208-211: Input.Windows の順序に依存しないよう、detector
のウィンドウ評価処理で入力スライスをコピーし、各ウィンドウの Column.At を基準に安定ソートしてから processWindow
を呼び出してください。元の Input.Windows は変更せず、ソート済みコピーを使って latestHot と result
を更新する既存の処理を維持してください。
In `@docs/design/2026_06_11_partial_hotspot_split_milestone3_automation.md`:
- Line 10: Update the document’s Updated date from 2026-07-18 to the actual
current date, 2026-07-17.
- Around line 549-550: Move state-map garbage-collection reconciliation against
the live catalog from M3-PR2b to M3-PR2a in the milestone table. Update PR2a’s
scope and tests to include Evaluate’s existing state.gc(live) behavior, and
remove that responsibility and its test from PR2b while preserving the other
PR2b items.
🪄 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: CHILL
Plan: Pro
Run ID: e092fee0-78f1-4657-85e5-7b515fd69d24
📒 Files selected for processing (3)
distribution/autosplit/detector.godistribution/autosplit/detector_test.godocs/design/2026_06_11_partial_hotspot_split_milestone3_automation.md
|
@codex review Addressed current-head CodeRabbit findings in 978a6e9:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (978a6e9):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 978a6e9079
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result *Result, | ||
| ) { | ||
| status := state.routes[route.RouteID] | ||
| if now.Before(status.CooldownUntil) || window.Column.At.Before(status.CooldownUntil) { |
There was a problem hiding this comment.
When an evaluation runs just after a cooldown deadline, this only rejects columns whose flush At is before the deadline. A committed ColumnWindow with At after CooldownUntil but At-Duration before it still contains traffic collected during cooldown, so with CandidateWindows=1 it can immediately promote a just-split child from cooldown-period load (and with the default it still counts as the first post-cooldown evidence). Since ColumnWindow carries Duration, reject/reset windows whose start overlaps the cooldown or require the next full post-cooldown column.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48e3982. The cooldown check now treats a window as cooldown evidence if Column.At-Duration is before CooldownUntil, so only a full post-cooldown column can advance confidence. Added TestCooldownDropsWindowThatStartedBeforeDeadline.
| if cumulative >= total/2 { | ||
| median = row | ||
| break |
There was a problem hiding this comment.
When keyviz label attribution is enabled, a column can contain multiple non-aggregate rows for the same route and sub-bucket (one per label). The score path sums those rows, but this median loop can stop on the first duplicate that crosses half the total, so LeftLoad/RightLoad exclude the remaining load in the same [Start,End) bucket and can report the hot side on the wrong child (for example bucket0 labels 1000+900 and bucket1 100). Those estimates are part of the detector decision and feed p50 target admission, so rows with the same sub-bucket bounds should be coalesced before the median is selected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48e3982. P50 selection now coalesces usable non-aggregate rows with identical route/sub-bucket bounds before weighted median selection, so duplicate label-attributed rows contribute as one bucket. Added TestP50CoalescesDuplicateSubBucketRows to assert left/right load estimates.
| return | ||
| } | ||
|
|
||
| status.ConsecutiveOver++ |
There was a problem hiding this comment.
This increments confidence every time a hot ColumnWindow is supplied, but the state does not remember the last processed MatrixColumn.At. If the sampler reader returns the same committed column again (for example an eval interval shorter than the keyviz step, or a trailing Snapshot window reused across cycles), a single hot flush can be counted repeatedly and promote after CandidateWindows evaluations instead of CandidateWindows distinct committed columns. Store and skip already-processed column boundaries, or make duplicate/stale columns a no-op before this increment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 48e3982. RouteStatus now tracks LastProcessedAt and processRouteWindow ignores duplicate or stale MatrixColumn.At boundaries before changing confidence, preventing one committed column from advancing hysteresis across repeated evaluations. Added TestDuplicateColumnsAreSkippedAcrossEvaluations.
|
@codex review Addressed current-head detector evidence findings in 48e3982:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (48e3982):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48e3982e6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if route.State != distribution.RouteStateActive { | ||
| state.ApplyRouteState(route.RouteID, route.State) | ||
| result.Events = append(result.Events, Event{RouteID: route.RouteID, Reason: SkipReasonNonActiveState}) | ||
| continue |
There was a problem hiding this comment.
When a route is non-active in an evaluation that includes committed windows, this only clears confidence and leaves LastProcessedAt unchanged because the route is omitted from active. If the sampler reader passes the same buffered MatrixColumn.At values again after the route returns to RouteStateActive, processRouteWindow will accept those old columns and count load from the fenced/migrating interval as fresh active evidence; with CandidateWindows=2, one skipped hot column plus one post-active hot column can immediately promote. The non-active skip needs to advance the route's processed watermark through the freshest skipped window (or otherwise suppress those buffered columns) before the route can rebuild confidence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in daae162. Non-active routes now advance LastProcessedAt through the freshest window in the evaluation while clearing confidence, so buffered columns observed during migrating/fenced state are skipped if replayed after the route becomes active again. Added TestNonActiveRouteAdvancesSkippedBufferedColumns.
| if window.Duration <= 0 { | ||
| result.Events = append(result.Events, Event{Reason: SkipReasonInvalidWindow, At: window.Column.At}) | ||
| for _, route := range active { | ||
| state.resetConfidenceAt(route.RouteID, window.Column.At) | ||
| delete(latestHot, route.RouteID) | ||
| } |
There was a problem hiding this comment.
This invalid-window path resets every active route before checking whether that column boundary is stale for the route. resetConfidenceAt keeps LastProcessedAt unchanged when window.Column.At is older, but it still clears ConsecutiveOver, so a duplicate/old zero-duration column kept in sampler history can erase already-earned hysteresis on every evaluation and prevent a hot route from ever reaching the candidate window count. The reset and latestHot deletion should be conditional on the invalid column being newer than the route's LastProcessedAt, matching the stale-column guard used for normal windows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in daae162. Invalid windows now reset confidence only when the invalid column is newer than the route's LastProcessedAt; stale invalid columns preserve existing hysteresis and do not clear the current candidate. Added TestStaleInvalidWindowDoesNotResetConfidence.
|
|
||
| // DefaultConfig returns the M3 detector defaults from the design doc. | ||
| func DefaultConfig() Config { | ||
| return Config{ |
There was a problem hiding this comment.
This default config literal leaves MaxRoutes as zero, and the admission check only enforces the cap when cfg.MaxRoutes > 0. If the scheduler uses DefaultConfig() or a zero-valued config with only selected overrides, autosplit has no total route-count guardrail and can keep emitting one split per hot cycle indefinitely, bypassing the documented cap meant to bound catalog growth. Please set a positive default here or make callers explicitly supply one before decisions are admitted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in daae162. DefaultConfig now sets MaxRoutes=1024 and withDefaults applies it to zero/partial configs, so autosplit keeps a positive route-count guardrail by default unless an explicit cap overrides it. Added TestDefaultConfigSetsRouteCap and TestZeroMaxRoutesUsesDefaultRouteCap, and updated the design doc default.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (daae162):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Addressed current-head detector guardrail findings in daae162:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daae162957
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // | ||
| // Non-active route states clear confidence but preserve any cooldown deadline. | ||
| func (s *DetectorState) ApplyRouteState(routeID uint64, state distribution.RouteState) { | ||
| s.applyRouteStateThrough(routeID, state, time.Time{}) |
There was a problem hiding this comment.
When the watcher uses this transition hook for an active→migrating→active interval that occurs between detector cycles, only clearing ConsecutiveOver leaves LastProcessedAt at the old column. The next Evaluate over buffered columns since that watermark will count hot columns from the non-active interval and can promote immediately after reactivation (e.g. CandidateWindows=2 with one migrating column plus one active column), so the route does not re-earn consecutive active evidence. Please carry a skipped-through column boundary/transition watermark through this path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4833261. ApplyRouteState now requires a processed-through column boundary and advances LastProcessedAt for non-active transitions, so future watcher wiring cannot clear confidence without also fencing buffered columns from the non-active interval. Caller audit found only Evaluate and the new regression test use this hook. Added TestApplyRouteStateAdvancesProcessedWatermark.
|
@codex review Addressed the current-head transition watermark finding in 4833261:
Caller audit:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (4833261):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (7a486a1):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review Current head 7a486a1 is merged with current main. The detector-only PR diff remains limited to detector.go, detector_test.go, and the M3 design status update. Validation:
|
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Author: bootjp
Summary
Validation
Summary by CodeRabbit