fix(upgrade): create missing install dir and skip stale stored path#1125
Conversation
`sentry cli upgrade` crashed with `ENOENT ... open '.../sentry.lock'` when the install directory did not exist at lock time. Two triggers: - A stale stored install path: a `SENTRY_INSTALL_DIR` test install (e.g. `/tmp/sentry-test-install`) recorded `install.path` there, the directory was later purged, but the DB row survived. `getCurlInstallPaths()` trusted it blindly and locked into a dead location. - The `~/.sentry/bin` fallback (and npm→nightly migration) is never created before `acquireLock` writes the lock file. Fixes: - `acquireLock` now `mkdir -p`s the lock file's parent directory before writing, so the fresh/migration/purged cases no longer crash. - `getCurlInstallPaths` only trusts a stored curl path when its directory still exists, falling through to the running-binary / `~/.sentry/bin` resolution otherwise. Adds red-first regression tests for both paths. Fixes CLI-1E1, CLI-1RV.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 247666c. Configure here.
Codecov Results 📊✅ Patch coverage is 100.00%. Project has 5047 uncovered lines. Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 81.27% 81.28% +0.01%
==========================================
Files 390 390 —
Lines 26963 26964 +1
Branches 17520 17521 +1
==========================================
+ Hits 21913 21917 +4
- Misses 5050 5047 -3
- Partials 1823 1823 —Generated by Codecov Action |
…rsion Cursor Bugbot: when the lock's parent path is an existing regular file, mkdirSync throws EEXIST, which the writeFileSync catch misrouted into handleExistingLock — it then read ENOENT and recursed into acquireLock until stack overflow. Move mkdirSync before the try so mkdir failures (EEXIST/ENOTDIR/EACCES) propagate as genuine errors. Adds a regression test asserting no RangeError when the parent path is a file.
Address adversarial review follow-ups: - The file-as-parent test now asserts the propagated error code is EEXIST. Previously it only checked `toThrow()`/not-RangeError, which passed whether mkdir was inside or outside the lock try/catch (vacuous). EEXIST propagates only when mkdir is outside the try; moving it inside yields ENOTDIR (via handleExistingLock), so the assertion now genuinely guards the placement. - Correct the acquireLock comment: a misplaced mkdir surfaces a misleading ENOTDIR (not infinite recursion) for the file-as-parent case. - Document that existsSync() in getCurlInstallPaths also treats EACCES / transient-unmount as "skip stored hint", which is acceptable because the execPath fallback still finds a genuine running install.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const lockPath = join(fileAsDir, "sentry.lock"); | ||
|
|
||
| let caught: NodeJS.ErrnoException | undefined; | ||
| try { | ||
| acquireLock(lockPath); | ||
| } catch (error) { | ||
| caught = error as NodeJS.ErrnoException; | ||
| } | ||
| expect(caught).toBeDefined(); | ||
| expect(caught?.code).toBe("EEXIST"); | ||
| }); |
There was a problem hiding this comment.
Bug: The test for acquireLock incorrectly asserts the error code EEXIST. On POSIX systems like Linux, the error code will be ENOTDIR, causing the test to fail in CI.
Severity: LOW
Suggested Fix
Modify the test assertion to account for platform differences. The test should check if the error code is either ENOTDIR (for POSIX) or EEXIST (for Windows), potentially using a conditional based on process.platform.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: test/lib/binary.test.ts#L499-L518
Potential issue: The test `"propagates the mkdir EEXIST when the parent path is a
regular file"` attempts to create a lock file in a path where a parent segment is a
file, not a directory. The test asserts that the resulting error code is `EEXIST`.
However, this behavior is specific to Windows. On POSIX systems like Linux (which is
used in CI), `mkdirSync` throws an `ENOTDIR` error in this scenario. Because the test
hardcodes the expectation to `EEXIST`, it will fail on the Linux-based CI environment,
blocking the PR. The production code is unaffected as it correctly propagates the error
regardless of the code.
Also affects:
src/lib/binary.ts:406~406
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Thanks, but this is a false positive — EEXIST is correct here on Linux.
The distinction is what mkdirSync targets:
dirname(lockPath)istestDir/not-a-dir, which is the regular file.mkdirSync(testDir/not-a-dir, { recursive: true })therefore targets a path that already exists as a non-directory → POSIXmkdir(2)returns EEXIST.ENOTDIRonly occurs when an intermediate segment is a non-directory (e.g.mkdirSync(testDir/not-a-dir/sub)), which this test does not do.
Verified empirically on Linux (mkdirSync(file, {recursive:true}) -> EEXIST, readFileSync under a non-dir -> ENOTDIR), and the Unit Tests CI job passed with this exact assertion on 4b5d009.
Asserting EEXIST specifically is intentional: it guards mkdir's placement. If mkdir ran inside the lock try/catch, the EEXIST would be routed into handleExistingLock, which then reads the lock under a non-dir and surfaces ENOTDIR. Broadening the assertion to also accept ENOTDIR would make the test pass for the buggy placement too (i.e. vacuous), which is exactly the gap this test closes.
…ng-install-dir # Conflicts: # .lore.md
1 participant
Problem
sentry cli upgradecrashes withUnexpected error: ENOENT ... open '.../sentry.lock'when the install directory does not exist at the moment the lock file is written. Reported by sergical in #discuss-cli (2026-06-22):Root cause
The upgrade pipeline derives the lock /
.downloadpaths from an install location it does not validate, and writes the lock before that directory is guaranteed to exist. Two triggers:SENTRY_INSTALL_DIR=/tmp/sentry-test-installtest install recordedinstall.paththere; the directory was later purged but the DB row survived.getCurlInstallPaths()trusted it blindly, so the upgrade tried to lock into a dead directory.~/.sentry/binfallback (and the npm/brew→nightly migration that funnels throughdownloadBinaryToTemp) is nevermkdir-ed beforeacquireLock.Fix
acquireLock()nowmkdir -ps the lock file's parent directory before the atomicwriteFileSync, so fresh installs, migrations, and purged-dir cases no longer crash.getCurlInstallPaths()only trusts a stored curl path when its directory still exists, otherwise falling through to the running-binary /~/.sentry/binresolution.The 2026-06-01
copyfilesymlink variant is already handled onmainby the realpath self-copy guard (#1046), so it is out of scope here.Tests
Red-first regression tests added (both confirmed to reproduce the exact
ENOENTbefore the fix):acquireLockcreates the parent directory when it does not exist.getCurlInstallPathsignores a stale stored path whose directory no longer exists (and still uses a stored path whose directory exists).pnpm run typecheckand Biome lint pass; full unit suite green (one unrelatedsourcemap/ziptimeout flake that passes in isolation).Related Sentry issues
Fixes CLI-1E1 (
/home/developer/.sentry/bin/sentry.lock, 9 events / 2 users) and CLI-1RV (/root/.sentry/bin/sentry.lock).Out of scope
sentry cli --versionsupport (sergical also asked) is deferred to a separate change.sentry upgradeis intentionally not aliased (reserved for SDK upgrade flows).