HarperFast / HarperFast/harper

Harden atomic config writes: createFileSync zero-byte window, symlink/bind-mount, mode preservation, and castConfigValue key mapping

Open
#1,143 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 6h
Merged PRs (30d)
200

Description

## Summary

A cross-model review (Codex + Gemini) of the 4.7 backport of the atomic config-write fix (HarperFast/harperdb#3129) surfaced four config-write hardening items that are **present in the current v5 line** (`5.1.0-beta.1`), not just 4.7. None is a regression in the atomic-write fix (PR #493) itself — they're residual edges of writing config via temp-file + rename, plus one pre-existing comparison bug. Filing here so v5 and 4.7 stay aligned (fix here, then re-backport).

All locations are in `config/configUtils.js`.

## Findings

### 1. `createConfigFile` reintroduces a zero-byte window on the create/install path — significant
`createConfigFile` calls `fs.createFileSync(configFilePath)` (L189) to create an **empty** file at the real config path *before* `atomicWriteFile(configFilePath, …)` (L200). Between the `createFileSync` and the rename, a concurrent reader (worker thread / config watcher) observes a 0-byte file — the exact `rootPath`/`path.join(undefined, …)` failure the atomic write was meant to eliminate, just on the install path instead of the update path.
- **Fix:** drop the redundant `fs.createFileSync(configFilePath)` (the temp-file write + rename already creates the target atomically), or only `ensureDirSync` the parent directory.

### 2. `atomicWriteFile` breaks symlinked / file-bind-mounted config — significant
`fs.renameSync(tempPath, filePath)` (L103) replaces the path entry itself. If `harperdb-config.yaml` is a **symlink** (K8s ConfigMap, Docker secret) or a **file bind-mount**, the rename replaces the symlink with a regular file (severing the live-update binding) or fails with `EXDEV`/`EBUSY` where the previous in-place `fs.writeFileSync` would have written through.
- **Fix:** resolve `fs.realpathSync(filePath)` (when it exists) and write/rename against the resolved target; or explicitly document that symlinked/bind-mounted config files are unsupported. Worth a Fabric/Docker deployment gut-check on how the config file is actually mounted.

### 3. `atomicWriteFile` does not preserve target mode/owner — significant
The temp file is created with the process umask (typically `0644`). After rename it replaces the original, so an admin's `chmod 600` (secret protection) is widened to `0644`, ownership can change if the writer runs under a different uid, and a `0444` "do not mutate" guard is bypassed (rename only needs directory write permission).
- **Fix:** `fs.statSync` the existing target and apply its mode (and owner where applicable) to the temp file before the rename.

### 4. `updateConfigValue` skip-if-unchanged compares the raw arg, not the mapped config key — significant
The equality check casts and looks up the **raw** arg:
```js
const castedValue = castConfigValue(arg, parsedArgs[arg]); // configUtils.js:554
if (!_.isEqual(castedValue, flatConfigObj[arg.toLowerCase()])) { … }
```
`castConfigValue`'s string-preservation branch is keyed on exact `CONFIG_PARAMS` names (e.g. `clustering_nodeName`). Passing the unmapped arg (e.g. `clustering_nodename`) misses that branch, so `CLUSTERING_NODENAME=1` casts `"1"` → number `1`, `_.isEqual(1, "1")` is false, and the config is **rewritten on every boot** even though the write path maps to `clustering_nodeName` and writes `"1"` back. Aliases (e.g. `LOG_TO_STDSTREAMS`) likewise look up `flatConfigObj.log_to_stdstreams` instead of `logging_stdstreams` and always rewrite. This defeats the "skip if unchanged" goal added in `4a68092b` for the affected params.
- **Fix:** resolve the mapped key first and use it for both cast and lookup, matching the write path: `const configParam = CONFIG_PARAM_MAP[arg.toLowerCase()] ?? arg;` then `castConfigValue(configParam, …)` and `flatConfigObj[configParam.toLowerCase()]`.

## Notes
- #1 and #4 are corroborated, low-risk correctness fixes. #2 and #3 are robustness gaps inherent to atomic-write-via-rename; #2 needs a deployment decision (how is the config mounted in Fabric/Docker?).
- Related: PR #493 (atomic write), `1c73a1c5` (unique temp path), `4a68092b` (env cast), and the EPERM/EACCES retry + temp-cleanup follow-up now in `atomicWriteFile`.
- Surfaced during: HarperFast/harperdb#3129 (4.7 backport).

---

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.