feat: improve local service reliability and test isolation#349
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesService supervision is centralized in ChangesService supervision
Test environment isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant run_services
participant _ServiceSupervisor
participant ServiceProcess
participant Pidfile
run_services->>_ServiceSupervisor: initialize and start services
_ServiceSupervisor->>ServiceProcess: poll child processes
ServiceProcess-->>_ServiceSupervisor: return exit code
_ServiceSupervisor->>_ServiceSupervisor: classify exit and update failure state
_ServiceSupervisor->>Pidfile: refresh active service PIDs
_ServiceSupervisor->>ServiceProcess: respawn eligible service
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
26ddace to
be0223a
Compare
be0223a to
e621f64
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
reflexio/cli/utils.py (2)
259-269: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring doesn't mention the new respawn/degradation behavior.
Worth a brief mention of the bounded-respawn policy (healthy window, rapid-failure limit, degraded marking) for future maintainers, since this materially changes
run_services's runtime behavior.🤖 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 `@reflexio/cli/utils.py` around lines 259 - 269, Update the run_services docstring to briefly document its bounded-respawn and degradation behavior, including the healthy-window reset, rapid-failure limit, and marking services degraded after the limit is reached. Keep the existing lifecycle and callback documentation intact.
374-390: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRespawn delay blocks the monitor loop for other services.
time.sleep(_SERVICE_RESPAWN_DELAY_SECS)runs on the main monitoring thread insidehandle_service_exit, so while one service is in its 2s respawn delay,proc.poll()isn't being checked for siblings until the next outer-loop pass. Bounded to 2s and mostly relevant only when multiple services fail near-simultaneously, so low real-world impact given the current design.🤖 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 `@reflexio/cli/utils.py` around lines 374 - 390, The respawn delay in handle_service_exit blocks monitoring of sibling services. Move the delayed restart out of the main monitoring thread, such as by scheduling it asynchronously, while preserving the shutting_down check and existing respawn/error handling around start_service and write_current_pidfile.tests/cli/test_utils.py (1)
108-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing pidfile-content assertion for respawn refresh.
This test confirms two processes get started but never checks the actual pidfile contents, despite "refresh the pidfile after successful respawn" being an explicit PR objective. Consider asserting
utils.read_pidfile(pidfile)reflects the new PID after respawn.🤖 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 `@tests/cli/test_utils.py` around lines 108 - 133, Update test_run_services_respawns_unexpected_exit to retain the pidfile path used by _patch_run_services_environment and assert via utils.read_pidfile that the pidfile contains the respawned process’s PID after run_services completes, while preserving the existing process-count and distinct-PID assertions.
🤖 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 `@reflexio/cli/utils.py`:
- Around line 282-296: Handle OSError and RuntimeError from subprocess.Popen
within the initial-start flow of run_services, ensuring a failed service does
not abort startup or leave previously started sibling processes unsupervised.
Reuse the existing cleanup and failure-handling behavior used by
handle_service_exit, and preserve the current respawn behavior while updating
the start_service/run_services interaction accordingly.
- Around line 402-413: The run_services startup flow blocks in
_wait_for_all_ready before monitoring child processes, delaying exit handling
and respawn. Update run_services and the readiness-monitoring logic so
process.poll() results are checked while waiting for readiness, and ensure the
on_all_ready callback still runs once when all services become ready; add
coverage in tests/cli/test_utils.py for this startup exit and callback path.
---
Nitpick comments:
In `@reflexio/cli/utils.py`:
- Around line 259-269: Update the run_services docstring to briefly document its
bounded-respawn and degradation behavior, including the healthy-window reset,
rapid-failure limit, and marking services degraded after the limit is reached.
Keep the existing lifecycle and callback documentation intact.
- Around line 374-390: The respawn delay in handle_service_exit blocks
monitoring of sibling services. Move the delayed restart out of the main
monitoring thread, such as by scheduling it asynchronously, while preserving the
shutting_down check and existing respawn/error handling around start_service and
write_current_pidfile.
In `@tests/cli/test_utils.py`:
- Around line 108-133: Update test_run_services_respawns_unexpected_exit to
retain the pidfile path used by _patch_run_services_environment and assert via
utils.read_pidfile that the pidfile contains the respawned process’s PID after
run_services completes, while preserving the existing process-count and
distinct-PID assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e93d6fa9-4bd1-4318-b47f-8ed0e3fb1a92
📒 Files selected for processing (2)
reflexio/cli/utils.pytests/cli/test_utils.py
Keep default test storage and debug logs in a temporary directory while preserving provider-backed test behavior.
wenchanghan
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/server/services/storage/test_storage_defaults.py (1)
33-35: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent
.envfrom overriding the isolated test environment during reload.When
server_moduleis reloaded, its module-level code executesload_reflexio_env(). Becausepatch.dict(..., clear=True)temporarily removesLOCAL_STORAGE_PATHfrom the environment,load_dotenvinsideload_reflexio_envwill read and re-inject it if the developer hasLOCAL_STORAGE_PATHdefined in their real~/.reflexio/.envfile. This causes the assertion on line 35 to fail locally for those developers.Patch
load_reflexio_envto be a no-op during the reload to maintain strict isolation.🛠️ Proposed fix
try: with patch.dict(os.environ, env, clear=True): - reloaded = importlib.reload(server_module) - assert expected == reloaded.LOCAL_STORAGE_PATH + with patch("reflexio.server.load_reflexio_env"): + reloaded = importlib.reload(server_module) + assert expected == reloaded.LOCAL_STORAGE_PATH🤖 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 `@tests/server/services/storage/test_storage_defaults.py` around lines 33 - 35, Patch server_module.load_reflexio_env as a no-op within the patch.dict reload context, before calling importlib.reload(server_module), so module initialization cannot re-read the developer’s .env file. Keep the isolated environment and existing LOCAL_STORAGE_PATH assertion unchanged.tests/cli/test_utils.py (1)
44-58: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winTest isolation broken by unmocked
get_stop_request_path.Writing test artifacts to a global, shared directory (
tempfile.gettempdir()) breaks test isolation and can cause flakiness or state leakage, especially when parallelized withpytest-xdist(as the tests use the same hardcoded ports). The shared root cause is failing to mockget_stop_request_pathto use the isolatedtmp_path.
tests/cli/test_utils.py#L44-L58: In_patch_run_services_environment, patchget_stop_request_pathto return an isolated path.tests/cli/test_utils.py#L249-L274: Intest_stop_services_writes_intent_before_killing_saved_pids, pointstop_request_pathtotmp_pathand apply the same monkeypatch before the mock supervisor runs.🛠️ Proposed fixes
For
_patch_run_services_environment:monkeypatch.setattr(utils, "ensure_requested_ports_available", lambda _ports: None) monkeypatch.setattr(utils, "get_pidfile_path", lambda _ports: pidfile) + monkeypatch.setattr( + utils, + "get_stop_request_path", + lambda name, port: tmp_path / f"{name}_{port}.stop", + ) monkeypatch.setattr(utils.signal, "signal", lambda *_args: None)For
test_stop_services_writes_intent_before_killing_saved_pids:pidfile.write_text('{"backend": 1234}') stop_requests: list[list[int]] = [] - stop_request_path = utils.get_stop_request_path("backend", 8071) + stop_request_path = tmp_path / "backend_8071.stop" monkeypatch.setattr(utils, "get_pidfile_path", lambda _ports: pidfile) + monkeypatch.setattr( + utils, + "get_stop_request_path", + lambda name, port: stop_request_path, + ) monkeypatch.setattr(utils, "find_pids_on_port", lambda _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 `@tests/cli/test_utils.py` around lines 44 - 58, Update tests/cli/test_utils.py lines 44-58 in _patch_run_services_environment to monkeypatch utils.get_stop_request_path so it returns an isolated path under tmp_path. Also update tests/cli/test_utils.py lines 249-274 in test_stop_services_writes_intent_before_killing_saved_pids to set stop_request_path under tmp_path and apply the same monkeypatch before starting the mock supervisor.
🧹 Nitpick comments (1)
tests/cli/test_utils.py (1)
51-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a nested function instead of
__setitem__for advancing the mock clock.While mutating
clockvia__setitem__inside alambdafunctionally bypasses Python's scope restrictions, it is a less readable idiom. Using a standard nested helper function makes the intent explicitly clear.♻️ Proposed refactor
clock = [0.0] monkeypatch.setattr(utils.time, "monotonic", lambda: clock[0]) + + def advance_time(seconds: float) -> None: + clock[0] += seconds + monkeypatch.setattr( utils.time, "sleep", - lambda seconds: clock.__setitem__(0, clock[0] + seconds), + advance_time, )🤖 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 `@tests/cli/test_utils.py` around lines 51 - 57, Replace the lambda passed to utils.time.sleep in the mock clock setup with a named nested helper function that advances clock[0] by the requested seconds, then monkeypatch sleep with that helper while preserving the existing monotonic 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 `@tests/conftest.py`:
- Line 50: Update the test environment setup in tests/conftest.py to set
REFLEXIO_LOG_DIR to a path under _REFLEXIO_TEST_HOME, alongside
LOCAL_STORAGE_PATH. Ensure server-import file handlers resolve logs within the
isolated test directory rather than the developer’s real home directory.
---
Outside diff comments:
In `@tests/cli/test_utils.py`:
- Around line 44-58: Update tests/cli/test_utils.py lines 44-58 in
_patch_run_services_environment to monkeypatch utils.get_stop_request_path so it
returns an isolated path under tmp_path. Also update tests/cli/test_utils.py
lines 249-274 in test_stop_services_writes_intent_before_killing_saved_pids to
set stop_request_path under tmp_path and apply the same monkeypatch before
starting the mock supervisor.
In `@tests/server/services/storage/test_storage_defaults.py`:
- Around line 33-35: Patch server_module.load_reflexio_env as a no-op within the
patch.dict reload context, before calling importlib.reload(server_module), so
module initialization cannot re-read the developer’s .env file. Keep the
isolated environment and existing LOCAL_STORAGE_PATH assertion unchanged.
---
Nitpick comments:
In `@tests/cli/test_utils.py`:
- Around line 51-57: Replace the lambda passed to utils.time.sleep in the mock
clock setup with a named nested helper function that advances clock[0] by the
requested seconds, then monkeypatch sleep with that helper while preserving the
existing monotonic 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 902833b7-f5b4-451f-a13f-0baa553fe58c
📒 Files selected for processing (5)
developer.mdreflexio/cli/utils.pytests/cli/test_utils.pytests/conftest.pytests/server/services/storage/test_storage_defaults.py
🚧 Files skipped from review as they are similar to previous changes (1)
- reflexio/cli/utils.py
Keep stop markers and environment reloads inside the test sandbox.
2 participants
Summary
Improve local service reliability and keep the test runtime isolated from developer data.
Before
~/.reflexiodirectory.After
Behavior
Validation
uv run ruff check reflexio/cli/utils.py tests/cli/test_utils.pyuv run pyright reflexio/cli/utils.py tests/cli/test_utils.pyuv run pytest tests/cli tests/server/api_endpoints/test_learning_status.py tests/server/services/storage/test_storage_defaults.py -o 'addopts='— 347 passedDEBUG_LOG_TO_CONSOLE=true uv run pytest tests/server/api_endpoints/test_learning_status.py -o 'addopts='— 9 passed; production log mtime unchangeduv run pytest -o 'addopts='— 4,835 passed, 124 skipped, 10 failures because the local embedding daemon was unavailable at127.0.0.1:8072; 2 warnings