Skip to content

fix(cli): load telemetry without the core and engine barrels, and stop the history test racing - #5153

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/history-test-waits-for-lock-try
Oct 7, 2026
Merged

miguel-heygen merged 2 commits into
mainfrom
fix/history-test-waits-for-lock-try

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

What

Fixes the Windows timeout in history.test.ts, "a CLI run waits while another process holds the history". It hit vitest's 20 s limit once on the engine-cli Windows job. Three changes:

  1. The test waits for the CLI to actually find the history held, instead of sleeping 1.5 s. It watches the history folder for the CLI's lock claim draft (owner.pid-<uuid>.tmp), armed before the child starts, then checks nothing was written and releases. If the CLI exits before it tries the lock, the test fails at once with its exit code instead of waiting out the timeout, and the child is killed when the test ends. Under CPU load the child used to reach the lock only after the test had already released it. It then took the lock on its first try, and the test passed without testing any waiting: 30 of 30 runs on a runner with 8 busy loops on 4 cores.
  2. CLI telemetry imports leaf subpaths instead of the @hyperframes/core and @hyperframes/engine barrels. Every command loads telemetry. events.ts pulled the whole core barrel (parsers, Babel, recast, linkedom, esbuild) for one string redaction function, and system.ts pulled the engine barrel (puppeteer-core included) for one memory reading. New subpaths, declared in each package's package-subpaths.json with the manifests regenerated: @hyperframes/core/telemetry-redaction and @hyperframes/engine/system-memory, plus the CLI bundle alias for the engine one. telemetry/canary.ts already states this rule for the startup path.
  3. The history command imports the existing @hyperframes/studio-server/history subpath instead of that package's barrel.

Why it timed out

The test's run time was 1.5 s plus a cold bun run src/cli.ts history. Measured on Windows runners, with a trace inside the child:

  • The child's start-up was mostly loading the command's module graph from TypeScript source. It took about 0.7 s on an idle runner, 4 to 5 s at twice the core count in CPU load, and 15 to 19 s when starved, with 4 to 8 s outliers even on an idle runner.
  • The lock was never the problem. Releasing it took at most 357 ms in about 170 samples, and no child ever threw, hit EPERM, or stayed alive after its work.
  • The reproduced timeout had the CI failure's shape: the test died waiting for the child to exit, and the child was still loading.

Changing the wait alone removes the race but not the cost: under starvation the fixed test still timed out, because the child had not yet tried the lock. The leaf imports remove most of that cost.

Proof

Windows runner main wait-for-lock-try only this PR
idle, 20-30 runs 1.6 s median, 9.3 s max 0.8 s median, 0.9 s max 0.28 s median, 5.3 s max
8 busy loops on 4 cores passes, but never tests waiting (30/30) 6.6 s median, always tests waiting not measured
  • Linux: importing the history command takes 26 ms instead of 149 ms (median of 9), and the two telemetry modules 14 ms instead of 171 ms.
  • history.test.ts and the telemetry tests: 348/348, 3 runs in a row. A CLI that crashes on start now fails the test in about 3 s with "the CLI exited (1) before trying the held lock". Typecheck passes for the CLI and core, the subpath check and its tests pass, the CLI bundle builds and inlines both subpaths, and node dist/cli.js history --help runs.

Not covered

  • Under CPU load the leaf imports could not be measured: at 24 busy loops on 4 cores, vitest itself took over an hour to load the test file. The idle Windows gain is the evidence for them.
  • On macOS, file watching can report a file removed a few milliseconds before the watcher started, so on a fast Mac the test's own lock claim can satisfy the wait. The final log check (one baseline, one entry) still proves the CLI did not fork the log. CI runs this test on Linux and Windows only.

Size

Under the 100-line floor on purpose: this fixes a flaky CI test on main on its own, so it can land and revert independently of any feature PR.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Edit accuracy: accurate 2059 (base branch 2059), smooth 1581 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

  • crop-none-px-r0-nested-z200: tracking 0.02, pressJump 0, drop 40.03, reload 40.05, render 40.03, renderKey -, undo true, teleport true / tracking 0.02, pressJump 0, drop 0.04, reload 0.06, render 0.03, renderKey -, undo true, teleport true / tracking 0.02, pressJump 0, drop 0.04, reload 0.06, render 0.03, renderKey -, undo true, teleport true

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 7, 2026 04:59

@jrusso1020 jrusso1020 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.

Approve at 51f5c36e.

  • The test fix is the right shape. Waiting for the CLI's owner.pid-* claim draft proves the CLI actually tried the held lock, which the 1.5 s sleep never proved. The race against exited turns a crash on start into an immediate failure instead of a timeout. Both onTestFinished cleanups cover the watcher and the child.
  • The leaf imports are real leaves. I checked at this head: core/src/telemetryRedaction.ts has no imports at all, and engine/src/services/systemMemory.ts imports only fs and os. So telemetry no longer drags in the core and engine barrels. @hyperframes/studio-server/history already existed, so that part is reuse, not a new surface.
  • The subpath entries match their neighbours. package-subpaths.json is the source and the manifests are regenerated. The engine entry carries no node condition, like the other engine subpaths, and the bundle alias is added for it.
  • CI is green.

Non-blocking: the macOS caveat in the body (the watcher can see the test's own claim) is fine because CI runs this test on Linux and Windows only, and the log assertion still holds there.

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 7, 2026
Merged via the queue into main with commit 9fbec22 Oct 7, 2026
177 of 178 checks passed
@miguel-heygen
miguel-heygen deleted the fix/history-test-waits-for-lock-try branch October 7, 2026 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants