From 868e3d49236f69017a13284accab9eed6a3a9b61 Mon Sep 17 00:00:00 2001 From: Mark <399551+mwiebe@users.noreply.github.com> Date: Fri, 25 Sep 2026 15:07:01 -0700 Subject: [PATCH 1/5] fix: close the resolved-value follow-ups from the #404 and #407 reviews Four follow-up items recorded after the create_job profile-contract change (#407), plus the findings of an independent audit of this changeset. Items refer to the design doc's follow-up list. Item 10 - eval_listcomp discarded a failing child's counters. Every child.evaluate/eval_node in the comprehension used `?`, returning before absorb_counters ran, so an iteration that spent memory and operations and then errored left the parent's counters untouched. Moot when the error propagates to the top, but an enclosing absorption site - an unresolved-test conditional or boolop swallowing a value error - resumed with the spend missing: measured, `[int('A' * 1000000) for x in [1]] if Session.Flag else []` reported peak_memory 512 after a 1 MB allocation. The child's counters (and the regex cache moved into it) are now handed back on every exit: the filter-error and non-bool-filter arms, the body error, and - via a Result> that is absorbed once before `?` - the result push's budget pre-check too, so the invariant holds without exemption. The loop variable's clone is still live at the push, so a push-time exceedance now reports it; two pinned diagnostics shift by exactly that clone's 64 bytes (16480 -> 16544; 134217824 -> 134217888) and their comments explain the composition. Five tests pin the five sites, each verified by mutation to fail when its own absorb is removed. Item 13 - contains_budget_error's sub-error recursion was mutation-survivable: no test drove a compound both-branches-fail error carrying a budget exceedance through a suppression site. Two tests: the compound propagates through a boolop (deleting the recursion fails it), and the control - a compound of two value errors - is still suppressed. Item 5 (second half) - the two check-symtab builders parsed script- level let bindings under different profiles: build_task_check_symtab with the caller's host profile (as pass 8 does), build_env_check_symtab via the public evaluate_let_bindings, which parses with the latest profile - so an environment let could accept syntax pass 8 refused, or a crate upgrade could silently widen what job creation accepts. Both now call one private evaluate_check_let_bindings: host profile, POSIX, the caller's budgets, one `script let binding '': ` diagnostic. The public evaluate_let_bindings is unchanged as the run-time entry point for openjd-sessions and openjd-for-js. Today no ExprExtension variants exist, so the fix is correctness by construction; the test pins what is observable: env and step-script let failures produce byte-identical diagnostics through create_job (fails pre-refactor - the env path used another message format). Two implicit decisions are now documented in job-creation.md: structurally malformed bindings are skipped at the check path because pass 8 rejects them at decode, and the check path's caret aligns to the bare expression where pass 8 and run time align to the full binding. Item 14 - four cleanups from the independent review of #407: the missing-extension check iterates the template profile's own extension set (exhaustive by construction) sorted for a deterministic message, pinned by a two-extension test; FsEval docs and validation.md pointed at a non-existent "Path Parameters" section (now preprocess_job_parameters); summary.md records the create_job context and limits summary passes; the sessions integration test uses default_validation_context() like every other caller. Verification: workspace clippy clean; expr/model/cli suites green; conformance 1139/1139. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> --- crates/openjd-expr/src/eval/evaluator.rs | 55 ++++++- .../tests/integration/test_memory.rs | 152 +++++++++++++++++- .../tests/integration/test_operation_limit.rs | 7 +- .../src/job/create_job/instantiate.rs | 92 ++++++----- crates/openjd-model/src/job/create_job/mod.rs | 10 +- .../validate_v2023_09/format_strings.rs | 5 +- .../test_job_creation_resolved_values.rs | 72 +++++++++ .../tests/integration/test_model_profile.rs | 32 ++++ .../integration/test_session_scenarios.rs | 15 +- specs/cli/summary.md | 3 + specs/expr/evaluator.md | 11 ++ specs/model/job-creation.md | 21 ++- specs/model/validation.md | 6 +- 13 files changed, 407 insertions(+), 74 deletions(-) diff --git a/crates/openjd-expr/src/eval/evaluator.rs b/crates/openjd-expr/src/eval/evaluator.rs index eefd6d09..4f79b356 100644 --- a/crates/openjd-expr/src/eval/evaluator.rs +++ b/crates/openjd-expr/src/eval/evaluator.rs @@ -1411,6 +1411,22 @@ impl<'a> Evaluator<'a> { self.operation_count = child.operation_count; } + /// Absorb a child's counters and then pass its result through — + /// on the error path too. A failing child has still spent memory + /// and operations in this evaluation; if the caller of this + /// comprehension absorbs the error (an unresolved-test conditional + /// or boolop absorbing a value error) and continues, the parent's + /// counters must include that spend or the budget under-meters + /// every subsequent absorbed failure. + fn absorb_and_pass( + &mut self, + child: &Evaluator, + result: Result, + ) -> Result { + self.absorb_counters(child); + result + } + /// Conclude a list comprehension whose result cannot be computed at /// this stage — because the iterable is unresolved, or because the /// filter evaluated to an unresolved condition on a concrete @@ -1435,7 +1451,8 @@ impl<'a> Evaluator<'a> { let mut child = self.child_evaluator(&combined); // Check filter clause type if present if let Some(if_clause) = if_clause { - let cond = child.evaluate(if_clause)?; + let cond = child.evaluate(if_clause); + let cond = self.absorb_and_pass(&child, cond)?; let cond_inner = unwrap_unresolved(&cond.expr_type()); let is_bool_compatible = cond_inner == ExprType::BOOL || cond_inner.code() == crate::types::TypeCode::Unresolved @@ -1454,8 +1471,8 @@ impl<'a> Evaluator<'a> { }); } } - let body_val = child.evaluate(&lc.elt)?; - self.absorb_counters(&child); + let body_val = child.evaluate(&lc.elt); + let body_val = self.absorb_and_pass(&child, body_val)?; let body_type = unwrap_unresolved(&body_val.expr_type()); self.track(ExprValue::unresolved(ExprType::list(body_type))) } @@ -1561,7 +1578,17 @@ impl<'a> Evaluator<'a> { child.regex_cache = std::mem::take(&mut self.regex_cache); let mut include = true; if let Some(if_clause) = gen.ifs.first() { - let cond = child.evaluate(if_clause)?; + let cond = match child.evaluate(if_clause) { + Ok(c) => c, + Err(e) => { + // The child's spend counts even though it + // failed (see `absorb_and_pass`); the regex + // cache moved into the child comes back too. + self.absorb_counters(&child); + self.regex_cache = child.regex_cache; + return Err(e); + } + }; if let ExprValue::Bool(b) = cond { include = b; } else if cond.is_unresolved() { @@ -1577,6 +1604,8 @@ impl<'a> Evaluator<'a> { filter_unresolved = true; break; } else { + self.absorb_counters(&child); + self.regex_cache = child.regex_cache; let err = ExpressionError::new(format!( "List comprehension filter must be a boolean, got {}", cond.expr_type() @@ -1588,12 +1617,22 @@ impl<'a> Evaluator<'a> { }); } } - if include { - let elt = child.eval_node(&lc.elt, elem_target.as_ref())?; - result.push(self, elt)?; - } + let elt = if include { + child.eval_node(&lc.elt, elem_target.as_ref()).map(Some) + } else { + Ok(None) + }; + // Hand the child's counters and the regex cache back before + // *any* exit below — the body error, the push's budget + // pre-check, or the normal end of the iteration. The loop + // variable's clone is still live at the push, so absorbing + // first also makes a push-time exceedance report the true + // usage. self.absorb_counters(&child); self.regex_cache = child.regex_cache; + if let Some(elt) = elt? { + result.push(self, elt)?; + } // Restore the iteration baseline: the child's tracked // transients (the loop-variable clone and any intermediates) // do not survive the iteration, and the result elements are diff --git a/crates/openjd-expr/tests/integration/test_memory.rs b/crates/openjd-expr/tests/integration/test_memory.rs index 10b6ab5f..a7357764 100644 --- a/crates/openjd-expr/tests/integration/test_memory.rs +++ b/crates/openjd-expr/tests/integration/test_memory.rs @@ -577,10 +577,12 @@ fn comprehension_over_range_memory_bounded_incrementally() { assert_eq!( e, [ - // 16480 = logical result bytes plus the Vec's projected - // post-doubling capacity slack: the check accounts for the - // buffer the push is about to allocate, not just elements. - "Expression memory usage (16480 bytes) exceeded limit (10000 bytes) + // 16544 = logical result bytes plus the Vec's projected + // post-doubling capacity slack (the check accounts for the + // buffer the push is about to allocate, not just elements) + // plus the 64-byte loop-variable clone, which is still live + // in the iteration's scope at the moment of the push. + "Expression memory usage (16544 bytes) exceeded limit (10000 bytes) ", " [x for x in range_expr('1-2000000')] ", @@ -984,3 +986,145 @@ fn boolop_does_not_reabsorb_budget_error_from_nested_conditional() { err.message() ); } + +/// `contains_budget_error` recurses into compound sub-errors. When +/// *both* branches of an unresolved-test conditional fail — one with a +/// budget exceedance, one with a value error — the conditional +/// produces a compound "Both branches fail" error whose own kind is not +/// a budget kind; only the recursion can see the budget exceedance +/// inside it. Wrapped in a boolop suppression site, that compound must +/// still propagate. (Without the recursion this test fails: the +/// compound is swallowed as a plain value error.) +#[test] +fn budget_error_inside_compound_both_branches_fail_error_propagates_through_boolop() { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + let err = ParsedExpression::new( + "Session.Flag or ('A' * 10000000 if Session.Flag else int('nope')) == 'x'", + ) + .and_then(|p| { + p.with_memory_limit(1024 * 1024) + .with_operation_limit(DEFAULT_OPERATION_LIMIT) + .evaluate_with_metrics(&[&st]) + }) + .expect_err("a compound error carrying a budget exceedance must propagate"); + let msg = err.message(); + assert!( + msg.contains("Both branches fail in the if/else:"), + "Got: {msg}" + ); + assert!(msg.contains("exceeded limit (1048576 bytes)"), "Got: {msg}"); + assert!(msg.contains("Cannot convert 'nope' to int"), "Got: {msg}"); +} + +/// Control: a compound both-branches-fail error with *no* budget +/// exceedance inside is still suppressed by the boolop. +#[test] +fn compound_both_branches_fail_without_budget_error_is_suppressed_by_boolop() { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + let result = + ParsedExpression::new("Session.Flag or (int('a') if Session.Flag else int('b')) == 7") + .and_then(|p| { + p.with_memory_limit(1024 * 1024) + .with_operation_limit(DEFAULT_OPERATION_LIMIT) + .evaluate_with_metrics(&[&st]) + }) + .expect("a compound value error must be suppressed"); + assert!(result.value.is_unresolved()); +} + +// ══════════════════════════════════════════════════════════════ +// A failing comprehension's spend is absorbed into the parent +// ══════════════════════════════════════════════════════════════ + +/// A comprehension iteration that fails has still spent the memory and +/// operations its filter/body consumed before failing. When an +/// unresolved-test conditional absorbs the failure and continues, the +/// parent's counters must include that spend — otherwise every absorbed +/// comprehension failure evaluates under-metered. Observable through +/// `peak_memory`: the failing iteration builds a 1 MB string before +/// erroring, and that high-water mark must survive the failure. +#[test] +fn failing_comprehension_spend_is_absorbed_by_enclosing_conditional() { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + // The body builds a 1 MB string and then fails converting it to int; + // the conditional absorbs the value error (Session.Flag might select + // the else branch at run time). + let result = ParsedExpression::new("[int('A' * 1000000) for x in [1]] if Session.Flag else []") + .and_then(|p| p.evaluate_with_metrics(&[&st])) + .expect("the conditional absorbs the if-branch's value error"); + assert!(result.value.is_unresolved()); + assert!( + result.peak_memory >= 1_000_000, + "the failed iteration's 1 MB allocation must be reflected in peak memory; got {}", + result.peak_memory + ); +} + +/// Shared shape for the four other absorption sites: an enclosing +/// unresolved-test conditional swallows the comprehension's value +/// error, and the 1 MB the failing site allocated before erroring must +/// survive into the parent's high-water mark. +fn assert_absorbed_spend(expr: &str) { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + st.set( + "Session.List", + ExprValue::unresolved(openjd_expr::ExprType::list(openjd_expr::ExprType::INT)), + ) + .unwrap(); + let result = ParsedExpression::new(expr) + .and_then(|p| p.evaluate_with_metrics(&[&st])) + .unwrap_or_else(|e| panic!("the conditional must absorb the value error for {expr}: {e}")); + assert!(result.value.is_unresolved()); + assert!( + result.peak_memory >= 1_000_000, + "{expr}: the failed site's 1 MB allocation must be reflected in peak memory; got {}", + result.peak_memory + ); +} + +/// Concrete loop, filter clause errors. +#[test] +fn failing_comprehension_filter_spend_is_absorbed() { + assert_absorbed_spend("[x for x in [1] if int('A' * 1000000) > 0] if Session.Flag else []"); +} + +/// Concrete loop, filter evaluates to a non-boolean (the type-error +/// arm) after spending. +#[test] +fn nonbool_comprehension_filter_spend_is_absorbed() { + assert_absorbed_spend("[x for x in [1] if 'A' * 1000000] if Session.Flag else []"); +} + +/// Unresolved-iterable path, filter clause errors. +#[test] +fn failing_unresolved_comprehension_filter_spend_is_absorbed() { + assert_absorbed_spend( + "[x for x in Session.List if int('A' * 1000000) > 0] if Session.Flag else []", + ); +} + +/// Unresolved-iterable path, body errors. +#[test] +fn failing_unresolved_comprehension_body_spend_is_absorbed() { + assert_absorbed_spend("[int('A' * 1000000) for x in Session.List] if Session.Flag else []"); +} diff --git a/crates/openjd-expr/tests/integration/test_operation_limit.rs b/crates/openjd-expr/tests/integration/test_operation_limit.rs index 670891fb..5571847c 100644 --- a/crates/openjd-expr/tests/integration/test_operation_limit.rs +++ b/crates/openjd-expr/tests/integration/test_operation_limit.rs @@ -884,9 +884,10 @@ fn comprehension_over_huge_range_bounded_lazily() { assert_eq!( e, [ - // 134217824 ~ 128 MiB: the Vec's projected post-doubling - // capacity, charged before the growth allocation happens. - "Expression memory usage (134217824 bytes) exceeded limit (100000000 bytes) + // 134217888 ~ 128 MiB: the Vec's projected post-doubling + // capacity, charged before the growth allocation happens, + // plus the 64-byte loop-variable clone live at the push. + "Expression memory usage (134217888 bytes) exceeded limit (100000000 bytes) ", " [x for x in range_expr('0-4611686018427387902')] ", diff --git a/crates/openjd-model/src/job/create_job/instantiate.rs b/crates/openjd-model/src/job/create_job/instantiate.rs index 311a2df9..8fdc246f 100644 --- a/crates/openjd-model/src/job/create_job/instantiate.rs +++ b/crates/openjd-model/src/job/create_job/instantiate.rs @@ -255,6 +255,58 @@ fn add_unresolved_session_symbols(symtab: &mut SymbolTable) -> Result<(), ModelE Ok(()) } +/// Evaluate a script's `let` bindings into a check symbol table, for +/// both check-symtab builders (step scripts and environments). +/// +/// Parses under the caller's host profile — the same profile pass 8 +/// parsed the bindings with — so syntax the profile does not enable is +/// refused here exactly as it was at template validation. (Parsing +/// with the latest profile instead would accept at job creation what +/// pass 8 refused, or vice versa after a crate upgrade.) Evaluates +/// under `PathFormat::Posix` like every other job-creation evaluation, +/// with the caller's budgets. A binding that fails to evaluate fails +/// job creation: its expression type-checked at pass 8 with everything +/// unresolved, so the failure comes from the real parameter values and +/// would deterministically recur in every session. +fn evaluate_check_let_bindings( + bindings: &[String], + check_symtab: &mut SymbolTable, + host_profile: &openjd_expr::ExprProfile, + budgets: super::EvalBudgets, +) -> Result<(), ModelError> { + let host_lib = openjd_expr::FunctionLibrary::for_profile(host_profile); + for binding in bindings { + let Some(eq_pos) = binding.find('=') else { + continue; + }; + let name = binding[..eq_pos].trim(); + let expr = binding[eq_pos + 1..].trim(); + if name.is_empty() || expr.is_empty() { + continue; + } + let parsed = openjd_expr::eval::ParsedExpression::with_profile(expr, host_profile) + .map_err(|e| { + ModelError::Expression(ExpressionError::new(format!( + "script let binding '{name}': {e}" + ))) + })?; + let val = budgeted( + parsed + .with_path_format(PathFormat::Posix) + .with_library(&host_lib), + budgets, + ) + .evaluate(&[check_symtab as &SymbolTable]) + .map_err(|e| { + ModelError::Expression(ExpressionError::new(format!( + "script let binding '{name}': {e}" + ))) + })?; + check_symtab.set(name, val)?; + } + Ok(()) +} + /// Build the check symbol table for a step script's carried-forward /// format strings (task scope): the step's symtab (concrete `Param.*` / /// `RawParam.*` / `Job.Name` / `Step.Name` / step-level `let` @@ -321,35 +373,7 @@ fn build_task_check_symtab( let host_profile = ctx .profile .to_expr_profile(openjd_expr::HostContext::Unresolved); - let host_lib = openjd_expr::FunctionLibrary::for_profile(&host_profile); - for binding in bindings { - if let Some(eq_pos) = binding.find('=') { - let name = binding[..eq_pos].trim(); - let expr = binding[eq_pos + 1..].trim(); - if !name.is_empty() && !expr.is_empty() { - let parsed = - openjd_expr::eval::ParsedExpression::with_profile(expr, &host_profile) - .map_err(|e| { - ModelError::Expression(ExpressionError::new(format!( - "script let binding '{name}': {e}" - ))) - })?; - let val = budgeted( - parsed - .with_path_format(PathFormat::Posix) - .with_library(&host_lib), - budgets, - ) - .evaluate(&[&check_symtab as &SymbolTable]) - .map_err(|e| { - ModelError::Expression(ExpressionError::new(format!( - "script let binding '{name}': {e}" - ))) - })?; - check_symtab.set(name, val)?; - } - } - } + evaluate_check_let_bindings(bindings, &mut check_symtab, &host_profile, budgets)?; } } @@ -394,15 +418,7 @@ pub(super) fn build_env_check_symtab( let host_profile = ctx .profile .to_expr_profile(openjd_expr::HostContext::Unresolved); - let host_lib = openjd_expr::FunctionLibrary::for_profile(&host_profile); - symtab = evaluate_let_bindings( - bindings, - &symtab, - Some(&host_lib), - PathFormat::Posix, - budgets.memory, - budgets.operations, - )?; + evaluate_check_let_bindings(bindings, &mut symtab, &host_profile, budgets)?; } } } diff --git a/crates/openjd-model/src/job/create_job/mod.rs b/crates/openjd-model/src/job/create_job/mod.rs index 7ba0ff73..6ced3282 100644 --- a/crates/openjd-model/src/job/create_job/mod.rs +++ b/crates/openjd-model/src/job/create_job/mod.rs @@ -112,11 +112,17 @@ pub fn create_job( ctx.profile.revision(), ))); } - let missing: Vec<&str> = crate::types::ModelExtension::ALL + // Iterate the template's own extension set rather than + // `ModelExtension::ALL`: exhaustive by construction, so a future + // variant omitted from `ALL` cannot escape the check. Sorted so the + // message is deterministic (the set is a HashSet). + let mut missing: Vec<&str> = template_profile + .extensions() .iter() - .filter(|e| template_profile.has_extension(**e) && !ctx.profile.has_extension(**e)) + .filter(|e| !ctx.profile.has_extension(**e)) .map(|e| e.as_str()) .collect(); + missing.sort_unstable(); if !missing.is_empty() { return Err(ModelError::Compatibility(format!( "create_job requires a context enabling every extension the template declares: \ diff --git a/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs b/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs index e6df2664..074f06ca 100644 --- a/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs +++ b/crates/openjd-model/src/template/validate_v2023_09/format_strings.rs @@ -827,8 +827,9 @@ fn check_resolved_constraint( /// Every evaluation runs under `PathFormat::Posix`: template validation /// and job creation happen outside host context, where the model keeps /// all paths POSIX (only `openjd-sessions` evaluates under -/// `PathFormat::host()` — see the Path Parameters section of -/// `specs/model/job-creation.md`). This keeps the two stages consistent +/// `PathFormat::host()` — see `preprocess_job_parameters` in +/// `specs/model/job-creation.md`, which explains why paths stay POSIX +/// until template evaluation on the host). This keeps the two stages consistent /// with each other and with the POSIX-format values `create_job` seeds /// into its check symbol tables, and makes validation outcomes /// independent of the OS running them. diff --git a/crates/openjd-model/tests/integration/test_job_creation_resolved_values.rs b/crates/openjd-model/tests/integration/test_job_creation_resolved_values.rs index 53f9cc7d..4b110140 100644 --- a/crates/openjd-model/tests/integration/test_job_creation_resolved_values.rs +++ b/crates/openjd-model/tests/integration/test_job_creation_resolved_values.rs @@ -669,3 +669,75 @@ fn listcomp_with_task_param_filter_over_bound_param_passes_create_job() { create_default(template, &[("Files", "a,b,c"), ("X", "x"), ("N", "1")]) .expect("an unresolved comprehension filter must not fail job creation"); } + +// ══════════════════════════════════════════════════════════════ +// Step-script and environment `let` bindings share one check path +// ══════════════════════════════════════════════════════════════ + +/// Both check-symtab builders evaluate `let` bindings through the same +/// helper: parsed under the caller's host profile (as pass 8 parsed +/// them — never the latest profile, which would accept syntax pass 8 +/// refused or vice versa after a crate upgrade), evaluated under POSIX +/// with the caller's budgets, and reported with one message format. A +/// value-dependent failure in an environment `let` is therefore +/// reported exactly like the same failure in a step-script `let`. +fn let_failure_message(template: &str, params: &[(&str, &str)]) -> String { + let msg = create_default(template, params).expect_err("expected the let binding to fail"); + // Isolate the let-binding diagnostic (path/context prefixes differ + // between the two scopes by design; the binding message must not). + let start = msg + .find("script let binding") + .unwrap_or_else(|| panic!("no let-binding diagnostic in:\n{msg}")); + msg[start..].to_string() +} + +#[test] +fn env_let_and_script_let_failures_report_identically_at_create_job() { + let script_let = r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "Test", + "parameterDefinitions": [ + {"name": "X", "type": "STRING"}, + {"name": "N", "type": "INT"} + ], + "steps": [{"name": "S", "script": { + "let": ["q = 1 / int(Param.X)"], + "actions": {"onRun": {"command": "echo", "args": ["{{ q }}"]}} + }}] + }"#; + let env_let = r#"{ + "specificationVersion": "jobtemplate-2023-09", + "extensions": ["EXPR"], + "name": "Test", + "parameterDefinitions": [ + {"name": "X", "type": "STRING"}, + {"name": "N", "type": "INT"} + ], + "jobEnvironments": [{"name": "Env", "script": { + "let": ["q = 1 / int(Param.X)"], + "actions": {"onEnter": {"command": "echo", "args": ["{{ q }}"]}} + }}], + "steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "echo"}}}}] + }"#; + let params = [("X", "0"), ("N", "1")]; + let from_script = let_failure_message(script_let, ¶ms); + let from_env = let_failure_message(env_let, ¶ms); + assert_eq!( + from_script, from_env, + "the two scopes must report identically" + ); + assert!( + from_env.starts_with("script let binding 'q': Division by zero"), + "Got:\n{from_env}" + ); + assert!( + from_env.contains(" 1 / int(Param.X)\n"), + "Got:\n{from_env}" + ); + assert!(from_env.contains(" ~~^~~~~~~~~~~~~~"), "Got:\n{from_env}"); + + // Control: a non-zero divisor passes both scopes. + create_default(script_let, &[("X", "2"), ("N", "1")]).expect("script let must pass"); + create_default(env_let, &[("X", "2"), ("N", "1")]).expect("env let must pass"); +} diff --git a/crates/openjd-model/tests/integration/test_model_profile.rs b/crates/openjd-model/tests/integration/test_model_profile.rs index 8fb0dc01..08e46df2 100644 --- a/crates/openjd-model/tests/integration/test_model_profile.rs +++ b/crates/openjd-model/tests/integration/test_model_profile.rs @@ -143,6 +143,38 @@ fn context_missing_declared_extension_is_rejected() { ); } +#[test] +fn context_missing_several_declared_extensions_lists_them_sorted() { + // The missing set is derived from the template's own extension set + // (a HashSet) and sorted, so the message is deterministic and + // exhaustive regardless of declaration order. + let tpl = yaml_val( + r#"{ + "specificationVersion": "jobtemplate-2023-09", + "name": "RenderJob", + "extensions": ["FEATURE_BUNDLE_1", "EXPR"], + "steps": [{"name": "S", "script": {"actions": {"onRun": {"command": "run"}}}}] + }"#, + ); + let jt = decode_job_template( + tpl, + Some(&["EXPR", "FEATURE_BUNDLE_1"]), + &CallerLimits::default(), + ) + .unwrap(); + let params = preprocess_posix_defaults(&jt); + + let ctx = ValidationContext::from_profile(ModelProfile::current()); + let err = create_job(&jt, ¶ms, &ctx).unwrap_err(); + assert_eq!( + err.to_string(), + "Compatibility error: create_job requires a context enabling every extension the \ + template declares: missing EXPR, FEATURE_BUNDLE_1. An application that does not \ + support an extension should reject the template at decode via its \ + supported-extensions list." + ); +} + // ─── Test D: latest() has all ModelExtension::ALL variants ────────────────── #[test] diff --git a/crates/openjd-sessions/tests/integration/test_session_scenarios.rs b/crates/openjd-sessions/tests/integration/test_session_scenarios.rs index 8c8e9afc..106223ae 100644 --- a/crates/openjd-sessions/tests/integration/test_session_scenarios.rs +++ b/crates/openjd-sessions/tests/integration/test_session_scenarios.rs @@ -206,20 +206,7 @@ async fn run_scenario(scenario_path: &Path) { .unwrap_or_else(|e| panic!("Failed to preprocess params for '{}': {e}", scenario.name)); // Create job - let ctx = { - let mut exts = std::collections::HashSet::new(); - if let Some(ext_list) = &job_template.extensions { - exts.extend( - ext_list - .iter() - .filter_map(|e| e.as_str().parse::().ok()), - ); - } - openjd_model::ValidationContext::with_extensions( - openjd_model::SpecificationRevision::V2023_09, - exts, - ) - }; + let ctx = job_template.default_validation_context(); let job = create_job(&job_template, &job_params, &ctx) .unwrap_or_else(|e| panic!("Failed to create job for '{}': {e}", scenario.name)); diff --git a/specs/cli/summary.md b/specs/cli/summary.md index a87d025b..f5fbc2c0 100644 --- a/specs/cli/summary.md +++ b/specs/cli/summary.md @@ -56,6 +56,9 @@ execute(args) ├── Resolve job_template_dir and current_working_dir ├── preprocess_job_parameters() → param_values ├── create_job() → Job + │ context: job_template.default_validation_context() with + │ common::caller_limits() layered on — the same limits the decode + │ ran under (see run.md § Session Configuration / check.md § Caller-Limits Policy) │ └── Dispatch on --step ├── Some(step_name) → output_step_summary(&job, step_name, output_format) diff --git a/specs/expr/evaluator.md b/specs/expr/evaluator.md index ffbbd5e2..049c9a37 100644 --- a/specs/expr/evaluator.md +++ b/specs/expr/evaluator.md @@ -415,6 +415,17 @@ Evaluates list comprehensions: `[expr for var in iterable if condition]`. A filter whose *type* can never be a boolean is still an error on both paths. - Operation count: +1 per iteration +- Each iteration's child evaluator hands its counters (memory + high-water mark, operation count) back to the parent on **every** + exit — including when the filter or body errors, and before the + result push's budget pre-check. A failing iteration has still spent + its memory and operations in this evaluation; if an enclosing + construct absorbs the error (an unresolved-test conditional or + boolop) and continues, the parent's counters must include that + spend, or every absorbed comprehension failure evaluates + under-metered. Absorbing before the push also means a push-time + exceedance reports the loop variable's clone, which is still live in + the iteration's scope at that moment. - Iterates lists without copying and symbolic ranges lazily - Pre-checks the growing result vector's values and projected capacity against the memory limit diff --git a/specs/model/job-creation.md b/specs/model/job-creation.md index 0d2305da..50fd16c0 100644 --- a/specs/model/job-creation.md +++ b/specs/model/job-creation.md @@ -253,7 +253,7 @@ time, with everything only a session can know left `Unresolved`: here, deterministically, rather than in every session). - **Session scope** (job and step environments): as above minus `Task.*`, plus this environment's `Env.File.*` (`Unresolved`) and its - script-level `let` bindings evaluated in via `evaluate_let_bindings`. + script-level `let` bindings evaluated in. An environment `let` binding that fails to evaluate fails job creation, under the same error policy as every other evaluation this stage performs (see below): the bindings only evaluate when the @@ -262,6 +262,25 @@ time, with everything only a session can know left `Unresolved`: the real parameter values and would deterministically recur in every session that enters the environment. +Both scopes evaluate their script-level `let` bindings through one +shared path: each binding is **parsed under the context's host +profile** — the same profile pass 8 parsed it with, never the latest +profile, so syntax the profile does not enable is refused here exactly +as at template validation (and a crate upgrade cannot make job creation +accept what pass 8 refused, or vice versa) — then evaluated under +`PathFormat::Posix` with the caller's budgets. Failures from either +scope carry the same `script let binding '': ` diagnostic, +with the caret aligned to the bare expression (pass 8 and the run-time +path align it to the full `name = expr` binding string; the check path +reports the expression alone because its message already names the +binding). Structurally malformed bindings — no `=`, empty name, empty +expression — are skipped rather than reported: pass 8 rejects all three +at decode, so they cannot reach a template that came through +`decode_job_template`, and a hand-built `JobTemplate` that bypassed +decode still fails on them at run time. The public +`evaluate_let_bindings` (below) is the run-time entry point used by +`openjd-sessions` and `openjd-for-js`; the check symtabs do not use it. + Failures are `ModelError::ModelValidation` at the same field paths pass 8 uses, e.g. `steps[0] -> script -> actions -> onRun -> args[0]:` / diff --git a/specs/model/validation.md b/specs/model/validation.md index fdcf494f..a657b1b4 100644 --- a/specs/model/validation.md +++ b/specs/model/validation.md @@ -243,8 +243,10 @@ right `ExprProfile` from a model profile. Every evaluation this pass performs — format-string expressions and `let` bindings alike — runs under `PathFormat::Posix`. Template validation happens outside host context, where the model keeps all -paths POSIX (see the path-parameters discussion in -`specs/model/job-creation.md`); only `openjd-sessions` evaluates under +paths POSIX (see `preprocess_job_parameters` in +`specs/model/job-creation.md`, which explains why paths stay POSIX +until template evaluation on the host); only `openjd-sessions` +evaluates under `PathFormat::host()`. This keeps pass 8 consistent with job creation's re-checks (which read POSIX-format values out of their check symbol tables) and makes validation outcomes independent of the OS running From 5da1bf7c57fb6389b41da486f297ee841b29d91b Mon Sep 17 00:00:00 2001 From: Mark <399551+mwiebe@users.noreply.github.com> Date: Mon, 28 Sep 2026 09:46:36 -0700 Subject: [PATCH 2/5] fix(expr): reset the live footprint when absorbing a failed iteration's spend Review finding on this PR, verified: absorb_counters copies all three counters, but only peak_memory and operation_count are cumulative spend - current_memory is the live footprint. The new error arms absorbed all three and returned, so after an enclosing unresolved-test conditional or boolop swallowed the error and kept evaluating on the same evaluator, current_memory still carried the failed iteration's transients: the loop-variable clone and the intermediates of the failed sub-expression, none of which are live. Measured with a binop failure (dispatch releases its inputs only on success, so the 1 MB string stays tracked when `'A' * 1000000 - 1` fails): len(['A' * 1000000 - 1 for x in [1]] if Session.Flag else []) + len('B' * 600000) # limit 1.5 MB passed on main (peak 600576) and failed on this branch with "memory usage (1600776 bytes) exceeded limit" - charging the later 600 KB allocation for a megabyte that was gone. The original under-metering bug and this over-metering one are the two halves of the same mistake: treating spend and footprint as one quantity. The error exits now use absorb_spend_and_reset, which absorbs peak/ops and resets current_memory to the pre-iteration baseline - exactly what the unresolved-filter break arm already did. eval_listcomp_unresolved captures its own baseline and resets on every exit, including success: nothing it tracks (a placeholder loop variable, filter/body values evaluated only for their types) survives the call, so the unconditional carry-forward there was a smaller pre-existing version of the same leak. The success path of the concrete loop is unchanged - the footprint is absorbed before the push (the clone is live at that moment) and reset afterwards. Four tests pin it, one per absorption site: each fails on the PR head with the 1.6 MB exceedance and passes now, while still asserting the failed iteration's 1 MB reaches peak_memory - the two properties do not trade off. The spec's ListComp section states the spend/footprint distinction. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> --- crates/openjd-expr/src/eval/evaluator.rs | 83 ++++++++++++------- .../tests/integration/test_memory.rs | 63 ++++++++++++++ specs/expr/evaluator.md | 28 ++++--- 3 files changed, 134 insertions(+), 40 deletions(-) diff --git a/crates/openjd-expr/src/eval/evaluator.rs b/crates/openjd-expr/src/eval/evaluator.rs index 4f79b356..f7977117 100644 --- a/crates/openjd-expr/src/eval/evaluator.rs +++ b/crates/openjd-expr/src/eval/evaluator.rs @@ -1411,20 +1411,25 @@ impl<'a> Evaluator<'a> { self.operation_count = child.operation_count; } - /// Absorb a child's counters and then pass its result through — - /// on the error path too. A failing child has still spent memory - /// and operations in this evaluation; if the caller of this - /// comprehension absorbs the error (an unresolved-test conditional - /// or boolop absorbing a value error) and continues, the parent's - /// counters must include that spend or the budget under-meters - /// every subsequent absorbed failure. - fn absorb_and_pass( - &mut self, - child: &Evaluator, - result: Result, - ) -> Result { - self.absorb_counters(child); - result + /// Absorb a child's *spend* — peak memory and operation count — + /// while rolling the *live footprint* back to `baseline`. For the + /// exits where the child's values do not survive: the failing + /// filter/body of a comprehension iteration, or an iteration whose + /// result is abandoned. A failing child has still spent memory and + /// operations in this evaluation; if an enclosing construct absorbs + /// the error (an unresolved-test conditional or boolop) and + /// continues, the parent's peak and op count must include that + /// spend or every absorbed failure evaluates under-metered. But the + /// child's *tracked* values — the loop-variable clone, the + /// intermediates of the failed sub-expression — are dropped with + /// it, so `current_memory` must not carry them forward: the same + /// enclosing construct keeps evaluating on this evaluator, and a + /// stale footprint would charge every later allocation for memory + /// that is no longer live. + fn absorb_spend_and_reset(&mut self, child: &Evaluator, baseline: usize) { + self.peak_memory = child.peak_memory; + self.operation_count = child.operation_count; + self.current_memory = baseline; } /// Conclude a list comprehension whose result cannot be computed at @@ -1443,6 +1448,10 @@ impl<'a> Evaluator<'a> { var_name: &str, elem_type: ExprType, ) -> Result { + // Nothing the child tracks survives this function: the loop + // variable is a placeholder and the filter/body values are + // evaluated only for their types. Every exit resets to here. + let memory_baseline = self.current_memory; let mut tmp = crate::symbol_table::SymbolTable::new(); tmp.set(var_name, ExprValue::unresolved(elem_type)) .map_err(|e| ExpressionError::new(e.to_string()))?; @@ -1452,7 +1461,13 @@ impl<'a> Evaluator<'a> { // Check filter clause type if present if let Some(if_clause) = if_clause { let cond = child.evaluate(if_clause); - let cond = self.absorb_and_pass(&child, cond)?; + let cond = match cond { + Ok(c) => c, + Err(e) => { + self.absorb_spend_and_reset(&child, memory_baseline); + return Err(e); + } + }; let cond_inner = unwrap_unresolved(&cond.expr_type()); let is_bool_compatible = cond_inner == ExprType::BOOL || cond_inner.code() == crate::types::TypeCode::Unresolved @@ -1460,6 +1475,7 @@ impl<'a> Evaluator<'a> { || (cond_inner.code() == crate::types::TypeCode::Union && cond_inner.params().contains(&ExprType::BOOL)); if !is_bool_compatible { + self.absorb_spend_and_reset(&child, memory_baseline); let err = ExpressionError::new(format!( "List comprehension filter must be a boolean, got {}", cond_inner @@ -1472,7 +1488,8 @@ impl<'a> Evaluator<'a> { } } let body_val = child.evaluate(&lc.elt); - let body_val = self.absorb_and_pass(&child, body_val)?; + self.absorb_spend_and_reset(&child, memory_baseline); + let body_val = body_val?; let body_type = unwrap_unresolved(&body_val.expr_type()); self.track(ExprValue::unresolved(ExprType::list(body_type))) } @@ -1582,9 +1599,10 @@ impl<'a> Evaluator<'a> { Ok(c) => c, Err(e) => { // The child's spend counts even though it - // failed (see `absorb_and_pass`); the regex - // cache moved into the child comes back too. - self.absorb_counters(&child); + // failed, but its tracked values are dropped + // with it (see `absorb_spend_and_reset`); the + // regex cache moved into the child comes back. + self.absorb_spend_and_reset(&child, memory_baseline); self.regex_cache = child.regex_cache; return Err(e); } @@ -1598,13 +1616,12 @@ impl<'a> Evaluator<'a> { // only pre-checks the budget; the finished list // would be tracked by make_list_checked, which is // never reached. - self.absorb_counters(&child); + self.absorb_spend_and_reset(&child, memory_baseline); self.regex_cache = child.regex_cache; - self.current_memory = memory_baseline; filter_unresolved = true; break; } else { - self.absorb_counters(&child); + self.absorb_spend_and_reset(&child, memory_baseline); self.regex_cache = child.regex_cache; let err = ExpressionError::new(format!( "List comprehension filter must be a boolean, got {}", @@ -1618,19 +1635,25 @@ impl<'a> Evaluator<'a> { } } let elt = if include { - child.eval_node(&lc.elt, elem_target.as_ref()).map(Some) + match child.eval_node(&lc.elt, elem_target.as_ref()) { + Ok(v) => Some(v), + Err(e) => { + self.absorb_spend_and_reset(&child, memory_baseline); + self.regex_cache = child.regex_cache; + return Err(e); + } + } } else { - Ok(None) + None }; // Hand the child's counters and the regex cache back before - // *any* exit below — the body error, the push's budget - // pre-check, or the normal end of the iteration. The loop - // variable's clone is still live at the push, so absorbing - // first also makes a push-time exceedance report the true - // usage. + // the push's budget pre-check and the normal end of the + // iteration. The loop variable's clone is still live at the + // push, so absorbing first makes a push-time exceedance + // report the true usage. self.absorb_counters(&child); self.regex_cache = child.regex_cache; - if let Some(elt) = elt? { + if let Some(elt) = elt { result.push(self, elt)?; } // Restore the iteration baseline: the child's tracked diff --git a/crates/openjd-expr/tests/integration/test_memory.rs b/crates/openjd-expr/tests/integration/test_memory.rs index a7357764..95d26b5d 100644 --- a/crates/openjd-expr/tests/integration/test_memory.rs +++ b/crates/openjd-expr/tests/integration/test_memory.rs @@ -1128,3 +1128,66 @@ fn failing_unresolved_comprehension_filter_spend_is_absorbed() { fn failing_unresolved_comprehension_body_spend_is_absorbed() { assert_absorbed_spend("[int('A' * 1000000) for x in Session.List] if Session.Flag else []"); } + +// ══════════════════════════════════════════════════════════════ +// An absorbed comprehension failure leaves no live footprint behind +// ══════════════════════════════════════════════════════════════ + +/// Absorbing a failing iteration's *spend* (peak, ops) must not also +/// carry its *live footprint* forward. The child's tracked values — the +/// loop-variable clone and the failed sub-expression's intermediates — +/// are dropped with it, and the enclosing construct that absorbs the +/// error keeps evaluating on this evaluator. `'A' * 1000000 - 1` fails +/// in the binop with the 1 MB string still tracked (dispatch releases +/// inputs only on success), so a stale footprint would charge the +/// later 600 KB allocation as 1.6 MB against a 1.5 MB limit. +fn assert_no_stale_footprint(expr: &str) { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + st.set( + "Session.List", + ExprValue::unresolved(openjd_expr::ExprType::list(openjd_expr::ExprType::INT)), + ) + .unwrap(); + let result = ParsedExpression::new(expr) + .and_then(|p| p.with_memory_limit(1_500_000).evaluate_with_metrics(&[&st])) + .unwrap_or_else(|e| panic!("{expr}: the dropped footprint must not be charged: {e}")); + // Peak still reflects the failed iteration's genuine 1 MB spend. + assert!( + result.peak_memory >= 1_000_000, + "{expr}: peak must include the failed iteration's spend; got {}", + result.peak_memory + ); +} + +#[test] +fn absorbed_comprehension_body_failure_leaves_no_stale_footprint() { + assert_no_stale_footprint( + "len(['A' * 1000000 - 1 for x in [1]] if Session.Flag else []) + len('B' * 600000)", + ); +} + +#[test] +fn absorbed_comprehension_filter_failure_leaves_no_stale_footprint() { + assert_no_stale_footprint( + "len([x for x in [1] if 'A' * 1000000 - 1] if Session.Flag else []) + len('B' * 600000)", + ); +} + +#[test] +fn absorbed_unresolved_comprehension_body_failure_leaves_no_stale_footprint() { + assert_no_stale_footprint( + "len(['A' * 1000000 - 1 for x in Session.List] if Session.Flag else []) + len('B' * 600000)", + ); +} + +#[test] +fn absorbed_unresolved_comprehension_filter_failure_leaves_no_stale_footprint() { + assert_no_stale_footprint( + "len([x for x in Session.List if 'A' * 1000000 - 1] if Session.Flag else []) + len('B' * 600000)", + ); +} diff --git a/specs/expr/evaluator.md b/specs/expr/evaluator.md index 049c9a37..c7e54770 100644 --- a/specs/expr/evaluator.md +++ b/specs/expr/evaluator.md @@ -415,17 +415,25 @@ Evaluates list comprehensions: `[expr for var in iterable if condition]`. A filter whose *type* can never be a boolean is still an error on both paths. - Operation count: +1 per iteration -- Each iteration's child evaluator hands its counters (memory - high-water mark, operation count) back to the parent on **every** - exit — including when the filter or body errors, and before the - result push's budget pre-check. A failing iteration has still spent - its memory and operations in this evaluation; if an enclosing - construct absorbs the error (an unresolved-test conditional or - boolop) and continues, the parent's counters must include that +- Each iteration's child evaluator hands its *spend* — memory + high-water mark and operation count — back to the parent on **every** + exit, including when the filter or body errors. A failing iteration + has still spent its memory and operations in this evaluation; if an + enclosing construct absorbs the error (an unresolved-test conditional + or boolop) and continues, the parent's counters must include that spend, or every absorbed comprehension failure evaluates - under-metered. Absorbing before the push also means a push-time - exceedance reports the loop variable's clone, which is still live in - the iteration's scope at that moment. + under-metered. The child's *live footprint* is handled differently: + on any exit where the child's values are dropped (a failing + filter/body, an abandoned iteration, and every exit of the + unresolved-iterable path) the parent's `current_memory` is reset to + its pre-iteration baseline rather than absorbed — the loop-variable + clone and the failed sub-expression's intermediates are not live + after the exit, and the absorbing construct keeps evaluating on this + evaluator, so a stale footprint would charge every later allocation + for memory that is gone. On the success path the footprint *is* + absorbed before the result push (the loop variable's clone is live at + that moment, so a push-time exceedance reports it) and reset to the + baseline afterwards. - Iterates lists without copying and symbolic ranges lazily - Pre-checks the growing result vector's values and projected capacity against the memory limit From 9873e7cf80d3377975d5bf65275d4464f5c274ad Mon Sep 17 00:00:00 2001 From: Mark <399551+mwiebe@users.noreply.github.com> Date: Mon, 28 Sep 2026 10:29:29 -0700 Subject: [PATCH 3/5] fix(expr): stop double-charging the pushed element at the comprehension pre-check Review finding on this PR, verified and correcting an earlier commit's claim. 63d8c37 moved the child's counter absorb ahead of result.push, attributing the resulting 64-byte shift in two pinned diagnostics to "the loop variable's clone, still live at the push". That attribution was wrong: the loop variable's slot in the temp symbol table is never tracked (tmp.set does not go through track). What the child tracks is the clone eval_name returns when the body reads `x` - i.e. the element itself, the very value about to be pushed. Absorbing that footprint before the push made BudgetedVec's pre-check count the element twice: once in the parent's current_memory, once in its own value_bytes. For an Int that is 64 bytes. For a body producing large values it is a full element per push: measured, `['A' * 100000 for x in range(5)]` needs a ~501 KB limit on main and ~601 KB on the PR head - the effective limit shrank by one element. Over-metering, the opposite of what this PR set out to fix. Every exit from an iteration now uses absorb_spend_and_reset: peak and op count come back from the child, current_memory resets to the pre-iteration baseline, and the push pre-check sees baseline + value_bytes + slack - each element charged exactly once, by BudgetedVec. The trailing baseline restore is gone (the reset already happened), absorb_counters is deleted as unused, and the two pinned diagnostics return to their original values with comments that say what the number actually is. A new test pins the scaling: five 100 KB elements fit under 560 KB (fails on the PR head at 601152). The spec paragraph is corrected to match; it no longer names a value that was not being charged. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> --- crates/openjd-expr/src/eval/evaluator.rs | 58 ++++++++----------- .../tests/integration/test_memory.rs | 36 ++++++++++-- .../tests/integration/test_operation_limit.rs | 7 +-- specs/expr/evaluator.md | 25 ++++---- 4 files changed, 71 insertions(+), 55 deletions(-) diff --git a/crates/openjd-expr/src/eval/evaluator.rs b/crates/openjd-expr/src/eval/evaluator.rs index f7977117..4864962f 100644 --- a/crates/openjd-expr/src/eval/evaluator.rs +++ b/crates/openjd-expr/src/eval/evaluator.rs @@ -1404,28 +1404,21 @@ impl<'a> Evaluator<'a> { } } - /// Propagate resource counters back from a child evaluator. - fn absorb_counters(&mut self, child: &Evaluator) { - self.current_memory = child.current_memory; - self.peak_memory = child.peak_memory; - self.operation_count = child.operation_count; - } - /// Absorb a child's *spend* — peak memory and operation count — - /// while rolling the *live footprint* back to `baseline`. For the - /// exits where the child's values do not survive: the failing - /// filter/body of a comprehension iteration, or an iteration whose - /// result is abandoned. A failing child has still spent memory and - /// operations in this evaluation; if an enclosing construct absorbs - /// the error (an unresolved-test conditional or boolop) and - /// continues, the parent's peak and op count must include that - /// spend or every absorbed failure evaluates under-metered. But the - /// child's *tracked* values — the loop-variable clone, the - /// intermediates of the failed sub-expression — are dropped with - /// it, so `current_memory` must not carry them forward: the same - /// enclosing construct keeps evaluating on this evaluator, and a - /// stale footprint would charge every later allocation for memory - /// that is no longer live. + /// while resetting the *live footprint* to `baseline`. Every exit + /// from a comprehension iteration goes through this: the child's + /// tracked values never survive it. A child that spent memory and + /// operations has spent them whether or not it succeeded; if an + /// enclosing construct absorbs a failure (an unresolved-test + /// conditional or boolop) and continues, the parent's peak and op + /// count must include that spend or every absorbed failure evaluates + /// under-metered. But the child's *tracked* values — the body's + /// intermediates and result, the failed sub-expression's operands — + /// are dropped with it (a pushed element is charged separately, once, + /// by `BudgetedVec`), so `current_memory` must not carry them + /// forward: the enclosing construct keeps evaluating on this + /// evaluator, and a stale footprint would charge every later + /// allocation for memory that is not live. fn absorb_spend_and_reset(&mut self, child: &Evaluator, baseline: usize) { self.peak_memory = child.peak_memory; self.operation_count = child.operation_count; @@ -1646,22 +1639,21 @@ impl<'a> Evaluator<'a> { } else { None }; - // Hand the child's counters and the regex cache back before - // the push's budget pre-check and the normal end of the - // iteration. The loop variable's clone is still live at the - // push, so absorbing first makes a push-time exceedance - // report the true usage. - self.absorb_counters(&child); + // Hand the child's spend and the regex cache back before the + // push's budget pre-check, so an exit there leaves the + // parent's peak/op counters current. The footprint resets to + // the iteration baseline: the child's tracked values — the + // body's intermediates and its result (`eval_name` tracks + // the clone it returns for `x`; the loop variable's own slot + // in the temp symtab is never tracked) — do not survive the + // iteration, and the element about to be pushed is charged + // once, by BudgetedVec's pre-check, not again through the + // child's footprint. + self.absorb_spend_and_reset(&child, memory_baseline); self.regex_cache = child.regex_cache; if let Some(elt) = elt { result.push(self, elt)?; } - // Restore the iteration baseline: the child's tracked - // transients (the loop-variable clone and any intermediates) - // do not survive the iteration, and the result elements are - // accounted separately by BudgetedVec. Peak memory keeps - // the high-water mark absorbed above. - self.current_memory = memory_baseline; } // The iterable is consumed by the comprehension: release its // tracked memory now that iteration is done (the borrowing diff --git a/crates/openjd-expr/tests/integration/test_memory.rs b/crates/openjd-expr/tests/integration/test_memory.rs index 95d26b5d..5119747e 100644 --- a/crates/openjd-expr/tests/integration/test_memory.rs +++ b/crates/openjd-expr/tests/integration/test_memory.rs @@ -577,12 +577,14 @@ fn comprehension_over_range_memory_bounded_incrementally() { assert_eq!( e, [ - // 16544 = logical result bytes plus the Vec's projected - // post-doubling capacity slack (the check accounts for the - // buffer the push is about to allocate, not just elements) - // plus the 64-byte loop-variable clone, which is still live - // in the iteration's scope at the moment of the push. - "Expression memory usage (16544 bytes) exceeded limit (10000 bytes) + // 16480 = logical result bytes (including the element being + // pushed, charged once by BudgetedVec) plus the Vec's + // projected post-doubling capacity slack: the check accounts + // for the buffer the push is about to allocate, not just + // elements. The iteration's own transients are not in the + // figure — the footprint is reset to the pre-iteration + // baseline before the push. + "Expression memory usage (16480 bytes) exceeded limit (10000 bytes) ", " [x for x in range_expr('1-2000000')] ", @@ -1191,3 +1193,25 @@ fn absorbed_unresolved_comprehension_filter_failure_leaves_no_stale_footprint() "len([x for x in Session.List if 'A' * 1000000 - 1] if Session.Flag else []) + len('B' * 600000)", ); } + +// ══════════════════════════════════════════════════════════════ +// The push pre-check charges each element once +// ══════════════════════════════════════════════════════════════ + +/// The element a comprehension is about to push is accounted by +/// BudgetedVec's pre-check. It must not *also* sit in the parent's +/// live footprint through the child's tracked result — that double- +/// charge would shrink the effective memory limit by one element per +/// push. Five 100 KB elements plus Vec slack fit comfortably under +/// 560 KB; a double-charge needs ~600 KB and fails. +#[test] +fn comprehension_push_precheck_charges_each_element_once() { + let st = SymbolTable::new(); + let r = ParsedExpression::new("['A' * 100000 for x in range(5)]") + .and_then(|p| p.with_memory_limit(560_000).evaluate_with_metrics(&[&st])) + .expect("five 100 KB elements must fit under a 560 KB limit"); + assert_eq!( + r.value.expr_type(), + openjd_expr::ExprType::list(openjd_expr::ExprType::STRING) + ); +} diff --git a/crates/openjd-expr/tests/integration/test_operation_limit.rs b/crates/openjd-expr/tests/integration/test_operation_limit.rs index 5571847c..670891fb 100644 --- a/crates/openjd-expr/tests/integration/test_operation_limit.rs +++ b/crates/openjd-expr/tests/integration/test_operation_limit.rs @@ -884,10 +884,9 @@ fn comprehension_over_huge_range_bounded_lazily() { assert_eq!( e, [ - // 134217888 ~ 128 MiB: the Vec's projected post-doubling - // capacity, charged before the growth allocation happens, - // plus the 64-byte loop-variable clone live at the push. - "Expression memory usage (134217888 bytes) exceeded limit (100000000 bytes) + // 134217824 ~ 128 MiB: the Vec's projected post-doubling + // capacity, charged before the growth allocation happens. + "Expression memory usage (134217824 bytes) exceeded limit (100000000 bytes) ", " [x for x in range_expr('0-4611686018427387902')] ", diff --git a/specs/expr/evaluator.md b/specs/expr/evaluator.md index c7e54770..c3977a3a 100644 --- a/specs/expr/evaluator.md +++ b/specs/expr/evaluator.md @@ -422,18 +422,19 @@ Evaluates list comprehensions: `[expr for var in iterable if condition]`. enclosing construct absorbs the error (an unresolved-test conditional or boolop) and continues, the parent's counters must include that spend, or every absorbed comprehension failure evaluates - under-metered. The child's *live footprint* is handled differently: - on any exit where the child's values are dropped (a failing - filter/body, an abandoned iteration, and every exit of the - unresolved-iterable path) the parent's `current_memory` is reset to - its pre-iteration baseline rather than absorbed — the loop-variable - clone and the failed sub-expression's intermediates are not live - after the exit, and the absorbing construct keeps evaluating on this - evaluator, so a stale footprint would charge every later allocation - for memory that is gone. On the success path the footprint *is* - absorbed before the result push (the loop variable's clone is live at - that moment, so a push-time exceedance reports it) and reset to the - baseline afterwards. + under-metered. The child's *live footprint* is never absorbed: on + every exit — success, failure, or abandonment, and on every exit of + the unresolved-iterable path — the parent's `current_memory` is reset + to its pre-iteration baseline. Nothing the child tracks survives the + iteration: the body's intermediates and its result are dropped (the + loop variable's own slot in the temp symbol table is never tracked; + what `eval_name` tracks is the clone it returns when the body reads + it), and the element being pushed is charged exactly once, by + `BudgetedVec`'s pre-check — not a second time through the child's + footprint, which would shrink the effective limit by one element per + push. The absorbing construct keeps evaluating on this evaluator, so + a stale footprint would charge every later allocation for memory + that is not live. - Iterates lists without copying and symbolic ranges lazily - Pre-checks the growing result vector's values and projected capacity against the memory limit From 81bda4ed6018c179e3f35027d20c3414b74b2df3 Mon Sep 17 00:00:00 2001 From: Mark <399551+mwiebe@users.noreply.github.com> Date: Mon, 28 Sep 2026 10:35:44 -0700 Subject: [PATCH 4/5] test(expr): pin the reported usage for a large-element comprehension exceedance Requested by review alongside the double-charge fix (0fceb54): the existing expectations only covered 64-byte elements, where one element counted twice is invisible. Two 600 KB elements fit under 1.5 MB; three exceed it at exactly 1800768 bytes - two held, one being pushed, plus the Vec's projected slack. On the pre-fix commit the two-element case already failed with that same figure, one push early. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> --- .../tests/integration/test_memory.rs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/crates/openjd-expr/tests/integration/test_memory.rs b/crates/openjd-expr/tests/integration/test_memory.rs index 5119747e..09783d42 100644 --- a/crates/openjd-expr/tests/integration/test_memory.rs +++ b/crates/openjd-expr/tests/integration/test_memory.rs @@ -1215,3 +1215,30 @@ fn comprehension_push_precheck_charges_each_element_once() { openjd_expr::ExprType::list(openjd_expr::ExprType::STRING) ); } + +/// The reported `used` for a large-element exceedance is the honest +/// figure: at the third push, two 600 KB elements are held plus the +/// third being pushed plus the Vec's projected slack — ~1.8 MB. A +/// double-charge of the pushed element would have reported the same +/// ~1.8 MB one push *earlier*, at the second element, when only ~1.2 MB +/// was live (pinned here indirectly: the two-element variant fits). +#[test] +fn large_element_comprehension_exceedance_reports_true_usage() { + let st = SymbolTable::new(); + ParsedExpression::new("['A' * 600000 for x in [1, 2]]") + .and_then(|p| p.with_memory_limit(1_500_000).evaluate(&[&st])) + .expect("two 600 KB elements must fit under a 1.5 MB limit"); + let e = ParsedExpression::new("['A' * 600000 for x in [1, 2, 3]]") + .and_then(|p| p.with_memory_limit(1_500_000).evaluate(&[&st])) + .unwrap_err() + .to_string(); + assert_eq!( + e, + [ + "Expression memory usage (1800768 bytes) exceeded limit (1500000 bytes)\n", + " ['A' * 600000 for x in [1, 2, 3]]\n", + " ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~", + ] + .concat() + ); +} From c905bee8307091ad56d12fbea712e3ff6b2d62ea Mon Sep 17 00:00:00 2001 From: Mark <399551+mwiebe@users.noreply.github.com> Date: Mon, 28 Sep 2026 11:41:50 -0700 Subject: [PATCH 5/5] fix(expr): drop the iterable's charge when a comprehension is abandoned Review finding on this PR, verified. The error exits reset current_memory to the per-iteration baseline, but that baseline is captured inside the loop, after the iterable was tracked - so an abandoned comprehension left its iterable charged on the evaluator an enclosing construct keeps evaluating on, even though nothing references it. The success path already handles this ("the iterable is consumed by the comprehension") with an explicit release after the loop; the error exits did not. Pre-existing (main's plain `?` exits skipped the release too), but the same class of residue this PR removes. Two more leaking exits than the review listed: the "Cannot iterate over" type error - which an enclosing construct can absorb, and whose iterable may be the large value itself - and the unresolved-iterable early return. Measured with a 600 KB list from the symbol table and a cheap body failure, then a 600 KB allocation under a 1 MB limit: 1200352 bytes reported before, peak 600192 after. The footprint is now captured *before* the iterable is evaluated, and every exit that abandons the comprehension resets to it: the unresolved-iterable return, the non-iterable type error, the three in-loop error arms (which already absorbed the child's spend), and the push's budget exceedance. By construction rather than by subtracting the iterable's size. Four tests pin the four absorbable exits; each fails on the previous commit and passes now. Two notes for the review thread. The review's literal example (`'A' * 1000000 - 1` over a 600 KB iterable, 1.5 MB limit) does not reach the absorption: 600 KB + 1 MB trips the limit inside the body as a budget exceedance, which nothing absorbs. And a large list *literal* as the iterable trips the limit while being built, before the comprehension runs: eval_list charges both the element and the list constructed from it. That is a separate, pre-existing eval_list accounting matter, noted in the test helper and out of scope here. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com> --- crates/openjd-expr/src/eval/evaluator.rs | 35 +++++++-- .../tests/integration/test_memory.rs | 72 +++++++++++++++++++ specs/expr/evaluator.md | 7 ++ 3 files changed, 107 insertions(+), 7 deletions(-) diff --git a/crates/openjd-expr/src/eval/evaluator.rs b/crates/openjd-expr/src/eval/evaluator.rs index 4864962f..4f57f320 100644 --- a/crates/openjd-expr/src/eval/evaluator.rs +++ b/crates/openjd-expr/src/eval/evaluator.rs @@ -1530,6 +1530,14 @@ impl<'a> Evaluator<'a> { None } }); + // Footprint before the iterable is tracked. Every exit that + // abandons the comprehension — an error an enclosing construct + // may absorb — resets to *this*: the iterable is consumed by the + // comprehension and its charge must not outlive it. (The + // per-iteration baseline captured inside the loop still includes + // the iterable; the success path releases it explicitly after + // the loop.) + let comprehension_baseline = self.current_memory; let iterable = self.eval_node(&gen.iter, None)?; let var_name = match &gen.target { ast::Expr::Name(n) => n.id.to_string(), @@ -1537,10 +1545,12 @@ impl<'a> Evaluator<'a> { }; // Unresolved iterable: per-element evaluation is impossible, so - // the comprehension as a whole is unknown. + // the comprehension as a whole is unknown. The iterable is + // consumed here like everywhere else below. if iterable.is_unresolved() { let inner = unwrap_unresolved(&iterable.expr_type()); let elem_type = inner.list_element_type().cloned().unwrap_or(ExprType::INT); + self.current_memory = comprehension_baseline; return self.eval_listcomp_unresolved(lc, gen.ifs.first(), &var_name, elem_type); } @@ -1557,6 +1567,9 @@ impl<'a> Evaluator<'a> { } else if let ExprValue::RangeExpr(r) = &iterable { Box::new(r.iter().map(ExprValue::Int)) } else { + // A type error an enclosing construct may absorb — the + // iterable's charge must not outlive the comprehension. + self.current_memory = comprehension_baseline; return Err(ExpressionError::type_error(format!( "Cannot iterate over {}", iterable.expr_type() @@ -1593,9 +1606,10 @@ impl<'a> Evaluator<'a> { Err(e) => { // The child's spend counts even though it // failed, but its tracked values are dropped - // with it (see `absorb_spend_and_reset`); the - // regex cache moved into the child comes back. - self.absorb_spend_and_reset(&child, memory_baseline); + // with it (see `absorb_spend_and_reset`), and so + // is the iterable — the comprehension is + // abandoned. The regex cache comes back. + self.absorb_spend_and_reset(&child, comprehension_baseline); self.regex_cache = child.regex_cache; return Err(e); } @@ -1614,7 +1628,7 @@ impl<'a> Evaluator<'a> { filter_unresolved = true; break; } else { - self.absorb_spend_and_reset(&child, memory_baseline); + self.absorb_spend_and_reset(&child, comprehension_baseline); self.regex_cache = child.regex_cache; let err = ExpressionError::new(format!( "List comprehension filter must be a boolean, got {}", @@ -1631,7 +1645,7 @@ impl<'a> Evaluator<'a> { match child.eval_node(&lc.elt, elem_target.as_ref()) { Ok(v) => Some(v), Err(e) => { - self.absorb_spend_and_reset(&child, memory_baseline); + self.absorb_spend_and_reset(&child, comprehension_baseline); self.regex_cache = child.regex_cache; return Err(e); } @@ -1652,7 +1666,14 @@ impl<'a> Evaluator<'a> { self.absorb_spend_and_reset(&child, memory_baseline); self.regex_cache = child.regex_cache; if let Some(elt) = elt { - result.push(self, elt)?; + if let Err(e) = result.push(self, elt) { + // Only a budget exceedance can land here, which no + // enclosing construct absorbs — but the exit drops + // the iterable like every other, so account for it + // the same way. + self.current_memory = comprehension_baseline; + return Err(e); + } } } // The iterable is consumed by the comprehension: release its diff --git a/crates/openjd-expr/tests/integration/test_memory.rs b/crates/openjd-expr/tests/integration/test_memory.rs index 09783d42..e7daf02c 100644 --- a/crates/openjd-expr/tests/integration/test_memory.rs +++ b/crates/openjd-expr/tests/integration/test_memory.rs @@ -1242,3 +1242,75 @@ fn large_element_comprehension_exceedance_reports_true_usage() { .concat() ); } + +// ══════════════════════════════════════════════════════════════ +// An abandoned comprehension's iterable is not charged afterwards +// ══════════════════════════════════════════════════════════════ + +/// The iterable is tracked before the loop and, on the success path, +/// released after it ("consumed by the comprehension"). An error exit +/// that an enclosing construct absorbs must drop it too: the +/// comprehension is abandoned and nothing references the iterable, so +/// leaving its 600 KB in `current_memory` would charge every later +/// allocation for it — here the 600 KB `'B'` string, which then reads +/// as 1.2 MB against a 1 MB limit. The failures are chosen to be cheap +/// (`int()` releases its argument before dispatch; the type errors +/// allocate nothing) so the iterable's residue is what is observed, +/// not a body-time budget exceedance. The large list comes from the +/// symbol table: a list *literal* is charged for both its element and +/// itself while being built — a separate, pre-existing `eval_list` +/// accounting matter — which would trip the limit before the +/// comprehension runs. +fn assert_iterable_not_charged_after_abandonment(expr: &str) { + let mut st = SymbolTable::new(); + st.set( + "Session.Flag", + ExprValue::unresolved(openjd_expr::ExprType::BOOL), + ) + .unwrap(); + st.set( + "Big", + ExprValue::make_list( + vec![ExprValue::String("C".repeat(600_000))], + openjd_expr::ExprType::STRING, + ) + .unwrap(), + ) + .unwrap(); + ParsedExpression::new(expr) + .and_then(|p| p.with_memory_limit(1_000_000).evaluate(&[&st])) + .unwrap_or_else(|e| panic!("{expr}: the abandoned iterable must not be charged: {e}")); +} + +/// Body error over a large iterable. +#[test] +fn abandoned_comprehension_body_error_drops_iterable_charge() { + assert_iterable_not_charged_after_abandonment( + "len([int('nope') for x in Big] if Session.Flag else []) + len('B' * 600000)", + ); +} + +/// Filter error over a large iterable. +#[test] +fn abandoned_comprehension_filter_error_drops_iterable_charge() { + assert_iterable_not_charged_after_abandonment( + "len([x for x in Big if int('nope') > 0] if Session.Flag else []) + len('B' * 600000)", + ); +} + +/// Non-boolean filter over a large iterable. +#[test] +fn abandoned_comprehension_nonbool_filter_drops_iterable_charge() { + assert_iterable_not_charged_after_abandonment( + "len([x for x in Big if 1] if Session.Flag else []) + len('B' * 600000)", + ); +} + +/// "Cannot iterate" type error: the iterable itself is the large value +/// (a string is tracked once, so a literal is fine here). +#[test] +fn abandoned_comprehension_cannot_iterate_drops_iterable_charge() { + assert_iterable_not_charged_after_abandonment( + "len([x for x in 'C' * 600000] if Session.Flag else []) + len('B' * 600000)", + ); +} diff --git a/specs/expr/evaluator.md b/specs/expr/evaluator.md index c3977a3a..2739ddba 100644 --- a/specs/expr/evaluator.md +++ b/specs/expr/evaluator.md @@ -435,6 +435,13 @@ Evaluates list comprehensions: `[expr for var in iterable if condition]`. push. The absorbing construct keeps evaluating on this evaluator, so a stale footprint would charge every later allocation for memory that is not live. +- The iterable is consumed by the comprehension. The success path + releases it after the loop; every exit that *abandons* the + comprehension — an unresolved iterable, a non-iterable value, a + failing filter or body, a push-time exceedance — resets + `current_memory` to the footprint captured *before* the iterable was + evaluated, so an enclosing construct that absorbs the error is not + charged for a list nothing references. - Iterates lists without copying and symbolic ranges lazily - Pre-checks the growing result vector's values and projected capacity against the memory limit