Skip to content

Add regression test for separate_turn_on_commands race condition#1379

Open
basnijholt wants to merge 1 commit intomainfrom
fix/separate-turn-on-commands-race-condition
Open

Add regression test for separate_turn_on_commands race condition#1379
basnijholt wants to merge 1 commit intomainfrom
fix/separate-turn-on-commands-race-condition

Conversation

@basnijholt
Copy link
Copy Markdown
Owner

@basnijholt basnijholt commented Jan 11, 2026

Summary

Bug Description

When separate_turn_on_commands: true is enabled and a light is turned off between the split brightness and color commands, the second command should be skipped. Currently it's not because:

  1. The proactive adaptation context is never cleared when light.turn_off is called
  2. This causes the off-check in _execute_adaptation_calls (switch.py:1337-1349) to be bypassed
  3. The second split command (color) is sent to an off light, turning it back on

Test plan

  • CI runs the test and it FAILS (confirming the bug exists)
  • After implementing the fix, CI runs the test and it PASSES

Related

Closes #1373 (after fix is implemented)

@basnijholt
Add regression test for separate_turn_on_commands race condition
45756aa 
Regression test for #1373

This test verifies that when `separate_turn_on_commands: true` is enabled
and a light is turned off between split commands, the second command
should be skipped.

The bug occurs because the proactive adaptation context is never cleared
when `light.turn_off` is called, causing the off-check to be bypassed
in `_execute_adaptation_calls`.

This test currently FAILS (demonstrating the bug) and will PASS after
the fix is implemented.
florianhorner pushed a commit to florianhorner/adaptive-lighting that referenced this pull request Apr 19, 2026
Merge PR basnijholt#1379: Add regression test for separate_turn_on_co…
6024fdd 
…mmands race condition (basnijholt)

Conflicts resolved:
- tests/test_switch.py: kept fork's test_automation_turn_on_from_off_not_marked_as_manual_control
  (from basnijholt#1378 fix) and appended upstream's test_separate_turn_on_commands_respects_light_off_state
  at end of file so both regression tests coexist.
florianhorner pushed a commit to florianhorner/adaptive-lighting that referenced this pull request Apr 19, 2026
test: mark PR basnijholt#1379 regression test as xfail
bc69d6f 
The test documents an unfixed bug (proactive adaptation context not
cleared on light.turn_off, causing second split command to be sent
to an off light). Upstream basnijholt#1379 added the test without the fix, so
we xfail it until the bug is addressed.
@florianhorner florianhorner mentioned this pull request Apr 19, 2026
Merged
4 tasks
florianhorner added a commit to florianhorner/adaptive-lighting that referenced this pull request Apr 19, 2026
@florianhorner @basnijholt @hesseleo
Cherry-pick upstream basnijholt#1462 + basnijholt#1379 (expand_light_…
e3aeb33 
…groups + race-condition regression test) (#12)

* Add regression test for separate_turn_on_commands race condition

Regression test for basnijholt#1373

This test verifies that when `separate_turn_on_commands: true` is enabled
and a light is turned off between split commands, the second command
should be skipped.

The bug occurs because the proactive adaptation context is never cleared
when `light.turn_off` is called, causing the off-check to be bypassed
in `_execute_adaptation_calls`.

This test currently FAILS (demonstrating the bug) and will PASS after
the fix is implemented.

* feat: add expand_light_groups option

Some light group entities act as a proxy that must receive a single combined
`light.turn_on` call to function correctly — for example virtual mixers like
<https://github.com/mion00/color-temperature-light-mixer> for instance,
that blend a warm and a cold white channel into one entity. In such setups the
individual member entities only expose `ColorMode.BRIGHTNESS`, so sending
separate per-member commands bypasses the mixing logic.

Setting `expand_light_groups: false` keeps the group entity in `self.lights`
instead of expanding it to its members. Adaptation commands go to the group,
and the interceptor no longer skips group entities for that switch.

Default is `true` — no behaviour change for existing configurations.

* test: mark PR basnijholt#1379 regression test as xfail

The test documents an unfixed bug (proactive adaptation context not
cleared on light.turn_off, causing second split command to be sent
to an off light). Upstream basnijholt#1379 added the test without the fix, so
we xfail it until the bug is addressed.

* fix: wire expand_light_groups into STEP_OPTIONS workarounds bucket

Without this, the option is defined in VALIDATION_TUPLES but invisible
in the 5-step options flow UI — users can't toggle it and it stays at
the default True forever. Caught by /plan-eng-review on PR #12.

---------

Co-authored-by: Bas Nijholt <bas@nijho.lt>
Co-authored-by: Leonhard Hesse <leonhard.hesse@outlook.com>
Co-authored-by: Florian Horner <florianhorner@macbook-pro-von-florian.tail6f28f8.ts.net>
@florianhorner florianhorner mentioned this pull request Apr 20, 2026
Merged
4 tasks
florianhorner pushed a commit to florianhorner/adaptive-lighting that referenced this pull request Apr 20, 2026
feat: warn on silent group expansion + adapt-cycle/manual-control tests
1560a91 
Given the Lightener curve card work depends on expand_light_groups=False
being honored everywhere, this commit closes three silent-failure gaps
from the follow-up review on PR #13:

Gap 1 — adapt-cycle dispatch test
  test_expand_light_groups_false_adapt_cycle_dispatches_to_group
  Spies on _adapt_light to lock in that the periodic adaptation cycle
  dispatches to the GROUP entity_id, not its members, when flag=False.
  Without this, the intercept path could behave correctly while the
  time-interval-driven adaptation silently bypasses Lightener curves on
  every adapt cycle.

Gap 2 — manual-control attribution test
  test_expand_light_groups_false_no_false_manual_control
  With take_over_control=True + flag=False, internal member state
  changes (Lightener fan-out, Hue bridge sync) must NOT populate
  manager.manual_control. Otherwise every adapt cycle would mark
  members as manually controlled and the next cycle would skip them
  entirely — a slow death spiral with curves that eventually never fire.

Gap 3 — one-time warning when AL expands a light group
  switch.py: AdaptiveSwitch._expand_light_groups now emits a WARNING
  (debounced via manager.expand_light_groups_warned set) pointing the
  user at the expand_light_groups: false flag. Fires only from the
  switch-configured code path; utility expansions in _switches_with_lights
  stay silent so matching doesn't spam the log.

  test_expand_light_groups_true_emits_warning_once asserts the warning
  fires exactly once per entity per HA run.
  test_expand_light_groups_false_does_not_warn asserts the warning
  stays silent for users who already opted in to the fix.

Full suite: 35 pre-existing failures (unchanged baseline), 0 new
regressions. 9/9 new expand_light_groups tests passing. 1 intentional
xfail from basnijholt#1379 untouched.
florianhorner added a commit to florianhorner/adaptive-lighting that referenced this pull request Apr 20, 2026
@florianhorner
feat: full depth on expand_light_groups — tests + Lightener-aware war…
5a2c99e 
…ning (#13)

* test: full coverage for expand_light_groups option

Adds 5 unit tests filling the P2 gap from /plan-eng-review on PR #12.
The new option's non-default branch had zero coverage; these lock in
behavior before users start flipping it (e.g. for Lightener, Hue
rooms, or Zigbee2MQTT groups).

- test_expand_light_groups_const_wiring: default True, VALIDATION_TUPLES,
  STEP_OPTIONS[workarounds] UI wiring, DOCS entry — catches the exact
  class of bug that required the follow-up STEP_OPTIONS fix in #12.
- test_expand_light_groups_false_keeps_group_entity: group stays in
  manager.lights, members are NOT tracked individually.
- test_expand_light_groups_true_expands_to_members: default behavior
  still produces the member-level tracking, preventing accidental
  regression.
- test_expand_light_groups_false_non_group_unaffected: no-op for
  non-group lights — safe for users who flip the flag without groups.
- test_expand_light_groups_false_group_turn_on_reaches_group: the
  core behavioral guarantee — a user turn_on on a group entity must
  reach the group, not get silently rerouted to members. This is
  what unblocks Lightener.

Full suite: 35 pre-existing failures (unchanged baseline), 0 new
regressions, 1 intentional xfail from PR basnijholt#1379.

* feat: warn on silent group expansion + adapt-cycle/manual-control tests

Given the Lightener curve card work depends on expand_light_groups=False
being honored everywhere, this commit closes three silent-failure gaps
from the follow-up review on PR #13:

Gap 1 — adapt-cycle dispatch test
  test_expand_light_groups_false_adapt_cycle_dispatches_to_group
  Spies on _adapt_light to lock in that the periodic adaptation cycle
  dispatches to the GROUP entity_id, not its members, when flag=False.
  Without this, the intercept path could behave correctly while the
  time-interval-driven adaptation silently bypasses Lightener curves on
  every adapt cycle.

Gap 2 — manual-control attribution test
  test_expand_light_groups_false_no_false_manual_control
  With take_over_control=True + flag=False, internal member state
  changes (Lightener fan-out, Hue bridge sync) must NOT populate
  manager.manual_control. Otherwise every adapt cycle would mark
  members as manually controlled and the next cycle would skip them
  entirely — a slow death spiral with curves that eventually never fire.

Gap 3 — one-time warning when AL expands a light group
  switch.py: AdaptiveSwitch._expand_light_groups now emits a WARNING
  (debounced via manager.expand_light_groups_warned set) pointing the
  user at the expand_light_groups: false flag. Fires only from the
  switch-configured code path; utility expansions in _switches_with_lights
  stay silent so matching doesn't spam the log.

  test_expand_light_groups_true_emits_warning_once asserts the warning
  fires exactly once per entity per HA run.
  test_expand_light_groups_false_does_not_warn asserts the warning
  stays silent for users who already opted in to the fix.

Full suite: 35 pre-existing failures (unchanged baseline), 0 new
regressions. 9/9 new expand_light_groups tests passing. 1 intentional
xfail from basnijholt#1379 untouched.

---------

Co-authored-by: Florian Horner <florianhorner@macbook-pro-von-florian.tail6f28f8.ts.net>
@basnijholt
Copy link
Copy Markdown
Owner Author

basnijholt commented Apr 24, 2026

Sorry it took me so long to get to this. I've been a bit overwhelmed by the number of AI-assisted PRs opened here recently, and I've also been spending nearly every spare hour on my biggest project so far, MindRoom.

I'm very supportive of using AI for coding, but many of these PRs still need careful human review because even plausible-looking changes can introduce subtle breakage. That backlog made me postpone reviewing them for a while.

I've now done a batch review with Codex / GPT-5.5 (xhigh). This comment is AI-assisted, but I've reviewed it before posting.

I like that this is test-only/regression-focused, but the branch is stale/conflicting with current main.

If this race condition is still relevant, please rebase the test onto current main so we can see whether the regression still fails/passes with the latest implementation. If it has been covered by later changes, I would close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Race condition with separate_turn_on_commands causes lights to show 'on at 0% brightness' after turn_off

1 participant

@basnijholt