fix(post-merge-feedback): timeout reconciliation + 並行起動 guard + 並列化#79
Merged
fix(post-merge-feedback): timeout reconciliation + 並行起動 guard + 並列化#79
Conversation
Phase B dogfood で発見されたタイムアウト経路の残存不具合と、ボトルネック分析で 判明した sequential 構造を併せて修正する。 ## 主な変更 ### 1. workflow の並列化 (.takt/workflows/post-merge-feedback.yaml) 3 つの analyze facet (analyze-pr / analyze-session / analyze-prepush-reports) は 独立した情報源を扱うため parallel block で並列実行する。 PR #78 計測 (sequential 12m13s) → parallel 想定 ~7m30s に短縮。 ### 2. TAKT_TIMEOUT_SECS: 600s → 1200s PR #77 (14m21s)、#78 (12m13s) 観測実績を踏まえた暫定値。並列化後の想定 7m30s に対し 2x の安全係数。docstring に「band-aid」と明記し、本質解は workflow 構造で 時間を抑えることだと残した。 ### 3. caller-side reconciliation (run() 末尾) Windows の child.kill() が takt の descendants を殺せず、Rust が timeout で kill した後も takt が orphan として走り続けて report を完成させるケースを観測した (PR #78 で kill 後 2m13s で feedback-report.md 完成)。 run_takt_workflow の戻り値に関わらず copy_feedback_report を必ず試行し、report が 存在すれば success 扱いとする。既存の .failed marker は cleanup_failed_marker で 除去する。 ### 4. 並行起動 guard (CONCURRENT_RUN_GUARD_SECS = 1500s) cross-invocation context overwrite race を発生源で予防。 .takt/post-merge-feedback-context.json が 1500s 以内に書かれていれば、orphan takt が走行中の可能性が高いとみなして新規実行を refuse する。 ### 5. テスト追加 (5 件) concurrent_run_guard / context_age_secs の振る舞いを検証。 ### 6. ADR-030 更新 - レイテンシモデル (parallel 想定 = max(analyze) + aggregate) - Reconciliation 設計の根拠 - 並行起動 guard の意図と Phase B 外切り出し方針 ## 残存リスク (Phase B 外) - Windows の job object でプロセスツリー全体を kill する根本対策 - 完全な per-invocation isolation (takt context dir 連携)
📝 Walkthroughウォークスルーワークフローが順序実行から並列実行に変更され、3つの分析ファセット（PR分析、セッション分析、プリプッシュレポート分析）が同時実行されるようになりました。フィードバック生成コードにはタイムアウト値が600秒から1200秒に引き上げられ、並行実行ガード機構と調和ロジックが追加されました。 変更内容
シーケンス図sequenceDiagram
participant Workflow as ワークフロー<br/>オーケストレータ
participant AnalyzePR as PR分析ステップ
participant AnalyzeSession as セッション分析<br/>ステップ
participant AnalyzePrepush as プリプッシュ分析<br/>ステップ
participant Aggregate as フィードバック<br/>集約ステップ
rect rgba(100, 150, 200, 0.5)
Note over Workflow,AnalyzePrepush: 新しい並列実行フロー
end
Workflow->>AnalyzePR: 分析開始（並列）
Workflow->>AnalyzeSession: 分析開始（並列）
Workflow->>AnalyzePrepush: 分析開始（並列）
par 並列実行
AnalyzePR-->>Workflow: 分析完了
and
AnalyzeSession-->>Workflow: 分析完了
and
AnalyzePrepush-->>Workflow: 分析完了
end
Workflow->>Aggregate: すべての結果を使用して集約
Aggregate-->>Workflow: フィードバックレポート生成
推定コードレビュー工数🎯 3 (中程度) | ⏱️ ~20分 関連する可能性のあるPR
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (4)
src/cli-merge-pipeline/src/feedback.rs (4)
432-435:SystemTime::elapsed()は将来 mtime（clock skew）でErrを返し guard を素通りさせる点に留意。
fs::metadata(...).modified()が NTFS / 時刻同期巻き戻し等で「未来の mtime」を返した場合、modified.elapsed()はSystemTimeErrorでNone化 →check_concurrent_run_guardはOk(())を返し、orphan が走っていても guard を通します。Windows-local 限定運用（時刻同期前提）なので実害は低いと思いますが、保守的にやるならelapsed()失敗時は age=0 扱いで guard を効かせる選択肢もあります。♻️ 防御的な代替実装の例
fn context_age_secs(context_path: &Path) -> Option<u64> { - let modified = fs::metadata(context_path).ok()?.modified().ok()?; - modified.elapsed().ok().map(|d| d.as_secs()) + let modified = fs::metadata(context_path).ok()?.modified().ok()?; + // 未来 mtime (clock skew) は age=0 として guard を効かせる側に倒す + Some(modified.elapsed().map(|d| d.as_secs()).unwrap_or(0)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-merge-pipeline/src/feedback.rs` around lines 432 - 435, context_age_secs currently returns None when modified.elapsed() errors (e.g. future mtime due to clock skew), which lets check_concurrent_run_guard skip the guard; change context_age_secs (the function calling fs::metadata(...).modified()) to treat elapsed() errors defensively by returning Some(0) (or otherwise clamping to 0) instead of None so a failed elapsed() enforces the guard rather than bypassing it.
848-865: stale ケースの実挙動が未検証。
concurrent_run_guard_passes_when_context_staleはコメント通り、CONCURRENT_RUN_GUARD_SECSが const のため stale 経路を直接通せずcontext_age_secsの存在確認のみに留まっています。check_concurrent_run_guardのage >= CONCURRENT_RUN_GUARD_SECS → Ok(())分岐そのものは現状ノーカバーです。filetimeクレートを足すか、guard ロジックを(context_path, ttl_secs)受け取りの薄いヘルパーに切り出して TTL=0 の単体テストを通すと、命名どおりの保証になります。♻️ ヘルパー切り出し案
-fn check_concurrent_run_guard(context_path: &Path) -> Result<(), String> { - let Some(age) = context_age_secs(context_path) else { - return Ok(()); - }; - if age >= CONCURRENT_RUN_GUARD_SECS { - return Ok(()); - } - Err(format!( +fn check_concurrent_run_guard_with_ttl( + context_path: &Path, + ttl_secs: u64, +) -> Result<(), String> { + let Some(age) = context_age_secs(context_path) else { + return Ok(()); + }; + if age >= ttl_secs { + return Ok(()); + } + Err(format!( "前回の post-merge-feedback workflow がまだ進行中の可能性 \ - (context.json が {}s 前に書かれた)。{}s 待つか、進行中の takt が無いことを\ + (context.json が {}s 前に書かれた)。{}s 待つか、進行中の takt が無いことを\ 確認してから手動で {} を削除してください。", age, - CONCURRENT_RUN_GUARD_SECS, + ttl_secs, context_path.display() )) } + +fn check_concurrent_run_guard(context_path: &Path) -> Result<(), String> { + check_concurrent_run_guard_with_ttl(context_path, CONCURRENT_RUN_GUARD_SECS) +}これで
check_concurrent_run_guard_with_ttl(&path, 0)を使った stale 通過テストが書けます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-merge-pipeline/src/feedback.rs` around lines 848 - 865, The test currently doesn't exercise the stale branch; extract the TTL-specific logic into a thin helper (e.g. check_concurrent_run_guard_with_ttl(context_path: &Path, ttl_secs: u64) -> Result<(), Error>) that implements the age >= CONCURRENT_RUN_GUARD_SECS check using the passed ttl_secs, refactor existing check_concurrent_run_guard to call this helper with the constant CONCURRENT_RUN_GUARD_SECS, and add a unit test that calls check_concurrent_run_guard_with_ttl(&path, 0) to assert it returns Ok(()) (or otherwise succeeds) to cover the stale-path behavior; alternatively, if you prefer file timestamp manipulation, add the filetime crate and modify the test to set mtime old enough and then call the original check_concurrent_run_guard to validate the stale branch.
442-457: 並行起動 guard が「正常完了直後の連続 merge」も最大 25 分ブロックする副作用に注意。
check_concurrent_run_guardはcontext.jsonの mtime のみを基準にしているため、orphan 残存の有無を区別できません。前回 run が 正常完了 していても context.json は run 開始時の mtime のままで、CONCURRENT_RUN_GUARD_SECS = 1500sが経過するまで次のpnpm merge-prがエラー終了します。Phase B のスコープ外とは承知していますが、
- 正常完了した経路 (
copy_feedback_report成功 +cleanup_failed_marker) で context.json も削除 / マーカー化する、- もしくは
<pr>.md/<pr>.md.failedの存否との AND で判定する、といった軽い改善で 25 分ロックの副作用は緩和できます。エラーメッセージにも「進行中の takt が無い場合は context.json を削除して再実行してください」と既に書かれているので、Phase C 以降で検討可能ならそれで十分です。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-merge-pipeline/src/feedback.rs` around lines 442 - 457, check_concurrent_run_guard currently only checks context.json mtime (via context_age_secs) which causes a finished run to still block new runs for CONCURRENT_RUN_GUARD_SECS; modify the flow so completed runs remove or mark context.json (in the success path that calls copy_feedback_report and cleanup_failed_marker) or change check_concurrent_run_guard to treat a run as finished when context.json is old OR when there is no active marker by also checking for the existence of the per-PR output files (e.g., "<pr>.md" or "<pr>.md.failed"); update references to check_concurrent_run_guard, context_age_secs, copy_feedback_report, and cleanup_failed_marker accordingly so successful completion either deletes/flags context.json or the guard uses an AND/OR check with "<pr>.md"/"<pr>.md.failed" presence to avoid blocking normal consecutive merges.
896-903: guard をfetch_pr_time_rangeより前に移すと余計なgh呼び出しを避けられます。現状は
fetch_pr_time_range(gh API 呼び出し) が先に走り、その後でcheck_concurrent_run_guardが refuse する可能性があります。並行起動を弾く判定には PR 時刻 range は不要なので、guard を最初に持って来ると無駄な外部 I/O とエラーパスが減ります。最適化レベルの提案です。♻️ 並び替え案
pub fn run(input: &FeedbackInput) -> Result<PathBuf, String> { - let range = fetch_pr_time_range(input.pr_number, input.owner_repo) - .map_err(|e| format!("PR 時刻 range 取得失敗: {}", e))?; - let context_path = input.repo_root.join(CONTEXT_PATH); let transcript_path = input.repo_root.join(TRANSCRIPT_PATH); check_concurrent_run_guard(&context_path)?; + + let range = fetch_pr_time_range(input.pr_number, input.owner_repo) + .map_err(|e| format!("PR 時刻 range 取得失敗: {}", e))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli-merge-pipeline/src/feedback.rs` around lines 896 - 903, Move the concurrent-run guard check to the start of run to avoid unnecessary GH API calls: call check_concurrent_run_guard(&context_path) immediately after computing context_path (or compute context_path first) and return early on failure, then call fetch_pr_time_range(input.pr_number, input.owner_repo) afterwards; update run to compute context_path/transcript_path, invoke check_concurrent_run_guard before fetch_pr_time_range, and keep existing error mapping logic unchanged for fetch_pr_time_range.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli-merge-pipeline/src/feedback.rs`:
- Around line 432-435: context_age_secs currently returns None when
modified.elapsed() errors (e.g. future mtime due to clock skew), which lets
check_concurrent_run_guard skip the guard; change context_age_secs (the function
calling fs::metadata(...).modified()) to treat elapsed() errors defensively by
returning Some(0) (or otherwise clamping to 0) instead of None so a failed
elapsed() enforces the guard rather than bypassing it.
- Around line 848-865: The test currently doesn't exercise the stale branch;
extract the TTL-specific logic into a thin helper (e.g.
check_concurrent_run_guard_with_ttl(context_path: &Path, ttl_secs: u64) ->
Result<(), Error>) that implements the age >= CONCURRENT_RUN_GUARD_SECS check
using the passed ttl_secs, refactor existing check_concurrent_run_guard to call
this helper with the constant CONCURRENT_RUN_GUARD_SECS, and add a unit test
that calls check_concurrent_run_guard_with_ttl(&path, 0) to assert it returns
Ok(()) (or otherwise succeeds) to cover the stale-path behavior; alternatively,
if you prefer file timestamp manipulation, add the filetime crate and modify the
test to set mtime old enough and then call the original
check_concurrent_run_guard to validate the stale branch.
- Around line 442-457: check_concurrent_run_guard currently only checks
context.json mtime (via context_age_secs) which causes a finished run to still
block new runs for CONCURRENT_RUN_GUARD_SECS; modify the flow so completed runs
remove or mark context.json (in the success path that calls copy_feedback_report
and cleanup_failed_marker) or change check_concurrent_run_guard to treat a run
as finished when context.json is old OR when there is no active marker by also
checking for the existence of the per-PR output files (e.g., "<pr>.md" or
"<pr>.md.failed"); update references to check_concurrent_run_guard,
context_age_secs, copy_feedback_report, and cleanup_failed_marker accordingly so
successful completion either deletes/flags context.json or the guard uses an
AND/OR check with "<pr>.md"/"<pr>.md.failed" presence to avoid blocking normal
consecutive merges.
- Around line 896-903: Move the concurrent-run guard check to the start of run
to avoid unnecessary GH API calls: call
check_concurrent_run_guard(&context_path) immediately after computing
context_path (or compute context_path first) and return early on failure, then
call fetch_pr_time_range(input.pr_number, input.owner_repo) afterwards; update
run to compute context_path/transcript_path, invoke check_concurrent_run_guard
before fetch_pr_time_range, and keep existing error mapping logic unchanged for
fetch_pr_time_range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3149b269-5c25-47d2-8362-c865fc1edce0
📒 Files selected for processing (3)
.takt/workflows/post-merge-feedback.yamldocs/adr/adr-030-deterministic-post-merge-feedback.mdsrc/cli-merge-pipeline/src/feedback.rs
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.
Summary
Phase B dogfood (PR #77 / #78) で発見されたタイムアウト経路の残存不具合と、ボトルネック分析で判明した sequential 構造を併せて修正する。
主な観測:
child.kill()が takt の descendants を殺せず、kill 後 2m13s で feedback-report.md が生成 →.failedmarker が誤情報になっていた.takt/post-merge-feedback-context.jsonの cross-invocation overwrite race が orphan + 新規起動の組み合わせで成立する修正内容
1. workflow を並列化 (.takt/workflows/post-merge-feedback.yaml)
3 つの analyze facet (
analyze-pr/analyze-session/analyze-prepush-reports) をparallel:block で並列実行。既存のpre-push-review.yamlパターンに踏襲。total = max(analyze) + aggregate に短縮。2.
TAKT_TIMEOUT_SECS: 600s → 1200s (feedback.rs:42)並列化想定 ~7m30s に対し 2x の安全係数。docstring に band-aid であることと、本質解は workflow 構造で時間を抑える方針を明記。
3. caller-side reconciliation (feedback.rs:836-877)
run_takt_workflowの戻り値に関わらずcopy_feedback_reportを必ず試行:Windows の
child.kill()は takt の descendants を殺せず orphan として走り続けるため、Rust が timeout した後でも report が完成するケースを救済。既存の.failedmarker は cleanup する。4. 並行起動 guard (feedback.rs:787-820)
CONCURRENT_RUN_GUARD_SECS = 1500(timeout 1200s より少し長い) を導入。run()冒頭で.takt/post-merge-feedback-context.jsonの経過時刻を確認し、TTL 内なら refuse:cross-invocation race を発生源で予防。
5. テスト追加 (5 件)
concurrent_run_guard_passes_when_context_absentconcurrent_run_guard_blocks_when_context_recentconcurrent_run_guard_passes_when_context_stalecontext_age_secs_returns_none_for_missingcontext_age_secs_returns_some_for_existing6. ADR-030 更新 (docs/adr/adr-030-deterministic-post-merge-feedback.md)
total = max(analyze) + aggregateと PR fix(post-merge-feedback): task labeling 規約導入で run dir 検索を決定論化 #78 計測表を追記Reconciliation (Phase B post-fix で追加)セクション並行起動 guard (Phase B post-fix で追加)セクションボトルネック分析の再掲
PR #78 trace.md から step ごとの実時間:
並列化により ~4m 43s 短縮。
残存リスク (Phase B 外)
child.kill()で descendants まで kill する根本対策。Plan B 相当だが工数大、reconciliation で実用上カバーできるため後回し.takt/post-merge-feedback-context.jsonの race を構造的に消す。Phase C 以降に検討Test plan
pnpm exec takt prompt post-merge-feedback: 並列化後の workflow が parse 成功 (Step 1 =analyzeparallel, Step 2 =aggregate-feedback)dogfood 検証: 本 PR マージ後の post-merge-feedback workflow が parallel 構成で
~7m30s以内に完了し、.claude/feedback-reports/<pr>.mdを正しく生成することを確認する。References
Summary by CodeRabbit
リリースノート
New Features
Bug Fixes
Documentation