Files

T

Yichen Jiang 3b1b912518 docs(settings): third-review contracts across READMEs, catalogs, and the write-path integrity note

The seam README states the JSON-shaped write boundary, watch-disposer
quiescence, async listener containment, and the drained teardown; the
provider README rewrites Behavior around the operation chain,
read-modify-write, writer lock, ready reconcile, and leaf-level YAML
diffs, and updates Known Limitations to the residual guarantees.
A new Agent Note records the round's decisions and supersedes the
original note's deferred-lockfile alternative (cross-linked in place).
Chinese counterparts updated pair-by-pair (three briefed minimal
updates, one whole-document translation); type-equiv, config, cordis,
and module-graph catalogs re-recorded.

2026-07-30 14:09:04 +08:00

7.4 KiB

Raw Blame History

Agent Note: settings write-path integrity and observer lifecycle

Status: implemented

English | 中文

Scope: the third review round over packages/settings/ — write-path data integrity in dsh-settings-local (operation chain, read-modify-write, cross-process writer lock, diff-shaped YAML edits) and observer lifecycle in dsh-settings (watch disposal, async listener containment, the JSON-shape write boundary). This note reverses one deferral recorded in the user-settings seam note: the cross-process lockfile now ships.

Problem

Review found the provider's write path could destroy state it never observed, and the seam's observer lifecycle leaked past disposal. Concretely: watcher reloads and document writes ran on two independent promise chains while every write rendered the whole next document from the cached text, so an external edit still inside the debounce window was overwritten — and the follow-up reload no-oped because the post-rename content matched the cache, erasing the edit without a trace. The initial load() raced the watcher's own setup, leaving a startup window whose changes never fire an event. Two processes sharing a harness home rendered from independent caches, last writer winning whole namespaces. On the seam side, a watch() disposer only removed the observer from its set — an invocation already chained onto the watcher tail still ran after disposal, and nothing drained started invocations at service dispose; the settings/updated manual fan-out caught only synchronous throws, so an async listener's rejection escaped as an unhandled rejection; and structuredClone admitted Dates, Maps, BigInts, and cycles that YAML/JSON storage silently distorts on the reload round-trip (a Date lands as a timestamp string, a BigInt as a plain number). YAML writes replaced the whole namespace node, deleting every comment inside the section a comment-preserving provider had promised to keep.

Decision

One operation chain, and every write is a read-modify-write. Watcher refreshes and persists from every namespace queue share a single settled chain, and persistSection begins by reconciling the on-disk text into the seam — publishing any unobserved difference first — before rendering against that fresh text. A write can no longer resurrect a stale document, and an on-disk document that turned invalid fails the write loud rather than being overwritten (the reload path keeps its warn-and-keep-last-good policy; the shared reconcileFromDisk throws and each caller picks its policy). The watcher's ready signal queues one extra reconcile, closing the startup gap between the initial load and the watcher becoming active.

Writes hold a wx-created <file>.lock sibling. The read-render-rename cycle runs under a cross-process writer lock with exponential backoff, a 2 s acquisition deadline, and stale takeover after 5 s (a crashed holder, broken with a warning). Readers never lock — the rename commit is atomic — so contention is writer-only and resolves in milliseconds. The lock constants are protocol invariants, not config: a holder rewrites one small document, so the deadline and stale age derive from that bound, not from deployment taste.

Observer disposal is quiescent. Watchers carry an active flag checked when a queued invocation would start, so a disposer that ran while the invocation waited prevents the start entirely; started invocations register in a service-level pendingTails set that the dispose drain awaits beside the write queues. The settings/updated fan-out contains a returned thenable's rejection through the same listener diagnostic as a sync throw, and the event contract now states that the INVARIANT rethrow serves synchronous listeners only — invariant companions must stay sync, which the shipped companion already is.

The write boundary admits JSON data only. The call-time snapshot is a single cloneJsonShaped walk that detaches the patch and rejects any non-JSON value — Date, Map, BigInt, non-finite number, function, symbol, class instance, undefined array entry, circular reference — with its $-rooted path before anything persists. Object entries that are explicitly undefined still skip (the sparse-patch contract), now enforced at the boundary instead of inside mergeLayers.

YAML edits are leaf-level diffs. renderYaml diffs the stored section against the next one and applies only setIn for changed values and deleteIn for removed keys, recursing through maps. Comments, anchors, and formatting survive on every untouched node and on the key node of every changed pair; arrays and other non-map values replace wholesale when unequal (deepEqualJson is the shared predicate), taking comments inside them along.

Alternatives considered

proper-lockfile instead of a hand-rolled lock — the dependency-over-hand-rolling policy was weighed: the library is barely maintained, its stale/retry policy is broader than this one-file protocol needs, and the shipped lock is ~40 lines with deterministic tests (including injected EEXIST/stat races). The policy favors dependencies that delete owned code; this one would replace 40 explained lines with an opaque peer.
Revision/CAS instead of a lock — rename cannot express compare-and-swap, so a CAS needs a version sidecar or content re-hash and a retry loop in every writer; the lock achieves the same serialization with one primitive and keeps readers free.
Merging external edits into the in-flight write's own section — the seam merges patches over the state visible at call time, so a same-namespace external edit racing a write still resolves last-write-wins; folding it in would need three-way merge semantics no consumer has asked for. The write publishes the external state first, so the loser is at least observed before being superseded.
Declaring async settings/updated listeners unsupported — the typed signature is void and lint flags misused promises, but an unlinted JS plugin can still register an async listener; a contract note cannot un-throw an unhandled rejection, so containment is the only defense that holds at runtime.
Keeping structuredClone and validating in the provider — the seam is the durable boundary's owner (every provider stores JSON-shaped documents), and rejecting at call time gives the caller the offending path; a provider-side check would reject after merge, blaming the merged section instead of the caller's value.

Consequences

update() gained a documented failure mode (lock deadline, invalid on-disk document) and the rejection messages carry $-rooted paths. Remaining, documented in the provider README: same-namespace concurrent edits stay last-write-wins (no per-value merge or revision check), a watcher event the OS never delivers leaves the cache stale until the next signal or write, and comments inside replaced arrays or attached inline to changed scalar values go with the value they described. The user-settings seam note's deferred-lockfile alternative is superseded by this note. The same defect classes exist in dsh-credentials-local (two chains over one .env, cached whole-file write-back, post-persist emit) and in the llm/adapters-updated fan-out on the stacked branches; those fixes belong to the PRs that introduce the packages and follow this template on merge-up.

7.4 KiB Raw Blame History