Repository navigation
fix(persistence): guard file reads against torn writes from external editors #3348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@moonshot-ai/agent-core-v2": patch | ||
| --- | ||
|
|
||
| Fix models and providers transiently disappearing when config.toml is saved non-atomically by an external editor while the daemon reloads it. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,8 @@ import type { | |
| import { toStorageIoError } from '#/persistence/interface/storage'; | ||
|
|
||
| const WATCH_DEBOUNCE_MS = 150; | ||
| const TORN_READ_RETRIES = 3; | ||
| const TORN_READ_RETRY_DELAY_MS = 15; | ||
|
|
||
| function isEnoent(error: unknown): boolean { | ||
| return (error as NodeJS.ErrnoException).code === 'ENOENT'; | ||
|
|
@@ -35,11 +37,23 @@ export class FileStorageService implements IFileSystemStorageService { | |
|
|
||
| async read(scope: string, key: string): Promise<Uint8Array | undefined> { | ||
| const filePath = this.pathFor(scope, key); | ||
| try { | ||
| return await readFile(filePath); | ||
| } catch (error) { | ||
| if (isEnoent(error)) return undefined; | ||
| throw toStorageIoError(error, { path: filePath, op: 'read' }); | ||
| for (let attempt = 0; ; attempt += 1) { | ||
| let bytes: Uint8Array; | ||
| try { | ||
| bytes = await readFile(filePath); | ||
| } catch (error) { | ||
| if (isEnoent(error)) return undefined; | ||
| throw toStorageIoError(error, { path: filePath, op: 'read' }); | ||
| } | ||
| if (attempt >= TORN_READ_RETRIES) return bytes; | ||
| let size: number | undefined; | ||
| try { | ||
| size = (await stat(filePath)).size; | ||
| } catch { | ||
| size = undefined; | ||
| } | ||
| if (size === undefined || size === bytes.length) return bytes; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an in-place writer truncates the file and pauses, or writes one chunk and pauses, Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 6171d28: a size match alone no longer settles the read for a hot file. When the mtime is within 100ms of now, acceptance requires a second stat 15ms later reporting identical size+mtime; a change between the two stats loops back into the retry. This narrows the truncate-and-pause case to a writer that stays paused across the confirmation gap — a read-side guard cannot be fully airtight against an in-place writer paused indefinitely mid-write (only atomic tmp+rename writes are, which is what the test-side change enforces); the guard remains best-effort defense for external editors.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update: the double-stat confirmation from 6171d28 is reverted in 01ba720. CI showed it caused real regressions: delaying every read of a recently-written file by 15ms (45ms for files under append) shifted server boot/reload timing enough to break the config changedFields baseline test and the fork-parity test. Since no read-side guard can be airtight against an in-place writer paused mid-write, and the extended guard demonstrably breaks real behavior to narrow a scenario that has never been observed, the PR keeps the simple size-verify guard (which eliminates the race that was actually reproduced) and relies on atomic tmp+rename writes as the real contract — now enforced in the test. |
||
| await new Promise((resolve) => setTimeout(resolve, TORN_READ_RETRY_DELAY_MS)); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This user-perceivable fix is shipped through the CLI, so the repository's changeset rules require listing
@moonshot-ai/kimi-code; attributing it only to the private internal@moonshot-ai/agent-core-v2package omits the CLI bump and changelog entry for the fix. Change the frontmatter package to@moonshot-ai/kimi-code.AGENTS.md reference: AGENTS.md:L85-L87
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6171d28: the changeset frontmatter now lists @moonshot-ai/kimi-code.