Repository navigation
feat: persist isolated changes and immutable attempts (E02) - #426
Merged
Merged
Conversation
Closes #400. core/engineering/store.ts is the file-backed store E01's contract described but did not implement. E01 fixed the record shapes, the id grammar, and the path layout, so this module owns only what a store can get wrong. Two storage classes, because the contract has two. change.json and slice.json are versioned mutable projections replaced under compare-and-swap on their integer revision field. Everything under attempts/ is create-only: a snapshot, proof, review or approval is an observation of something that happened, and editing one is rewriting history rather than recording it. A correction is a new record naming the one it supersedes. An attempt is refused whatever revision the caller offers. The CAS token is the record's own revision, not a content digest. My first draft used a digest, which reads as elegant and is wrong: the contract defines revision as the token, and a digest would let two writers who produced identical bytes both believe they won. Serialization was a real bug, not a precaution. Compare-and-swap is a read followed by a write, and without a lock eight concurrent writers all read revision 1, all found their ifRevision satisfied, and all wrote: eight reported winners for one revision, seven updates lost. The per-change lock uses the existing acquireLock from core/status.ts; unrelated changes still proceed in parallel. Containment is checked twice, because once is not enough. The textual check catches a traversing id before touching the filesystem; the realpath check catches a symlink planted in a directory we already created, and it runs again after mkdir since that is the first moment a newly created directory's real identity can be confirmed. Corrupt records are reported, never skipped. A record that vanishes from a listing is indistinguishable from one that was never written, which is how history goes missing quietly. listChanges returns corrupt entries beside healthy ones with the reader's own reason. Distribution isolation, which the issue makes part of the acceptance: .codecarto/engineering/ is excluded from template copying, from both shipped gitignore files, and from the npm tarball. Measured rather than assumed - the gitignore rule and the files[] carve-out are INDEPENDENTLY sufficient, so the end-to-end pack test cannot catch the loss of either one, and each is pinned separately. E01's module-graph guard caught store.ts, as it caught snapshots.ts before it. The purity rule is split rather than dropped: every contract file except the store must still import nothing that touches the filesystem, and the store may use only fs, path, and the shared write/lock primitives. Verified the rule still bites by injecting node:fs into validation.ts. One guard was removed rather than shipped. A schema-version check in the reader could never fire, because E01's validator checks schema_version before any other field and already emits unsupported-schema-version. It looked load-bearing and was unreachable. The behaviour is still asserted here, since the store depends on it even though it does not implement it. Verified: 13/13 store tests, 7/7 distribution tests, 1029/1029 full suite, build exit 0, git diff --check clean. Six mutation checks bite: removing the per-change lock, the create-only rule, the stale-revision check, the idempotency digest comparison, symlink containment, or corrupt-entry reporting each turns the suite red.
All three passed on the version submitted for review. BLOCKING - a revision could move BACKWARDS. The check refused an equal revision but let a lower one through, which turns compare-and-swap into an ABA race. Roll the stored record back to 1 and every writer still holding ifRevision:1 - including one stalled since before the intervening updates - passes its check and overwrites work it never saw, silently: put(rev:1, ifRevision:3) -> accepted, v3 gone put(rev:2, ifRevision:1) -> accepted, "STALE-WRITER-WON" on disk A CAS token that is not monotonic is not a CAS token. Now strictly greater. Reads escaped the namespace while writes were contained. resolveInside realpath-checked dirname only, never the leaf, so a change.json that was ITSELF a symlink to a file outside the namespace was read straight through: get() returned the outside contents and listChanges() called the change healthy. Writes happened to survive it because rename replaces a link rather than following it - which is precisely why checking the write path missed it. The leaf is now included; such a record reads as corrupt. One idempotency key could serve two changes. The key lookup ran before the lock, and the lock is per-CHANGE while a key is GLOBAL, so concurrent puts to different changes sharing a key never serialized: both wrote, no conflict was reported, and the surviving receipt named only one of them, so retrying the other re-executed it. The check now runs under a per-key lock taken before the change lock, in a fixed order so the two cannot deadlock. Applying R11 - the class is "a guarantee enforced on the write path but not the read path". Swept core/engineering/store.ts: containment (was read-unsafe, closed), validation (already applied on read via readRecordFile), schema version (already on read), create-only (write-only by nature - a read cannot violate it). No further instance. Review also confirmed what holds: cross-process CAS is genuinely safe (8 separate processes, one winner, 7 stale-revision), acquireLock breaks a dead holder's ticket in ~2ms, listChanges() does not see .lock files since it filters to directories, and the two distribution-isolation mechanisms really are independently sufficient with no fourth leak path. Verified: 16/16 store tests, 7/7 distribution tests, 1032/1032 full suite, build exit 0. Three new mutation checks bite.
test (22), test (24) and test-windows all failed on 66ea9e7 with ENOENT: no such file or directory, stat '.../.codecarto/engineering' while the same suite was green locally. The control asserted that the namespace must already exist, reasoning that an empty offenders list proves nothing if the packer had nothing to exclude. That reasoning is right and the implementation was backwards: the namespace is gitignored, so it exists only on a machine that has done engineering work and never in a fresh checkout. It passed here because my worktree had one. The control now PLANTS a synthetic record, re-runs npm pack, asserts the tarball is still clean, and removes it in a finally block. That tests the exclusion under the condition it is meant to cover rather than depending on local state, and it works identically on a fresh checkout and a working machine. Verified by moving the namespace aside to reproduce the CI condition: 7/7 with no namespace present, and the directory is not left behind afterwards. Removing both exclusions turns the control red, so it bites. 1032/1032 full suite.
test-windows caught two defects Linux hid. The namespace prefix strip in resolveInside assumed a forward slash. engineeringPaths speaks POSIX, but two call sites compose with path.join, which emits a backslash on Windows, so the prefix went unrecognized and every locked path resolved to engineering\engineering\..., which exists nowhere: ENOENT: ...\.codecarto\engineering\engineering\changes\chg_...a1.lock.c.2e0d Eleven store tests failed on that alone. Separators are normalized before the comparison, which covers both join() call sites (the change lock and the idempotency record). The distribution test spawned npm directly. On Windows npm is a .cmd shim and is not spawnable by execFile, so the pack test died with spawn npm ENOENT. It now runs npm's own npm-cli.js with the node already executing, which needs no shell and behaves identically everywhere, falling back to npm.cmd if that entry point is not where it is expected. Applying R11 - the class is "POSIX path assumptions in the store's path handling". Swept core/engineering/store.ts: the prefix strip (closed), recordPath (out of scope, it only concatenates engineeringPaths output which is already POSIX and never touches the filesystem), and the realpath containment loop (out of scope, it uses dirname/relative/resolve which are platform-correct by construction). No further instance. The second class, "spawning a binary by bare name in a test", has exactly one instance in the files this change touches, now closed. Other suites reach npm only by reading package.json. Verified: 16/16 store, 7/7 distribution, 1032/1032 full suite, build exit 0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #400. The file-backed store E01's contract described but did not implement.
Two storage classes, because the contract has two
change.jsonandslice.jsonare versioned mutable projections, replaced under compare-and-swap on their integerrevision. Everything underattempts/is create-only: a snapshot, proof, review or approval is an observation of something that happened, and editing one is rewriting history rather than recording it. A correction is a new record naming the one it supersedes. An attempt is refused whatever revision the caller offers.The CAS token is the record's own
revision, not a content digest. My first draft used a digest, which reads as elegant and is wrong: the contract definesrevisionas the token, and a digest would let two writers who produced identical bytes both believe they won.Serialization was a real bug, not a precaution
Compare-and-swap is a read followed by a write. Without a lock, eight concurrent writers all read revision 1, all found their
ifRevisionsatisfied, and all wrote — eight reported winners for one revision, seven updates lost. The test caught it before the lock existed. Per-change lock via the existingacquireLock; unrelated changes still proceed in parallel.Containment is checked twice
The textual check catches a traversing id before touching the filesystem. The realpath check catches a symlink planted in a directory we already created — and runs again after
mkdir, since that is the first moment a newly created directory's real identity can be confirmed.Corrupt records are reported, never skipped
A record that vanishes from a listing is indistinguishable from one that was never written, which is how history goes missing quietly.
listChangesreturns corrupt entries beside healthy ones with the reader's own reason, and a truncated record does not make its siblings unreadable.Distribution isolation (part of the acceptance criteria)
.codecarto/engineering/is excluded from template copying, both shipped gitignore files, and the npm tarball. RED first, and the leak was real: a fresh workspace inherited another project's change records and the tarball shipped them.Measured rather than assumed — the gitignore rule and the
files[]carve-out are independently sufficient, so the end-to-end pack test cannot catch the loss of either one. Each is pinned separately.Two notes on the guards
E01's module-graph guard caught
store.ts, as it caughtsnapshots.tsbefore it. The purity rule is split rather than dropped: every contract file except the store must still import nothing touching the filesystem; the store may use onlyfs,path, and the shared write/lock primitives. I verified the rule still bites by injectingnode:fsintovalidation.ts.One guard was removed rather than shipped: a schema-version check in the reader could never fire, because E01's validator checks
schema_versionbefore any other field and already emitsunsupported-schema-version. It looked load-bearing and was unreachable. The behaviour is still asserted here, since the store depends on it even though it does not implement it.Verification
13/13 store tests, 7/7 distribution tests, 1029/1029 full suite, build exit 0,
git diff --checkclean.Six mutation checks bite: removing the per-change lock, the create-only rule, the stale-revision check, the idempotency digest comparison, symlink containment, or corrupt-entry reporting each turns the suite red.