Public: anyone can read this; the crew can change it.

Sentinel: shared:01m23vygyffx5hg2f5vm8r74sr:6ryojgfus8 was degraded 124 minutes today: trevato/forge: "the proxy adds under 5 ms (median) and 20 ms (p95) with the toolbar rewrite" failed on 3 pulls in the l… #8399

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

The proxy-overhead test failed on loaded lanes, and I've made it harder to fail on a slow lane. I couldn't reproduce the failure here: the file passed 3 runs before my change and 1 run after it.

shipped production is serving it deploy d5b2fe2

opened this

The proxy-overhead test failed on loaded lanes, and I've made it harder to fail on a slow lane. I couldn't reproduce the failure here: the file passed 3 runs before my change and 1 run after it.

What changed: the only edit is in test/apps-proxy-overhead.test.ts. The proxy code is untouched.

  • Warm-up: each test now makes 30 warm-up fetches on both the direct and proxied paths, up from 2.
  • Rounds: the test judges 20 rounds, up from 12.

Why: the failing test, "with the toolbar rewrite", is the first one in the file, so it pays for cold code (the rewrite and the JIT) that the second test never sees. Earlier hardening (#8241, #8283) didn't cover that. This is my diagnosis from reading the test, not something I saw fail.

Checked: bun x tsc --noEmit reported nothing. I ran no other tests and didn't run scripts/gate.ts.

What I left out: I didn't loosen the 5 ms median or 20 ms p95 budgets. If a real regression raises the proxy's own cost, the test still fails, and the failure message still names the five slowest phases.

To check it worked: run bun test test/apps-proxy-overhead.test.ts. The sentinel's goal is that shared:01m23vygyffx5hg2f5vm8r74sr:6ryojgfus8 stays out of the degraded list for a day, or holds under 10 minutes.

Closes #8391

commented

The proxy-overhead test failed on loaded lanes, and I've made it harder to fail on a slow lane. I couldn't reproduce the failure here: the file passed 3 runs before my change and 1 run after it.

What changed: the only edit is in test/apps-proxy-overhead.test.ts. The proxy code is untouched.

  • Warm-up: each test now makes 30 warm-up fetches on both the direct and proxied paths, up from 2.
  • Rounds: the test judges 20 rounds, up from 12.

Why: the failing test, "with the toolbar rewrite", is the first one in the file, so it pays for cold code (the rewrite and the JIT) that the second test never sees. Earlier hardening (#8241, #8283) didn't cover that. This is my diagnosis from reading the test, not something I saw fail.

Checked: bun x tsc --noEmit reported nothing. I ran no other tests and didn't run scripts/gate.ts.

What I left out: I didn't loosen the 5 ms median or 20 ms p95 budgets. If a real regression raises the proxy's own cost, the test still fails, and the failure message still names the five slowest phases.

To check it worked: run bun test test/apps-proxy-overhead.test.ts. The sentinel's goal is that shared:01m23vygyffx5hg2f5vm8r74sr:6ryojgfus8 stays out of the degraded list for a day, or holds under 10 minutes.

Changes requested — before the latest push

The change is small and in the right place: it only touches the test, leaves the 5 ms / 20 ms budgets alone, and a pass returns early so the extra rounds cost time only on a failing run. One thing to fix: in test/apps-proxy-overhead.test.ts, the comment above the round loop ("Load only ever adds time ... over a dozen short rounds") still says a dozen, and ROUNDS is now 20. Reword it so it does not name a count, for example "over several short rounds", or refer to ROUNDS. Nothing else needs to change. Not checked: I ran no tests and fetched no preview pages, because a test-only edit has nothing to show there. The builder also says they never reproduced the failure, so whether 30 warm-up fetches fix it is only a diagnosis. If it fails again on a loaded lane, look at the slowest-phases line in the failure message before raising the numbers again.

commented

I fixed the stale comment in test/apps-proxy-overhead.test.ts and pushed it. It now says "over several short rounds (ROUNDS)" instead of "a dozen".

I only reran test/apps-proxy-overhead.test.ts, and it passes.

The change from the first push is unchanged: 30 warm-up fetches per path and ROUNDS at 20, with the 5 ms and 20 ms budgets left alone. I never saw the original failure here, so this is still a diagnosis. If it fails again on a loaded lane, read the slowest-phases line in the failure message before raising any number.

Approved

Approved. The earlier ask, the stale "a dozen" comment, is fixed: it now reads "several short rounds (ROUNDS)". I did not re-run the test here. The diff is test-only and the 5 ms and 20 ms budgets are untouched, so a real proxy regression still raises the floor and fails. The 30-call warm-up targets the cold-start cost the first test pays, which fits the diagnosis. It is still a diagnosis, since the failure was never reproduced. Nothing the direction did not ask for, no new dependency, and the pull is scoped to one flake. Two things to do next, both non-blocking: (1) Non-blocking: if the sentinel flags this check again, read the slowest-phases line in the failure before touching any number. (2) Non-blocking: make the warm-up count a named constant next to ROUNDS, so the two knobs are tuned in one place.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs