fix(cli-push-runner): diff stage の timeout 欠落を修正 (push パイプライン改善 T6) - #283
Merged
Conversation
stages/diff.rs の run_diff_cmd は Command::output() で子プロセスの終了を無限に 待っていた。他 stage は全て timeout 付き (jj 系 30s / gate 600s / push 300s) で、 diff だけが穴だった。ADR-045 の並列 workspace 運用で jj の lock 競合が起きると、 pnpm push は診断も timeout も無いまま停止し、手動 kill するしかない。 変更: - run_diff_cmd を spawn + drain_pipe_unlimited × 2 + wait_with_timeout_safe に 載せ替え。timeout 時は Err → DiffResult::Error = exit 5 で中断する (fail-closed / ADR-043)。診断に超過秒数・コマンド・jj lock 競合を疑う旨を含める。 - DiffConfig に timeout: Option<u64> を追加 (未指定は 60s = DEFAULT_DIFF_TIMEOUT_SECS)。 [push] timeout と同形。60s は jj 系 30s より長め: diff は working copy の snapshot + 大 diff の書き出しを伴い、jj bookmark list (読み取りのみ) より重い。 timeout の目的はハング検知でありlatency 制限ではないため、誤 timeout で pipeline 全体を落とすより余裕側に倒す (値はユーザー承認済み)。 T5 の run_cmd_shell_unlimited を使わない理由: run_cmd_shell_* は 3 variant すべてが combine_output で stdout と stderr を結合する。diff の stdout は reviewers が読む レビュー対象そのものとしてファイルに書かれるため、jj が stderr に出す警告 (並列 workspace 運用時の Concurrent modification detected = まさに本 PR が想定する状況) の 混入は許容できない。同型の「全量 + 分離 + timeout」は bookmark_check の run_jj_bookmark_list にもあるが、direct args で signature 非互換のため共通化しない (ADR-044 層 1)。variant を追加しなかった判定は ADR-044 に記録した。 実装上の落とし穴 (回帰テストが初版を検出): timeout 後に reader thread を join する初版は、timeout 1s に対し制御が戻るまで 9.6s 掛かった。cmd /c <command> の child は cmd.exe で、孫 (実際の jj) は child.kill() の対象外。孫が pipe の書き込み端を保持したままなので EOF が来ず、 join が孫の自然終了までブロックする = timeout が意味を成さない (本 PR が直す ハングの再生産)。child を kill した 2 経路では join せず detach して即座に返す。 教訓: timeout の回帰テストは Err の内容だけでなく経過時間を assert すること。 検証: - 回帰テスト mod t6_diff_timeout 7 本 + config 2 本 (ADR-049 の流儀。206 → 215 pass)。 bad = 応答しないコマンドを timeout で打ち切り 5s 以内に制御を返すこと / good = timeout 内に終わるコマンドを誤って打ち切らないこと。「stderr を diff に 混ぜない」契約も seal (run_cmd_shell_* に載せ替えると落ちる)。 cli-push-runner のテスト全体が 9.66s → 1.55s = timeout が効いている証跡。 - サンドボックスの jj repo で [diff] command を ping -t (永久応答 = 返らない jj diff の代役) にし、@- から build した修正前 exe と比較。before は diff stage の 所要時間が外側 kill に追随 (25s→24.4s / 10s→9.4s) = 内部に上限が無く放置すれば 無限待ち・診断なし。after は 3.0s で exit 5 + 診断あり。実 jj diff (既定 60s) が 誤 timeout しないことも確認。before の run 後に ping.exe が残存し、孫が kill を 生き延びる実機裏付けも取れた。 - cargo clippy --workspace --all-targets --all-features -- -D warnings で warning 0、 cargo test --workspace 1557 pass、--ignored スイートも pass。 発見 (本 PR 外): lib-subprocess の run_cmd_shell_* 3 variant が同じ穴を持ち、 timeout が wall-clock を縛れない (実測 9.23s / timeout_secs = 1 指定)。影響は quality_gate step_timeout / push timeout / cli-merge-pipeline。1 PR 1 変更のため 本 PR では触れず、計画書 §6 backlog 10 に登録した。 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthrough
Changesdiff timeout 設定と実行
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DiffStage
participant ChildProcess
participant PipeDrainers
participant WaitWithTimeoutSafe
DiffStage->>ChildProcess: spawn diff command
ChildProcess->>PipeDrainers: separate stdout and stderr
DiffStage->>WaitWithTimeoutSafe: wait with configured timeout
WaitWithTimeoutSafe-->>DiffStage: result or timeout
DiffStage-->>DiffStage: return stdout or DiffResult::Error
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)(該当なし) 差分概要 (レビュー指摘が無いための軽量サマリー)diff stage (
次のアクション
|
aloekun
added a commit
that referenced
this pull request
Jul 17, 2026
* docs(push-pipeline-fix-plan): T6 の PR 番号を #283 に backfill T6 の作業コミット時点では PR が未採番だったため、計画書の §4 実施結果と §8 判定記録に「PR 未採番 — 採番後に backfill」と書いて負債を明示していた。PR #283 がマージされたため採番情報のみを更新する。 変更 (3 箇所、いずれも採番情報のみ): - §4 T6 実施結果の見出し: PR 未採番 → PR #283 - §4 T6 の backlog 10 への申し送り: 「本 PR では触れず」→「PR #283 では触れず」 (T5 が §4/§8 の「本 PR」を番号へ解決した慣習に揃える) - §8 判定記録の T6 行: PR 未採番 → PR #283 由来: PR #282 (T5) の post-PR レビューで「T4 行が『本 PR』のまま放置され PR #282 で backfill する羽目になった」負債が指摘され、同じ形を繰り返さないために T6 では 未採番であることを明示していた。本コミットでその明示を回収する。 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks-stop-quality): 品質ゲートの cwd 依存を修正 (push パイプライン改善 T7) Stop hook はセッションの cwd を継承して起動されるため、cwd がリポジトリルート 以外 (例: .takt/runs に cd したまま Stop) だと 2 つの症状が黙って出ていた: 1. hooks-config.toml のルート相対 step (file-length) が「指定されたパスが 見つかりません」で失敗し、品質ゲートが誤 block する (2026-07-16 に実発火)。 pnpm / cargo 系 step は設定ファイルを上方探索するため偶然通っており、 ルート相対パスを書いた step だけが壊れる非対称が発見を遅らせていた。 2. takt subsession 判定が <cwd>/.takt/runs を探して空振りし、active run を 検出できない。ADR-004 の edit:false subsession skip が効かなくなる。 同一の根本原因なので main 冒頭で cwd を 1 度だけプロジェクトルートへ正規化する。 ルートは exe パス (<root>/.claude/<hook>.exe) から導出する — CLAUDE_PROJECT_DIR env は VSCode 拡張環境で空になることを実測確認した (ADR-005 の不安定性が現存)。 config / pipeline lock / telemetry が既に採る exe-relative 規約と同形。 ルート特定不能時は警告のみで継続 (fail-open、pipeline_is_running と同じ線引き)。 main.rs が 800 行上限に触れたため takt 判定を takt_subsession.rs へ分離した。 回帰テスト: tests/t7_cwd_independence.rs に E2E 5 本 + unit 2 本 (26 → 33)。 exe を <root>/.claude/ に staging して spawn し、exe-relative 解決を実配置で 検証する。正規化の呼び出しを外すと bad 2 本が失敗し good 3 本は通ることを確認済み。 * fix(review): apply CodeRabbit fixes for #284 Resolved findings: - [Major] src/hooks-stop-quality/tests/t7_cwd_independence.rs:96 hook の終了ステータスを確認してください * fix(review): CodeRabbit 指摘の stderr 出力を補完 (#284) auto-fix が追加した assert_hook_success は exit code assert 自体は入れたが、 指摘の「失敗時は stderr を出す」部分が未達だった: メッセージに stdout を渡しており、 かつ stderr.join() より前に呼ばれるため構造上 stderr を出せない。 本 hook の診断 (cwd 正規化の警告等) は eprintln! = stderr にしか出ないため、 指摘が想定する「非 0 exit かつ stdout が空」の失敗では stderr だけが手掛かりになる。 stderr を join してから assert に渡す形へ補正した。 guard が空振りでないことを実証済み: staged exe を where.exe (非 0 exit・stdout 空) に 差し替えると 5 本すべてが exit code Some(2) で失敗する (guard 導入前なら None を 期待する 3 本が false green で素通りしていた)。 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Command::output()(無限待ち) を timeout 付き実行に載せ替え、超過時はDiffResult::Error= exit 5 で中断する (fail-closed)[diff] timeoutを追加 (未指定は既定 60s)。[push] timeoutと同形の escape hatchrun_cmd_shell_*は 3 variant すべてが両者を結合するため使わず、callsite 実装に留めたmod t6_diff_timeout7 + config 2)。timeout の検証は 経過時間を assert するlib-subprocessの同型欠陥は本 PR では触れず backlog に登録Context
Why:
stages/diff.rsのrun_diff_cmdはCommand::output()で子プロセスの終了を無限に待っていた。他 stage は全て timeout 付き (jj 系 30s / gate 300s / push 300s) で、diff だけが穴だった。ADR-045 の並列 workspace 運用で jj の lock 競合が起きると、pnpm pushは診断も timeout も無いまま停止し手動 kill するしかない。Trigger:
docs/push-pipeline-fix-plan.mdの T6 (2026-07-16 の push パイプライン調査)。in the wild の発火記録は無く、T5 と同じくコード監査で「他 stage は全て timeout 付き」という非対称として特定されたもの。なぜ T5 の
run_cmd_shell_unlimitedを使わないか:run_cmd_shell_*は 3 variant すべてがcombine_outputで stdout と stderr を結合する。diff の stdout は reviewers が読むレビュー対象そのものとしてファイルに書かれるため、jj が stderr に出す警告 (並列 workspace 運用時のConcurrent modification detected= まさに本 PR が想定する状況) の混入は許容できない。結合の有無は variant の軸 (drain 戦略) では表現できず、4 つ目の variant は骨格に載らない別 family の新設になるため、ADR-044 層 1 に従い callsite に残置した。実装上の落とし穴 (回帰テストが初版を検出): 「timeout 後に reader thread を join する」初版は timeout 1s に対し制御が戻るまで 9.6s 掛かった。
cmd /c <command>の child は cmd.exe で、孫 (実際のjj) はchild.kill()の対象外。孫が pipe の書き込み端を保持したままなので EOF が来ず、join が孫の自然終了までブロックする = timeout が意味を成さない。child を kill した 2 経路では join せず detach する形に修正した。Scope decision: 実装中に
lib-subprocessのrun_cmd_shell_*3 variant が同じ穴を持つことが判明した (実測 9.23s /timeout_secs = 1指定)。影響は quality_gatestep_timeout/ pushtimeout/ cli-merge-pipeline。計画 §2 原則 4 (1 PR 1 変更) に従い本 PR では触れず、§6 backlog 10 に登録した。Validation
cargo test -p cli-push-runner: 215 pass (206 → 215、+9 本)cargo test --workspace: 1557 pass /--ignoredスイートも passcargo clippy --workspace --all-targets --all-features -- -D warnings: warning 0pnpm pushpre-push review (pre-push-review-refute, 2026-07-17): verdict=APPROVE (simplicity / security とも、fix iteration 0)。dogfood 実測 338s (pre_checks 1.2s / quality_gate 48.9s / diff 0.1s / takt 285.1s / push 2.3s)[diff] commandをping -t(永久応答 = 返らないjj diffの代役) にし、@-から build した修正前 exe と比較。before は diff stage の所要時間が外側 kill にそのまま追随 (25s→24.4s / 10s→9.4s) = 内部に上限が無く、放置すれば無限待ち・診断なし。after は 3.0s で exit 5 + 「jj lock 競合を疑え」の診断。実jj diff(既定 60s) が誤 timeout せず 510 行を書き出すことも確認ping.exeが残存し、孫が kill を生き延びる (= join がブロックする) ことを実機で裏付けReferences
run_cmd_shell_unlimitedを追加した PR。本 PR がそれを使わない理由の前提Summary by CodeRabbit
[diff] timeoutでタイムアウト時間を変更できます。