From c8836f7d657394631d7b699016d0aeb618938a7d Mon Sep 17 00:00:00 2001 From: legop3 Date: Thu, 20 Aug 2026 15:27:23 -0400 Subject: [PATCH] rover server restart desync fix hopefully --- .../src/services/assignmentService/index.js | 16 ++++- server/src/services/roverManager/index.js | 3 + .../services/roverManager/rosterLifecycle.js | 8 +++ .../services/roverManager/roverLifecycle.js | 36 +++++++++++ .../roverManager/roverLifecycle.test.js | 59 +++++++++++++++++++ server/src/services/sessionService/index.js | 28 ++++++++- 6 files changed, 146 insertions(+), 4 deletions(-) create mode 100644 server/src/services/roverManager/roverLifecycle.test.js diff --git a/server/src/services/assignmentService/index.js b/server/src/services/assignmentService/index.js index 2b9c9320..2215e4b1 100644 --- a/server/src/services/assignmentService/index.js +++ b/server/src/services/assignmentService/index.js @@ -97,8 +97,20 @@ roverManager.managerEvents.on('private', ({ roverId, open }) => { } }); -roverManager.managerEvents.on('rover', ({ action }) => { - if (action === 'removed' || action === 'upsert') { +roverManager.managerEvents.on('rover', ({ roverId, action }) => { + if (action === 'removed') { + /* + The physical rover record is the authority for current driver ownership. + Once it disappears, every assignment that names it must be released and + run through ordinary placement again. Leaving those map entries intact + lets the same id become visible after reconnect without recreating its + driver membership, which is the exact stale-UI/video-auth split this + lifecycle boundary must prevent. + */ + reassignFromRover(roverId); + return; + } + if (action === 'upsert') { reassignWaiting(); } }); diff --git a/server/src/services/roverManager/index.js b/server/src/services/roverManager/index.js index 59965b5b..9d351f23 100644 --- a/server/src/services/roverManager/index.js +++ b/server/src/services/roverManager/index.js @@ -72,6 +72,7 @@ const { } = privateAccess; const roverLifecycle = createRoverLifecycle({ + io, rovers, socketToRovers, managerEvents, @@ -86,6 +87,7 @@ const roverLifecycle = createRoverLifecycle({ const { requestControl, releaseControl, + removeRoverDrivers, isDriver, canDrive, getRoversForSocket, @@ -119,6 +121,7 @@ const rosterLifecycle = createRosterLifecycle({ normalizePrivateSafety, stopDockGuard: (...args) => stopDockGuard(...args), getControlDenialReason, + removeRoverDrivers, }); const { diff --git a/server/src/services/roverManager/rosterLifecycle.js b/server/src/services/roverManager/rosterLifecycle.js index c3209cef..5973c7bd 100644 --- a/server/src/services/roverManager/rosterLifecycle.js +++ b/server/src/services/roverManager/rosterLifecycle.js @@ -24,6 +24,7 @@ function createRosterLifecycle(deps) { isRoverVisibleToSocket, normalizePrivateSafety, stopDockGuard, + removeRoverDrivers, } = deps; function ensureRecord(id) { @@ -106,7 +107,14 @@ function createRosterLifecycle(deps) { function removeRover(id) { const record = rovers.get(id); if (!record) return; + /* + Remove the public record before emitting driver-removal events. Any + session sync caused by those events must already see this rover as + offline, while removeRoverDrivers still receives the captured record so + it can clean the reverse membership index and Socket.IO room membership. + */ rovers.delete(id); + removeRoverDrivers(id, record); stopDockGuard(id); privateButtonStates.delete(id); privateNoUsersSince.delete(id); diff --git a/server/src/services/roverManager/roverLifecycle.js b/server/src/services/roverManager/roverLifecycle.js index c180f3bd..31cbe056 100644 --- a/server/src/services/roverManager/roverLifecycle.js +++ b/server/src/services/roverManager/roverLifecycle.js @@ -3,6 +3,7 @@ // Scope: Keeps runtime behavior unchanged by reusing rover-manager state maps and injected policy helpers. function createRoverLifecycle(deps) { const { + io, rovers, socketToRovers, managerEvents, @@ -85,6 +86,40 @@ function createRoverLifecycle(deps) { managerEvents.emit('driver', { socketId: socket.id, roverId, action: 'remove' }); } + function removeRoverDrivers(roverId, removedRecord = null) { + /* + A rover connection owns the record that contains its driver set, but the + reverse socket-to-rover index outlives that record. Disconnect cleanup + must therefore remove both halves before a reconnect creates a fresh + record with the same id. Otherwise session assignment can name the rover + while video/control authorization correctly sees no driver membership. + + removedRecord is accepted because rosterLifecycle deliberately deletes + the public rover record first. Session syncs triggered by the driver + events below will consequently hide the unavailable rover immediately, + even before assignmentService finishes normal reassignment. + */ + const record = removedRecord || rovers.get(roverId); + if (!record) return []; + const driverIds = Array.from(record.drivers || []); + + driverIds.forEach((socketId) => { + const joined = socketToRovers.get(socketId); + if (joined) { + joined.delete(roverId); + if (joined.size === 0) socketToRovers.delete(socketId); + } + + const socket = io.sockets.sockets.get(socketId); + socket?.leave(record.room); + record.drivers.delete(socketId); + turnService.driverRemoved(roverId, socketId); + managerEvents.emit('driver', { socketId, roverId, action: 'remove' }); + }); + + return driverIds; + } + function isDriver(roverId, socket) { const record = rovers.get(roverId); if (!record) return false; @@ -175,6 +210,7 @@ function createRoverLifecycle(deps) { removeSocket, requestControl, releaseControl, + removeRoverDrivers, isDriver, canDrive, getRoversForSocket, diff --git a/server/src/services/roverManager/roverLifecycle.test.js b/server/src/services/roverManager/roverLifecycle.test.js new file mode 100644 index 00000000..a4fa379a --- /dev/null +++ b/server/src/services/roverManager/roverLifecycle.test.js @@ -0,0 +1,59 @@ +// Rover Manager Lifecycle Tests +// Purpose: Verifies that physical rover removal clears every ownership index before a same-id reconnect. +// Scope: Covers driver membership cleanup only; assignment placement policy remains in assignmentService. +const test = require('node:test'); +const assert = require('node:assert/strict'); +const EventEmitter = require('node:events'); +const { createRoverLifecycle } = require('./roverLifecycle'); + +test('removing a rover clears driver sets, reverse membership, rooms, and turns', () => { + const roverId = 'rover-one'; + const socketId = 'driver-one'; + const leftRooms = []; + const removedTurns = []; + const driverEvents = []; + const socket = { + id: socketId, + leave: (room) => leftRooms.push(room), + }; + const record = { + id: roverId, + room: `rover:${roverId}`, + drivers: new Set([socketId]), + }; + const rovers = new Map([[roverId, record]]); + const socketToRovers = new Map([[socketId, new Set([roverId])]]); + const managerEvents = new EventEmitter(); + managerEvents.on('driver', (event) => driverEvents.push(event)); + + const lifecycle = createRoverLifecycle({ + io: { sockets: { sockets: new Map([[socketId, socket]]) } }, + rovers, + socketToRovers, + managerEvents, + turnService: { + driverRemoved: (removedRoverId, removedSocketId) => { + removedTurns.push([removedRoverId, removedSocketId]); + }, + }, + isAdmin: () => false, + sendAlert: () => {}, + ALERT_COLOR: '#000000', + getMode: () => 'public', + getControlDenialReason: () => null, + }); + + /* Mirror rosterLifecycle's ordering: the public record is gone before the + captured record is supplied for complete membership cleanup. */ + rovers.delete(roverId); + const removedDriverIds = lifecycle.removeRoverDrivers(roverId, record); + + assert.deepEqual(removedDriverIds, [socketId]); + assert.equal(record.drivers.size, 0); + assert.equal(socketToRovers.has(socketId), false); + assert.deepEqual(leftRooms, [`rover:${roverId}`]); + assert.deepEqual(removedTurns, [[roverId, socketId]]); + assert.deepEqual(driverEvents, [ + { socketId, roverId, action: 'remove' }, + ]); +}); diff --git a/server/src/services/sessionService/index.js b/server/src/services/sessionService/index.js index ece2628f..a4908d21 100644 --- a/server/src/services/sessionService/index.js +++ b/server/src/services/sessionService/index.js @@ -135,6 +135,20 @@ function buildUserEntry(socket) { const role = getRole(socket); const assignment = assignmentService.describeAssignment(socket.id); const primaryRover = roverManager.getPrimaryRoverForSocket(socket.id); + /* + assignmentService owns automatic placement policy, while roverManager owns + actual control membership. Validate both candidate indexes before exposing + presence because neither cached direction is authoritative without the + physical rover record agreeing that this socket is one of its drivers. + */ + const verifiedPrimaryRover = primaryRover + && roverManager.isDriver(primaryRover, socket) + ? primaryRover + : null; + const verifiedAssignmentRover = assignment?.roverId + && roverManager.isDriver(assignment.roverId, socket) + ? assignment.roverId + : null; const ptzChatTarget = getPtzChatTargetForSocket(socket.id); return { socketId: socket.id, @@ -147,7 +161,7 @@ function buildUserEntry(socket) { the PTZ chat target while the socket is queued or operating so presence, queue lookup, and chat identity all agree. */ - roverId: ptzChatTarget?.roverId || primaryRover || assignment?.roverId || null, + roverId: ptzChatTarget?.roverId || verifiedPrimaryRover || verifiedAssignmentRover || null, }; } @@ -172,7 +186,17 @@ function buildSession(socket) { })); const roster = roverManager.getRosterForSocket(socket); const assignment = assignmentService.describeAssignment(socket?.id || ''); - const assignmentRoverId = filterVisibleRoverId(socket, assignment?.roverId); + /* + Visibility alone is insufficient here: a reconnected rover can be visible + before a stale assignment map has recreated actual driver membership. The + session contract consumed by every UI surface must require both visibility + and roverManager's authoritative membership check. + */ + const verifiedAssignmentRover = assignment?.roverId + && roverManager.isDriver(assignment.roverId, socket) + ? assignment.roverId + : null; + const assignmentRoverId = filterVisibleRoverId(socket, verifiedAssignmentRover); const activeDrivers = filterActiveDriversForSocket(getActiveDrivers(), socket); const turnQueues = filterTurnQueuesForSocket(getTurnQueues(), socket); const socials = features.socials && configuredSocials?.length ? configuredSocials : [];