Prevent older watch rebuilds from overwriting newer CSS - #20446
Prevent older watch rebuilds from overwriting newer CSS#20446michaelglass wants to merge 7 commits into
Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the current shutdown flow waits for queued rebuilds, tears down and flushes watchers while the queue remains open, and then closes only after accepted work completes. Reviews (9): Last reviewed commit: "Test drain directly" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWatch mode now routes filtered file events through a serialized batch queue. The queue waits for the initial build, coalesces pending batches, reports callback errors, and drains accepted work during shutdown. Watcher swaps and active callbacks are tracked. Shutdown awaits swaps and cleanups before closing the queue. Tests cover queue behavior and shutdown ordering. Merge Risk: 🔵 Low · up to Watch mode can still lose file changes and exit cleanly if stdin closes during watcher replacement. This is a bounded, localized correctness risk that should receive owner awareness or follow-up. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 43b2b5ea-e581-4229-b730-4a13a35cdd99
📒 Files selected for processing (3)
packages/@tailwindcss-cli/src/commands/build/index.tspackages/@tailwindcss-cli/src/utils/serial-batches.test.tspackages/@tailwindcss-cli/src/utils/serial-batches.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
c8f0653 to
1209165
Compare
michaelglass
changed the title
michaelglass
changed the title
a13f399 to
8b55bd9
Compare
8b55bd9 to
35a1670
Compare
michaelglass
changed the title
The existing tests cover the scheduling: serial-batches asserts that only one rebuild runs at a time, and the watcher tests cover event filtering and shutdown flushing. Neither asserts the outcome the bug was reported as — that what ends up written is the newest change. This drives the watcher wiring with an older change whose rebuild is slow and a newer change whose rebuild is fast, and asserts the newer result is written last. Against the previous fire-and-forget behaviour the two rebuilds overlap and the stale one lands on top, so the test fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lcj4iQ3fBxMwAu2rf4zLbC
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96c3020e-df7f-41d9-9fea-333af2a1afb7
📒 Files selected for processing (1)
packages/@tailwindcss-cli/src/commands/build/index.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
The test gated the slow rebuild on `written.length === 0`, but the first rebuild is suspended at that await, so the second rebuild saw a length of 0 too and waited on the same promise. Both then resolved in call order and the assertion held even with no serialization at all — the test could not fail. Count rebuilds instead, and wait for the first one to have actually started before submitting the second change. Verified both directions against a non-serialized queue: the old shape passed, the new one produces ['newer-change', 'older-change'] and fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lcj4iQ3fBxMwAu2rf4zLbC
A rebuild spliced the old cleanups out of `cleanupWatchers` and only pushed the new generation back after awaiting them. If stdin closed inside that window, shutdown saw an empty list, resolved immediately and closed the queue — so the files the old generation flushed on its way out were pushed into a closed queue and ignored, and the process exited 0 with stale CSS. Register the new generation before awaiting the old one, so the list is never empty, and track the in-flight swap so shutdown waits for it before closing. The ordering now lives in `shutdownWatchMode` so it can be asserted directly: the queue must still be open while a swap is flushing. Against the previous ordering that assertion fails — the queue is already closed and the flush has not landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lcj4iQ3fBxMwAu2rf4zLbC
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1da3bc6a-0959-4069-b755-bbc48921d734
📒 Files selected for processing (2)
packages/@tailwindcss-cli/src/commands/build/index.test.tspackages/@tailwindcss-cli/src/commands/build/index.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
Shutdown captured the swap promise once, so it waited for whichever swap was current when stdin closed. A rebuild already in flight can start its own swap after that point, and the queue was then closed while the later one was still flushing — the same dropped changes, one interleaving further out. Read the current swap each time round instead, until it stops changing. The regression test drives that interleaving directly: it replaces the swap while shutdown is already waiting, and asserts the queue stays open until the later one has flushed. Against capture-by-value the queue is already closed with only the first swap's work landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lcj4iQ3fBxMwAu2rf4zLbC
Tracking the swap promise was the wrong shape. It could always be read at a moment when the in-flight rebuild had not assigned its swap yet, so shutdown saw the old one settle, closed the queue, and dropped whatever the later generation flushed. Each fix narrowed that window without closing it. A full rebuild runs inside the queue, so the queue already knows when one is in flight — including a rebuild that has not yet replaced its watchers. Add `drain()`, wait for it before tearing the watchers down, and close only after their flush has landed. That removes the swap bookkeeping entirely. Both orderings are pinned: cleanup must not run while a rebuild is in flight, and what the watchers flush on the way out must still be processed. Against no-drain the cleanup runs early; against closing first the flush is ignored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lcj4iQ3fBxMwAu2rf4zLbC
drain() is public on SerialBatches but was only exercised through shutdown. Cover it on its own: it waits for in-flight work and leaves the queue open, so a shutdown can drain before the watchers flush and still have that work accepted, and it returns immediately when nothing is running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lcj4iQ3fBxMwAu2rf4zLbC
1 participant
Disclosure: This PR was generated entirely by Codex.
What I saw
I paused one rebuild inside a plugin, then changed the input again. The newer rebuild finished first, but the older rebuild finished afterward and overwrote it. The generated CSS was stale even though the newest edit had already been processed.
Fix
Wait for each rebuild before starting the next one. Keep changes that arrive while a rebuild is running, process them afterward, and flush a pending change before watch mode shuts down.
Also drop the files we write ourselves from a batch. A rebuild was only skipped when the output file was the single change, so our own output arriving alongside a real edit still triggered an extra rebuild.
And let a rebuild finish before shutting down. Closing stdin in the middle of one ended watch mode before the watchers had handed over what they had collected, so those changes were dropped and we exited as if all was well. Shutdown now waits for the rebuild in progress, then takes the watchers down, and only stops listening once what they hand over has been dealt with.
Test plan
['newer-change', 'older-change'].pnpm test— 5511 tests pass, plus the Rust suite.pnpm prettier --check packages/@tailwindcss-cli/src/commands/build/index.test.ts— passes.tsc --noEmit— no new type errors introduced by this change.[ci-all]