Skip to content

fix(runner): bound the networkidle wait; name the assertion timeout - #85

Merged
myselfsiddharth merged 1 commit into
mainfrom
track1/b4-bounded-waits
Jul 28, 2026
Merged

fix(runner): bound the networkidle wait; name the assertion timeout#85
myselfsiddharth merged 1 commit into
mainfrom
track1/b4-bounded-waits

Conversation

@myselfsiddharth

Copy link
Copy Markdown
Contributor

Came out of a founder question — "if this ships to a lot of people, will it be super laggy
because of Playwright?"
Playwright is not the cost: PRD non-goal #1 puts Chromium under the
baseline too, so Paragent removes model round-trips from a loop that already had a browser. But
reading the runner to answer that turned up one unbounded wait and one policy hardcoded in
seven places.

1. The unbounded networkidle — a 30s worst case nobody chose

src/runner/actions.ts — a wait step with no positive duration
called page.waitForLoadState("networkidle") with no timeout, inheriting Playwright's 30s
default. If the page never goes quiet for 500 ms the step burned all 30 s and then failed
anyway
: maximum latency for zero information.

Now bounded by NETWORK_IDLE_WAIT_MS (5000 ms), overridable per run via
ReplayRunnerOptions.networkIdleWaitMs. Measured against a page that never reaches idle
(tests/unit/runner-bounded-wait.test.ts):

unbounded (before) bounded (after)
default 30.8 s 5.0 s
1 s override 30.0 s 1.0 s

I checked my own assumption and it was wrong

I assumed Grafana would trigger this, since dashboards auto-refresh. It does not.
networkidle settles on /d/paragent-seed in ~3 ms (measured live on 11.0.0, 2026-07-28)
— the seed dashboard sets no refresh interval and TestData is generated client-side.

So this is a latent worst case, reachable on any surface with continuous polling, streaming
or websockets, not a hang observed on the current test-bed. Bounding it is cheap insurance
taken before the gate runs, not a fix for a live symptom. Recorded that way in the code comment
and in docs/gate/runner.md rather than left as the more flattering claim.

⚠️ This changes which steps pass

A page that first goes quiet at 12 s satisfied the wait before and does not now. That is
deliberate, and it is cheap today precisely because no gate number exists — gate:matrix is
dry-run only (#62). After a published measurement, the same change would be an expensive silent
shift.

2. timeout_ms named, deliberately not moved

Seven timeout_ms: 5000 literals in src/compiler/assertions.ts
became one DEFAULT_ASSERTION_TIMEOUT_MS, overridable via CompileOptions.assertionTimeoutMs.

The value is unchanged, on purpose. A timeout is an assertion-strength knob, not a
performance one: shorter is stricter. Lowering it to make replay feel faster would raise the
failure rate and move step-level replay-validity — the number PRD §9 gates on — while looking
like a tuning change. That is exactly the shape the assertion-immutability invariant forbids.

It is still the dominant term in worst-case latency, because the runner spends the full budget
on failure: a 12-step task with three stale locators waits 3 × 5 s before repair even
starts. Worth revisiting — after a measurement, not before one.

tests/unit/compiler.test.ts now pins the emitted default and the override. Nothing pinned it
before; the only timeout_ms values in tests/ were hand-written 1000 fixtures.

Verification

Artifacts byte-identical, as an unchanged default requires:

npm run compile -- --in contracts/examples/trajectory.example.json \
  --out artifacts/compiled/traj-example-grafana-login-nav.bundle.json
git diff --stat artifacts/     # no change

Guard proven to bite. Reverting the bound to the unbounded call:

× gives up at the configured bound instead of Playwright's 30s default   30024ms
    AssertionError: expected 30018 to be less than 10000
× honours the default bound when no override is supplied                 30762ms
    AssertionError: expected 30761 to be less than 10000
✓ pins the default bound
✓ still uses a plain sleep when the step carries a positive duration

They fail on the clock, not because both sides were edited together.

Filed, not folded in

Both need a contract decision, so they are issues rather than scope creep:

Merge note

Test-merged against all five other open branches. #79, #80, #82 merge clean. #78 and #81
conflict only on the updated: frontmatter date line
in docs/gate/runner.md (and
compiler.md for #78) — 2026-07-27 vs 2026-07-28, no substantive overlap. Resolution: keep the
later date.

Tests

npm run ci            # green — 51 unit (6 new), 1 integration, secret-scan clean
npm run test:canary   # 6 pass

🤖 Generated with Claude Code

@myselfsiddharth
myselfsiddharth requested a review from a team as a code owner July 28, 2026 07:35
@github-actions
github-actions Bot requested a review from OM152002 July 28, 2026 07:35
@github-actions github-actions Bot added documentation Improvements or additions to documentation gate PRD section 9 gate measurement area: compiler Touches compiler area: runner Touches runner size/L <= 600 changed lines labels Jul 28, 2026

@OM152002 OM152002 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bounding the wait is right and the verification is solid. One design question that I think matters more than the bound itself, and one stale docstring.

Verified

  • Artifacts byte-identical. Re-ran the compile against contracts/examples/trajectory.example.json; git diff --stat artifacts/ is empty. That is the claim an unchanged default has to make, and it holds.
  • The bounds are real. 1003ms and 5002ms on the never-idle page — the tests measure the clock, not a constant, so they would catch a removed bound.
  • Merge claims mostly accurate. Clean against main, #80 and #82; date-only conflict with #81, confirmed. But #78 is not date-only — it conflicts on both docs/gate/runner.md and docs/gate/compiler.md, and #78 is already approved and ahead of this in the queue.

A timeout here is still a step failure, and that feeds the repair loop

replay.ts:130 routes any non-PASS outcome into the repair loop, so a networkidle timeout is not just faster now — it is a failed step that spends repair budget. On exactly the surfaces this PR names as the risk (continuous polling, streaming, websockets), a parameterless wait will now fail deterministically at 5 s, get proposed a corrected_action twice, and land on REPAIR_EXHAUSTED. No action repair can fix "this page never goes quiet", so the budget is spent on a step that cannot be repaired, and the run's success_with_le_2_repairs reflects a scaffolding condition rather than churn.

The PR cites the right precedent and then does not apply it. page-state.ts:69 uses a 250 ms bound whose timeout means no claimnetwork_idle = false, "leave it false rather than inventing success" — not failure. A parameterless wait is a settling hint, not a post-condition; the post-condition is the assertion that runs immediately after with its own 5 s budget. Treating idle-not-reached as "proceed, note it" rather than TIMEOUT would keep the latency win and stop it consuming repairs.

Not asking you to change it in this PR necessarily — but the outcome classification deserves a decision on the record, because right now the bound makes a doomed step fail 6× faster without making it any less doomed.

The test file still carries the retracted claim

tests/unit/runner-bounded-wait.test.ts header:

On a page that never goes quiet (a Grafana dashboard auto-refreshes, which is the entire test-bed)

The PR body, actions.ts and docs/gate/runner.md all correct precisely this — Grafana does not trigger it, networkidle settles in ~3 ms. The correction landed in three places and missed the fourth, and the test file is where the next reader goes to learn why the test exists. Given "I checked my own assumption and it was wrong" is a section heading in this PR, the one file still asserting the wrong assumption should be fixed.

Minor

NETWORK_IDLE_WAIT_MS and DEFAULT_ASSERTION_TIMEOUT_MS are both 5000, and the comment leans on them matching — but they are independent constants in different packages with nothing tying them together. Fine as-is; just don't let a later reader think the equality is enforced.

Naming the seven 5000 literals and refusing to move the value is the right instinct, and the reasoning — that a timeout is an assertion-strength knob and lowering it would move replay-validity while looking like a perf tweak — is the correct read of the invariant. Pinning the emitted default so it cannot drift silently is the part that will actually hold the line.

Two latency defects found while answering "will a shipped Paragent feel laggy
because of Playwright?" Playwright is not the cost — PRD non-goal #1 puts
Chromium under the baseline too — but reading the runner turned up one unbounded
wait and one policy hardcoded seven times.

1. A `wait` step with no positive duration called
   page.waitForLoadState("networkidle") with NO timeout, inheriting Playwright's
   30s default. Nobody chose 30s. If the page never goes quiet for 500ms the
   step burned all 30s and then failed anyway: maximum latency for zero
   information. Now bounded by NETWORK_IDLE_WAIT_MS (5000), overridable via
   ReplayRunnerOptions.networkIdleWaitMs.

   Measured against a page that never reaches idle:
     unbounded  default 30.8s   1s-override 30.0s
     bounded    default  5.0s   1s-override  1.0s

   HONEST SCOPE: the seeded Grafana dashboard does NOT trigger this —
   networkidle settles there in ~3ms (measured on 11.0.0, /d/paragent-seed),
   because the seed dashboard sets no refresh interval and TestData is generated
   client-side. I assumed it would and checked; it does not. This is a latent
   worst case reachable on any polling/streaming surface, not a hang observed on
   the current test-bed. Recorded that way in docs/gate/runner.md.

   REACHING THE BOUND IS NOT A STEP FAILURE. A parameterless wait is a settling
   hint; the post-condition is the assertion that runs right after it with its
   own budget. So the step proceeds and records settled:false — the posture
   page-state.ts already used for its 250ms probe, where a timeout means "no
   claim" rather than failure. Classifying it as TIMEOUT would route the step
   into the repair loop (replay.ts sends every non-PASS outcome there), spending
   both repair attempts on a condition no corrected_action can fix: "this page
   never goes quiet" is not a locator problem. success_with_le_2_repairs would
   then report a scaffolding condition as churn. Bounding it that way would have
   made a doomed step fail 6x faster without making it any less doomed.

   Nothing is hidden by proceeding: if the page is broken the assertion fails on
   its own evidence, and that failure IS worth a repair attempt. A step that
   genuinely wants idle as its post-condition has the `network-idle` assertion
   type, where a timeout is a real failure because it was a real claim.

   settled:false surfaces as StepAttemptResult.notes — deliberately not
   error_message, since the step can be PASS. In-memory only: metrics.schema.json
   has no field for it, so nothing aggregates how often a hint went unanswered.
   Contract change with no measurement to shape it yet; recorded as an open
   question rather than guessed at.

   This still CHANGES WHICH STEPS PASS: a page first going quiet at 12s held the
   step until it did, and now the step continues at 5s and the assertion decides.
   Deliberate, and cheap today because no gate number exists (#62 keeps
   gate:matrix dry-run only). After a published measurement it would be an
   expensive silent shift.

2. Seven `timeout_ms: 5000` literals in src/compiler/assertions.ts became one
   named DEFAULT_ASSERTION_TIMEOUT_MS, overridable per compile via
   CompileOptions.assertionTimeoutMs. THE VALUE IS DELIBERATELY UNMOVED.

   A timeout is an assertion-strength knob, not a perf knob: shorter is
   stricter. Lowering it to make replay feel faster would raise the failure rate
   and move step-level replay-validity — the number PRD §9 gates on — while
   looking like a tuning change. Naming it is the fix; choosing a different
   number should follow a measurement, not precede one. tests/unit/compiler.test.ts
   now pins the emitted default, which nothing did before.

   NETWORK_IDLE_WAIT_MS and DEFAULT_ASSERTION_TIMEOUT_MS are both 5000 by
   coincidence, not by construction — independent constants in different
   packages with nothing enforcing the match. Said so in both comments so a
   later reader does not infer a coupling that is not there.

Artifacts regenerate byte-identical, as the unchanged default requires.

Guard proven, both halves: reverting the bound makes the timing assertions fail
at 30.0s/30.8s, and reverting the classification makes the runner-level test fail
with repair_count 1 instead of 0. Neither passes because both sides moved
together.

Filed rather than folded in, both needing a contract decision: #83 (the recorder
drops a wait step's duration, so replay does networkidle instead of the recorded
sleep — latent, no committed artifact contains a wait step) and #84 (no run-level
wall-clock budget; maxRepairsPerRun caps count, not time).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@myselfsiddharth
myselfsiddharth force-pushed the track1/b4-bounded-waits branch from 97300d3 to 5f37c47 Compare July 28, 2026 16:11
@myselfsiddharth

Copy link
Copy Markdown
Contributor Author

Pushed 5f37c47 — rebased onto main @ 3b6d4ae (conflicts gone) and took the design point rather than just recording it.

The classification is changed, not just documented

You were right that this is the part that matters. A parameterless wait is a settling hint; the post-condition is the assertion that runs right after it with its own budget. So reaching the bound now proceeds and records settled: false instead of returning TIMEOUT — the same posture as page-state.ts:69, where a timeout means no claim.

The reasoning went in the code, not only the docs: routing it into the repair loop spends both attempts on something no corrected_action can fix, and success_with_le_2_repairs ends up reporting scaffolding as churn. Nothing is hidden by proceeding — if the page is broken the assertion fails on its own evidence, and that failure is worth a repair attempt. A step that genuinely wants idle as a post-condition already has the network-idle assertion type, where a timeout is a real failure because it was a real claim.

settled: false surfaces as StepAttemptResult.notes — deliberately not error_message, since the step can be PASS. It is in-memory only: metrics.schema.json has no field for it, so nothing aggregates how often a hint went unanswered across a matrix run. That is a contract change with no measurement to shape it yet, so it is an open question in docs/gate/runner.md rather than a guess.

New test, and it bites: does not spend repair budget on a page that never goes quiet — one wait step against the never-idle server, asserting repair_count: 0, task_success: true, and the note still recorded. Reverting the classification fails it with expected 1 to be +0, alongside the two timing assertions failing on settled. Also added a positive control (settled: true on a quiet page), because otherwise false could be a constant.

Stale claim — fixed

The test-file header no longer says Grafana is the never-idle case. It now carries the same correction as the other three places (settles in ~3 ms on /d/paragent-seed, worst case is latent) and states what each test is actually instrumenting.

Minor — taken

Both comments now say the 5000/5000 equality is coincidence: independent constants in two packages, nothing enforcing the match, either can move alone. Also added as an open question that neither number has been fitted to an observation.

Merge

Rebased onto main, which now contains #78, #80, #81 and #82 — so the pre-merge conflict analysis in the PR body is superseded rather than corrected. Against current main the only conflicts were the updated: frontmatter lines in docs/gate/runner.md and docs/gate/compiler.md (2026-07-27 vs 2026-07-28, kept the later); the substantive #78 content in both files auto-merged. CONFLICTINGMERGEABLE.

Re-verified after all of it

npm run ci green (109 unit + 1 integration, lint-docs: clean), test:canary 6 pass, and artifacts still regenerate byte-identical (git diff --stat artifacts/ empty) — the compiler side did not move.

🤖 Generated with Claude Code

@myselfsiddharth
myselfsiddharth merged commit 080d600 into main Jul 28, 2026
13 checks passed
@myselfsiddharth
myselfsiddharth deleted the track1/b4-bounded-waits branch July 28, 2026 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: compiler Touches compiler area: runner Touches runner documentation Improvements or additions to documentation gate PRD section 9 gate measurement size/L <= 600 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants