Skip to content

fix: double start bug#2589

Merged
paul-nechifor merged 1 commit into
mainfrom
paul/fix/double-start-bug
Jun 25, 2026
Merged

fix: double start bug#2589
paul-nechifor merged 1 commit into
mainfrom
paul/fix/double-start-bug

Conversation

@paul-nechifor

Copy link
Copy Markdown
Contributor

Problem

In certain tests (test_dimos_skills) lcm.start could be called twice.

Solution

Make LCMService.start idempotent.

@greptile-apps

greptile-apps Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes LCMService.start() idempotent by adding a dedicated _start_lock and an early-return guard (if self._thread is not None and self._thread.is_alive(): return). The stop() method is also brought under _start_lock and now explicitly sets self._thread = None after joining, enabling the guard to distinguish a stopped thread from a running one.

  • _start_lock is correctly initialized in __init__, __setstate__, and excluded from __getstate__ — the full lifecycle is covered.
  • stop() now sets self._thread = None only on the non-self-join path; when the LCM thread calls stop() on itself, the thread reference is left set but the is_alive() check in start() still allows a future restart, so behavior is correct in both paths.
  • The test is updated to assert service._thread is None (matching the new cleanup) rather than not service._thread.is_alive().

Confidence Score: 5/5

Safe to merge — the change is narrowly scoped to making start() idempotent and does not alter any message-handling or subscription logic.

The lock is correctly initialized in all three state-management paths (init, setstate, getstate), the early-return guard is sound, and stop() reliably nulls the thread reference on the join path. No existing behavior is changed for the normal start-then-stop lifecycle.

No files require special attention.

Important Files Changed

Filename Overview
dimos/protocol/service/lcmservice.py Adds _start_lock to make start() idempotent and brings stop() under the same lock; thread reference is now nulled after join for clean state.
dimos/protocol/service/test_lcmservice.py Single assertion updated to match new stop() behavior (thread set to None rather than just dead).

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant LCMService
    participant _start_lock
    participant LCMThread

    Note over Caller,LCMThread: Idempotent start() - second call is a no-op

    Caller->>LCMService: start()
    LCMService->>_start_lock: acquire
    LCMService->>LCMService: _thread alive? No
    LCMService->>LCMThread: create and start thread
    LCMThread-->>LCMService: _loop_running.set()
    LCMService->>_start_lock: release

    Caller->>LCMService: start() again
    LCMService->>_start_lock: acquire
    LCMService->>LCMService: _thread alive? Yes - return
    LCMService->>_start_lock: release

    Caller->>LCMService: stop()
    LCMService->>_start_lock: acquire
    LCMService->>LCMThread: _stop_event.set()
    LCMService->>LCMThread: join(timeout)
    LCMThread-->>LCMService: thread exits
    LCMService->>LCMService: "_thread = None"
    LCMService->>_start_lock: release
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant LCMService
    participant _start_lock
    participant LCMThread

    Note over Caller,LCMThread: Idempotent start() - second call is a no-op

    Caller->>LCMService: start()
    LCMService->>_start_lock: acquire
    LCMService->>LCMService: _thread alive? No
    LCMService->>LCMThread: create and start thread
    LCMThread-->>LCMService: _loop_running.set()
    LCMService->>_start_lock: release

    Caller->>LCMService: start() again
    LCMService->>_start_lock: acquire
    LCMService->>LCMService: _thread alive? Yes - return
    LCMService->>_start_lock: release

    Caller->>LCMService: stop()
    LCMService->>_start_lock: acquire
    LCMService->>LCMThread: _stop_event.set()
    LCMService->>LCMThread: join(timeout)
    LCMThread-->>LCMService: thread exits
    LCMService->>LCMService: "_thread = None"
    LCMService->>_start_lock: release
Loading

Reviews (4): Last reviewed commit: "fix: double start bug" | Re-trigger Greptile

Comment thread dimos/protocol/service/lcmservice.py Outdated
@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

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

Files with missing lines Patch % Lines
dimos/protocol/service/lcmservice.py 75.60% 5 Missing and 5 partials ⚠️
@@            Coverage Diff             @@
##             main    #2589      +/-   ##
==========================================
+ Coverage   69.61%   70.88%   +1.27%     
==========================================
  Files         878      866      -12     
  Lines       79326    77251    -2075     
  Branches     7126     6867     -259     
==========================================
- Hits        55220    54759     -461     
+ Misses      22301    20691    -1610     
+ Partials     1805     1801       -4     
Flag Coverage Δ
OS-ubuntu-24.04-arm 62.99% <71.42%> (+<0.01%) ⬆️
OS-ubuntu-latest 65.82% <71.42%> (-0.01%) ⬇️
Py-3.10 65.82% <71.42%> (+<0.01%) ⬆️
Py-3.11 65.82% <71.42%> (+<0.01%) ⬆️
Py-3.12 65.82% <71.42%> (-0.01%) ⬇️
Py-3.13 65.82% <71.42%> (+<0.01%) ⬆️
Py-3.14 65.83% <71.42%> (-0.01%) ⬇️
Py-3.14t 65.82% <71.42%> (-0.01%) ⬇️
SelfHosted-Large 30.11% <45.23%> (+0.01%) ⬆️
SelfHosted-Linux 37.80% <42.85%> (+<0.01%) ⬆️
SelfHosted-macOS 36.59% <42.85%> (+<0.01%) ⬆️

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

Files with missing lines Coverage Δ
dimos/protocol/service/test_lcmservice.py 100.00% <100.00%> (ø)
dimos/protocol/service/lcmservice.py 88.18% <75.60%> (-3.34%) ⬇️

... and 13 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 Jun 24, 2026
@paul-nechifor paul-nechifor force-pushed the paul/fix/double-start-bug branch from ebe1601 to 3f3752a Compare June 24, 2026 23:11
@github-actions github-actions Bot added ready-to-merge Required CI checks have passed on this PR and removed ready-to-merge Required CI checks have passed on this PR labels Jun 24, 2026
Comment thread dimos/protocol/service/lcmservice.py
@paul-nechifor paul-nechifor force-pushed the paul/fix/double-start-bug branch from 3f3752a to f9d4fe4 Compare June 25, 2026 00:38
@github-actions github-actions Bot removed the ready-to-merge Required CI checks have passed on this PR label Jun 25, 2026
Comment thread dimos/protocol/service/lcmservice.py Outdated
@paul-nechifor paul-nechifor force-pushed the paul/fix/double-start-bug branch from f9d4fe4 to d24a035 Compare June 25, 2026 01:41
@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Jun 25, 2026
@paul-nechifor paul-nechifor merged commit 9e5f371 into main Jun 25, 2026
27 checks passed
@paul-nechifor paul-nechifor deleted the paul/fix/double-start-bug branch June 25, 2026 02:37
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.

3 participants