Fix sector locking issue with NPCs

NPCs were frequently aborting uncleanly after being podded due to the
decision about what action to take being made before a sector lock is
acquired (e.g. it will try to perform its selected trade, but can't
find the port anymore because it's no longer in the same sector).

We fix this by checking that we are still in the same sector after we
acquire the sector lock. If not, skip the chosen action and let it
try again (with a new action and a new lock).

Note that we could have moved the sector locking to before the action
selection. This would ensure that executing the selected action is
always valid, but it would not avoid the sector consistency check after
lock acquisition. So we keep the lock acquisition where it is to keep
lock time to a minimum.

src/lib/Default/smr.inc.php

@@ -366,7 +366,6 @@ function do_voodoo(): never {
 		// Get the nominal player information (this may change after locking).
 		// We don't force a reload here in case we don't need to lock.
 		$player = $session->getPlayer();
-		$sectorID = $player->getSectorID();
 
 		if (!$session->ajax //AJAX should never do anything that requires a lock.
 			//&& ($var->file == 'current_sector.php' || $var->file == 'map_local.php') //Neither should CS or LM and they gets loaded a lot so should reduce lag issues with big groups.
@@ -392,7 +391,7 @@ function do_voodoo(): never {
 
 				// Reload player now that we have a lock.
 				$player = $session->getPlayer(true);
-				if ($player->getSectorID() !== $sectorID) {
+				if ($player->getSectorID() !== $lock->getSectorID()) {
 					// Player sector changed after reloading! Release lock and try again.
 					$lock->release();
 					do_voodoo();

src/lib/Smr/SectorLock.php

@@ -126,6 +126,13 @@ public function acquire(int $gameID, int $accountID, int $sectorID): bool {
 		throw new Exception('Sector lock acquisition timed out!');
 	}
 
+	public function getSectorID(): int {
+		if ($this->sectorID === null) {
+			throw new Exception('Must acquire lock before calling this method!');
+		}
+		return $this->sectorID;
+	}
+
 	/**
 	 * @phpstan-assert-if-true !null $this->lockID
 	 */

src/tools/npc/npc.php

@@ -232,14 +232,22 @@ function processContainer(PlayerPageProcessor $container): never {
 			throw new Exception('We are executing the same container twice?');
 		}
 	}
-	clearCaches(); //Clear caches of anything we have used for decision making before processing container and getting lock.
 	$previousContainer = $container;
 	debug('Executing container', $container);
 	// The next "page request" must occur at an updated time.
 	Epoch::update();
 	$session->setCurrentVar($container);
 
-	SectorLock::getInstance()->acquireForPlayer($player);
+	// Acquire a lock in the sector where we chose our action
+	$lock = SectorLock::getInstance();
+	$lock->acquireForPlayer($player);
+	clearCaches(); // do not retain anything from before lock acquisition
+	if ($session->getPlayer(true)->getSectorID() !== $lock->getSectorID()) {
+		// NPC sector was modified externally (e.g. back to HQ in a pod) while
+		// deciding what to do, so skip this action and select a new action.
+		throw new ForwardAction();
+	}
+
 	$container->build($player);
 }
 

test/SmrTest/lib/SectorLockTest.php

@@ -127,4 +127,17 @@ public function test_isActive(): void {
 		self::assertFalse($lock->isActive());
 	}
 
+	public function test_getSectorID(): void {
+		$lock = SectorLock::getInstance();
+		$lock->acquire(1, 2, 3);
+		self::assertSame($lock->getSectorID(), 3);
+	}
+
+	public function test_getSectorID_before_acquire(): void {
+		$lock = SectorLock::getInstance();
+		$this->expectException(Exception::class);
+		$this->expectExceptionMessage('Must acquire lock before calling this method!');
+		$lock->getSectorID();
+	}
+
 }