Skip to content

fix(pl-middle-layer): stop the render test racing a cold block fetch - #1837

Open
DenKoren wants to merge 2 commits into
chore/build-on-windowsfrom
fix/render-test-flaky-timeout
Open

DenKoren wants to merge 2 commits into
chore/build-on-windowsfrom
fix/render-test-flaky-timeout

Conversation

@DenKoren

@DenKoren DenKoren commented Sep 17, 2026 •

Copy link
Copy Markdown
Member

Stacked on #1836 (chore/build-on-windows) — review that one first; this branch
only adds the commits on top. Base retargets to main once #1836 merges.

Fixes the intermittent test JS render enter numbers failure seen on #1836,
which is a pre-existing flake on main, not something that branch introduced.

Decisions

The bug is the scope of the abort signal, not its value. awaitBlockDone
builds one AbortSignal.timeout(timeout) outside the while loop and reuses
it for every overview.awaitChange(...). So the value is the budget for the
entire wait, not for one change notification — a name that reads like a per-wait
timeout but behaves like a deadline. Both copies of the helper have this shape.

The old budgets could not cover the work they were waiting on. The live
caller in render.test.ts adds a block by from-registry-v1 from
https://block.registry.platforma.bio/releases and then runs it, so its 2000ms
had to cover a cold remote fetch plus execution on a CI runner. That is a race
against the network, which is why it failed intermittently and why the failure
looked like nothing in particular.

Both defaults are now 10s. render.test.ts was 2000ms and
DEFAULT_AWAIT_BLOCK_DONE_TIMEOUT in sdk/test was 5000ms. 10s sits well
inside the 80s testTimeout of the middle-layer suite, so a genuinely stuck
block still fails the test rather than hanging it, and callers that need longer
keep passing their own value (15s–50s across the repo).

Raised the defaults rather than restructuring the helper. A progress
watchdog — reset the deadline on each observed change, with an overall cap —
buys nothing here: during the fetch there is no overview change to reset on,
which is exactly the interval that has to be tolerated, so the idle bound would
have to be just as generous as the total. It would also be a new pattern in a
repo that consistently uses a single overall budget.

The abort message now says what was pending. It was a bare "Aborted.",
which is what made the CI failure take a log dig to identify. It now names the
block, its last calculationStatus and the budget, and dumps blockOverview
beside the block state — matching what the sdk/test copy already prints.

Key files to review

  1. lib/node/pl-middle-layer/src/middle_layer/render.test.ts — the signal's
    scope is the whole point, so read the comment against the loop below it.
  2. sdk/test/src/test-block.ts — same shape, published package; the default
    here is exposed to every block test that does not pass its own timeout.
  3. .changeset/await-block-done-default-timeout.md — patch bump for
    @platforma-sdk/test.

Verification

types:check, formatter:check and linter:check pass on both packages. I
have not reproduced the flake locally — it needs a live backend and a cold
registry fetch, and a green local run would not be evidence either way for a
timing race. The claim rests on the read of the code: a 2s deadline spanning a
network download, with every sibling caller at 15s or more.

…block fetch

awaitBlockDone builds one AbortSignal.timeout outside the loop, so its
argument is the budget for the whole wait, not for a single change
notification. The default was 2000ms, and the only live caller waits on a
block that is first downloaded from the remote registry and then run. Two
seconds is a race against the network, so 'test JS render enter numbers'
failed intermittently in CI with a bare 'Aborted.'

Raise the default to 30000ms. Every other caller of this helper in the repo
already passes 15-50s for the same reason; 2000 was the outlier, and the
file's test timeout is 80s, so the wait still fails well inside it.

The abort message now names the block, its last status and the budget, and
dumps the overview next to the state, so the next timeout says what was
still pending instead of 'Aborted.'
@changeset-bot

changeset-bot Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 6060a2c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@platforma-sdk/test Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment on lines +40 to +41
* download on a CI runner. Other callers of this helper across the repo pass
* 15-50s for the same reason.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Misleading caller description

The comment says other callers of “this helper” pass 15–50-second timeouts, but this helper’s only active caller uses the default. The similarly named helpers elsewhere are separate implementations. Describing them as analogous helpers would make the timeout rationale accurate and easier to verify.

Suggested change
* download on a CI runner. Other callers of this helper across the repo pass
* 15-50s for the same reason.
* download on a CI runner. Analogous helpers elsewhere in the repo use
* 15-50s for the same reason.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/node/pl-middle-layer/src/middle_layer/render.test.ts
Line: 40-41

Comment:
**Misleading caller description**

The comment says other callers of “this helper” pass 15–50-second timeouts, but this helper’s only active caller uses the default. The similarly named helpers elsewhere are separate implementations. Describing them as analogous helpers would make the timeout rationale accurate and easier to verify.

```suggestion
 * download on a CI runner. Analogous helpers elsewhere in the repo use
 * 15-50s for the same reason.
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Claude Code

Both copies of the helper build one AbortSignal.timeout outside the wait
loop, so the value is the budget for the whole wait rather than for a single
change notification, and it has to fit a block that is fetched before it runs.

render.test.ts was at 2000ms and sdk/test at 5000ms; both are a race on a
cold runner for a caller that passes no timeout of its own. Set both to 10s.
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