diff --git a/app/Provisioning/Jobs/ApplyVpnPeer.php b/app/Provisioning/Jobs/ApplyVpnPeer.php index 1dd6676..570a13d 100644 --- a/app/Provisioning/Jobs/ApplyVpnPeer.php +++ b/app/Provisioning/Jobs/ApplyVpnPeer.php @@ -4,6 +4,7 @@ namespace App\Provisioning\Jobs; use App\Models\VpnPeer; use App\Services\Wireguard\WireguardHub; +use Illuminate\Support\Facades\Cache; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Bus\Dispatchable; @@ -37,6 +38,13 @@ class ApplyVpnPeer implements ShouldQueue } public function handle(WireguardHub $hub): void + { + // Shared with SyncVpnPeers so a reconciliation cannot read the hub + // around this mutation and then act on what it saw. + Cache::lock('wireguard:hub', 30)->block(10, fn () => $this->apply($hub)); + } + + private function apply(WireguardHub $hub): void { // Read the CURRENT desired state rather than the one captured at // dispatch time. A retried block job would otherwise undo an unblock diff --git a/app/Provisioning/Jobs/SyncVpnPeers.php b/app/Provisioning/Jobs/SyncVpnPeers.php index 79ffb2c..97d72ad 100644 --- a/app/Provisioning/Jobs/SyncVpnPeers.php +++ b/app/Provisioning/Jobs/SyncVpnPeers.php @@ -5,6 +5,7 @@ namespace App\Provisioning\Jobs; use App\Models\Host; use App\Models\VpnPeer; use App\Services\Wireguard\WireguardHub; +use Illuminate\Support\Facades\Cache; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldBeUnique; use Illuminate\Contracts\Queue\ShouldQueue; @@ -39,6 +40,26 @@ class SyncVpnPeers implements ShouldBeUnique, ShouldQueue } public function handle(WireguardHub $hub): void + { + // Reading the hub and reconciling must not straddle a peer removal: a + // snapshot taken before ApplyVpnPeer removes a peer and purges its + // tombstone would otherwise be adopted afterwards as a fresh, enabled + // access — restoring exactly what was revoked. ApplyVpnPeer holds the + // same lock, so read and mutation are mutually exclusive. + $retryRemoval = []; + Cache::lock('wireguard:hub', 30)->block(10, function () use ($hub, &$retryRemoval) { + $this->reconcile($hub, $retryRemoval); + }); + + // Dispatched outside the lock: with the sync queue driver this executes + // inline, and ApplyVpnPeer takes the very same lock. + foreach ($retryRemoval as $publicKey) { + ApplyVpnPeer::dispatch($publicKey, null, false); + } + } + + /** @param list $retryRemoval collected, dispatched by the caller */ + private function reconcile(WireguardHub $hub, array &$retryRemoval): void { $snapshots = $hub->peers(); $now = now(); @@ -56,7 +77,7 @@ class SyncVpnPeers implements ShouldBeUnique, ShouldQueue // rather than adopting the peer back into the console — otherwise a // failed removal quietly becomes a live access again. if ($peer?->trashed()) { - ApplyVpnPeer::dispatch($publicKey, null, false); + $retryRemoval[] = $publicKey; continue; } diff --git a/tests/Feature/Admin/VpnTest.php b/tests/Feature/Admin/VpnTest.php index 9f13bef..8630522 100644 --- a/tests/Feature/Admin/VpnTest.php +++ b/tests/Feature/Admin/VpnTest.php @@ -321,3 +321,19 @@ it('never re-adds a peer whose row is gone, however stale the job', function () expect($hub->peerIps())->toBe([]) ->and(VpnPeer::withTrashed()->count())->toBe(0); }); + +it('serializes hub reads against hub mutations', function () { + $hub = vpnHub(); + $peer = VpnPeer::factory()->create(); + $hub->addPeer($peer->public_key, $peer->allowed_ip); + + // A removal in flight holds this lock. A sync must wait for it instead of + // acting on a snapshot taken around the mutation. + expect(Illuminate\Support\Facades\Cache::lock('wireguard:hub', 1)->get())->toBeTrue(); + + $started = microtime(true); + (new SyncVpnPeers)->handle($hub); + + expect(microtime(true) - $started)->toBeGreaterThan(0.8) + ->and($peer->fresh()->present)->toBeTrue(); +});