Skip to content

fix(model): do not cap <IntRangeExpr> expansion at the list-form limit #318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
leongdl merged 1 commit into from
Aug 14, 2026
Merged

fix(model): do not cap <IntRangeExpr> expansion at the list-form limit #318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 2 additions & 16 deletions crates/openjd-model/src/job/create_job/ranges.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,15 +209,9 @@ fn resolve_int_range(
&openjd_expr::FormatStringOptions::new().with_path_format(PathFormat::Posix),
) {
match val {
// Range expressions are not length-capped; only the list
// forms are. See `EffectiveLimits::max_task_param_range_len`.

@github-actions github-actions Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the cap here removes the only bound on an IntRangeExpr expansion, and there is at least one path that then materializes the full expansion into memory.

make_chunk_node in step_param_space.rs eagerly collects when adaptive chunking is selected:

// crates/openjd-model/src/job/step_param_space.rs:1720-1722
let values: Vec<i64> = match range {
    job::TaskParamRange::List(v) => v.clone(),
    job::TaskParamRange::RangeExpr(r) => r.iter().collect(),   // <-- full expansion
};

Adaptive chunking is selected whenever a CHUNK[INT] parameter sets chunks.targetRuntimeSeconds > 0 (new_inner, ~line 1307), and AdaptiveChunkNode.values is a Vec<i64>. So a template with

type: CHUNK[INT]
range: "1-1000000000"
chunks: { defaultTaskCount: 10, targetRuntimeSeconds: 60 }

now allocates ~8 GB before any limit is consulted. RangeExpr values are bounded only by MAX_RANGE_VALUE_MAGNITUDE (2^62), so 1-4611686018427387903 is also accepted by the parser.

The stated backstop does not cover this:

  • CallerLimits::max_task_count is Option<u64> and defaults to None, so in the default configuration there is no bound at all.
  • Even when it is set, the check in create_job (mod.rs:115) runs after instantiate_step for all steps, and it builds the iterator with new_with_chunk_override(ps, Some(1)) — which deliberately skips adaptive_info. So the count check never exercises the allocating path, and cannot pre-empt a consumer that later iterates the space for real.

The lazy RangeExprNode / ContiguousChunkNode paths are genuinely index-based and fine (the test_truly_lazy_trillion_element_space test covers those). The adaptive path is the outlier.

Suggest either bounding the eager collect in make_chunk_node (return a ModelError instead of allocating), or keeping a separate expansion limit that is decoupled from the spec-derived max_task_param_range_len so the §3.4.1.1.1 argument in this PR still holds.

ExprValue::RangeExpr(r) => {
if r.len() > limits.max_task_param_range_len {
return Err(ModelError::DecodeValidation(format!(
"Task parameter '{}' range exceeds {} elements ({} elements)",
param_name,
limits.max_task_param_range_len,
r.len()
)));
}
return Ok(job::TaskParamRange::RangeExpr(r));
}
val if val.is_list() => {
Expand Down Expand Up @@ -254,14 +248,6 @@ fn resolve_int_range(
let range_expr: RangeExpr = resolved
.parse()
.map_err(|e: openjd_expr::ExpressionError| ModelError::Expression(e))?;
if range_expr.len() > limits.max_task_param_range_len {
return Err(ModelError::DecodeValidation(format!(
"Task parameter '{}' range exceeds {} elements ({} elements)",
param_name,
limits.max_task_param_range_len,
range_expr.len()
)));
}
Ok(job::TaskParamRange::RangeExpr(range_expr))
}
}
Expand Down
10 changes: 10 additions & 0 deletions crates/openjd-model/src/template/validate_v2023_09/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ pub struct EffectiveLimits {
/// raises the job-template limit to 200.
pub max_env_template_param_count: usize,
pub max_filename_len: usize,
/// Maximum elements in a task parameter's *list*-form range —
/// `<IntRangeList>` (§3.4.1.1), `<FloatRangeList>` (§3.4.1.2) and
/// `<StringRangeList>` (§3.4.1.3).
///
/// Do not apply this to an `<IntRangeExpr>` expansion. §3.4.1.1.1
/// constrains that form only by "no two ranges may overlap", and its stated
/// purpose is expressing frame ranges succinctly — capping the expansion
/// rejects the form's primary use case and pre-empts the host service's own
/// task-count limits, which it may raise per account. A host that wants to
/// bound fan-out has `CallerLimits::max_task_count`.

@github-actions github-actions Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doc comment asserts CallerLimits::max_task_count is a sufficient substitute for the removed cap. It is not, for two reasons beyond the eager-allocation path I flagged in ranges.rs:

1. It is opt-in and defaults to off. CallerLimits::max_task_count is Option<u64> (types.rs:427) and CallerLimits::default() leaves it None — asserted by the crate's own test at test_caller_limits.rs:581. Every caller that does not explicitly set it (including all the decode_ok/check_err helpers the new tests use) now has no bound on range-expression fan-out at all, where before it had 1024.

2. It does not gate the O(num_chunks) containment scan. StaticChunkNode::validate_containment is a linear scan that rebuilds a RangeExpr per chunk:

// step_param_space.rs:859
if (0..self.num_chunks).any(|i| self.chunk_range_expr(i) == *r) {

ContiguousChunkNode::validate_containment (~line 425) is the same shape — it iterates every chunk looking for a match. num_chunks comes from total_len.div_ceil(default_task_count), so range: "1-1000000000" with defaultTaskCount: 10 yields 10^8 iterations, each doing a format! + parse::<RangeExpr>().

This is reachable from openjd-cli: execute_explicit_tasks calls iter.validate_containment(&values) per user-supplied task-parameter set (crates/openjd-cli/src/run/execution.rs:439), and a non-matching value costs the full scan. Under the old cap num_chunks <= 1024, so this was bounded; it no longer is, and max_task_count is not consulted on this path at all.

If the §3.4.1.1.1 reading is right that the spec does not cap the expansion, then these two containment scans need to become index-arithmetic lookups (locate the chunk containing r's first value, then compare) rather than linear searches — otherwise the cap removal converts a validation rejection into a CPU-exhaustion vector.

pub max_task_param_range_len: usize,
pub max_task_param_string_len: usize,
pub max_job_param_string_len: usize,
Expand Down
13 changes: 5 additions & 8 deletions crates/openjd-model/src/template/validate_v2023_09/structure.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1053,18 +1053,15 @@ fn validate_task_param_range(
}
}
IntRange::Expression(expr) => {
// Grammar only. `max_task_param_range_len` deliberately does not
// apply to an expression's expansion — see its doc comment.
let raw = expr.raw();
if !raw.contains("{{") {
match raw.parse::<openjd_expr::RangeExpr>() {
Ok(range) => {
if range.len() > limits.max_task_param_range_len {
errors.add(path, format!("INT parameter '{}' range expression expands to {} elements (max {}).", tp.name, range.len(), limits.max_task_param_range_len));
}
}
Err(e) => errors.add(
if let Err(e) = raw.parse::<openjd_expr::RangeExpr>() {
errors.add(
path,
format!("INT parameter '{}' range expression error: {e}", tp.name),
),
);
}
}
}
Expand Down
28 changes: 28 additions & 0 deletions crates/openjd-model/tests/integration/test_chunk_int.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,3 +856,31 @@ fn chunks_parameter_name() {
let tasks: Vec<_> = iter.collect();
assert_eq!(tasks.len(), 2);
}

// ══════════════════════════════════════════════════════════════
// Range length — §3.4 caps the list form only
// ══════════════════════════════════════════════════════════════

/// §3.4.1.1.1 `<IntRangeExpr>` states no element cap, so a CHUNK[INT] range
/// expression may expand past `max_task_param_range_len`.
#[test]
fn range_expression_expansion_is_not_capped() {

@github-actions github-actions Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is vacuous with respect to the change: it would pass unmodified on the base commit.

The CHUNK_INT arm of validate_task_param_range never had a length cap on the expression form — the diff leaves it untouched, and in the current file (structure.rs:1155-1166) it only does a grammar parse:

IntRange::Expression(expr) => {
    if !expr.raw().contains("{{") {
        if let Err(e) = expr.raw().parse::<openjd_expr::RangeExpr>() {

Only the INT arm was capped and only that arm was edited. Since decode_ok stops at decode_job_template, "1-1025" through a CHUNK[INT] parameter was already accepted before this PR.

That makes this a no-op regression guard. To actually pin the CHUNK[INT] behavior the cap removal affects, the test needs to reach create_job (which calls resolve_int_range, the site this PR edits) and construct the iterator.

for range in ["1-1025", "1-5000", "1-100000:2"] {
decode_ok(&chunk_job(&format!(
r#"{{"name": "foo", "type": "CHUNK[INT]", "range": "{range}", "chunks": {{"defaultTaskCount": 10, "rangeConstraint": "CONTIGUOUS"}}}}"#
)));
}
}

@github-actions github-actions Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"1-100000:2" in this list is the interesting case, and it points at a third place the removed cap was load-bearing.

count_contiguous_chunks_from_sub_ranges documents itself as O(R) where R is the number of sub-ranges, "not the number of values". That is true only for step == 1 sub-ranges. The else branch expands per value:

// step_param_space.rs:359-364
} else {
    // Step > 1: each value is isolated (has gaps between them).
    let count = sr.len();
    for idx in 0..count {                       // <-- O(values), not O(sub-ranges)
        let val = sr.get(idx).expect("index within sub-range bounds");

This runs at node-construction time — ContiguousChunkNode::new calls count_contiguous_chunks_for_range eagerly to cache num_chunks — so it is not deferred by the lazy-iteration design that makes RangeExprNode safe.

Under the old 1024 cap this loop was bounded. With the cap removed, range: "1-1000000000:2" + rangeConstraint: CONTIGUOUS is 5x10^8 iterations of sr.get() before a single task is produced, and 1-4611686018427387903:2 is accepted by the RangeExpr parser (bounded only by MAX_RANGE_VALUE_MAGNITUDE).

Note this test only reaches decode_job_template, which validates the template but never constructs a StepParameterSpaceIterator — so it will pass without exercising the path. A test that goes through create_job + iterator construction with a stepped CONTIGUOUS range would surface it.

The counting for stepped sub-ranges is closed-form (each value is its own interval, so it contributes ceil(1/dtc) == 1 chunk unless it is adjacent to the previous interval, which for step >= 2 it never is). Replacing the loop with arithmetic would restore the documented O(R) and remove the need for a cap here.


/// §3.4.1.1 item 4 caps the list form at 1024 elements. That cap stays.
#[test]
fn range_list_is_still_capped() {
let values: Vec<String> = (0..1025).map(|v| v.to_string()).collect();
check_err(
&chunk_job(&format!(
r#"{{"name": "foo", "type": "CHUNK[INT]", "range": [{}], "chunks": {{"defaultTaskCount": 10, "rangeConstraint": "CONTIGUOUS"}}}}"#,
values.join(",")
)),
&["range exceeds 1024 elements"],
);
}
54 changes: 54 additions & 0 deletions crates/openjd-model/tests/integration/test_parameter_space.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,3 +739,57 @@ fn combination_expr_leading_star_rejected() {
"Leading star in combination should be rejected"
);
}

// ══════════════════════════════════════════════════════════════
// Task parameter range length — §3.4 caps the list forms only
// ══════════════════════════════════════════════════════════════

/// §3.4.1.1.1 `<IntRangeExpr>` states no element cap, so the expansion of a
/// range expression must not be bounded by `max_task_param_range_len`. The
/// form exists to express frame ranges, which routinely exceed 1024 values.
#[test]
fn int_range_expression_expansion_is_not_capped() {
for range in ["1-1024", "1-1025", "1-5000", "1-100000:2"] {

@github-actions github-actions Bot Aug 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR removes the cap at two sites, but the new tests only cover one of them.

decode_ok here calls decode_job_template (helper at line 29), which runs the validation pipeline only — it never calls create_job. So these tests exercise the structure.rs::validate_task_param_range removal but not the ranges.rs::resolve_int_range removal, which is the site that actually produces the TaskParamRange::RangeExpr a consumer will later iterate.

Two consequences:

  1. The ranges.rs change is unverified by this PR. Notably resolve_int_range has two paths that were capped — the typed-eval ExprValue::RangeExpr(r) arm (line 214) and the string-resolution fallback (line 251) — and neither is reached by a decode-only test. A create_job-based test would also cover the "1-{{Param.Count}}" format-string case, which decode skips entirely because of the !raw.contains("{{") guard in structure.rs.

  2. The list-form caps that these tests assert still apply (int_range_list_is_still_capped, string_range_list_is_still_capped) are likewise only proving the structure.rs cap fires. The parallel caps in ranges.rs (lines 190, 229, 324, 377) are what protect a caller that constructs a Job directly, and they remain untested here.

Suggest adding at least one test that goes through create_job and then StepParameterSpaceIterator::new, in the style of lazy_param_space_range_expr_within_limit in test_step_param_space_iter.rs — that is the path where an uncapped expansion actually costs something.

Separately: that existing test is now named ..._within_limit and its explanatory comment about max_task_param_range_len was deleted in this PR, leaving a name that references a limit no longer applied to range expressions. Worth renaming.

decode_ok(&job_with_param_space(&format!(
r#"{{"taskParameterDefinitions": [{{"name": "Frame", "type": "INT", "range": "{range}"}}]}}"#
)));
}
}

/// A malformed range expression is still rejected — dropping the length cap
/// must not drop grammar validation.
#[test]
fn int_range_expression_grammar_is_still_validated() {
check_err(
&job_with_param_space(
r#"{"taskParameterDefinitions": [{"name": "Frame", "type": "INT", "range": "1-10,5-15"}]}"#,
),
&["range expression error"],
);
}

/// §3.4.1.1 item 4 caps `<IntRangeList>` at 1024 elements. That cap stays.
#[test]
fn int_range_list_is_still_capped() {
let values: Vec<String> = (0..1025).map(|v| v.to_string()).collect();
check_err(
&job_with_param_space(&format!(
r#"{{"taskParameterDefinitions": [{{"name": "Frame", "type": "INT", "range": [{}]}}]}}"#,
values.join(",")
)),
&["range exceeds 1024 elements"],
);
}

/// §3.4.1.3 caps `<StringRangeList>` too.
#[test]
fn string_range_list_is_still_capped() {
let values: Vec<String> = (0..1025).map(|v| format!("\"v{v}\"")).collect();
check_err(
&job_with_param_space(&format!(
r#"{{"taskParameterDefinitions": [{{"name": "S", "type": "STRING", "range": [{}]}}]}}"#,
values.join(",")
)),
&["range exceeds 1024 elements"],
);
}
1 change: 0 additions & 1 deletion crates/openjd-model/tests/integration/test_step_param_space_iter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,6 @@ fn test_single_param_getitem() {

#[test]
fn lazy_param_space_range_expr_within_limit() {
// max_task_param_range_len is 1024 for all configs (not raised by FB1)
let template = yaml_val(
r#"
specificationVersion: "jobtemplate-2023-09"
Expand Down
6 changes: 6 additions & 0 deletions specs/model/parameters.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,12 @@ Numeric limits derived from context. FEATURE_BUNDLE_1 raises many limits:
| `max_command_len` | 1024 | 1024 |
| `max_description_len` | 2048 | 2048 |

`max_task_param_range_len` applies to the **list** forms of a task parameter
range only — `<IntRangeList>` (§3.4.1.1), `<FloatRangeList>` (§3.4.1.2) and
`<StringRangeList>` (§3.4.1.3). §3.4.1.1.1 `<IntRangeExpr>` states no element
cap, so an expression's expansion is unbounded here; a host that needs to bound
fan-out uses `CallerLimits::max_task_count`.

### EffectiveRules

Structural rules derived from context:
Expand Down
Loading