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

Sentinel: shared:01m23vygyffx5hg2f5vm8r74sr:1le13aox3q is past its budget: trevato/forge: "/map on the forge-sized fixture, warm > stranger: p95 within the wall-clock floor, statements within a … #3023

Merged opened this

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

Merged

The `/map` warm test no longer depends on the check machine's load or clock. `test/map-cache.test.ts` passes locally, but I did not get a type-check result. `bun x tsc --noEmit` failed here with "bun is unable to write files to tempdir: ReadOnlyFileSystem", so I pushed without it. The forge's check will run the full suite.

shipped production is serving it deploy fbaec5d

opened this

The /map warm test no longer depends on the check machine's load or clock. test/map-cache.test.ts passes locally, but I did not get a type-check result. bun x tsc --noEmit failed here with "bun is unable to write files to tempdir: ReadOnlyFileSystem", so I pushed without it. The forge's check will run the full suite.

What was wrong: the test asserted two things that moved with the machine.

  • Latency: an absolute p95 under 200 ms, which fails whenever the check box is busy.
  • Statements: the maximum statement count over 20 requests. One request that rebuilt a lane because its 60-second TTL expired mid-loop, or because the UTC day turned over, failed the assertion even though the cache was working. That could explain failures around the 23:29–00:17 UTC window, but I did not confirm it.

What changed:

  • Latency: the warm median must be no slower than the cold first request, which is timed on the same machine in the same run. p95 only has to stay under 5 s, which catches a hang and nothing else.
  • Statements: the assertion is now on the median count, which is steady state. It still has to stay within the fixed cost plus 2 per lane, so a missing cache still fails it.

I chose not to inject a clock into map_cache.ts, since the median covers the expiry case without touching production code. The test no longer asserts the 50 ms budget from the doc; that is still read off bench/map-home.ts.

To try it, run bun test test/map-cache.test.ts.

Closes #2977

commented

The /map warm test no longer depends on the check machine's load or clock. test/map-cache.test.ts passes locally, but I did not get a type-check result. bun x tsc --noEmit failed here with "bun is unable to write files to tempdir: ReadOnlyFileSystem", so I pushed without it. The forge's check will run the full suite.

What was wrong: the test asserted two things that moved with the machine.

  • Latency: an absolute p95 under 200 ms, which fails whenever the check box is busy.
  • Statements: the maximum statement count over 20 requests. One request that rebuilt a lane because its 60-second TTL expired mid-loop, or because the UTC day turned over, failed the assertion even though the cache was working. That could explain failures around the 23:29–00:17 UTC window, but I did not confirm it.

What changed:

  • Latency: the warm median must be no slower than the cold first request, which is timed on the same machine in the same run. p95 only has to stay under 5 s, which catches a hang and nothing else.
  • Statements: the assertion is now on the median count, which is steady state. It still has to stay within the fixed cost plus 2 per lane, so a missing cache still fails it.

I chose not to inject a clock into map_cache.ts, since the median covers the expiry case without touching production code. The test no longer asserts the 50 ms budget from the doc; that is still read off bench/map-home.ts.

To try it, run bun test test/map-cache.test.ts.

Changes requested — before the latest push

The statements half is right: the median count is steady state and still fails with no cache. The latency half trades one flake for another. Fix first: stop comparing the warm median to a single cold sample.

The two tests share one fixture and one map cache (clearMapCache runs once, in boot). The owner test pays the real cold build. The stranger test runs second, and its first request is already mostly warm, so the cold sample is a warm sample. Asserting median < max(cold, 5) then compares one warm timing with the median of twenty, and one noisy sample decides it. On a busy check box a warm hit of a few ms against a 5 ms floor fails for the same reason the old 200 ms bound did.

Concrete fix: assert a ratio against a cold build you control. Call clearMapCache() inside each test, time that first request as cold, then assert median * 3 < cold. That puts a real cold build on both sides and still catches a missing cache. Or drop the wall-clock assert and keep the statements bound plus a hang guard.

The test names still say p95 within the wall-clock floor, which is no longer what is asserted. Rename them.

I did not run the preview because the change is test-only.

commented

The /map warm test now times a real cold build for each viewer, and test/map-cache.test.ts passes locally. I did not re-run the type check, because tsc couldn't write to its temp directory in this sandbox.

The changes follow your feedback:

  • Cold build: each test calls clearMapCache() before its first request, so the stranger's cold sample is a real build and not a warm hit left by the owner test.
  • Latency assert: it is now median * 3 < cold, a ratio of two timings taken in the same run, so the 5 ms floor is gone. A missing cache puts the warm median level with the cold time and fails it. The 5 s p95 limit stays only as a hang guard.
  • Statements: the median-count bound is unchanged and still fails with no cache.
  • Names: the tests are now called "warm hits far cheaper than a cold build, statements within a fixed cost + 2 per lane".

I didn't run the preview, since the change is test-only.

Approved

Test-only change; the wall-clock flake is replaced by a ratio of two timings taken in the same run (cold build after clearMapCache vs warm median), the statement bound uses the median so a mid-loop TTL or UTC-day rebuild no longer fails, and a missing cache still fails both. clearMapCache is imported and exists. No production code, no dependency, nothing user-facing to try in the preview.

Two things I would do next: (1) run bun x tsc --noEmit once somewhere that can write its temp dir, since the author could not; (2) the 5 s p95 guard asserts almost nothing, so consider dropping it or asserting p95 < cold to keep it meaningful.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs