Repository navigation
fix: recover abandoned configuration locks safely - #6
Conversation
|
Thanks for the careful write-up and the reproduction — the stuck-lock problem is real and worth fixing. I can't merge this version yet, though, because of one Windows behavior that breaks the fail-closed guarantee. On Windows, renaming a directory onto an existing file succeeds and replaces the file. await fs.writeFile('x.lock', '');
await fs.mkdir('staged'); await fs.writeFile('staged/a.json', '{}');
await fs.rename('staged', 'x.lock'); // resolves; x.lock is now a directoryThe PR's own test Any fix needs to make sure a lock file can never be replaced on Windows, for example by checking that nothing exists at the lock path before the rename and handling the race where one appears in between. If that can't be made race-free, an acquire step that cannot replace anything (such as Happy to review again once it's updated. |
|
Thanks for catching this. You are right: the earlier simulation only exercised an existing directory and missed Windows replacing an existing file. Pushed 990e88d. Acquisition now uses exclusive Empty and incomplete owner records now fail closed. A crash during initialization or release can therefore require manual cleanup after confirming no writer/migration is active; both protocol translations document this tradeoff. During release and recovery, only the successful unique-marker remover attempts directory removal, so a losing reaper or delayed release cannot remove a successor's empty reservation. The updated 22 lock tests pass locally on Linux and in an independent review, including paused/killed empty or partial initializers, failed owner-write cleanup, the raced legacy file, and the existing post-marker crash/concurrent-save cases. Combined config/setup/migration/desktop-settings validation is 37 passed and 6 Windows-only skipped; TypeScript and release checks pass. Native Windows validation of this revision is still pending in CI, so the PR remains a draft. The description now corrects the earlier coverage claim. I have left the PowerShell startup cost unchanged in this revision rather than weakening the process-identity check. Identity is resolved before taking the directory lock. |
|
Thanks again for identifying the Windows lock-file issue. The fix in 990e88d now passes CI on Windows, macOS and Ubuntu, including all 22 lock regressions on native Windows. The PR is still a draft, but I would appreciate another review of the revised acquisition and recovery approach when convenient. |
|
Thanks, this revision addresses the blocker. Exclusive I reran the lock, home-migration, config/setup CLI and desktop settings tests locally on Windows 11 / Node 24: all pass. On the PowerShell cost: I agree with keeping it. A cheaper estimate of the creation time would have to match the later lookup exactly, and a mismatch would evict a live owner, so it isn't worth the trade. Settings writes are infrequent enough that the extra half second is acceptable. Merging now; it will ship in 0.3.15. Thanks again for the careful work on this one. |
Problem
If a process exits while
Configuration.save()holdsconfig.json.lock, every later settings write times out. Reproduced on v0.3.13 by killing a child after it acquired the real config lock; the next save failed withEEXISTafter about 10 seconds.Changes
mkdir, which cannot replace an existing legacy lock file, then record host, PID and process creation time in a unique owner marker.config_lockedwith a repair hint on timeout. Legacy files and empty, incomplete or unrecognized lock directories remain fail-closed.Validation
Updated in 990e88d after the Windows report:
git diff --checkpassed.npm run release:check: passed.The nonblocking Windows PowerShell startup cost is unchanged in this safety-focused revision; process identity is still resolved before acquiring the directory.
AI disclosure
This change was implemented and reviewed with an OpenAI coding assistant. The reproduction, regression tests and validation limits above were checked explicitly; local validation limits are distinguished from the CI results above. No installer or full (
--full) render-suite validation is claimed.