Skip to content

fix: recover abandoned configuration locks safely - #6

Merged
LBEILC merged 2 commits into
LBEILC:mainfrom
liuyinchu:fix/config-lock-recovery
Oct 25, 2026
Merged

LBEILC merged 2 commits into
LBEILC:mainfrom
liuyinchu:fix/config-lock-recovery

Conversation

@liuyinchu

@liuyinchu liuyinchu commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Problem

If a process exits while Configuration.save() holds config.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 with EEXIST after about 10 seconds.

Changes

  • Acquire the lock with exclusive mkdir, which cannot replace an existing legacy lock file, then record host, PID and process creation time in a unique owner marker.
  • Recover only a verified dead owner, including a reused PID with a different known creation time. Never steal a live or unverifiable owner based on lock age.
  • During release and recovery, only the process that successfully removes the unique owner marker removes the empty directory. Losing reapers and delayed releases leave a replacement owner's initialization untouched.
  • Return config_locked with a repair hint on timeout. Legacy files and empty, incomplete or unrecognized lock directories remain fail-closed.
  • Conservative limitation: interruption during owner initialization or release can leave an unidentifiable directory that requires manual removal after confirming no settings write or migration is running. This avoids reclaiming a live writer's initialization window.
  • Add 22 regressions covering interrupted child processes, concurrent saves, legacy files racing acquisition, empty/partial initialization, PID reuse, live owners, cleanup/recovery races and failed publication. Document the behavior in English and Chinese.

Validation

Updated in 990e88d after the Windows report:

  • Linux / Node 24.19: focused config/setup/home-migration/desktop-settings tests: 37 passed, 6 Windows-specific skipped.
  • The 22 lock regressions also passed independently, plus an additional stale-reaper/empty-successor probe; git diff --check passed.
  • Desktop TypeScript and npm run release:check: passed.
  • Local desktop build was not completed because the font download was blocked in this environment. Full browser/render and desktop smoke checks were not rerun locally; Chromium was unavailable in the earlier local validation.
  • Previous-head CI passed on Linux and macOS but failed the legacy-file regression on Windows. The original simulated Windows check covered directory collisions, not directory-over-file replacement, and missed that defect.
  • Updated-head CI passed on Windows, macOS and Ubuntu, including renderer tests, desktop tests/build and smoke checks. The native Windows log confirms all 22 lock tests passed, with no failures or skips, on Windows Server 2025 / Node 24.21.0. This PR remains a draft for maintainer review.

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.

@LBEILC

LBEILC commented Oct 21, 2026

Copy link
Copy Markdown
Owner

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. fs.rename(staged, lock) therefore does not fail when config.json.lock is a legacy empty lock file; it silently takes it over. That is exactly the lock held by home-migration.mjs (and by an older renderer version mid-write), so a settings save can now run concurrently with a migration. Minimal check on Windows 11 / Node 24:

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 directory

The PR's own test legacy empty lock files stay untouched, with a manual recovery hint fails locally on Windows for this reason ("Missing expected rejection"). The other 12 new tests pass here. CI is now approved, so the Windows job should show the same result.

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 mkdir of the lock directory followed by writing the owner file) is also fine. A smaller note, not a blocker: processCreatedAt starts PowerShell on Windows, about 0.5 s per config set here, and the desktop app runs one CLI call per settings change. If the identity check can stay cheap on the uncontended path, that would be better.

Happy to review again once it's updated.

Copy link
Copy Markdown
Contributor Author

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 mkdir(config.json.lock), not a precheck followed by rename. Existing legacy files cannot be replaced, including when one appears at the acquisition boundary.

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.

@liuyinchu

Copy link
Copy Markdown
Contributor Author

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.

@LBEILC
LBEILC marked this pull request as ready for review October 25, 2026 02:17
@LBEILC

LBEILC commented Oct 25, 2026

Copy link
Copy Markdown
Owner

Thanks, this revision addresses the blocker. Exclusive mkdir can't replace an existing lock file, the home-migration file lock and the new directory lock still exclude each other in both directions, and recovery stays fail-closed for live or unverifiable owners. The unique owner marker also handles the delayed-release and competing-reaper cases cleanly.

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.

@LBEILC
LBEILC merged commit e20b845 into LBEILC:main Oct 25, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants