Repository navigation
refactor(mesh): remove v1 mesh from gateway and public API - #1476
Conversation
📝 WalkthroughWalkthroughRemoves v1 mesh synchronization and HA HTTP surface from model_gateway, consolidates mesh crate exports, adds a stable 8-byte hashing module and a shared serde ChangesMesh crate: hashing, wire types, and re-exports
Model Gateway: removal of v1 mesh sync and HA admin surface
Sequence Diagram(s)(omitted — change is a broad removal/refactor across many components; no new multi-component sequential control flow to visualize) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labelsci, mesh Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 refactors the mesh networking layer by removing legacy "v1" synchronization logic and the MeshSyncManager hooks from the gateway. Key changes include moving shared value types like WorkerState to a new types module, introducing a hash module for stable content hashing, and deleting the v1 management HTTP routes and integration tests. Feedback focuses on the safety of the Eq implementation for types containing floats and an optimization to avoid intermediate allocations during token hashing.
| // Manual Eq via the derived PartialEq; the f64 comparison is | ||
| // bitwise via `PartialEq`, which is acceptable because the | ||
| // gateway either advertises a deterministic value or zero. | ||
| impl Eq for WorkerState {} |
There was a problem hiding this comment.
Implementing Eq for a struct containing f64 is problematic because f64 does not satisfy the reflexivity requirement of Eq (specifically, NaN != NaN). If WorkerState.load ever becomes NaN, this implementation will violate the Eq trait contract, which can lead to logic errors or panics in collections that rely on it. Additionally, the comment on line 32 mentions an "epsilon discipline" for equality, but the struct uses derived PartialEq which performs standard float comparison. Consider using a wrapper type like ordered_float or removing the Eq implementation if it's not strictly required.
| pub fn hash_token_path(tokens: &[u32]) -> u64 { | ||
| let bytes: Vec<u8> = tokens.iter().flat_map(|t| t.to_le_bytes()).collect(); | ||
| let hash = blake3::hash(&bytes); |
There was a problem hiding this comment.
The hash_token_path function currently allocates a Vec<u8> to store the byte representation of all tokens before hashing. For large requests (e.g., 32K tokens), this results in a ~128KB intermediate allocation. Since blake3::Hasher supports incremental updates, you can avoid this allocation by updating the hasher directly in a loop.
pub fn hash_token_path(tokens: &[u32]) -> u64 {
let mut hasher = blake3::Hasher::new();
for t in tokens {
hasher.update(&t.to_le_bytes());
}
let hash = hasher.finalize();
let h = u64::from_le_bytes(hash.as_bytes()[..8].try_into().unwrap());
if h == GLOBAL_EVICTION_HASH {
1
} else {
h
}
}References
- When computing hashes from a slice of primitives, avoid unnecessary intermediate allocations by using the streaming interface of the hashing library to write bytes incrementally.
![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: a7d9698eab
ℹ️ 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".
| // v1 mesh sync (set_mesh_sync, WorkerStateSubscriber, TreeStateSubscriber) | ||
| // is removed in this PR. State sync across mesh peers is not wired in this | ||
| // branch — v2 adapters in `model_gateway/src/mesh/adapters/` are built and | ||
| // tested but not yet started from `server.rs`. That wiring lands in a |
There was a problem hiding this comment.
With mesh enabled, startup now explicitly skips all sync wiring and does not start any v2 adapters, so worker/tree/rate-limit state never replicates across peers. I checked model_gateway/src and the only WorkerSyncAdapter/TreeSyncAdapter/RateLimitSyncAdapter constructions are in adapter test code, so production startup has no inbound or outbound mesh sync path; this breaks multi-node routing/failover behavior until another change lands.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| pub const GLOBAL_EVICTION_HASH: u64 = 0; | ||
|
|
||
| /// Compute a compact 8-byte hash of a prefix path. Returns a | ||
| /// non-zero hash; `0` is reserved for [`GLOBAL_EVICTION_HASH`]. | ||
| #[expect( | ||
| clippy::unwrap_used, | ||
| reason = "blake3 always returns 32 bytes; [..8] into [u8; 8] cannot fail" | ||
| )] | ||
| pub fn hash_node_path(path: &str) -> u64 { | ||
| let hash = blake3::hash(path.as_bytes()); | ||
| let h = u64::from_le_bytes(hash.as_bytes()[..8].try_into().unwrap()); | ||
| if h == GLOBAL_EVICTION_HASH { | ||
| 1 | ||
| } else { | ||
| h | ||
| } | ||
| } | ||
|
|
||
| /// Compute a compact 8-byte hash of a token-id sequence. Returns | ||
| /// a non-zero hash; `0` is reserved for [`GLOBAL_EVICTION_HASH`]. | ||
| #[expect( | ||
| clippy::unwrap_used, | ||
| reason = "blake3 always returns 32 bytes; [..8] into [u8; 8] cannot fail" | ||
| )] | ||
| pub fn hash_token_path(tokens: &[u32]) -> u64 { | ||
| let bytes: Vec<u8> = tokens.iter().flat_map(|t| t.to_le_bytes()).collect(); | ||
| let hash = blake3::hash(&bytes); | ||
| let h = u64::from_le_bytes(hash.as_bytes()[..8].try_into().unwrap()); | ||
| if h == GLOBAL_EVICTION_HASH { | ||
| 1 | ||
| } else { | ||
| h |
There was a problem hiding this comment.
🟡 Nit: GLOBAL_EVICTION_HASH, hash_node_path, and hash_token_path are now defined in both hash.rs and tree_ops.rs (lines 86–129) with identical implementations. Internal callers (sync.rs:44, sync.rs:551) still reference crate::tree_ops::GLOBAL_EVICTION_HASH.
This duplication is a maintenance hazard — if someone updates the hash logic in one module, the other silently diverges. Consider having tree_ops.rs re-export from crate::hash instead of carrying its own copy:
// in tree_ops.rs, replace the local definitions with:
pub use crate::hash::{hash_node_path, hash_token_path, GLOBAL_EVICTION_HASH};![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model_gateway/src/mesh/adapters/worker_sync.rs (1)
97-100:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftTombstones are now dropped, so remote removals never leave the registry.
When one node calls
on_worker_removed(), peers hit thisNonebranch and only log it. The imported worker then stays registered and potentially routable on every other gateway until some unrelated local cleanup happens. This PR removes the old path, so a real remote-remove hook needs to land with it or worker deletions stop propagating cluster-wide.🤖 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/adapters/worker_sync.rs` around lines 97 - 100, The None branch in worker_sync.rs currently only logs a tombstone and drops it, so remote removals never clear the registry; replace the debug-only branch with a call into the remote-remove handler used locally (invoke the same removal flow as on_worker_removed or a new Registry::handle_remote_worker_removal/registry.remove_worker_remote(worker_id) helper) so the imported worker is actually removed and the removal is propagated via the existing registry/mesh hooks; ensure you pass worker_id and any mesh context, update any function signatures invoked (e.g., on_worker_removed) if needed, and keep logging for success/failure rather than silently dropping the tombstone.model_gateway/src/worker/registry.rs (1)
1237-1246:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDifferentiate between absent and corrupted specs—don't silently register a fallback worker when
specis present but undecodable.Empty
specis the old-node compatibility path and safely falls back to the minimal builder. A non-empty decode failure, however, signals corruption or schema drift and should not be hidden. Silent fallback registers a worker with defaultworker_type,connection_mode,runtime_type, and only the singlemodel_idfrom state—potentially wrong connection mode, worker type, model set, and other config. This mis-configured worker can then route traffic incorrectly.💡 Suggested fix
- let worker = match bincode::deserialize::<openai_protocol::worker::WorkerSpec>(&state.spec) - { - Ok(spec) if !state.spec.is_empty() => { - super::builder::BasicWorkerBuilder::from_spec(spec).build() - } - _ => super::builder::BasicWorkerBuilder::new(&state.url) - .model(ModelCard::new(&state.model_id)) - .build(), - }; + let worker = if state.spec.is_empty() { + super::builder::BasicWorkerBuilder::new(&state.url) + .model(ModelCard::new(&state.model_id)) + .build() + } else { + match bincode::deserialize::<openai_protocol::worker::WorkerSpec>(&state.spec) { + Ok(spec) => super::builder::BasicWorkerBuilder::from_spec(spec).build(), + Err(err) => { + tracing::warn!(url = %state.url, %err, "dropping mesh worker with invalid spec"); + return; + } + } + };🤖 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/worker/registry.rs` around lines 1237 - 1246, The code currently falls back to a minimal worker whenever bincode::deserialize::<openai_protocol::worker::WorkerSpec>(&state.spec) fails, which hides the difference between an absent/empty spec and a corrupted/non-decodable spec; change the logic in the match that constructs the worker so that: if state.spec is empty keep the existing fallback using BasicWorkerBuilder::new(&state.url).model(ModelCard::new(&state.model_id)).build(), but if state.spec is non-empty and deserialization returns Err treat it as a hard failure (do not silently construct a minimal worker) — surface or return the error (or skip registration) and include the deserialization error context referencing WorkerSpec, state.spec, and BasicWorkerBuilder::from_spec so callers can handle corrupted/spec-schema-drift cases appropriately.
🤖 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 `@crates/mesh/src/types.rs`:
- Around line 33-48: WorkerState implements Eq despite having an f64 field
(load) which can be NaN, breaking reflexivity; either validate load on
deserialization or use a non-NaN wrapper before asserting Eq. Fix by adding
runtime validation where WorkerState instances are created from untrusted input
(e.g., in model_gateway::mesh::adapters::worker_sync.rs where
bincode::deserialize::<WorkerState>() is called) to reject or normalize NaN
loads, or replace the f64 load with a guaranteed non-NaN type (or newtype)
inside the WorkerState definition and adjust the impl std::hash::Hash and impl
Eq accordingly so Eq is only implemented for a type that cannot contain NaN.
In `@model_gateway/src/middleware/concurrency.rs`:
- Around line 205-209: The cluster-wide gating was removed (the
MeshSyncManager::check_global_rate_limit call) which causes global limits to
degrade to per-node only; restore cluster enforcement until v2 exists by
reintroducing the MeshSyncManager::check_global_rate_limit invocation in the
same path where local token-bucket logic runs (the concurrency middleware in
concurrency.rs), have it short‑circuit/return an error when the global check
fails, and keep the existing per-node token bucket as a fallback; ensure the
restored call uses the same return/err semantics as prior code and add a
unit/integration test exercising multiple nodes (or a mocked MeshSyncManager) to
prevent regression.
---
Outside diff comments:
In `@model_gateway/src/mesh/adapters/worker_sync.rs`:
- Around line 97-100: The None branch in worker_sync.rs currently only logs a
tombstone and drops it, so remote removals never clear the registry; replace the
debug-only branch with a call into the remote-remove handler used locally
(invoke the same removal flow as on_worker_removed or a new
Registry::handle_remote_worker_removal/registry.remove_worker_remote(worker_id)
helper) so the imported worker is actually removed and the removal is propagated
via the existing registry/mesh hooks; ensure you pass worker_id and any mesh
context, update any function signatures invoked (e.g., on_worker_removed) if
needed, and keep logging for success/failure rather than silently dropping the
tombstone.
In `@model_gateway/src/worker/registry.rs`:
- Around line 1237-1246: The code currently falls back to a minimal worker
whenever
bincode::deserialize::<openai_protocol::worker::WorkerSpec>(&state.spec) fails,
which hides the difference between an absent/empty spec and a
corrupted/non-decodable spec; change the logic in the match that constructs the
worker so that: if state.spec is empty keep the existing fallback using
BasicWorkerBuilder::new(&state.url).model(ModelCard::new(&state.model_id)).build(),
but if state.spec is non-empty and deserialization returns Err treat it as a
hard failure (do not silently construct a minimal worker) — surface or return
the error (or skip registration) and include the deserialization error context
referencing WorkerSpec, state.spec, and BasicWorkerBuilder::from_spec so callers
can handle corrupted/spec-schema-drift cases appropriately.
🪄 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: 6b42962d-941b-4550-82af-55e084d2d17d
📒 Files selected for processing (18)
crates/mesh/Cargo.tomlcrates/mesh/benches/mesh_serialization.rscrates/mesh/src/hash.rscrates/mesh/src/lib.rscrates/mesh/src/stores.rscrates/mesh/src/sync.rscrates/mesh/src/types.rsmodel_gateway/src/mesh/adapters/worker_sync.rsmodel_gateway/src/middleware/concurrency.rsmodel_gateway/src/policies/cache_aware.rsmodel_gateway/src/policies/mod.rsmodel_gateway/src/policies/registry.rsmodel_gateway/src/routers/mesh/handlers.rsmodel_gateway/src/routers/mesh/mod.rsmodel_gateway/src/routers/mod.rsmodel_gateway/src/server.rsmodel_gateway/src/worker/registry.rsmodel_gateway/tests/mesh_integration_test.rs
💤 Files with no reviewable changes (9)
- model_gateway/src/routers/mesh/mod.rs
- model_gateway/src/routers/mod.rs
- crates/mesh/benches/mesh_serialization.rs
- model_gateway/tests/mesh_integration_test.rs
- model_gateway/src/policies/mod.rs
- crates/mesh/src/sync.rs
- crates/mesh/Cargo.toml
- model_gateway/src/routers/mesh/handlers.rs
- model_gateway/src/policies/registry.rs
The v1 mesh sync (MeshSyncManager, TreeOperation, TreeState, etc.)
is replaced by the v2 TreeSync / WorkerSync / RateLimitSync adapters
that landed in prior PRs. This PR rips out every gateway-side caller
of v1 and drops the v1 types from the smg-mesh public surface so a
follow-up can redesign the e2e topology / service-discovery without
the legacy weight.
Gateway-side removals:
- `model_gateway/src/server.rs`: drop the mesh_routes admin tree,
start_rate_limit_task wiring, mesh_handler-driven set_mesh_sync /
register_worker_state_subscriber.
- `model_gateway/src/middleware/concurrency.rs`: drop the
`MeshSyncManager::check_global_rate_limit` gate. Cluster-wide
rate limiting will return via the v2 RateLimitSyncAdapter.
- `model_gateway/src/worker/registry.rs`: drop the OptionalMeshSyncManager
field, set_mesh_sync, all sync_worker_state / remove_worker_state
calls, and the v1 `WorkerStateSubscriber` trait impl. The remote-
apply path (`on_remote_worker_state`) survives as an inherent
method; the v2 `WorkerSyncAdapter` is its only caller.
- `model_gateway/src/policies/{mod,registry,cache_aware}.rs`: drop
the mesh_sync field on PolicyRegistry / CacheAwarePolicy and every
sync_tree_insert_hash / sync_tree_operation / apply_remote_tree_*
/ apply_tenant_delta / export_tree_state / restore_tree_state hook.
`apply_repair_page`, the v2 cold-start path, is kept as-is and a
repurposed round-trip test now seeds via that path.
- `model_gateway/src/routers/mesh/`: delete the v1 admin endpoints
(cluster_status, ha/*, app_config, etc.) wholesale.
- `model_gateway/tests/mesh_integration_test.rs`: delete; covered
the v1 sync manager surface.
smg-mesh surface:
- Stop re-exporting MeshSyncManager, StateStores, TreeOperation,
TreeState, TreeInsertOp, TreeKey, TenantInsert, TenantEvict,
WorkerStateSubscriber, TreeStateSubscriber, AppState, PolicyState,
MembershipState, RateLimitConfig, GLOBAL_RATE_LIMIT_KEY /
COUNTER_KEY, OptionalMeshSyncManager.
- Move the still-needed pieces (`hash_node_path`, `hash_token_path`,
`GLOBAL_EVICTION_HASH`, `WorkerState`) into fresh `hash.rs` and
`types.rs` modules so they survive the v1 file deletion in the
follow-up PR.
- Delete `crates/mesh/benches/mesh_serialization.rs`; it benchmarked
v1 TreeState codec performance.
The legacy v1 modules (sync.rs, stores.rs, tree_ops.rs, collector.rs,
consistent_hash.rs, rate_limit_window.rs) remain as private `mod`
declarations during the transition. The inbound gossip dispatch in
controller.rs / ping_server.rs / node_state_machine.rs still
references their types; deleting that requires gutting the dispatch
loop, which is the topology / service_discovery redesign tracked in
the follow-up PR.
Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The bench file `crates/mesh/benches/mesh_serialization.rs` was deleted in the previous commit (it measured v1 `TreeState` codec performance, which doesn't exist anymore). The workflow that runs it via `cargo bench --bench mesh_serialization` was left behind and now fails with `no bench target named mesh_serialization in smg-mesh`. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
`WorkerState::load` is `f64`, which can be NaN. NaN violates `Eq`'s reflexivity (`NaN != NaN`), so `impl Eq for WorkerState` is unsound and a malicious or buggy peer could trip it by gossiping a non-finite load. The manual `Hash` impl had the same root issue (coerced `f64` to `i64`, also load-dependent). Carried over verbatim from v1 `stores.rs:340`; not introduced in this PR. No call site in the workspace keys a `HashMap`/`HashSet` by `WorkerState` or otherwise requires `Eq`/`Hash` trait bounds, so deletion costs nothing. Derived `PartialEq` still covers `assert_eq!` in tests and the rare field-by-field comparison. Flagged by Gemini and CodeRabbit on PR #1476. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
The previous implementation collected all `u32` tokens into a `Vec<u8>` via `flat_map(|t| t.to_le_bytes()).collect()` before hashing. At 32K tokens × 4 bytes that's a ~128 KB transient allocation on every request hot path that calls into the cache-aware policy. Switch to `blake3::Hasher::update` per token. Blake3's streaming construction is mathematically identical to hashing the concatenated bytes — a regression test (`hash_token_path_matches_concat_then_hash`) locks in wire-compatibility with hashes already stored in the CRDT or held by peers running the prior version. Carried over from v1 `tree_ops.rs:120`; not introduced by this PR. Flagged by Gemini on PR #1476. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
Duplicate definitions of GLOBAL_EVICTION_HASH, hash_node_path, and hash_token_path lived in both modules. tree_ops.rs now re-exports from crate::hash so internal v1 call sites (sync.rs:44, 551) keep resolving while the canonical implementations stay in one place. Flagged by Claude on PR #1476. Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
219dc5b to
dadc884
Compare
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/server.rs`:
- Around line 1311-1315: startup() currently accepts mesh_server_config and
proceeds to start the mesh/server-discovery path even though v2 peer-state sync
isn’t wired here; update startup() in server.rs to validate mesh_server_config
early and fail fast (return an error) or explicitly skip mesh startup with a
loud error-level log and non-zero exit when the v2 adapters in
model_gateway/src/mesh/adapters/ are not being started; check for the
presence/enablement flag for the v2 adapters (or a boolean like
mesh_server_config.enabled) and if set while adapters aren’t wired, refuse to
proceed or disable mesh with a clear error referencing mesh_server_config and
startup().
🪄 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: ea769b9f-d004-4e3a-bc81-55c6ab329040
📒 Files selected for processing (20)
.github/workflows/benchmark-mesh-serialization.ymlcrates/mesh/Cargo.tomlcrates/mesh/benches/mesh_serialization.rscrates/mesh/src/hash.rscrates/mesh/src/lib.rscrates/mesh/src/stores.rscrates/mesh/src/sync.rscrates/mesh/src/tree_ops.rscrates/mesh/src/types.rsmodel_gateway/src/mesh/adapters/worker_sync.rsmodel_gateway/src/middleware/concurrency.rsmodel_gateway/src/policies/cache_aware.rsmodel_gateway/src/policies/mod.rsmodel_gateway/src/policies/registry.rsmodel_gateway/src/routers/mesh/handlers.rsmodel_gateway/src/routers/mesh/mod.rsmodel_gateway/src/routers/mod.rsmodel_gateway/src/server.rsmodel_gateway/src/worker/registry.rsmodel_gateway/tests/mesh_integration_test.rs
💤 Files with no reviewable changes (10)
- crates/mesh/Cargo.toml
- crates/mesh/benches/mesh_serialization.rs
- model_gateway/src/routers/mesh/mod.rs
- .github/workflows/benchmark-mesh-serialization.yml
- model_gateway/src/routers/mod.rs
- model_gateway/tests/mesh_integration_test.rs
- crates/mesh/src/sync.rs
- model_gateway/src/routers/mesh/handlers.rs
- model_gateway/src/policies/mod.rs
- model_gateway/src/policies/registry.rs
| // v1 mesh sync (set_mesh_sync, WorkerStateSubscriber, TreeStateSubscriber) | ||
| // is removed in this PR. State sync across mesh peers is not wired in this | ||
| // branch — v2 adapters in `model_gateway/src/mesh/adapters/` are built and | ||
| // tested but not yet started from `server.rs`. That wiring lands in a | ||
| // follow-up PR. |
There was a problem hiding this comment.
Fail fast while mesh config still boots a non-functional cluster path.
Line 1311 explicitly says peer-state sync is not wired in this branch, but startup() still accepts mesh_server_config and starts the mesh/server-discovery path earlier. That means a deployment can come up looking mesh-enabled while never replicating remote worker or policy state. Please reject that config (or skip mesh startup with a loud warning) until the v2 adapters are actually started 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/server.rs` around lines 1311 - 1315, startup() currently
accepts mesh_server_config and proceeds to start the mesh/server-discovery path
even though v2 peer-state sync isn’t wired here; update startup() in server.rs
to validate mesh_server_config early and fail fast (return an error) or
explicitly skip mesh startup with a loud error-level log and non-zero exit when
the v2 adapters in model_gateway/src/mesh/adapters/ are not being started; check
for the presence/enablement flag for the v2 adapters (or a boolean like
mesh_server_config.enabled) and if set while adapters aren’t wired, refuse to
proceed or disable mesh with a clear error referencing mesh_server_config and
startup().
…ct#1476) Signed-off-by: Chang Su <8605658+CatherineSue@users.noreply.github.com>
1 participant
Description
Problem
The v1 mesh sync stack (
MeshSyncManager,TreeOperation,TreeState,WorkerStateSubscriber,TreeStateSubscriber, …) was the original gateway↔cluster bridge. It is now superseded by the v2TreeSync/WorkerSync/RateLimitSyncadapters layered on top ofMeshKV's typed CRDT/stream namespaces. The two paths have coexisted while v2 baked; that's over.Leaving v1 in place blocks the next round of work: redesigning the e2e integration (topology /
service_discovery.rs, gossip dispatch incontroller.rs/ping_server.rs/node_state_machine.rs) without dragging the legacy types and admin surface along.Solution
This is the first PR in a short sequence:
smg-mesh. Mesh-internal modules that still reference v1 types remain as privatemoddeclarations during the transition.smg-mesh.The gateway loses every v1 caller, the
routers/mesh/*admin endpoints, the global-rate-limit gate in the concurrency middleware, and the v1 mesh integration test. The v2WorkerSyncAdapterkeeps usingWorkerRegistry::on_remote_worker_state, which survives as an inherent method (was previously theWorkerStateSubscribertrait impl).Changes
Gateway
model_gateway/src/server.rs— drop themesh_routesadmin tree,start_rate_limit_taskwiring, andmesh_handler-drivenset_mesh_sync/register_worker_state_subscriber.model_gateway/src/middleware/concurrency.rs— drop theMeshSyncManager::check_global_rate_limitgate. Cluster-wide rate limiting will return via the v2RateLimitSyncAdapter.model_gateway/src/worker/registry.rs— drop theOptionalMeshSyncManagerfield,set_mesh_sync, allsync_worker_state/remove_worker_statecalls, and the v1WorkerStateSubscriberimpl. The remote-apply methodon_remote_worker_stateis kept as an inherent method (v2WorkerSyncAdaptercalls it).model_gateway/src/policies/{mod,registry,cache_aware}.rs— drop themesh_syncfield onPolicyRegistry/CacheAwarePolicyand everysync_tree_insert_hash/sync_tree_operation/apply_remote_tree_*/apply_tenant_delta/export_tree_state/restore_tree_state_from_meshhook.apply_repair_page(the v2 cold-start path) and the populate-site hash_index are kept; a repurposedtest_apply_known_remote_insert_round_tripnow seeds viaapply_repair_page.model_gateway/src/routers/mesh/{mod,handlers}.rs— delete (admin endpoints for cluster_status,ha/*, app_config, …).model_gateway/tests/mesh_integration_test.rs— delete; exercised only the v1 sync manager surface.smg-mesh
MeshSyncManager,StateStores,TreeOperation,TreeState,TreeInsertOp,TreeKey,TenantInsert,TenantEvict,WorkerStateSubscriber,TreeStateSubscriber,AppState,MembershipState,PolicyState,RateLimitConfig,GLOBAL_RATE_LIMIT_KEY/_COUNTER_KEY,OptionalMeshSyncManager.crates/mesh/src/hash.rs(hash_node_path,hash_token_path,GLOBAL_EVICTION_HASH) andcrates/mesh/src/types.rs(WorkerState) so the still-needed pieces survive the upcoming v1 file deletion.stores.rsre-exportsWorkerStatefromtypes.rsso there is a single canonical type during the transition.crates/mesh/benches/mesh_serialization.rs(benched the v1TreeStatecodec).Scope explicitly deferred to the follow-up PR
sync.rs/stores.rs/tree_ops.rs/collector.rs/consistent_hash.rs/rate_limit_window.rs. They're still referenced internally by gossip dispatch (controller.rs,ping_server.rs,node_state_machine.rs); deleting them requires the topology redesign.Test Plan
Ran against the worktree (
/Users/chang/opensource/smg-v1-removal) after the cut:cargo +nightly fmt --checkpassescargo clippy --all-targets -p smg -p smg-meshpasses (no warnings)cargo test -p smg --lib— 835 passed, 4 ignoredcargo test -p smg-mesh --lib— 262 passed, 2 ignoredcargo test -p smg --lib policies::cache_aware::tests— 21 / 21 (incl. v2apply_repair_pageandapply_known_remote_insert_*paths)cargo test -p smg --lib policies::registry::tests— 3 / 3cargo test -p smg --lib worker::registry::tests— 19 / 19cargo test -p smg --lib mesh::adapters— 62 / 62 (worker_sync round-trip + tree_sync repair + rate_limit_sync backfill still green)Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Bug Fixes
Removals