fix(addons): atomic Ring device creation + invalidate ingest gate cache (R15)
Codex R15 on the Ring bridge flagged two issues: - [P1] Concurrent first-sight messages (retained info + motion on bridge startup) could create duplicate devices — no uniqueness on ring_id. Added a partial unique index on (config->>'ring_id'); resolveDevice now creates and, on the unique violation, re-fetches the winner (race-safe). - [P2] The install-gate cache was never invalidated, so ingest was dropped for up to 15s after install and devices kept being created for up to 15s after uninstall. AddonService now forgets the shared cache key on install/uninstall. +3 tests (unique-index guard, repeated-messages-reuse-device, cache invalidation). Suite 43 green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>feat/phase-1-bootstrap
parent
a4489ac7d3
commit
8086a9647b
|
|
@ -10,6 +10,7 @@ use App\Services\AddonService;
|
|||
use App\Support\Mqtt\RingNormalizer;
|
||||
use App\Support\Mqtt\RingTopics;
|
||||
use Illuminate\Contracts\Queue\ShouldQueue;
|
||||
use Illuminate\Database\UniqueConstraintViolationException;
|
||||
use Illuminate\Foundation\Queue\Queueable;
|
||||
use Illuminate\Support\Carbon;
|
||||
use Illuminate\Support\Facades\Cache;
|
||||
|
|
@ -87,8 +88,13 @@ class IngestRingMessage implements ShouldQueue
|
|||
|
||||
private function resolveDevice(string $ringId, string $category): Device
|
||||
{
|
||||
return Device::where('config->ring_id', $ringId)->first()
|
||||
?? Device::create([
|
||||
$existing = Device::where('config->ring_id', $ringId)->first();
|
||||
if ($existing !== null) {
|
||||
return $existing;
|
||||
}
|
||||
|
||||
try {
|
||||
return Device::create([
|
||||
'name' => Str::headline($category).' · '.Str::upper(Str::substr($ringId, -4)),
|
||||
'vendor' => 'Ring',
|
||||
'protocol' => 'mqtt',
|
||||
|
|
@ -96,10 +102,15 @@ class IngestRingMessage implements ShouldQueue
|
|||
'last_seen_at' => now(),
|
||||
'config' => ['cloud' => true, 'ring_id' => $ringId, 'ring_category' => $category],
|
||||
]);
|
||||
} catch (UniqueConstraintViolationException) {
|
||||
// A concurrent worker created it first (devices_ring_id_unique) — use the winner.
|
||||
return Device::where('config->ring_id', $ringId)->firstOrFail();
|
||||
}
|
||||
}
|
||||
|
||||
private function ringInstalled(): bool
|
||||
{
|
||||
return Cache::remember('addon:ring:installed', 15, fn () => Addon::where('key', 'ring')->where('installed', true)->exists());
|
||||
return Cache::remember(AddonService::installedCacheKey('ring'), 15,
|
||||
fn () => Addon::where('key', 'ring')->where('installed', true)->exists());
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@
|
|||
namespace App\Services;
|
||||
|
||||
use App\Models\Addon;
|
||||
use Illuminate\Support\Facades\Cache;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
|
||||
/**
|
||||
|
|
@ -12,6 +13,12 @@ use Illuminate\Support\Facades\DB;
|
|||
*/
|
||||
class AddonService
|
||||
{
|
||||
/** Cache key the ingest gate reads to avoid a DB hit per message. */
|
||||
public static function installedCacheKey(string $key): string
|
||||
{
|
||||
return "addon:{$key}:installed";
|
||||
}
|
||||
|
||||
public function row(string $key): Addon
|
||||
{
|
||||
return Addon::firstOrCreate(['key' => $key]);
|
||||
|
|
@ -26,6 +33,10 @@ class AddonService
|
|||
$addon = $this->row($key);
|
||||
$addon->forceFill(['installed' => true])->save();
|
||||
|
||||
// Invalidate the ingest gate immediately — otherwise messages are dropped for up to the
|
||||
// cache TTL right after install (and, on uninstall below, devices keep being created).
|
||||
Cache::forget(self::installedCacheKey($key));
|
||||
|
||||
return $addon;
|
||||
}
|
||||
|
||||
|
|
@ -41,6 +52,8 @@ class AddonService
|
|||
'config' => null,
|
||||
'connected_at' => null,
|
||||
])->save();
|
||||
|
||||
Cache::forget(self::installedCacheKey($key));
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -0,0 +1,20 @@
|
|||
<?php
|
||||
|
||||
use Illuminate\Database\Migrations\Migration;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
|
||||
return new class extends Migration
|
||||
{
|
||||
public function up(): void
|
||||
{
|
||||
// Concurrent first-sight Ring messages (retained info + motion on bridge startup) must not
|
||||
// create two rows for one device. A partial unique index on the JSON ring_id lets the
|
||||
// second concurrent insert fail so the ingest job can re-fetch the winner (race-safe).
|
||||
DB::statement("CREATE UNIQUE INDEX devices_ring_id_unique ON devices ((config->>'ring_id')) WHERE config->>'ring_id' IS NOT NULL");
|
||||
}
|
||||
|
||||
public function down(): void
|
||||
{
|
||||
DB::statement('DROP INDEX IF EXISTS devices_ring_id_unique');
|
||||
}
|
||||
};
|
||||
|
|
@ -6,6 +6,7 @@ use App\Models\Addon;
|
|||
use App\Services\AddonRegistry;
|
||||
use App\Services\AddonService;
|
||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||
use Illuminate\Support\Facades\Cache;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
use Tests\TestCase;
|
||||
|
||||
|
|
@ -78,6 +79,22 @@ class AddonTest extends TestCase
|
|||
$this->assertNull($addon->config);
|
||||
}
|
||||
|
||||
public function test_install_and_uninstall_invalidate_the_ingest_gate_cache(): void
|
||||
{
|
||||
$key = AddonService::installedCacheKey('ring');
|
||||
$service = app(AddonService::class);
|
||||
|
||||
// Prime a stale "not installed" value, then install → cache must be cleared.
|
||||
Cache::put($key, false, 60);
|
||||
$service->install('ring');
|
||||
$this->assertFalse(Cache::has($key));
|
||||
|
||||
// Prime a stale "installed" value, then uninstall → cache must be cleared.
|
||||
Cache::put($key, true, 60);
|
||||
$service->uninstall('ring');
|
||||
$this->assertFalse(Cache::has($key));
|
||||
}
|
||||
|
||||
public function test_mark_status_only_applies_to_installed_addons(): void
|
||||
{
|
||||
$service = app(AddonService::class);
|
||||
|
|
|
|||
|
|
@ -84,6 +84,26 @@ class RingIngestTest extends TestCase
|
|||
$this->assertDatabaseCount('devices', 0);
|
||||
}
|
||||
|
||||
public function test_ring_id_is_unique_so_concurrent_creates_cannot_duplicate(): void
|
||||
{
|
||||
Device::create(['name' => 'A', 'vendor' => 'Ring', 'protocol' => 'mqtt', 'config' => ['ring_id' => 'dup1']]);
|
||||
|
||||
$this->expectException(\Illuminate\Database\UniqueConstraintViolationException::class);
|
||||
Device::create(['name' => 'B', 'vendor' => 'Ring', 'protocol' => 'mqtt', 'config' => ['ring_id' => 'dup1']]);
|
||||
}
|
||||
|
||||
public function test_repeated_messages_reuse_the_same_device(): void
|
||||
{
|
||||
$this->installRing();
|
||||
|
||||
(new IngestRingMessage('ring/loc1/camera/cam9/ding/state', 'ON'))->handle();
|
||||
(new IngestRingMessage('ring/loc1/camera/cam9/motion/state', 'ON'))->handle();
|
||||
(new IngestRingMessage('ring/loc1/camera/cam9/info/state', json_encode(['batteryLevel' => 50])))->handle();
|
||||
|
||||
$this->assertSame(1, Device::where('config->ring_id', 'cam9')->count());
|
||||
$this->assertSame(3, Device::where('config->ring_id', 'cam9')->first()->entities()->count());
|
||||
}
|
||||
|
||||
public function test_out_of_order_messages_do_not_overwrite_newer_state(): void
|
||||
{
|
||||
$this->installRing();
|
||||
|
|
|
|||
Loading…
Reference in New Issue