Add distributed mesh inference for multi-device compute sharing#1
Add distributed mesh inference for multi-device compute sharing#1Jackson57279 wants to merge 15 commits into
Conversation
Features: - Auto-discovery using mDNS + IP scanning (parallel) - mTLS transport with auto-generated Ed25519 certificates - Leader election (oldest node wins) - Route strategy: least-loaded node selection - Split strategy: layer-wise model distribution for large models - Real-time dashboard with OpenWebUI-inspired design - WebSocket-based chat interface with streaming - CLI 'up' command for easy mesh joining Architecture: - MeshDiscovery: mDNS + IP scanner for peer finding - MeshTransport: async mTLS connections - MeshCoordinator: leader election, state sync, job scheduling - DistributedInferenceEngine: transparent routing/splitting - MeshDashboard: FastAPI + Socket.IO + vanilla JS frontend Security: - Automatic TLS certificate generation per-node - Certificate pinning for trusted peers - 30-day certificate rotation Usage: miniforge up # Join mesh with auto-discovery miniforge up --ip 192.168.1.100 # Connect to specific peer
Host/Worker Topology: - Host: Stores all models, streams to workers on-demand - Worker: No local models required, downloads from host automatically - ModelRegistry: Tracks model locations across mesh - Weight Streaming: Chunk-based model transfer (16MB chunks) - Auto-download: Workers fetch models from host on first use CLI Changes: - --mode host|worker|auto: Choose node mode - --host-ip: Specify host IP for workers - --model-path: Register local model for host mode Usage: # Host (has models) miniforge up --mode host --model-path /path/to/model.gguf # Worker (no models, auto-downloads) miniforge up --mode worker --host-ip 192.168.1.100 Benefits: - Download models only once (on host) - Workers join with zero storage requirement - Automatic caching on workers for performance
|
@cubic-dev-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
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:
📝 WalkthroughWalkthroughAdds a new "mesh" optional dependency and implements a Miniforge mesh subsystem (discovery, transport, coordinator, engines, registry, security, dashboard) with CLI wiring; refactors core backend/config/model interfaces; and adds utility scripts and model registry entries. ChangesMesh Feature (types, discovery, transport, coordinator, engines, registry, security, dashboard, CLI wiring)
Project Configuration
Core backend & model refactor
Model registry additions
Utility scripts
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Discovery
participant Transport
participant Coordinator
participant Engine
User->>CLI: mesh up --mode host
CLI->>Discovery: start()
CLI->>Transport: start()
CLI->>Coordinator: start(discovery, transport)
Discovery-->>Coordinator: on_peer_discovered(peer)
Transport-->>Coordinator: on_connection_event("connected", conn)
User->>CLI: generate(prompt)
CLI->>Coordinator: schedule_inference(prompt, strategy="auto")
Coordinator->>Transport: get_connection_by_node_id(node_id)
Transport->>Engine: send("inference_request")
Engine-->>Transport: send("inference_response")
Transport-->>Coordinator: on_connection_event("message", response)
Coordinator->>CLI: job completed / return result
CLI-->>User: display result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
- Reorder route registration (static before template routes) - Add Jinja2 context processors for url() function - Add error handling for template rendering with fallback HTML - Update static file URLs to use Jinja2 url() function
- Move index.html to templates/ directory - Add named static route with append_version=True - Properly configure Jinja2 with FileSystemLoader - Static files now served from static/, templates from templates/
- Provides a non-Jinja2 version for direct file access - Useful for testing static files without running the server - Falls back gracefully if template rendering fails
- Use /static/style.css and /static/app.js directly - Avoids dependency on Jinja2 url() function - More reliable for initial rendering
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (8)
src/miniforge/mesh/cli.py-118-134 (1)
118-134:⚠️ Potential issue | 🟠 Major
--host-ipand--ipflags are effectively no-ops.
- Lines 118-120:
--host-ipis only printed;host_node_idis never populated from it (it staysNonethrough to L174), so worker mode can't actually target a specific host.- Lines 122-134: when
--ipis passed, aPeerInfois constructed and then dropped — there is no call todiscovery/transport/coordinatorto initiate the connection. The log message "Connecting to peer …" is misleading; nothing is connected.Either wire these into the discovery/transport layer (e.g.,
await discovery.add_manual_peer(peer)orawait transport.connect(peer)) or remove the flags until the plumbing exists so users don't rely on advertised-but-broken behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/cli.py` around lines 118 - 134, The --host-ip and --ip flags are no-ops: when args.host_ip is provided you should set the host_node_id or otherwise populate the host target used later (instead of only printing) and when args.ip is provided you must actually hand the constructed PeerInfo to the discovery/transport layer instead of dropping it. Fix by assigning the host target variable used later (e.g., set host_node_id or host_peer based on args.host_ip) and then call the appropriate async API with the created PeerInfo (e.g., await discovery.add_manual_peer(peer) or await transport.connect(peer) or forward it to coordinator.connect_peer) so the CLI actually initiates a connection; ensure the code paths using host_node_id and PeerInfo now receive those values.src/miniforge/mesh/transport.py-350-371 (1)
350-371:⚠️ Potential issue | 🟠 MajorHeartbeats advertise bogus resource values.
ram_available=self.ram_gbreports total RAM as available, andcpu_percent=0.0is constant.MeshCoordinator._select_least_loaded_nodesorts by exactly these two fields, so every node appears equally idle with full RAM, and "least loaded" routing is a no-op. Thepsutilpackage is already standard for this (psutil.virtual_memory().available / 1e9,psutil.cpu_percent(interval=None)). Worth populating before merging, since the routing heuristic is one of the PR's advertised features.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/transport.py` around lines 350 - 371, In _heartbeat_loop replace the bogus resource fields with real metrics: import psutil and send ram_available as psutil.virtual_memory().available / 1e9 and cpu_percent as psutil.cpu_percent(interval=None); ensure values are floats and catch/handle psutil exceptions (fallback to self.ram_gb and 0.0) so the payload still contains ram_available and cpu_percent for MeshCoordinator._select_least_loaded_node to use; add the psutil import at top of the module and keep the existing exception logging behavior.src/miniforge/mesh/coordinator.py-105-130 (1)
105-130:⚠️ Potential issue | 🟠 Major
all_nodesfabricates a stale self-entry on every call.Each property access builds a brand‑new
NodeState(ip="127.0.0.1", port=0, ram_available=0.0, cpu_percent=0.0, ...). Consequences:
total_resources["available_ram_gb"]excludes this node's actual available RAM (always adds 0 for self).is_leaderon the synthetic self is current, butlast_heartbeatdefaults totime.time()at construction, which accidentally makes self always look "fresh" to any caller that relies on it.dashboard.api_nodesexposesip=127.0.0.1,port=0for this node._select_least_loaded_nodesorts including this synthetic self withcpu_percent=0.0, biasing routing toward self.Track self as a real
NodeStatein_nodes(or a dedicatedself._self_state) and updateram_available,cpu_percent,ip,portin the heartbeat/sync loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/coordinator.py` around lines 105 - 130, The all_nodes property is constructing a synthetic self NodeState (in all_nodes) causing stale/wrong values (ip="127.0.0.1", port=0, ram_available=0, cpu_percent=0) which biases total_resources and selection logic; instead stop fabricating self on every call and keep a real mutable self state in the coordinator (either store the coordinator's own NodeState in the existing _nodes mapping keyed by self.node_id or introduce a dedicated attribute self._self_state), ensure heartbeat/sync code updates its ram_available, cpu_percent, ip and port, then change all_nodes to return list(self._nodes.values()) (or include self._self_state) so total_resources, _select_least_loaded_node and dashboard.api_nodes use the up-to-date self information.src/miniforge/mesh/registry.py-131-170 (1)
131-170:⚠️ Potential issue | 🟠 Major
stream_weightsblocks the event loop and has no error-path completion signal.
open(path, "rb")+f.read(CHUNK_SIZE)are synchronous calls in anasync def. On a multi-GB model each 16 MiB read plus the synchronouschunk_callback(which inhost_worker.pyschedulesasyncio.create_task) blocks every other coroutine on the host — heartbeats, peer handshakes, and incoming inference requests all stall during a weight transfer. Consider wrapping reads withasyncio.to_thread(...)or usingaiofiles, and makechunk_callbackawaitable so the host can apply backpressure per chunk.Also: if
f.reador the callback raises mid-stream, the caller never sends a terminatingmodel_chunkwithcomplete=True/ error, so the worker blocks until its 300 s timeout. Afinallythat forwards a terminal frame (or a documented contract that the caller must do so) is worth adding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/registry.py` around lines 131 - 170, stream_weights is doing blocking file I/O and synchronous callbacks inside an async def which stalls the event loop and also lacks a guaranteed terminal signal on errors; modify stream_weights to perform file reads off the event loop (use asyncio.to_thread or aiofiles when reading Path(model.model_path)) and change the chunk_callback contract to be awaitable (await chunk_callback(chunk) instead of calling it directly) so backpressure can be applied (check caller in host_worker.py to await created tasks or accept async callback), and wrap the read/send loop in try/except/finally so a terminal model_chunk frame (complete=True or an error indicator) is always sent even if an exception occurs.src/miniforge/mesh/coordinator.py-369-393 (1)
369-393:⚠️ Potential issue | 🟠 MajorLeader election compares incompatible timestamps.
candidatesmixesself._started_at(when this process booted) with each remote node'slast_heartbeat(when we last heard from it). Pickingminby that key doesn't select the oldest node — it selects whichever value happens to be smaller between "my boot time" and "last time a peer contacted me". On a freshly joined node,last_heartbeatfrom a long‑running peer will be much newer thanself._started_at, so this node will (incorrectly) elect itself.You need the peer's own
started_at, not ours. Add astarted_atfield toNodeState, populate it from the handshake / heartbeat payload, and use it in the key. Also, leader election here is purely local (no consensus / message exchange), so split‑brain is likely — each node will locally elect whoever it thinks is oldest. Consider broadcasting the elected leader and treating divergent opinions deterministically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/coordinator.py` around lines 369 - 393, The election incorrectly mixes this process's boot time (self._started_at) with peers' last_heartbeat; update NodeState to include a started_at timestamp, populate that started_at from the handshake/heartbeat payload handling code (where remote node info is parsed), and in _run_election use each node.started_at (not last_heartbeat) when building candidates so the min key compares actual boot times (e.g., candidates = [(self.node_id, self._started_at)] + [(node.node_id, node.started_at) for node in self._nodes.values()]); after computing winner in _run_election, broadcast the elected leader to peers (or call the existing heartbeat/send API) and implement handling of incoming leader announcements to set _leader_id/_is_leader deterministically to reduce split‑brain.src/miniforge/mesh/dashboard.py-215-253 (1)
215-253:⚠️ Potential issue | 🟠 MajorManual connect bypasses public API and pollutes node table.
Two concerns:
self.coordinator._connect_to_peer(peer)reaches into a private method; the coordinator should expose a publicconnect_to(ip, port)(or similar) that does peer resolution, logging, and state updates atomically.PeerInfo(node_id="unknown", ...)is persisted intoMeshCoordinator._nodeskeyed by"unknown"(seecoordinator._connect_to_peer). A second manual connect will overwrite the first, and leader election / routing will see spuriousnode_id="unknown"entries. Prefer deferring theNodeStateinsert until the realnode_idis learned from the handshake (MINIFORGE_OK|<node_id>|...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/dashboard.py` around lines 215 - 253, api_connect currently constructs a PeerInfo with node_id="unknown" and calls the private coordinator method _connect_to_peer, which both leaks into a private API and inserts a bogus NodeState keyed by "unknown"; instead, add a public coordinator method (e.g., connect_to(ip, port) or connect_peer) that handles peer resolution, connection handshake, logging and only inserts/updates MeshCoordinator._nodes after the real node_id is learned from the handshake, and change api_connect to call that new public method with ip and port (removing creation of PeerInfo and any direct access to _connect_to_peer); ensure the handshake path parses the MINIFORGE_OK response to get the canonical node_id before creating NodeState so repeated manual connects do not overwrite or pollute the node table.src/miniforge/mesh/discovery.py-174-212 (1)
174-212:⚠️ Potential issue | 🟠 Major
_scan_networkbuilds and discards a coroutine list, then duplicates the work.Lines 187‑195 populate
tasks = [...]with coroutines fromself._check_host(...)that are never awaited (RuntimeWarning: coroutine was never awaited, plus no limiting). The real scan then runs again viacheck_with_limitin lines 207‑212, duplicating the host iteration and doing the filtering twice.🧹 Proposed fix
- # Create scan tasks for all hosts - tasks = [] - hosts = list(network.hosts())[:50] # Limit to first 50 hosts - - for host in hosts: - host_str = str(host) - if host_str == local_ip: - continue - for port in ports_to_check: - tasks.append(self._check_host(host_str, port)) - - if not tasks: - return - - # Run checks with semaphore to limit concurrency + hosts = list(network.hosts())[:50] semaphore = asyncio.Semaphore(20) async def check_with_limit(host: str, port: int) -> None: async with semaphore: await self._check_host(host, port) await asyncio.gather(*[ - check_with_limit(str(h), p) - for h in hosts[:50] + check_with_limit(str(h), p) + for h in hosts for p in ports_to_check if str(h) != local_ip ], return_exceptions=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/discovery.py` around lines 174 - 212, _scan_network is creating coroutines into tasks via self._check_host(...) and never awaiting them (causing RuntimeWarning) and then re-iterating hosts to run check_with_limit, duplicating work; fix by removing the unused tasks list and constructing the awaited work once using the concurrency limiter (semaphore) — e.g. build the gather call from check_with_limit(...) for each host/port (skipping local_ip) and await that with return_exceptions=True, or alternatively create tasks that wrap calls with the semaphore and await asyncio.gather on that tasks list; ensure you do not call self._check_host(...) directly without awaiting and keep semaphore/ check_with_limit usage consistent.src/miniforge/mesh/transport.py-1-16 (1)
1-16:⚠️ Potential issue | 🟠 MajorWire SSL context through asyncio calls or remove mTLS claims from module docstring.
The file header advertises "Async mTLS transport" and
MeshSecurityis instantiated, butstart_server()andopen_connection()omit thesslparameter. Thecreate_tls_context()method exists inMeshSecuritybut is never invoked. Connections remain cleartext, allowing any LAN host to intercept or injectinference_request/model_requestmessages. Passssl=self.security.create_tls_context(server_mode=True)tostart_server()andssl=self.security.create_tls_context(server_mode=False)toopen_connection(), or update the docstring to reflect actual TCP-only behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/transport.py` around lines 1 - 16, The module claims "Async mTLS transport" but never wires the TLS context into asyncio calls; ensure MeshSecurity.create_tls_context is used and passed as the ssl argument to asyncio start_server and open_connection: call self.security.create_tls_context(server_mode=True) when invoking start_server() (for server-side sockets) and self.security.create_tls_context(server_mode=False) when invoking open_connection() (for client-side sockets), or if you intentionally want plaintext remove/update the mTLS claim in the module docstring and delete the unused MeshSecurity.create_tls_context usage to avoid misleading documentation; locate references to MeshSecurity, start_server(), open_connection(), and create_tls_context() in transport.py to implement the change.
🟡 Minor comments (9)
src/miniforge/mesh/cli.py-260-266 (1)
260-266:⚠️ Potential issue | 🟡 Minor
--mode autois not handled byis_host = args.mode == "host".
args.modeacceptshost | worker | auto(defaultauto), butis_host = args.mode == "host"at line 59 meansautosilently collapses to worker-mode semantics. Either handleautoexplicitly (e.g., elect via mesh state after discovery) or restrictchoicestohost|workeruntilautois implemented. Combined withhost_node_idbeingNone, anautonode effectively becomes a worker with no host target.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/cli.py` around lines 260 - 266, The CLI accepts mode "auto" but the code determines host/worker via is_host = args.mode == "host", causing "auto" to behave as worker; update the handling of args.mode in the CLI/startup flow: either (A) implement explicit "auto" election logic after discovery (use mesh state/peer discovery to set is_host and populate host_node_id) and ensure functions that rely on is_host, host_node_id, and any Host-specific setup (e.g., model storage initialization) use the elected values, or (B) constrain the CLI choices to ["host","worker"] until auto is implemented by changing the choices for up_parser and removing/avoiding the "auto" default; refer to the symbols args.mode, is_host, and host_node_id to locate and update the decision logic so "auto" is not silently treated as worker.src/miniforge/mesh/static/app.js-65-90 (1)
65-90:⚠️ Potential issue | 🟡 Minor
window.NODE_IDis never set — self-row highlighting is dead code.Lines 67 and 69 compare
node.node_id === window.NODE_ID, but no code (this file,index.html, ordashboard.py) ever assignswindow.NODE_ID. It is permanentlyundefined, so every node is treated as "not self": thenode-row selfclass and the(You)suffix never get applied onceupdateNodesTable()replaces the initial server-rendered row.Fix by injecting the node id from the template (e.g., in
index.htmladd<script>window.NODE_ID = "{{ node_id }}";</script>beforeapp.jsand passnode_idin the dashboard render context).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/static/app.js` around lines 65 - 90, The client-side self-highlighting is broken because updateNodesTable() checks window.NODE_ID (used in comparisons like node.node_id === window.NODE_ID and class "node-row self" / name "(You)"), but window.NODE_ID is never set; fix it by injecting the current node id into the page before app.js loads (e.g., add a script in the HTML template that assigns window.NODE_ID = "{{ node_id }}"), and ensure the server-side dashboard render (dashboard.py) includes node_id in the template context so updateNodesTable() can correctly mark the self row and append "(You)".src/miniforge/cli.py-167-173 (1)
167-173:⚠️ Potential issue | 🟡 MinorSilently swallowing
ImportErrorwill mask real bugs in mesh submodules.The
meshsubpackage's__init__.pyeagerly imports 8 submodules (discovery,transport,coordinator,engine,security,dashboard,registry,host_worker). ANameError, typo, or unrelated import failure inside any of them surfaces here asImportErrorand is silently discarded — making it indistinguishable from "mesh extras not installed". For reference,mesh/cli.pycurrently has anOptionalundefined-name bug that would be completely hidden by this handler.At minimum, log the exception so operators have a signal when something non-obvious is wrong:
🔧 Suggested change
# Mesh command (distributed inference) try: from miniforge.mesh.cli import add_mesh_subparser add_mesh_subparser(subparsers) - except ImportError: - # Mesh dependencies not installed - pass + except ImportError as e: + # Mesh extras not installed, or a mesh submodule failed to import. + # Log at debug so real breakage isn't silent, but don't block the CLI. + import logging + logging.getLogger(__name__).debug("Mesh CLI not available: %s", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/cli.py` around lines 167 - 173, The current try/except around "from miniforge.mesh.cli import add_mesh_subparser" silently swallows ImportError and can hide real errors in mesh submodules; update the block to catch ImportError, log the full exception (use logging.exception or the existing processLogger) with a clear message mentioning add_mesh_subparser/mesh.cli, and only suppress the exception when it clearly indicates missing optional extras (e.g., check "No module named" in the exception message); otherwise re-raise so real bugs (like the undefined Optional in mesh/cli.py) are visible.src/miniforge/mesh/static/app.js-101-128 (1)
101-128:⚠️ Potential issue | 🟡 MinorConcurrent sends during streaming corrupt the assistant message.
If the user presses Enter a second time while a stream is still in flight,
currentStreamMessageis overwritten with a new placeholder. Any remainingchat_chunkevents from the first stream then append to the second message's DOM node, and the first message is orphaned mid-response.chat_completefor the first stream will also prematurely unsetcurrentStreamMessage, cutting off the second.At minimum, disable the send button / early-return while a stream is in progress:
🔧 Suggested change
function sendMessage() { const input = document.getElementById('chat-input'); const message = input.value.trim(); if (!message) return; + if (currentStreamMessage) return; // don't start a new stream mid-flight // Add user message to chat addUserMessage(message); input.value = '';A more robust fix would be to track a per-stream id on the server events and match chunks to their placeholder, but a simple guard is sufficient for the dashboard's intended use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/static/app.js` around lines 101 - 128, The sendMessage function allows a new send while a streaming response is active, causing currentStreamMessage to be overwritten and interleaving chunks; add a guard at the start of sendMessage that prevents a new send when a stream is in-progress (e.g., check if currentStreamMessage is non-null or maintain an isStreaming flag), disable the send button / Enter handling while streaming, and only create a new placeholder via addAssistantMessage and set currentStreamMessage when not already streaming; also ensure chat_complete clears the same flag/currentStreamMessage so subsequent sends are allowed.src/miniforge/mesh/static/index.html-34-71 (1)
34-71:⚠️ Potential issue | 🟡 MinorHardcoded resource values will show incorrect data on first paint.
Values like
1,28 GB,8 coresare baked into the initial HTML regardless of the actual node configuration (users can pass--ram/--cores). For a node with 64 GB / 16 cores, the dashboard will briefly lie on every load until the first/api/statusround-trip (and up to 5 s between refreshes).Either pass real values through Jinja (you already render
node_name/is_leaderserver-side) or initialize these fields to a neutral placeholder (—) so users don't see wrong data.🔧 Suggested change
- <div class="resource-item"> - <span class="resource-label">Nodes</span> - <span id="total-nodes" class="resource-value">1</span> - </div> - <div class="resource-item"> - <span class="resource-label">Total RAM</span> - <span id="total-ram" class="resource-value">28 GB</span> - </div> - <div class="resource-item"> - <span class="resource-label">Available RAM</span> - <span id="available-ram" class="resource-value">28 GB</span> - </div> - <div class="resource-item"> - <span class="resource-label">CPU Cores</span> - <span id="total-cpus" class="resource-value">8</span> - </div> + <div class="resource-item"> + <span class="resource-label">Nodes</span> + <span id="total-nodes" class="resource-value">—</span> + </div> + <div class="resource-item"> + <span class="resource-label">Total RAM</span> + <span id="total-ram" class="resource-value">—</span> + </div> + <div class="resource-item"> + <span class="resource-label">Available RAM</span> + <span id="available-ram" class="resource-value">—</span> + </div> + <div class="resource-item"> + <span class="resource-label">CPU Cores</span> + <span id="total-cpus" class="resource-value">—</span> + </div>Also note: the hardcoded self-row at lines 62-68 is replaced wholesale on the first
/api/nodesfetch byupdateNodesTable(), so it only shows during the initial flash and is also stale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/static/index.html` around lines 34 - 71, The initial HTML hardcodes resource values and a self node row that flash stale data; replace those static values with server-rendered placeholders (use the existing Jinja variables like node_name and is_leader) or neutral placeholders (e.g. "—") for the elements with IDs total-nodes, total-ram, available-ram, total-cpus and the self row (class node-row self) so the first paint doesn't show incorrect resources; ensure the template injects real values when available or neutral placeholders and keep updateNodesTable() unchanged (it will still replace the self-row on the first /api/nodes fetch).src/miniforge/mesh/security.py-96-110 (1)
96-110:⚠️ Potential issue | 🟡 MinorReplace deprecated
datetime.utcnow()with timezone-aware alternative.Lines 96, 133, and 134 use
datetime.utcnow(), which emitsDeprecationWarningin Python 3.12 and is slated for removal. Replace withdatetime.now(timezone.utc)and use thenot_valid_after_utcproperty on certificates. Thecryptographylibrary ≥ 42 provides these UTC-aware alternatives (yourpyproject.tomlpinscryptography>=42.0.0).🔧 Suggested change
-from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone ... - if cert.not_valid_after > datetime.utcnow() + timedelta(days=1): + if cert.not_valid_after_utc > datetime.now(timezone.utc) + timedelta(days=1): ... - .not_valid_before(datetime.utcnow() - timedelta(days=1)) - .not_valid_after(datetime.utcnow() + timedelta(days=self.CERT_VALIDITY_DAYS)) + .not_valid_before(datetime.now(timezone.utc) - timedelta(days=1)) + .not_valid_after(datetime.now(timezone.utc) + timedelta(days=self.CERT_VALIDITY_DAYS))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/security.py` around lines 96 - 110, Replace use of the deprecated naive timestamp check using datetime.utcnow(): in the certificate-loading block (where cert.not_valid_after is compared to datetime.utcnow() + timedelta(days=1)) switch to timezone-aware values by importing timezone and using datetime.now(timezone.utc) and, where available, use the certificate's UTC-aware property cert.not_valid_after_utc from the cryptography API; update the comparison to use cert.not_valid_after_utc (or convert cert.not_valid_after to UTC if that property isn't present) + timedelta(days=1), leaving the rest of the logic (calling self._generate_certificate(), writing cert.public_bytes(serialization.Encoding.PEM) to cert_path, and logging via logger) unchanged.src/miniforge/mesh/coordinator.py-186-238 (1)
186-238:⚠️ Potential issue | 🟡 Minor
strategy="local"andstrategy="remote"fall through silently.The docstring documents four strategies but the body only handles
auto,route, andsplit. Passing"local"or"remote"(asDistributedInferenceEngine.generatedoes withstrategy="route"viaschedule_inference, so this is specifically about callers passing the values from the docstring) leavesjob.status = "pending"andjob.assigned_node = None, and the method still returns the job — callers will wait forever in_wait_for_job_completion.Either remove the invalid values from the docstring, or add explicit branches (and raise for unknown strategies to mirror
DistributedInferenceEngine.generate'sValueError).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/coordinator.py` around lines 186 - 238, The schedule_inference method documents "local" and "remote" strategies but never handles them, causing jobs to remain pending; update schedule_inference to explicitly handle "local" (assign to current node or force local execution) and "remote" (route to a remote node via _select_least_loaded_node or similar) and set job.assigned_node, job.status="running", and job.started_at accordingly, and add an else that raises a ValueError for unknown strategies to match DistributedInferenceEngine.generate; reference schedule_inference, _select_least_loaded_node, _estimate_model_size, DistributedInferenceEngine.generate, and ensure _notify_state_change is still called before returning.src/miniforge/mesh/discovery.py-316-319 (1)
316-319:⚠️ Potential issue | 🟡 Minor
remove_serviceis a no-op so mDNS peers are never pruned.When a peer goes offline, the mDNS browser emits
remove_service, but we do nothing — the entry lingers in_peersuntil a separate caller invokesremove_stale_peers(). Implement a real removal that resolves the node to itsPeerInfo(either keep aname → keymap when handlingadd_serviceor look up by storednode_name) and calls_notify(peer, "removed")plusself._peers.pop(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/discovery.py` around lines 316 - 319, remove_service currently does nothing so peers never get pruned; implement it to resolve the service name to the corresponding PeerInfo and remove it from the in-memory registry. Either maintain a name→key map when handling add_service (update the map in add_service and use it here) or search self._peers for an entry whose PeerInfo.node_name matches the provided name, then call self._notify(peer, "removed") and pop that key from self._peers; ensure any auxiliary map is cleaned up and mirror the same removal logic used by remove_stale_peers.src/miniforge/mesh/registry.py-235-244 (1)
235-244:⚠️ Potential issue | 🟡 MinorTruncated SHA-256 defeats its purpose.
return h.hexdigest()[:16]keeps only 64 bits. If the goal is integrity (detecting silent corruption on download), this is fine for accidents but trivially forgeable and reduces collision resistance enormously. Since the PR frames the mesh as security-conscious (mTLS, cert pinning), return the full digest and compare via constant-time comparison on the worker afterdownload_modelcompletes. Storing/transmitting the full 64 hex chars is negligible cost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/registry.py` around lines 235 - 244, The _calculate_checksum function currently truncates the SHA256 hex to 16 chars (64 bits); change it to return the full SHA-256 hex digest (all 64 hex chars) instead of h.hexdigest()[:16], and ensure downstream verification uses a constant-time comparison when validating after download (e.g., in the worker code that runs after download_model completes) so integrity checks transmit/store the full digest and compare securely.
🧹 Nitpick comments (9)
src/miniforge/mesh/security.py (2)
176-183:sslshould be imported at module level (also referenced in the return annotation).Line 176 uses
"ssl.SSLContext"as a stringified return type annotation butsslis imported inside the function body at line 178. The string annotation works at runtime, but any tool doingget_type_hints()(FastAPI/pydantic/etc., or mypy in some modes) will fail to resolve it. Move the import to the top of the module — it's a standard-library module, there's no reason to defer it.🔧 Suggested change
import logging import os +import ssl from datetime import datetime, timedelta ... - def create_tls_context(self, server_mode: bool = False) -> "ssl.SSLContext": + def create_tls_context(self, server_mode: bool = False) -> ssl.SSLContext: """Create SSL context with mTLS.""" - import ssl - context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER if server_mode else ssl.PROTOCOL_TLS_CLIENT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/security.py` around lines 176 - 183, Move the local import of ssl out of create_tls_context and import it at module level so get_type_hints can resolve the return annotation; update the function signature to use a real annotation (ssl.SSLContext) instead of the stringified "ssl.SSLContext", remove the inner import statement in create_tls_context, and keep the function body using ssl.PROTOCOL_TLS_SERVER/CLIENT, ssl.TLSVersion, and ssl.CERT_REQUIRED as before (refer to the create_tls_context function and the ssl symbol).
135-144: Nit: avoid the__import__+ walrus dance; importipaddressnormally.The inline
ipaddress:=__import__("ipaddress").ip_address("127.0.0.1")at line 138 is hard to read and leaks anipaddressbinding into the enclosing method scope (which then shadows theipaddressmodule name for the rest of the function). Just import at the top of the file:🔧 Suggested change
import logging import os +import ipaddress from datetime import datetime, timedelta ... - x509.IPAddress(ipaddress:=__import__("ipaddress").ip_address("127.0.0.1")), + x509.IPAddress(ipaddress.ip_address("127.0.0.1")),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/security.py` around lines 135 - 144, Replace the inline "__import__" + walrus expression used to build the IPAddress in the x509.SubjectAlternativeName block with a normal module import: add "import ipaddress" at the top of the module and call ipaddress.ip_address("127.0.0.1") in the SubjectAlternativeName list (i.e., where x509.IPAddress(...) is constructed) so you avoid leaking a temporary binding and improve readability; keep the x509.SubjectAlternativeName(...), x509.DNSName("localhost"), and .sign(key, hashes.SHA256()) code unchanged.src/miniforge/mesh/__init__.py (1)
3-10: Eager re-exports couple everyminiforge.meshconsumer to all 8 submodules.Any consumer that imports anything from
miniforge.mesh(includingminiforge.mesh.cli, which you import in the top-level CLI) pays the cost of loading all heavy mesh deps (cryptography,msgpack,zeroconf,aiohttp,aiohttp-jinja2,python-socketio,aiodns) and surfaces any import-time failure in any submodule as anImportErrorat the package root.This also makes the
except ImportErrorinsrc/miniforge/cli.pymuch broader than intended — aNameError/SyntaxErrorin e.g.host_worker.pyis indistinguishable from "user didn't install themeshextra".Consider either keeping
__init__.pyfree of side-effect imports and letting callers import from submodules directly, or using__getattr__-based lazy loading (PEP 562). Given this is new code, the former is simplest:🔧 Suggested change (minimal)
-"""Miniforge Mesh - Distributed inference across multiple devices.""" - -from miniforge.mesh.discovery import MeshDiscovery, PeerInfo -from miniforge.mesh.transport import MeshTransport, MeshConnection -from miniforge.mesh.coordinator import MeshCoordinator, NodeState, InferenceJob -from miniforge.mesh.engine import DistributedInferenceEngine -from miniforge.mesh.security import MeshSecurity -from miniforge.mesh.dashboard import MeshDashboard -from miniforge.mesh.registry import ModelRegistry, ModelInfo -from miniforge.mesh.host_worker import HostWorkerEngine - -__all__ = [ - "MeshDiscovery", - "PeerInfo", +"""Miniforge Mesh - Distributed inference across multiple devices.""" + +__all__ = [ + "MeshDiscovery", + "PeerInfo", ... ] + +def __getattr__(name: str): + import importlib + mapping = { + "MeshDiscovery": "miniforge.mesh.discovery", + "PeerInfo": "miniforge.mesh.discovery", + "MeshTransport": "miniforge.mesh.transport", + "MeshConnection": "miniforge.mesh.transport", + "MeshCoordinator": "miniforge.mesh.coordinator", + "NodeState": "miniforge.mesh.coordinator", + "InferenceJob": "miniforge.mesh.coordinator", + "DistributedInferenceEngine": "miniforge.mesh.engine", + "MeshSecurity": "miniforge.mesh.security", + "MeshDashboard": "miniforge.mesh.dashboard", + "ModelRegistry": "miniforge.mesh.registry", + "ModelInfo": "miniforge.mesh.registry", + "HostWorkerEngine": "miniforge.mesh.host_worker", + } + if name in mapping: + return getattr(importlib.import_module(mapping[name]), name) + raise AttributeError(name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/__init__.py` around lines 3 - 10, The package __init__.py currently eagerly imports and re-exports MeshDiscovery, MeshTransport, MeshCoordinator, DistributedInferenceEngine, MeshSecurity, MeshDashboard, ModelRegistry, HostWorkerEngine, etc., causing heavy dependency loads and masking import errors; remove these top-level imports and stop re-exporting submodule symbols from miniforge.mesh so consumers import directly from their specific submodules (e.g., miniforge.mesh.discovery.MeshDiscovery, miniforge.mesh.transport.MeshTransport, miniforge.mesh.coordinator.MeshCoordinator/NodeState/InferenceJob, miniforge.mesh.engine.DistributedInferenceEngine, miniforge.mesh.security.MeshSecurity, miniforge.mesh.dashboard.MeshDashboard, miniforge.mesh.registry.ModelRegistry/ModelInfo, miniforge.mesh.host_worker.HostWorkerEngine), or alternatively implement a PEP 562 __getattr__ in __init__.py to lazily import those names on demand so importing miniforge.mesh does not load all heavy deps or convert top-level import sites (like the CLI) to import required symbols from the specific submodule instead.src/miniforge/mesh/cli.py (2)
154-175: Duplicated branch logic; the only difference isis_host/host_node_id.Both branches import
HostWorkerEngineand construct it with identical arguments exceptis_hostandhost_node_id(which isNonein both cases anyway — see the--host-ipcomment above). Collapse to a single construction:🔧 Suggested refactor
- # Initialize engine based on mode - if is_host: - # Host uses distributed engine with registry - from miniforge.mesh.host_worker import HostWorkerEngine - engine = HostWorkerEngine( - local_engine=local_engine, - coordinator=coordinator, - registry=registry, - node_id=node_id, - is_host=True, - ) - else: - # Worker uses host/worker engine - from miniforge.mesh.host_worker import HostWorkerEngine - engine = HostWorkerEngine( - local_engine=local_engine, - coordinator=coordinator, - registry=registry, - node_id=node_id, - is_host=False, - host_node_id=host_node_id, - ) + from miniforge.mesh.host_worker import HostWorkerEngine + engine = HostWorkerEngine( + local_engine=local_engine, + coordinator=coordinator, + registry=registry, + node_id=node_id, + is_host=is_host, + host_node_id=host_node_id, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/cli.py` around lines 154 - 175, The HostWorkerEngine construction is duplicated; instead import HostWorkerEngine once and collapse the two branches into a single construction that passes the variable is_host and host_node_id directly (e.g., engine = HostWorkerEngine(local_engine=local_engine, coordinator=coordinator, registry=registry, node_id=node_id, is_host=is_host, host_node_id=host_node_id)), removing the branch-specific duplicate code and keeping a single assignment to engine.
68-68: Nit: f-string without placeholders.Ruff F541. Either drop the
fprefix here (and similarly on any other constant f-strings) or embed a real placeholder.- print(f"🔥 Miniforge Mesh Starting...") + print("🔥 Miniforge Mesh Starting...")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/cli.py` at line 68, The print call using a constant f-string triggers Ruff F541; in the print statement that currently reads print(f"🔥 Miniforge Mesh Starting...") remove the unnecessary f prefix (change to print("🔥 Miniforge Mesh Starting...")) or replace it with a real placeholder if interpolation is intended; also scan for any other constant f-strings in this module (e.g., similar startup/log prints) and apply the same change.pyproject.toml (1)
47-56: Minor: redundant/inconsistent dependency declarations.
aiohttpis already a core dependency at line 27 (aiohttp>=3.9); re-listingaiohttp>=3.9.0here undermeshis redundant (and the.0suffix makes them look mismatched even though they're equivalent).- Separately, the
llama-cppextra at lines 39-41 pinsllama-cpp-python>=0.3.5while the core dep at line 35 requires>=0.3.20. The stricter core pin wins, but the extra should either match or be removed to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 47 - 56, The mesh extra currently re-declares aiohttp and the llama-cpp extra has an inconsistent version compared to the core deps; remove the redundant aiohttp entry from the mesh extras (or change it to exactly match the core spec "aiohttp>=3.9") and update the llama-cpp extra's dependency to match the core requirement ("llama-cpp-python>=0.3.20") or remove the duplicate from the extra so all declarations are consistent (look for the "mesh" extras block and the extra that references "llama-cpp-python" to apply the change).src/miniforge/mesh/registry.py (1)
172-206:download_modelregisters an incomplete file if the stream errors midway.If
request_layer_streamraises after some bytes have been written, the partialoutput_pathis left on disk andregister_local_modelis never called — but the next boot, if any caller ever inspectscache_dirby filename, the truncated file is sitting there. Safer pattern: download tooutput_path.with_suffix(".gguf.part"),os.replace(tmp, output_path)only on success, and remove the partial on exception. Also consider verifying the downloaded file against the host-providedModelInfo.checksumbefore registering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/registry.py` around lines 172 - 206, The download_model implementation can leave a truncated file and fail to register if the stream errors mid-download; change download_model to stream into a temporary path (e.g., output_path.with_suffix(".gguf.part")), write chunks to that temp file, and on full successful completion replace the temp with os.replace(temp, output_path) then call register_local_model(model_id, str(output_path)); on any exception ensure the temp file is removed (cleanup) so no partial files remain; additionally, if ModelInfo.checksum (or equivalent metadata) is available from the source node, compute and verify the checksum of the final file before replacing/registering and fail/clean up on mismatch.src/miniforge/mesh/dashboard.py (1)
267-278: Unbounded, silently-swallowed broadcast tasks.
asyncio.create_task(self._broadcast_status())drops the reference, so exceptions are only visible via the task's ownexcept Exception: logger.debug(...)and the task can be garbage-collected mid-flight (RUF006). More importantly, coordinator state changes can fire rapidly (peer add/update/remove, heartbeats), producing a burst of overlapping broadcasts. Consider coalescing (e.g. anasyncio.Eventset here and a single long-running broadcaster task draining it), and keep a strong reference to any task you do spawn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/dashboard.py` around lines 267 - 278, _on_state_change currently fires a detached asyncio.create_task(self._broadcast_status()) which can be garbage-collected, swallow exceptions, and produce overlapping bursts; change this to start and keep a strong reference to a single long-lived broadcaster task (e.g., self._broadcaster_task) and use an asyncio.Event (e.g., self._broadcast_event) that _on_state_change sets instead of creating tasks; implement a long-running method (e.g., _broadcaster_loop) that awaits the event, clears it, coalesces rapid sets (optionally with asyncio.wait_for/sleep debounce), calls the existing _broadcast_status() safely inside try/except and logs errors, and ensure the task is created once (on init/start) and cancelled on shutdown.src/miniforge/mesh/engine.py (1)
236-247: Busy‑wait + unbounded connection fan-out.
_wait_for_job_completionpollsjob.statusevery 100 ms; anasyncio.Eventset from the coordinator's_handle_inference_responsewould be both cheaper and race‑free. Similarly,_get_connection_to_nodeneeds to look up bynode_id; thefor conn in connections.values(): return connpattern guarantees it returns the wrong connection for any mesh with ≥2 peers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/engine.py` around lines 236 - 247, _wait_for_job_completion busy-waits on job.status; change it to await an asyncio.Event tied to the InferenceJob (e.g., add an event attribute on InferenceJob or a job_id->Event map) and have the coordinator set that event from _handle_inference_response when the job completes/ fails, then await the event in _wait_for_job_completion to avoid polling/races. Also fix _get_connection_to_node to look up the connection by node_id in coordinator.transport.connections (use the node_id key or a node_id->MeshConnection mapping) instead of returning the first entry; return None if no matching MeshConnection is found. Ensure references: InferenceJob (event), _wait_for_job_completion, _handle_inference_response, _get_connection_to_node, coordinator.transport.connections, MeshConnection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51cad341-8413-4bcb-ba71-260098357bbc
📒 Files selected for processing (15)
pyproject.tomlsrc/miniforge/cli.pysrc/miniforge/mesh/__init__.pysrc/miniforge/mesh/cli.pysrc/miniforge/mesh/coordinator.pysrc/miniforge/mesh/dashboard.pysrc/miniforge/mesh/discovery.pysrc/miniforge/mesh/engine.pysrc/miniforge/mesh/host_worker.pysrc/miniforge/mesh/registry.pysrc/miniforge/mesh/security.pysrc/miniforge/mesh/static/app.jssrc/miniforge/mesh/static/index.htmlsrc/miniforge/mesh/static/style.csssrc/miniforge/mesh/transport.py
| async def _read_loop(self) -> None: | ||
| """Read and dispatch messages.""" | ||
| while self._running: | ||
| try: | ||
| # Read length prefix | ||
| length_data = await self.reader.readexactly(4) | ||
| length = struct.unpack(">I", length_data)[0] | ||
|
|
||
| if length > 10 * 1024 * 1024: # Max 10MB | ||
| logger.warning(f"Message too large ({length} bytes), dropping connection") | ||
| break | ||
|
|
||
| # Read message data | ||
| data = await self.reader.readexactly(length) | ||
| msg_dict = msgpack.unpackb(data, raw=False) | ||
|
|
||
| msg = MeshMessage( | ||
| msg_type=msg_dict["type"], | ||
| payload=msg_dict["payload"], | ||
| timestamp=msg_dict["timestamp"], | ||
| node_id=msg_dict["node_id"], | ||
| ) | ||
|
|
||
| self.last_seen = time.time() | ||
| await self._dispatch(msg) | ||
|
|
||
| except asyncio.IncompleteReadError: | ||
| logger.info(f"Connection closed by {self.node_id}") | ||
| break | ||
| except Exception as e: | ||
| logger.warning(f"Error reading from {self.node_id}: {e}") | ||
| break | ||
|
|
||
| self._running = False | ||
|
|
There was a problem hiding this comment.
10 MiB per-message cap is below registry chunk size.
length > 10 * 1024 * 1024 drops the connection, but ModelRegistry.CHUNK_SIZE is 16 MiB (src/miniforge/mesh/registry.py:42) and every model_chunk message is sent as a single length‑prefixed payload. The first model chunk will trip this branch and kill the worker's connection mid‑download. Either raise the cap (with care — msgpack decode of large blobs is expensive and creates backpressure issues) or reduce CHUNK_SIZE to ~4‑8 MiB plus room for framing/payload keys.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 122-122: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/miniforge/mesh/transport.py` around lines 93 - 127, The per-message max
size check in _read_loop currently rejects payloads > 10*1024*1024 which
conflicts with ModelRegistry.CHUNK_SIZE (16 MiB) so model_chunk messages will be
dropped; update the logic to allow the registry chunk size (or a safe upper
bound) by either increasing the cap to exceed ModelRegistry.CHUNK_SIZE (e.g., >=
ModelRegistry.CHUNK_SIZE + framing overhead) or else reduce
ModelRegistry.CHUNK_SIZE to a value under the cap (4–8 MiB); locate the size
check in _read_loop and ensure the warning/log text still reflects the new
threshold and that any increased cap has appropriate safeguards against
excessive memory use and msgpack decoding backpressure.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/miniforge/mesh/dashboard.py (1)
1-1: Docstring is incorrect — this is aiohttp, not FastAPI.Module docstring says "FastAPI dashboard" but the implementation uses
aiohttp+python-socketio(lines 10-13). Update for accuracy.-"""FastAPI dashboard with OpenWebUI-style interface.""" +"""aiohttp dashboard with OpenWebUI-style interface."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/dashboard.py` at line 1, The module docstring incorrectly states "FastAPI dashboard"; update the top-level docstring to accurately describe the implementation as an aiohttp-based dashboard using python-socketio (reflecting the use of aiohttp and socketio in this module), e.g., mention "aiohttp + python-socketio OpenWebUI-style interface" so it matches the code that constructs the aiohttp app/socketio handlers.src/miniforge/mesh/static/fallback.html (1)
100-101: Nit: missing labels on peer IP/port inputs.Placeholder text is not an accessible name; screen readers will announce the inputs as unlabeled. Consider adding explicit
<label>elements (oraria-label) forpeer-ipandpeer-port. Same applies to thechat-inputtextarea (lines 77-81). Applies equally totemplates/index.html(lines 108-120, 84-88).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/static/fallback.html` around lines 100 - 101, The peer IP and port inputs (elements with ids "peer-ip" and "peer-port") and the chat textarea ("chat-input") lack accessible labels; placeholders are not sufficient for screen readers, so add explicit labels or aria-label attributes for these elements to provide an accessible name. Update the markup around the input elements (peer-ip, peer-port) and the textarea (chat-input) to include either a <label for="...">... from the visible text or add aria-label="Peer IP"/aria-label="Peer Port"/aria-label="Chat message" attributes to the existing elements; ensure the label text is meaningful and that the for attribute matches the element id, and apply the same change to the equivalent inputs in templates/index.html.src/miniforge/mesh/templates/index.html (1)
34-47: Hardcoded resource placeholders can mislead on first paint.
28 GB,8 cores,1 node,127.0.0.1are baked into the initial markup and will briefly be visible beforeapp.jshydrates, displaying values that almost certainly don't match the actual host. Prefer neutral placeholders (e.g.—/Loading…) matching whatfallback.htmlalready uses, so users aren't shown incorrect numbers if JS is slow/disabled.Proposed fix
- <span id="total-nodes" class="resource-value">1</span> + <span id="total-nodes" class="resource-value">—</span> @@ - <span id="total-ram" class="resource-value">28 GB</span> + <span id="total-ram" class="resource-value">—</span> @@ - <span id="available-ram" class="resource-value">28 GB</span> + <span id="available-ram" class="resource-value">—</span> @@ - <span id="total-cpus" class="resource-value">8</span> + <span id="total-cpus" class="resource-value">—</span> @@ - <tr class="node-row self"> - <td>{{ node_name }} (You)</td> - <td>127.0.0.1</td> - <td>28 GB</td> - <td>8 cores</td> - <td><span class="status-badge active">Active</span></td> - </tr> + <tr class="node-row self"> + <td colspan="5" style="text-align: center; padding: 2rem;"> + Loading mesh data… + </td> + </tr>Also applies to: 62-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/miniforge/mesh/templates/index.html` around lines 34 - 47, The template currently hardcodes resource values (e.g. "28 GB", "8", "1") which show before JS hydrates; update the markup for elements with ids total-nodes, total-ram, available-ram, total-cpus (and the similar host/IP placeholders mentioned around lines 62-68) to use neutral placeholders such as "—" or "Loading…" matching fallback.html instead of real numbers, keeping the same element ids and classes so app.js can hydrate them later; ensure no semantic/ARIA changes so hydration logic continues to target total-nodes, total-ram, available-ram, total-cpus (and the host/ip element ids) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/miniforge/mesh/dashboard.py`:
- Line 36: The dashboard currently defaults host: str = "0.0.0.0" and exposes
/api/* and Socket.IO with no auth; change the default host parameter to
"127.0.0.1" (require explicit CLI/flag to allow "0.0.0.0"), and implement an
aiohttp middleware (e.g., auth_middleware) that enforces a minimum auth scheme
(shared token or basic auth or a signed cookie derived from the node Ed25519 key
material) on all /api/* routes (specifically /api/chat and /api/connect) and
integrate the same check into the python-socketio event handlers so socket
events require the token/cookie; ensure the middleware is registered on the
aiohttp app and the socketio connection handler validates auth, and update the
dashboard docs to state the default localhost binding and opt‑in exposure model.
- Around line 318-321: The _on_state_change method currently fires
asyncio.create_task(self._broadcast_status()) and discards the Task (risking
GC/cancel) and may be called off the running loop; fix by adding an
instance-level set self._bg_tasks initialized in __init__ (e.g., self._bg_tasks:
set[asyncio.Task] = set()) and have _on_state_change create the task and store
it (add to set and remove on completion), and ensure task creation is
thread-safe by obtaining the running loop (capture it at start() or call
asyncio.get_running_loop()) and using loop.call_soon_threadsafe to schedule
creation of the Task that runs _broadcast_status, so
_notify_state_change/_on_state_change won’t raise RuntimeError when invoked
off-loop.
- Around line 176-197: The fallback HTML in the exception handler of the
dashboard render uses an unescaped f-string injecting
self.coordinator.node_name, self.coordinator.node_id, self.coordinator.is_leader
and the exception e, introducing XSS; fix by replacing the inline HTML with a
safe static response (preferably return web.FileResponse(...) to serve the
existing static/fallback.html) or, if keeping dynamic values, escape every
interpolated value with html.escape before inserting into the template (apply to
node_name, node_id, is_leader and str(e)) and ensure the exception text is
sanitized/trimmed; update the exception block in the Dashboard render handler in
src/miniforge/mesh/dashboard.py accordingly.
- Around line 266-304: The api_connect handler improperly trusts request JSON
and calls a private coordinator method with a placeholder PeerInfo; validate and
sanitize inputs (ensure "ip" is a valid IP address or CIDR-restricted allowed
host, ensure "port" is an integer in 1-65535) and return 400 on bad input; stop
calling the private method _connect_to_peer — add/use a public
MeshCoordinator.connect_to_peer(peer: PeerInfo) or connect_to_peer_by_addr(ip,
port) and call that from api_connect; and avoid creating a permanent placeholder
node with node_id="unknown" — either defer inserting nodes into
coordinator._nodes until the handshake returns a real node_id or use a temporary
key derived from (ip,port) that does not clobber real node_id entries so manual
connects do not overwrite or pollute aggregated resources.
---
Nitpick comments:
In `@src/miniforge/mesh/dashboard.py`:
- Line 1: The module docstring incorrectly states "FastAPI dashboard"; update
the top-level docstring to accurately describe the implementation as an
aiohttp-based dashboard using python-socketio (reflecting the use of aiohttp and
socketio in this module), e.g., mention "aiohttp + python-socketio
OpenWebUI-style interface" so it matches the code that constructs the aiohttp
app/socketio handlers.
In `@src/miniforge/mesh/static/fallback.html`:
- Around line 100-101: The peer IP and port inputs (elements with ids "peer-ip"
and "peer-port") and the chat textarea ("chat-input") lack accessible labels;
placeholders are not sufficient for screen readers, so add explicit labels or
aria-label attributes for these elements to provide an accessible name. Update
the markup around the input elements (peer-ip, peer-port) and the textarea
(chat-input) to include either a <label for="...">... from the visible text or
add aria-label="Peer IP"/aria-label="Peer Port"/aria-label="Chat message"
attributes to the existing elements; ensure the label text is meaningful and
that the for attribute matches the element id, and apply the same change to the
equivalent inputs in templates/index.html.
In `@src/miniforge/mesh/templates/index.html`:
- Around line 34-47: The template currently hardcodes resource values (e.g. "28
GB", "8", "1") which show before JS hydrates; update the markup for elements
with ids total-nodes, total-ram, available-ram, total-cpus (and the similar
host/IP placeholders mentioned around lines 62-68) to use neutral placeholders
such as "—" or "Loading…" matching fallback.html instead of real numbers,
keeping the same element ids and classes so app.js can hydrate them later;
ensure no semantic/ARIA changes so hydration logic continues to target
total-nodes, total-ram, available-ram, total-cpus (and the host/ip element ids)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c4cb3c5-7ab3-46cb-8ad4-35902f548f05
📒 Files selected for processing (4)
src/miniforge/mesh/dashboard.pysrc/miniforge/mesh/static/fallback.htmlsrc/miniforge/mesh/templates/index.htmlsrc/miniforge/models/registry.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
28 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/miniforge/mesh/static/app.js">
<violation number="1" location="src/miniforge/mesh/static/app.js:77">
P1: Unescaped peer data is rendered with `innerHTML` in the nodes table, creating an XSS risk.</violation>
</file>
<file name="src/miniforge/mesh/discovery.py">
<violation number="1" location="src/miniforge/mesh/discovery.py:142">
P1: `ServiceInfo(addresses=...)` expects packed network-byte-order addresses (4 bytes via `socket.inet_aton`), but `.encode()` on the IP string produces the UTF-8 representation (e.g. `b'192.168.1.5'`, 11 bytes). This will either crash or register a corrupted address with mDNS. Use `parsed_addresses` for string IPs, or `socket.inet_aton()` for packed bytes.</violation>
<violation number="2" location="src/miniforge/mesh/discovery.py:195">
P2: The coroutines created by `self._check_host(host_str, port)` and appended to `tasks` are never awaited — they are immediately orphaned. The code then creates *new* coroutines via `check_with_limit` in the `asyncio.gather` below. This produces `RuntimeWarning: coroutine '_check_host' was never awaited` for every scanned host. Replace the coroutine instantiation with a simple sentinel (e.g., append a tuple) or remove the first loop entirely and use a host list for the emptiness check.</violation>
<violation number="3" location="src/miniforge/mesh/discovery.py:337">
P1: `asyncio.create_task()` is called from `_handle_service`, but this method is invoked by `ServiceBrowser` from its internal background thread — not from the asyncio event loop thread. This will raise `RuntimeError: no running event loop`. Use `asyncio.run_coroutine_threadsafe(coro, loop)` with a stored event loop reference, or switch from `ServiceBrowser` to `AsyncServiceBrowser`.</violation>
</file>
<file name="src/miniforge/mesh/templates/index.html">
<violation number="1" location="src/miniforge/mesh/templates/index.html:8">
P2: Avoid a hard dependency on the external Socket.IO CDN here; if it is unavailable, the dashboard JavaScript fails before any real-time features can load.</violation>
</file>
<file name="src/miniforge/mesh/coordinator.py">
<violation number="1" location="src/miniforge/mesh/coordinator.py:354">
P1: If the stale-node cleanup in `_state_sync_loop` removes the current leader from `_nodes`, the election loop will never clear `_leader_id` because `self._nodes.get(self._leader_id)` returns `None` and the timeout check is skipped entirely. The mesh gets stuck with a phantom leader and no re-election.</violation>
<violation number="2" location="src/miniforge/mesh/coordinator.py:379">
P1: Leader election compares incompatible timestamps: `_started_at` for self vs. `last_heartbeat` for remote nodes. Since heartbeats update `last_heartbeat` continuously, a healthy remote node appears "younger" over time and a stale/dead node appears "oldest" — the election will prefer dead nodes over live ones. Remote nodes need to advertise their actual start time (e.g., via the heartbeat payload) for the comparison to be meaningful.</violation>
</file>
<file name="src/miniforge/mesh/registry.py">
<violation number="1" location="src/miniforge/mesh/registry.py:151">
P2: `layer_indices` is ignored, so the method silently streams the entire model even when specific layers are requested.</violation>
<violation number="2" location="src/miniforge/mesh/registry.py:159">
P1: Blocking file reads inside this async method can stall the event loop during large model streaming. Use non-blocking/offloaded I/O for chunk reads.</violation>
<violation number="3" location="src/miniforge/mesh/registry.py:198">
P1: Synchronous file writes in this async download loop block the event loop; offload writes to avoid degrading mesh responsiveness.</violation>
</file>
<file name="src/miniforge/mesh/host_worker.py">
<violation number="1" location="src/miniforge/mesh/host_worker.py:98">
P1: Missing `await` on the stream path. `local_engine.generate()` is an async method, so calling it without `await` returns a coroutine object instead of the `AsyncIterator[str]` the caller expects. The non-stream path on line 103 correctly uses `await`.</violation>
</file>
<file name="src/miniforge/mesh/static/fallback.html">
<violation number="1" location="src/miniforge/mesh/static/fallback.html:15">
P3: The fallback header never updates `#node-name`, so it will stay stuck on "Loading...".</violation>
</file>
<file name="src/miniforge/mesh/cli.py">
<violation number="1" location="src/miniforge/mesh/cli.py:60">
P2: `Optional` is referenced without being imported, which breaks strict type checking.</violation>
<violation number="2" location="src/miniforge/mesh/cli.py:119">
P1: `--host-ip`/`--ip` are accepted but never used to connect, so manual peer/host connection does not work despite CLI help text promising it.</violation>
</file>
<file name="src/miniforge/mesh/security.py">
<violation number="1" location="src/miniforge/mesh/security.py:142">
P0: Ed25519 keys do not accept a hash algorithm parameter. Passing `hashes.SHA256()` will raise `ValueError` at runtime, making certificate generation always fail. According to the `cryptography` library docs, the `algorithm` argument must be `None` for Ed25519 keys.</violation>
<violation number="2" location="src/miniforge/mesh/security.py:194">
P0: The pinned-certs CA bundle is never actually written (the loop body is `pass`), so the file is always empty. With `verify_mode = ssl.CERT_REQUIRED`, every mTLS handshake will fail because no CA is trusted. The `else` branch falls back to `CERT_NONE`, disabling verification entirely. Neither code path produces a working mTLS context.
The `_pinned_certs` set only stores fingerprint strings, not the full PEM certificates, so even fixing the loop wouldn't help — the full peer certificate PEM needs to be stored (e.g., in `pin_peer_cert`) to later write a valid CA bundle.</violation>
</file>
<file name="src/miniforge/mesh/dashboard.py">
<violation number="1" location="src/miniforge/mesh/dashboard.py:36">
P1: Binding the dashboard to `0.0.0.0` by default with no authentication middleware exposes `/api/chat` (arbitrary inference), `/api/connect` (outbound peer connections / SSRF), and the Socket.IO endpoint to the entire LAN. Default to `127.0.0.1` and require explicit opt-in (e.g. `--host 0.0.0.0`) to expose externally.</violation>
<violation number="2" location="src/miniforge/mesh/dashboard.py:46">
P1: Socket.IO is configured with `cors_allowed_origins="*"`, allowing any origin to connect to the dashboard event channel.</violation>
<violation number="3" location="src/miniforge/mesh/dashboard.py:191">
P2: The fallback HTML renders raw exception text to clients, leaking internal error details.</violation>
<violation number="4" location="src/miniforge/mesh/dashboard.py:191">
P1: XSS vulnerability in the template-error fallback: `{e}`, `{self.coordinator.node_name}`, `{self.coordinator.node_id}`, and `{self.coordinator.is_leader}` are interpolated directly into HTML via an f-string without escaping. The exception string `{e}` is especially dangerous since it is reached precisely when template rendering fails — which could be triggered by attacker-controlled input. Escape all values with `html.escape()` before interpolation, or serve the existing static `fallback.html` instead.</violation>
<violation number="5" location="src/miniforge/mesh/dashboard.py:270">
P1: SSRF: `ip` from the JSON body is used verbatim with no validation — it could be a hostname, an internal IP, or any arbitrary string. Combined with the unauthenticated endpoint, any LAN user can make the daemon open a TLS connection to arbitrary `ip:port` targets. Validate that `ip` is a proper IP address (e.g., via `ipaddress.ip_address(ip)`) and that `port` is an integer in `1..65535`.
Additionally, `PeerInfo` is created with `node_id="unknown"` and zero resources — every manual connect overwrites the same `"unknown"` slot in `coordinator._nodes`, so only the last manual connection is tracked.</violation>
<violation number="6" location="src/miniforge/mesh/dashboard.py:284">
P2: Manual peer connections all use `node_id="unknown"`, causing node-state collisions and overwrites for multiple manual peers.</violation>
<violation number="7" location="src/miniforge/mesh/dashboard.py:321">
P2: The `asyncio.create_task()` result is discarded, so the task only has a weak reference from the event loop and may be garbage-collected before the broadcast completes (RUF006). Additionally, `_on_state_change` is a synchronous callback that may be invoked when no event loop is running (e.g. during startup/teardown), causing `asyncio.create_task` to raise `RuntimeError`. Store a reference in a `set` on the instance and guard with `asyncio.get_running_loop()`.</violation>
</file>
<file name="src/miniforge/mesh/transport.py">
<violation number="1" location="src/miniforge/mesh/transport.py:212">
P0: The transport claims mTLS but neither the server (`asyncio.start_server`) nor the client (`asyncio.open_connection`) passes an SSL context. `MeshSecurity.create_tls_context()` exists but is never called. All mesh traffic—including inference payloads—is sent as plaintext TCP.
Pass `ssl=self.security.create_tls_context(server_mode=True)` to `start_server` and `ssl=self.security.create_tls_context(server_mode=False)` to `open_connection`.</violation>
<violation number="2" location="src/miniforge/mesh/transport.py:248">
P2: When an incoming connection ends (the `while conn._running` loop exits), the `finally` block removes the entry from `_connections` but never calls `self._notify(peer_id, conn, "disconnected")`. Peer-disconnect callbacks will not fire for non-stale disconnects, so the rest of the mesh won't be notified.</violation>
<violation number="3" location="src/miniforge/mesh/transport.py:297">
P1: `peer_key` is referenced in the `finally` block but is not defined on several code paths (timeout, invalid handshake, incomplete handshake). This will raise `NameError`, crash the handler, and leak the socket.
Initialize `peer_key` before the `try` block.</violation>
</file>
<file name="src/miniforge/mesh/engine.py">
<violation number="1" location="src/miniforge/mesh/engine.py:80">
P1: Missing `await` on `self.local_engine.generate(stream=True)`. Since `generate` is an `async def`, the caller receives a bare coroutine instead of the `AsyncIterator[str]`. Streaming via the local strategy will fail at runtime.</violation>
<violation number="2" location="src/miniforge/mesh/engine.py:241">
P1: `_get_connection_to_node` ignores `node_id` and returns the first connection it finds. In a multi-node mesh, inference requests will be sent to an arbitrary peer rather than the coordinator's chosen target, silently breaking routing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…buted-inference Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # pyproject.toml # src/miniforge/cli.py # src/miniforge/models/registry.py
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/miniforge/core/backends/llama_cpp.py (1)
333-377:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGlobal
jinja2.Environment.__init__monkeypatch is not safe under concurrent backend initialization.When
initialize()runs on multipleLlamaCppBackendinstances concurrently, each one patchesjinja2.Environment.__init__globally and captures_orig_env_init = jinja2.Environment.__init__(line 336). If Backend A patches first and Backend B patches before A'sfinallyblock runs, B captures A's wrapper as "original". When B restores infinally, it restores A's wrapper to the module. Result: the global environment runs with A's wrapper installed for the remainder of the process, not the original.The idiomatic approach is to pass
extensions=['jinja2.ext.loopcontrols']to theEnvironmentconstructor or callenv.add_extension()once at application startup, not patch globally per-backend initialization.Consider a module-level one-shot patch (idempotent) or refactor to let llama-cpp create its own
Environment:♻️ Proposed fix (idempotent, install once at module scope)
+_JINJA2_PATCHED = False + +def _ensure_jinja2_loopcontrols() -> None: + global _JINJA2_PATCHED + if _JINJA2_PATCHED: + return + try: + import jinja2 + _orig = jinja2.Environment.__init__ + def _env_init_with_loopcontrols(self, *args, **kwargs): + ext = list(kwargs.get("extensions") or []) + lc = "jinja2.ext.loopcontrols" + if lc not in ext: + ext.append(lc) + kwargs["extensions"] = ext + _orig(self, *args, **kwargs) + jinja2.Environment.__init__ = _env_init_with_loopcontrols + _JINJA2_PATCHED = True + except Exception as exc: + logger.debug("Could not patch jinja2 loopcontrols: %s", exc) @@ - try: - import jinja2 - - _orig_env_init = jinja2.Environment.__init__ - - def _env_init_with_loopcontrols(self, *args, **kwargs): - ext = kwargs.get("extensions") or [] - lc = "jinja2.ext.loopcontrols" - if lc not in ext: - ext = list(ext) + [lc] - kwargs["extensions"] = ext - _orig_env_init(self, *args, **kwargs) - - jinja2.Environment.__init__ = _env_init_with_loopcontrols - except Exception: - pass + _ensure_jinja2_loopcontrols() @@ - finally: - # Restore original Environment.__init__ to avoid side effects - with contextlib.suppress(Exception): - jinja2.Environment.__init__ = _orig_env_init # noqa: F821 + # No per-init restore needed: jinja2 patch is installed once and is idempotent.🤖 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 `@src/miniforge/core/backends/llama_cpp.py` around lines 333 - 377, The monkeypatch of jinja2.Environment.__init__ inside LlamaCppBackend.initialize() is unsafe under concurrent initializations (it captures wrappers and can leave a wrapped init installed); remove the per-initialize monkeypatch block that defines _orig_env_init and _env_init_with_loopcontrols and instead ensure jinja2's loopcontrols extension is added in a safe, idempotent way: either (A) when constructing any jinja2.Environment (pass extensions=['jinja2.ext.loopcontrols'] or call env.add_extension('jinja2.ext.loopcontrols') at the point where Environment() is created), or (B) perform a one-time, module-level install performed at import-time with a module-level flag (e.g. _jinja2_loopcontrols_installed) to avoid repeated global monkeypatching; update/remove the code around initialize(), the jinja2 import and monkeypatch (symbols: initialize(), LlamaCppBackend, jinja2.Environment.__init__, _env_init_with_loopcontrols) accordingly so no per-instance global reassignment occurs.
♻️ Duplicate comments (1)
src/miniforge/mesh/discovery.py (1)
81-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack scheduled callback tasks so they aren't GC'd.
asyncio.create_taskreturns a task that the event loop only weakly references. If the awaitable is long‑running and_notifyreturns before it completes, Python may garbage-collect the task and silently cancel the callback (e.g.,MeshCoordinator._on_peer_event). Static analysis (Ruff RUF006) is flagging this on line 87. The same pattern exists inMeshServiceListener._handle_serviceat line 340.🛡️ Proposed fix
self._scan_task: Optional[asyncio.Task] = None self._broadcast_task: Optional[asyncio.Task] = None + self._callback_tasks: Set[asyncio.Task] = set() @@ def _notify(self, peer: PeerInfo, event: str) -> None: """Notify all callbacks of peer event.""" for cb in self._callbacks: try: result = cb(peer, event) if inspect.isawaitable(result): - asyncio.create_task(result) + task = asyncio.create_task(result) + self._callback_tasks.add(task) + task.add_done_callback(self._callback_tasks.discard) except Exception as e: logger.warning(f"Discovery callback error: {e}")Apply the same pattern to
MeshServiceListener._handle_serviceat line 340.🤖 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 `@src/miniforge/mesh/discovery.py` around lines 81 - 89, The discovery _notify method is creating tasks with asyncio.create_task but not storing them, which lets long-running callback tasks be garbage-collected and cancelled; modify Discovery._notify to store created tasks (e.g., append to a self._tasks set or list) and ensure they are cleaned up when done (add a done callback to remove from the collection) so the task object is strongly referenced until completion; apply the same fix pattern to MeshServiceListener._handle_service and ensure you reference the task collection (e.g., self._tasks) consistently and remove tasks in their add_done_callback to avoid leaks.
🧹 Nitpick comments (8)
src/miniforge/core/backends/llama_cpp.py (2)
351-351: 💤 Low valueReplace deprecated
asyncio.get_event_loop()withasyncio.get_running_loop().
asyncio.get_event_loop()is deprecated since Python 3.10 when called outside a running loop and is slated for removal. All three call sites (initialize,generate,generate_stream) run insideasync def, soasyncio.get_running_loop()is the correct API.♻️ Proposed fix
- loop = asyncio.get_event_loop() + loop = asyncio.get_running_loop()(apply at lines 351, 423, 482)
Also applies to: 423-423, 482-482
🤖 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 `@src/miniforge/core/backends/llama_cpp.py` at line 351, Replace deprecated asyncio.get_event_loop() calls with asyncio.get_running_loop() in the async functions to avoid using the deprecated API: locate the three occurrences in the async functions initialize, generate, and generate_stream (search for asyncio.get_event_loop() within those functions) and change each to asyncio.get_running_loop() so the code uses the correct running-loop API for coroutines.
175-178: 💤 Low valueRe-raise with explicit exception chaining.
Re-raising an
ImportErrorinside anexcept ImportErrorshould preserve the cause viafrom. Ruff B904.♻️ Proposed fix
- except ImportError: + except ImportError as exc: raise ImportError( "llama-cpp-python not installed. Install with: uv pip install llama-cpp-python" - ) + ) from exc🤖 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 `@src/miniforge/core/backends/llama_cpp.py` around lines 175 - 178, The ImportError handler in src/miniforge/core/backends/llama_cpp.py re-raises a new ImportError without preserving the original exception; change the except block to "except ImportError as e:" and re-raise using "raise ImportError('llama-cpp-python not installed. Install with: uv pip install llama-cpp-python') from e" so the original cause is preserved (update the except in the llama_cpp import block where the current raise occurs).reap_infer.py (2)
246-316: ⚡ Quick winWorker subprocess invocation isn't portable.
["python", "reap_infer.py", ...](lines 248‑249) assumes the right interpreter is namedpythononPATHand that the script lives in the current working directory. In a virtualenv on Windows, in CI, or when a user runs the script viapython /full/path/reap_infer.py, this spawns a different (or missing) Python or fails to locate the script. Usesys.executableand__file__.♻️ Proposed fix
+import sys @@ - cmd: list[str] = [ - "python", - "reap_infer.py", + cmd: list[str] = [ + sys.executable, + str(Path(__file__).resolve()),🤖 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 `@reap_infer.py` around lines 246 - 316, The worker command in _build_worker_cmd currently hardcodes "python" and "reap_infer.py"; update it to use the running interpreter and script path by replacing those literals with sys.executable and the absolute path of __file__ (e.g. os.path.abspath(__file__)), ensuring you import sys and os at top if not present; keep the rest of the argument assembly unchanged and continue to convert values to strings as done for other flags and for worker_idx.
285-288: 💤 Low valueDead branch:
if not args.no_flash_attn: pass.Lines 285‑287 do nothing; only the
elsepath appends--no-flash-attn. Collapse into a single condition.♻️ Proposed cleanup
- if not args.no_flash_attn: - pass - else: + if args.no_flash_attn: cmd.append("--no-flash-attn")🤖 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 `@reap_infer.py` around lines 285 - 288, The conditional block around args.no_flash_attn is dead — remove the no-op branch and simplify: change the if/else that currently does "if not args.no_flash_attn: pass else: cmd.append('--no-flash-attn')" to a single conditional that appends "--no-flash-attn" when args.no_flash_attn is true (reference variables: args.no_flash_attn and cmd.append).src/miniforge/mesh/cli.py (2)
69-69: 💤 Low valueStray
fprefix on a string literal with no placeholders.Ruff F541 — purely cosmetic.
♻️ Proposed fix
- print(f"🔥 Miniforge Mesh Starting...") + print("🔥 Miniforge Mesh Starting...")🤖 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 `@src/miniforge/mesh/cli.py` at line 69, The print call uses an unnecessary f-string prefix: change the statement print(f"🔥 Miniforge Mesh Starting...") to a plain string literal by removing the leading "f" (i.e., print("🔥 Miniforge Mesh Starting...")), updating the occurrence of that exact print invocation in src/miniforge/mesh/cli.py.
21-21: 💤 Low valueImporting a private symbol (
_default_config_dir) fromminiforge.utils.config.The leading underscore signals "module-internal." Promote it to a public helper (e.g.,
default_config_dir) and re-export it fromminiforge.utils.config, or move the path resolution into a dedicated public utility. Cross-module dependence on_‑prefixed names will break silently the first timeconfig.pyis refactored.🤖 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 `@src/miniforge/mesh/cli.py` at line 21, The code imports the private symbol _default_config_dir from miniforge.utils.config; change this to use a public API by adding a public helper (e.g., default_config_dir) in miniforge.utils.config that either re-exports _default_config_dir or implements the path resolution, then update src/miniforge/mesh/cli.py to import default_config_dir instead of _default_config_dir; ensure the public name is exported from miniforge.utils.config so other modules (like CLI) no longer depend on an underscore-prefixed internal symbol.src/miniforge/models/minimax.py (1)
144-175: 💤 Low valueHoist
MODEL_ARCHITECTUREto module scope.This dict is rebuilt on every call to
_load_model. It's pure configuration data and benefits from being a module-level constant (and trivially testable / patchable in tests).♻️ Proposed refactor
+MODEL_ARCHITECTURE: Dict[str, int] = { + # MiniMax M2 series: 62 layers + "MiniMax-M2": 62, + "MiniMax-M2.1": 62, + # ... (move full mapping here) + "Kimi-K2.5": 64, +} + + class Miniforge: ... async def _load_model(...): ... - # Model architecture mapping: model_id -> n_layers - MODEL_ARCHITECTURE = { - "MiniMax-M2": 62, - ... - } - - def get_n_layers(model_id: str) -> int: + def get_n_layers(model_id: str) -> int: """Get number of layers for a model, with fallback to 62.""" model_id_lower = model_id.lower() for key, layers in MODEL_ARCHITECTURE.items(): if key.lower() in model_id_lower: return layers return 62🤖 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 `@src/miniforge/models/minimax.py` around lines 144 - 175, The MODEL_ARCHITECTURE dict is currently constructed inside _load_model on every call; move it to module scope as a constant (e.g., define MODEL_ARCHITECTURE at top-level of src/miniforge/models/minimax.py) and update _load_model to reference that module-level constant instead of rebuilding it; ensure the constant name remains MODEL_ARCHITECTURE so existing references in _load_model continue to work and tests can patch it easily.src/miniforge/utils/config.py (1)
225-233: 💤 Low valueSpecify UTF-8 encoding when writing config.
open(path, "w")uses the platform default encoding (cp1252 on many Windows installs), which can mangle non-ASCII model names or paths.yaml.dump(...)also alphabetizes keys by default — passsort_keys=Falseif you'd like to preserve the dataclass declaration order.♻️ Proposed fix
- with open(path, "w") as f: - yaml.dump(asdict(self), f, default_flow_style=False) + with open(path, "w", encoding="utf-8") as f: + yaml.dump(asdict(self), f, default_flow_style=False, sort_keys=False)🤖 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 `@src/miniforge/utils/config.py` around lines 225 - 233, The to_yaml method currently opens the file with open(path, "w") which uses the platform default encoding and can corrupt non-ASCII names; update the file write to explicitly use UTF-8 (e.g., open(path, "w", encoding="utf-8") or path.open(..., encoding="utf-8")), and call yaml.dump(asdict(self), f, default_flow_style=False, sort_keys=False) so keys keep the dataclass declaration order; keep the existing parent mkdir and logger.info behavior.
🤖 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 `@src/miniforge/mesh/cli.py`:
- Around line 60-61: The host_node_id variable is declared but never set so
HostWorkerEngine gets host_node_id=None, and the --host-ip/--ip flags are only
printed or build a PeerInfo and discarded; fix by wiring the flags into
transport.connect_to_peer(...) (use args.host_ip and args.ip), perform the
handshake to obtain the remote node_id, assign that node_id to host_node_id, and
pass host_node_id into the HostWorkerEngine constructor used in
_worker_generate; also remove the no-op PeerInfo creation or replace it with the
actual connect call and ensure any coordinator/transport method used
returns/sets the peer's node_id.
- Around line 144-153: The code attempts to read args.quantization but the
subparser doesn't define that flag, causing AttributeError when registering a
local model; update the call in the host registration branch so it uses a safe
fallback (e.g., pass quantization=getattr(args, "quantization", None) or omit
the quantization kwarg) instead of directly accessing args.quantization, or
alternatively add a --quantization argument to the subparser; locate the host
branch around is_host and args.model_path and adjust the
registry.register_local_model invocation (the register_local_model call and
args.quantization reference) to use the safe fallback or the new CLI arg.
In `@src/miniforge/mesh/discovery.py`:
- Around line 188-215: The code builds a `tasks` list of
`self._check_host(host_str, port)` coroutines but never awaits them (dead
coroutines) while separately reconstructing coroutines for `asyncio.gather`; fix
by removing the unused build or using `tasks` directly: create bounded
coroutines via the existing `check_with_limit(host, port)` and append those to
`tasks` (skipping `local_ip` and limiting to first 50 hosts), then call `await
asyncio.gather(*tasks, return_exceptions=True)`; ensure you no longer construct
raw `self._check_host(...)` coroutines into `tasks` (use `check_with_limit`) and
remove the duplicated inline list comprehension so only one awaited set of
coroutines exists.
In `@src/miniforge/mesh/engine.py`:
- Line 105: _active_jobs entries are never removed and _wait_for_job_completion
busy-waits; update the job lifecycle so entries placed into self._active_jobs
(where job.job_id is assigned in the blocks that call _execute_remote and
_execute_split) are removed in a finally block after those calls return, and
make _wait_for_job_completion event-driven by adding an asyncio.Event on the Job
object (e.g., Job.completion_event) that the coordinator sets when it marks the
job completed/failed; then replace the 100ms polling loop in
_wait_for_job_completion with an await job.completion_event.wait() (and still
check final status once awakened). Ensure the coordinator path that updates
job.status sets the event after updating status so awaiting tasks wake reliably.
In `@src/miniforge/models/minimax.py`:
- Around line 498-507: quant_ratios currently omits UD-* and 1–3-bit quant names
causing fallback to ratio=0.5 and grossly incorrect model_gb; fix by deriving
the ratio from the existing quant_bpw table (use bpw/16) instead of the
hardcoded quant_ratios map. Update the block that computes ratio and model_gb in
minimax.py (replace use of quant_ratios.get(quantization, 0.5) and subsequent
model_gb = params * 2 * ratio) to look up bpw from the same quant_bpw table used
elsewhere (or compute bpw via the same mapping logic used in
M7Config.__post_init__), compute ratio = bpw / 16.0, then compute model_gb =
params * 2 * ratio so UD-IQ*/UD-TQ* and 1–3 bit quantizations are handled
consistently for MemoryManager.register_model_memory and downstream budgeting.
In `@src/miniforge/utils/config.py`:
- Around line 216-223: The from_yaml method currently assumes yaml.safe_load
returns a mapping and fails when it returns None for empty/comment-only files;
update the function (around from_yaml, where data = yaml.safe_load(f) and
filtered_data is built) to coerce data into an empty dict when None (e.g., data
= yaml.safe_load(f) or {}), then continue using cls.__dataclass_fields__ to
build valid_fields and filtered_data before returning cls(**filtered_data).
---
Outside diff comments:
In `@src/miniforge/core/backends/llama_cpp.py`:
- Around line 333-377: The monkeypatch of jinja2.Environment.__init__ inside
LlamaCppBackend.initialize() is unsafe under concurrent initializations (it
captures wrappers and can leave a wrapped init installed); remove the
per-initialize monkeypatch block that defines _orig_env_init and
_env_init_with_loopcontrols and instead ensure jinja2's loopcontrols extension
is added in a safe, idempotent way: either (A) when constructing any
jinja2.Environment (pass extensions=['jinja2.ext.loopcontrols'] or call
env.add_extension('jinja2.ext.loopcontrols') at the point where Environment() is
created), or (B) perform a one-time, module-level install performed at
import-time with a module-level flag (e.g. _jinja2_loopcontrols_installed) to
avoid repeated global monkeypatching; update/remove the code around
initialize(), the jinja2 import and monkeypatch (symbols: initialize(),
LlamaCppBackend, jinja2.Environment.__init__, _env_init_with_loopcontrols)
accordingly so no per-instance global reassignment occurs.
---
Duplicate comments:
In `@src/miniforge/mesh/discovery.py`:
- Around line 81-89: The discovery _notify method is creating tasks with
asyncio.create_task but not storing them, which lets long-running callback tasks
be garbage-collected and cancelled; modify Discovery._notify to store created
tasks (e.g., append to a self._tasks set or list) and ensure they are cleaned up
when done (add a done callback to remove from the collection) so the task object
is strongly referenced until completion; apply the same fix pattern to
MeshServiceListener._handle_service and ensure you reference the task collection
(e.g., self._tasks) consistently and remove tasks in their add_done_callback to
avoid leaks.
---
Nitpick comments:
In `@reap_infer.py`:
- Around line 246-316: The worker command in _build_worker_cmd currently
hardcodes "python" and "reap_infer.py"; update it to use the running interpreter
and script path by replacing those literals with sys.executable and the absolute
path of __file__ (e.g. os.path.abspath(__file__)), ensuring you import sys and
os at top if not present; keep the rest of the argument assembly unchanged and
continue to convert values to strings as done for other flags and for
worker_idx.
- Around line 285-288: The conditional block around args.no_flash_attn is dead —
remove the no-op branch and simplify: change the if/else that currently does "if
not args.no_flash_attn: pass else: cmd.append('--no-flash-attn')" to a single
conditional that appends "--no-flash-attn" when args.no_flash_attn is true
(reference variables: args.no_flash_attn and cmd.append).
In `@src/miniforge/core/backends/llama_cpp.py`:
- Line 351: Replace deprecated asyncio.get_event_loop() calls with
asyncio.get_running_loop() in the async functions to avoid using the deprecated
API: locate the three occurrences in the async functions initialize, generate,
and generate_stream (search for asyncio.get_event_loop() within those functions)
and change each to asyncio.get_running_loop() so the code uses the correct
running-loop API for coroutines.
- Around line 175-178: The ImportError handler in
src/miniforge/core/backends/llama_cpp.py re-raises a new ImportError without
preserving the original exception; change the except block to "except
ImportError as e:" and re-raise using "raise ImportError('llama-cpp-python not
installed. Install with: uv pip install llama-cpp-python') from e" so the
original cause is preserved (update the except in the llama_cpp import block
where the current raise occurs).
In `@src/miniforge/mesh/cli.py`:
- Line 69: The print call uses an unnecessary f-string prefix: change the
statement print(f"🔥 Miniforge Mesh Starting...") to a plain string literal by
removing the leading "f" (i.e., print("🔥 Miniforge Mesh Starting...")),
updating the occurrence of that exact print invocation in
src/miniforge/mesh/cli.py.
- Line 21: The code imports the private symbol _default_config_dir from
miniforge.utils.config; change this to use a public API by adding a public
helper (e.g., default_config_dir) in miniforge.utils.config that either
re-exports _default_config_dir or implements the path resolution, then update
src/miniforge/mesh/cli.py to import default_config_dir instead of
_default_config_dir; ensure the public name is exported from
miniforge.utils.config so other modules (like CLI) no longer depend on an
underscore-prefixed internal symbol.
In `@src/miniforge/models/minimax.py`:
- Around line 144-175: The MODEL_ARCHITECTURE dict is currently constructed
inside _load_model on every call; move it to module scope as a constant (e.g.,
define MODEL_ARCHITECTURE at top-level of src/miniforge/models/minimax.py) and
update _load_model to reference that module-level constant instead of rebuilding
it; ensure the constant name remains MODEL_ARCHITECTURE so existing references
in _load_model continue to work and tests can patch it easily.
In `@src/miniforge/utils/config.py`:
- Around line 225-233: The to_yaml method currently opens the file with
open(path, "w") which uses the platform default encoding and can corrupt
non-ASCII names; update the file write to explicitly use UTF-8 (e.g., open(path,
"w", encoding="utf-8") or path.open(..., encoding="utf-8")), and call
yaml.dump(asdict(self), f, default_flow_style=False, sort_keys=False) so keys
keep the dataclass declaration order; keep the existing parent mkdir and
logger.info behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5427f3f6-78e8-46fc-a2ef-f93b9bfdbace
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
pyproject.tomlreap_infer.pysrc/miniforge/cli.pysrc/miniforge/core/backends/llama_cpp.pysrc/miniforge/mesh/cli.pysrc/miniforge/mesh/discovery.pysrc/miniforge/mesh/engine.pysrc/miniforge/mesh/host_worker.pysrc/miniforge/models/minimax.pysrc/miniforge/models/registry.pysrc/miniforge/utils/config.py
✅ Files skipped from review due to trivial changes (2)
- src/miniforge/models/registry.py
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/miniforge/cli.py
| is_host = args.mode == "host" | ||
| host_node_id: Optional[str] = None |
There was a problem hiding this comment.
--host-ip and --ip are advertised but have no effect; host_node_id is never set on workers.
Three coupled defects in the wiring:
- Line 61 declares
host_node_id: Optional[str] = Noneand never assigns it. The workerHostWorkerEngineis constructed withhost_node_id=None(line 175), so_worker_generateshort-circuits to"[Error: Model not available and no host configured]"whenever the model isn't already cached locally — the documented--host-ipflow is unreachable. - Lines 120‑121 just print a message for
--host-ip; no connect call is made intotransport/coordinator. - Lines 124‑135 build a
PeerInfofor--ipand immediately discard it.
If these flags are intentional for this PR, please wire them through to transport.connect_to_peer(...) (or equivalent) and resolve the host's node_id after handshake; otherwise drop the flags from the help text to avoid misleading users.
Also applies to: 119-135
🤖 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 `@src/miniforge/mesh/cli.py` around lines 60 - 61, The host_node_id variable is
declared but never set so HostWorkerEngine gets host_node_id=None, and the
--host-ip/--ip flags are only printed or build a PeerInfo and discarded; fix by
wiring the flags into transport.connect_to_peer(...) (use args.host_ip and
args.ip), perform the handshake to obtain the remote node_id, assign that
node_id to host_node_id, and pass host_node_id into the HostWorkerEngine
constructor used in _worker_generate; also remove the no-op PeerInfo creation or
replace it with the actual connect call and ensure any coordinator/transport
method used returns/sets the peer's node_id.
| if is_host and args.model_path: | ||
| try: | ||
| registry.register_local_model( | ||
| model_id=args.model or "minimax", | ||
| model_path=args.model_path, | ||
| quantization=args.quantization or "Q4_K_M", | ||
| ) | ||
| print(f"📦 Registered model: {args.model_path}") | ||
| except FileNotFoundError as e: | ||
| print(f"⚠️ Warning: {e}") |
There was a problem hiding this comment.
args.quantization is undefined — AttributeError whenever a host registers a local model.
The subparser at lines 253‑350 declares no --quantization argument, so args.quantization raises AttributeError the moment a user runs mesh up --mode host --model-path .... Either add the flag to the subparser or drop the reference and rely on the registry default.
🐛 Proposed fix
up_parser.add_argument(
"--model-path",
help="Path to model file (required for host mode to serve models)",
type=str,
)
+
+ up_parser.add_argument(
+ "--quantization",
+ "-q",
+ help="Quantization label for the registered model (default: Q4_K_M)",
+ type=str,
+ default="Q4_K_M",
+ )🤖 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 `@src/miniforge/mesh/cli.py` around lines 144 - 153, The code attempts to read
args.quantization but the subparser doesn't define that flag, causing
AttributeError when registering a local model; update the call in the host
registration branch so it uses a safe fallback (e.g., pass
quantization=getattr(args, "quantization", None) or omit the quantization kwarg)
instead of directly accessing args.quantization, or alternatively add a
--quantization argument to the subparser; locate the host branch around is_host
and args.model_path and adjust the registry.register_local_model invocation (the
register_local_model call and args.quantization reference) to use the safe
fallback or the new CLI arg.
| max_tokens=max_tokens, | ||
| strategy="route", | ||
| ) | ||
| self._active_jobs[job.job_id] = job |
There was a problem hiding this comment.
_active_jobs is never reaped, and _wait_for_job_completion busy-polls.
Two independent maintenance concerns:
- Jobs are inserted into
self._active_jobs(lines 105, 119) but no code path ever removes them. Long-running nodes accumulate completed/failed jobs and grow memory unboundedly. Pop the entry in afinallyafter_execute_remote/_execute_splitreturns. _wait_for_job_completionpollsjob.statusevery 100ms (line 239). For correctness on a busy mesh this should be event-driven — set anasyncio.Eventon the job when the coordinator marks itcompleted/failedandawait event.wait()here.
Also applies to: 119-119, 236-247
🤖 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 `@src/miniforge/mesh/engine.py` at line 105, _active_jobs entries are never
removed and _wait_for_job_completion busy-waits; update the job lifecycle so
entries placed into self._active_jobs (where job.job_id is assigned in the
blocks that call _execute_remote and _execute_split) are removed in a finally
block after those calls return, and make _wait_for_job_completion event-driven
by adding an asyncio.Event on the Job object (e.g., Job.completion_event) that
the coordinator sets when it marks the job completed/failed; then replace the
100ms polling loop in _wait_for_job_completion with an await
job.completion_event.wait() (and still check final status once awakened). Ensure
the coordinator path that updates job.status sets the event after updating
status so awaiting tasks wake reliably.
| quant_ratios = { | ||
| "Q8_0": 1.0, | ||
| "Q6_K": 0.75, | ||
| "Q5_K_M": 0.625, | ||
| "Q4_K_M": 0.5, | ||
| "Q3_K_M": 0.375, | ||
| "Q2_K": 0.25, | ||
| } | ||
| ratio = quant_ratios.get(quantization, 0.5) | ||
| model_gb = params * 2 * ratio |
There was a problem hiding this comment.
quant_ratios is missing the UD- / 1‑3 bit entries used elsewhere in this file.*
The per-quant bpw table at lines 203‑208 and the valid_quants list in M7Config.__post_init__ enumerate UD-IQ1 / UD-IQ2 / UD-TQ1 / Q3_K_M etc., but the quant_ratios map used to register memory only knows Q2_K through Q8_0. Anything else falls through to ratio = 0.5, producing a model_gb of params * 1.0 — wildly wrong for a 161B model loaded as UD-TQ1_0 (~16 GB on disk vs. the ~322 GB this would register). This propagates into MemoryManager.register_model_memory and any KV-budgeting decisions that follow.
♻️ Suggested fix
quant_ratios = {
"Q8_0": 1.0,
"Q6_K": 0.75,
"Q5_K_M": 0.625,
"Q4_K_M": 0.5,
"Q3_K_M": 0.375,
"Q2_K": 0.25,
+ # 1-3 bit / Unsloth dynamic quants (rough fp16 fraction)
+ "UD-TQ1_0": 0.083, "UD-IQ1_S": 0.092, "UD-IQ1_M": 0.10,
+ "UD-IQ2_XXS": 0.17, "UD-IQ2_M": 0.19, "UD-Q2_K_XL": 0.21,
+ "UD-IQ3_XXS": 0.25, "UD-IQ3_S": 0.27, "UD-Q3_K_M": 0.28, "UD-Q3_K_XL": 0.31,
+ "UD-IQ4_XS": 0.34, "UD-Q4_K_S": 0.37, "UD-Q4_K_M": 0.38, "UD-Q4_K_XL": 0.40,
+ "UD-Q5_K_S": 0.45, "UD-Q5_K_M": 0.46, "UD-Q5_K_XL": 0.48,
+ "UD-Q6_K": 0.55, "UD-Q6_K_XL": 0.58, "UD-Q8_K_XL": 0.75,
+ "F16": 1.0, "BF16": 1.0,
}Even better, derive ratios from the existing quant_bpw table (bpw / 16) so there's a single source of truth.
🤖 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 `@src/miniforge/models/minimax.py` around lines 498 - 507, quant_ratios
currently omits UD-* and 1–3-bit quant names causing fallback to ratio=0.5 and
grossly incorrect model_gb; fix by deriving the ratio from the existing
quant_bpw table (use bpw/16) instead of the hardcoded quant_ratios map. Update
the block that computes ratio and model_gb in minimax.py (replace use of
quant_ratios.get(quantization, 0.5) and subsequent model_gb = params * 2 *
ratio) to look up bpw from the same quant_bpw table used elsewhere (or compute
bpw via the same mapping logic used in M7Config.__post_init__), compute ratio =
bpw / 16.0, then compute model_gb = params * 2 * ratio so UD-IQ*/UD-TQ* and 1–3
bit quantizations are handled consistently for
MemoryManager.register_model_memory and downstream budgeting.
| with open(path) as f: | ||
| data = yaml.safe_load(f) | ||
|
|
||
| # Filter to only valid fields | ||
| valid_fields = {f.name for f in cls.__dataclass_fields__.values()} | ||
| filtered_data = {k: v for k, v in data.items() if k in valid_fields} | ||
|
|
||
| return cls(**filtered_data) |
There was a problem hiding this comment.
from_yaml crashes on an empty config file.
yaml.safe_load(f) returns None for an empty (or comment-only) YAML file. Line 221 then calls data.items() and raises AttributeError: 'NoneType' object has no attribute 'items'. Coerce to a dict before filtering.
🛡️ Proposed fix
- with open(path) as f:
- data = yaml.safe_load(f)
+ with open(path, encoding="utf-8") as f:
+ data = yaml.safe_load(f) or {}
+
+ if not isinstance(data, dict):
+ logger.warning("Config file %s did not contain a YAML mapping; using defaults", path)
+ return cls()
# Filter to only valid fields
valid_fields = {f.name for f in cls.__dataclass_fields__.values()}
filtered_data = {k: v for k, v in data.items() if k in valid_fields}🤖 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 `@src/miniforge/utils/config.py` around lines 216 - 223, The from_yaml method
currently assumes yaml.safe_load returns a mapping and fails when it returns
None for empty/comment-only files; update the function (around from_yaml, where
data = yaml.safe_load(f) and filtered_data is built) to coerce data into an
empty dict when None (e.g., data = yaml.safe_load(f) or {}), then continue using
cls.__dataclass_fields__ to build valid_fields and filtered_data before
returning cls(**filtered_data).
- MeshConnection now tracks peer_node_id for proper routing - MeshTransport maintains node_id->connection mapping - Coordinator captures connection in inference request handler closure - DistributedInferenceEngine registers as coordinator inference handler - Add run_mesh.py standalone distributed inference runner
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
src/miniforge/mesh/engine.py (2)
108-122:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
_active_jobsis still never reaped, and_wait_for_job_completionstill busy-polls.Same maintenance concerns as before: entries inserted at lines 108 and 122 are never removed (memory grows unbounded over a long-running mesh node), and
_wait_for_job_completionpolls every 100 ms instead of awaiting an event set byMeshCoordinator._handle_inference_response. Pop in atry/finallyaround_execute_remote/_execute_split, and store anasyncio.EventonInferenceJobthat the coordinator signals on completion/failure.Also applies to: 239-242
🤖 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 `@src/miniforge/mesh/engine.py` around lines 108 - 122, _active_jobs entries are never removed and _wait_for_job_completion busy-polls; fix by adding an asyncio.Event to the InferenceJob (e.g., job.completion_event) that MeshCoordinator._handle_inference_response sets when a job finishes/errs, replace polling in _wait_for_job_completion to await that event, and wrap calls that register jobs (in methods that call _execute_remote and _execute_split / schedule_inference) in try/finally so self._active_jobs.pop(job.job_id, None) runs in finally to always remove the job entry; ensure coordinator code signals job.completion_event on both success and failure so awaiting tasks wake correctly.
165-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLocal fallback paths still drop sampling parameters.
The
target_node == self.node_idbranch (lines 171–174), the no-connection fallback (lines 180–183), and the_execute_splitsingle-node fallback (lines 218–221) all callself.local_engine.generate(prompt=..., max_tokens=...), silently overriding anytemperature/top_p/top_k/streamthat the user passed togenerate(). Same root cause as the wire-format gap above; both should be fixed by threading sampling params throughInferenceJob.Also applies to: 212-221
🤖 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 `@src/miniforge/mesh/engine.py` around lines 165 - 183, The local fallback paths in _execute_remote and the single-node fallback in _execute_split call local_engine.generate only with prompt and max_tokens, dropping sampling params (temperature, top_p, top_k, stream); fix by extending InferenceJob to carry sampling fields and update callers to pass those fields through to local_engine.generate (e.g., use job.temperature, job.top_p, job.top_k, job.stream) so both the target_node == self.node_id branch, the no-connection fallback (where conn is None), and the _execute_split single-node fallback all forward the full set of sampling/streaming parameters to local_engine.generate instead of hardcoding only prompt and max_tokens.src/miniforge/mesh/transport.py (3)
299-359:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
peer_keyUnboundLocalError + writer leak in exception paths.
peer_keyis only assigned at line 330. Any failure in lines 308–329 (handshakeTimeoutError,decode()failure, or the two early-return branches falling through tofinally) raisesUnboundLocalErrorat line 356, masking the real error. Additionally, theexcept asyncio.TimeoutError(line 351) andexcept Exception(line 353) paths never close the writer, leaking sockets on every failed handshake.🐛 Proposed fix
async def _handle_incoming( self, reader: asyncio.StreamReader, writer: asyncio.StreamWriter ) -> None: """Handle incoming connection.""" peer_addr = writer.get_extra_info("peername") + peer_key: Optional[str] = None logger.debug(f"Incoming connection from {peer_addr}") try: ... except asyncio.TimeoutError: logger.warning(f"Handshake timeout from {peer_addr}") except Exception as e: logger.warning(f"Error handling incoming from {peer_addr}: {e}") finally: - if peer_key in self._connections: + if peer_key and peer_key in self._connections: conn = self._connections.pop(peer_key) if conn.peer_node_id in self._node_id_to_connection: del self._node_id_to_connection[conn.peer_node_id] + if not writer.is_closing(): + writer.close() + try: + await writer.wait_closed() + except Exception: + pass🤖 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 `@src/miniforge/mesh/transport.py` around lines 299 - 359, The _handle_incoming coroutine may raise UnboundLocalError because peer_key is assigned only after parsing the handshake and the finally block always references it, and the writer is never closed on exception/early-return paths; fix by initializing peer_key and conn to None at the start of _handle_incoming, ensure every early-return path closes writer (await writer.drain optional) and waits for writer.wait_closed(), and in the finally block check if peer_key is not None before touching self._connections and only attempt to close the writer/connection if conn is not None (call conn.close()/await conn.wait_closed() or writer.close()/await writer.wait_closed() as appropriate); update references to MeshConnection, peer_key, conn, and writer in _handle_incoming accordingly.
251-297:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftOutbound and inbound flows still use mismatched
peer_keyschemes.Outbound keys by
f"{ip}:{mesh_port}"(line 253), inbound keys byf"{peer_addr[0]}:{peer_addr[1]}"where the second component is the ephemeral client port (line 330). The new_node_id_to_connectionmap only stores the most recent connection per peer; if A dials B and B simultaneously dials A, each side ends up with twoMeshConnectionentries in_connectionswriting over two different sockets, and the older one will only be reaped via_cleanup_staleafter 30s of silence (heartbeats keep both alive). Makepeer_node_id(learned from handshake) the canonical key for_connectionsand drop the duplicate map.Also applies to: 329-345
🤖 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 `@src/miniforge/mesh/transport.py` around lines 251 - 297, connect_to_peer and the inbound acceptor use different peer_key formats (ip:port vs ephemeral client port), causing duplicate MeshConnection entries in _connections and a fragile _node_id_to_connection map; make peer_node_id the canonical map key and remove the duplicate map. Change logic in connect_to_peer (and the inbound handshake handler that constructs peer_key) to use the peer_id parsed from the handshake (peer_node_id) as the key when storing/removing connections in _connections, update calls to _notify and logging to reference peer_node_id instead of peer_key, and remove or stop updating _node_id_to_connection so only the peer_node_id→MeshConnection mapping is maintained and relied upon by _cleanup_stale and other consumers.
103-105:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win10 MiB per-message cap is still below
ModelRegistry.CHUNK_SIZE(16 MiB).This was previously flagged but remains unchanged. Any
model_chunkmessage will trip this branch and tear down the worker connection mid-download. Either raise the cap (with msgpack-decode/backpressure considerations) or shrinkCHUNK_SIZEto ~4–8 MiB plus framing overhead.🤖 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 `@src/miniforge/mesh/transport.py` around lines 103 - 105, The per-message size check in transport.py that drops connections for messages >10MB conflicts with ModelRegistry.CHUNK_SIZE (16MiB) and will abort model_chunk transfers; update the logic in the length check (the branch that logs "Message too large (...)") to allow at least ModelRegistry.CHUNK_SIZE plus framing overhead (or alternatively reduce ModelRegistry.CHUNK_SIZE to ~4–8MiB plus overhead), and ensure you account for msgpack-decoding/backpressure implications when increasing the cap; locate the check in the receiving path that references length and either raise the constant to match ModelRegistry.CHUNK_SIZE+overhead or adjust ModelRegistry.CHUNK_SIZE so both sides agree.
🧹 Nitpick comments (5)
run_mesh.py (3)
101-101: 💤 Low value
f-string without placeholders (Ruff F541).Drop the
fprefix.♻️ Proposed fix
- print(f"🔥 Miniforge Mesh Node Starting...") + print("🔥 Miniforge Mesh Node Starting...")🤖 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 `@run_mesh.py` at line 101, The print statement using an f-string has no placeholders—remove the unnecessary f prefix on the print call (the print invocation that currently reads print(f"🔥 Miniforge Mesh Node Starting...") should be changed to a normal string literal) so it no longer triggers Ruff F541; update the print invocation accordingly.
285-294: 💤 Low value
KeyboardInterruptmay not be caught here.Under
asyncio.run, the loop typically translates Ctrl-C into aCancelledErrorat the awaited point, not aKeyboardInterruptinside the coroutine; the bareKeyboardInterruptonly fires if it's raised synchronously between awaits, which is rare. Thefinallyblock still runs, so cleanup is fine, but the friendly "🛑 Shutting down..." message may not print on Ctrl-C. If you want the banner reliably, handleKeyboardInterruptinmain()aroundasyncio.run(...)instead.🤖 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 `@run_mesh.py` around lines 285 - 294, The Ctrl-C banner may not print because KeyboardInterrupt rarely propagates inside the coroutine; move the KeyboardInterrupt handling out of the coroutine and into the caller that invokes asyncio.run so the message always prints: remove KeyboardInterrupt from the inner except around the loop (leave handling for asyncio.CancelledError if desired) and wrap the asyncio.run(...) call in a try/except KeyboardInterrupt block in main (where you can print "🛑 Shutting down...") while ensuring coordinator.stop() and engine.cleanup() still run (they can remain in the coroutine's finally or be awaited from main after cancellation); refer to the coroutine containing the try/except and the functions coordinator.stop and engine.cleanup and the site that calls asyncio.run to implement this change.
152-170: ⚡ Quick winDead
PeerInfoconstruction.Lines 154–163 build a
PeerInfothat is never referenced — line 166 callstransport.connect_to_peer(peer_ip, mesh_port)directly with the raw args. Either remove the unused object, or feed it through discovery so the coordinator learns about the peer (currently, when--peeris used,coordinator._nodeswill only learn the peer once_connect_to_peerruns from the discovery loop, not from this manual handshake).♻️ Minimal cleanup
if peer_ip: print(f"🔗 Connecting to peer {peer_ip}:{mesh_port}...") - from miniforge.mesh.discovery import PeerInfo - peer = PeerInfo( - ip=peer_ip, - port=mesh_port, - node_id="manual", - node_name="manual", - ram_gb=0, - cpu_cores=0, - last_seen=0, - ) # Give discovery a moment, then force connection attempt await asyncio.sleep(1) conn = await transport.connect_to_peer(peer_ip, mesh_port)🤖 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 `@run_mesh.py` around lines 152 - 170, The PeerInfo instance built in the block (PeerInfo(...)) is unused; either remove that construction entirely, or make it actually register the peer with the discovery/coordinator so the coordinator learns about the peer (e.g., add the PeerInfo into coordinator._nodes or call the discovery method that accepts a PeerInfo such as discovery._connect_to_peer / coordinator.register_node with the created peer) before or instead of calling transport.connect_to_peer(peer_ip, mesh_port); ensure you reference the same PeerInfo object (the variable peer) when registering so coordinator._nodes and discovery reflect the manual handshake.src/miniforge/mesh/coordinator.py (2)
284-284: ⚡ Quick winDetached
asyncio.create_taskcan be GC'd.Per Ruff RUF006: the task object is not held anywhere, so the event loop only keeps a weak reference and the connection coroutine may be cancelled mid-flight under memory pressure. Track the task (e.g., a
setof pending connect tasks withdiscardon completion) orawaitit.🤖 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 `@src/miniforge/mesh/coordinator.py` at line 284, The detached asyncio.create_task call starting _connect_to_peer can be garbage-collected and cancelled; fix by tracking the Task on the coordinator (e.g., add a self._pending_connect_tasks: set attribute) and replace asyncio.create_task(self._connect_to_peer(peer)) with creating the task, adding it to self._pending_connect_tasks, and attaching a done callback that discards the task from the set and logs/handles exceptions from task.exception(); ensure the set is initialized (e.g., in __init__) and used wherever connect tasks are spawned.
327-332: 💤 Low valueUnused locals in
_handle_inference_request.
prompt,model_id, andmax_tokensare extracted but never used (they're only consumed via the originalpayloaddict passed toself._inference_handler). Drop them or pulljob_iddirectly:job_id = payload.get("job_id").🤖 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 `@src/miniforge/mesh/coordinator.py` around lines 327 - 332, In _handle_inference_request, remove the unused local assignments for prompt, model_id, and max_tokens (they are unused because the original payload is passed to self._inference_handler); instead read only job_id from payload (job_id = payload.get("job_id")) or, if you intended to use those values, pass the extracted prompt/model_id/max_tokens into self._inference_handler call; update the function to either drop the three variables or consume them so no unused locals remain.
🤖 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 `@src/miniforge/mesh/coordinator.py`:
- Around line 263-277: The _select_least_loaded_node function is biased because
all_nodes includes a synthetic self NodeState with default cpu_percent=0.0 and
ram_available=0.0, so self will win ties; fix by either populating the synthetic
self entry with real local metrics (e.g., use psutil to set cpu_percent and
ram_available on the NodeState representing self before sorting) or by excluding
self from the candidate list when at least one peer is active (filter out
entries where node_id == self.node_id from candidates if any peers exist).
Update the code paths that construct or use all_nodes/NodeState so the self
entry has measured metrics or is removed when peers are reachable, and keep the
existing fallback to return self.node_id only when no active peers remain.
- Around line 366-373: Handle missing node_id in _handle_heartbeat by falling
back to connection-aware sender fields: if payload.get("node_id") is None, try
payload.get("sender") and payload.get("sender_id") (these are set by
MeshTransport._heartbeat_loop when dispatch is connection-aware) and use that
value for node lookup in self._nodes before updating node.last_heartbeat,
node.ram_available and node.cpu_percent so _state_sync_loop doesn't erroneously
reap peers; keep behavior of skipping if no valid id is found.
- Around line 398-422: The election uses incompatible timestamps
(self._started_at vs NodeState.last_heartbeat) causing every node to elect
itself; fix by propagating and comparing each peer's actual startup time
instead: include a started_at field in the heartbeat/handshake payload in
transport.py, store that value on first registration into NodeState.started_at
(add started_at attribute to NodeState if missing) and change _run_election to
build candidates from (node.node_id, node.started_at) for peers (keeping the
lexicographic node_id tiebreaker) while still using self._started_at for the
local entry; ensure last_heartbeat remains for liveness only.
In `@src/miniforge/mesh/engine.py`:
- Around line 185-190: The payload and local calls drop sampling params: update
the InferenceJob data structure and schedule_inference signature to include
temperature, top_p, top_k, and stream, ensure all call sites that construct
InferenceJob (including the local fallback paths around the schedule_inference
calls) populate those fields, add those fields to the dict sent in conn.send for
"inference_request", and in _on_inference_request extract them and pass them
through to local_engine.generate(..., temperature=..., top_p=..., top_k=...,
stream=...) so remote and local paths honor the requested decoding parameters.
In `@src/miniforge/mesh/transport.py`:
- Around line 367-371: The heartbeat payload sent from MeshConnection/transport
currently omits node_id causing MeshCoordinator._handle_heartbeat to drop
messages (because _dispatch strips MeshMessage.node_id and passes only
msg.payload); fix by including the sender's node id in the heartbeat payload
when calling conn.send("heartbeat", {...}) — e.g., add "node_id": self.node_id
to that payload in transport.send heartbeat code; as a more durable alternative,
consider changing _dispatch to pass the originating MeshConnection or
peer_node_id into handlers (similar to coordinator._on_connection_event) so
handlers don't rely on node_id in payload.
---
Duplicate comments:
In `@src/miniforge/mesh/engine.py`:
- Around line 108-122: _active_jobs entries are never removed and
_wait_for_job_completion busy-polls; fix by adding an asyncio.Event to the
InferenceJob (e.g., job.completion_event) that
MeshCoordinator._handle_inference_response sets when a job finishes/errs,
replace polling in _wait_for_job_completion to await that event, and wrap calls
that register jobs (in methods that call _execute_remote and _execute_split /
schedule_inference) in try/finally so self._active_jobs.pop(job.job_id, None)
runs in finally to always remove the job entry; ensure coordinator code signals
job.completion_event on both success and failure so awaiting tasks wake
correctly.
- Around line 165-183: The local fallback paths in _execute_remote and the
single-node fallback in _execute_split call local_engine.generate only with
prompt and max_tokens, dropping sampling params (temperature, top_p, top_k,
stream); fix by extending InferenceJob to carry sampling fields and update
callers to pass those fields through to local_engine.generate (e.g., use
job.temperature, job.top_p, job.top_k, job.stream) so both the target_node ==
self.node_id branch, the no-connection fallback (where conn is None), and the
_execute_split single-node fallback all forward the full set of
sampling/streaming parameters to local_engine.generate instead of hardcoding
only prompt and max_tokens.
In `@src/miniforge/mesh/transport.py`:
- Around line 299-359: The _handle_incoming coroutine may raise
UnboundLocalError because peer_key is assigned only after parsing the handshake
and the finally block always references it, and the writer is never closed on
exception/early-return paths; fix by initializing peer_key and conn to None at
the start of _handle_incoming, ensure every early-return path closes writer
(await writer.drain optional) and waits for writer.wait_closed(), and in the
finally block check if peer_key is not None before touching self._connections
and only attempt to close the writer/connection if conn is not None (call
conn.close()/await conn.wait_closed() or writer.close()/await
writer.wait_closed() as appropriate); update references to MeshConnection,
peer_key, conn, and writer in _handle_incoming accordingly.
- Around line 251-297: connect_to_peer and the inbound acceptor use different
peer_key formats (ip:port vs ephemeral client port), causing duplicate
MeshConnection entries in _connections and a fragile _node_id_to_connection map;
make peer_node_id the canonical map key and remove the duplicate map. Change
logic in connect_to_peer (and the inbound handshake handler that constructs
peer_key) to use the peer_id parsed from the handshake (peer_node_id) as the key
when storing/removing connections in _connections, update calls to _notify and
logging to reference peer_node_id instead of peer_key, and remove or stop
updating _node_id_to_connection so only the peer_node_id→MeshConnection mapping
is maintained and relied upon by _cleanup_stale and other consumers.
- Around line 103-105: The per-message size check in transport.py that drops
connections for messages >10MB conflicts with ModelRegistry.CHUNK_SIZE (16MiB)
and will abort model_chunk transfers; update the logic in the length check (the
branch that logs "Message too large (...)") to allow at least
ModelRegistry.CHUNK_SIZE plus framing overhead (or alternatively reduce
ModelRegistry.CHUNK_SIZE to ~4–8MiB plus overhead), and ensure you account for
msgpack-decoding/backpressure implications when increasing the cap; locate the
check in the receiving path that references length and either raise the constant
to match ModelRegistry.CHUNK_SIZE+overhead or adjust ModelRegistry.CHUNK_SIZE so
both sides agree.
---
Nitpick comments:
In `@run_mesh.py`:
- Line 101: The print statement using an f-string has no placeholders—remove the
unnecessary f prefix on the print call (the print invocation that currently
reads print(f"🔥 Miniforge Mesh Node Starting...") should be changed to a normal
string literal) so it no longer triggers Ruff F541; update the print invocation
accordingly.
- Around line 285-294: The Ctrl-C banner may not print because KeyboardInterrupt
rarely propagates inside the coroutine; move the KeyboardInterrupt handling out
of the coroutine and into the caller that invokes asyncio.run so the message
always prints: remove KeyboardInterrupt from the inner except around the loop
(leave handling for asyncio.CancelledError if desired) and wrap the
asyncio.run(...) call in a try/except KeyboardInterrupt block in main (where you
can print "🛑 Shutting down...") while ensuring coordinator.stop() and
engine.cleanup() still run (they can remain in the coroutine's finally or be
awaited from main after cancellation); refer to the coroutine containing the
try/except and the functions coordinator.stop and engine.cleanup and the site
that calls asyncio.run to implement this change.
- Around line 152-170: The PeerInfo instance built in the block (PeerInfo(...))
is unused; either remove that construction entirely, or make it actually
register the peer with the discovery/coordinator so the coordinator learns about
the peer (e.g., add the PeerInfo into coordinator._nodes or call the discovery
method that accepts a PeerInfo such as discovery._connect_to_peer /
coordinator.register_node with the created peer) before or instead of calling
transport.connect_to_peer(peer_ip, mesh_port); ensure you reference the same
PeerInfo object (the variable peer) when registering so coordinator._nodes and
discovery reflect the manual handshake.
In `@src/miniforge/mesh/coordinator.py`:
- Line 284: The detached asyncio.create_task call starting _connect_to_peer can
be garbage-collected and cancelled; fix by tracking the Task on the coordinator
(e.g., add a self._pending_connect_tasks: set attribute) and replace
asyncio.create_task(self._connect_to_peer(peer)) with creating the task, adding
it to self._pending_connect_tasks, and attaching a done callback that discards
the task from the set and logs/handles exceptions from task.exception(); ensure
the set is initialized (e.g., in __init__) and used wherever connect tasks are
spawned.
- Around line 327-332: In _handle_inference_request, remove the unused local
assignments for prompt, model_id, and max_tokens (they are unused because the
original payload is passed to self._inference_handler); instead read only job_id
from payload (job_id = payload.get("job_id")) or, if you intended to use those
values, pass the extracted prompt/model_id/max_tokens into
self._inference_handler call; update the function to either drop the three
variables or consume them so no unused locals remain.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f3b9124-2ee6-402b-a79b-08e3fa44b15b
📒 Files selected for processing (4)
run_mesh.pysrc/miniforge/mesh/coordinator.pysrc/miniforge/mesh/engine.pysrc/miniforge/mesh/transport.py
| async def _handle_heartbeat(self, payload: Dict[str, Any]) -> None: | ||
| """Handle heartbeat from peer.""" | ||
| node_id = payload.get("node_id") | ||
| if node_id in self._nodes: | ||
| node = self._nodes[node_id] | ||
| node.last_heartbeat = time.time() | ||
| node.ram_available = payload.get("ram_available", 0.0) | ||
| node.cpu_percent = payload.get("cpu_percent", 0.0) |
There was a problem hiding this comment.
Heartbeat handler is unreachable in practice.
payload.get("node_id") returns None for every heartbeat sent by MeshTransport._heartbeat_loop (transport.py lines 367–371), so the if node_id in self._nodes: branch is never entered. As a result last_heartbeat/ram_available/cpu_percent are never refreshed from peers, and _state_sync_loop (line 424) will reap every peer after LEADER_TIMEOUT (30 s). See the proposed fix at the source in transport.py; a complementary safeguard here is to also accept the sender id from a connection-aware dispatch.
🤖 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 `@src/miniforge/mesh/coordinator.py` around lines 366 - 373, Handle missing
node_id in _handle_heartbeat by falling back to connection-aware sender fields:
if payload.get("node_id") is None, try payload.get("sender") and
payload.get("sender_id") (these are set by MeshTransport._heartbeat_loop when
dispatch is connection-aware) and use that value for node lookup in self._nodes
before updating node.last_heartbeat, node.ram_available and node.cpu_percent so
_state_sync_loop doesn't erroneously reap peers; keep behavior of skipping if no
valid id is found.
| async def _run_election(self) -> None: | ||
| """Run leader election.""" | ||
| logger.info("Running leader election") | ||
|
|
||
| # Collect all candidates (self + known nodes) | ||
| candidates = [ | ||
| (self.node_id, self._started_at) | ||
| ] | ||
| for node in self._nodes.values(): | ||
| # Use node_id as tiebreaker (lexicographic) | ||
| candidates.append((node.node_id, node.last_heartbeat)) | ||
|
|
||
| # Oldest node wins | ||
| winner = min(candidates, key=lambda x: (x[1], x[0])) | ||
| winner_id = winner[0] | ||
|
|
||
| self._leader_id = winner_id | ||
| self._is_leader = (winner_id == self.node_id) | ||
|
|
||
| if self._is_leader: | ||
| logger.info("Elected as leader") | ||
| else: | ||
| logger.info(f"Leader elected: {winner_id}") | ||
|
|
||
| self._notify_state_change() |
There was a problem hiding this comment.
Leader election compares incompatible timestamps — every node elects itself.
self._started_at is the wall-clock time this node booted (line 84), but for peers you use node.last_heartbeat (line 408), which NodeState's default_factory=time.time initializes to the moment the peer was discovered and which floats forward with each heartbeat. So:
- self timestamp ≈ when this process started
- peer timestamp ≥ when this process discovered the peer (always strictly later in absolute time)
min(...) therefore always picks self on every node simultaneously, producing split-brain leadership where every node sets self._is_leader = True. The fix requires propagating each node's actual started_at (e.g., via heartbeat or handshake) into NodeState and comparing against that.
🐛 Sketch of the fix
`@dataclass`
class NodeState:
...
+ started_at: float = field(default_factory=time.time)
last_heartbeat: float = field(default_factory=time.time)
...
async def _run_election(self) -> None:
candidates = [(self.node_id, self._started_at)]
for node in self._nodes.values():
- candidates.append((node.node_id, node.last_heartbeat))
+ candidates.append((node.node_id, node.started_at))
winner = min(candidates, key=lambda x: (x[1], x[0]))started_at then needs to be sent in the handshake/heartbeat payload (see the related heartbeat payload comment in transport.py) and stored when the node is first registered.
🤖 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 `@src/miniforge/mesh/coordinator.py` around lines 398 - 422, The election uses
incompatible timestamps (self._started_at vs NodeState.last_heartbeat) causing
every node to elect itself; fix by propagating and comparing each peer's actual
startup time instead: include a started_at field in the heartbeat/handshake
payload in transport.py, store that value on first registration into
NodeState.started_at (add started_at attribute to NodeState if missing) and
change _run_election to build candidates from (node.node_id, node.started_at)
for peers (keeping the lexicographic node_id tiebreaker) while still using
self._started_at for the local entry; ensure last_heartbeat remains for liveness
only.
| await conn.send("inference_request", { | ||
| "job_id": job.job_id, | ||
| "prompt": job.prompt, | ||
| "model_id": job.model_id, | ||
| "max_tokens": job.max_tokens, | ||
| }) |
There was a problem hiding this comment.
Sampling parameters are dropped on the wire and on the receiving side.
The inference_request payload sent at lines 185–190 carries only prompt, model_id, and max_tokens; temperature, top_p, top_k, and stream are silently discarded. _on_inference_request (lines 248–272) likewise calls local_engine.generate(prompt=..., max_tokens=...), so even if you added the fields to the payload they wouldn't be honored. This means any user calling generate(temperature=0.0, top_p=0.5, ...) with strategy="remote" gets defaults instead of their requested decoding policy — silently. The same applies to the local fallbacks at lines 171–174, 180–183, and 218–221, which was previously raised; please plumb the params through InferenceJob (or a shared payload struct) so they reach every code path.
🐛 Suggested propagation
await conn.send("inference_request", {
"job_id": job.job_id,
"prompt": job.prompt,
"model_id": job.model_id,
"max_tokens": job.max_tokens,
+ "temperature": job.temperature,
+ "top_p": job.top_p,
+ "top_k": job.top_k,
})Add temperature/top_p/top_k fields to InferenceJob (and schedule_inference's signature) so they are tracked per-job, then forward them in _on_inference_request to local_engine.generate(...).
Also applies to: 248-272
🤖 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 `@src/miniforge/mesh/engine.py` around lines 185 - 190, The payload and local
calls drop sampling params: update the InferenceJob data structure and
schedule_inference signature to include temperature, top_p, top_k, and stream,
ensure all call sites that construct InferenceJob (including the local fallback
paths around the schedule_inference calls) populate those fields, add those
fields to the dict sent in conn.send for "inference_request", and in
_on_inference_request extract them and pass them through to
local_engine.generate(..., temperature=..., top_p=..., top_k=..., stream=...) so
remote and local paths honor the requested decoding parameters.
There was a problem hiding this comment.
4 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/miniforge/mesh/coordinator.py">
<violation number="1" location="src/miniforge/mesh/coordinator.py:338">
P2: `_inference_handler` is always awaited even though its declared contract allows non-async callables, which can crash request handling at runtime.</violation>
<violation number="2" location="src/miniforge/mesh/coordinator.py:343">
P2: Do not include raw exception messages in peer responses; return a generic failure message and keep details in logs.</violation>
</file>
<file name="src/miniforge/mesh/transport.py">
<violation number="1" location="src/miniforge/mesh/transport.py:175">
P2: The new node-id connection index is not cleared in transport shutdown, so lookups can return stale closed connections after `stop()`.</violation>
</file>
<file name="src/miniforge/mesh/engine.py">
<violation number="1" location="src/miniforge/mesh/engine.py:256">
P1: Sampling parameters (`temperature`, `top_p`, `top_k`) are not extracted from the incoming payload in this handler. The caller's `generate(temperature=0.0, top_p=0.5, ...)` values are serialized into `InferenceJob` but when the request arrives here, only `job_id`, `prompt`, and `max_tokens` are unpacked — the rest are silently replaced with defaults. Extract these fields from `payload` and forward them to `local_engine.generate()`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| """Handle incoming inference request from remote node (coordinator callback).""" | ||
| job_id = payload.get("job_id", "unknown") | ||
| prompt = payload.get("prompt", "") | ||
| max_tokens = payload.get("max_tokens", 512) |
There was a problem hiding this comment.
P1: Sampling parameters (temperature, top_p, top_k) are not extracted from the incoming payload in this handler. The caller's generate(temperature=0.0, top_p=0.5, ...) values are serialized into InferenceJob but when the request arrives here, only job_id, prompt, and max_tokens are unpacked — the rest are silently replaced with defaults. Extract these fields from payload and forward them to local_engine.generate().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/miniforge/mesh/engine.py, line 256:
<comment>Sampling parameters (`temperature`, `top_p`, `top_k`) are not extracted from the incoming payload in this handler. The caller's `generate(temperature=0.0, top_p=0.5, ...)` values are serialized into `InferenceJob` but when the request arrives here, only `job_id`, `prompt`, and `max_tokens` are unpacked — the rest are silently replaced with defaults. Extract these fields from `payload` and forward them to `local_engine.generate()`.</comment>
<file context>
@@ -240,21 +243,18 @@ async def _wait_for_job_completion(self, job: InferenceJob) -> None:
+ """Handle incoming inference request from remote node (coordinator callback)."""
+ job_id = payload.get("job_id", "unknown")
+ prompt = payload.get("prompt", "")
+ max_tokens = payload.get("max_tokens", 512)
+
logger.info(f"Handling remote request {job_id}")
</file context>
| logger.error(f"Inference handler error for {job_id}: {e}") | ||
| await conn.send("inference_response", { | ||
| "job_id": job_id, | ||
| "result": f"[Error: {e}]", |
There was a problem hiding this comment.
P2: Do not include raw exception messages in peer responses; return a generic failure message and keep details in logs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/miniforge/mesh/coordinator.py, line 343:
<comment>Do not include raw exception messages in peer responses; return a generic failure message and keep details in logs.</comment>
<file context>
@@ -319,8 +333,23 @@ async def _handle_inference_request(self, payload: Dict[str, Any]) -> None:
+ logger.error(f"Inference handler error for {job_id}: {e}")
+ await conn.send("inference_response", {
+ "job_id": job_id,
+ "result": f"[Error: {e}]",
+ "status": "failed",
+ })
</file context>
| "result": f"[Error: {e}]", | |
| "result": "[Error: Inference execution failed]", |
|
|
||
| if self._inference_handler: | ||
| try: | ||
| await self._inference_handler(payload, conn) |
There was a problem hiding this comment.
P2: _inference_handler is always awaited even though its declared contract allows non-async callables, which can crash request handling at runtime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/miniforge/mesh/coordinator.py, line 338:
<comment>`_inference_handler` is always awaited even though its declared contract allows non-async callables, which can crash request handling at runtime.</comment>
<file context>
@@ -319,8 +333,23 @@ async def _handle_inference_request(self, payload: Dict[str, Any]) -> None:
- # For now, just acknowledge
+ if self._inference_handler:
+ try:
+ await self._inference_handler(payload, conn)
+ except Exception as e:
+ logger.error(f"Inference handler error for {job_id}: {e}")
</file context>
| await self._inference_handler(payload, conn) | |
| handler_result = self._inference_handler(payload, conn) | |
| if asyncio.iscoroutine(handler_result): | |
| await handler_result |
| self.security = security | ||
|
|
||
| self._connections: Dict[str, MeshConnection] = {} | ||
| self._node_id_to_connection: Dict[str, MeshConnection] = {} |
There was a problem hiding this comment.
P2: The new node-id connection index is not cleared in transport shutdown, so lookups can return stale closed connections after stop().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/miniforge/mesh/transport.py, line 175:
<comment>The new node-id connection index is not cleared in transport shutdown, so lookups can return stale closed connections after `stop()`.</comment>
<file context>
@@ -170,6 +172,7 @@ def __init__(
self.security = security
self._connections: Dict[str, MeshConnection] = {}
+ self._node_id_to_connection: Dict[str, MeshConnection] = {}
self._callbacks: List[Callable[[str, MeshConnection, str], None]] = []
self._running = False
</file context>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
src/miniforge/mesh/transport.py (3)
305-357:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
peer_key/conninfinallyand always close the accepted stream.Any timeout or parse error before Line 330 leaves
peer_keyundefined, so thefinallyblock can raiseUnboundLocalErrorand mask the real handshake failure. Those exception paths also skip a final close on the accepted socket.Suggested fix
async def _handle_incoming( self, reader: asyncio.StreamReader, writer: asyncio.StreamWriter ) -> None: """Handle incoming connection.""" + peer_key: Optional[str] = None + conn: Optional[MeshConnection] = None peer_addr = writer.get_extra_info("peername") logger.debug(f"Incoming connection from {peer_addr}") try: ... - conn = MeshConnection(reader, writer, self.node_id, self.security, peer_node_id=peer_id) + conn = MeshConnection(reader, writer, self.node_id, self.security, peer_node_id=peer_id) await conn.start() ... finally: - if peer_key in self._connections: - conn = self._connections.pop(peer_key) + if peer_key and peer_key in self._connections: + conn = self._connections.pop(peer_key) if conn.peer_node_id in self._node_id_to_connection: del self._node_id_to_connection[conn.peer_node_id] + if not writer.is_closing(): + writer.close() + await writer.wait_closed()🤖 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 `@src/miniforge/mesh/transport.py` around lines 305 - 357, Initialize peer_key and conn to None before the try block and in the finally block first check if conn is not None to pop from self._connections and call its shutdown/close method; additionally always close the accepted socket stream (writer.close() and await writer.wait_closed() if writer is set) for error/timeout/parse paths where MeshConnection was never created; reference symbols: peer_key, conn, reader, writer, MeshConnection, self._connections, self._node_id_to_connection, and the existing finally block that currently pops by peer_key.
103-105:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRaise the frame cap above the mesh chunk size.
Line 103 still drops anything over 10 MiB, but model transfer elsewhere in the mesh is chunked into single framed messages. If those chunks are larger than 10 MiB, the first
model_chunkwill tear down the connection mid-download.🤖 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 `@src/miniforge/mesh/transport.py` around lines 103 - 105, The 10 MiB hard cap is tearing down chunked model transfers; replace the literal check with a higher or configurable frame limit that matches or exceeds the mesh chunk size (e.g., define FRAME_MAX_SIZE or use existing MESH_CHUNK_SIZE/MODEL_CHUNK_MAX_SIZE) and use that constant in the if (replace "10 * 1024 * 1024" with FRAME_MAX_SIZE); ensure the new limit is documented/commented and enforced in the same location where length is checked so large framed model_chunk messages are not dropped mid-download.
253-289:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse
peer_idas the canonical connection key.Outbound stores
ip:mesh_port, while inbound storesip:ephemeral_client_portfrom Line 330. The same logical peer can therefore exist twice in_connections, and disconnect/heartbeat bookkeeping will drift between those entries.Also applies to: 329-342
🤖 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 `@src/miniforge/mesh/transport.py` around lines 253 - 289, The code currently keys outbound connections by ip:port (peer_key) which can differ from inbound keys (ephemeral port), causing duplicate entries and drift; after reading and parsing peer_id from the handshake (the variable peer_id in the connect/handshake flow that creates MeshConnection and uses MeshConnection.start), switch to using peer_id as the canonical key: first check self._node_id_to_connection (or self._connections) for an existing connection by peer_id and return/replace it, store the new MeshConnection under self._connections[peer_id] and self._node_id_to_connection[peer_id], and remove any stale ip:port keyed entries (peer_key) if present; apply the same canonicalization change to the inbound-handshake code path that currently uses ip:ephemeral_port (the block around lines 329-342) so all connect/heartbeat/disconnect bookkeeping consistently uses peer_id.src/miniforge/mesh/discovery.py (1)
221-236:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThe scan probe still does not match the transport protocol.
_check_host()sendsMINIFORGE_PROBEand waits forMINIFORGE_NODE, butMeshTransport._handle_incoming()only acceptsMINIFORGE_NODE|...and responds withMINIFORGE_OK|.... The subnet-scan fallback cannot discover any peer as written.🤖 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 `@src/miniforge/mesh/discovery.py` around lines 221 - 236, The probe/swap mismatch: _check_host() currently sends the literal MINIFORGE_PROBE but MeshTransport._handle_incoming() expects incoming peers to send MINIFORGE_NODE|... and then replies MINIFORGE_OK|..., so subnet scan never discovers peers. Fix _check_host() to send a MINIFORGE_NODE formatted handshake (e.g., "MINIFORGE_NODE|{node_id}|{node_name}|{ram_gb}|{cpu_cores}\n") instead of MINIFORGE_PROBE and then wait for a response containing "MINIFORGE_OK" (parse the returned fields accordingly), or alternatively update MeshTransport._handle_incoming() to accept MINIFORGE_PROBE and reply with MINIFORGE_NODE/MINIFORGE_OK consistently—use the existing symbols _check_host(), MeshTransport._handle_incoming(), MINIFORGE_PROBE, MINIFORGE_NODE, and MINIFORGE_OK to keep the protocol aligned.
🧹 Nitpick comments (1)
src/miniforge/mesh/discovery.py (1)
81-89: ⚡ Quick winKeep strong references to these background discovery tasks.
Both event paths fire
asyncio.create_task(...)and immediately drop the handle. Since these tasks propagate peer add/update events, keep them in a set and remove them on completion instead of fire-and-forget.Also applies to: 336-336
🤖 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 `@src/miniforge/mesh/discovery.py` around lines 81 - 89, The callback invocation in _notify currently fire-and-forgets any awaitable via asyncio.create_task(result); keep strong references to these tasks by adding a set (e.g., self._background_tasks) to the Discovery class, add each created task to that set, and attach a done callback that removes the task from the set when complete; apply the same pattern to the other create_task call-site in this module (the other discovery callback path) so no background discovery tasks are immediately dropped.
🤖 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 `@src/miniforge/mesh/discovery.py`:
- Around line 170-175: The scan loop in run/_scan loop only updates last_seen on
successful probes and never evicts dead nodes; call the peer cleanup routine
after each scan cycle to prune stale entries. Specifically, after awaiting
self._scan_network() (and also inside the except block so cleanup still runs on
error), invoke self.remove_stale_peers() to remove entries from peers that have
expired, ensuring the coordinator stops treating dead endpoints as discoverable;
use the existing remove_stale_peers method name to locate and perform the call.
- Around line 142-146: The ServiceInfo registration is using ASCII-encoded IP
bytes causing zeroconf to fail; replace
addresses=[self._get_local_ip().encode()] with the packed network-byte-order
bytes for the IP returned by self._get_local_ip() (use socket.inet_aton for IPv4
and socket.inet_pton(AF_INET6, ...) for IPv6, or detect address family via
ipaddress.ip_address and pack accordingly) so ServiceInfo.addresses contains 4
or 16 byte packed addresses compatible with the ipaddress.ip_address(...)
parsing used elsewhere; adjust the ServiceInfo construction in the block that
creates info (and any related code that reads info.addresses) to use the packed
bytes.
In `@src/miniforge/mesh/transport.py`:
- Around line 258-297: The outbound writer opened in the try block is not closed
on error, leaking sockets; update the except block that currently logs and
returns to first check for a created writer (e.g. if "writer" in locals() and
not writer.is_closing()), call writer.close() and await writer.wait_closed()
before returning, so any partially-open outbound connection is properly closed;
reference writer, writer.close(), writer.wait_closed(), MeshConnection and conn
to locate the handshake/connect code path to modify.
- Around line 219-223: The server and client socket calls are currently created
without TLS; update the asyncio calls to use the MeshSecurity TLS context: when
creating the server assign self._server = await
asyncio.start_server(self._handle_incoming, host="0.0.0.0", port=self.mesh_port,
ssl=self.security.create_tls_context(server_mode=True)) so the server uses mTLS,
and when dialing use asyncio.open_connection(...,
ssl=self.security.create_tls_context(server_mode=False)) so outgoing connections
use the client TLS context; ensure you pass the respective
create_tls_context(server_mode=...) from the MeshSecurity instance
(self.security) to both calls.
---
Duplicate comments:
In `@src/miniforge/mesh/discovery.py`:
- Around line 221-236: The probe/swap mismatch: _check_host() currently sends
the literal MINIFORGE_PROBE but MeshTransport._handle_incoming() expects
incoming peers to send MINIFORGE_NODE|... and then replies MINIFORGE_OK|..., so
subnet scan never discovers peers. Fix _check_host() to send a MINIFORGE_NODE
formatted handshake (e.g.,
"MINIFORGE_NODE|{node_id}|{node_name}|{ram_gb}|{cpu_cores}\n") instead of
MINIFORGE_PROBE and then wait for a response containing "MINIFORGE_OK" (parse
the returned fields accordingly), or alternatively update
MeshTransport._handle_incoming() to accept MINIFORGE_PROBE and reply with
MINIFORGE_NODE/MINIFORGE_OK consistently—use the existing symbols _check_host(),
MeshTransport._handle_incoming(), MINIFORGE_PROBE, MINIFORGE_NODE, and
MINIFORGE_OK to keep the protocol aligned.
In `@src/miniforge/mesh/transport.py`:
- Around line 305-357: Initialize peer_key and conn to None before the try block
and in the finally block first check if conn is not None to pop from
self._connections and call its shutdown/close method; additionally always close
the accepted socket stream (writer.close() and await writer.wait_closed() if
writer is set) for error/timeout/parse paths where MeshConnection was never
created; reference symbols: peer_key, conn, reader, writer, MeshConnection,
self._connections, self._node_id_to_connection, and the existing finally block
that currently pops by peer_key.
- Around line 103-105: The 10 MiB hard cap is tearing down chunked model
transfers; replace the literal check with a higher or configurable frame limit
that matches or exceeds the mesh chunk size (e.g., define FRAME_MAX_SIZE or use
existing MESH_CHUNK_SIZE/MODEL_CHUNK_MAX_SIZE) and use that constant in the if
(replace "10 * 1024 * 1024" with FRAME_MAX_SIZE); ensure the new limit is
documented/commented and enforced in the same location where length is checked
so large framed model_chunk messages are not dropped mid-download.
- Around line 253-289: The code currently keys outbound connections by ip:port
(peer_key) which can differ from inbound keys (ephemeral port), causing
duplicate entries and drift; after reading and parsing peer_id from the
handshake (the variable peer_id in the connect/handshake flow that creates
MeshConnection and uses MeshConnection.start), switch to using peer_id as the
canonical key: first check self._node_id_to_connection (or self._connections)
for an existing connection by peer_id and return/replace it, store the new
MeshConnection under self._connections[peer_id] and
self._node_id_to_connection[peer_id], and remove any stale ip:port keyed entries
(peer_key) if present; apply the same canonicalization change to the
inbound-handshake code path that currently uses ip:ephemeral_port (the block
around lines 329-342) so all connect/heartbeat/disconnect bookkeeping
consistently uses peer_id.
---
Nitpick comments:
In `@src/miniforge/mesh/discovery.py`:
- Around line 81-89: The callback invocation in _notify currently
fire-and-forgets any awaitable via asyncio.create_task(result); keep strong
references to these tasks by adding a set (e.g., self._background_tasks) to the
Discovery class, add each created task to that set, and attach a done callback
that removes the task from the set when complete; apply the same pattern to the
other create_task call-site in this module (the other discovery callback path)
so no background discovery tasks are immediately dropped.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b64c031-aa6c-4c1d-98f7-e9b0733ca391
📒 Files selected for processing (3)
run_mesh_simple.pysrc/miniforge/mesh/discovery.pysrc/miniforge/mesh/transport.py
| info = ServiceInfo( | ||
| type_=self.SERVICE_TYPE, | ||
| name=f"{self.node_name}.{self.SERVICE_TYPE}", | ||
| addresses=[self._get_local_ip().encode()], | ||
| port=self.mesh_port, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the discovery.py file
find . -name "discovery.py" -type fRepository: Zapdev-labs/miniforge
Length of output: 99
🏁 Script executed:
# Get the structure and key sections of the file
head -50 src/miniforge/mesh/discovery.pyRepository: Zapdev-labs/miniforge
Length of output: 1313
🏁 Script executed:
# Find the _get_local_ip method
rg "_get_local_ip" src/miniforge/mesh/discovery.py -A 10Repository: Zapdev-labs/miniforge
Length of output: 1415
🏁 Script executed:
# Check imports and ServiceInfo usage
rg "ServiceInfo|import" src/miniforge/mesh/discovery.py | head -20Repository: Zapdev-labs/miniforge
Length of output: 584
🏁 Script executed:
# Check if socket module is imported and how inet functions are used elsewhere
rg "socket\." src/miniforge/mesh/discovery.pyRepository: Zapdev-labs/miniforge
Length of output: 130
🏁 Script executed:
# Let's also check if there are any tests that might reveal expected behavior
find . -name "*test*discovery*" -o -name "*discovery*test*" | head -10Repository: Zapdev-labs/miniforge
Length of output: 47
🏁 Script executed:
# Check zeroconf library version and look for ServiceInfo documentation/usage
rg "ServiceInfo" src/miniforge/mesh/discovery.py -B 5 -A 15Repository: Zapdev-labs/miniforge
Length of output: 2409
🏁 Script executed:
# Look for any logging or error handling related to address registration
rg "addresses|register|mDNS" src/miniforge/mesh/discovery.pyRepository: Zapdev-labs/miniforge
Length of output: 1012
🏁 Script executed:
# Check if there are any comments or documentation about address format
cat -n src/miniforge/mesh/discovery.py | grep -A 20 "ServiceInfo"Repository: Zapdev-labs/miniforge
Length of output: 2812
🏁 Script executed:
# Look for requirements or setup files to find zeroconf version
find . -name "requirements*.txt" -o -name "setup.py" -o -name "pyproject.toml" | xargs cat 2>/dev/null | grep -i zeroconfRepository: Zapdev-labs/miniforge
Length of output: 91
🏁 Script executed:
# Let's check the zeroconf library documentation by examining version constraints
rg "zeroconf" . --type txt --type toml --type cfgRepository: Zapdev-labs/miniforge
Length of output: 93
🏁 Script executed:
# Let's look at how the receiving side processes addresses to understand what format is expected
cat -n src/miniforge/mesh/discovery.py | sed -n '320,340p'Repository: Zapdev-labs/miniforge
Length of output: 1031
🏁 Script executed:
# Check if there are any actual tests that might show whether address encoding works
find . -type f -name "*.py" -path "*/test*" | xargs grep -l "ServiceInfo\|discovery" 2>/dev/nullRepository: Zapdev-labs/miniforge
Length of output: 49
🏁 Script executed:
# Let's verify the exact behavior by checking if ipaddress.ip_address() can parse ASCII-encoded bytes
python3 << 'EOF'
import ipaddress
# Test what the current code produces
ip_string = "192.168.1.10"
ascii_bytes = ip_string.encode() # b"192.168.1.10" - 12 bytes
print(f"ASCII bytes: {ascii_bytes}")
print(f"Length: {len(ascii_bytes)}")
# Test what socket.inet_aton produces
import socket
packed_bytes = socket.inet_aton(ip_string) # 4 bytes
print(f"\nPacked bytes: {packed_bytes}")
print(f"Length: {len(packed_bytes)}")
# Test if ipaddress.ip_address() can parse each
print("\n--- Testing ipaddress.ip_address() ---")
try:
result = ipaddress.ip_address(ascii_bytes)
print(f"ASCII bytes parse succeeded: {result}")
except Exception as e:
print(f"ASCII bytes parse failed: {type(e).__name__}: {e}")
try:
result = ipaddress.ip_address(packed_bytes)
print(f"Packed bytes parse succeeded: {result}")
except Exception as e:
print(f"Packed bytes parse failed: {type(e).__name__}: {e}")
EOFRepository: Zapdev-labs/miniforge
Length of output: 331
🏁 Script executed:
# Let's verify zeroconf library expectations by checking its source or documentation
pip show zeroconf 2>/dev/null | grep -i version || echo "Checking alternative method"Repository: Zapdev-labs/miniforge
Length of output: 93
🏁 Script executed:
# Let's trace through what happens when a service is received - can the registration actually succeed?
# Check if ServiceInfo accepts ASCII bytes or requires packed bytes
python3 << 'EOF'
import socket
# Simulate zeroconf behavior - ServiceInfo typically expects addresses as bytes
# but the format matters
# Test what zeroconf expects
ip_string = "192.168.1.10"
ascii_bytes = ip_string.encode() # What current code does
packed_bytes = socket.inet_aton(ip_string) # What fix suggests
print("Current approach (ASCII bytes):")
print(f" Value: {ascii_bytes}")
print(f" Type: {type(ascii_bytes)}")
print(f" Length: {len(ascii_bytes)}")
print("\nProposed approach (packed bytes):")
print(f" Value: {packed_bytes}")
print(f" Type: {type(packed_bytes)}")
print(f" Length: {len(packed_bytes)}")
# The zeroconf library expects network byte order (packed format)
# ASCII bytes of length 12 are invalid
print("\nZeroconf expects:")
print(" IPv4: 4-byte packed format (e.g., b'\\xc0\\xa8\\x01\\n')")
print(" IPv6: 16-byte packed format")
print("\nCurrent code violates this expectation.")
EOFRepository: Zapdev-labs/miniforge
Length of output: 402
🏁 Script executed:
# Let's verify if there are any integration tests or real-world examples
find . -type f -name "*.py" | xargs grep -l "ServiceInfo" 2>/dev/null | head -5Repository: Zapdev-labs/miniforge
Length of output: 99
🌐 Web query:
zeroconf python ServiceInfo addresses format requirements
💡 Result:
The addresses parameter in python-zeroconf's ServiceInfo constructor is a list of bytes representing IP addresses in network byte order. IPv4 addresses are 4-byte sequences (e.g., socket.inet_aton('192.168.1.1')), and IPv6 addresses are 16-byte sequences (e.g., socket.inet_pton(socket.AF_INET6, '::1')) [1][2][3]. Alternatively, use parsed_addresses, a list of string IP addresses (e.g., ['192.168.1.1', '::1']), which the library internally encodes to bytes via _encode_address or similar functions [1][2]. You cannot provide both parameters together [1]. The library validates and separates them into IPv4/IPv6 lists, raising TypeError for invalid formats, and suggests using socket.inet_pton for strings [1]. For registration, common usage is addresses=[socket.inet_aton(ip)] for IPv4 [4][5]. To decode later, use socket.inet_ntoa(info.addresses[0]) for IPv4 or info.parsed_addresses() [4].
Citations:
- 1: https://python-zeroconf.readthedocs.io/en/latest/_modules/zeroconf/_services/info.html
- 2: https://python-zeroconf.readthedocs.io/en/latest/api.html
- 3: https://python-zeroconf.readthedocs.io/en/stable/api.html
- 4: https://stackoverflow.com/questions/51593574/python-zeroconf-show-ipv4-addresses
- 5: https://forums.balena.io/t/discover-avahi-zeroconf-services-inside-container/48665
Fix mDNS address format: use packed bytes instead of ASCII-encoded IP string.
ServiceInfo.addresses requires network byte order (4 bytes for IPv4, 16 bytes for IPv6). The current code addresses=[self._get_local_ip().encode()] produces ASCII bytes (e.g., b"192.168.1.10", 12 bytes), which violates zeroconf's format requirements. This causes ipaddress.ip_address(info.addresses[0]) at line 323 to fail when peers try to parse the registration, silently breaking peer discovery via the exception handler at lines 337-338.
Suggested fix
+import socket
...
info = ServiceInfo(
type_=self.SERVICE_TYPE,
name=f"{self.node_name}.{self.SERVICE_TYPE}",
- addresses=[self._get_local_ip().encode()],
+ addresses=[socket.inet_aton(self._get_local_ip())],
port=self.mesh_port,
properties={🤖 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 `@src/miniforge/mesh/discovery.py` around lines 142 - 146, The ServiceInfo
registration is using ASCII-encoded IP bytes causing zeroconf to fail; replace
addresses=[self._get_local_ip().encode()] with the packed network-byte-order
bytes for the IP returned by self._get_local_ip() (use socket.inet_aton for IPv4
and socket.inet_pton(AF_INET6, ...) for IPv6, or detect address family via
ipaddress.ip_address and pack accordingly) so ServiceInfo.addresses contains 4
or 16 byte packed addresses compatible with the ipaddress.ip_address(...)
parsing used elsewhere; adjust the ServiceInfo construction in the block that
creates info (and any related code that reads info.addresses) to use the packed
bytes.
| while self._running: | ||
| try: | ||
| await self._scan_network() | ||
| except Exception as e: | ||
| logger.warning(f"IP scan error: {e}") | ||
| await asyncio.sleep(10) # Scan every 10 seconds |
There was a problem hiding this comment.
Prune stale peers from the scan loop.
Failed probes only stop refreshing last_seen; nothing here ever calls remove_stale_peers(). Once a node disappears it stays in peers indefinitely, so the coordinator keeps treating a dead endpoint as discoverable.
Suggested fix
while self._running:
try:
await self._scan_network()
+ await self.remove_stale_peers()
except Exception as e:
logger.warning(f"IP scan error: {e}")
await asyncio.sleep(10) # Scan every 10 seconds🧰 Tools
🪛 Ruff (0.15.12)
[warning] 173-173: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@src/miniforge/mesh/discovery.py` around lines 170 - 175, The scan loop in
run/_scan loop only updates last_seen on successful probes and never evicts dead
nodes; call the peer cleanup routine after each scan cycle to prune stale
entries. Specifically, after awaiting self._scan_network() (and also inside the
except block so cleanup still runs on error), invoke self.remove_stale_peers()
to remove entries from peers that have expired, ensuring the coordinator stops
treating dead endpoints as discoverable; use the existing remove_stale_peers
method name to locate and perform the call.
| self._server = await asyncio.start_server( | ||
| self._handle_incoming, | ||
| host="0.0.0.0", | ||
| port=self.mesh_port, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "transport.py" | head -20Repository: Zapdev-labs/miniforge
Length of output: 99
🏁 Script executed:
cat -n ./src/miniforge/mesh/transport.py | sed -n '210,270p'Repository: Zapdev-labs/miniforge
Length of output: 2531
🏁 Script executed:
rg -i "meshsecurity" --type py -A 5 -B 5Repository: Zapdev-labs/miniforge
Length of output: 7287
🏁 Script executed:
cat -n ./src/miniforge/mesh/security.py | head -100Repository: Zapdev-labs/miniforge
Length of output: 4291
🏁 Script executed:
cat -n ./src/miniforge/mesh/security.py | sed -n '100,200p'Repository: Zapdev-labs/miniforge
Length of output: 5075
🏁 Script executed:
cat -n ./src/miniforge/mesh/security.py | sed -n '200,220p'Repository: Zapdev-labs/miniforge
Length of output: 338
🏁 Script executed:
cat -n ./src/miniforge/mesh/transport.py | sed -n '160,210p'Repository: Zapdev-labs/miniforge
Length of output: 2247
🏁 Script executed:
rg "create_tls_context|ssl\.|SSLContext" ./src/miniforge/mesh/transport.pyRepository: Zapdev-labs/miniforge
Length of output: 47
🏁 Script executed:
wc -l ./src/miniforge/mesh/transport.pyRepository: Zapdev-labs/miniforge
Length of output: 103
🏁 Script executed:
cat -n ./src/miniforge/mesh/transport.pyRepository: Zapdev-labs/miniforge
Length of output: 17134
Pass the SSL context from MeshSecurity to both asyncio.start_server() and asyncio.open_connection().
At lines 219-223, asyncio.start_server() is missing the ssl=self.security.create_tls_context(server_mode=True) parameter. At lines 259-262, asyncio.open_connection() is missing the ssl=self.security.create_tls_context(server_mode=False) parameter. The module's advertised mTLS, certificate pinning, and peer authentication are completely bypassed; connections are currently plaintext TCP with a simple magic-string handshake.
🧰 Tools
🪛 Ruff (0.15.12)
[error] 221-221: Possible binding to all interfaces
(S104)
🤖 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 `@src/miniforge/mesh/transport.py` around lines 219 - 223, The server and
client socket calls are currently created without TLS; update the asyncio calls
to use the MeshSecurity TLS context: when creating the server assign
self._server = await asyncio.start_server(self._handle_incoming, host="0.0.0.0",
port=self.mesh_port, ssl=self.security.create_tls_context(server_mode=True)) so
the server uses mTLS, and when dialing use asyncio.open_connection(...,
ssl=self.security.create_tls_context(server_mode=False)) so outgoing connections
use the client TLS context; ensure you pass the respective
create_tls_context(server_mode=...) from the MeshSecurity instance
(self.security) to both calls.
| try: | ||
| reader, writer = await asyncio.wait_for( | ||
| asyncio.open_connection(ip, port), | ||
| timeout=5.0, | ||
| ) | ||
|
|
||
| # Send handshake | ||
| handshake = f"{self.PROTOCOL_MAGIC.decode()}|{self.node_id}|{self.node_name}|{self.ram_gb}|{self.cpu_cores}\n" | ||
| writer.write(handshake.encode()) | ||
| await writer.drain() | ||
|
|
||
| # Read response | ||
| response = await asyncio.wait_for(reader.readline(), timeout=5.0) | ||
| response_str = response.decode().strip() | ||
|
|
||
| if not response_str.startswith("MINIFORGE_OK|"): | ||
| logger.warning(f"Peer {peer_key} rejected handshake: {response_str}") | ||
| writer.close() | ||
| await writer.wait_closed() | ||
| return None | ||
|
|
||
| # Parse peer info from response | ||
| parts = response_str.split("|") | ||
| peer_id = parts[1] if len(parts) > 1 else "unknown" | ||
|
|
||
| # Create connection | ||
| conn = MeshConnection(reader, writer, self.node_id, self.security, peer_node_id=peer_id) | ||
| conn.on("heartbeat", lambda _: None) # Update last_seen | ||
| await conn.start() | ||
|
|
||
| self._connections[peer_key] = conn | ||
| self._node_id_to_connection[peer_id] = conn | ||
| self._notify(peer_id, conn, "connected") | ||
|
|
||
| logger.info(f"Connected to peer {peer_id} @ {peer_key}") | ||
| return conn | ||
|
|
||
| except Exception as e: | ||
| logger.debug(f"Failed to connect to {peer_key}: {e}") | ||
| return None |
There was a problem hiding this comment.
Close the outbound writer when the handshake path throws.
Once Line 259 succeeds, any later timeout/decode failure falls into the catch-all at Line 295 and returns None without closing writer. Repeated discovery retries will accumulate half-open sockets/file descriptors.
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 295-295: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@src/miniforge/mesh/transport.py` around lines 258 - 297, The outbound writer
opened in the try block is not closed on error, leaking sockets; update the
except block that currently logs and returns to first check for a created writer
(e.g. if "writer" in locals() and not writer.is_closing()), call writer.close()
and await writer.wait_closed() before returning, so any partially-open outbound
connection is properly closed; reference writer, writer.close(),
writer.wait_closed(), MeshConnection and conn to locate the handshake/connect
code path to modify.
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="run_mesh_benchmark.py">
<violation number="1" location="run_mesh_benchmark.py:64">
P2: Handler attribution is race-prone: selecting the globally latest job from `coordinator._jobs` can mismatch requests during concurrent benchmarks.</violation>
<violation number="2" location="run_mesh_benchmark.py:102">
P2: `--requests` is not validated, but the code always accesses `prompts[0]` and `prompts[1]`, which can crash with `IndexError` for small values.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
| parser.add_argument("--ctx", type=int, default=4096, help="Context window (default 4096 for weight caching)") | ||
| parser.add_argument("--max-tokens", type=int, default=64) | ||
| parser.add_argument("--requests", type=int, default=4, help="Concurrent requests for benchmark") | ||
| args = parser.parse_args() |
There was a problem hiding this comment.
P2: --requests is not validated, but the code always accesses prompts[0] and prompts[1], which can crash with IndexError for small values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At run_mesh_benchmark.py, line 102:
<comment>`--requests` is not validated, but the code always accesses `prompts[0]` and `prompts[1]`, which can crash with `IndexError` for small values.</comment>
<file context>
@@ -0,0 +1,254 @@
+ parser.add_argument("--ctx", type=int, default=4096, help="Context window (default 4096 for weight caching)")
+ parser.add_argument("--max-tokens", type=int, default=64)
+ parser.add_argument("--requests", type=int, default=4, help="Concurrent requests for benchmark")
+ args = parser.parse_args()
+
+ import uuid
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
| jobs = coordinator._jobs | ||
| # Get most recent job | ||
| if jobs: | ||
| latest_job = max(jobs.values(), key=lambda j: j.created_at) |
There was a problem hiding this comment.
P2: Handler attribution is race-prone: selecting the globally latest job from coordinator._jobs can mismatch requests during concurrent benchmarks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At run_mesh_benchmark.py, line 64:
<comment>Handler attribution is race-prone: selecting the globally latest job from `coordinator._jobs` can mismatch requests during concurrent benchmarks.</comment>
<file context>
@@ -0,0 +1,254 @@
+ jobs = coordinator._jobs
+ # Get most recent job
+ if jobs:
+ latest_job = max(jobs.values(), key=lambda j: j.created_at)
+ handler = latest_job.assigned_node or node_id
+ if handler == node_id:
</file context>
1 participant
Summary
This PR adds a complete distributed inference mesh system that allows multiple miniforge devices to auto-discover each other and share compute resources.
Features
🔍 Auto-Discovery
🔐 Security
🧠 Inference Strategies
🌐 Web Dashboard
🖥️ CLI
Architecture
New Dependencies
Added to under :
Files Added
Usage Example
Device A (192.168.1.100):
Device B (192.168.1.101):
Combined Resources:
Testing
Checklist
Related
Based on deep research into:
Summary by CodeRabbit