fix: guard against stale EXECUTING schedules from Netro cloud#53
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughVersion increment and enhanced schedule processing logic to detect and ignore stale "EXECUTING" schedules that have already completed. Added validation checks comparing schedule end times against current time, with comprehensive test coverage for both SprinklerHandler and ZoneHandler flows across API versions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Netro Sprinklers.indigoPlugin/Contents/Server Plugin/device_handlers.py (2)
237-245: Dead ternary — both branches returnfloat(raw_value).The V1 branch collapses to the same expression on both sides of the conditional:
return float(raw_value) if isinstance(raw_value, str) else float(raw_value)This is a no-op
isinstancecheck — likely a leftover from when the string path did something different. Simplify:♻️ Proposed fix
- else: - # V1: Millisecond timestamp (may be string) - return float(raw_value) if isinstance(raw_value, str) else float(raw_value) + else: + # V1: Millisecond timestamp (may be string) + return float(raw_value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Netro` Sprinklers.indigoPlugin/Contents/Server Plugin/device_handlers.py around lines 237 - 245, The V1 branch contains a dead ternary that returns float(raw_value) in both cases; replace the conditional expression with a single return float(raw_value) to simplify the timestamp parsing logic (leave the surrounding try/except and the debug log referencing raw_value and api_version unchanged). Locate the timestamp parsing code that checks api version V1 and uses raw_value (the line currently: return float(raw_value) if isinstance(raw_value, str) else float(raw_value)) and change it to a single return of float(raw_value).
178-181: Duplicate debug-log string across SprinklerHandler and ZoneHandler.The same message —
"Ignoring stale EXECUTING schedule id=%s zone=%s end_time=%s"— is emitted verbatim from bothSprinklerHandler.process_schedules(lines 178-181) andZoneHandler.process_zone_schedules(lines 673-676). Since_schedule_actually_executingis the single source of truth for the decision, a small helper (or having_schedule_actually_executingitself emit the debug line at decision time) would remove the duplication and keep the log format consistent if it ever changes.Also applies to: 673-676
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Netro` Sprinklers.indigoPlugin/Contents/Server Plugin/device_handlers.py around lines 178 - 181, Both SprinklerHandler.process_schedules and ZoneHandler.process_zone_schedules duplicate the same debug message when a schedule is considered stale; centralize that logging by moving the debug emit into the single decision point _schedule_actually_executing (or create a small helper called e.g. log_stale_executing_schedule called by both callers). Update _schedule_actually_executing (or the new helper) to accept the schedule dict and responsible zone/sprinkler id and emit the "Ignoring stale EXECUTING schedule id=%s zone=%s end_time=%s" debug line there, then remove the duplicate debug calls from process_schedules and process_zone_schedules so formatting and semantics remain consistent.tests/conftest.py (1)
163-165: Fragile fixture dating — prefer monkeypatchingtime.time()over year 2099.Moving
id=100’s timestamps to2099-04-07is a pragmatic way to keeptest_v2_schedule_iso_timestamp_parsing/test_v2_source_valuesexercising the “genuine EXECUTING” path without a monkeypatch, but it couples a broadly used shared fixture to the real wall clock. The tests that explicitly validate time-boundary behavior (e.g.test_schedule_actually_executing_time_boundary) already demonstrate the cleaner pattern —monkeypatch.setattr("device_handlers.time.time", ...)— which eliminates time coupling entirely.Optional: restore realistic dates in the fixture and monkeypatch
device_handlers.time.timeinside the two tests that depend on the EXECUTING entry being “live”. Tests would then be insensitive to the system clock and future changes to tz handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 163 - 165, The shared fixture in tests/conftest.py hardcodes entry id=100 timestamps to year 2099 which couples tests to the real clock; revert those timestamps to realistic values and instead update the two tests that rely on the entry being "EXECUTING" (test_v2_schedule_iso_timestamp_parsing and test_v2_source_values) to monkeypatch device_handlers.time.time to a fixed epoch that makes id=100 fall within the executing window (use the pattern shown in test_schedule_actually_executing_time_boundary via monkeypatch.setattr("device_handlers.time.time", ...)); keep the fixture reusable for other tests and only manipulate time inside the specific tests that need the live EXECUTING state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Netro` Sprinklers.indigoPlugin/Contents/Info.plist:
- Line 6: The PluginVersion in Info.plist is set to an already-existing tag
(2026.4.5) causing CI version-check failures; update the <string> value for
PluginVersion in Netro Sprinklers.indigoPlugin/Contents/Info.plist to the next
patch release (e.g., 2026.4.6) so the release workflow can create the new
tag—locate the PluginVersion entry (the <string> value currently "2026.4.5") and
change it to the bumped version, then commit the change.
---
Nitpick comments:
In `@Netro` Sprinklers.indigoPlugin/Contents/Server Plugin/device_handlers.py:
- Around line 237-245: The V1 branch contains a dead ternary that returns
float(raw_value) in both cases; replace the conditional expression with a single
return float(raw_value) to simplify the timestamp parsing logic (leave the
surrounding try/except and the debug log referencing raw_value and api_version
unchanged). Locate the timestamp parsing code that checks api version V1 and
uses raw_value (the line currently: return float(raw_value) if
isinstance(raw_value, str) else float(raw_value)) and change it to a single
return of float(raw_value).
- Around line 178-181: Both SprinklerHandler.process_schedules and
ZoneHandler.process_zone_schedules duplicate the same debug message when a
schedule is considered stale; centralize that logging by moving the debug emit
into the single decision point _schedule_actually_executing (or create a small
helper called e.g. log_stale_executing_schedule called by both callers). Update
_schedule_actually_executing (or the new helper) to accept the schedule dict and
responsible zone/sprinkler id and emit the "Ignoring stale EXECUTING schedule
id=%s zone=%s end_time=%s" debug line there, then remove the duplicate debug
calls from process_schedules and process_zone_schedules so formatting and
semantics remain consistent.
In `@tests/conftest.py`:
- Around line 163-165: The shared fixture in tests/conftest.py hardcodes entry
id=100 timestamps to year 2099 which couples tests to the real clock; revert
those timestamps to realistic values and instead update the two tests that rely
on the entry being "EXECUTING" (test_v2_schedule_iso_timestamp_parsing and
test_v2_source_values) to monkeypatch device_handlers.time.time to a fixed epoch
that makes id=100 fall within the executing window (use the pattern shown in
test_schedule_actually_executing_time_boundary via
monkeypatch.setattr("device_handlers.time.time", ...)); keep the fixture
reusable for other tests and only manipulate time inside the specific tests that
need the live EXECUTING state.
🪄 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
Run ID: 80300233-a9b1-401e-bec6-8a197211fa1c
📒 Files selected for processing (5)
Netro Sprinklers.indigoPlugin/Contents/Info.plistNetro Sprinklers.indigoPlugin/Contents/Server Plugin/device_handlers.pytests/conftest.pytests/test_device_handlers.pytests/test_zone_handler.py
Netro sometimes leaves a completed schedule flagged "EXECUTING" long after its end_time has passed — seen with MANUAL runs on Pixie controllers. Trusting status alone leaves the controller's activeZone and the zone's isIrrigating stuck on True for days. Add SprinklerHandler._schedule_actually_executing() which requires both status == "EXECUTING" AND (end_time missing OR end_time in the future). Fall through to trusting status when end_time can't be verified, so a genuinely-running manual schedule without end_time still registers as active. Use the guard in: - SprinklerHandler.process_schedules (controller-level activeZone/ activeSchedule) - ZoneHandler.process_zone_schedules (per-zone isIrrigating). A stale EXECUTING on the zone's own history is demoted to lastWatering so the record still shows when the run actually happened. Fixtures updated: sample_v2_schedules and sample_schedules_response had past end_times on their EXECUTING entries; bumped to 2099/year-4096 so they continue exercising the genuine-running path. Bumps PluginVersion 2026.4.4 → 2026.4.5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review-driven follow-up to the stale-EXECUTING fix: Critical: - Overwrite status to "EXECUTED" when demoting a stale EXECUTING to last_schedule, so the UI no longer reports "Last watering: Executing" for a watering that has actually ended. Important: - Harden _schedule_actually_executing against a non-dict entry in the schedules array; return False (AttributeError previously escaped the callers' except (KeyError, TypeError)). - Log the stale-EXECUTING demotion at debug level in both handlers so the suppression is observable when diagnosing a "why is my zone suddenly idle?" report. - Log unparseable timestamps at debug in _parse_schedule_sort_key — both the "trust status" fallback and the "never next" sort result are now traceable. - Reorder inf check before V1 ms→seconds divide; the divide is still safe today (inf/1000 == inf) but the ordering is less fragile for future refactors. - Replace brittle 3-day-past ISO dates in V2 tests with 2000-01-01 so assertions aren't clock-dependent. Test additions: - Multi-EXECUTING in same payload (controller picks genuine, zone isIrrigating stays True). - id-comparison path when a stale EXECUTING coexists with a real EXECUTED (both orderings). - Boundary test via monkeypatched time.time (past / exactly-now / future). - V2 tz-aware ISO (+00:00) stale detection. - Unparseable end_time string falls back to trusting status. - Non-dict schedule entry returns False instead of raising. - V2 stale test now asserts lastWateringSource and lastWateringStatus for symmetry with V1. Drop redundant section-banner comment in test_device_handlers.py. 451 tests pass (was 443). Pylint 9.67/10. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
simons-plugins
force-pushed
the
fix/stuck-executing-schedule
branch
from
5fd5c62 to
74dadba
Compare
1 participant
Summary
isIrrigating: True(and controlleractiveZone) when Netro cloud leaves a completed schedule markedEXECUTINGafter itsend_timehas passed.EXECUTINGthree days later. The HTML dashboard and Indigo both showed the zone as watering indefinitely.SprinklerHandler._schedule_actually_executing()which requiresstatus == "EXECUTING"AND (end_timemissing OR in the future). Wire into bothprocess_schedules(controller state) andprocess_zone_schedules(zone state). A staleEXECUTINGon the zone's own history is demoted tolastWateringso the record still shows when the run actually happened.end_timecan't be verified, so a genuinely-running manual job withoutend_timestill registers as active.Test plan
pytest— 443/443 pass (436 existing + 7 new inTestStuckExecutingSchedule+ SprinklerHandler stale-executing cases)EXECUTINGentries had pastend_times — bumped to 2099 / year-4096 so they continue to exercise the genuine-running pathFront Garden Window Bed - Front Garden(id 1939528366) flipsisIrrigatingto False within one poll cycle on jarvisVersion
2026.4.4→2026.4.5(patch — internal reliability, no user-visible behavior change except unsticking the flag)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores