Add request deduplication support to Shelly emulator#333
Merged
Conversation
Add a shared RequestDeduplicator helper and wire it into both the CT002 emulator (keyed by parsed consumer id) and the Shelly emulator (keyed by battery IP, since Shelly sources use ephemeral ports). DEDUPE_TIME_WINDOW can now be set under [GENERAL] to apply regardless of which device type is emulated; the existing [CT002]/[CT003] override still wins when present. Surface dedupe_time_window, smooth_target_alpha, max_smooth_step, and deadband in the HA add-on UI so users don't need a custom config file for common tuning. https://claude.ai/code/session_01YZtwVEn4bic9TtfqLmDaZS
|
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 42 minutes and 2 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
WalkthroughThis PR introduces a generic request deduplication mechanism that drops burst-repeat requests from the same source within a configured time window. A new Changes
Sequence DiagramsequenceDiagram
participant Client as UDP/HTTP Client
participant Handler as Device Handler<br/>(CT002/Shelly)
participant Dedup as RequestDeduplicator
participant Clock as Clock
participant Device as Device Logic
Client->>Handler: Request arrives
Handler->>Dedup: should_process(source_id)
Dedup->>Clock: Get current time
Clock-->>Dedup: time_value
alt Within Dedupe Window
Dedup-->>Handler: false
Handler->>Handler: Log suppressed request
Handler-->>Client: (no response)
else Outside Dedupe Window
Dedup->>Clock: Get current time
Clock-->>Dedup: time_value
Dedup->>Dedup: Update last_seen[source_id]
Dedup-->>Handler: true
Handler->>Device: Process request
Device-->>Client: Response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
🧹 Nitpick comments (3)
src/astrameter/request_dedupe_test.py (1)
1-60: Test coverage is solid.Window=0 disabled path, within-window drop, post-window accept, key independence, purge semantics, and empty-purge safety are all covered. The
FakeClockcallable is a clean way to drive the injected clock.One optional addition: a test confirming that a dropped request does not refresh the "last accepted" timestamp (i.e., window is measured from last accept, not last attempt) — this is an observable behavior worth locking in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/astrameter/request_dedupe_test.py` around lines 1 - 60, Add a test that verifies a dropped request does not update the last-accepted timestamp: create a FakeClock and RequestDeduplicator, call should_process("k") to accept once, advance clock less than the window and call should_process("k") expecting False, then advance the clock so that if the failed attempt had refreshed the timestamp it would still be blocked but because it shouldn't refresh the second call the third call after enough time should return True; reference RequestDeduplicator and its should_process method and use FakeClock to advance time and assert the expected True/False sequence.src/astrameter/ct002/ct002.py (1)
146-148: Nit: redundant clock fallback.
RequestDeduplicatoralready defaultsclocktotime.monotonicwhenNone. Passingclock or time.timehere overrides that default with wall-clock time to stay consistent with the rest of CT002 (e.g.,_cleanup_consumersusestime.time()), which is fine and intentional — just worth a short comment so future readers don't "fix" it toclockonly and inadvertently mix monotonic/wall-clock time sources between_dedup.purge_older_than(clock) and_cleanup_consumers(time.time).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/astrameter/ct002/ct002.py` around lines 146 - 148, Add a short clarifying comment where self._dedup is constructed to explain that passing clock or time.time intentionally overrides RequestDeduplicator's default monotonic clock so the deduplicator uses wall-clock time to stay consistent with the rest of CT002 (e.g., _cleanup_consumers uses time.time()), preventing future maintainers from changing it back to the default and mixing monotonic vs wall-clock for _dedup.purge_older_than and _cleanup_consumers.src/astrameter/request_dedupe.py (1)
10-41: Clean, focused utility — LGTM.A couple of minor notes worth considering (not blockers):
- Unbounded growth by design.
_lastonly shrinks viapurge_older_than. Both current callers (CT002 via_cleanup_consumers, Shelly per PR context) purge periodically, but it's worth a docstring note so future callers don't forget to callpurge_older_thanand leak memory on high-cardinality keys.- NaN window.
max(0.0, float('nan'))is not well-defined (NaN comparisons are all False), so a NaNwindow_secondscould slip past the clamp. Unlikely from config in practice; an explicitif not math.isfinite(window_seconds) or window_seconds < 0: ... = 0.0would be more defensive.- Dropped requests don't refresh the timestamp, so the window is measured from the last accepted request, not the last seen one. This is a reasonable choice (prevents a sustained burst from indefinitely extending suppression) — worth one line in the docstring to make it explicit for callers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/astrameter/request_dedupe.py` around lines 10 - 41, Update RequestDeduplicator to (1) defensively handle NaN/inf window_seconds in __init__: treat non-finite or negative values as 0.0 (use math.isfinite and comparison) and assign to self._window; (2) document two behaviors in the class docstring: that _last is unbounded unless callers invoke purge_older_than (so callers should schedule periodic purges) and that should_process measures the window from the last accepted request (dropped requests do not refresh the timestamp). Refer to the RequestDeduplicator class, its __init__, should_process, purge_older_than and the _last/_window attributes when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ha_addon/config.yaml`:
- Around line 61-62: The config schema uses float types for watt-based CT fields
(max_smooth_step and deadband) but the runtime (src/astrameter/main.py) reads
DEADBAND with cfg.getint(...) which will break on fractional values; change the
UI/schema types for the DEADBAND and max_smooth_step keys to integer (or
otherwise match the runtime parser) so values like 20.5 are rejected and only
integers are allowed, and keep the schema name references (deadband,
max_smooth_step, and the runtime symbol DEADBAND / the cfg.getint call in
src/astrameter/main.py) aligned.
In `@ha_addon/run.sh`:
- Around line 149-157: The script in run.sh currently emits SMOOTH_TARGET_ALPHA,
MAX_SMOOTH_STEP and DEADBAND under the [HOMEASSISTANT] block, but
src/astrameter/main.py reads DEADBAND from the CT section (ct_section), so move
these three CT tuning variables into the CT section output instead of
HOMEASSISTANT: when constructing the config for each CT (the same place you
print the CT section header like “[CT002]”/“[CT003]” or use the ct_section
variable), emit lines "SMOOTH_TARGET_ALPHA=…", "MAX_SMOOTH_STEP=…", and
"DEADBAND=…" there (only if bashio::config.has_value for each), so the values
are available to the code that reads ct_section in src/astrameter/main.py.
In `@README.md`:
- Around line 306-309: The README's configuration examples are missing the
DEDUPE_TIME_WINDOW key in the [GENERAL] block; update the example so
DEDUPE_TIME_WINDOW appears under [GENERAL] (not only under [CT002]) so Shelly
users can apply the global dedupe window. Edit the README sample config to add
the DEDUPE_TIME_WINDOW setting in the [GENERAL] section with a brief comment
mirroring the existing description, ensuring the key name DEDUPE_TIME_WINDOW and
the bracketed section labels [GENERAL] and [CT002] are present for clarity.
In `@src/astrameter/shelly/shelly.py`:
- Line 54: The deduper is currently initialized as
RequestDeduplicator(dedupe_time_window) but later a cleanup loop purges entries
older than BATTERY_INACTIVE_TIMEOUT_SECONDS which can truncate any
DEDUPE_TIME_WINDOW > 120; update the implementation so the configured
dedupe_time_window is stored (e.g. self._dedupe_time_window) and pass the full
configured window into RequestDeduplicator or adjust the cleanup logic to use
max(BATTERY_INACTIVE_TIMEOUT_SECONDS, self._dedupe_time_window) when purging;
locate usages of RequestDeduplicator, self._dedup, dedupe_time_window and the
cleanup loop that references BATTERY_INACTIVE_TIMEOUT_SECONDS and change the
purge horizon to the larger retention value so configured windows >120s are
honored.
---
Nitpick comments:
In `@src/astrameter/ct002/ct002.py`:
- Around line 146-148: Add a short clarifying comment where self._dedup is
constructed to explain that passing clock or time.time intentionally overrides
RequestDeduplicator's default monotonic clock so the deduplicator uses
wall-clock time to stay consistent with the rest of CT002 (e.g.,
_cleanup_consumers uses time.time()), preventing future maintainers from
changing it back to the default and mixing monotonic vs wall-clock for
_dedup.purge_older_than and _cleanup_consumers.
In `@src/astrameter/request_dedupe_test.py`:
- Around line 1-60: Add a test that verifies a dropped request does not update
the last-accepted timestamp: create a FakeClock and RequestDeduplicator, call
should_process("k") to accept once, advance clock less than the window and call
should_process("k") expecting False, then advance the clock so that if the
failed attempt had refreshed the timestamp it would still be blocked but because
it shouldn't refresh the second call the third call after enough time should
return True; reference RequestDeduplicator and its should_process method and use
FakeClock to advance time and assert the expected True/False sequence.
In `@src/astrameter/request_dedupe.py`:
- Around line 10-41: Update RequestDeduplicator to (1) defensively handle
NaN/inf window_seconds in __init__: treat non-finite or negative values as 0.0
(use math.isfinite and comparison) and assign to self._window; (2) document two
behaviors in the class docstring: that _last is unbounded unless callers invoke
purge_older_than (so callers should schedule periodic purges) and that
should_process measures the window from the last accepted request (dropped
requests do not refresh the timestamp). Refer to the RequestDeduplicator class,
its __init__, should_process, purge_older_than and the _last/_window attributes
when making these changes.
🪄 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: 9205694e-55dd-452f-90e4-4e3f2aa5a239
📒 Files selected for processing (14)
CHANGELOG.mdREADME.mdconfig.ini.exampleha_addon/config.yamlha_addon/run.shha_addon/translations/en.yamlsrc/astrameter/ct002/ct002.pysrc/astrameter/ct002/ct002_test.pysrc/astrameter/main.pysrc/astrameter/request_dedupe.pysrc/astrameter/request_dedupe_test.pysrc/astrameter/shelly/shelly.pysrc/astrameter/shelly/shelly_udp_test.pysrc/astrameter/web_config.py
- Make the Shelly dedup test deterministic by driving _handle_request directly with a fake transport and a fake clock injected into the deduplicator; no more real UDP sockets or sleep-based timing. - Honor DEDUPE_TIME_WINDOW values greater than the 120s Shelly battery-inactivity horizon by purging with max(horizon, window). - Clarify in CT002 why the deduplicator is constructed with time.time instead of defaulting to time.monotonic (shares a timebase with _cleanup_consumers). - RequestDeduplicator: treat non-finite and negative windows as disabled, and document the "window-from-last-accepted" semantic and the caller responsibility to purge. - Add tests for the new guards and for "dropped request must not refresh the timestamp." - Add DEDUPE_TIME_WINDOW to the README [GENERAL] example so Shelly users see it without hunting through the CT section. https://claude.ai/code/session_01YZtwVEn4bic9TtfqLmDaZS
BalancerConfig.deadband was wired through main.py -> CT002 -> BalancerConfig but never actually read anywhere; the only "deadband" semantics that still matter are BALANCE_DEADBAND (multi-battery imbalance floor) and the per-powermeter DeadbandPowermeter wrapper. Drop the dead field, its clamp, the constructor parameter, and the corresponding cfg.getint read. Purge deadband=... from all BalancerConfig test constructions. While here, fix two related mis-placements that were misleading users: - web_config.py listed SMOOTH_TARGET_ALPHA / MAX_SMOOTH_STEP / DEADBAND under the [CT002] schema, but the runtime only reads them from powermeter sections or [GENERAL] (config_loader.py:161-252). Move them to GENERAL. - README described the same three keys as "CT002/CT003 active-steering" options. They are powermeter-wrapper options. Move them into the per-powermeter options list alongside THROTTLE_INTERVAL. https://claude.ai/code/session_01YZtwVEn4bic9TtfqLmDaZS
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Refactored request deduplication logic into a reusable
RequestDeduplicatorclass and extended deduplication support to the Shelly emulator. Previously, deduplication was only available for CT002/CT003 devices and was hardcoded to use source IP addresses. Now both device types support configurable deduplication windows, with CT002/CT003 keyed by consumer ID and Shelly keyed by battery IP.Key Changes
New
RequestDeduplicatorclass (src/astrameter/request_dedupe.py):purge_older_than()method to clean up stale entriesCT002 refactoring (
src/astrameter/ct002/ct002.py):_last_response_timetracking withRequestDeduplicatorinstancepurge_older_than()instead of manual stale entry removalShelly deduplication (
src/astrameter/shelly/shelly.py):dedupe_time_windowparameter toShelly.__init__()Configuration updates (
src/astrameter/main.py):DEDUPE_TIME_WINDOWsetting under[GENERAL]sectionTest coverage:
RequestDeduplicatorwith fake clock injectionDocumentation and configuration:
config.ini.examplewithDEDUPE_TIME_WINDOWdocumentationREADME.mdto document Shelly deduplication supportImplementation Details
RequestDeduplicatoruses a simple dictionary to track the last seen timestamp for each key(now - last) < windowto determine if a request should be droppedtime.monotonichttps://claude.ai/code/session_01YZtwVEn4bic9TtfqLmDaZS
Summary by CodeRabbit
Release Notes
New Features
DEDUPE_TIME_WINDOWconfiguration option to suppress repeated requests from the same source within a configurable time window.smooth_target_alpha,max_smooth_step, anddeadband.Documentation