fix(editor): never fail an operation because its announcement could not go out

Author: dnlblasco

README.md

@@ -66,6 +66,15 @@ REVERB_SCHEME=https
 
 Start the server with `php artisan reverb:start` and give it a route from the public host. Behind Cloudflare, WebSockets must be enabled for the zone.
 
+Reverb needs **two** paths proxied to it, not one:
+
+| Path | Used by | Direction |
+|---|---|---|
+| `/app/{key}` | browser and plugin | the WebSocket itself |
+| `/apps/{id}/events` | this application | publishing an event |
+
+Routing only `/app` leaves the sockets working while every publish fails. Nothing breaks visibly, because a failed announcement is logged rather than raised, but the other open windows stop hearing about changes.
+
 ## Demo mode
 
 Opening the site without a session shows the editor running on the example menus the plugin ships. Everything renders and every editor works; only saving is unavailable, because there is no server behind it.

app/Http/Controllers/Api/MenuController.php

@@ -9,7 +9,9 @@
 use App\Services\RpcBridge;
 use Illuminate\Http\JsonResponse;
 use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Log;
 use Illuminate\Validation\Rule;
+use Throwable;
 
 /**
  * Menu operations, each one forwarded to the plugin that owns the session.
@@ -64,9 +66,22 @@ private function forward(Request $request, RpcBridge $rpc, RpcAction $action, ar
         return response()->json($rpc->call($session->server, $action, $payload));
     }
 
+    /**
+     * Lets the other windows know a menu changed.
+     *
+     * Broadcasting is a courtesy: the file is already written, so a broadcaster
+     * that is down or misrouted must not turn a successful save into a 500.
+     */
     private function announce(Request $request, string $platform, string $fileName, string $change): void
     {
-        MenuChanged::dispatch($this->session($request), $platform, $fileName, $change);
+        try {
+            MenuChanged::dispatch($this->session($request), $platform, $fileName, $change);
+        } catch (Throwable $failure) {
+            Log::warning('Could not announce a menu change', [
+                'fileName' => $fileName,
+                'reason' => $failure->getMessage(),
+            ]);
+        }
     }
 
     private function session(Request $request): EditorSession

app/Http/Controllers/Api/Plugin/HeartbeatController.php

@@ -9,6 +9,8 @@
 use App\Services\RealtimeConfig;
 use Illuminate\Http\JsonResponse;
 use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Log;
+use Throwable;
 
 /**
  * Keeps the server marked as reachable and tells open windows when it comes back.
@@ -40,10 +42,18 @@ public function __invoke(Request $request, RealtimeConfig $realtime): JsonRespon
         ]);
     }
 
+    /**
+     * A heartbeat keeps the server marked reachable whether or not the open
+     * windows can be told about it, so a broken broadcaster is only logged.
+     */
     private function announceReturn(Server $server): void
     {
-        $server->editorSessions()
-            ->alive()
-            ->each(fn (EditorSession $session) => PluginStatusChanged::dispatch($session, true));
+        try {
+            $server->editorSessions()
+                ->alive()
+                ->each(fn (EditorSession $session) => PluginStatusChanged::dispatch($session, true));
+        } catch (Throwable $failure) {
+            Log::warning('Could not announce that a server came back', ['reason' => $failure->getMessage()]);
+        }
     }
 }

app/Services/RpcBridge.php

@@ -7,7 +7,9 @@
 use App\Exceptions\ServerOfflineException;
 use App\Models\Server;
 use Illuminate\Support\Facades\Cache;
+use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Str;
+use Throwable;
 
 /**
  * Request/response bridge between a browser request and the plugin.
@@ -37,7 +39,14 @@ public function call(Server $server, RpcAction $action, array $payload = []): ar
         if ($server->uses_polling) {
             $this->enqueue($server, $requestId, $action, $payload);
         } else {
-            RpcRequested::dispatch($server, $requestId, $action, $payload);
+            try {
+                RpcRequested::dispatch($server, $requestId, $action, $payload);
+            } catch (Throwable $failure) {
+                // The channel is the fast path, not the only one. Queue the
+                // request so the plugin still finds it on its next poll.
+                Log::warning('Could not publish an RPC request, queueing it', ['reason' => $failure->getMessage()]);
+                $this->enqueue($server, $requestId, $action, $payload);
+            }
         }
 
         return $this->awaitResponse($requestId);

resources/js/api/client.ts

@@ -20,8 +20,24 @@ export class ApiError extends Error {
     }
 }
 
-function csrfToken(): string {
-    return document.querySelector<HTMLMetaElement>('meta[name="csrf-token"]')?.content ?? '';
+/**
+ * Validating a session regenerates it, which rotates the CSRF token and leaves
+ * the one rendered into the page stale. The cookie is rewritten on every
+ * response, so it is the only value that stays current.
+ */
+export function csrfHeaders(): Record<string, string> {
+    const cookie = document.cookie
+        .split('; ')
+        .find(entry => entry.startsWith('XSRF-TOKEN='))
+        ?.slice('XSRF-TOKEN='.length);
+
+    if (cookie !== undefined && cookie !== '') {
+        return { 'X-XSRF-TOKEN': decodeURIComponent(cookie) };
+    }
+
+    const meta = document.querySelector<HTMLMetaElement>('meta[name="csrf-token"]')?.content;
+
+    return meta === undefined ? {} : { 'X-CSRF-TOKEN': meta };
 }
 
 async function request<T>(method: string, url: string, body?: unknown): Promise<T> {
@@ -30,7 +46,7 @@ async function request<T>(method: string, url: string, body?: unknown): Promise<
         headers: {
             Accept: 'application/json',
             'X-Requested-With': 'XMLHttpRequest',
-            'X-CSRF-TOKEN': csrfToken(),
+            ...csrfHeaders(),
             ...(body === undefined ? {} : { 'Content-Type': 'application/json' }),
         },
         body: body === undefined ? undefined : JSON.stringify(body),

resources/js/components/AdminTerminal.tsx

@@ -1,5 +1,5 @@
 import { useState } from 'react';
-import { ApiError } from '../api/client';
+import { ApiError, csrfHeaders } from '../api/client';
 
 interface CommandResult {
     success: boolean;
@@ -30,7 +30,7 @@ export function AdminTerminal({ onClose }: { onClose: () => void }) {
                 headers: {
                     Accept: 'application/json',
                     'Content-Type': 'application/json',
-                    'X-CSRF-TOKEN': csrfToken(),
+                    ...csrfHeaders(),
                     'X-Admin-Token': token,
                 },
                 body: JSON.stringify({ command }),
@@ -125,10 +125,6 @@ function render(result: CommandResult): string[] {
     return lines;
 }
 
-function csrfToken(): string {
-    return document.querySelector<HTMLMetaElement>('meta[name="csrf-token"]')?.content ?? '';
-}
-
 function readToken(): string {
     try {
         return sessionStorage.getItem(TOKEN_STORAGE_KEY) ?? '';

tests/Feature/BroadcastFailureTest.php

@@ -0,0 +1,108 @@
+<?php
+
+namespace Tests\Feature;
+
+use App\Enums\RpcAction;
+use App\Models\EditorSession;
+use App\Models\Server;
+use App\Services\RpcBridge;
+use Database\Factories\ServerFactory;
+use Illuminate\Foundation\Testing\RefreshDatabase;
+use Tests\TestCase;
+
+/**
+ * A broadcaster that cannot be reached is an infrastructure problem. It must
+ * never turn an operation that already succeeded into a failure.
+ */
+class BroadcastFailureTest extends TestCase
+{
+    use RefreshDatabase;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        // Point the broadcaster at a port nothing listens on.
+        config([
+            'broadcasting.default' => 'reverb',
+            'broadcasting.connections.reverb.key' => 'key',
+            'broadcasting.connections.reverb.secret' => 'secret',
+            'broadcasting.connections.reverb.app_id' => '1',
+            'broadcasting.connections.reverb.options' => [
+                'host' => '127.0.0.1',
+                'port' => 1,
+                'scheme' => 'http',
+                'useTLS' => false,
+            ],
+        ]);
+    }
+
+    public function test_a_save_still_succeeds_when_the_change_cannot_be_announced(): void
+    {
+        $session = $this->readySession();
+        $this->withAnsweringPlugin();
+
+        $this->actingAs($session, 'editor')
+            ->postJson(route('api.menus.store'), [
+                'platform' => 'java',
+                'fileName' => 'shop.yml',
+                'content' => 'menu: {}',
+            ])
+            ->assertOk();
+    }
+
+    public function test_a_delete_still_succeeds_when_the_change_cannot_be_announced(): void
+    {
+        $session = $this->readySession();
+        $this->withAnsweringPlugin();
+
+        $this->actingAs($session, 'editor')
+            ->deleteJson(route('api.menus.destroy'), ['platform' => 'java', 'fileName' => 'shop.yml'])
+            ->assertOk();
+    }
+
+    public function test_a_heartbeat_still_succeeds_when_the_return_cannot_be_announced(): void
+    {
+        $server = Server::factory()->offline()->create();
+        EditorSession::factory()->create(['server_id' => $server->id]);
+
+        $this->withHeaders(['X-Server-Uuid' => $server->uuid, 'X-Server-Token' => ServerFactory::TOKEN])
+            ->postJson(route('plugin.heartbeat'))
+            ->assertOk();
+
+        $this->assertTrue($server->refresh()->isOnline());
+    }
+
+    public function test_a_request_that_cannot_be_published_is_queued_for_the_next_poll(): void
+    {
+        config(['editor.rpc.timeout' => 0.2, 'editor.rpc.poll_interval_ms' => 20]);
+        $bridge = app(RpcBridge::class);
+        $server = Server::factory()->onChannel()->create();
+
+        rescue(fn () => $bridge->call($server, RpcAction::MenuList));
+
+        $this->assertCount(1, $bridge->collectPending($server));
+    }
+
+    private function readySession(): EditorSession
+    {
+        return EditorSession::factory()
+            ->for(Server::factory())
+            ->create(['confirmed' => true, 'consumed' => true, 'active' => false]);
+    }
+
+    /**
+     * A plugin that always answers, so the test isolates what it is about: the
+     * response of an operation whose broadcast could not go out.
+     */
+    private function withAnsweringPlugin(): void
+    {
+        $this->app->instance(RpcBridge::class, new class extends RpcBridge
+        {
+            public function call(Server $server, RpcAction $action, array $payload = []): array
+            {
+                return ['ok' => true, 'payload' => [], 'error' => null];
+            }
+        });
+    }
+}