fix: tolerate a concurrent symlink when linking the plugin dev SDK - #11059
Open
markkuhr wants to merge 3 commits into
Open
fix: tolerate a concurrent symlink when linking the plugin dev SDK#11059markkuhr wants to merge 3 commits into
markkuhr wants to merge 3 commits into
Conversation
Workspace installs run this for several packages at once. Each one checks for an existing link, removes it, then creates its own, so two packages can interleave between the check and the create and the loser fails the whole install with EEXIST. Treat EEXIST as success. The link already points where this run intended, so the postcondition holds and there is nothing to repair. Any other error still propagates.
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
Contributor
Greptile SummaryThe PR makes plugin development SDK linking tolerate a concurrent
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains, and the previously requested PR-description information has been supplied.
|
| Filename | Overview |
|---|---|
| scripts/link-plugin-dev-sdk.mjs | Wraps SDK symlink creation so a concurrent EEXIST is treated as successful while other filesystem errors still propagate. |
| scripts/link-plugin-dev-sdk.test.js | Adds a deterministic multi-process regression test covering concurrent SDK-link creation and child-process failures. |
Reviews (3): Last reviewed commit: "test: harden the SDK link race test agai..." | Re-trigger Greptile
The window only opens under real parallelism, so the test drives separate processes. In-process calls cannot interleave, because the underlying filesystem calls are synchronous. Workers busy-wait to a shared start instant, then all link the same package directory. Reproduces reliably against the unfixed linker: eleven of twelve workers fail with EEXIST on the first round.
Three problems with the harness, none with what it proves. A worker that fails to spawn (EAGAIN under process pressure) emits "error" and may never emit "close", so the promise never settled and the run would have hung until the 20 minute job timeout. Handle "error" and report it as a failed worker. Every worker busy-waited the full startup budget, pinning each core for the whole window. Sleep to just before the shared start instant and spin only the last few milliseconds. Same wall time, roughly a tenth of the CPU. The dynamic import happened after the barrier, putting module load inside the race window. Import first so every worker arrives at the symlink call warm, and report distinct worker errors instead of one identical line per worker. Reproduction is unchanged: 11 of 12 workers still fail on the first round against the unfixed linker.
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.
Thinking Path
Linked Issues or Issue Description
No existing issue. Describing it here per CONTRIBUTING.md.
What happened?
pnpm installfails intermittently withEEXIST: file already exists, symlink ... @paperclipai/plugin-sdk. Re-running usually succeeds.Expected behavior
The install completes. The symlink exists and points at the in-repo SDK.
Steps to reproduce
Run
pnpm installon a clean checkout with several plugin packages present. The failure is timing dependent. The test added here reproduces it deterministically: twelve processes link the same package directory at a shared start instant, and eleven fail against the unfixed linker.Paperclip version or commit
master, a388ea1.
Deployment mode
Not applicable. This is a build step.
Installation method
From source,
pnpm install.What Changed
linkSdkIntotreatsEEXISTfromsymlinkSyncas success and returnstrue. Any other error still propagates. That is the whole behavior change: one error code on one call.scripts/link-plugin-dev-sdk.test.js, which thetest:release-registryjob already runs. It drives separate processes, because in-process calls cannot interleave when the underlying filesystem calls are synchronous.errorevent so a spawn failure cannot leave the run hanging until the job timeout, sleep to just before the shared start instant instead of busy-waiting the whole window, import the module before the barrier so module load is not inside the race window, and report distinct worker errors instead of one identical line per worker.Verification
node --test scripts/link-plugin-dev-sdk.test.js— 8 pass in 2.1s.The contrast is the actual evidence. Restoring the original
link-plugin-dev-sdk.mjsfrom master and re-running fails the new test on round 0 with 11 of 12 workers reporting EEXIST, reproduced 3 for 3.On CI cost: the race test measured 2102ms on a GitHub-hosted runner (2224ms before the harness change) in the
Typecheck + Release Registryjob, which takes about 4 minutes overall. The harness change cut CPU from roughly 1074% to 107% at the same wall time, so the 60 spawned processes no longer pin every core while they wait. Worker and round counts are left at 12 and 5 — one round already fails 11 of 12 workers without the fix, and the extra rounds are cheap redundancy for machines where a given round happens to miss the window.Risks
Low. The change narrows to one error code on one call. The postcondition is unchanged: after either path the link exists and points at the in-repo SDK. A real (non-symlink) install is still left alone, and a stale link pointing elsewhere is still replaced.
Two things worth naming for a reviewer:
security-reviewcheck reports neutral, "Security Review Recommended". The flag issuspicious-test / shell-exec: the new test importsspawnfromnode:child_process. That is inherent to the test — the race window only opens under real process-level parallelism — and the spawned command isprocess.execPathrunning a literal source string with paths interpolated throughJSON.stringify. No network, no shell, no external input.Model Used
Claude Opus 4.5 (
claude-opus-4-5) for the fix and the initial test; Claude Opus 5 (claude-opus-5, 1M context) for the test-harness hardening and the CI cost analysis. Extended thinking, with tool use and code execution.Checklist