Skip to content

fix: Defer adaptive chunk adjustment without a measurable sample - #232

Open
leongdl wants to merge 1 commit into
OpenJobDescription:mainlinefrom
leongdl:fix-adaptive-chunk-zero-duration
Open

fix: Defer adaptive chunk adjustment without a measurable sample#232
leongdl wants to merge 1 commit into
OpenJobDescription:mainlinefrom
leongdl:fix-adaptive-chunk-zero-duration

Conversation

@leongdl

@leongdl leongdl commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

What

Adaptive chunking computed both

duration_per_task   = completed_task_duration / completed_task_count
adaptive_chunk_size = target_runtime_seconds / duration_per_task

with neither divisor guarded. This implements step 3 of the adaptive chunking
algorithm in the CLI specification, added upstream in
openjd-rs #282:

If the cumulative duration is zero or non-finite, keep the current chunk size
and wait for a measurable sample.

Severity: conformance and hardening, not a reachable crash

Being explicit, because it would be easy to oversell this. I could not reach
either divisor through a normal openjd run:

  • completed_task_count cannot be zero. It accumulates
    len(IntRangeExpr.from_str(...)), and an IntRangeExpr cannot be empty —
    "1-0" and "5-1" raise ValueError, "0-0" and "1-1" have length 1.
  • completed_task_duration cannot be 0.0 in practice. run_task() always
    spawns and waits on a real subprocess, and raises before the accumulation line
    on failure, so the measured delta would have to fit inside a single
    perf_counter tick (~42 ns on my host) against a ~10 ms floor for the cheapest
    possible subprocess.

The guard is still worth having, because the two implementations fail
differently if it is ever reached: the Rust CLI produces inf and saturates to
a huge chunk size, while this one raises ZeroDivisionError out of the run.

How

Extracted a module-level _calculate_adaptive_chunk_size returning
Optional[int], mirroring openjd-rs's calculate_adaptive_chunk_size, so the
deferral is unit-testable without having to provoke an unreachable timing
condition end to end.

Testing

Mutation-checked: 10 mutants, 0 survivors. Removing the guard or any one of
its three arms, making it always defer, ignoring the sentinel at the call site,
and removing the conservative blend or the max(..., 1) clamp are each caught by
a named test. Removing the sentinel check is caught by the pre-existing
test_openjd_run_on_chunked_job_adaptive_chunking, which is the evidence that
the extraction is behaviour-preserving on the real path.

One finding worth surfacing: my first draft used continue on the deferral path,
which also skips the maximum-task countdown further down the loop and so silently
breaks --maximum-tasks. I caught it by reading, then confirmed the suite would
not have — that mutant survived. test_openjd_run_on_chunked_job_maximum_task_count[1]
does cover adaptive chunking with --maximum-tasks, but with real durations the
estimate never defers, so the interaction was unpinned. Added
test_maximum_task_count_is_honoured_when_every_estimate_defers, which forces
every estimate to defer; it catches that mutant now.

301 passed, 2 skipped   (289 before)
ruff clean · black clean · mypy clean

Why not on #230

#230 is the RFC
0007/0008 feature work. It is already MERGEABLE and waiting only on approval,
and this change is unrelated to its scope — not worth resetting its review over.

Correction to an earlier version of this description: I originally wrote that
#230's merge unblocks the openjd-specifications conformance suite. That is wrong.
mainline already carries openjd-model >= 0.9, < 0.12; #230 only raises the
openjd-sessions floor to >= 0.10.11. The red specs suite is caused by the
released cli 0.7.5 pinning openjd-model < 0.10 (so it resolves model 0.9.0,
which has no EXPR support), and the specs workflows install openjd-cli from the
public registries. What that suite needs is a cli release, not this or any
other PR merge.

Adaptive chunking computed

    duration_per_task   = completed_task_duration / completed_task_count
    adaptive_chunk_size = target_runtime_seconds / duration_per_task

with neither divisor guarded. Implements step 3 of the adaptive chunking
algorithm in the CLI specification, added upstream in openjd-rs #282: "If
the cumulative duration is zero or non-finite, keep the current chunk size
and wait for a measurable sample."

Be clear about severity: this is spec conformance and hardening, NOT a fix
for a reachable crash. I could not reach either divisor through a normal
`openjd run`:

- completed_task_count cannot be zero. It accumulates
  len(IntRangeExpr.from_str(...)), and an IntRangeExpr cannot be empty --
  "1-0" and "5-1" raise ValueError, "0-0" and "1-1" have len 1.
- completed_task_duration cannot be 0.0 in practice. run_task() always
  spawns and waits on a real subprocess, and raises before the
  accumulation line on failure, so the measured delta would have to fit
  inside one perf_counter tick (~42 ns on this host, against a ~10 ms
  floor for the cheapest possible subprocess).

Correcting an earlier claim of mine: I previously recorded this as a
reproduced defect. It was not. The "reproduction" replayed the arithmetic
with a hardcoded 0.0, which demonstrates nothing about the code path.

The guard is still worth having, because the two implementations fail
differently if it is ever reached: Rust produces inf and saturates to a
huge chunk size, while Python raises ZeroDivisionError out of the run.

Extracted as a module-level _calculate_adaptive_chunk_size returning
Optional[int], mirroring openjd-rs's calculate_adaptive_chunk_size, so the
deferral is unit-testable without provoking an unreachable timing
condition.

Mutation-checked, 10 mutants, 0 survivors: removing the guard or any one
of its three arms, making it always defer, ignoring the sentinel at the
call site, and removing the blend or the clamp are each caught by name.
Removing the sentinel check is caught by the pre-existing end-to-end
test_openjd_run_on_chunked_job_adaptive_chunking, which is the evidence
the extraction is behaviour-preserving on the real path.

One finding from that exercise worth recording. My first draft used
`continue` on the deferral path, which also skips the maximum-task
countdown further down the loop and so silently breaks --maximum-tasks. I
caught it by reading, and then confirmed the suite would NOT have: the
mutant survived. The existing
test_openjd_run_on_chunked_job_maximum_task_count[1] covers adaptive
chunking with --maximum-tasks, but with real durations the estimate never
defers, so the interaction was unpinned. Adds
test_maximum_task_count_is_honoured_when_every_estimate_defers, which
forces every estimate to defer; it now catches that mutant.

Raised as its own PR rather than added to OpenJobDescription#230: that PR is the RFC
0007/0008 feature work, is already MERGEABLE and waiting only on
approval, and its merge unblocks the specifications conformance suite.
This change is unrelated to its scope and not urgent enough to reset its
review.

Verified: 301 passed / 2 skipped (289 before), ruff clean, black clean,
mypy clean.

Signed-off-by: David Leong <116610336+leongdl@users.noreply.github.com>
@leongdl
leongdl requested a review from a team as a code owner July 30, 2026 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants