You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Coverage uses per-process continuous profraw files and Linux counter-relocation flags for profile persistence.
WASM result handling
Layer / File(s)
Summary
Shared ignored-result helper libs/web/src/lib.rs
Adds the public generic ignore helper for intentionally discarded values.
Browser module result handling services/ws-modules/*/src/lib.rs
Replaces explicit result drops around browser callbacks, timers, lifecycle operations, rendering, and status updates with et_web::ignore(...) or ignored bindings.
WASI polling result handling services/ws-modules/wasi-*/src/lib.rs
Stores polling results in throwaway locals without changing sleep behavior.
Runner shutdown coordination
Layer / File(s)
Summary
Shutdown signal propagation services/ws-pyo3-runner/src/agent.rs
Passes a Notify shutdown signal into the WebSocket drive loop, which exits when notified.
Watcher and runner orchestration services/ws-pyo3-runner/src/main.rs
Coordinates Ctrl-C and RUNNER_TIMEOUT through a watcher task and shared shutdown notifier.
We reviewed changes in f469545...97f1434 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces graceful shutdown mechanisms and improved 'must_use' handling, but several issues must be addressed before merging. A high-priority fix is required for the non-standard usage of 'coreutils' in the coverage configuration, which will likely break CI environments. While the PR is titled 'Increase coverage', the provided coverage report is empty, and a required test scenario for LLVM continuous mode coverage persistence is missing. Additionally, the improvement in coverage is offset by the introduction of significant code duplication across six browser modules, which should be centralized to avoid increasing technical debt.
About this PR
The coverage report provided is empty. Please ensure that coverage data is correctly generated and attached so that the quantitative impact of these changes can be verified.
Please provide a detailed PR description. The current lack of context makes it difficult to verify if the implementation fully aligns with the intended architectural changes for coverage collection and runner lifecycle.
There is a systemic inconsistency in how 'must_use' results are handled. While most modules were updated to use 'et_web::ignore', 'comm1' still uses 'let _status' in several places. Please standardize this across all modules modified in this PR.
3 comments outside of the diffservices/ws-modules/audio1/src/lib.rs
line 163 🟡 MEDIUM RISK
This module uses the same boilerplate logic for logging, error description, and WebSocket connection as 5 other modules. Consider centralizing these common browser utilities into the 'et-web' library to reduce technical debt and maintenance overhead.
services/ws-modules/wasi-comm1/src/lib.rs
line 133 ⚪ LOW RISK
Suggestion: The 'wait_for_agent_id' and 'sleep_ms' functions are duplicated between the WASI modules. These should be moved to a common library to avoid diverging logic.
services/ws-pyo3-runner/src/agent.rs
line 171 🟡 MEDIUM RISK
Suggestion: Replace 'storage_task.abort();' with 'let _ = storage_task.await;' to ensure the storage worker processes any final operations from the Python 'on_shutdown' hook before the process exits.
Test suggestions
Verify pyo3-runner graceful shutdown on timeout triggers teardown and clean exit
Verify project root resolution from the CARGO_MANIFEST_DIR environment
Verify recipient acknowledgement notifies the sender with MessageDeliveryStatus::Acknowledged
Verify coverage data persists for SIGKILLed processes using LLVM continuous mode
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify coverage data persists for SIGKILLed processes using LLVM continuous mode
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
Invoke the utilities directly (e.g., 'dirname', 'uname') instead of prefixing them with 'coreutils' to ensure compatibility across standard shell environments.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The call to 'serde_json::from_str' currently has no effect as the result is discarded. Remove this call if validation is not needed, or update it to log a warning (e.g., using 'tracing::warn!') if parsing fails to avoid silent diagnostic gaps.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: Use the newly introduced 'et_web::ignore' utility here instead of 'let _status'. This ensures consistency with other browser-based WASM modules updated in this PR and adheres to the project's workspace-wide linting strategy for 'must_use' results.
Race connect_and_register() with shutdown run() still awaits connect_and_register(...) before drive(), so Ctrl-C/RUNNER_TIMEOUT is ignored during startup. This is bounded by the default WS_CONNECT_ACK_TIMEOUT, but if that timeout is disabled (none/off/disabled) the process can stay stuck in connect/register until the server answers. Select on shutdown.notified() here too and tear down the partially initialized worker/socket on the shutdown branch.
🤖 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 `@services/ws-pyo3-runner/src/agent.rs` around lines 119 - 161, Race the
startup connect_and_register operation in run against shutdown.notified() so
shutdown is honored even when the connection acknowledgment timeout is disabled.
On shutdown, tear down the partially initialized Python worker and storage task
before returning the appropriate shutdown result; preserve the existing
registration, agent-id initialization, and drive flow when connection succeeds.
🧹 Nitpick comments (1)
services/ws-pyo3-runner/src/main.rs (1)
50-66: 🧹 Nitpick | 🔵 Trivial
No upper bound on teardown once shutdown is requested.
Once shutdown.notify_one() fires, drive() returns promptly, but run()'s subsequent teardown (worker.join(), awaiting the WS close send, the Python on_shutdown hook) has no timeout of its own. RUNNER_TIMEOUT now only guarantees a prompt start of teardown, not a bounded total exit time — an unresponsive Python on_shutdown hook or a slow socket close could hang the process past the configured timeout. Worth considering a secondary bound (e.g. wrapping run_agent(...) itself in an outer tokio::time::timeout that force-exits if teardown doesn't complete promptly) as a safety net.
🤖 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 `@services/ws-pyo3-runner/src/main.rs` around lines 50 - 66, Bound the entire
run lifecycle, including teardown after shutdown notification, rather than
applying RUNNER_TIMEOUT only to drive(). Update the run/run_agent flow around
the shutdown watcher so the configured timeout also limits worker.join(), the
WebSocket close, and the Python on_shutdown hook; force the existing
shutdown/exit behavior when the outer timeout expires, while preserving normal
completion and timeout-triggered shutdown handling.
🤖 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 `@libs/path/tests/find.rs`:
- Around line 11-15: Strengthen the assertion in the
find_project_root_from_manifest test to verify the resolved path is the
repository root, not merely an existing directory. Assert that
root.join(".dprint.jsonc").is_file() (or compare root with an independently
determined expected root), while retaining the existing directory validation if
useful.
In `@services/ws-pyo3-runner/src/main.rs`:
- Around line 46-67: Handle the io::Result returned by tokio::signal::ctrl_c()
in the shutdown watcher instead of treating every completion as an interrupt. In
the ctrl_c branch, log and propagate signal-handler installation errors so
run_agent does not perform an unintended shutdown, while preserving the existing
shutdown notification for a successful interrupt and the timeout path.
---
Outside diff comments:
In `@services/ws-pyo3-runner/src/agent.rs`:
- Around line 119-161: Race the startup connect_and_register operation in run
against shutdown.notified() so shutdown is honored even when the connection
acknowledgment timeout is disabled. On shutdown, tear down the partially
initialized Python worker and storage task before returning the appropriate
shutdown result; preserve the existing registration, agent-id initialization,
and drive flow when connection succeeds.
---
Nitpick comments:
In `@services/ws-pyo3-runner/src/main.rs`:
- Around line 50-66: Bound the entire run lifecycle, including teardown after
shutdown notification, rather than applying RUNNER_TIMEOUT only to drive().
Update the run/run_agent flow around the shutdown watcher so the configured
timeout also limits worker.join(), the WebSocket close, and the Python
on_shutdown hook; force the existing shutdown/exit behavior when the outer
timeout expires, while preserving normal completion and timeout-triggered
shutdown handling.
🪄 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 Plus
Run ID: a358c962-0480-4dee-95f2-a15bb0a08495
📥 Commits
Reviewing files that changed from the base of the PR and between f469545 and 97f1434.
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the resolved path is the project root, not merely a directory.
root.is_dir() does not verify the helper walked to the repository root; returning any existing manifest directory would still pass. Assert that root.join(".dprint.jsonc").is_file() (or compare against an independently determined expected root) to exercise the actual contract.
🤖 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 `@libs/path/tests/find.rs` around lines 11 - 15, Strengthen the assertion in
the find_project_root_from_manifest test to verify the resolved path is the
repository root, not merely an existing directory. Assert that
root.join(".dprint.jsonc").is_file() (or compare root with an independently
determined expected root), while retaining the existing directory validation if
useful.
ctrl_c() errors are silently treated as a real interrupt.
_ = tokio::signal::ctrl_c() => info!("interrupted; shutting down") discards the io::Result<()>. If installing the signal handler fails (rare, but possible, e.g. handler-limit exhaustion), the branch still resolves and this arm unconditionally logs "interrupted" and calls shutdown.notify_one(), causing an unintended shutdown rather than surfacing the real failure.
🛠️ Proposed fix to distinguish signal-install failure from a real interrupt
- _ = tokio::signal::ctrl_c() => info!("interrupted; shutting down"),+ result = tokio::signal::ctrl_c() => match result {+ Ok(()) => info!("interrupted; shutting down"),+ Err(err) => {+ warn!("failed to listen for ctrl_c: {err}");+ return;+ }+ },
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
// Graceful shutdown: ctrl_c or the optional RUNNER_TIMEOUT trips `shutdown`, which `run_agent`'s `drive`
// loop selects on. That returns the run loop so `run_agent` still executes its teardown (queue on_shutdown,
// join the Python worker, close the socket) instead of the run future being dropped mid-flight. The watcher
// runs concurrently and is abandoned when `run_agent` returns and the process exits.
let shutdown = std::sync::Arc::new(tokio::sync::Notify::new());
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/ws-pyo3-runner/src/main.rs` around lines 46 - 67, Handle the
io::Result returned by tokio::signal::ctrl_c() in the shutdown watcher instead
of treating every completion as an interrupt. In the ctrl_c branch, log and
propagate signal-handler installation errors so run_agent does not perform an
unintended shutdown, while preserving the existing shutdown notification for a
successful interrupt and the timeout path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary by CodeRabbit