feat(smg): deterministic worker ids — restart-stable mesh keys#1668
feat(smg): deterministic worker ids — restart-stable mesh keys#1668CatherineSue wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR makes worker IDs deterministic from node and URL data, derives mesh server names from ChangesDeterministic Worker IDs and Mesh Stability
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
dependencies
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces deterministic, restart-stable worker IDs derived from the node name and worker URL using UUID v5. This ensures that restarted nodes republish workers under their previous mesh keys, preventing orphan keys and resolving tombstones. Additionally, the reconciliation process in WorkerSyncAdapter is updated to detect and re-assert locally-owned state over stale store values that might arise from prior incarnations. Regarding feedback, there is a performance inefficiency in reconcile_once where worker_state_of serializes the worker's spec to JSON bytes, which is then deserialized back inside store_matches for comparison. It is recommended to optimize store_matches by performing a cheap byte-wise comparison first to avoid expensive JSON parsing in the common case where nothing has changed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-implemented PR. Reviewed all four changed files:
registry.rs — WorkerId::derived with chained UUIDv5 namespacing is collision-free by construction (the test covering the byte-shift case confirms this). OnceLock is the right primitive for the single-init namespace. Pre-minting the candidate before taking the DashMap entry in both reserve_id_for_url and register_inner correctly avoids holding the shard guard across the mint — and deterministic minting means the discarded candidate is identical to the existing entry when the URL was already reserved.
worker_sync.rs — The reconcile re-assertion is the key correctness fix. The else if Local guard correctly limits re-puts to owned workers (mesh imports are never re-published). By reconcile time the Lamport clock has observed the stale timestamps via the merge, so the re-put wins and no feedback loop forms (each node only re-asserts its own workers).
wiring.rs — Namespace installation before worker-init workflows is the right sequencing point.
Tests — Five new tests cover the critical paths: deterministic id stability across remove/re-register, cross-node/cross-URL distinctness (including the chained-namespace byte-shift case), mesh-off random id preservation, wiring integration, and the reconcile shadow fix. Good coverage.
No issues found. 0 🔴 · 0 🟡 · 0 🟣
CatherineSue
force-pushed
the
chang/mesh-deterministic-ids
branch
from
196b224 to
07679f8
Compare
![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: 07679f8999
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ea4e0ab79
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // and unique per node, so it yields the same identity across | ||
| // restarts (deterministic worker ids, no ghost member per boot). | ||
| // ':' is the rl shard-key separator, hence the '-'. | ||
| format!("Mesh_{}", advertise_addr.to_string().replace(':', "-")) |
There was a problem hiding this comment.
When mesh users omit --mesh-server-name, this now reuses the same self_name after a restart. If peers have already advanced the old incarnation to Down/higher version, the restarted node advertises version 1 again and is ignored by GossipService::merge_state unless node.version > entry.version (crates/mesh/src/gossip_service.rs lines 190-197); peers also filter Down nodes out of their connection candidates (crates/mesh/src/gossip_controller.rs lines 162-165), so the node can stay absent from membership/CRDT fanout after an ungraceful restart. The default needs a fresh membership incarnation/version or a separate stable worker-id namespace rather than reusing the gossip name at version 1.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The scenario is real but not new with this change: any deployment using the recommended explicit --mesh-server-name has had exactly this exposure all along — a stable name restarting at version 1 against a circulating Down rumor at a higher version. 3ea4e0a extends stable names to default configs, so the fix belongs at the membership layer, and it already exists in the sibling mesh-stack PR #1674 (commit 1da437f): SWIM-style self-refutation in merge_state (a node hearing itself reported Suspected/Down re-asserts Alive past the rumor's version) plus Down-entry expiry after dead_timeout, either of which unblocks the restarted node (refutation immediately; expiry within 60s). Duplicating that hunk here would conflict at merge time, so the honest framing is an ordering note: merging #1671→#1674 before (or together with) this stack closes the window entirely; merging this stack first leaves the same brief exposure explicit-name deployments already have today, until #1674 lands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Hi @CatherineSue, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
CatherineSue
force-pushed
the
chang/mesh-deterministic-ids
branch
from
3ea4e0a to
d595755
Compare
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/src/worker/registry.rs`:
- Around line 189-195: mint_worker_id currently derives IDs from the raw URL
string which can differ from the normalized URL used later (see
WorkerService::create_worker, reserve_id_for_url, register_inner), causing
mismatched IDs; fix by canonicalizing/normalizing the URL before deriving the ID
(i.e., run the same normalization used in registration/reservation so you call
WorkerId::derived(node, canonicalized_url) instead of using the raw url), or
alternatively propagate the reserved WorkerId through the normalization step so
reservation and registration use the same identity source; update mint_worker_id
(and the other two mint sites) to perform normalization prior to calling
WorkerId::derived.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dfddcc5f-41f5-4f6c-a85f-2419e07d4132
📒 Files selected for processing (5)
model_gateway/Cargo.tomlmodel_gateway/src/main.rsmodel_gateway/src/mesh/adapters/worker_sync.rsmodel_gateway/src/mesh/wiring.rsmodel_gateway/src/worker/registry.rs
CatherineSue
force-pushed
the
chang/mesh-deterministic-ids
branch
from
d595755 to
cf91f0d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf91f0d1a6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // workers under their previous mesh keys, so tombstones resolve | ||
| // and no orphan keys form. Installed here because this runs | ||
| // before the worker-init workflows register anything. | ||
| worker_registry.set_worker_id_namespace(&node_name); |
There was a problem hiding this comment.
Fresh evidence: the Python config builder still falls back to a random Mesh_#### name when mesh_server_name is omitted (bindings/python/src/lib.rs:1216-1221), and server startup passes that value into MeshAdapters::start here. In Python/Docker launches with mesh enabled but no explicit name, this line therefore seeds deterministic worker IDs from a different namespace on every restart, so worker:{id} keys change and old mesh-imported workers remain under stale keys; the stable default/requirement needs to be applied to every MeshServerConfig producer before using node_name as the ID namespace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3d85ca. The Python config builder now derives the default self_name from the advertise address (Mesh_{advertise_addr} with :→-), mirroring the main.rs default (cf91f0d), so the deterministic worker-id namespace is restart-stable for Python/Docker launches with mesh enabled but no explicit name. An explicit mesh_server_name still takes precedence.
(Out of scope here: the explicit-name path in the Python builder still skips validate_mesh_server_name, unlike main.rs — noting as a separate hardening follow-up.)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/python/src/lib.rs`:
- Around line 1249-1251: The self.mesh_server_name field in the Python binding
is being used without validation, unlike the main.rs implementation which uses
validate_mesh_server_name. Before using the cloned mesh_server_name value in the
format call, add validation to ensure it is not empty and does not contain the
colon character (:), which is the mesh KV shard-key separator. Apply the same
validation logic that exists in main.rs to check the user-provided name and
reject invalid values to prevent downstream issues in mesh operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 884acb7a-a4ec-41aa-b334-a071bc71d18d
📒 Files selected for processing (1)
bindings/python/src/lib.rs
|
Hi @CatherineSue, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
A restarted node previously minted fresh v7 ids, orphaning its old
worker:{id} keys cluster-wide (one per worker per restart, never
reclaimable) and breaking tombstone resolution on peers that imported
under the old id. Ids now derive from (node_name, url) via chained
UUIDv5 namespacing, so the same node republishes the same keys across
restarts; mesh-off deployments keep random ids. MeshAdapters::start
installs the identity before the worker-init workflows register
anything; reservations derive identically.
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
…ation Restart-stable ids reuse keys, but the restarted node's Lamport clock starts fresh: its first publish loses LWW to the prior incarnation's higher-timestamp ops once peers gossip them back, shadowing the live state (indefinitely for disable_health_check workers, which emit no further status transitions) and resurrecting pre-restart tombstones. The reconcile pass now re-puts locally-owned state it finds mismatched in the store — by then the clock has observed the stale timestamps via the merge, so the re-put wins and the shadow is bounded to one pass. Found by adversarial review of the deterministic-ids commit. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The auto-generated default was Mesh_{4 random chars} per boot, which
silently defeated deterministic worker ids for any deployment without
an explicit --mesh-server-name (a fresh uuid5 namespace every restart
re-keys every worker) and left one ghost mesh member behind per
restart. The advertise address is required, routable, and unique per
node, so it provides a restart-stable default identity; an explicit
name still takes precedence.
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The Python config builder fell back to a random Mesh_#### name when
mesh_server_name was omitted, so on Python/Docker launches with mesh
enabled but no explicit name, the deterministic worker-id namespace
reseeded every restart and old mesh-imported workers stayed under stale
worker:{id} keys. Mirror the main.rs default: derive the name from the
required, routable, per-node-unique advertise address.
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
CatherineSue
force-pushed
the
chang/mesh-deterministic-ids
branch
from
e3d85ca to
6878f7a
Compare
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/mesh/wiring.rs (1)
41-59: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate
node_namebefore installing the worker namespace.
Callingset_worker_id_namespace(&node_name)first can leave a sharedWorkerRegistrystuck with an invalid namespace if startup panics later; a retry on the same registry will keep that bad value. Moving the validation ahead of this call avoids the permanent poison case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/mesh/wiring.rs` around lines 41 - 59, Validate node_name before calling worker_registry.set_worker_id_namespace in Mesh wiring start, because the shared WorkerRegistry can be left with a bad namespace if startup fails afterward. Move the node_name validation ahead of set_worker_id_namespace in start, and only install the worker namespace after the name has been confirmed valid; keep the rest of the startup flow (configure_crdt_prefix, WorkerSyncAdapter::new, RateLimitSyncAdapter::new, worker.start, rate_limit.start) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/src/main.rs`:
- Around line 1556-1566: Reject runtime_worker_threads values of 0 before
creating the Tokio runtime in main, since the match arm that calls
tokio::runtime::Builder::new_multi_thread().worker_threads(n) will panic on
Some(0). Add validation in the server_config/runtime configuration path or guard
this branch so 0 falls back to the default runtime sizing, and keep the existing
runtime initialization flow otherwise unchanged.
---
Outside diff comments:
In `@model_gateway/src/mesh/wiring.rs`:
- Around line 41-59: Validate node_name before calling
worker_registry.set_worker_id_namespace in Mesh wiring start, because the shared
WorkerRegistry can be left with a bad namespace if startup fails afterward. Move
the node_name validation ahead of set_worker_id_namespace in start, and only
install the worker namespace after the name has been confirmed valid; keep the
rest of the startup flow (configure_crdt_prefix, WorkerSyncAdapter::new,
RateLimitSyncAdapter::new, worker.start, rate_limit.start) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2966de62-2eab-4e08-81a4-c223bd0b194b
📒 Files selected for processing (6)
bindings/python/src/lib.rsmodel_gateway/Cargo.tomlmodel_gateway/src/main.rsmodel_gateway/src/mesh/adapters/worker_sync.rsmodel_gateway/src/mesh/wiring.rsmodel_gateway/src/worker/registry.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/mesh/wiring.rs (1)
41-59: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate
node_namebefore installing the worker namespace.
Callingset_worker_id_namespace(&node_name)first can leave a sharedWorkerRegistrystuck with an invalid namespace if startup panics later; a retry on the same registry will keep that bad value. Moving the validation ahead of this call avoids the permanent poison case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/mesh/wiring.rs` around lines 41 - 59, Validate node_name before calling worker_registry.set_worker_id_namespace in Mesh wiring start, because the shared WorkerRegistry can be left with a bad namespace if startup fails afterward. Move the node_name validation ahead of set_worker_id_namespace in start, and only install the worker namespace after the name has been confirmed valid; keep the rest of the startup flow (configure_crdt_prefix, WorkerSyncAdapter::new, RateLimitSyncAdapter::new, worker.start, rate_limit.start) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/src/main.rs`:
- Around line 1556-1566: Reject runtime_worker_threads values of 0 before
creating the Tokio runtime in main, since the match arm that calls
tokio::runtime::Builder::new_multi_thread().worker_threads(n) will panic on
Some(0). Add validation in the server_config/runtime configuration path or guard
this branch so 0 falls back to the default runtime sizing, and keep the existing
runtime initialization flow otherwise unchanged.
---
Outside diff comments:
In `@model_gateway/src/mesh/wiring.rs`:
- Around line 41-59: Validate node_name before calling
worker_registry.set_worker_id_namespace in Mesh wiring start, because the shared
WorkerRegistry can be left with a bad namespace if startup fails afterward. Move
the node_name validation ahead of set_worker_id_namespace in start, and only
install the worker namespace after the name has been confirmed valid; keep the
rest of the startup flow (configure_crdt_prefix, WorkerSyncAdapter::new,
RateLimitSyncAdapter::new, worker.start, rate_limit.start) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2966de62-2eab-4e08-81a4-c223bd0b194b
📒 Files selected for processing (6)
bindings/python/src/lib.rsmodel_gateway/Cargo.tomlmodel_gateway/src/main.rsmodel_gateway/src/mesh/adapters/worker_sync.rsmodel_gateway/src/mesh/wiring.rsmodel_gateway/src/worker/registry.rs
🛑 Comments failed to post (1)
model_gateway/src/main.rs (1)
1556-1566: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
tokio Builder worker_threads panic zero assertion💡 Result:
In the Tokio runtime, the
Builder::worker_threadsmethod is designed to set the number of worker threads for the multi-threaded runtime [1][2]. This method contains an assertion that causes a panic if the provided value is 0 [3][4]. Specifically, the implementation uses the following assertion: assert!(val > 0, "Worker threads cannot be set to 0"); [3][4] Because of this assertion, attempting to configure the runtime with zero worker threads will trigger a panic at runtime during the builder configuration process [1][3]. Historically, documentation for this method sometimes incorrectly stated that callingworker_threadswould panic if used with thecurrent_threadruntime flavor [5][6]. However, this was clarified in later versions: theworker_threadsmethod does not panic when used withcurrent_thread; instead, the setting is simply ignored, as thecurrent_threadruntime does not use a pool of worker threads [5][7]. The explicit panic behavior remains strictly tied to passing a value of 0 [1][3].Citations:
- 1: https://docs.rs/tokio/latest/tokio/runtime/struct.Builder.html
- 2: https://docs.rs/tokio/latest/tokio/attr.main.html?search=
- 3: https://docs.rs/tokio/latest/src/tokio/runtime/builder.rs.html
- 4: https://github.com/tokio-rs/tokio/blob/master/tokio/src/runtime/builder.rs
- 5: tokio-rs/tokio#4849
- 6: https://docs.rs/tokio/1.20.1/tokio/runtime/struct.Builder.html
- 7: https://docs.rs/tokio/1.47.1/tokio/runtime/struct.Builder.html
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the runtime_worker_threads config definition and validation paths. rg -n --hidden --no-ignore-vcs "runtime_worker_threads|worker_threads\\(" model_gateway/src main.rs model_gateway || true # Map the main file around the cited lines and any config/validation modules. for f in \ model_gateway/src/main.rs \ model_gateway/src/config.rs \ model_gateway/src/lib.rs do if [ -f "$f" ]; then echo "== $f ==" wc -l "$f" fi done # Show the relevant slice around the cited code. sed -n '1535,1585p' model_gateway/src/main.rs # Find any explicit validation that rejects zero. rg -n --hidden --no-ignore-vcs "must be greater than 0|> 0|!= 0|Some\\(0\\)|runtime_worker_threads.*validate|validate.*runtime_worker_threads" model_gateway/src || trueRepository: lightseekorg/smg
Length of output: 18228
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the config validation path for runtime_worker_threads specifically. sed -n '1,260p' model_gateway/src/config/validation.rs printf '\n==== runtime_worker_threads references ====\n' rg -n "runtime_worker_threads" model_gateway/src/config model_gateway/src/main.rs model_gateway/src/server.rs || true printf '\n==== server_config validation / conversion call sites ====\n' rg -n "\.validate\(\)\?|to_server_config|to_router_config" model_gateway/src/main.rs model_gateway/src/config model_gateway/src/server.rs || true # Show the exact validation helpers around any generic >0 checks that might cover this field. rg -n "Must be > 0|Port must be > 0|health_check_port|queue_size|worker_threads" model_gateway/src/config/validation.rs || trueRepository: lightseekorg/smg
Length of output: 16078
Reject
runtime_worker_threads = 0before building the Tokio runtime
Some(0)still reachesBuilder::worker_threads(0), which panics withWorker threads cannot be set to 0. Reject 0 in config validation or fall back to the default here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model_gateway/src/main.rs` around lines 1556 - 1566, Reject runtime_worker_threads values of 0 before creating the Tokio runtime in main, since the match arm that calls tokio::runtime::Builder::new_multi_thread().worker_threads(n) will panic on Some(0). Add validation in the server_config/runtime configuration path or guard this branch so 0 falls back to the default runtime sizing, and keep the existing runtime initialization flow otherwise unchanged.
…espace install The Python config builder accepted any explicit mesh_server_name while main.rs rejects empty names and ':' (the rl shard-key separator) — mirror that validation. In MeshAdapters::start, enforce the same invariant before set_worker_id_namespace so an invalid name cannot be captured by the registry's first-writer-wins id namespace ahead of the panic that rejects it. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
|
Re the two review-body findings from the latest CodeRabbit pass:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e3a71d7ed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // and unique per node, so it yields the same identity across | ||
| // restarts (deterministic worker ids, no ghost member per boot). | ||
| // ':' is the rl shard-key separator, hence the '-'. | ||
| format!("Mesh_{}", advertise_addr.to_string().replace(':', "-")) |
There was a problem hiding this comment.
Separate from the earlier membership-version concern, this fallback makes the CRDT replica identity stable for default mesh deployments because MeshKV::new derives replica_id from server_name (crates/mesh/src/kv.rs:485-486), while the Lamport clock still restarts at 0 and LWW drops remote ops solely by (replica_id, timestamp) (crates/mesh/src/crdt_kv/engine/lww.rs:373-384). After a restart without --mesh-server-name, the node republishes workers with the same replica id and low timestamps; peers that still have the prior incarnation's log treat those updates as duplicates, even for changed specs or different worker keys, and local reconcile will not re-put because its own store already matches. Keep the mesh/CRDT identity per-incarnation or persist/bump the clock, and use a separate stable namespace for deterministic worker IDs; the mirrored Python fallback at bindings/python/src/lib.rs:1271 has the same issue.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified against the code and declining — the premise does not hold: the stable name never reaches the CRDT replica identity.
- The id LWW stamps ops with (and dedups by) is
ReplicaId(Uuid::now_v7()), minted fresh per boot:MeshKV::newbuilds the store viaCrdtOrMap::new()→ReplicaId::new()(crates/mesh/src/crdt_kv/crdt.rs:62). A restarted node is a brand-new replica to the CRDT layer, so its(replica_id, timestamp)op-ids cannot collide with the prior incarnation's and peers never drop them as duplicates. - The
hash(server_name)u64 at crates/mesh/src/kv.rs:485-486 is a separate field consumed only byValueEntry— scaffolding marked#[expect(dead_code)]for later v1-compat work; theMeshKV::replica_id()accessor has no production callers. It is not the LWW op identity. - The residual cross-incarnation effect is the one this PR already handles: the prior incarnation's higher-timestamp ops can shadow the new replica's writes under LWW until the first merge, after which the clock has observed those timestamps and
reconcile_oncere-asserts locally-owned state (bounded to one pass) — c75ba34.
Agreed the dead ValueEntry.replica_id scaffolding is confusing — happy to remove it in a separate cleanup PR so it can't be mistaken for the live identity.
|
Hi @CatherineSue, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
1 participant
Description
Problem
Worker ids were random UUIDv7 minted per registration, so every gateway restart re-keyed every worker: the old
worker:{id}keys were orphaned cluster-wide (one per worker per restart, unreclaimable — only the owner may tombstone, and the owner no longer knows the id), and tombstones for the new id never resolved on peers still holding the import under the old id.Solution
Ids derive from
(node_name, url)via chained UUIDv5 namespacing — the same node republishes the same keys across restarts, so tombstones resolve and no orphan keys form.MeshAdapters::startinstalls the node identity before the worker-init workflows register anything; reservations (reserve_id_for_url) derive identically; mesh-off deployments keep random v7 ids.The subtle part (found by adversarial review, fixed in-PR): key reuse re-introduces same-key contention that random ids structurally avoided. A restarted node's Lamport clock starts fresh, so its first publish loses LWW to its own prior incarnation's ops once peers gossip them back — shadowing live state and resurrecting pre-restart tombstones. The reconcile pass now re-asserts locally-owned state it finds mismatched in the store; by reconcile time the clock has observed the stale timestamps via the merge, so the re-put wins and the shadow is bounded to one ~30s pass. (Builds on the
store_matchescomparison from the parent PR.)Known limitations (honest scoping)
Changes
model_gateway/src/worker/registry.rs—WorkerId::derived(chained v5 — collision-free without separator conventions),id_namespace: OnceLock,set_worker_id_namespace, deterministic minting at both mint sites (shard guards never held across the mint)model_gateway/src/mesh/wiring.rs— namespace install inMeshAdapters::startmodel_gateway/src/mesh/adapters/worker_sync.rs— reconcile re-assert on store mismatch; module-doc gap updatedmodel_gateway/Cargo.toml— uuidv5featureTest Plan
cargo test -p smggreen;cargo clippy --all-targets --all-features -- -D warningsclean.deterministic_ids_survive_reregistration(register→remove→re-register → same id; reservations agree),derived_ids_differ_across_nodes_and_urls(incl. the byte-shifting collision case),random_ids_without_namespace(mesh-off unchanged),start_installs_deterministic_worker_ids(wiring),reconcile_reasserts_local_state_over_stale_store_value(the shadow fix).Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses🤖 Generated with Claude Code
Summary by CodeRabbit