Skip to content

Shared utilities: fs_atomic and json_util (hypaware/core/util) - #240

Merged
platypii merged 7 commits into
masterfrom
shared-utils
Jul 5, 2026
Merged

Shared utilities: fs_atomic and json_util (hypaware/core/util)#240
platypii merged 7 commits into
masterfrom
shared-utils

Conversation

@platypii

Copy link
Copy Markdown
Contributor

Option A from the complexity review: hoist the two most-copied helper families into one shared module surface, exported as hypaware/core/util for plugins and imported relatively within core.

What's in here (discrete commits)

  1. Style the OAuth loopback landing page (Kenny's change, folded in): self-contained styled callback page with inline Hyperparam mark and light/dark support, and a title/detail split in respond().
  2. Add src/core/util/: fs_atomic.js (async/sync atomic tmp+rename writers, JSON variants, optional mode/dirMode/fsync, an expectedMtimeMs optimistic-concurrency guard throwing ConcurrentEditError, and ENOENT-tolerant readFileIfExists/readJsonIfExists) and json_util.js (isPlainObject, stringValue, parseMaybeJson, sortKeys, canonicalJson, sha256Hex, errCode), with 14 new unit tests.
  3. Migrate core tmp+rename writes (17 sites): daemon pid/status/unit files, config control state and action markers, cache cursor/spool/progress/ingest-seq, plugin lock, sink watermarks, remote credentials/seeds, CLI seed writes.
  4. Migrate plugin tmp+rename writes (6 sites): codex config and claude settings writers keep their exported signatures and domain error classes (they rewrap ConcurrentEditError as their CONCURRENT_EDIT errors); central identity, context-graph-enrich state, claude session-context compaction, vector-search shard metadata.
  5. Dedupe JSON value helpers (~36 copies deleted): isPlainObject x16, stringValue x9, parseMaybeJson x4, canonicalJson/sortKeys x4, sha256Hex/hashString x5, errCode x4.

Net: +722 / -589 across 45 files, but the insertions are almost entirely the new documented+tested module and the loopback page; the migration itself deletes ~430 lines of hand-rolled copies.

Correctness gained, not just lines

  • Every migrated write now removes its temp file when the rename fails; before, only the daemon unit writers and the three settings writers did, so a failed write leaked uniquely-named .tmp files at 15+ sites.
  • The three mtime-guarded settings writers (claude, codex, client detach) now share one guard implementation instead of three hand-synced copies.

Deliberately left alone

  • The iceberg resolver's local writer (interleaved ifNoneMatch 412 collision checks around the rename), the config pointer flip (symlink swap), git_fetch's install-directory swap, and vector-search's shard file (streams through hypvector's writer).
  • Divergent helper variants with different semantics: otel's stringValue (returns null), ai-gateway-graph's parseMaybeJson (returns null on parse failure), codex's tryParseJson, claude settle's safeParseJson.

Behavior notes for review

  • Temp names are unified as <target>.<pid>.<rand>.tmp; nothing scans for the old patterns.
  • JSON state files now consistently end with a trailing newline (a few previously did not); all readers JSON.parse.
  • A handful of writers gained a mkdir -p of the parent they previously assumed existed.

Verification

  • npm run typecheck clean; npm run build:types emits types/core/util/.
  • npm test: 1829 tests, 1828 pass, 0 fail, 1 skipped (includes 14 new unit tests for the shared modules).
  • Smoke flows: cache_lifecycle_maintenance, claude_attach_detach, daemon_foreground_start_stop, remote_oidc_login, plugin_install_local_dir, iceberg_export_local_fs, backfill_codex_fixture, all ok.

Replace the bare-paragraph callback response with a self-contained landing
page: inline Hyperparam mark, light/dark support, and a title/detail split
in respond(). No external fetches, since the browser only reached an
ephemeral loopback port.
One atomic tmp+rename writer (sync/async, JSON variants, optional mode/
dirMode/fsync/expectedMtimeMs guard) and one set of value-inspection and
canonical-JSON helpers, to replace the 20+ hand-rolled tmp+rename writes
and the isPlainObject/stringValue/canonicalJson/errCode copies scattered
across core and plugins. Exported as hypaware/core/util for plugins.
Replaces 17 hand-rolled atomic-write sites (daemon pid/status/unit files,
config control state and action markers, cache cursor/spool/progress/
ingest-seq, plugin lock, sink watermarks, remote credentials and seeds)
with atomicWriteJson/atomicWriteFile(+Sync), and the copy-pasted
catch-ENOENT read dance with readFileIfExistsSync. Every migrated site
now cleans up its temp file when the rename fails; before, only the
daemon unit writers did.
Left alone on purpose: the iceberg resolver's writer (interleaved 412
collision checks), the config pointer flip (symlink, not a file write),
and git_fetch's directory swap.
Behavior notes: temp names are unified as <target>.<pid>.<rand>.tmp and
JSON files now consistently end with a trailing newline.
The codex config and claude settings writers keep their exported
signatures and domain errors but delegate the stat-guard/tmp/fsync/
cleanup dance to atomicWriteFile, rewrapping ConcurrentEditError as
their CONCURRENT_EDIT error. central identity, context-graph-enrich
state, claude session-context compaction, and vector-search shard
metadata move to the plain helpers. The vector shard file itself still
streams through hypvector's writer, so its tmp+rename stays.
Replaces the pasted per-file copies with imports: isPlainObject (16
copies), stringValue (9), parseMaybeJson (4), canonicalJson/sortKeys
(4), sha256Hex (3, plus the two plugin_install hashString twins), and
errCode (4). All copies were semantically identical; the divergent
variants stay local on purpose (otel's stringValue returns null, and
ai-gateway-graph's parseMaybeJson returns null on parse failure).
…rint
Replace the @typedef in fs_atomic.js with an AtomicWriteOptions interface
in a sibling types.d.ts imported via @import, per the repo's type-in-.d.ts
convention. Drop identity_client's dead path import and route
fingerprintToken through the shared sha256Hex, dropping its lone createHash
import.
@platypii

Copy link
Copy Markdown
ContributorAuthor

Review

Ran a multi-angle review (line-by-line, removed-behavior, cross-file, reuse, efficiency, altitude, conventions, tests). Consensus: this is a clean mechanical hoist — no correctness regressions. Modes (0o600/0o700/0o644), fsync flags, the mtime/CONCURRENT_EDIT guards, ConcurrentEditError class identity across the re-export, tmp-cleanup, and every migrated JSON helper are faithfully preserved; the hypaware/core/util subpath resolves; no imports break; divergent helper variants were correctly left alone.

Two convention/cleanup items are already fixed in c5bf3a4: the @typedef now lives as an AtomicWriteOptions interface in a sibling types.d.ts, and identity_client.js dropped its dead path import and routes fingerprintToken through the shared sha256Hex. The items below remain.

Worth fixing

1. Unconditional mkdir added to hot-path writerssrc/core/util/fs_atomic.js:82
atomicWriteFile runs fs.mkdir(dirname, { recursive: true }) on every call. Several migrated writers previously did no mkdir because their directory was guaranteed to exist:

  • src/core/cache/streaming-reader.js:266 (writeProgress) is called once per batch inside the spool flush loop — the hottest cache write path. A large flush now issues N redundant recursive mkdir syscalls on a directory that was just read from.
  • src/core/cache/ingest-seq.js:58 (writeNextSeq) re-mkdirs cacheRoot immediately after reserveBlock already did (ingest-seq.js:110).
  • Also claude/src/session_context.js:168 (compaction; dir created microseconds earlier at :52) and vector-search/src/refresh.js:262.

Suggest an opt-out (e.g. mkdir: false) so hot callers can skip the guard. Low-medium severity, but a real per-write regression in a tight loop.

Design / quality notes (lower priority)

2. fsync: true doesn't fsync the parent directoryfs_atomic.js:90
The fsync branch syncs the temp file's data but never fsyncs the directory after rename. Across a power loss the data can be on disk while the rename isn't durable — the durability the option promises isn't fully delivered. Not a regression (the old client_detach writer had the same gap), but this is new shared infra other callers will trust.

3. Rewrap boilerplate copy-pasted at 3 sitesclaude/settings.js, codex/config-file.js, client_detach_disk.js
The { mode: 0o600, fsync: true, expectedMtimeMs } call plus the identical catch (err) { if (err instanceof ConcurrentEditError) throw new XError(..., { code: 'CONCURRENT_EDIT', cause: err.cause ?? err }) } is a single "guarded config write" operation that wasn't named. An onConcurrentEdit factory param would collapse all three and stop a fourth adapter from forgetting the rewrap.

4. Test coverage gapstest/core/util-fs-atomic.test.js

  • dirMode is never asserted, though credential/control dirs depend on dirMode: 0o700.
  • The fsync path is exercised but its semantics aren't asserted — deleting handle.sync() would keep the suite green.
  • The mtime-rejection test relies on the v1 write and v2 rename landing in different mtime ticks; on a coarse-resolution mount (some WSL/network mounts) they can collide, making it flaky and masking the guard's real mtime-granularity limit.

5. Minor:writePersistedIdentity (gateway_seed.js:209) and writePersistedFile (identity_client.js:314) are now near-byte-identical — the same dedup this PR exists to do. loopback.js:272's LANDING_PAGE.replace('{{title}}', title) is now $-sensitive (harmless today; all callers pass literals). The trailing-newline normalization on ~7 state files is benign — every reader uses JSON.parse.

Bottom line: safe to merge on correctness. #1 is the only remaining item with runtime cost; the rest are follow-ups.

atomicWriteFile ran a recursive mkdir on every call; the migrated
per-batch writeProgress and per-block writeNextSeq previously did none.
Add a mkdir option (default true) and pass mkdir:false at those two
sites, whose target directory is already guaranteed to exist.
@platypii

Copy link
Copy Markdown
ContributorAuthor

Addressed issues. Merging.

@platypii
platypii merged commit 0ed3c19 into masterJul 5, 2026
4 checks passed
@platypii
platypii deleted the shared-utils branch July 5, 2026 16:44
Sign up for freeto 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.

1 participant

@platypii