fix: resolve the per-script timeout parent-side and keep timeout output - #227
Merged
Merged
Conversation
The kill timer lives in the parent: build_util read BUILD_SCRIPT_TIMEOUT once at import into TIMEOUT_SECS and passed that straight to subprocess.run, while the per-script env from env_config.build_env_for_script went only to the CHILD. A profile that set BUILD_SCRIPT_TIMEOUT on an overrides pattern was therefore silently ignored — there was no way to budget one script class without moving the cap for every script in the run. timeout_for(env) resolves it parent-side, used by both execute_script and execute_notebook. Precedence is profile value > ambient global > 300 default. A profile value deliberately beats the ambient global because run_all exports BUILD_SCRIPT_TIMEOUT unconditionally, even when 300 was only its CLI default — so the parent cannot distinguish a deliberate operator cap from the default, and the opposite rule would make per-script budgets work under CI while being silently ignored under run_all. Malformed, zero and negative values fall back to the global rather than disabling the cap. Second defect, same path: the TimeoutExpired handler discarded e.stdout and e.stderr and recorded only "Timed out after Ns". A killed script cannot report its own progress, so a TIMEOUT artefact could not say WHICH block was running — which is why the three jax_grad timeouts in PyAutoHeart workspace-smoke run 30858578587 could not be diagnosed from CI output at all. The handler now keeps a truncated tail of each captured stream and records the cap that was in force. No schema change: profiles express this with the existing `set:` key, which validate_env_profiles already allows (ALLOWED_OVERRIDE_KEYS is pattern/set/ unset, so a new `timeout:` key would have been rejected). The SLOW banner in slow_skip_check quotes build_util.TIMEOUT_SECS as the cap; with per-script budgets that figure is the run-wide DEFAULT, not necessarily the cap a given script ran under, and the text now says so. Understating a cap is not cosmetic — it biases every "too slow to un-skip?" call toward parking scripts that would pass (the 60s-cap myth, #172). Tests: 25 covering resolution, malformed/zero/negative fallback, precedence vs the ambient global, the release 1800s case, output rendering/decoding/ truncation, and end-to-end kills through a real subprocess for both the matching and non-matching script. Verified against the unfixed behaviour first: 11 of them fail without this change. Refs #226 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 24, 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.
Phase 1 of #226. Runner-side only — no workspace profile is changed here, so no
script's cap moves yet. Phase 2 sets the actual
jax_grad/budget once measured.Why
Three
autolens_workspace_testjax_grad scripts TIMEOUT at the 300s smoke cap(PyAutoHeart workspace-smoke run 30858578587). Two defects in the same code path
made that both unfixable and undiagnosable:
1. A per-script budget was impossible.
BUILD_SCRIPT_TIMEOUTis read once atimport into
TIMEOUT_SECSand passed straight tosubprocess.run(timeout=...).The kill timer lives in the PARENT, but the per-script env from
env_config.build_env_for_scriptis handed only to the CHILD — so a profilesetting
BUILD_SCRIPT_TIMEOUTon anoverridespattern was silently ignored.There was no way to budget one script class without moving the cap for the
entire run.
2. A TIMEOUT could not say where it died. The
TimeoutExpiredhandlerdiscarded
e.stdout/e.stderrand recorded only"Timed out after Ns". Akilled script cannot report its own progress, so the CI artefact named the file
and nothing else. This is why the three timeouts could not be diagnosed from CI
output at all — and why the underlying question (real slowdown, or a cap that
never fitted?) is still open for one of the three.
Fixing (2) is what makes the measurement in phase 2 legible.
What changed
build_util.timeout_for(env)resolves the effective cap parent-side; used byboth
execute_scriptandexecute_notebook.TimeoutExpiredhandlers keep a truncated tail of each captured streamand record the cap in force.
slow_skip_checkbanner now says "default" cap rather than asserting one capfor every script.
Precedence: profile value > ambient global > 300 default. A profile value
deliberately beats the ambient global, because
run_all.py:256exportsBUILD_SCRIPT_TIMEOUTunconditionally — even when 300 was only its CLI default— so the parent cannot distinguish a deliberate operator cap from the default.
The opposite rule would make per-script budgets work under Heart's workflow
(which does not go through
run_all) while being silently ignored locally: theexact silent-divergence class this change exists to remove.
Malformed, zero and negative values fall back to the global rather than
disabling the cap.
API Changes
No breaking changes. Behaviour additions only:
build_util.timeout_for(env=None) -> int. Previously callers readbuild_util.TIMEOUT_SECSdirectly; that constant is unchanged and still thefallback, so existing callers keep working.
EXISTING
set:key —set: {BUILD_SCRIPT_TIMEOUT: "1800"}on anoverridespattern is now honoured by the parent, where before it was silently dropped.
No profile schema change:
validate_env_profiles.ALLOWED_OVERRIDE_KEYSis{pattern, set, unset}, so a newtimeout:key would have been rejected —reusing
set:keeps every existing profile valid.ScriptResult.error_messagefor a TIMEOUT now carries the cap inforce plus a truncated output tail. Anything parsing that string for exactly
"Timed out after Ns"should match a prefix, not the whole value.No workspace or library source is affected; no release is required.
Testing
tests/test_script_timeout.py— 25 tests: resolution, malformed/zero/negativefallback, precedence vs the ambient global, the
mode=release1800s case,output rendering/decoding/truncation, and end-to-end kills through a real
subprocess for BOTH a matching and a non-matching script.
Control-tested against the unfixed behaviour first: with
timeout_forand_timeout_outputstubbed back to the old semantics, 11 of the 25 fail —including every end-to-end case. The negative-control test (non-matching script
keeps the global cap) correctly passes in both states.
Full suite: 281 passed, 2 failed. Both failures are in
test_python_matrix_workflow.py(experimental_python_314job key) and wereverified to fail identically on unmodified
mainat9601f18— pre-existingand unrelated to this change.
Refs #226