perf(wifi): one status fetch per monitor tick instead of three - #411
Conversation
The wifi monitor daemon fetched WiFi status + ethernet state before its AP-mode check, the check internally fetched the same state again (with retry), and the daemon fetched a third time afterwards — each fetch is several nmcli subprocess forks, every 30s, forever, even on a perfectly healthy link. check_and_manage_ap_mode's decision logic is extracted to _manage_ap_mode(status, ethernet, ap_active); the new check_and_manage_ap_mode_with_state() runs the single (retrying) fetch battery and returns (changed, status, ethernet, ap_active_after) — the post-state is derivable because state only ever flips via one enable or one disable. The original bool-returning method delegates, so existing callers are untouched. The daemon's dead pre-fetch is removed and its post-check reads use the returned state; the AP-enable retry semantics (_get_wifi_status_with_retry) are preserved exactly. ~10-15 forks/30s drops to ~4-6. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam
|
Warning Review limit reached
Next review available in: 15 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWiFiManager now performs one combined WiFi, Ethernet, and AP-state check, returns the resulting state, and retains a boolean compatibility wrapper. The monitoring daemon uses the returned AP state for snapshots, logging, and recovery decisions. New tests cover state handling and exceptions. ChangesWiFi/AP state coordination
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant wifi_monitor_daemon
participant WiFiManager
participant NetworkState
wifi_monitor_daemon->>WiFiManager: check_and_manage_ap_mode_with_state()
WiFiManager->>NetworkState: Fetch WiFi, Ethernet, and AP state
NetworkState-->>WiFiManager: Return connectivity snapshot
WiFiManager->>WiFiManager: Manage AP mode from snapshot
WiFiManager-->>wifi_monitor_daemon: Return changed, status, Ethernet, and AP state
wifi_monitor_daemon->>wifi_monitor_daemon: Update logs and recovery conditions
Possibly related PRs
🚥 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/wifi_manager.py`:
- Around line 2570-2578: Update check_and_manage_ap_mode_with_state to add an
explicit return type hint describing its four-value return tuple: state-changed
status, WiFiStatus, Ethernet connection status, and post-check AP activity.
- Around line 2593-2594: Add type annotations to the parameters of
_manage_ap_mode, while preserving its existing bool return annotation and
behavior. Use the appropriate project-consistent types for status,
ethernet_connected, and ap_active.
In `@test/test_wifi_check_state.py`:
- Line 64: In test/test_wifi_check_state.py, update the unpacking assignments
from check_and_manage_ap_mode_with_state: rename status and ethernet to _status
and _ethernet at lines 64 and 73, and rename ethernet and ap_after to _ethernet
and _ap_after at line 86; leave the used variables 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03881246-c341-4c50-8977-ebca451f77ee
📒 Files selected for processing (3)
scripts/utils/wifi_monitor_daemon.pysrc/wifi_manager.pytest/test_wifi_check_state.py
…d-var lint in tests - check_and_manage_ap_mode_with_state now declares its Tuple[bool, WiFiStatus, bool, bool] return type instead of being untyped. - _manage_ap_mode's status/ethernet_connected/ap_active parameters are now typed (WiFiStatus, bool, bool), matching its existing -> bool return hint. - test_wifi_check_state.py: rename unused unpacked variables (status/ ethernet/ap_after) to underscore-prefixed names at the three call sites that don't use them, leaving the used ones untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
1 participant
Summary
PR 7 of the performance series. Each 30s wifi-monitor tick spawned ~10–15
nmclisubprocesses on a healthy link: the daemon pre-fetched status+ethernet (results never used — dead code),check_and_manage_ap_mode()re-fetched internally, and the daemon fetched a third time for its state logging._manage_ap_mode(status, ethernet, ap_active); newcheck_and_manage_ap_mode_with_state()performs the single fetch battery — keeping the retrying_get_wifi_status_with_retrythe AP-enable decision depends on — and returns(changed, status, ethernet, ap_active_after). Post-state is derivable (state only flips via one enable or one disable per call).Verification
6 new tests (single fetch per call, observed-state passthrough, ap_after inversion on both enable and disable, bool wrapper back-compat, exception path never raises); full wifi suites 14/14 green. AP failover behavior is logic-identical by construction (same decision function, same inputs); a physical unplug test on real hardware is the remaining spot check since the devpi is reached over the network being tested.
🤖 Generated with Claude Code
https://claude.ai/code/session_01FqzC1nzTWL4kaqgMaQZFam
Summary by CodeRabbit
Bug Fixes
Tests