-
Notifications
You must be signed in to change notification settings - Fork 483
Increase post-result watchdog default to 120s and add frontmatter timeout override #51292
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.
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
Changes from all commits
d524487
a8b3831
e70abf1
78a5d9a
cf9d95e
cbafa8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # ADR-51292: Increase Harness Watchdog Default and Add Frontmatter Timeout Control | ||
|
|
||
| **Date**: 2026-08-08 | ||
| **Status**: Draft | ||
| **Deciders**: pelikhan | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| The post-result harness watchdog kills any engine process that is silent (no stdio) for longer than a configured idle timeout after it has emitted a terminal safe output. The previous default was 20 seconds. In practice, large-repo workflows commonly pass through quiet shell phases (package installs, builds, index refreshes) that comfortably exceed 20 seconds without producing output, causing the watchdog to send a premature SIGTERM and leave runs incomplete. | ||
|
|
||
| The only existing override surface was `GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS` set via `engine.env`, which requires the caller to know the env var name, know the unit is milliseconds, and accept that the override is indistinguishable from other env-var customizations. This surface is not surfaced in schema validation or reference docs in a discoverable way. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will raise `DEFAULT_POST_RESULT_WATCHDOG_IDLE_TIMEOUT_MS` from 20 seconds (20 000 ms) to 2 minutes (120 000 ms) in `process_runner.cjs` — the shared location read by all harnesses — and we will add `engine.harness.watchdog-timeout` as a first-class frontmatter integer field (unit: seconds) that compiles to `GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS` (unit: milliseconds). The new field sits alongside the existing retry-policy fields under `engine.harness`, follows the same literal-or-expression pattern, and is validated by the main workflow JSON schema. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Raise the default only, no new frontmatter field | ||
|
|
||
| Increasing the default to 120 s without adding a frontmatter override surface would fix the most common regression at minimal API cost. Workflows with genuine requirements for a shorter or longer timeout would still have to fall back to the raw `engine.env.GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS` override, with its undiscoverable name and millisecond unit. This option trades surface simplicity for discoverability and per-workflow configurability. | ||
|
|
||
| #### Alternative 2: Keep the 20 s default, improve env-var documentation only | ||
|
|
||
| Documenting `GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS` more prominently in reference docs and adding schema validation for it as an `engine.env` value would let advanced users tune the timeout without changing the default. This avoids the risk that a longer default masks genuinely stuck processes, but does not address the root cause: the 20 s window is too short for routine large-repo operations and results in hard-to-diagnose incomplete runs. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - Premature watchdog kills during legitimate quiet phases (installs, builds, index refreshes) are eliminated for typical large-repo workflows under the new 2-minute default. | ||
| - Workflow authors can tune per-workflow watchdog behavior via a discoverable, schema-validated frontmatter field without memorising env var names or unit conversions. | ||
| - Explicit `engine.env.GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS` overrides continue to take precedence, preserving backward compatibility for callers already using the env-var surface. | ||
|
|
||
| #### Negative | ||
| - A genuinely stuck post-result process now takes up to 2 minutes (instead of 20 seconds) to be forcibly terminated under the new default, increasing the worst-case cost of a hung run. | ||
| - Adding `watchdog-timeout` to `engine.harness` extends the frontmatter API surface; future breaking changes to this field require a deprecation path. | ||
|
|
||
| #### Neutral | ||
| - The `watchdog-timeout` frontmatter field accepts seconds; the runtime converts to milliseconds before injecting `GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS`. This unit difference is intentional (seconds are more human-readable in config) and is documented in schema and reference docs. | ||
| - The existing `MIN_POST_RESULT_WATCHDOG_TIMEOUT_MS` (50 ms) and `MAX_POST_RESULT_WATCHDOG_TIMEOUT_MS` (10 min) guards are unchanged and continue to prevent obviously invalid overrides. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12797,6 +12797,18 @@ | |
| } | ||
| ], | ||
| "description": "Maximum delay cap in ms. Accepts a literal integer or a GitHub Actions expression." | ||
| }, | ||
| "watchdog-timeout": { | ||
| "oneOf": [ | ||
| { | ||
| "type": "integer", | ||
| "minimum": 1 | ||
| }, | ||
| { | ||
| "type": "string" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Schema enforces 💡 SuggestionAdd "watchdog-timeout": {
"oneOf": [
{
"type": "integer",
"minimum": 1,
"maximum": 600
},
{ "type": "string" }
]
}And add a negative test in @copilot please address this. |
||
| } | ||
| ], | ||
| "description": "Post-result idle watchdog timeout in seconds. Accepts a literal integer or a GitHub Actions expression." | ||
| } | ||
| }, | ||
| "additionalProperties": false | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,33 @@ func parseHarnessMaxRetriesValue(raw any) string { | |
| return parseIntOrExpressionValue(raw, 0, "harness.max-retries") | ||
| } | ||
|
|
||
| // parseHarnessWatchdogTimeoutValue parses harness.watchdog-timeout (seconds) | ||
| // and converts it to milliseconds for GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS. | ||
| // Accepts a positive integer (converted seconds→ms) or a GitHub Actions expression | ||
| // template (${{ ... }}), which is passed through unchanged and must already be in ms. | ||
| func parseHarnessWatchdogTimeoutValue(raw any) string { | ||
| seconds := parseIntOrExpressionValue(raw, 1, "harness.watchdog-timeout") | ||
| if seconds == "" { | ||
| return "" | ||
| } | ||
| // GitHub Actions expressions do not support arithmetic operators; pass through | ||
| // unchanged. Callers using an expression must supply a value already in ms. | ||
| if _, ok := extractWrappedGitHubExpression(seconds); ok { | ||
| return seconds | ||
| } | ||
| parsedSeconds, err := strconv.ParseInt(seconds, 10, 64) | ||
| if err != nil { | ||
| engineLog.Printf("Ignoring invalid harness.watchdog-timeout value: %q", seconds) | ||
| return "" | ||
| } | ||
| const maxInt64Div1000 = int64((1<<63)-1) / 1000 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/codebase-design] The 💡 Details
The real upper bound enforced by the JS runtime is const maxWatchdogSeconds = 600
if parsedSeconds > maxWatchdogSeconds {
engineLog.Printf("Ignoring out-of-range harness.watchdog-timeout value (max 600 s): %q", seconds)
return ""
}This gives users a meaningful error when they exceed the JS-side clamp, keeping Go and JS bounds in sync. @copilot please address this. |
||
| if parsedSeconds > maxInt64Div1000 { | ||
| engineLog.Printf("Ignoring out-of-range harness.watchdog-timeout value: %q", seconds) | ||
| return "" | ||
| } | ||
| return strconv.FormatInt(parsedSeconds*1000, 10) | ||
| } | ||
|
|
||
| func parseIntOrExpressionValue(raw any, minValue int, fieldName string) string { | ||
| if val, ok := typeutil.ParseIntValue(raw); ok && val >= minValue { | ||
| return strconv.Itoa(val) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,46 @@ func TestResolveEngineID(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestApplyEngineHarnessRetryEnv(t *testing.T) { | ||
| t.Run("injects harness policy env vars including watchdog timeout", func(t *testing.T) { | ||
| env := map[string]string{} | ||
| workflowData := &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ | ||
| HarnessMaxRetries: "6", | ||
| HarnessInitialDelayMs: "10000", | ||
| HarnessBackoffMultiplier: "2", | ||
| HarnessMaxDelayMs: "180000", | ||
| HarnessWatchdogTimeoutMs: "120000", | ||
| }, | ||
| } | ||
|
|
||
| applyEngineHarnessRetryEnv(env, workflowData) | ||
|
|
||
| assert.Equal(t, "6", env["GH_AW_HARNESS_MAX_RETRIES"]) | ||
| assert.Equal(t, "10000", env["GH_AW_HARNESS_INITIAL_DELAY_MS"]) | ||
| assert.Equal(t, "2", env["GH_AW_HARNESS_BACKOFF_MULTIPLIER"]) | ||
| assert.Equal(t, "180000", env["GH_AW_HARNESS_MAX_DELAY_MS"]) | ||
| assert.Equal(t, "120000", env["GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS"]) | ||
| }) | ||
|
|
||
| t.Run("engine.env override still wins after harness policy env injection", func(t *testing.T) { | ||
| env := map[string]string{} | ||
| workflowData := &WorkflowData{ | ||
| EngineConfig: &EngineConfig{ | ||
| HarnessWatchdogTimeoutMs: "120000", | ||
| Env: map[string]string{ | ||
| "GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS": "90000", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| applyEngineHarnessRetryEnv(env, workflowData) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The precedence test ( 💡 SuggestionCapture an intermediate snapshot to make the test a real precedence proof: applyEngineHarnessRetryEnv(env, workflowData)
assert.Equal(t, "120000", env["GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS"], "harness injection should write 120000 first")
applyEngineAndAgentEnv(env, workflowData, nil)
assert.Equal(t, "90000", env["GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS"], "engine.env override should win")@copilot please address this. |
||
| applyEngineAndAgentEnv(env, workflowData, nil) | ||
|
|
||
| assert.Equal(t, "90000", env["GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS"]) | ||
| }) | ||
| } | ||
|
|
||
| func TestBuildStandardNpmEngineInstallStepsNoCooldown(t *testing.T) { | ||
| steps := BuildStandardNpmEngineInstallStepsNoCooldown( | ||
| "@github/copilot", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design]
watchdog-timeoutuses seconds while every otherengine.harnessfield uses milliseconds (initial-delay-ms,max-delay-ms,backoff-multiplier). This unit inconsistency is a user-facing footgun.💡 Discussion
All sibling fields in the same
engine.harnessblock use milliseconds. A user copying the retry-policy pattern might writewatchdog-timeout: 120000expecting 120 s, but would get 33 hours.Consider:
watchdog-timeout-ms(aligns with the injected env varGH_AW_HARNESS_WATCHDOG_TIMEOUT_MS) — no s→ms conversion needed, no unit mismatch.watchdog-timeout-secondsorwatchdog-timeout-sto make the unit explicit.If the seconds choice is intentional (human-friendly), the docs and schema description should explicitly warn about the unit difference from the other fields.
@copilot please address this.