Skip to content

fix: zenoh forking issue - #3503

Merged
paul-nechifor merged 1 commit into
mainfrom
paul/fix/zenoh-fork-issue
Aug 20, 2026
Merged

fix: zenoh forking issue#3503
paul-nechifor merged 1 commit into
mainfrom
paul/fix/zenoh-fork-issue

Conversation

@paul-nechifor

Copy link
Copy Markdown
Contributor

Problem

The Coordinator RPC is broken when using Zenoh.

Closes #3395

Solution

Start the Zenoh sessions (i.e. call ModuleCoordinator.build) only after forking.

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.97403% with 37 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
dimos/cli/dimos.py 55.35% 23 Missing and 2 partials ⚠️
dimos/core/test_daemon_zenoh.py 65.62% 10 Missing and 1 partial ⚠️
dimos/core/daemon.py 96.00% 0 Missing and 1 partial ⚠️
@@            Coverage Diff             @@
##             main    #3503      +/-   ##
==========================================
+ Coverage   76.40%   76.44%   +0.03%     
==========================================
  Files        1244     1245       +1     
  Lines      120503   120621     +118     
  Branches    10737    10743       +6     
==========================================
+ Hits        92069    92205     +136     
+ Misses      25333    25315      -18     
  Partials     3101     3101              
Flag Coverage Δ
OS-ubuntu-24.04-arm 70.93% <75.97%> (+0.04%) ⬆️
OS-ubuntu-latest 72.68% <75.97%> (+0.04%) ⬆️
Py-3.10 72.67% <75.97%> (+0.03%) ⬆️
Py-3.11 72.67% <75.97%> (+0.03%) ⬆️
Py-3.12 72.68% <75.97%> (+0.04%) ⬆️
Py-3.13 72.67% <75.97%> (+0.04%) ⬆️
Py-3.14 72.68% <75.97%> (+0.04%) ⬆️
Py-3.14t 72.68% <75.97%> (+0.04%) ⬆️
SelfHosted-Large 29.81% <21.42%> (-0.01%) ⬇️
SelfHosted-Linux 35.76% <21.42%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
dimos/core/demos/stress_test_module.py 89.47% <ø> (-1.01%) ⬇️
dimos/core/test_daemon.py 99.18% <100.00%> (+0.07%) ⬆️
dimos/protocol/service/test_zenohservice.py 98.92% <100.00%> (+0.10%) ⬆️
dimos/protocol/service/zenohservice.py 97.36% <100.00%> (+0.08%) ⬆️
dimos/core/daemon.py 91.93% <96.00%> (+27.64%) ⬆️
dimos/core/test_daemon_zenoh.py 65.62% <65.62%> (ø)
dimos/cli/dimos.py 66.46% <55.35%> (+1.37%) ⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Aug 18, 2026
mustafab0
mustafab0 previously approved these changes Aug 19, 2026
@paul-nechifor
paul-nechifor added this pull request to the merge queue Aug 19, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 19, 2026
leshy
leshy previously approved these changes Aug 19, 2026
@leshy
leshy added this pull request to the merge queue Aug 19, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 19, 2026
@paul-nechifor
paul-nechifor dismissed stale reviews from leshy and mustafab0 via b419b55 August 19, 2026 23:58
@paul-nechifor
paul-nechifor force-pushed the paul/fix/zenoh-fork-issue branch from 9b03c67 to b419b55 Compare August 19, 2026 23:58
@github-actions github-actions Bot removed the ready-to-merge Required CI checks have passed on this PR label Aug 19, 2026
@greptile-apps

greptile-apps Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This change moves Zenoh-owning coordinator initialization into the daemon grandchild and adds a readiness status channel back to the launching CLI process. The real subprocess daemon flow did not complete: dimos run --daemon remained running for 120 seconds after starting the daemon and multiprocessing forkserver, so the independent Zenoh client ping could not run.

Confidence Score: 4/5

Not safe to merge until daemon startup reliably returns after the coordinator is ready.

The focused end-to-end test reproduced the failure with the real CLI double-fork flow, isolated state directories, Zenoh transport, and a separate intended client process. The launcher timeout is directly observed, although the captured run does not isolate the internal operation that prevents readiness from being reported.

Files Needing Attention: dimos/cli/dimos.py needs attention around coordinator construction and readiness reporting; the daemon startup path should be traced through ModuleCoordinator.build() to ensure the success or failure status is always delivered.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex produced a proof for the posted P1 finding and linked it to the associated review comment.
  • The focused test spawned the daemon and multiprocessing forkserver but timed out waiting for dimos run --daemon after 120 seconds, so launcher success and an external CoordinatorRPC pong were not observed.
  • Artifacts capturing the fallback test dependency failure, focused daemon Zenoh timeout, virtualenv environment check, and daemon test cleanup were collected to support the proofs.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (1)

  1. General comment

    P1 Daemon launcher never returns after spawning Zenoh daemon

    • Bug
      • The focused real-subprocess test timed out while waiting for dimos run demo-mcp-stress-test --daemon --disable mcp-server --viewer none --n-workers 1, so it could not advance to the independently spawned Zenoh CoordinatorRPC ping.
    • Cause
      • The launcher process remained alive after spawning the daemon child and multiprocessing forkserver; the E2E trace establishes the unmet completion contract but does not isolate the internal wait or inherited-descriptor source.
    • Fix
      • Ensure the parent CLI process closes or redirects descriptors inherited by the forkserver/daemon and exits after daemon readiness is established, then retain this external-ping regression test.

    T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "fix: zenoh forking issue" | Re-trigger Greptile

Comment thread dimos/cli/dimos.py
@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Aug 20, 2026
@paul-nechifor
paul-nechifor added this pull request to the merge queue Aug 20, 2026
Merged via the queue into main with commit 5dbbb3f Aug 20, 2026
34 of 35 checks passed
@paul-nechifor
paul-nechifor deleted the paul/fix/zenoh-fork-issue branch August 20, 2026 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Required CI checks have passed on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--daemon + Zenoh: Coordinator RPC service unreachable after daemonize (Dimos.connect / out-of-process clients time out)

3 participants