refactor(self-update): use active markers instead of lifetime locks#205
Conversation
The previous process-lifetime version lock prevented concurrent CLI invocations on the same version from coexisting, because each process held an exclusive lock on the version lock file for its entire run. Split the lock into two concerns: install locks guard mutation of a version directory, while per-process active markers advertise which versions are currently in use. Multiple processes can now run the same version simultaneously, and a self-update that targets a version held by another process now returns a busy outcome before clobbering its on-disk binary. Signed-off-by: Kevin Cui <bh@bugs.cc>
Summary by CodeRabbitRelease Notes
WalkthroughThis PR refactors the self-update version locking mechanism from a single process-lifetime lock model to a split filesystem layout with three lock types: active version markers (per-process under Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/application/self-update/lock.ts (1)
627-633:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle non-directory lock paths gracefully in
readDirectoryEntries.If a lock path expected to be a directory is actually a file,
readdirthrows and aborts stale-cleanup/active-owner discovery instead of skipping malformed entries. This can block self-update flows.Suggested fix
async function readDirectoryEntries(path: string): Promise<string[]> { try { return await readdir(path); } catch (error) { - if (isPathMissingError(error)) { + if (isPathMissingError(error) || isDirectoryReadError(error)) { return []; } throw error; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/self-update/lock.ts` around lines 627 - 633, readDirectoryEntries currently treats only missing-path errors via isPathMissingError, but if the lock path is a file (readdir throws ENOTDIR or equivalent) it aborts cleanup; update readDirectoryEntries to also treat "not a directory" errors as non-fatal by detecting the error code/name (e.g., ENOTDIR or error indicating the path is not a directory) and return an empty array (skip the malformed entry) instead of rethrowing; keep rethrowing for unexpected errors and reference the existing readDirectoryEntries function and isPathMissingError predicate when implementing the extra check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/application/self-update/lock.ts`:
- Around line 627-633: readDirectoryEntries currently treats only missing-path
errors via isPathMissingError, but if the lock path is a file (readdir throws
ENOTDIR or equivalent) it aborts cleanup; update readDirectoryEntries to also
treat "not a directory" errors as non-fatal by detecting the error code/name
(e.g., ENOTDIR or error indicating the path is not a directory) and return an
empty array (skip the malformed entry) instead of rethrowing; keep rethrowing
for unexpected errors and reference the existing readDirectoryEntries function
and isPathMissingError predicate when implementing the extra check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f12ea5b8-b1ad-46e4-be4b-3e28fa99aecd
📒 Files selected for processing (7)
src/application/bootstrap/run-cli.tssrc/application/self-update/core.test.tssrc/application/self-update/core.tssrc/application/self-update/lock.test.tssrc/application/self-update/lock.tssrc/application/self-update/paths.test.tssrc/application/self-update/paths.ts
The previous process-lifetime version lock prevented concurrent CLI invocations on the same version from coexisting, because each process held an exclusive lock on the version lock file for its entire run.
Split the lock into two concerns: install locks guard mutation of a version directory, while per-process active markers advertise which versions are currently in use. Multiple processes can now run the same version simultaneously, and a self-update that targets a version held by another process now returns a busy outcome before clobbering its on-disk binary.