Add autosplit committed-window reader#1152
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
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 (63ea6a7):
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. |
📝 WalkthroughWalkthrough
Changesコミット済みウィンドウと observe-only 統合
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ObserveSnapshot
participant ReadCommittedWindows
participant SnapshotSource
participant CommittedWindowsFromColumns
participant Evaluate
ObserveSnapshot->>ReadCommittedWindows: SnapshotReadConfig を渡す
ReadCommittedWindows->>SnapshotSource: Snapshot(from, now)
SnapshotSource-->>ReadCommittedWindows: MatrixColumn スナップショット
ReadCommittedWindows->>CommittedWindowsFromColumns: 列を ColumnWindow へ変換
CommittedWindowsFromColumns-->>ReadCommittedWindows: Windows と newest を返す
ObserveSnapshot->>Evaluate: Windows と Now を渡す
Evaluate-->>ObserveSnapshot: Result を返す
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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: 2
🤖 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 `@docs/design/2026_06_11_partial_hotspot_split_milestone3_automation.md`:
- Line 12: Update the “Updated” date in the milestone 3 automation design
document to the actual modification date, replacing the future/review-following
date while preserving the document’s date format.
In `@keyviz/sampler.go`:
- Around line 1239-1243: Flush 全体を専用 mutex で直列化し、At の取得から history への push、および
lastFlushAt の更新までを同じロックで保護してください。対象は Flush の処理フローと、WindowStart を補正して col
を追加する箇所であり、並行実行時も列順序と実際の window 境界が一致する状態を維持してください。
🪄 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: fe18b72f-4498-4d5e-ace9-832fcc18f14d
📒 Files selected for processing (7)
distribution/autosplit/detector.godistribution/autosplit/sampler_reader.godistribution/autosplit/sampler_reader_test.godocs/design/2026_06_11_partial_hotspot_split_milestone3_automation.mdkeyviz/ring_buffer.gokeyviz/sampler.gokeyviz/sampler_test.go
| Author: bootjp | ||
| Date: 2026-06-11 | ||
| Updated: 2026-07-18 | ||
| Updated: 2026-07-23 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
更新日を実際の更新日に修正してください。
2026-07-23 はレビュー日である 2026-07-22 の翌日です。未来日付の設計記録にならないよう、実際の更新日へ訂正してください。
🤖 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 `@docs/design/2026_06_11_partial_hotspot_split_milestone3_automation.md` at
line 12, Update the “Updated” date in the milestone 3 automation design document
to the actual modification date, replacing the future/review-following date
while preserving the document’s date format.
| col.WindowStart = s.lastFlushAt | ||
| if !col.WindowStart.Before(col.At) { | ||
| col.WindowStart = col.At.Add(-s.opts.Step) | ||
| } | ||
| s.lastFlushAt = col.At |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Flush 全体を直列化して、列順序と境界を一致させてください。
At は Line 1215 で history lock の前に取得されます。並行した Flush で新しい At の列が先に push されると、古い列は lastFlushAt より前になり補正され、ring buffer の時系列順序と実際の window 境界が壊れます。Range の二分探索や detector の duration 判定が誤ります。専用 mutex で時刻取得から history push までを直列化してください。
修正案
type MemSampler struct {
+ flushMu sync.Mutex
// historyMu guards the ring buffer.
historyMu sync.Mutex
}
func (s *MemSampler) Flush() {
if s == nil {
return
}
+ s.flushMu.Lock()
+ defer s.flushMu.Unlock()
+
col := MatrixColumn{At: s.now()}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| col.WindowStart = s.lastFlushAt | |
| if !col.WindowStart.Before(col.At) { | |
| col.WindowStart = col.At.Add(-s.opts.Step) | |
| } | |
| s.lastFlushAt = col.At | |
| type MemSampler struct { | |
| flushMu sync.Mutex | |
| // historyMu guards the ring buffer. | |
| historyMu sync.Mutex | |
| } | |
| func (s *MemSampler) Flush() { | |
| if s == nil { | |
| return | |
| } | |
| s.flushMu.Lock() | |
| defer s.flushMu.Unlock() | |
| col := MatrixColumn{At: s.now()} |
🤖 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 `@keyviz/sampler.go` around lines 1239 - 1243, Flush 全体を専用 mutex で直列化し、At の取得から
history への push、および lastFlushAt の更新までを同じロックで保護してください。対象は Flush
の処理フローと、WindowStart を補正して col を追加する箇所であり、並行実行時も列順序と実際の window
境界が一致する状態を維持してください。
![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: 63ea6a7f62
ℹ️ 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 !col.At.After(lastProcessedAt) { | ||
| continue |
There was a problem hiding this comment.
When an autosplit evaluation is delayed past the retained keyviz history (or HistoryColumns is smaller than the confidence window), Snapshot can return only columns whose first At is already well after LastProcessedAt. This loop treats those columns as normal new evidence and Evaluate preserves the route's existing ConsecutiveOver, so a route with e.g. 2/3 hot columns before the gap can promote on the first retained hot column instead of failing closed and re-earning the full window count. Detect that the fetched history no longer bridges LastProcessedAt and reset/advance the detector state (or surface the gap to the caller) before appending windows.
Useful? React with 👍 / 👎.
| if start.IsZero() || !start.Before(col.At) { | ||
| skipped++ | ||
| continue |
There was a problem hiding this comment.
When a fetched committed column has no proven lower boundary (for example a zero/malformed WindowStart), this branch only increments SkippedInvalid and drops the column before Evaluate sees it. That preserves any existing ConsecutiveOver streak across an unscored interval, so a route that was hot before the invalid column can promote on the next hot column even though the unknown window should fail closed. Clear route confidence through col.At when skipping here, or pass an invalid ColumnWindow to the detector so its reset path runs.
Useful? React with 👍 / 👎.
1 participant
Summary
Evidence
Summary by CodeRabbit