diff --git a/Cargo.lock b/Cargo.lock index bde1bc4..00e9541 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1104,7 +1104,7 @@ checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" [[package]] name = "postil-cli" -version = "0.8.23" +version = "0.8.24" dependencies = [ "aho-corasick", "anyhow", diff --git a/Cargo.toml b/Cargo.toml index a5b4324..27dc18f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "postil-cli" -version = "0.8.23" +version = "0.8.24" edition = "2024" description = "Postil: a low-noise AI review gate. Silent on clean PRs, hard gate on real risk." license = "Apache-2.0" diff --git a/src/llm.rs b/src/llm.rs index 7d97673..6c48f4f 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -2632,8 +2632,13 @@ impl LlmClient { review_deadline, total_deadline, )?; + // The slot bounds one model operation so a review's batch waves all fit + // inside the generator phase. Pinning it to the large-diff slot applies + // a many-wave bound to every hosted review: a single-wave review is cut + // at that slot, and because the slot then equals the phase remainder + // the timeout retry is unreachable, so one slow answer is terminal. client.review_model_timeout = Some(timeouts.request.min(Duration::from_secs( - crate::review::LARGE_DIFF_LLM_REQUEST_TIMEOUT_SECS, + crate::review::single_wave_request_timeout_secs(default_request_timeout.as_secs()), ))); Ok(client) } @@ -6710,7 +6715,7 @@ mod tests { assert_eq!( client.review_model_timeout, Some(Duration::from_secs( - crate::review::LARGE_DIFF_LLM_REQUEST_TIMEOUT_SECS + crate::review::SINGLE_WAVE_LLM_REQUEST_TIMEOUT_SECS )) ); let remaining = client.remaining_budget(LlmPhase::Total).unwrap().unwrap(); @@ -6719,6 +6724,70 @@ mod tests { assert!(remaining > Duration::from_secs(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS - 5)); } + /// A single-wave hosted review must get its whole generator phase for the + /// one model call it makes. Bounding it to the many-wave slot caps every + /// hosted review at that slot, and because the slot then equals the phase + /// remainder the timeout retry is unreachable, so one slow answer is + /// terminal. + #[test] + fn single_wave_hosted_reviews_keep_the_whole_generator_phase() { + let _lock = env_lock().lock().unwrap(); + let _env = EnvRestore::capture(&[REQUEST_TIMEOUT_ENV, TOTAL_TIMEOUT_ENV, "POSTIL_API_KEY"]); + EnvRestore::remove(TOTAL_TIMEOUT_ENV); + EnvRestore::set("POSTIL_API_KEY", "test-key"); + // The hosted worker raises the request timeout above both per-review + // defaults, so the slot has to come from the review's own choice. + EnvRestore::set(REQUEST_TIMEOUT_ENV, "420"); + + let config = Config::default(); + let review_budget = Duration::from_secs(crate::review::hosted_review_timeout_secs(&config)); + + let ordinary = LlmClient::from_env_for_remote_review( + &config, + Instant::now(), + Duration::from_secs(crate::review::HOSTED_LLM_REQUEST_TIMEOUT_SECS), + review_budget, + Duration::from_secs(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS), + ) + .unwrap(); + assert_eq!( + ordinary.review_model_timeout, + Some(Duration::from_secs( + crate::review::SINGLE_WAVE_LLM_REQUEST_TIMEOUT_SECS + )) + ); + assert!( + ordinary.review_model_timeout.unwrap() + > Duration::from_secs(crate::review::LARGE_DIFF_LLM_REQUEST_TIMEOUT_SECS), + "a one-wave review must not inherit the many-wave slot" + ); + // The schedule check runs once the diff is fetched, against whatever is + // left of the phase then, so the slot needs headroom under the reserve + // rather than to merely equal it. + assert!( + ordinary.review_model_timeout.unwrap() + < review_budget + - Duration::from_secs(crate::review::HOSTED_REVIEW_SCHEDULING_RESERVE_SECS), + "the slot must leave the generator phase room for its scheduling reserve" + ); + + let large = LlmClient::from_env_for_remote_review( + &config, + Instant::now(), + Duration::from_secs(crate::review::LARGE_DIFF_LLM_REQUEST_TIMEOUT_SECS), + review_budget, + Duration::from_secs(crate::review::HOSTED_LLM_TOTAL_TIMEOUT_SECS), + ) + .unwrap(); + assert_eq!( + large.review_model_timeout, + Some(Duration::from_secs( + crate::review::LARGE_DIFF_LLM_REQUEST_TIMEOUT_SECS + )), + "many-wave reviews still need the short slot so every wave fits" + ); + } + #[test] fn local_from_env_has_no_total_deadline_without_env_override() { let _lock = env_lock().lock().unwrap(); diff --git a/src/review.rs b/src/review.rs index 6598d40..a4839fc 100644 --- a/src/review.rs +++ b/src/review.rs @@ -64,6 +64,14 @@ pub(crate) const HOSTED_LLM_REQUEST_TIMEOUT_SECS: u64 = 240; /// bounded by this slot. Admission prices complete batch waves and sequential /// cascades against the review-phase deadline. pub(crate) const LARGE_DIFF_LLM_REQUEST_TIMEOUT_SECS: u64 = 60; +/// The slot a review that fits in a single batch wave gets for its one model +/// operation. The large-diff slot above is sized so many waves fit; spending it +/// on a single wave ends the review the first time the model answers slower +/// than a fraction of the phase it was given. This sits above the review +/// model's observed tail latency and below the generator phase less its +/// scheduling reserve, which `ensure_hosted_review_schedule` enforces against +/// the deadline that actually remains once the diff has been fetched. +pub(crate) const SINGLE_WAVE_LLM_REQUEST_TIMEOUT_SECS: u64 = 180; pub(crate) const HOSTED_LLM_REVIEW_TIMEOUT_SECS: u64 = 420; pub(crate) const HOSTED_REVIEW_SCHEDULING_RESERVE_SECS: u64 = 30; const FORGE_READ_TIMEOUT_SECS: u64 = 60; @@ -157,6 +165,16 @@ pub(crate) fn large_diff_batch_concurrency(cfg: &Config) -> usize { (MAX_LARGE_DIFF_CONCURRENCY / consensus_width).max(1) } +/// A review spread across many waves keeps the slot each wave was priced +/// against; anything else is a single wave and gets the single-wave slot. +pub(crate) fn single_wave_request_timeout_secs(review_request_timeout_secs: u64) -> u64 { + if review_request_timeout_secs <= LARGE_DIFF_LLM_REQUEST_TIMEOUT_SECS { + review_request_timeout_secs + } else { + SINGLE_WAVE_LLM_REQUEST_TIMEOUT_SECS.min(review_request_timeout_secs) + } +} + fn hosted_request_timeout_secs(deterministic_large_review: bool) -> u64 { if deterministic_large_review { LARGE_DIFF_LLM_REQUEST_TIMEOUT_SECS