bug(storage): settings.json, atomic replace has no durability fence and a corrupted file is unrecoverable; the atomic-write pattern drifts across stores
- Dominant language
- TypeScript
- Stars
- 5.4k
- Forks
- 502
- Avg merge
- 1d 2h
- Merged PRs (30d)
- 715
Description
### What happened
Two coupled defects in `packages/storage/src/settings-store.ts`, one on the write path and one on the read path:
**1. `write()` ([settings-store.ts:209-214](https://github.com/apache/maka/blob/main/packages/storage/src/settings-store.ts#L209-L214)) performs temp + rename with no durability fence at all.**
```ts
private async write(settings: AppSettings): Promise {
await mkdir(dirname(this.settingsPath), { recursive: true });
const tempPath = `${this.settingsPath}.${process.pid}.${Date.now()}.tmp`;
await writeFile(tempPath, JSON.stringify(settings, null, 2) + '\n', 'utf8');
await rename(tempPath, this.settingsPath);
}
```
There is no `handle.sync()` on the temp file and no directory sync after the rename, so nothing orders the rename behind the file's data blocks. On a power loss or hard crash inside the write window, POSIX does not guarantee the data reached disk even though the rename did (the classic rename-without-fsync hazard — ext4's delayed-allocation zero-length files being the well-known instance), leaving `settings.json` present but zero-length or truncated. Additionally, the temp name is predictable (`pid` + `Date.now()`), the file is created without `wx`/O_EXCL, and a failed rename leaves the temp file behind.
**2. `readOrCreate()` ([settings-store.ts:92-102](https://github.com/apache/maka/blob/main/packages/storage/src/settings-store.ts#L92-L102)) only treats ENOENT as recoverable.**
```ts
try {
const text = await readFile(this.settingsPath, 'utf8');
return normalizeSettings(JSON.parse(text));
} catch (error) {
if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error;
/* first run: write defaults */
}
```
A zero-length or truncated file makes `JSON.parse` throw a `SyntaxError`, which has no `.code`, so it falls into the rethrow branch: no fallback to defaults and no recovery guidance. The error propagates out of `get()`, and the desktop main call sites I checked invoke it without a catch (e.g. `apps/desktop/src/main/runtime-host-settings-ipc-main.ts:146` and `:179`, `apps/desktop/src/main/client-settings-ipc-main.ts:41-42`), so settings loading fails outright.
Net effect: the write path can produce a corrupt file, and the read path treats corrupt as fatal. A *missing* settings.json gracefully gets defaults; a *corrupted* one bricks settings load until the user figures out they must delete `/settings.json` by hand. `mcp-config-store.ts:181-190` has the same ENOENT-only `readOrCreate`, so its read path shares the recovery gap.
### How to reproduce
Real-world trigger (intermittent, low probability, deterministic consequence): change any setting, then lose power / hard reset inside the write window.
Deterministic simulation of the post-crash state:
1. Locate the workspace settings file `/settings.json` (constructed at `settings-store.ts:80`)
2. Truncate it — either empty it completely or keep the first ~20 bytes so the JSON is cut mid-token
3. Start the desktop app (or call `settingsStore.get()` directly)
Expected: settings load falls back to defaults, the same grace already extended to a missing file. Actual: a `SyntaxError` propagates out of `get()` and settings loading fails with no recovery path.
### Environment
- Maka version or commit: 8c491e64b (main)
- OS and version: macOS 14 (the code paths are cross-platform; the missing-fsync hazard is POSIX-relevant)
- Surface: Desktop / `@maka/storage` from source
- Node.js version, if running from source: 24.x
### Logs, screenshots, or additional context
The deeper issue is divergence: the same "atomic JSON file replace" pattern is implemented three times inside one package, at three different strictness levels — and the strictest one is the package's own documented standard.
| hardening | `settings-store` `write()` (:209) | `mcp-config-store` `write()` (:197) | `credential-store` `writeSecretFileAtomic()` (:261) |
|---|---|---|---|
| unpredictable temp name | ✗ `pid`+`Date.now()` | ✓ `randomUUID()` | ✓ `randomUUID()` |
| exclusive create (`wx` / O_EXCL) | ✗ | ✓ | ✓ |
| 0600 temp + chmod | ✗ | ✓ | ✓ |
| fsync temp before rename | ✗ | ✗ | ✓ `handle.sync()` |
| fsync parent dir after rename | ✗ | ✗ | ✓ `syncDirectory()` |
| temp cleanup on failure | ✗ | ✓ (swallows the error) | ✓ (rethrows) |
`credential-store`'s JSDoc states the standard explicitly:
> Owner-only atomic write for a credentials file: a 0700 dir, an exclusive 0600 temp ('wx'/O_EXCL so we never follow a pre-planted symlink at a predictable path), a durability fence before and after the atomic rename, and temp cleanup on failure.
`settings-store` meets none of these properties; `mcp-config-store` meets all but the durability fence. Meanwhile the fence primitives already exist and are exported — `stable-storage.ts:90-133` provides `syncFile` / `syncDirectory` / `syncDirectoryChain`, already used by marker-file, memory-bundle-io and the session-bundle paths — and `credential-store.ts:281` even carries a private duplicate of `syncDirectory`.
Suggested direction (happy to take this once triaged):
1. Extract one shared atomic-write helper implementing the credential-store standard (randomUUID temp, `wx`, 0600, fsync file → rename → fsync dir, cleanup that rethrows) and point `settings-store` and `mcp-config-store` at it — removing the drift rather than patching each site.
2. Harden both `readOrCreate()` implementations to distinguish "missing" from "corrupt": on corrupt, move the file aside (e.g. `settings.json.corrupt-`), rewrite defaults, and surface a warning — or at minimum throw a typed error naming the file to remove, instead of letting a bare `SyntaxError` propagate through startup.
Contributor guide
Assessment
This issue has not been assessed yet.