diff --git a/src/Controller/DeviceImageController.php b/src/Controller/DeviceImageController.php index 6908709..0d361cf 100644 --- a/src/Controller/DeviceImageController.php +++ b/src/Controller/DeviceImageController.php @@ -83,8 +83,18 @@ class DeviceImageController extends AbstractController // the second flush triggers a second publish to keep it accurate. $this->mercure->publishDevice((int) $device->getId(), $this->serializer->serialize($device)); - // Locked image bypasses rotation entirely. - $image = $device->getLockedImage() ?? $this->rotation->advance($device); + // Locked image bypasses rotation entirely. Otherwise, only advance the + // rotation when the device's configured schedule says it's due — + // off-schedule polls (boot, button-mash, scheduler-driven status check) + // hand back the device's current image without rotating, so the user + // doesn't see surprise refreshes between configured wake times. + if ($device->getLockedImage() !== null) { + $image = $device->getLockedImage(); + } elseif ($this->rotation->isDue($device)) { + $image = $this->rotation->advance($device); + } else { + $image = $device->getCurrentImage(); + } if ($image === null) { $this->logger->info('device.poll.no_image', [ diff --git a/src/MessageHandler/AdvanceRotationMessageHandler.php b/src/MessageHandler/AdvanceRotationMessageHandler.php index c3f4e11..ebacd39 100644 --- a/src/MessageHandler/AdvanceRotationMessageHandler.php +++ b/src/MessageHandler/AdvanceRotationMessageHandler.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace App\MessageHandler; use App\Entity\Device; -use App\Entity\DeviceImageHistory; use App\Message\AdvanceRotationMessage; use App\Service\RotationService; use Doctrine\ORM\EntityManagerInterface; @@ -26,7 +25,7 @@ class AdvanceRotationMessageHandler $devices = $this->em->getRepository(Device::class)->findAll(); foreach ($devices as $device) { - if (!$this->isDue($device)) { + if (!$this->rotationService->isDue($device)) { $this->logger->debug('rotation.not_due', ['device_id' => $device->getId()]); continue; } @@ -37,57 +36,4 @@ class AdvanceRotationMessageHandler } } } - - private function isDue(Device $device): bool - { - $wakeTimes = $device->getWakeTimes(); - if (!empty($wakeTimes)) { - $tz = new \DateTimeZone($device->getTimezone()); - $now = new \DateTimeImmutable('now', $tz); - - // Find the most recent wake time that has already passed today. - // If none have hit yet, the next slot is in the future — not due. - $boundary = null; - foreach ($wakeTimes as $minutes) { - $candidate = $now->setTime((int) ($minutes / 60), $minutes % 60, 0); - if ($candidate <= $now && ($boundary === null || $candidate > $boundary)) { - $boundary = $candidate; - } - } - if ($boundary === null) { - return false; - } - - $entry = $this->em->createQueryBuilder() - ->select('h') - ->from(DeviceImageHistory::class, 'h') - ->where('h.device = :device') - ->andWhere('h.servedAt >= :wakeTime') - ->setParameter('device', $device) - ->setParameter('wakeTime', $boundary) - ->setMaxResults(1) - ->getQuery() - ->getOneOrNullResult(); - - return $entry === null; - } - - // Interval-based: due if last history is older than rotationIntervalMinutes - $last = $this->em->createQueryBuilder() - ->select('h') - ->from(DeviceImageHistory::class, 'h') - ->where('h.device = :device') - ->setParameter('device', $device) - ->orderBy('h.servedAt', 'DESC') - ->setMaxResults(1) - ->getQuery() - ->getOneOrNullResult(); - - if ($last === null) { - return true; - } - - $elapsed = (new \DateTimeImmutable())->getTimestamp() - $last->getServedAt()->getTimestamp(); - return $elapsed >= ($device->getRotationIntervalMinutes() * 60); - } } diff --git a/src/Service/RotationService.php b/src/Service/RotationService.php index 4c313a0..8b35841 100644 --- a/src/Service/RotationService.php +++ b/src/Service/RotationService.php @@ -19,6 +19,73 @@ class RotationService private readonly LoggerInterface $logger, ) {} + /** + * True when the device's configured schedule says a rotation should occur + * right now. The contract: + * + * - In wakeTimes mode: there must be a wake time that has already passed + * today AND no history entry exists since that boundary. + * - In interval mode: the most recent history entry (if any) must be at + * least `rotationIntervalMinutes` old. A device with no history is due. + * + * Used by both AdvanceRotationMessageHandler (scheduler-driven) and + * DeviceImageController (poll-driven) so off-schedule polls don't + * surprise the user with an unrequested rotation — see the user-visible + * regression the original poll-rotates-unconditionally code caused. + */ + public function isDue(Device $device): bool + { + $wakeTimes = $device->getWakeTimes(); + if (!empty($wakeTimes)) { + $tz = new \DateTimeZone($device->getTimezone()); + $now = new \DateTimeImmutable('now', $tz); + + // Find the most recent wake time that has already passed today. + // If none have hit yet, the next slot is in the future — not due. + $boundary = null; + foreach ($wakeTimes as $minutes) { + $candidate = $now->setTime((int) ($minutes / 60), $minutes % 60, 0); + if ($candidate <= $now && ($boundary === null || $candidate > $boundary)) { + $boundary = $candidate; + } + } + if ($boundary === null) { + return false; + } + + $entry = $this->em->createQueryBuilder() + ->select('h') + ->from(DeviceImageHistory::class, 'h') + ->where('h.device = :device') + ->andWhere('h.servedAt >= :wakeTime') + ->setParameter('device', $device) + ->setParameter('wakeTime', $boundary) + ->setMaxResults(1) + ->getQuery() + ->getOneOrNullResult(); + + return $entry === null; + } + + // Interval-based: due if last history is older than rotationIntervalMinutes. + $last = $this->em->createQueryBuilder() + ->select('h') + ->from(DeviceImageHistory::class, 'h') + ->where('h.device = :device') + ->setParameter('device', $device) + ->orderBy('h.servedAt', 'DESC') + ->setMaxResults(1) + ->getQuery() + ->getOneOrNullResult(); + + if ($last === null) { + return true; + } + + $elapsed = (new \DateTimeImmutable())->getTimestamp() - $last->getServedAt()->getTimestamp(); + return $elapsed >= ($device->getRotationIntervalMinutes() * 60); + } + /** * Select the next image for the device, record history, update currentImage. * Returns null if no ready images exist in the pool. diff --git a/tests/Functional/Controller/DeviceImageControllerTest.php b/tests/Functional/Controller/DeviceImageControllerTest.php index 862c2b9..50cc757 100644 --- a/tests/Functional/Controller/DeviceImageControllerTest.php +++ b/tests/Functional/Controller/DeviceImageControllerTest.php @@ -336,13 +336,15 @@ class DeviceImageControllerTest extends AppWebTestCase $this->assertResponseStatusCodeSame(204); } - // When wakeTimes is set, X-Interval-Ms should be > 0 and <= 24h in ms + // When wakeTimes is set and one slot has already passed today, X-Interval-Ms + // should be > 0 and <= 24h. Use [0] (midnight) so the slot is always past + // regardless of wall-clock time at test execution. public function test_wake_times_interval_used_when_set(): void { $setup = $this->createTestSetup(); $device = $setup['device']; - $device->setWakeTimes([3 * 60])->setTimezone('UTC'); + $device->setWakeTimes([0])->setTimezone('UTC'); $this->em()->flush(); $this->client->request('GET', '/api/device/' . self::MAC . '/image'); @@ -354,15 +356,16 @@ class DeviceImageControllerTest extends AppWebTestCase } // With multiple wake times, X-Interval-Ms must point to the *earliest* - // upcoming time, not just the first in the list. + // upcoming time, not just the first in the list. Use evenly-spaced slots + // including 00:00 so at least one is always past regardless of run time. public function test_wake_times_picks_earliest_upcoming(): void { $setup = $this->createTestSetup(); $device = $setup['device']; - // Use a fixed UTC tz; with three slots evenly spread, the gap to the - // next slot can never exceed 24h / count = 8h. - $device->setWakeTimes([6 * 60, 14 * 60, 22 * 60])->setTimezone('UTC'); + // 00:00, 08:00, 16:00 — gap to next slot is at most 8h regardless of + // when the test runs, and at least one is always in the past. + $device->setWakeTimes([0, 8 * 60, 16 * 60])->setTimezone('UTC'); $this->em()->flush(); $this->client->request('GET', '/api/device/' . self::MAC . '/image'); @@ -373,6 +376,35 @@ class DeviceImageControllerTest extends AppWebTestCase $this->assertLessThanOrEqual(8 * 60 * 60 * 1000, $intervalMs); } + // Off-schedule poll (wakeTimes set, but the most-recent past slot has + // already been served): the controller MUST NOT advance the rotation. + // It returns 304 (device's X-Current-Image-Id matches the existing + // currentImage) instead of a fresh 200. + public function test_off_schedule_poll_returns_304_without_advancing(): void + { + $setup = $this->createTestSetup(); + $device = $setup['device']; + + // First poll establishes a current image while wakeTimes is unset + // (so isDue is true and rotation runs). + $this->client->request('GET', '/api/device/' . self::MAC . '/image'); + $this->assertResponseStatusCodeSame(200); + $imageId = $this->client->getResponse()->headers->get('X-Image-Id'); + + // Now configure wakeTimes such that the most-recent past slot has + // already been served (the poll above wrote a history entry just now, + // and 00:00 is the most-recent past slot in UTC). + $device->setWakeTimes([0])->setTimezone('UTC'); + $this->em()->flush(); + + // A second poll right now is "off-schedule" — server should NOT + // advance, and since we report the current image, server should 304. + $this->client->request('GET', '/api/device/' . self::MAC . '/image', [], [], [ + 'HTTP_X_Current_Image_Id' => $imageId, + ]); + $this->assertResponseStatusCodeSame(304); + } + // Returns 204 when RenderedAsset has Ready status but filePath is null (device.poll.no_asset path) public function test_returns_204_when_ready_asset_has_null_file_path(): void {