ci: give each job its own pnpm install dir (fixes what #130 missed) - #131
Merged
Merged
Conversation
Follow-up to #130, which did not fix the race. Part of #125. #130 added `standalone: true` on the theory that the node_modules layout was what raced. It is not. pnpm/action-setup clears its install directory before writing to it regardless of install mode, so with the default ~/setup-pnpm that `rmdir` is shared by every job on a self-hosted box. #130's own run passed only because nothing else was running beside it; the moment two jobs started together again, both #127 and #129 reproduced the identical error on branches that already contained the fix: ENOTEMPTY: directory not empty, rmdir '~/setup-pnpm/node_modules/.bin/store/v11/files/03' `dest: ${{ runner.temp }}/setup-pnpm` is the actual fix: runner.temp is per-job and cleaned up by the runner, so no two jobs share a directory to race on. That alone would regress something worth keeping. The pnpm store lived under the install dir, so making the install dir disposable would make the store disposable too, and every job would re-download ~1100 packages — contradicting the reason this workflow deliberately skips `cache: pnpm`. So the store is pinned to a stable path with `--store-dir ~/.pnpm-store`. That separation is also what makes the original failure impossible to repeat: a shared content-addressed store is pnpm's normal mode and is safe for concurrent readers. It was only fragile here because it happened to sit inside the one directory the action deletes.
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
1 participant
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.
#130 did not fix the race. This is the correction.
What I got wrong in #130
I added
standalone: trueon the theory that thenode_moduleslayout was what raced. It isn't.pnpm/action-setupclears its install directory before writing to it regardless of install mode, so with the default~/setup-pnpmthatrmdiris shared by every job on a self-hosted box.#130's own run passed only because nothing else was running beside it. The moment two jobs started together again, both #127 and #129 reproduced the identical error on branches that already contained the fix:
Both branches verifiably had
standalone: true(2 occurrences each). So the evidence is unambiguous.The actual fix
dest: ${{ runner.temp }}/setup-pnpm—runner.tempis per-job and cleaned up by the runner, so no two jobs share a directory to race on. This is a structural fix rather than a behavioural guess: there is no longer a shared mutable path.The regression that would have caused, and how it's handled
The pnpm store lived under the install dir. Making the install dir disposable would make the store disposable too, and every job would re-download ~1100 packages — directly contradicting the reason this workflow deliberately skips
cache: pnpm(that comment measures the alternative at up to 79s of post-job cache save).So the store is pinned separately:
--store-dir ~/.pnpm-store.That separation is also what makes the original failure impossible to repeat. A shared content-addressed store is pnpm's normal mode and is safe for concurrent readers — it was only fragile here because it happened to sit inside the one directory the action deletes. Once those two concerns are on different paths, neither can corrupt the other.
How to validate this one properly
#130 taught me that a single green run proves nothing here, because a lone job never races. This needs two jobs running concurrently to be meaningful — which happens naturally once this and another PR are in flight together. I'll confirm against a concurrent pair rather than a solo pass before treating it as fixed.
Refs #125.
🤖 Generated with Claude Code