Skip to content

fix(lib-jj-helpers): pipeline lock takeover の二重Acquiredレース解消 - #273

Merged
aloekun merged 1 commit into
masterfrom
pipeline-lock-takeover-race-fix
Jul 14, 2026
Merged

fix(lib-jj-helpers): pipeline lock takeover の二重Acquiredレース解消#273
aloekun merged 1 commit into
masterfrom
pipeline-lock-takeover-race-fix

Conversation

@aloekun

@aloekun aloekun commented Jul 14, 2026

Copy link
Copy Markdown
Owner

Summary

PR #271 で導入した pipeline lock の stale takeover ロジック (pipeline_lock.rs) に、cargo test --workspace の並列実行下で顕在化する flaky なレース条件があったため修正します。PR #272 (docs のみの想定) の quality gate 実行中に concurrent_stale_takeover_only_one_wins が偶発的に失敗し発覚しました。

原因

takeover_stale_lockremove_file が無条件実行だったため、以下の interleaving で 2 プロセスとも Acquired になり得ました:

  1. プロセス A: remove_file → 成功 (stale lock 削除)
  2. プロセス A: create_new → 成功 (A の fresh lock 作成、A = Acquired)
  3. プロセス B: remove_fileA が作った fresh lock を検証なしに削除
  4. プロセス B: create_new → 成功 (B の lock 作成、B も Acquired)

修正

remove_file 実行直前に、stale と判定した時点の生 content (stale_snapshot) と現在の content を再比較し、一致する場合のみ削除するように変更しました。他プロセスが同じ隙間で先に takeover 済み (content が変化済み) なら削除をスキップし、後続の create_new が自然に AlreadyExists で失敗して Busy に落ちます。

  • read_fresh_lock のロジックを is_fresh_content に分離し、生 content を直接受け取れるようにして再利用
  • concurrent_stale_takeover_only_one_wins (実スレッドレース) を 10 回連続実行して再発しないことを確認
  • pre-push review の fix iteration で takeover_stale_lock_skips_remove_when_snapshot_is_stale (決定論的な regression test) が追加され、レース条件に頼らずこの修正を直接検証

Test plan

  • cargo test --workspace green (全 crate)
  • cargo clippy --workspace --all-targets -- -D warnings warnings なし
  • concurrent_stale_takeover_only_one_wins を 10 回連続実行し flaky でないことを確認
  • pre-push review (security-review / simplicity-review) 実施、metrics override は justification 付きで承認済み (既存レビュー済みコードの前提行数増加、今回の diff は test 追加のみ)

Summary by CodeRabbit

  • バグ修正
    • ロック取得中に別の処理がロック内容を更新した場合、誤って有効なロックを削除しないよう改善しました。
    • 競合発生時は既存のロックを保持したまま、処理中であることを正しく通知します。
    • 古いロックの引き継ぎ判定を強化し、同時実行時の安全性と安定性を向上しました。

…再検証を追加 — 二重Acquiredレースの解消

concurrent_stale_takeover_only_one_wins が cargo test --workspace 並列実行下でflakyだったのは、
takeover 時の remove_file が無条件で、先着プロセスが作った fresh lock を後発側が検証なしに
消してしまい両方Acquiredになりうる残余レースが実際に再現したため。remove直前に現在のcontentを
再読込しstale時点のsnapshotと比較、一致する場合のみ削除するよう変更。10回連続テストで再現しないことを確認。
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9fb16b97-b637-43b6-aea8-187504aa5e9a

📥 Commits

Reviewing files that changed from the base of the PR and between c5f7cb5 and 7cf5df7.

📒 Files selected for processing (1)
  • src/lib-jj-helpers/src/pipeline_lock.rs

📝 Walkthrough

Walkthrough

既存ロックの鮮度判定を内容ベースに分離し、stale takeover 前後のロック内容を比較する処理と、その競合ケースを検証する回帰テストを追加した。

Changes

パイプラインロック競合処理

Layer / File(s) Summary
鮮度判定とスナップショット取得
src/lib-jj-helpers/src/pipeline_lock.rs
is_fresh_content を切り出し、既存ロックの内容をスナップショットとして取得して fresh/stale 判定と takeover に利用する。
takeover 前の再比較と回帰検証
src/lib-jj-helpers/src/pipeline_lock.rs
stale 判定後にロック内容が変化した場合は削除せず、差し替え後の内容が残ったまま Busy になることをテストする。

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed pipeline lock takeover の二重 Acquired レース修正という本変更の主旨を的確に表しています。
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pipeline-lock-takeover-race-fix

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 PR Monitor 分析 (GitHub Actions バックストップ)

  • トリガー: issue_comment (created) / 実行 run
  • CI: CodeRabbit チェックは pass 表示だが、これは要約コメント投稿完了を示すのみで実コードレビューは未実施 (下記参照)。他の CI チェックは検出されず (required checks なし)。
  • レビュー状況: CodeRabbit はレビュー未着 — レート制限超過により今回のレビューは開始されず (review limit reached, 次回利用可能まで約9分、コメント投稿時点 2026-07-14T14:25:18Z)。人間レビュー・他 bot からの承認/指摘なし (reviews API 応答は空)。インラインコメントも 0 件。
  • Verdict: user_decision (レビュー未実施のため fix/approve いずれの判断材料もなし。CodeRabbit のレート制限解消後の再レビュー待ち、または手動 @coderabbitai review トリガーが必要)

Applicable Findings (Critical / High / Major)

該当なし (レビュー指摘 0 件)

Applicable Findings (Medium 以下)

該当なし

Filtered (not applicable)

該当なし

次のアクション

  • CodeRabbit のレート制限解除後 (約9分後) に @coderabbitai review をコメントするか、レート制限解除を待って自動再レビューを確認する。
  • diff 概要: src/lib-jj-helpers/src/pipeline_lock.rs 1ファイルのみ変更。stale lock takeover 時の TOCTOU レース (remove_file 前に別プロセスが先に takeover 済みかどうかをスナップショット比較で再確認) を修正し、regression guard テスト (takeover_stale_lock_skips_remove_when_snapshot_is_stale) を追加。既存の read_fresh_lock ロジックを is_fresh_content として分離し、acquire_pipeline_lock_at からもスナップショット取得済み content を再利用する形にリファクタ。ADR-043 (fail-open advisory lock) の残余 TOCTOU 許容方針に沿ったコメントも更新されている。
  • CodeRabbit の実レビューがまだ得られていないため、マージ判断は次回レビュー結果を確認してから行うことを推奨。

@aloekun

aloekun commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 PR Monitor 分析 (GitHub Actions バックストップ)

  • トリガー: issue_comment (created) / 実行 run
  • CI: CodeRabbit チェックは pending (Review in progress)。他の CI チェックは検出されず (required checks なし)。
  • レビュー状況: 前回分析 (2026-07-14T14:27:18Z 投稿) 以降、PR 作者 (aloekun) が 2026-07-14T15:44:16Z@coderabbitai review を手動トリガー。CodeRabbit は 2026-07-14T15:44:21Z に「Review triggered」と応答したが、要約コメント (id 4970286610) は 2026-07-14T15:44:39Z 時点でも「Currently processing new changes... please wait」のままで、実際のレビュー結果 (指摘事項・approve/request-changes) はまだ投稿されていない。pulls/273/reviews API・インラインコメント (pulls/273/comments) は共に 0 件のまま。人間レビュー・他 bot からの指摘もなし。
  • Verdict: user_decision (CodeRabbit のレビューが実行中で結果未確定のため、fix/approve いずれの判断材料もまだ無い)

Applicable Findings (Critical / High / Major)

該当なし (レビュー指摘 0 件)

Applicable Findings (Medium 以下)

該当なし

Filtered (not applicable)

該当なし

次のアクション

  • CodeRabbit のレビュー完了 (要約コメントの "processing" 状態解消、または新規レビュー/インラインコメントの投稿) を待って再分析する。
  • diff 概要 (前回分析から変更なし): src/lib-jj-helpers/src/pipeline_lock.rs 1ファイルのみ変更。stale lock takeover 時の TOCTOU レース (remove_file 前にスナップショット再比較で他プロセスの先行 takeover を検知しスキップ) を修正し、regression guard テスト (takeover_stale_lock_skips_remove_when_snapshot_is_stale) を追加。read_fresh_lock のロジックを is_fresh_content に分離し acquire_pipeline_lock_at からも再利用。ADR-043 (fail-open advisory lock) の残余 TOCTOU 許容方針に沿ったコメント更新も含む。

@aloekun
aloekun merged commit 9c0028e into master Jul 14, 2026
1 check passed
@aloekun
aloekun deleted the pipeline-lock-takeover-race-fix branch July 14, 2026 16:19
aloekun added a commit that referenced this pull request Jul 15, 2026
* docs(todo): PR #273 post-merge feedback採用6件 (順位301-306)

- 301: TOCTOU (remove+create_new) パターン検出 lint rule (exclusive lock実装限定)
- 302: deterministic concurrency test テンプレート記録
- 303: advisory lock の TOCTOU window 許容可否 明示コメント設計チェックリスト
- 304: quality gate混入時の jj split + jj rebase 復旧パターン記録
- 305: metrics violation の pre-existing 判定基準明文化
- 306: quality gate isolation機構見送りのnegative result記録

* fix(review): apply CodeRabbit fixes for #274

Resolved findings:
- [Major] docs/todo13.md:1241 コメントの有無では TOCTOU 対策を検出できません。
- [Major] docs/todo13.md:1317 pre-existing override の監査証跡を完了基準に追加してください。
- [Major] docs/todo13.md:1337 **復旧 convention は isolation の代替ではありません。** `jj split`/`jj rebase` は混入後の復旧策であり、混在した変更に対する gate 実行を予防しな…

* fix(review): CodeRabbit Major 3件対応 — 順位301/304/305/306 の完了基準・記録内容を修正 (PR #274)

- 301: comment-presence のみのlint検出をpattern検出(読込→比較→remove_fileの出現順序)へ強化、
  negative fixture (コメントのみ実装が検出されること)を追加、271.mdで既に却下された類似案との
  関連を明記
- 305: pre-existing override監査証跡に基準時点/現時点の計測差分を追加要件化
- 304/306: 'recoveryはisolationの代替' という誤った表現を修正。304に混在gate結果の無効化・
  再実行手順を追加、306に予防機能欠如という残存リスクと再検討条件を明記
- 本文中の順位N直接参照(ADR-033違反)を3箇所修正 (スペースなし表記のため既存grepで未検出だった分含む)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant