fix: Defer adaptive chunk adjustment without a measurable sample - #232
Open
leongdl wants to merge 1 commit into
Open
fix: Defer adaptive chunk adjustment without a measurable sample#232leongdl wants to merge 1 commit into
leongdl wants to merge 1 commit into
Conversation
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>
crowecawcaw
approved these changes
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adaptive chunking computed both
with neither divisor guarded. This implements step 3 of the adaptive chunking
algorithm in the CLI specification, added upstream in
openjd-rs #282:
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_countcannot be zero. It accumulateslen(IntRangeExpr.from_str(...)), and anIntRangeExprcannot be empty —"1-0"and"5-1"raiseValueError,"0-0"and"1-1"have length 1.completed_task_durationcannot be0.0in practice.run_task()alwaysspawns 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_countertick (~42 ns on my host) against a ~10 ms floor for the cheapestpossible subprocess.
The guard is still worth having, because the two implementations fail
differently if it is ever reached: the Rust CLI produces
infand saturates toa huge chunk size, while this one raises
ZeroDivisionErrorout of the run.How
Extracted a module-level
_calculate_adaptive_chunk_sizereturningOptional[int], mirroring openjd-rs'scalculate_adaptive_chunk_size, so thedeferral 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 bya named test. Removing the sentinel check is caught by the pre-existing
test_openjd_run_on_chunked_job_adaptive_chunking, which is the evidence thatthe extraction is behaviour-preserving on the real path.
One finding worth surfacing: my first draft used
continueon 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 wouldnot 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 theestimate never defers, so the interaction was unpinned. Added
test_maximum_task_count_is_honoured_when_every_estimate_defers, which forcesevery estimate to defer; it catches that mutant now.
Why not on #230
#230 is the RFC
0007/0008 feature work. It is already
MERGEABLEand 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.
mainlinealready carriesopenjd-model >= 0.9, < 0.12; #230 only raises theopenjd-sessionsfloor to>= 0.10.11. The red specs suite is caused by thereleased 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.