Repository navigation
Conversation
run-executor-timeout-honesty.test.ts armed the real watchdog with a 400 ms no-progress budget. The watchdog is armed before the connector subprocess is spawned, so that budget also had to cover node startup, the START handshake and the ingest round-trip. On a loaded host those exceeded it: the run was killed with run_timed_out before its records were ingested, so the records_emitted: 5 and "survives past the budget" tests failed (16 of 16 parallel runs under CPU load; the file passes alone). Let the attempt watchdog take its timer functions: createAttemptWatchdog and RunExecutorDeps gain an optional `watchdogTimers`, defaulting to the global timers, so production behaviour is unchanged. The tests now inject a manual timer, wait for the event they assert on (records durably ingested, the local-only boundary latched), assert the delay the watchdog armed (one budget, or the four-budget ceiling after the boundary) and fire it explicitly. The phase-boundary connector waits for a release marker instead of sleeping for a multiple of the budget. Under CPU load (3 busy loops per core, 16 parallel runs) the old file failed in 16 of 16 runs; the new one failed in 0 of 48. Replacing the ceiling with the primary budget in run-executor.ts makes the boundary tests fail. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
…default timers WatchdogTimers.setTimeout now returns a canceller instead of a Node timer, so a test implementation no longer has to fabricate a NodeJS.Timeout and cast it back in clearTimeout. unref moves into the production default. The test header now says what the suite proves with injected timers. One real-timer test covers the default wiring: a tiny budget and a hanging connector must end as run_timed_out. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
tnunamak
changed the title
A NaN or negative maxRunWallClockMs read as "no ceiling", so a hanging connector was never killed and nothing recorded why. createAttemptWatchdog and createRunExecutor now throw on it. Infinity and 0 stay the documented explicit opt-outs. Timer delays are clamped to 2^31 - 1 ms: budget * 4 could overflow to Infinity (read as unbounded) and Node fires a longer setTimeout almost at once. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
markProgress latched the local-only phase and reported a suppression even when nothing was armed: with an opt-out budget (0 or Infinity), after the watchdog fired, or after an owner cancel. The caller then recorded a "watchdog suppressed" event for a suppression that never happened. It now latches only while a primary timer could still fire. The WatchdogTimers canceller is documented as idempotent, and the cancelled timer is dropped after the latch. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
The scheduler already rejects a NaN or negative maxRunWallClockMs with resolveNonNegativeMsOrInfinity before createRunExecutor sees it, and that is the only production caller. The watchdog's own copy had the same message, so it and its tests are removed. Behaviour is unchanged. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
tnunamak
changed the title
Move the timer-delay clamp and the local-only latch guard out of this change so the production diff stays within the mutation cohort bound. They return as their own change with their own tests. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
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.
No change for users. The scheduler's run watchdog now accepts injectable timers, defaulting to the real ones, so tests can fire it on demand.
The run-executor timeout tests armed the real watchdog with a 400 ms budget, and that budget also covered process startup and the ingest round-trip. On a loaded host the run was killed before its records were ingested, so
records_emitted: Nand the local-only-phase test failed.The tests now wait for the event they assert on (records ingested, phase boundary latched), check the delay the watchdog armed, and fire it explicitly. One test uses the real timers with a hanging connector and expects
run_timed_out.The timer seam returns a canceller, so a test implementation does not have to fake a Node timer handle.
Proof: with 3 busy loops per core and 16 parallel runs, the file failed 16 of 16 times before and 0 of 48 after. The suites for the watchdog and scheduler config pass.
The non-required "mutate changed production files" job cannot sample
scheduler/run-executor.tsbecause 12 test files import it, so it fails for any change to that file beyond a trivial one.Assisted-by: AI