Repository navigation
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.
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).
…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.
… bounds, backoff and identity retries R3-5, R3-9, R3-11: start() and stop() are steps on one promise chain over four states (idle, starting, running, stopping), safe un-awaited. stop() during a start makes it abandon and undo what it set up; a failed start undoes everything and rejects (never an unhandled rejection); start() after stop() takes the identity lock and the inbox again. discovery.stop() lets go of its server before it waits. A relay URL is checked in the constructor and in invites (EBADRELAY). R3-4: records a relay pacer never wrote (the connection closed, or the session ended) are held for the peer's key, offered again to its next session, and listed in pendingDeliveries() until a socket writes them. Bounded; the residual (written and then lost) is documented. R3-1/R3-2: the ws ceiling is 8 MiB and the frame bound is per message; an oversized message is dropped and counted, the connection stays up, and the roster is read past the bound. R3-3: 4004/4006 are retried on a long backoff with a ping-pong detector (identityRetry: false keeps MMP 4.4.7). R3-10: the backoff resets on relay-peers, not on open. R3-15: the liveness timeout terminates. R3-13: sessions close before the relay, and its last flush sends the 1010 notices.
…ty without birth times; linear frame parsing
R3-7: lines are cut on bytes and decoded whole (lib/line-reader.js) in the interior and the daemon,
which refuse a line that is not UTF-8 and bound lines in bytes; the CLI, SymDaemonClient and the
child-process readers decode with setEncoding('utf8'). The interior no longer signs text damaged by
a read boundary.
R3-6: a Unix socket path longer than sun_path is mapped, the same in the daemon and every client, to
/tmp/sym-<uid>/<digest>.sock in a 0700 directory the clients check before dialling.
R3-14: a creation time of 0 is unknown; a socket's identity adds the inode change time and a token
the daemon writes into the socket's times.
R3-12: FrameParser keeps its chunks and copies a frame once, when it is whole.
… notes on the identity-close retry
…ose keeps its queue; the ping-pong holds An independent read of the diff found: - a re-offer stopped by the pacer's room with nothing of its own waiting never ran again: it now re-arms on a timer, and checks the pacer's room (half of each bound spare) before it offers, so a re-offer never makes the pacer refuse a sealed frame (which ends the session); - taking back the records queued before a sealed 1010 turned the notice into a sequence gap at the peer: a session that sent the notice keeps its queue, which its peer can still open; - the ping-pong detector's window equalled the longest backoff, so a real ping-pong fell out of it, and the host was told on every close: the ping-pong now holds until a connection outlasts 70 min, and the host is told at the first close in the window and when a ping-pong is found; - records came back to a stopping node and were held after the stop dropped the rest: anything handed back to a node that is not running is said undelivered at once; - re-offers no longer go to a relay client session still waiting to supersede (offered when it does); a failed start is 'stopping' while it undoes; start() on a running node with nothing queued queues nothing, so a stop() in the same turn still starts at once; the redelivery metrics never throw into the teardown.
…he ping-pong's hold
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. |
![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: 1ac9934ab7
ℹ️ 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".
Comment on lines
845
to
+846
| this._relayUrl = opts.relay || null; | ||
| assertRelayUrl(this._relayUrl, 'SymNode: relay'); |
There was a problem hiding this comment.
When an embedder catches EBADRELAY, the constructor has already acquired the identity lock earlier in the constructor, but the partially constructed node is inaccessible and cannot release it. A subsequent node for the same identity in that process receives only the same-process no-op release, so even stopping that node leaves lock.pid held until process exit, preventing another process from using the identity. Move this validation ahead of identity loading/lock acquisition or explicitly release the lock on failure.
Useful? React with 👍 / 👎.
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.
The transport and node-lifecycle half of 0.14.3: the defects the R3 review proved by running them. Each fix has a test that fails on a
git archiveof 632b95d and passes here. The CHANGELOG has the full account, under "Transport and node lifecycle" in the 0.14.3 section.Fixed
EBADRELAY).pendingDeliveries()until a socket writes them. The list is bounded.relayIdentityRetry: falsekeeps MMP §4.4.7.setEncoding('utf8').sun_pathis mapped to/tmp/sym-<uid>/<digest>.sock. The daemon creates that directory (0700) and every client checks it.Needs a decision
identity-collision, so one spoofed 4004 still stops a channel host. It should exit only oninfo.pingPong.Not here
Verified
npm audit: 0 vulnerabilities.node:22-slimandnode:20-slim.origin/main(0.7.0).