From b24d453ad1f32cdfc5694772ef6f342d7807ae7d Mon Sep 17 00:00:00 2001 From: legop3 Date: Tue, 11 Aug 2026 21:34:29 -0400 Subject: [PATCH] redo and move rover ranking a little --- .../src/services/assignmentService/index.js | 36 +++------- .../assignmentService/roverRanking.js | 65 +++++++++++++++++ .../assignmentService/roverRanking.test.js | 69 +++++++++++++++++++ 3 files changed, 143 insertions(+), 27 deletions(-) create mode 100644 server/src/services/assignmentService/roverRanking.js create mode 100644 server/src/services/assignmentService/roverRanking.test.js diff --git a/server/src/services/assignmentService/index.js b/server/src/services/assignmentService/index.js index aa15644f..3a4928ef 100644 --- a/server/src/services/assignmentService/index.js +++ b/server/src/services/assignmentService/index.js @@ -7,6 +7,7 @@ const logger = require('../../globals/logger').child('assignment'); const { MODES, getMode, modeEvents } = require('../modeManager'); const { roleEvents, getRole, isAdmin, isLockdownAdmin } = require('../roleService'); const roverManager = require('../roverManager'); +const { compareRoversForAssignment } = require('./roverRanking'); const socketRefs = new Map(); // socketId -> socket const assignments = new Map(); // socketId -> roverId @@ -241,35 +242,16 @@ function pickRover(socket, options = {}) { if (candidates.length === 0) { return null; } - const dockedRank = (rover) => { - if (!rover) return 0; - if (rover.docked === true) return -1; - if (rover.docked === false) return 1; - const sensors = rover.lastSensor?.decoded || rover.lastSensor?.sensors || null; - const docked = sensors?.chargingSources?.homeBase; - if (docked === true) return -1; - if (docked === false) return 1; - return 0; - }; - const idleRank = (rover) => (rover?.drivers?.size === 0 ? 1 : 0); - const compare = (a, b) => { - const aEmpty = idleRank(a); - const bEmpty = idleRank(b); - if (aEmpty !== bEmpty) return bEmpty - aEmpty; - const aDockRank = dockedRank(a); - const bDockRank = dockedRank(b); - if (aEmpty === 1 && aDockRank !== bDockRank) { - return bDockRank - aDockRank; - } - if (a.drivers.size !== b.drivers.size) { - return a.drivers.size - b.drivers.size; - } - return bDockRank - aDockRank; - }; - candidates.sort(compare); + /* + Eligibility is resolved above, while this shared comparator owns only the + requested placement order: undocked-and-empty, battery, then driver count. + Keeping those concerns separate prevents a ranking change from weakening + lock, private-rover, role, or mode access checks. + */ + candidates.sort(compareRoversForAssignment); const best = candidates[0]; if (!best) return null; - const bestTier = candidates.filter((entry) => compare(entry, best) === 0); + const bestTier = candidates.filter((entry) => compareRoversForAssignment(entry, best) === 0); if (!bestTier.length) return best; return bestTier[Math.floor(Math.random() * bestTier.length)] || best; } diff --git a/server/src/services/assignmentService/roverRanking.js b/server/src/services/assignmentService/roverRanking.js new file mode 100644 index 00000000..e8d9959b --- /dev/null +++ b/server/src/services/assignmentService/roverRanking.js @@ -0,0 +1,65 @@ +// Rover assignment ranking +// Purpose: Ranks otherwise eligible rovers using the fleet's assignment priorities. +// Scope: Contains only deterministic comparison logic; access checks and the final random tie-break remain in assignmentService. + +function readDockedState(rover) { + /* + The rover record normally exposes the server's canonical docked state. The + sensor fallback covers the short interval where telemetry has arrived but + the derived top-level field has not yet been synchronized. Unknown docking + state deliberately remains unknown instead of being treated as undocked. + */ + if (rover?.docked === true || rover?.docked === false) return rover.docked; + const sensors = rover?.lastSensor?.decoded || rover?.lastSensor?.sensors || null; + const homeBase = sensors?.chargingSources?.homeBase; + return homeBase === true || homeBase === false ? homeBase : null; +} + +function driverCount(rover) { + /* + Production rover records use a Set. Returning a safe high-level count here + keeps ranking predictable for partially initialized records and makes the + comparator straightforward to exercise with small test fixtures. + */ + return Number.isFinite(rover?.drivers?.size) ? rover.drivers.size : 0; +} + +function batteryPercentage(rover) { + /* + percentDisplay is the canonical server-normalized percentage used by the + rest of the application. Missing or invalid telemetry receives no invented + percentage; the comparator places unknown batteries after every known one. + */ + const percentage = rover?.batteryState?.percentDisplay; + return Number.isFinite(percentage) ? percentage : null; +} + +function compareRoversForAssignment(left, right) { + /* + An undocked rover with nobody assigned is the most useful placement because + it starts a fresh driving session without adding another user to a queue. + Both conditions must be true to receive this first-priority rank. + */ + const leftReadyAndEmpty = readDockedState(left) === false && driverCount(left) === 0; + const rightReadyAndEmpty = readDockedState(right) === false && driverCount(right) === 0; + if (leftReadyAndEmpty !== rightReadyAndEmpty) return leftReadyAndEmpty ? -1 : 1; + + const leftBattery = batteryPercentage(left); + const rightBattery = batteryPercentage(right); + const leftHasBattery = leftBattery != null; + const rightHasBattery = rightBattery != null; + if (leftHasBattery !== rightHasBattery) return leftHasBattery ? -1 : 1; + if (leftHasBattery && leftBattery !== rightBattery) return rightBattery - leftBattery; + + /* + Battery-equivalent rovers are balanced by current assignment load. Returning + zero after this comparison is intentional: assignmentService randomly picks + within that exact best tier so stable Map insertion order does not create a + permanent favorite rover. + */ + return driverCount(left) - driverCount(right); +} + +module.exports = { + compareRoversForAssignment, +}; diff --git a/server/src/services/assignmentService/roverRanking.test.js b/server/src/services/assignmentService/roverRanking.test.js new file mode 100644 index 00000000..7e4261de --- /dev/null +++ b/server/src/services/assignmentService/roverRanking.test.js @@ -0,0 +1,69 @@ +// Rover assignment ranking tests +// Purpose: Locks the operator-defined rover priority order against accidental comparator regressions. +// Scope: Tests pure ranking only; assignment side effects and access policy remain owned by their existing services. +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { compareRoversForAssignment } = require('./roverRanking'); + +function rover({ id, docked, battery, drivers = 0 }) { + /* + Set size matches the production rover contract without introducing socket or + rover-manager dependencies into these focused ordering tests. + */ + return { + id, + docked, + batteryState: battery == null ? null : { percentDisplay: battery }, + drivers: new Set(Array.from({ length: drivers }, (_, index) => `${id}-driver-${index}`)), + }; +} + +function rankedIds(entries) { + return entries.sort(compareRoversForAssignment).map((entry) => entry.id); +} + +test('an undocked empty rover outranks every rover that is docked or occupied', () => { + const result = rankedIds([ + rover({ id: 'occupied-high', docked: false, battery: 100, drivers: 1 }), + rover({ id: 'docked-high', docked: true, battery: 100 }), + rover({ id: 'ready-empty', docked: false, battery: 20 }), + ]); + + assert.equal(result[0], 'ready-empty'); +}); + +test('battery percentage ranks rovers after undocked-and-empty readiness', () => { + const result = rankedIds([ + rover({ id: 'low', docked: false, battery: 35 }), + rover({ id: 'high', docked: false, battery: 90 }), + rover({ id: 'middle', docked: false, battery: 60 }), + ]); + + assert.deepEqual(result, ['high', 'middle', 'low']); +}); + +test('known battery percentage outranks missing battery telemetry', () => { + const result = rankedIds([ + rover({ id: 'unknown', docked: true, battery: null }), + rover({ id: 'known', docked: true, battery: 5 }), + ]); + + assert.deepEqual(result, ['known', 'unknown']); +}); + +test('driver count breaks a battery-percentage tie', () => { + const result = rankedIds([ + rover({ id: 'busy', docked: false, battery: 70, drivers: 3 }), + rover({ id: 'less-busy', docked: false, battery: 70, drivers: 1 }), + ]); + + assert.deepEqual(result, ['less-busy', 'busy']); +}); + +test('exactly equivalent rovers remain tied for random selection by assignmentService', () => { + const left = rover({ id: 'left', docked: false, battery: 80, drivers: 1 }); + const right = rover({ id: 'right', docked: false, battery: 80, drivers: 1 }); + + assert.equal(compareRoversForAssignment(left, right), 0); + assert.equal(compareRoversForAssignment(right, left), 0); +});