fix(pipeline-lock): stale takeover の同時取得(race)を排除 — 実測で確定・両OS検証 - #312
Merged
Conversation
pipeline lock (push/merge の二重起動防止) の stale takeover が、高競合下で 2 スレッドとも Acquired になり得た。`concurrent_stale_takeover_only_one_wins` テストが Windows で ~10% flaky に落ちていた症状の根本原因。 ## 実測で確定させた真因 (2 段階) Windows + WSL Linux の双方でシーケンス番号付きトレースを取り、憶測ではなく 観測で 2 つの独立した欠陥を特定した。 ### 欠陥 1: 本番コードの TOCTOU (2 スレッド版テストの本物の race) 旧 takeover は `read → 再比較 → remove_file → create_new`。**再比較と remove が 非アトミック**で、A が再比較を通過した直後に B が takeover を完走 (fresh lock 作成) → A の remove が B の fresh lock を破壊 → A も create_new 成功で 2 スレッド とも Acquired。旧 doc の「128bit token 偶然一致が必要=無視できる」は誤りで、 通常のスケジューリング窓だった。 修正は 2 重の防御: - **sentinel** (`<lock>.takeover` の create_new) で「takeover を試みるスレッド」を 1 つに直列化。 - sentinel 保持者は remove+create ではなく **temp へ全内容を書いてから rename で path を atomic 置換**する。path が一度も不在にならないため、他スレッドの 親 fast-path create_new が割り込めず、読み手が空/部分書き込みを観測する窓も消える。 - create_new 直後・write_all 前の空ファイルを stale と誤判定しないよう、lock content を Fresh / Held(空=書き込み中) / Stale の 3 値に分類 (姉妹 lock.rs が WP-15 で塞いだ bug class と同型)。 sentinel だけ / rename だけ では不十分だった経緯 (Linux 高競合で PARENT 経路と TAKEOVER 経路が各 1 Acquired) は関数 doc に実測ログ付きで記録。 ### 欠陥 2: 追加した stress テスト自身の偽陽性 8 スレッド stress テストの初版は `map(join).filter().count()` の遅延イテレータで 数えており、**まだ acquire 中の他スレッドの傍らで先行結果が drop** され、その `PipelineLock::drop` が lock を削除 → 走行中スレッドが正当に再取得し「2 Acquired」に 見えた (同時保持ではなく解放後の再取得 = テストアーティファクト)。全ガードを Vec に collect してから数える形に修正し、「同時点で 2 つ保持され得るか」だけを検証する。 ## 検証 - 2 スレッド版 (両ガード保持 = 本物の race を突く) は master で 3/30 失敗、本修正で Windows/Linux とも多数回 pass。 - 8 スレッド stress は旧本番コードで確実に失敗、本修正で Windows 30/30・Linux 30/30 pass。 - 公開 API (hold_pipeline_lock / acquire_pipeline_lock) は不変で呼び出し元は無改修。 - cargo test --workspace 全 pass / clippy clean を Windows + WSL Linux で確認。
📝 WalkthroughWalkthroughChangesPipeline lock takeover
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant acquire_pipeline_lock_at
participant takeover_stale_lock
participant LockFile
Worker->>acquire_pipeline_lock_at: lock 取得要求
acquire_pipeline_lock_at->>LockFile: content 読み取り
LockFile-->>acquire_pipeline_lock_at: Stale
acquire_pipeline_lock_at->>takeover_stale_lock: stale takeover
takeover_stale_lock->>LockFile: takeover sentinel を確保
takeover_stale_lock->>LockFile: temp 書き込み後 rename
LockFile-->>Worker: Acquired または Busy
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)(該当なし — レビュー指摘が未着のため) Applicable Findings (Medium 以下)(該当なし) Filtered (not applicable)(該当なし) 次のアクション
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/lib-jj-helpers/src/pipeline_lock.rs`:
- Around line 359-363: Update the std::fs::write failure branch in the pipeline
lock takeover flow to remove the temporary file via remove_file(&tmp) before
returning PipelineLockResult::Unavailable, matching the cleanup performed in the
rename failure branch. Preserve the existing failure reason and return behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 494d5024-91a2-4e66-a71f-44dcc28392ee
📒 Files selected for processing (2)
src/lib-jj-helpers/src/pipeline_lock.rssrc/lib-jj-helpers/src/pipeline_lock/tests.rs
Comment on lines
+359
to
+363
| if let Err(e) = std::fs::write(&tmp, content) { | ||
| return PipelineLockResult::Unavailable { | ||
| reason: format!("takeover temp 書き込み失敗: {}", e), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
書き込み失敗時に temp ファイルが残置される。
std::fs::write は書き込み途中で失敗しても空/部分ファイルを作成済みのことがあり、この分岐では削除していません。rename 失敗分岐（Line 370）では remove_file(&tmp) で後始末しているのと非対称で、ディスク満杯などの失敗時に <lock>.new.<token> が蓄積します。
🧹 提案: 書き込み失敗時も temp を除去
let tmp = takeover_tmp_path(path, &token);
if let Err(e) = std::fs::write(&tmp, content) {
+ let _ = std::fs::remove_file(&tmp);
return PipelineLockResult::Unavailable {
reason: format!("takeover temp 書き込み失敗: {}", e),
};
}📝 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.
Suggested change
| if let Err(e) = std::fs::write(&tmp, content) { | |
| return PipelineLockResult::Unavailable { | |
| reason: format!("takeover temp 書き込み失敗: {}", e), | |
| }; | |
| } | |
| if let Err(e) = std::fs::write(&tmp, content) { | |
| let _ = std::fs::remove_file(&tmp); | |
| return PipelineLockResult::Unavailable { | |
| reason: format!("takeover temp 書き込み失敗: {}", e), | |
| }; | |
| } |
🤖 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 `@src/lib-jj-helpers/src/pipeline_lock.rs` around lines 359 - 363, Update the
std::fs::write failure branch in the pipeline lock takeover flow to remove the
temporary file via remove_file(&tmp) before returning
PipelineLockResult::Unavailable, matching the cleanup performed in the rename
failure branch. Preserve the existing failure reason and return behavior.
Contributor
🤖 PR Monitor 分析 (GitHub Actions バックストップ)
Applicable Findings (Critical / High / Major)(該当なし) Applicable Findings (Medium 以下)
Filtered (not applicable)(該当なし) 次のアクション
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
pipeline lock（
pnpm push/ merge pipeline の二重起動を防ぐ advisory lock）の stale takeover が、高競合下で 2 スレッドともAcquiredになり得た欠陥を修正します。concurrent_stale_takeover_only_one_winsテストが Windows で ~10% flaky に落ちていた症状の根本原因で、range 修正 PR の push を非決定的にブロックしていました。実測で確定させた（推論では 3 回外した）
この race は机上の推論が 3 回連続で外れ、そのたびに「1 つしか Acquired にならないはず」と導けてしまいました。最終的にグローバル atomic シーケンス番号を仕込んで Windows + WSL Linux 双方で実測したことで真因が確定しました。#309 の教訓「検証せずに主張しない」を自分のコードのデバッグに適用した形です。
観測で 2 つの独立した欠陥が判明しました。
欠陥 1: 本番コードの TOCTOU（2 スレッド版テストの本物の race）
旧 takeover は
read → 再比較 → remove_file → create_new。再比較と remove が非アトミックで、A が再比較を通過した直後に B が takeover を完走（fresh lock 作成）→ A のremove_fileが B の fresh lock を破壊 → A もcreate_new成功で 2 スレッドともAcquired。旧 doc の「128bit token の偶然一致が必要 = 無視できる」は誤りで、通常のスケジューリング窓でした（だから 1/2^128 ではなく 10% で落ちる）。修正は多層:
<lock>.takeoverのcreate_new）で takeover を試みるスレッドを 1 つに直列化renameで path を atomic 置換。path が一度も不在にならないため、他スレッドの親 fast-pathcreate_newが割り込めず、読み手が空/部分書き込みを観測する窓も消えるcreate_new直後・write_all前の空ファイルを stale と誤判定しないよう、lock content をFresh/Held（空=書き込み中）/Staleの 3 値に分類（姉妹cli-pr-monitor/src/lock.rsが WP-15 で塞いだ bug class と同型）sentinel だけ / rename だけ では不十分だった経緯（Linux 高競合で PARENT 経路と TAKEOVER 経路が各 1 つ
Acquired）を、実測ログ付きで関数 doc に記録しています。欠陥 2: 追加した stress テスト自身の偽陽性
8 スレッド stress テストの初版は
map(join).filter().count()の遅延イテレータで数えており、まだ acquire 中の他スレッドの傍らで先行結果が drop され、そのPipelineLock::dropが lock を削除 → 走行中スレッドが正当に再取得し「2 Acquired」に見えていました。これは同時保持ではなく解放後の再取得で、lock のバグではなくテストのアーティファクトでした。全ガードをVecに collect してから数える形に修正しています。テスト自身を疑う判断も観測から来ました。これに気づかず「テストが落ちる = 本番が悪い」と決めつけ続けていたら、正しい本番修正をしても永遠にテストが落ち、無限に「修正」を重ねていました。
pre-push セルフレビューが追加した自己修復（要注記）
この PR の push 時の pre-push レビューが、私の sentinel 実装に新たな gap を検出し、fix step がコードを追加しました。 sentinel 保持者が
perform_takeover中（ミリ秒オーダー）にクラッシュすると sentinel が孤立し、以降本物の stale lock があっても永久Busyに倒れる、というものです。追加された自己修復:SENTINEL_STALE_SECS(30s) 経過した孤立 sentinel を回収Acquiredを再現）ため、content 由来の reclaim gate で「同じ stale content を読んだスレッド同士だけが 1 つのcreate_newで競い、勝者だけが除去→再作成」する方式正直な申告: この自己修復部分は fix step（自動）が書いたコードです。私が手で正しさを証明したのではなく、実測で検証しました（下記）。concurrency 領域は本 PR で私自身が 3 回推論を外しているため、実測結果を推論より重く扱っています。
検証（Windows + WSL Linux 両方で実測）
hold_pipeline_lock/acquire_pipeline_lock）は不変で呼び出し元（cli-push-runner / cli-merge-pipeline）は無改修cargo test --workspace全 pass /clippy --workspace --all-targets --all-features -- -D warningsclean を Windows + WSL Linux で確認依存関係（マージ順序）
この lock 修正は、後続の range 修正 PR（順位 288/264、
[diff]stage を PR 全体に修正）の push を非決定的にブロックしていた flaky テストの原因です。本 PR を先にマージすると、range PR を master に rebase した際に flaky が解消されます。🤖 Generated with Claude Code
Summary by CodeRabbit