Conversation
Contributor
|
What's this improving and fixing? Commit message should be more detailed. |
Zee2413
approved these changes
Mar 30, 2026
chrisqm-dev
approved these changes
Mar 31, 2026
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.
1. Atomic writes via write-to-temp-then-rename (EncryptedFileStore.ts)
Both save() and saveSync() now write to a temporary file (..tmp) first, then atomically rename it to the final path. Previously, writeFileSync/writeFile wrote directly to the target file.
Why it's needed: If the process crashes or is killed mid-write, a direct write can leave a truncated/corrupted file on disk. The rename operation is atomic on most filesystems — the file either has the old content or
the new content, never a partial state. This prevents data loss in the encrypted file store.
2. Guard against use-after-close (FileStoreFactory)
A closed flag was added to FileStoreFactory. Three methods now check it:
Why it's needed: The factory sets up setInterval (metrics) and setTimeout (cleanup) timers in the constructor. If close() clears these timers but a callback was already queued on the event loop, it could fire after
close and hit errors (e.g., accessing a deleted directory or emitting metrics on a torn-down telemetry instance). The closed guard prevents that race.
3. Lock acquisition moved earlier in constructor recovery path (EncryptedFileStore.ts)
The file lock is now acquired before readFile() in the constructor, not just around the saveSync() fallback. The old code only locked when it needed to recreate a corrupted store; the new code locks for the entire read-
then-maybe-recover block.
Why it's needed: Without the lock around readFile(), another process could be writing to the file concurrently while this instance reads it, leading to a read of partially-written data. By holding the lock during the
entire constructor initialization, the instance gets a consistent snapshot of the file.