fix(automations): honor conditions + atomic cooldown claim (R15)
Codex R15 flagged two P1 correctness bugs in AutomationEngine: - Conditions were never evaluated — a trigger→condition→action rule ran its actions unconditionally. Now every stored condition must match current entity state (AND) before actions run; a false condition does NOT touch the cooldown clock, so the rule stays armed for its next legitimate trigger. - Cooldown was read-check-then-save, so concurrent queue workers on a burst of state changes could all pass the check and each fire. Now claimed with a single conditional UPDATE — exactly one worker wins the race (H5). +3 tests (condition true/false, cooldown-not-armed-on-failed-condition). AutomationTest 9 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>feat/phase-1-bootstrap
parent
d10172ac26
commit
3ed49ee882
File diff suppressed because it is too large
Load Diff
|
|
@ -4,6 +4,7 @@ namespace App\Services;
|
|||
|
||||
use App\Models\Automation;
|
||||
use App\Models\Entity;
|
||||
use Illuminate\Support\Carbon;
|
||||
use Illuminate\Support\Facades\Log;
|
||||
|
||||
/**
|
||||
|
|
@ -54,17 +55,61 @@ class AutomationEngine
|
|||
|
||||
private function maybeRun(Automation $automation): void
|
||||
{
|
||||
if ($automation->cooldown_seconds > 0
|
||||
&& $automation->last_triggered_at !== null
|
||||
&& $automation->last_triggered_at->gt(now()->subSeconds($automation->cooldown_seconds))) {
|
||||
// Conditions gate the actions but must not touch the cooldown clock — a rule whose
|
||||
// conditions are currently false should stay armed for its next legitimate trigger.
|
||||
if (! $this->conditionsMet($automation)) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Atomically claim the cooldown window: concurrent queue workers processing a burst of
|
||||
// state changes would otherwise all read the same stale last_triggered_at and each fire
|
||||
// the action. The conditional UPDATE lets exactly one worker win the race (H5).
|
||||
if (! $this->claimCooldown($automation)) {
|
||||
return;
|
||||
}
|
||||
|
||||
foreach ($automation->actions ?? [] as $action) {
|
||||
$this->runAction($automation, $action);
|
||||
}
|
||||
}
|
||||
|
||||
$automation->forceFill(['last_triggered_at' => now()])->save();
|
||||
/** Every stored condition ({device, entity_key, field, equals}) must match current state (AND). */
|
||||
private function conditionsMet(Automation $automation): bool
|
||||
{
|
||||
foreach ($automation->conditions ?? [] as $condition) {
|
||||
$entity = $this->findEntity($condition['device'] ?? null, $condition['entity_key'] ?? null);
|
||||
$state = $entity?->state?->state ?? [];
|
||||
$field = $condition['field'] ?? 'on';
|
||||
|
||||
if (! array_key_exists($field, $state) || $state[$field] != ($condition['equals'] ?? true)) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
/** Reserve the run atomically; returns false when the cooldown is still active. */
|
||||
private function claimCooldown(Automation $automation): bool
|
||||
{
|
||||
$now = Carbon::instance(now());
|
||||
|
||||
$query = Automation::query()->whereKey($automation->getKey());
|
||||
|
||||
if ($automation->cooldown_seconds > 0) {
|
||||
$threshold = $now->copy()->subSeconds($automation->cooldown_seconds);
|
||||
$query->where(fn ($q) => $q
|
||||
->whereNull('last_triggered_at')
|
||||
->orWhere('last_triggered_at', '<=', $threshold));
|
||||
}
|
||||
|
||||
$claimed = $query->update(['last_triggered_at' => $now]) > 0;
|
||||
|
||||
if ($claimed) {
|
||||
$automation->last_triggered_at = $now;
|
||||
}
|
||||
|
||||
return $claimed;
|
||||
}
|
||||
|
||||
private function runAction(Automation $automation, array $action): void
|
||||
|
|
@ -72,10 +117,7 @@ class AutomationEngine
|
|||
$type = $action['type'] ?? null;
|
||||
|
||||
if ($type === 'switch') {
|
||||
$entity = Entity::query()
|
||||
->whereHas('device', fn ($q) => $q->where('uuid', $action['device'] ?? null))
|
||||
->where('key', $action['entity_key'] ?? null)
|
||||
->with('device', 'state')->first();
|
||||
$entity = $this->findEntity($action['device'] ?? null, $action['entity_key'] ?? null);
|
||||
|
||||
if ($entity === null) {
|
||||
return;
|
||||
|
|
@ -89,7 +131,19 @@ class AutomationEngine
|
|||
|
||||
$this->commands->setOn($entity, (bool) ($action['on'] ?? true), 'automation');
|
||||
} elseif ($type === 'notify') {
|
||||
Log::info("[automation".($automation->dry_run ? ' dry-run' : '')."] {$automation->name}: ".($action['message'] ?? ''));
|
||||
Log::info('[automation'.($automation->dry_run ? ' dry-run' : '')."] {$automation->name}: ".($action['message'] ?? ''));
|
||||
}
|
||||
}
|
||||
|
||||
private function findEntity(?string $deviceUuid, ?string $entityKey): ?Entity
|
||||
{
|
||||
if ($deviceUuid === null || $entityKey === null) {
|
||||
return null;
|
||||
}
|
||||
|
||||
return Entity::query()
|
||||
->whereHas('device', fn ($q) => $q->where('uuid', $deviceUuid))
|
||||
->where('key', $entityKey)
|
||||
->with('device', 'state')->first();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -147,6 +147,78 @@ class AutomationTest extends TestCase
|
|||
$this->assertDatabaseHas('commands', ['source' => 'automation', 'command' => 'turn_on']);
|
||||
}
|
||||
|
||||
public function test_condition_blocks_action_when_false(): void
|
||||
{
|
||||
[$trigger, $triggerEntity] = $this->makeSwitch();
|
||||
[$target, $targetEntity] = $this->makeSwitch();
|
||||
// A separate sensor whose state the condition inspects.
|
||||
[$sensor, $sensorEntity] = $this->makeSwitch();
|
||||
$sensorEntity->state()->update(['state' => ['on' => false]]);
|
||||
|
||||
Automation::create([
|
||||
'name' => 'Only when sensor on',
|
||||
'enabled' => true,
|
||||
'trigger_type' => 'state_change',
|
||||
'trigger_config' => ['device' => $trigger->uuid, 'entity_key' => $triggerEntity->key, 'field' => 'on', 'equals' => true],
|
||||
'conditions' => [['device' => $sensor->uuid, 'entity_key' => $sensorEntity->key, 'field' => 'on', 'equals' => true]],
|
||||
'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => false]],
|
||||
'cooldown_seconds' => 0,
|
||||
]);
|
||||
|
||||
app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]);
|
||||
|
||||
$this->assertDatabaseCount('commands', 0);
|
||||
}
|
||||
|
||||
public function test_condition_allows_action_when_true(): void
|
||||
{
|
||||
[$trigger, $triggerEntity] = $this->makeSwitch();
|
||||
[$target, $targetEntity] = $this->makeSwitch();
|
||||
[$sensor, $sensorEntity] = $this->makeSwitch();
|
||||
$sensorEntity->state()->update(['state' => ['on' => true]]);
|
||||
|
||||
Automation::create([
|
||||
'name' => 'Only when sensor on',
|
||||
'enabled' => true,
|
||||
'trigger_type' => 'state_change',
|
||||
'trigger_config' => ['device' => $trigger->uuid, 'entity_key' => $triggerEntity->key, 'field' => 'on', 'equals' => true],
|
||||
'conditions' => [['device' => $sensor->uuid, 'entity_key' => $sensorEntity->key, 'field' => 'on', 'equals' => true]],
|
||||
'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => false]],
|
||||
'cooldown_seconds' => 0,
|
||||
]);
|
||||
|
||||
app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]);
|
||||
|
||||
$this->assertDatabaseHas('commands', ['source' => 'automation', 'command' => 'turn_off']);
|
||||
}
|
||||
|
||||
public function test_failed_condition_does_not_start_the_cooldown(): void
|
||||
{
|
||||
[$trigger, $triggerEntity] = $this->makeSwitch();
|
||||
[$target, $targetEntity] = $this->makeSwitch();
|
||||
[$sensor, $sensorEntity] = $this->makeSwitch();
|
||||
$sensorEntity->state()->update(['state' => ['on' => false]]);
|
||||
|
||||
$automation = Automation::create([
|
||||
'name' => 'Armed',
|
||||
'enabled' => true,
|
||||
'trigger_type' => 'state_change',
|
||||
'trigger_config' => ['device' => $trigger->uuid, 'entity_key' => $triggerEntity->key, 'field' => 'on', 'equals' => true],
|
||||
'conditions' => [['device' => $sensor->uuid, 'entity_key' => $sensorEntity->key, 'field' => 'on', 'equals' => true]],
|
||||
'actions' => [['type' => 'switch', 'device' => $target->uuid, 'entity_key' => $targetEntity->key, 'on' => false]],
|
||||
'cooldown_seconds' => 300,
|
||||
]);
|
||||
|
||||
// Condition false → nothing happens and the cooldown must stay unarmed …
|
||||
app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]);
|
||||
$this->assertNull($automation->refresh()->last_triggered_at);
|
||||
|
||||
// … so once the condition becomes true, the very next trigger fires immediately.
|
||||
$sensorEntity->state()->update(['state' => ['on' => true]]);
|
||||
app(AutomationEngine::class)->onStateChange($trigger->uuid, $triggerEntity->key, ['on' => true]);
|
||||
$this->assertDatabaseCount('commands', 1);
|
||||
}
|
||||
|
||||
public function test_disabled_automation_never_runs(): void
|
||||
{
|
||||
[$trigger, $triggerEntity] = $this->makeSwitch();
|
||||
|
|
|
|||
Loading…
Reference in New Issue