fix(enabled): close disabled-script execution on the async paths (review)
Holistic Phase-1 review found the "a disabled script can't execute via any path" guarantee (§4.3) held only for the sync user-route, the execute-by-id bypass, and the trigger outbox arm — three paths still ran disabled scripts: - Queue arm: `list_active_queue_consumers` filtered the trigger's `enabled` but not the bound script's. Add `JOIN scripts … AND s.enabled = TRUE` so a disabled script's queue trigger stops consuming. - Async-HTTP (202) + queued invoke() arms: `build_http_request` / `build_invoke_request` hardcoded `active: true`. Set it to `script.enabled`, and MOVE the dispatcher's fire-time `active` drop to after the source-kind match so it covers all three arms uniformly (previously trigger-only). - `invoke()` (script-to-script): `resolve_id`/`resolve_name` checked cross-app isolation but not `enabled`. A disabled target now resolves to NotFound (indistinguishable from absent), matching the data plane. Also (review LOW/§4.7): - The route-on/script-off 404 returned `NotFound(script_id)`, leaking the internal id and distinguishable from absent. Return the same flat "no route matches" 404 as the unmatched case. - Add the §4.7 "enabled endpoint with no route and no trigger" reachability warning (was unimplemented; only disabled-target shipped). Tested: manager-core lib 368 + orchestrator 75 + 22 project-tool journeys (incl. a new enabled-route→disabled-script flat-404 e2e) green; clippy -D warnings clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -473,6 +473,9 @@ impl ApplyService {
|
||||
// deployed-but-unreachable. Not an error (it's valid desired state),
|
||||
// but surfaced so the operator isn't surprised by a silent 404.
|
||||
report.warnings.extend(disabled_target_warnings(bundle));
|
||||
report
|
||||
.warnings
|
||||
.extend(unreachable_endpoint_warnings(bundle));
|
||||
|
||||
let bundle_scripts: HashMap<String, &BundleScript> = bundle
|
||||
.scripts
|
||||
@@ -1426,6 +1429,31 @@ fn reject_reserved_path(path: &str) -> Result<(), ApplyError> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// §4.7 reachability warning: an *enabled* endpoint script with no route and
|
||||
/// no trigger has no event surface — it's only reachable via the
|
||||
/// execute-by-id bypass / `invoke()`. Modules are exempt (never invoked
|
||||
/// directly); disabled endpoints are intentionally inert, so skip them.
|
||||
fn unreachable_endpoint_warnings(bundle: &Bundle) -> Vec<String> {
|
||||
let bound: HashSet<&str> = bundle
|
||||
.routes
|
||||
.iter()
|
||||
.map(|r| r.script.as_str())
|
||||
.chain(bundle.triggers.iter().map(BundleTrigger::script))
|
||||
.collect();
|
||||
bundle
|
||||
.scripts
|
||||
.iter()
|
||||
.filter(|s| s.kind == ScriptKind::Endpoint && s.enabled && !bound.contains(s.name.as_str()))
|
||||
.map(|s| {
|
||||
format!(
|
||||
"endpoint `{}` has no route or trigger — only reachable via the \
|
||||
execute-by-id bypass / invoke()",
|
||||
s.name
|
||||
)
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// §4.7 reachability warnings: an *enabled* route or trigger bound to a
|
||||
/// script the manifest marks *disabled* is deployed but unreachable (the
|
||||
/// route 404s, the trigger won't fire). Valid desired state, so a warning —
|
||||
@@ -2179,6 +2207,37 @@ mod tests {
|
||||
assert!(disabled_target_warnings(&b).is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn warns_on_unreachable_endpoint() {
|
||||
// §4.7: an enabled endpoint with no route and no trigger is flagged.
|
||||
let mut b = empty_bundle();
|
||||
b.scripts = vec![bundle_script("orphan", "x")];
|
||||
let w = unreachable_endpoint_warnings(&b);
|
||||
assert!(
|
||||
w.iter().any(|m| m.contains("no route or trigger")),
|
||||
"expected an unreachable-endpoint warning: {w:?}"
|
||||
);
|
||||
// Bound by a route → no warning.
|
||||
b.routes = vec![BundleRoute {
|
||||
script: "orphan".into(),
|
||||
method: None,
|
||||
host_kind: HostKind::Any,
|
||||
host: String::new(),
|
||||
host_param_name: None,
|
||||
path_kind: PathKind::Exact,
|
||||
path: "/o".into(),
|
||||
dispatch_mode: DispatchMode::Sync,
|
||||
enabled: true,
|
||||
}];
|
||||
assert!(unreachable_endpoint_warnings(&b).is_empty());
|
||||
// A module is exempt even with no binding.
|
||||
let mut m = empty_bundle();
|
||||
let mut lib = bundle_script("lib", "x");
|
||||
lib.kind = ScriptKind::Module;
|
||||
m.scripts = vec![lib];
|
||||
assert!(unreachable_endpoint_warnings(&m).is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn trigger_diff_create_noop_delete() {
|
||||
let s = script("h", "x");
|
||||
|
||||
@@ -611,23 +611,6 @@ impl Dispatcher {
|
||||
| OutboxSourceKind::Pubsub
|
||||
| OutboxSourceKind::Email => {
|
||||
let resolved = self.resolve_trigger(&row).await?;
|
||||
// §4.3 fire-time re-check: if the trigger or its target script
|
||||
// was disabled after this row was enqueued, drop the row rather
|
||||
// than fire a stale event. Closes the outbox gap where the
|
||||
// match-time `enabled` check can't see a later toggle.
|
||||
if !resolved.active {
|
||||
tracing::debug!(
|
||||
outbox_id = %row.id,
|
||||
app_id = %row.app_id,
|
||||
"trigger or target script disabled since enqueue; dropping row"
|
||||
);
|
||||
self.outbox
|
||||
.delete(row.id)
|
||||
.await
|
||||
.map_err(|e| DispatcherError::Outbox(e.to_string()))?;
|
||||
drop(permit);
|
||||
return Ok(());
|
||||
}
|
||||
let req = match self.build_exec_request(&row, &resolved).await {
|
||||
Ok(req) => req,
|
||||
Err(err) => {
|
||||
@@ -644,6 +627,26 @@ impl Dispatcher {
|
||||
}
|
||||
};
|
||||
|
||||
// §4.3 fire-time re-check, for EVERY outbox source (trigger, async
|
||||
// HTTP/202, and invoke): if the target script (or, for triggers, the
|
||||
// trigger) was disabled after this row was enqueued, drop it rather
|
||||
// than fire a stale event. The match-time `enabled` check can't see a
|
||||
// later toggle, so this is the gate that makes a disabled script
|
||||
// genuinely non-invocable on the async paths too.
|
||||
if !resolved.active {
|
||||
tracing::debug!(
|
||||
outbox_id = %row.id,
|
||||
app_id = %row.app_id,
|
||||
"target script/trigger disabled since enqueue; dropping outbox row"
|
||||
);
|
||||
self.outbox
|
||||
.delete(row.id)
|
||||
.await
|
||||
.map_err(|e| DispatcherError::Outbox(e.to_string()))?;
|
||||
drop(permit);
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
// The gate permit auto-releases when this scope ends or when
|
||||
// the executor finishes. We hand control to the executor and
|
||||
// wait synchronously here — sync HTTP and dispatcher share the
|
||||
@@ -863,7 +866,9 @@ impl Dispatcher {
|
||||
let resolved = ResolvedTrigger {
|
||||
trigger_kind: TriggerKind::Kv, // placeholder; HTTP doesn't have a kind
|
||||
is_dead_letter_handler: false,
|
||||
active: true,
|
||||
// §4.3: an async-HTTP (202) row whose script was disabled after
|
||||
// enqueue is dropped at fire time by the post-match active check.
|
||||
active: script.enabled,
|
||||
script_id,
|
||||
script_source: script.source,
|
||||
script_name: payload.script_name,
|
||||
@@ -961,7 +966,9 @@ impl Dispatcher {
|
||||
let resolved = ResolvedTrigger {
|
||||
trigger_kind: TriggerKind::Cron, // placeholder; not used downstream
|
||||
is_dead_letter_handler: false,
|
||||
active: true,
|
||||
// §4.3: a queued invoke() whose target script was disabled after
|
||||
// enqueue is dropped at fire time by the post-match active check.
|
||||
active: script.enabled,
|
||||
script_id: script.id,
|
||||
script_source: script.source,
|
||||
script_name: script.name,
|
||||
|
||||
@@ -83,6 +83,11 @@ impl InvokeServiceImpl {
|
||||
if script.app_id != cx.app_id {
|
||||
return Err(InvokeError::CrossApp);
|
||||
}
|
||||
if !script.enabled {
|
||||
// §4.3: a disabled script is not invocable via any path. Surface as
|
||||
// NotFound (indistinguishable from absent), like the data plane.
|
||||
return Err(InvokeError::NotFound(format!("id {script_id}")));
|
||||
}
|
||||
Ok(ResolvedScript {
|
||||
script_id: script.id,
|
||||
app_id: script.app_id,
|
||||
@@ -103,6 +108,9 @@ impl InvokeServiceImpl {
|
||||
.await
|
||||
.map_err(|e| InvokeError::Backend(e.to_string()))?
|
||||
.ok_or_else(|| InvokeError::NotFound(format!("name {name:?}")))?;
|
||||
if !script.enabled {
|
||||
return Err(InvokeError::NotFound(format!("name {name:?}")));
|
||||
}
|
||||
Ok(ResolvedScript {
|
||||
script_id: script.id,
|
||||
app_id: script.app_id,
|
||||
|
||||
@@ -1532,7 +1532,8 @@ impl TriggerRepo for PostgresTriggerRepo {
|
||||
t.registered_by_principal \
|
||||
FROM triggers t \
|
||||
JOIN queue_trigger_details d ON d.trigger_id = t.id \
|
||||
WHERE t.kind = 'queue' AND t.enabled = TRUE",
|
||||
JOIN scripts s ON s.id = t.script_id \
|
||||
WHERE t.kind = 'queue' AND t.enabled = TRUE AND s.enabled = TRUE",
|
||||
)
|
||||
.fetch_all(&self.pool)
|
||||
.await?;
|
||||
|
||||
@@ -227,11 +227,18 @@ where
|
||||
.resolve(matched.matched.script_id)
|
||||
.await?
|
||||
.ok_or(ApiError::NotFound(matched.matched.script_id))?;
|
||||
// An enabled route bound to a disabled script (§4.3, §4.7) is
|
||||
// unreachable: 404 as if no route matched (the disabled route is already
|
||||
// dropped from the match table; this covers the route-on/script-off case).
|
||||
// An enabled route bound to a disabled script (§4.3, §4.7) is unreachable.
|
||||
// Return the SAME flat "no route matches" 404 as the unmatched case — not
|
||||
// `NotFound(script_id)`, which would both leak the internal script id to an
|
||||
// anonymous caller and be distinguishable from absent.
|
||||
if !script.enabled {
|
||||
return Err(ApiError::NotFound(matched.matched.script_id));
|
||||
return Ok((
|
||||
StatusCode::NOT_FOUND,
|
||||
Json(serde_json::json!({
|
||||
"error": format!("no route matches {method} {path}")
|
||||
})),
|
||||
)
|
||||
.into_response());
|
||||
}
|
||||
|
||||
// Drain the body now that we know we'll execute. 10 MiB cap matches
|
||||
|
||||
@@ -125,6 +125,55 @@ fn disabling_a_route_makes_it_404_then_reenable() {
|
||||
);
|
||||
}
|
||||
|
||||
#[ignore = "needs DATABASE_URL pointing at a running Postgres"]
|
||||
#[test]
|
||||
fn enabled_route_to_disabled_script_404s_flatly() {
|
||||
let Some(fx) = common::fixture_or_skip() else {
|
||||
return;
|
||||
};
|
||||
let env = common::admin_env(fx);
|
||||
let slug = common::unique_slug("enbl-os");
|
||||
common::pic_as(&env)
|
||||
.args(["apps", "create", &slug])
|
||||
.assert()
|
||||
.success();
|
||||
let _guard = AppGuard::new(&env.url, &env.token, &slug);
|
||||
let host = format!("{slug}.test");
|
||||
common::pic_as(&env)
|
||||
.args(["apps", "domains", "add", &slug, &host])
|
||||
.assert()
|
||||
.success();
|
||||
|
||||
let dir = TempDir::new().unwrap();
|
||||
fs::create_dir_all(dir.path().join("scripts")).unwrap();
|
||||
fs::write(dir.path().join("scripts/hello.rhai"), "\"hi\"").unwrap();
|
||||
let manifest_path = dir.path().join("picloud.toml");
|
||||
// Route stays enabled; the SCRIPT it binds is disabled (route-on/script-off).
|
||||
fs::write(
|
||||
&manifest_path,
|
||||
format!(
|
||||
"[app]\nslug = \"{slug}\"\nname = \"OS\"\n\n\
|
||||
[[scripts]]\nname = \"hello\"\nfile = \"scripts/hello.rhai\"\nenabled = false\n\n\
|
||||
[[routes]]\nscript = \"hello\"\nmethod = \"GET\"\n\
|
||||
host_kind = \"any\"\npath_kind = \"exact\"\npath = \"/hello\"\n"
|
||||
),
|
||||
)
|
||||
.unwrap();
|
||||
apply(&env, &manifest_path);
|
||||
|
||||
// 404, and the body must be the flat "no route matches" form — never the
|
||||
// internal script id (no info leak; indistinguishable from absent).
|
||||
let client = reqwest::blocking::Client::new();
|
||||
let resp = client
|
||||
.get(format!("{}/hello", env.url))
|
||||
.header(reqwest::header::HOST, &host)
|
||||
.send()
|
||||
.unwrap();
|
||||
assert_eq!(resp.status().as_u16(), 404);
|
||||
let body = resp.text().unwrap();
|
||||
assert!(body.contains("no route matches"), "flat 404 body: {body}");
|
||||
}
|
||||
|
||||
fn apply(env: &common::TestEnv, manifest_path: &Path) {
|
||||
common::pic_as(env)
|
||||
.args(["apply", "--file"])
|
||||
|
||||
Reference in New Issue
Block a user