Repository navigation
0.14.3: an agent no longer loses its key to the 0.13 -> by-id upgrade - #56
Merged
Merged
Conversation
…y-id upgrade
An XMesh review of lib/config.js 0.14.2 returned DO-NOT-SHIP with 13 findings. Each was reproduced in a
sandbox state dir before anything was changed; the data-loss one was also reported from a developer's machine.
What lost the key: migrateNodeDir refused to move a 0.13 node when nodes/by-id/<nodeId>/ existed, even empty or
holding only lock.pid, and left the name unindexed. loadIdentity then read "refused" as "no such agent", made a
new identity, renamed the 0.13 directory onto the new node's path and wrote the new identity.json over the old
one. A pinned start was bricked instead. The guard asked whether a directory existed; it must ask whether an
identity does.
Root causes, changed rather than patched:
- A second claim on a nodeId is a different identity (or content that cannot merge) already filed under it. An
empty directory or a stale lock claims nothing: the node is merged into it. The same identity on both sides is
an interrupted move and is finished. A real duplicate is still refused and nothing is overwritten.
- loadIdentity treated every answer it did not recognise as "mint". It now mints only when nothing on the host is
known under the name: a 0.13 identity still under the name, or a name indexing a node whose identity is missing,
halts (IdentityHaltError).
- The name is indexed before anything moves and the identity moves last, so a second starter finds the agent and
an interrupted move is re-entered by the identity's absence. A step that fails because another process just did
it is re-run; a path that became an alias of its destination is never merged onto itself.
- Key files: rename keeps the mode 0.13 wrote (0644). identity.json and e2e-keypair.json now end at 0600 and their
directories at 0700 after a migration; loading narrows an existing key file, which heals nodes already migrated by
0.14.0-0.14.2. A rewrite of identity.json is atomic.
- A move across file systems copies, verifies byte for byte, then removes the source.
- Smaller: an identity with a public key and no private key halts; the live-lock fallback checks the tombstone;
a symlink that is not a by-id link no longer reads as already indexed; SYM_IDENTITY_DIR switched on for a node
whose identity is in its node dir halts with instructions instead of making a new identity.
Not fixed, listed in the CHANGELOG: migrateIdentities still discards per-node errors; loadIdentity({nodeId, name})
re-points a name that indexes another node; SYM_IDENTITY_DIR first-time does not move the key for the operator;
an explicit nodeId unknown to the index is ignored when a new identity is made.
tests/identity-migration-by-id.test.js: 36 tests, 30 of which fail on 0.14.2. Four real processes starting one
0.13 agent at once, 320 starters: every one loaded the original agent. Seventeen mutants of lib/config.js, each
caught.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
A name whose identity file is missing halts and names what to remove to start the name again as a different agent. It named only nodes/by-name/<name>.json, but nodes/<name> is a link to the old by-id directory and re-indexes the name from it on the next start, so removing the index alone halts again. The message now names both, and a test follows the message to the end (the first removal is shown not to be enough). The test count in the CHANGELOG is now 37 (31 fail on 0.14.2).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e31e380d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fs.chmodSync(dst, st.mode & 0o777); | ||
| return; | ||
| } | ||
| fs.copyFileSync(src, dst, fs.constants.COPYFILE_EXCL); |
There was a problem hiding this comment.
On POSIX, a crashed node can leave <node dir>/interior/interior.sock behind (lib/interior.js places the socket there, and lib/relocation.js already treats it as ephemeral). When this new EXDEV fallback migrates such a node across filesystems, copyFileSync is called on the socket and fails with ENXIO; loadIdentity then fails after its retries, so the node cannot upgrade or start. Skip socket entries such as interior.sock during the copy rather than treating every non-directory entry as a regular file.
Useful? React with 👍 / 👎.
…n lost the key) The merge into an existing by-id directory was safe only if no other process was doing the same. It assumed each step was idempotent, so several processes could interleave them. They cannot: when one finishes it replaces nodes/<name> with the compat link, and another, still holding the old path, then sees that path lead to the destination, finds the "duplicate" file identical to itself and deletes it - the only copy of identity.json. Plain concurrent runs passed 320 starters. A fuzzer that stalls every file call for a few ms in four real processes lost the identity file in 2 of 300 rounds (everything else filed, identity.json gone). A Linux run under a throttled CPU (node 22, 0.3 cpu) failed the new race test twice in 40 with the same symptom: identity.json missing from by-id after every starter had finished. The design assumption was "a retry makes concurrent migration safe". It does not, and a guard on the alias only narrows the window. The migrator now holds a lock (nodes/.migrating/lock.pid, the identity lock) while it moves something; others wait, read the state again on their turn, and find it done. It is taken only when a 0.13 directory is there to move. The retry loop and its ENOENT tolerance are gone; the alias check stays as a last guard on a key file. With the lock the fuzzer ran 1,200 starters (300 rounds, husk) and 1,600 (400 rounds, none) with every starter loading the original agent and no errors. Also fixed in this commit: the F6 test passed for the wrong reason once the lock existed (its injected write failure hit the lock file's write); it now fails only the identity temp file. A starter that read the index just before another process wrote it, and then migrates nothing, re-reads the index instead of making a new identity; that line had no test and now has one. Tests: 41 (33 fail on 0.14.2); 17 mutants, each caught.
…g it waited The test asserted the second start waited at least 250 ms of a 400 ms hold. Under a throttled cpu (node 20 in a container at 0.3 cpu) the holder's timer had run for 150-170 ms before the test process was told it held the lock, so 4 of 30 runs measured 225-243 ms and failed although the start had waited. It now has the holder report the time it let go, and asserts the start returned at or after that time (a start that moved the node alongside the holder returns well before it). Mutant "no migration lock" still fails it.
loadIdentity ran the migration only for a start by name alone (`!nodeId && name`). mesh-channel starts every
node as { name, nodeId: <the pin in node.json>, create: false }, so a pinned node whose files were still the
0.13 nodes/<name>/ could not start (EIDENTITYABSENT) until something started it by name; with the default
create: true, 0.14.2 made a new identity and lost the key, and the first 0.14.3 draft halted. Reproduced in a
sandbox state dir on fe285a3 and on the branch before this change (plain, empty by-id/<pin>, create true/false).
When the pin's identity is not in place and a 0.13 directory under the name is still there, loadIdentity now
runs the same locked, idempotent migrateNodeDir(name) and goes on to load the pinned agent. It is one path:
the unpinned and pinned starts share the block; migrateNodeDir takes the pin as an optional second argument.
It is not run when there is no 0.13 directory to move, so a pinned start writes nothing it did not before.
Decision, a pin that is a DIFFERENT nodeId than the directory's: that directory is another agent's, so it is
left where it is, unindexed (migrateNodeDir answers 'skipped-duplicate' before it moves or indexes anything).
The pinned identity stays absent and the start fails as before: IdentityAbsentError with create: false, and
with create: true the halt that names the stranded 0.13 identity and the pin. Nothing is lost and nothing is
made. That agent moves when it is started by name or by its own pin.
Tests (tests/identity-migration-by-id.test.js, 51 now): plain and empty/locked by-id/<pin> with create false;
create true loads the original and does not mint; SYM_IDENTITY_DIR; a pin that is another agent's; a link to
another agent does not get indexed by a pinned start; the name indexed to another agent; a live 0.13 holder and
the tombstone; four real processes with injected stalls, with and without a by-id dir already in place. Eight
of them fail on 0.14.3 as pushed (051f44b) and on fe285a3; two guard against over-migrating. Four new mutants
(no pinned migrate, no pin guard, no legacy-dir gate, adopting the index) are each caught.
…t, exclusive name claim The design assumption that failed twice: the state of an identity was inferred from whatever was at a path (a partial destination, an unreadable index, a node dir without identity.json, a stale lock's content). Replaced: - 0.13 -> by-id migration COPIES: staged beside its destination (nodes/by-id/.<id>.partial-*), identity.json last, synced, verified byte for byte, installed by ONE rename. The loser of two commits finds the same identity and discards its staging. The 0.13 directory is then renamed aside to .legacy-0.13/<name> (never deleted) and the compat link takes its place. A by-id dir without the commit record (.sym-commit) is a husk: emptied with rmdir, or its files merged into the staging and the husk kept at .<id>.husk. mergeMove, moveTree and the nodes/.migrating lock (Atomics.wait up to 30 s) are gone. - A name is claimed by an exclusive create (link) of its index entry after the commit; a losing first start discards its minted identity and loads the winner. The index reads as absent only on ENOENT; anything else halts. Index writes are fsynced. - A mint happens only when there is no trace of the name; a trace that provably is the name's identity is adopted, any other halts with what it is. - A live 0.13 process in the 0.13 directory refuses the start (EIDENTITYLOCK). - A stale lock is removed only by the holder of a claim named after that lock's file identity and content (compare-and-delete); locks are created whole via link. - relocation: intent, bundle, tombstone beside the key, scrub; resumable and abortable (abortExport); imports staged with 0700 dirs, 0600 keys, the key only in the identity dir. - validateName refuses names Windows cannot hold; compat links compared as resolved paths. tests/identity-commit.test.js: 25 tests, all failing on 632b95d.
…art installed A start copying the 0.13 directory while another start sets it aside can see it go mid-copy. That start's error is not the agent's state: the other start installed the identity and claimed the name, so loadIdentity reads it by the index instead of failing. Tests: four guard tests for the staged commit (a copy that does not read back is not installed; a linked subdirectory is copied as a directory; a husk with a live lock refuses; a live process's staging is left alone).
…tombstone, export intent, commit record) from a bundle
…identity (R4-9)
loadIdentity({ name, nodeId: pin }) re-pointed the name's index entry to the pin whenever it
named another identity. mesh-channel pins node_id from a repository's .sym/node.json, so a
folder pinning X under "reviewer" silently took the name from another folder's agent Y: Y's
next start by name ran as X and Y was left without its name, and a cloned repository could
choose which identity on the host a session signs as.
Now a pinned start may claim a name nobody has, and that is all. A name whose entry names
another identity halts (IdentityHaltError, saying both and how to resolve it) and the entry
is left as it is; with create: false and the pin not on this host it is IdentityAbsentError,
saying the name is another agent's. A pinned start is never answered with the name's agent.
…nd by the multi-process fuzz)
Four starts at a 0.13 node over a lock-only by-id husk: one start judged the target a husk,
another emptied it (stale lock removed, rmdir) and committed its copy there, and the first then
read the directory's entries (the committed copy's), took them for a husk's files and renamed
the committed directory aside to .<id>.husk. Same identity, nothing lost, but an installed
directory moved: 9 of 40 rounds (6 of 40 pinned).
The husk is now examined again immediately before it is set aside; a commit found there is
the other start's ('exists'). A husk with files cannot be replaced by a commit (a rename onto
a non-empty directory fails), so after that look only another claim can move it, and then the
rename aside fails. Lock artifacts (a claim, a temp lock file) are not a husk's content: dead
ones (and this process's own, left by a failed attempt) are removed before the rmdir; one that
stays has the husk set aside with it.
The real cross-filesystem test is named for what it now checks: no rename crosses the volumes.
… aside (the fuzz's race, made deterministic)
…s and kill -9 fuzz, suite, gate, audit)
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
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.
0.14.3: an agent no longer loses its key to the 0.13 -> by-id upgrade
An XMesh review of
lib/config.js0.14.2 returned DO-NOT-SHIP with 13 findings. The first fix (632b95d) was reviewed again (R1) and failed the same way twice over: it read the state of an identity from whatever was at a path (a by-id directory partly there, an index entry that could not be read, a node directory withoutidentity.json, a stale lock's content), and each patch for one case made another. R1 proved eleven defects by running them. This PR replaces the design assumption instead of patching the cases.The design: identity state is a record committed in one atomic step
nodes/by-id/.<nodeId>.partial-<pid>-<rand>(beside its destination, so on its volume), links followed,identity.jsonlast, every file synced, verified byte for byte, a commit record (.sym-commit) written, and installed by ONE rename. Several starts at once: one rename wins, the rest find the same identity and discard their staging. Then the 0.13 directory is renamed aside tonodes/.legacy-0.13/<name>(never deleted, keys 0600) and the compat link takes its place.rmdir(cannot remove anything with content) and the commit made; a husk with files is merged into the staging and kept at.<nodeId>.husk; a differing file or identity is a conflict (halt, nothing changed); a live lock refuses.nodes/<name>link or non-empty dir,SYM_IDENTITY_DIR/<name>, an aside dir, an unindexed installed identity of that name). A trace that provably is the name's identity is adopted (the name claimed for it); any other trace halts with what it is.EIDENTITYLOCK).relocation.abortExport). Import: staged, dirs 0700, keys 0600, the key only in the identity dir.validateNamerefuses names Windows cannot hold (mesh-channel'snodeNameProblemrule); compat links compared as resolved paths (junctions).Deleted:
moveTree,mergeMove,sameTree,mergeConflict,legacyDirsRemain,acquireMigrationLockand thenodes/.migratinglock (Atomics.wait up to 30 s).lib/node.jsis not changed.Also fixed here: R4-9 (mesh-channel review): a pinned start no longer re-points a name another identity holds; it halts and leaves the entry (create: false with the pin absent: IdentityAbsentError saying the name is another agent's).
Verification (numbers in the CHANGELOG)
tests/identity-commit.test.js(new): 33 tests, 32 fail on 632b95d.tests/identity-migration-by-id.test.js: 53 tests (14 fail on 632b95d); every changed expectation carries its reason in the test.Still known (CHANGELOG)
The two-rename window between setting the 0.13 dir aside and the link (a 0.13 build started in exactly that state would mint); a husk merge race that leaves files kept aside, not merged; the aside copies are never removed by sym (disk); 0.13 processes still reclaim locks non-atomically; abortExport not on the CLI; consumers that list
nodes/(xmeshsrc/node-tree.ts) should skip dot-names (.legacy-0.13). Behaviour changes: a node dir with files and no identity halts; names Windows cannot hold are refused; a pinned start never takes a name.Not merged, not tagged, not published.