test(e2e): fix the post-merge race in journey (a), and the electron CI cache race - #54
Merged
Conversation
…them The post-merge run on master failed on generatedUpTo, expected 1 and received 0, in a test that had passed locally many times and on the pull request. It is a race in the test, not a bug in the app, and the ordering makes that unambiguous. start-book.ts streams chapter 1, saves it, generates its quiz, and only then writes generatedUpTo and status in its finalize step. Chapter 1's prose therefore reaches the screen several steps before those fields exist on disk. Reading the book once at that moment asks a question the server has not answered yet, and the only reason it ever passed is that the intervening work takes under a millisecond against fakes on an idle machine. The busier post-merge runner lost that gamble. Reproduced deterministically before fixing it, by giving chapter 1's scripted stream a per chunk delay so the marker sentence renders while the stream is still running. A bare read at that instant returns 0 every time, and a polled read returns 1. Both on-disk assertions in that test now poll. Polling waits for a write on a deadline exactly the way a web-first assertion waits for the screen, and it weakens nothing, because the expected values are unchanged and a value that never arrives still fails. The revised table of contents assertion gets the same treatment for the same reason, since the parse and the save both happen after that stream ends too, and it was one busy runner away from the same failure. Every other on-disk assertion in the suite was checked and left alone. Each one already waits on a signal the server produces after the write it cares about, a reload, a refetched list, a dialog that only closes on success, or a recorded model request, so a bare read there is both correct and the stronger claim. No retries were added. A retry would have turned this into a green run and taught us nothing. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
The electron job failed with "Electron failed to install correctly" on a run whose only change was a test assertion, having passed twice before on identical workflow code. That pattern is the tell for a cache race rather than for anything in the diff. All three jobs share one pnpm store cache keyed on the lockfile, and two of them set ELECTRON_SKIP_BINARY_DOWNLOAD because they never launch Electron. Whichever of those saves the store first can hand this job an electron package whose postinstall was skipped and whose binary is therefore absent. Which job wins that write varies run to run, so the failure reads as flake even though the cause is deterministic once you know it is there. The job now runs the electron package's own postinstall after installing. It checks the installed version first and downloads only what is missing, reusing the binary cache the step above restores, so it is idempotent and costs nothing when the store cache was already clean. Verified locally that a second run against a complete install exits zero without downloading. Repairing here rather than dropping the skip flag from the other two jobs, because those jobs genuinely have no use for a ninety megabyte binary and making them pay for it to protect a third job would be the wrong trade. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Master went red on the merge commit for Phase 6. This fixes that, and a second, independent CI race that surfaced while fixing it. Two commits, two unrelated causes, both diagnosed rather than retried.
1. The test race that turned master red
A race in the test, not a bug in the app, and the ordering makes it unambiguous.
server/services/start-book.tsstreams chapter 1, saves it, generates its quiz, and only then writesgeneratedUpToandstatusin its finalize step. Chapter 1's prose reaches the screen several steps before those fields exist on disk, so reading the book once at that moment asks a question the server has not answered yet. The only reason it ever passed is that the intervening work takes under a millisecond against fakes on an idle machine. The busier post-merge runner lost that gamble.Reproduced before fixing. Giving chapter 1's scripted stream a per-chunk delay makes the window deterministic, because the marker sentence then renders while the stream is still running:
Both on-disk assertions in that test now poll. So does the revised table-of-contents assertion, which was one busy runner away from the same failure for the same reason, since its parse and save also happen after the stream ends. Polling waits for a write on a deadline exactly the way a web-first assertion waits for the screen, and it weakens nothing, because the expected values are unchanged and a value that never arrives still fails.
Every other on-disk assertion was audited and deliberately left alone, because each already waits on a signal the server produces after the write it cares about, so a bare read there is both correct and the stronger claim.
library-crudrenamepage.reload(), so the list is re-read from disklibrary-crudtaghandleSaveTagsawaitsupdateBookthenfetchBooks(), so the popover is server-derivedlibrary-cruddeleteepub-importexpect(dialog).toBeHidden(), and the dialog only closes on the success pathepub-exportwaitForEvent('download'), andwriteEpubprecedessucceed()adaptive-loopgeneration-failurewizardstatusis written before the stream that produces the error2. The Electron CI cache race the first fix exposed
The first push made
e2e-webgreen ande2e-electronfail withElectron failed to install correctly, on a run whose only change was a test assertion, having passed twice before on identical workflow code. That pattern is the tell for a cache race rather than for anything in the diff.All three jobs share one pnpm store cache keyed on the lockfile, and two of them set
ELECTRON_SKIP_BINARY_DOWNLOADbecause they never launch Electron. Whichever of those saves the store first can hand the Electron job a package whose postinstall was skipped and whose binary is therefore absent. Which job wins that write varies run to run, so it reads as flake even though the cause is deterministic once you know it is there. It would have kept biting at random forever.The job now runs the electron package's own postinstall after installing. It checks the installed version first and downloads only what is missing, reusing the binary cache the step above restores, so it is idempotent and costs nothing when the store cache was clean. Verified locally that a second run against a complete install exits zero without downloading.
Repaired there rather than by dropping the skip flag from the other two jobs, because those jobs genuinely have no use for a ninety megabyte binary.
No retries were added
Neither cause was papered over. A retry would have turned both green and taught us nothing, which is exactly what
retries: 0exists to prevent. The flake policy did its job the first time it was tested.Verification
No production file touched. The only non-test change is the added CI step.
https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5