Skip to content

fix: stop two merge-queue flakes in the history prune and timeline tests - #4786

Merged
miguel-heygen merged 5 commits into
mainfrom
fix/prune-history-test-clock
Sep 30, 2026
Merged

miguel-heygen merged 5 commits into
mainfrom
fix/prune-history-test-clock

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

What

Two merge-queue flakes, each fixed at its root.

1. History prune tests (studio-server). The history prune tests compute "14 days later" as Date.now() + KEEP_GONE_PROJECT_HISTORY_MS, and at four places expect a history last used just before that to be pruned. They now use pastKeepWindow(), which adds 1 ms.

2. Timeline scroll-end timer (studio). @tanstack/react-virtual goes from 3.14.6 to 3.14.13, which brings @tanstack/virtual-core from 3.17.4 to 3.17.11. virtual-core 3.17.8 made the scroll-end debounce cancel on cleanup. Before that, a scroll within 150 ms of unmount left a timer that later called notify, and so a React update. useTimelineVirtualRows dispatches a scroll whenever the virtualizer becomes ready.

Why

1. Date.now() rounds down to the millisecond. A history's last use is a file's mtime, which keeps sub-millisecond precision. When the test writes project.json and reads Date.now() in the same millisecond, the mtime can be a fraction of a millisecond after "now". now - lastUsed then comes out just under 14 days, the history is kept, and the test gets []. That is what failed in merge-group runs 36725492503 and 36750393590 (pruneHistories.test.ts:231, expected [ id ], received []).

Real users are not affected by 1. A last use a fraction of a millisecond in the future keeps a gone project's history for that fraction longer, which is the safe direction.

2. In run 36752435633 (job 110014311398, on #4761, which does not touch this code), all 6217 studio tests passed, but vitest failed the shard on an unhandled ReferenceError: window is not defined. The stack runs from virtual-core's debounce timeout (utils.js:65), through observeOffset's scroll-end callback and Virtualizer.notify, to react-virtual's onChange and react-dom's requestUpdateLane. That timer fired after timelineStackingSyncExport.test.tsx had finished and its window was torn down. For users, the leftover timer only schedules an update on an unmounted tree.

How

  • One helper, pastKeepWindow(), at the top of the test, used by all four places that expected a prune at exactly 14 days (lines 108, 181, 209, 230 on main).
  • packages/studio/package.json and bun.lock: the react-virtual bump. Only the two TanStack lock entries and the specifier change.
  • useTimelineVirtualRows.test.tsx: a new test builds a Virtualizer on a scroll element, mounts it, dispatches a scroll, unmounts, waits 200 ms (past virtual-core's 150 ms scroll-end delay) and expects no onChange after unmount.
  • No studio or studio-server source changes. The upgrade also brings virtual-core's other 3.17.5 to 3.17.11 fixes, including how the scroll position is corrected when the focused row resizes; this PR's full studio suite is the check that nothing we rely on moved.

Test plan

On a hosted ubuntu-24.04 runner (kernel 6.17.0-1022-azure), in a temporary probe workflow on this PR, run 36754147351:

  • Root cause: in 18,805 of 20,000 writes, the file's mtimeMs came after the Date.now() read right after the write (19,042 of 20,000 when the file was stat'ed first). The largest gap was 0.983 ms, under the 1 ms margin. On a 5.15 kernel and a 6.12 kernel, the same probe found 0 of 20,000, which is why this passed on some machines.
  • Fails without the fix: main's pruneHistories.test.ts, run 200 times as 4 parallel workers x 50 npx vitest run <file>, failed 3 times. All 3 were prunes a history recorded without its disk only when its folder is there without the project, the test that failed in the merge queue.
  • Passes with the fix: the fixed file, same loop, 200 of 200 passed.

The loop, from packages/studio-server:

seq 4 | xargs -P4 -I{} bash -c 'f=0; for i in $(seq 50); do npx vitest run src/history/pruneHistories.test.ts > /tmp/out-{}-$i 2>&1 || f=$((f+1)); done; echo "worker {} failed $f/50"'

The probe workflow was removed from the branch (commit "ci: remove the temporary history prune probe").

For 2, on a Linux x86_64 host with the branch installed, from packages/studio:

  • Passes with the fix: react-virtual 3.14.13 (virtual-core 3.17.11), useTimelineVirtualRows.test.tsx run 200 times as 4 parallel workers x 50: 200 of 200 passed.
  • Fails without the fix: react-virtual 3.14.6 (virtual-core 3.17.4, main's pin), the same loop with seq 10 per worker (4 x 10): 40 of 40 failed. Each time the new test got one onChange after unmount, and the other 5 tests passed.
seq 4 | xargs -P4 -I{} bash -c 'f=0; for i in $(seq 50); do npx vitest run src/player/components/useTimelineVirtualRows.test.tsx > /tmp/out-{}-$i 2>&1 || f=$((f+1)); done; echo "worker {} failed $f/50"'

What I did NOT exercise

  • The full studio-server suite 200 times; the loop runs this file alone, 4 at a time.
  • Reproducing flake 2 through timelineStackingSyncExport.test.tsx itself. It depends on when vitest tears down the window, so the regression test checks the virtualizer contract directly, through the library's _didMount/_willUpdate, the lifecycle calls every TanStack adapter makes.

Before

The new regression test on main's react-virtual 3.14.6: the virtualizer still reports a change after unmount.

Before: the new test fails on react-virtual 3.14.6

After

The same test on this PR's react-virtual 3.14.13: no change after unmount.

After: the new test passes on react-virtual 3.14.13

Comment thread .github/workflows/tmp-prune-probe.yml Fixed
@miguel-heygen miguel-heygen changed the title test(studio-server): history prune tests allow a file time a moment past Date.now() test: stop two merge-queue flakes in the history prune and timeline tests Sep 30, 2026
@miguel-heygen miguel-heygen changed the title test: stop two merge-queue flakes in the history prune and timeline tests fix: stop two merge-queue flakes in the history prune and timeline tests Sep 30, 2026
@miguel-heygen
miguel-heygen marked this pull request as ready for review September 30, 2026 20:23

@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 57d61f2c. Both fixes go after the real cause. Neither loosens an assertion or adds a sleep to make a race go away.

History prune tests. The prune rule keeps a history while now - lastUsed < 14 days, and lastUsed is the max mtimeMs of project.json / log.jsonl (pruneHistories.ts:160,186). mtimeMs has sub-millisecond precision and Date.now() is floored, so Date.now() + 14 days can land a fraction of a millisecond short of the boundary. Adding 1 ms fixes the test's reading of the clock, not the rule. The boundary is still tested tightly: the 13-day assertion at :107 still expects []. The 1 ms margin is enough because the Date.now() read happens after the write. The probe's largest gap, 0.983 ms, agrees.

Timeline scroll-end timer. The failing stack goes through virtual-core's scroll-end debounce into notify after the test window is gone. virtual-core 3.17.8 (TanStack/virtual#1256) cancels that timer on teardown, so this is fixed in the library. The new test drives the same lifecycle (_didMount, _willUpdate, scroll, unmount) and waits past the 150 ms delay. That wait is how the test observes the timer; it is not there to give a race time to finish.

The bump is patch-only. react-virtual 3.14.6 to 3.14.13 and virtual-core 3.17.4 to 3.17.11. The lock diff has only the two TanStack entries plus the specifier. I read every changelog entry in the range against how useTimelineVirtualRows uses the virtualizer: fixed estimateSize, getItemKey, a custom rangeExtractor, scrollMargin, useFlushSync: false, and a resizeItem on the focused row. It uses no anchorTo, measureElement, directDomUpdates or smooth scrollToIndex, so most entries don't reach it. The ones that do:

  • 3.17.6 only compensates the scroll position for rows entirely above the fold on a re-measure. That affects the focused-row resizeItem, and the new rule is the safer one: a focused row that straddles the fold no longer drags scrollTop.
  • 3.17.7 notifies synchronously when a compensation moves scrollTop.
  • 3.17.8 is the fix this PR wants.
  • 3.17.11 reads the current scroll offset when the scroll-end fallback fires.

None of these conflict with the timeline's rule that it owns scrollTop and the virtualizer only observes it.

Local run at this head: pruneHistories.test.ts 15/15, and packages/studio/src/player/components 109 files, 1460/1460. I did not repeat the 200-run loops.

CI: all green. The two "Studio and player captures" failures at 19:50Z were superseded by successful runs from 20:04Z on. The only non-green entry is the WIP app check, still in progress.

Nit, not blocking: the regression test uses the virtualizer's underscore lifecycle methods. That matches what the adapters call, but a future TanStack bump could rename them and break the test without anything in the timeline changing.

— Rames

@miguel-heygen
miguel-heygen marked this pull request as draft September 30, 2026 20:37
@miguel-heygen
miguel-heygen marked this pull request as ready for review September 30, 2026 20:38
@miguel-heygen
miguel-heygen added this pull request to the merge queue Sep 30, 2026
Merged via the queue into main with commit 299670e Sep 30, 2026
213 of 215 checks passed
@miguel-heygen
miguel-heygen deleted the fix/prune-history-test-clock branch September 30, 2026 21:02
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.

4 participants