From e33a551ad5cd8b3719d712b56ca8afe9a19c31d2 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Thu, 11 Jun 2026 17:36:46 +0200 Subject: [PATCH] fix(manager-core): visibility-timeout warn threshold tracks live env budget Stage 6 follow-up Obs 1: the hard-coded SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS = 300 didn't follow PICLOUD_DISPATCHER_ASYNC_EXEC_TIMEOUT_SEC. A deploy that raised the executor budget produced false-positive warns; a deploy that lowered it missed real visibility races. Refactor validate_queue_visibility_timeout to take safe_limit_secs as an explicit parameter. The call site reads the live env-overridable value via safe_visibility_vs_exec_budget_secs() each call; unit tests inject the boundary directly without touching global env state. New test safe_limit_threshold_tracks_caller_value asserts both budget-raised and budget-lowered code paths. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/manager-core/src/triggers_api.rs | 95 +++++++++++++++++++------ 1 file changed, 72 insertions(+), 23 deletions(-) diff --git a/crates/manager-core/src/triggers_api.rs b/crates/manager-core/src/triggers_api.rs index f531494..dd94ea3 100644 --- a/crates/manager-core/src/triggers_api.rs +++ b/crates/manager-core/src/triggers_api.rs @@ -40,22 +40,41 @@ use crate::trigger_repo::{ /// work). Reject below this with a 422 + actionable error. const MIN_QUEUE_VISIBILITY_TIMEOUT_SECS: u32 = 30; -/// Soft warning ceiling: when the configured visibility is shorter than -/// the dispatcher's per-message executor budget (default 300s, see -/// `DEFAULT_ASYNC_EXEC_TIMEOUT` in dispatcher.rs), a handler that runs -/// for the full budget will be reclaimed mid-execution and the message -/// double-delivered. Don't reject — short handlers are a valid choice -/// — but log loudly so the operator can see the mismatch. -const SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS: u32 = 300; +/// Default soft-warning ceiling when `PICLOUD_DISPATCHER_ASYNC_EXEC_TIMEOUT_SEC` +/// is unset — matches `DEFAULT_ASYNC_EXEC_TIMEOUT` in dispatcher.rs. +/// Kept as a constant rather than imported so the validator stays +/// self-contained for unit tests. +const DEFAULT_SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS: u32 = 300; + +/// Read the dispatcher's per-message executor budget at validation +/// time so the warn-threshold tracks the live env-overridable value +/// (`PICLOUD_DISPATCHER_ASYNC_EXEC_TIMEOUT_SEC`). Without this, a deploy +/// that raises the budget produces false-positive warns and a deploy +/// that lowers it misses real races. +fn safe_visibility_vs_exec_budget_secs() -> u32 { + if let Ok(v) = std::env::var("PICLOUD_DISPATCHER_ASYNC_EXEC_TIMEOUT_SEC") { + if let Ok(n) = v.trim().parse::() { + if n > 0 { + return n; + } + } + } + DEFAULT_SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS +} /// Validate the script-supplied visibility timeout. Hard-rejects below /// [`MIN_QUEUE_VISIBILITY_TIMEOUT_SECS`]; warn-logs (does not reject) -/// when in `[MIN, SAFE)` because a handler running past `visibility_secs` -/// will get double-delivered when reclaim sweeps the stale claim. +/// when below `safe_limit_secs` — a handler running past +/// `visibility_secs` will get double-delivered when reclaim sweeps the +/// stale claim. /// -/// Extracted from the handler so it's unit-testable without spinning up -/// the full Axum harness. -fn validate_queue_visibility_timeout(secs: Option) -> Result<(), TriggersApiError> { +/// `safe_limit_secs` is parameterized so the call site reads the +/// live env-overridable executor budget (PICLOUD_DISPATCHER_ASYNC_EXEC_TIMEOUT_SEC) +/// and the tests can inject explicit boundaries without env-var races. +fn validate_queue_visibility_timeout( + secs: Option, + safe_limit_secs: u32, +) -> Result<(), TriggersApiError> { let Some(s) = secs else { return Ok(()); // None → fall back to TriggerConfig's default. }; @@ -66,10 +85,10 @@ fn validate_queue_visibility_timeout(secs: Option) -> Result<(), TriggersAp a handler can't return before reclaim fires)" ))); } - if s < SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS { + if s < safe_limit_secs { tracing::warn!( visibility_timeout_secs = s, - executor_budget_secs = SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS, + executor_budget_secs = safe_limit_secs, "queue trigger visibility_timeout_secs is below the dispatcher's per-message \ executor budget; a handler that runs longer than {s}s will be reclaimed \ mid-execution and the message double-delivered. Either raise visibility or \ @@ -612,7 +631,10 @@ async fn create_queue_trigger( "queue_name must not be empty".into(), )); } - validate_queue_visibility_timeout(input.visibility_timeout_secs)?; + validate_queue_visibility_timeout( + input.visibility_timeout_secs, + safe_visibility_vs_exec_budget_secs(), + )?; validate_trigger_target(&*s.scripts, app_id, input.script_id).await?; let req = crate::trigger_repo::CreateQueueTrigger { @@ -762,31 +784,38 @@ mod tests { use super::*; use crate::app_repo::{AppLookup, AppRepository}; + const TEST_SAFE_LIMIT: u32 = DEFAULT_SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS; + #[test] fn visibility_timeout_none_passes() { - assert!(validate_queue_visibility_timeout(None).is_ok()); + assert!(validate_queue_visibility_timeout(None, TEST_SAFE_LIMIT).is_ok()); } #[test] fn visibility_timeout_above_safe_passes() { - // ≥ 300s: safely above the executor budget, no warn. - assert!(validate_queue_visibility_timeout(Some(600)).is_ok()); - assert!(validate_queue_visibility_timeout(Some(300)).is_ok()); + // ≥ safe-limit: safely above the executor budget, no warn. + assert!(validate_queue_visibility_timeout(Some(600), TEST_SAFE_LIMIT).is_ok()); + assert!(validate_queue_visibility_timeout(Some(TEST_SAFE_LIMIT), TEST_SAFE_LIMIT).is_ok()); } #[test] fn visibility_timeout_between_min_and_safe_warns_but_passes() { - // [30, 300): valid but the operator should know. We assert the + // [MIN, safe): valid but the operator should know. We assert the // call succeeds; the warn-log emission is a side effect tracing // would surface in real ops — covered visually, not asserted. - assert!(validate_queue_visibility_timeout(Some(60)).is_ok()); - assert!(validate_queue_visibility_timeout(Some(MIN_QUEUE_VISIBILITY_TIMEOUT_SECS)).is_ok()); + assert!(validate_queue_visibility_timeout(Some(60), TEST_SAFE_LIMIT).is_ok()); + assert!(validate_queue_visibility_timeout( + Some(MIN_QUEUE_VISIBILITY_TIMEOUT_SECS), + TEST_SAFE_LIMIT + ) + .is_ok()); } #[test] fn visibility_timeout_below_min_rejected() { for too_short in [0u32, 1, 5, 10, MIN_QUEUE_VISIBILITY_TIMEOUT_SECS - 1] { - let err = validate_queue_visibility_timeout(Some(too_short)).unwrap_err(); + let err = + validate_queue_visibility_timeout(Some(too_short), TEST_SAFE_LIMIT).unwrap_err(); match err { TriggersApiError::Invalid(msg) => { assert!( @@ -799,6 +828,26 @@ mod tests { } } + /// The safe-limit threshold has to track the live env-overridable + /// executor budget — a deploy that raises + /// PICLOUD_DISPATCHER_ASYNC_EXEC_TIMEOUT_SEC must shift the warn + /// boundary with it; a deploy that lowers it must keep us warning + /// on the larger range. Tested via the explicit-parameter shape so + /// we don't touch global env state from tests. + #[test] + fn safe_limit_threshold_tracks_caller_value() { + // Operator raised the executor budget to 600s. A 400s + // visibility — previously safe under the 300s default — is + // now sub-budget and must warn (call succeeds, no err). + assert!(validate_queue_visibility_timeout(Some(400), 600).is_ok()); + // Same value with the default 300s safe-limit: above the + // threshold, also Ok but without the warn-log path. + assert!(validate_queue_visibility_timeout(Some(400), 300).is_ok()); + // Operator lowered the budget to 60s. A 90s visibility is + // above-safe, passes silently. + assert!(validate_queue_visibility_timeout(Some(90), 60).is_ok()); + } + use crate::trigger_repo::{ CreateCronTrigger, CreateEmailTrigger, CreateFilesTrigger, CreatePubsubTrigger, DeadLetterTriggerMatch, DocsTriggerMatch, EmailInboundTarget, FilesTriggerMatch,