docs(tickets): llm-worker-transient-retry完了
This commit is contained in:
parent
09a1cde92c
commit
72e03f9e8f
1
TODO.md
1
TODO.md
|
|
@ -7,7 +7,6 @@
|
|||
- Pod: 任意ターンからの Fork（複数ターン巻き戻しを汎用化） → [tickets/pod-session-fork.md](tickets/pod-session-fork.md)
|
||||
- Pod: 子→親の TurnEnded/Errored callback を親由来ターンのみに絞る → [tickets/pod-parent-turn-callback.md](tickets/pod-parent-turn-callback.md)
|
||||
- llm-worker のエラー耐性
|
||||
- HTTP transient リトライ → [tickets/llm-worker-transient-retry.md](tickets/llm-worker-transient-retry.md)
|
||||
- ストリーム途中失敗時の継続 → [tickets/llm-worker-stream-continuation.md](tickets/llm-worker-stream-continuation.md)
|
||||
- ネイティブ GUI クライアント MVP → [tickets/native-gui-mvp.md](tickets/native-gui-mvp.md)
|
||||
- TUI 拡充
|
||||
|
|
|
|||
|
|
@ -1,77 +0,0 @@
|
|||
# llm-worker: HTTP transient リトライ
|
||||
|
||||
## 背景
|
||||
|
||||
`crates/llm-worker/src/llm_client/transport.rs` はリトライを持たず、
|
||||
upstream が一時的に不調だったときのエラーがそのまま `WorkerError` に
|
||||
伝播して Run が中断する。実セッションでも以下が観測されている:
|
||||
|
||||
- セッション `019de419-6f8b-71a0-9e56-f0b9a6c7098b.jsonl:85` で
|
||||
`API error (status: 503): upstream connect error ... Connection refused`。
|
||||
これは `transport.rs:194-216` で `response.status().is_success()` 前に
|
||||
返される pre-stream の経路。リクエストはまだ消費されていない。
|
||||
- Anthropic の `overloaded_error` (529) や Codex backend の 503 も
|
||||
同経路で観測される transient な事象。
|
||||
|
||||
これらは「ヘッダが返る前の段階」で出るため、SSE を読み始めて
|
||||
出力トークンを発生させる前であり、素朴な再送でべき等に復旧できる典型ケース。
|
||||
ストリームが途中で切れた場合のリカバリは別の話（→ `llm-worker-stream-continuation`）。
|
||||
|
||||
## 方針
|
||||
|
||||
`transport.rs` の HTTP 送信層に transient エラー向けの再送を追加する。
|
||||
SSE 読み出し開始後 (`response.bytes_stream()` 以降) のエラーは対象外で、
|
||||
従来どおり `ClientError::Sse` として上に流す。
|
||||
|
||||
scheme（OpenAI / Anthropic / Responses 等）に依存しない共通処理として、
|
||||
すべての client から同じ振る舞いで使える形にする。
|
||||
|
||||
## 要件
|
||||
|
||||
### リトライ対象
|
||||
|
||||
- HTTP ステータス: 408 / 425 / 429 / 500 / 502 / 503 / 504 / 529
|
||||
- `reqwest::Error::is_connect()` / `is_timeout()` 由来の送信失敗
|
||||
- それ以外の `ClientError::Api { status }` および `ClientError::Json`、
|
||||
ストリーム開始後の `ClientError::Sse` は対象外
|
||||
|
||||
判定は `is_retryable(&ClientError) -> bool` を `error.rs` に置いて一箇所に集約する。
|
||||
|
||||
### バックオフ
|
||||
|
||||
- フルジッタ付き指数（base/cap は実装時に妥当な値で固定。後で manifest 化したくなったら別ticket）
|
||||
- `Retry-After` ヘッダがあれば指数バックオフを上書きしてその時間待つ
|
||||
- 上限: 最大試行回数 + 累積タイムアウトの両方を持つ
|
||||
|
||||
### ログ
|
||||
|
||||
- リトライ発火ごとに `warn!`（ステータス、attempt 番号、次の wait）
|
||||
|
||||
### 既存挙動の温存
|
||||
|
||||
- ストリーム途中で切れた場合の挙動には手を入れない
|
||||
（`transport.rs:231` の `ClientError::Sse` 経路はそのまま）
|
||||
- 成功時のレイテンシに観測可能なオーバヘッドを足さない
|
||||
|
||||
## 完了条件
|
||||
|
||||
- `is_retryable` のテーブル駆動 unit test
|
||||
- 503 / 529 / connect refused をモックした unit test が、
|
||||
規定回数までリトライして「最終的に成功」「上限到達でエラー」の両ケースを通る
|
||||
- `Retry-After: 5` を返すモックでは指数を上書きして 5s 待っている
|
||||
（仮想時間で検証）
|
||||
- mid-stream で `ClientError::Sse` を起こすモックでリトライが発火しない
|
||||
- `cargo check` / `cargo test` が `llm-worker` で通る
|
||||
|
||||
## 範囲外
|
||||
|
||||
- mid-stream（SSE 読み中）の継続再開 → `llm-worker-stream-continuation`
|
||||
- プロバイダ別の細かい retry policy（共通既定で十分）
|
||||
- リトライ上限値の manifest からの上書き（必要になったら別ticket）
|
||||
|
||||
## Review
|
||||
|
||||
- 状態: Approve
|
||||
- レビュー詳細: [./llm-worker-transient-retry.review.md](./llm-worker-transient-retry.review.md)
|
||||
- 対象 commit: `7183847`
|
||||
- 日付: 2026-05-04
|
||||
|
|
@ -1,62 +0,0 @@
|
|||
# Review: llm-worker HTTP transient リトライ
|
||||
|
||||
対象 commit: `7183847` (`develop` `1451998` から派生)
|
||||
対象 branch: `llm-worker-transient-retry`
|
||||
|
||||
## 前提・要件の確認
|
||||
|
||||
### リトライ対象 (要件)
|
||||
- HTTP ステータス 408 / 425 / 429 / 500 / 502 / 503 / 504 / 529 → `crates/llm-worker/src/llm_client/error.rs:80-89` の `is_retryable` で網羅。
|
||||
- `reqwest::Error::is_connect()` / `is_timeout()` → 同 `is_retryable` で対応。
|
||||
- それ以外 (`Json` / `Sse` / `Config` / 上記以外の `Api`) を弾く → 同所で明示的に `false`。
|
||||
- `is_retryable` を `error.rs` に置く要件 → 充足。
|
||||
- テーブル駆動 unit test → `error.rs:104-121` (retryable) / `:113-121` (non-retryable) / `:128-134` (Json/Sse/Config) で網羅。
|
||||
|
||||
### バックオフ (要件)
|
||||
- フルジッタ + 指数バックオフ → `crates/llm-worker/src/llm_client/retry.rs:40-47` で `min(base * 2^attempt, cap)` の上限から jitter で抽選。
|
||||
- `Retry-After` で上書き → `transport.rs:182-188` で取り出し、`:255` で `retry_after.unwrap_or_else(|| policy.backoff(...))` により上書き。
|
||||
- 上限: 最大試行回数 + 累積タイムアウトの両方 → `transport.rs:251-258`。`max_attempts` (default 4) と `total_timeout` (default 30s) で二段に打ち切り。
|
||||
|
||||
### ログ (要件)
|
||||
- リトライ発火ごとに `warn!` (status / attempt / wait) → `transport.rs:259-264`。`error = %err` で表示するためエラー本体に status が含まれる (`ClientError::Display` の "API error (status: N)")。`attempt = next_attempt` と `wait_ms` も出ており要件通り。
|
||||
|
||||
### 既存挙動の温存 (要件)
|
||||
- mid-stream `ClientError::Sse` 経路 (旧 `transport.rs:231` 相当) は `bytes_stream()` 以降にあり、retry loop の外。`tests/transport_retry_test.rs:196-227` で「mid-stream Sse は再送しない (受信回数 1)」を検証済み。
|
||||
- 成功パスの計測オーバヘッド: 成功時は loop 1 周のみで `break resp` → 余分な await/allocation なし。`headers.clone()` が増えるが retry 不要時にも走る。`HeaderMap` の clone は軽量だが厳密にはわずかな増分あり。問題視するレベルではない。
|
||||
|
||||
### 完了条件
|
||||
- `is_retryable` のテーブル駆動 unit test → ✓ (`error.rs` mod tests)
|
||||
- 503 / 529 / connect refused モックでリトライ + 上限到達 → ✓ (`transport_retry_test.rs:88-153`)
|
||||
- `Retry-After: 5` の指数上書き → 実装は「base/cap=1ms に絞った policy で `Retry-After: 1` を観察し、実時間で elapsed >= 1s を assert」(`transport_retry_test.rs:155-194`)。チケット文面の「5s / 仮想時間」とは異なるが、検証している性質 ("指数バックオフを `Retry-After` 値で上書きして待つ") は同一。後述。
|
||||
- mid-stream `Sse` で再送しない → ✓ (`:196-227`)
|
||||
- `cargo check` / `cargo test` → ✓ (実行確認済み: lib 8/8 pass、`transport_retry_test` 6/6 pass、warning は既存の `end_scope` のみで本 PR 由来でない)
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
|
||||
- 配置: `llm_client/retry.rs` 新設、`error.rs` への `is_retryable` 追加、`transport.rs` 内のリトライループ。すべて `llm-worker` の低レベル基盤に閉じており、上位層 (worker / pod) を巻き込んでいない。`MEMORY.md:llm-worker scope` に整合。
|
||||
- scheme 非依存性: `Scheme` trait に retry 関連の API を増やしていない。`HttpTransport<S>` の単一の retry loop で全 scheme をカバーする方針はチケット記載 ("scheme に依存しない共通処理") と一致。
|
||||
- 依存追加: `wiremock = "0.6.5"` が `[dev-dependencies]` に追加 (`Cargo.toml:28`)。`cargo add` 経由かは履歴から確認できないが、配置・バージョン指定とも妥当で `MEMORY.md:Use cargo add` の趣旨に反する痕跡は見えない。
|
||||
- API 表面: `HttpTransport::with_retry_policy` を新規 pub 公開。チケット範囲外 (manifest 化) に踏み込んでおらず、test 用 + 将来フックとして 1 メソッドのみ。過剰実装ではない。
|
||||
- 不要な抽象: なし。`RetryPolicy` は struct + `Default` + `backoff()` のみで、trait 化や builder 化に走っていない。`classify_error_response` は旧 inline ロジックの抽出で、retry loop が再利用するために必要な分解。
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### Blocking
|
||||
|
||||
なし。
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
|
||||
- **`Retry-After` 検証の方針差**: チケット文面は「5s / 仮想時間」、実装は「1s / 実時間」。検証している不変条件 ("`Retry-After` が指数バックオフより大きいときに `Retry-After` 値に従って待つ") は同等で、テスト所要時間も ~1s なので CI への影響も限定的。ただし将来 `tokio::time::pause()` を使う方針に揃えたくなったら、`reqwest`/`wiremock` を実 socket でなく `tower::Service` レベルに張り替える必要があり、軽い改修で済まない。本 PR の判断は妥当だが、ticket 文面とのズレはレビューレポートに残す価値あり。
|
||||
- **connect refused テストが retry 回数を assert していない** (`tests/transport_retry_test.rs:136-153`): 最終的に `Http` エラーが返ることだけ確認しているので、`fast_policy(3)` で実際に 3 回試行したかは間接確認のみ。retry loop 自体は wiremock の 503/529 ケースで覆われているので致命傷ではないが、`reqwest::Error::is_connect()` 経路を打つ唯一のテストでもあるので、「connect 失敗が retryable と判定されて確実に retry される」ことを示すために試行回数を観測したい。listener を立てて connection を drop する方式 (`std::net::TcpListener::bind` + `drop`) で観測できる。
|
||||
- **Retry-After の HTTP-date 非対応**: 仕様上は秒数のみ対応で OK (ユーザー合意済み)。Anthropic / Codex の実装が将来 HTTP-date 形式を返したらサイレントに無視されて指数バックオフに戻る (現状エラーにはならない)。後追いで気付きにくい挙動なので、`Retry-After` 形式の合意を別ドキュメント (この `.review.md` か後続 ticket) で残しておくと安全。
|
||||
- **Custom auth provider のヘッダ取得が retry loop の外** (`transport.rs:228` の `build_headers().await?`): 初回試行のヘッダを clone して使い回す。OAuth access token の有効期限が極端に短い場合、長め (>1 token TTL) の `total_timeout` で粘ると古いトークンを送り続ける可能性がある。現状の Codex OAuth は実質長寿命で問題ないが、retry 中に provider を再 invoke する設計に切り替えたいときの拡張余地として認識しておくと良い。今 ticket では範囲外。
|
||||
|
||||
### Nits
|
||||
|
||||
- `jitter_nanos` で `max_nanos + 1` のオーバーフロー: `cap = 10s` 想定なら届かないが、将来 cap を `u64::MAX` に近づけたら破綻する。`max_nanos.saturating_add(1)` または `wrapping_rem` 等への置き換えで防御可。実害なし。
|
||||
- `tokio::time::Instant` を import しているが、`std::time::Instant` でも実害はない (`tokio::time::sleep` は両方を扱える)。`tokio::time::pause()` 連動を見越した選択であれば、コメントで一行触れておくと意図が伝わりやすい。
|
||||
|
||||
## 判断
|
||||
|
||||
**Approve** — チケットの背景・要件・完了条件はすべて満たされており、コードベースの層構造・命名・抽象度を歪めていない。`Retry-After` テストの方針差は実装者が自己申告しているとおり等価な性質を検証しているため非ブロッキング。`with_retry_policy` の pub 化と manifest 送りの分離もチケット記載どおりで、過剰実装ではない。
|
||||
Loading…
Reference in New Issue
Block a user