fabi/PiCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(manager-core): visibility-timeout warn threshold tracks live env budget

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-06-11 17:36:46 +02:00
parent b0f7b72dd6
commit e33a551ad5
1 changed files with 72 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

95
crates/manager-core/src/triggers_api.rs
View File

@@ -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::<u32>() {
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<u32>) -> 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<u32>,
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<u32>) -> 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,