fix(review): close 5 follow-up gaps from Stage 6 audit re-review
- F-T-003 actually closed: clients/typescript/src/subscribe.ts now bounds the 401-refresh loop at 3 consecutive refusals, surfaces an onError describing the loop, and resets on any successful stream open. New test covers the cap. The Stage 2 dashboard fix only addressed adminRequest in dashboard/src/lib/api.ts; the originally- cited TS client file was untouched. - users/invitations subtab dropped the redundant api.apps.get fetch the other 5 subtabs already shed in Stage 6. - Queue visibility-timeout validator pulled out as validate_queue_visibility_timeout, with two thresholds: hard-reject below MIN_QUEUE_VISIBILITY_TIMEOUT_SECS (30s — catches typos), warn- log between MIN and SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS (300s) so operators see when their visibility is below the dispatcher's per-message executor budget. Stage 6 only had the hard floor; the reviewer caught that a 60s handler still races a 30s visibility even after the floor. Four new unit tests cover none/above-safe/ between/below-min. - pic dead-letters count subcommand: cheap headless probe for unresolved DL totals, parallels the dashboard's badge query. - pic dead-letters replay now has a happy-path integration test (replay_against_real_dl_row_succeeds): inserts a synthetic DL row directly via the rust-postgres sync driver (added as dev-dep), drives `pic dead-letters replay`, asserts the row is resolved with reason=replayed and count drops back to 0. Plus a count smoke test. All 75 CLI integration tests + 16 TS client tests + 4 new visibility-timeout unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -34,12 +34,51 @@ use crate::trigger_repo::{
|
||||
TriggerRepo, TriggerRepoError,
|
||||
};
|
||||
|
||||
/// Minimum allowed queue visibility timeout. Anything shorter races the
|
||||
/// dispatcher's per-message executor budget; reclaim would re-deliver
|
||||
/// the message before the handler returned. Picked to outlast a slow
|
||||
/// handler by a comfortable margin.
|
||||
/// Hard floor on queue visibility timeout. Anything shorter is a typo
|
||||
/// in practice (the dispatcher itself ticks every 100ms; reclaim ticks
|
||||
/// every 30s; an executor needs more wall-clock than this to do useful
|
||||
/// 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;
|
||||
|
||||
/// 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.
|
||||
///
|
||||
/// 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> {
|
||||
let Some(s) = secs else {
|
||||
return Ok(()); // None → fall back to TriggerConfig's default.
|
||||
};
|
||||
if s < MIN_QUEUE_VISIBILITY_TIMEOUT_SECS {
|
||||
return Err(TriggersApiError::Invalid(format!(
|
||||
"visibility_timeout_secs must be >= {MIN_QUEUE_VISIBILITY_TIMEOUT_SECS} \
|
||||
(shorter than the dispatcher's tick + reclaim cadence; \
|
||||
a handler can't return before reclaim fires)"
|
||||
)));
|
||||
}
|
||||
if s < SAFE_VISIBILITY_VS_EXEC_BUDGET_SECS {
|
||||
tracing::warn!(
|
||||
visibility_timeout_secs = s,
|
||||
executor_budget_secs = SAFE_VISIBILITY_VS_EXEC_BUDGET_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 \
|
||||
accept the at-least-once semantics for this queue."
|
||||
);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
pub struct TriggersState {
|
||||
pub triggers: Arc<dyn TriggerRepo>,
|
||||
@@ -573,21 +612,7 @@ async fn create_queue_trigger(
|
||||
"queue_name must not be empty".into(),
|
||||
));
|
||||
}
|
||||
// Reject obviously-too-short visibility timeouts. The dispatcher
|
||||
// budgets up to ~5 minutes of executor wall-clock per message
|
||||
// (DEFAULT_ASYNC_EXEC_TIMEOUT in dispatcher.rs); a 10s visibility
|
||||
// timeout would race the executor and cause the reclaim task to
|
||||
// re-deliver the message before the first handler returned, so the
|
||||
// queue would silently double-deliver. The audit's Low finding.
|
||||
if let Some(secs) = input.visibility_timeout_secs {
|
||||
if secs < MIN_QUEUE_VISIBILITY_TIMEOUT_SECS {
|
||||
return Err(TriggersApiError::Invalid(format!(
|
||||
"visibility_timeout_secs must be >= {MIN_QUEUE_VISIBILITY_TIMEOUT_SECS} \
|
||||
(shorter than the dispatcher's per-message executor budget; \
|
||||
reclaim would race the handler)"
|
||||
)));
|
||||
}
|
||||
}
|
||||
validate_queue_visibility_timeout(input.visibility_timeout_secs)?;
|
||||
validate_trigger_target(&*s.scripts, app_id, input.script_id).await?;
|
||||
|
||||
let req = crate::trigger_repo::CreateQueueTrigger {
|
||||
@@ -736,6 +761,44 @@ mod tests {
|
||||
|
||||
use super::*;
|
||||
use crate::app_repo::{AppLookup, AppRepository};
|
||||
|
||||
#[test]
|
||||
fn visibility_timeout_none_passes() {
|
||||
assert!(validate_queue_visibility_timeout(None).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());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn visibility_timeout_between_min_and_safe_warns_but_passes() {
|
||||
// [30, 300): 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());
|
||||
}
|
||||
|
||||
#[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();
|
||||
match err {
|
||||
TriggersApiError::Invalid(msg) => {
|
||||
assert!(
|
||||
msg.contains(&MIN_QUEUE_VISIBILITY_TIMEOUT_SECS.to_string()),
|
||||
"error should cite the minimum: {msg}"
|
||||
);
|
||||
}
|
||||
other => panic!("expected Invalid for {too_short}s, got {other:?}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
use crate::trigger_repo::{
|
||||
CreateCronTrigger, CreateEmailTrigger, CreateFilesTrigger, CreatePubsubTrigger,
|
||||
DeadLetterTriggerMatch, DocsTriggerMatch, EmailInboundTarget, FilesTriggerMatch,
|
||||
|
||||
Reference in New Issue
Block a user