fix: stop two merge-queue flakes in the history prune and timeline tests - #4786
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
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 dragsscrollTop. - 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
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 usepastKeepWindow(), which adds 1 ms.2. Timeline scroll-end timer (studio).
@tanstack/react-virtualgoes from 3.14.6 to 3.14.13, which brings@tanstack/virtual-corefrom 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 callednotify, and so a React update.useTimelineVirtualRowsdispatches 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 writesproject.jsonand readsDate.now()in the same millisecond, the mtime can be a fraction of a millisecond after "now".now - lastUsedthen 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), throughobserveOffset's scroll-end callback andVirtualizer.notify, to react-virtual'sonChangeand react-dom'srequestUpdateLane. That timer fired aftertimelineStackingSyncExport.test.tsxhad finished and its window was torn down. For users, the leftover timer only schedules an update on an unmounted tree.How
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.jsonandbun.lock: the react-virtual bump. Only the two TanStack lock entries and the specifier change.useTimelineVirtualRows.test.tsx: a new test builds aVirtualizeron a scroll element, mounts it, dispatches a scroll, unmounts, waits 200 ms (past virtual-core's 150 ms scroll-end delay) and expects noonChangeafter unmount.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:
mtimeMscame after theDate.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.pruneHistories.test.ts, run 200 times as 4 parallel workers x 50npx vitest run <file>, failed 3 times. All 3 wereprunes a history recorded without its disk only when its folder is there without the project, the test that failed in the merge queue.The loop, from
packages/studio-server: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:useTimelineVirtualRows.test.tsxrun 200 times as 4 parallel workers x 50: 200 of 200 passed.seq 10per worker (4 x 10): 40 of 40 failed. Each time the new test got oneonChangeafter unmount, and the other 5 tests passed.What I did NOT exercise
timelineStackingSyncExport.test.tsxitself. 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.
After
The same test on this PR's react-virtual 3.14.13: no change after unmount.