Symptom: with wakeTimes=[4 AM, 9 PM, 9:15 PM], the frame rotated to a
fresh photo at 10:14 AM when the device was reconnected and polled.
The wakeTimes only governed *when* the device polled (via X-Interval-Ms);
they didn't gate whether the server picked a new image when it did.
Power-on or button-press polls would always rotate.
Fix: move the existing isDue() logic from AdvanceRotationMessageHandler
into RotationService as a public method, and gate
DeviceImageController::image so off-schedule polls return the device's
current image (which 304s when X-Current-Image-Id matches) rather than
calling advance(). The scheduler-driven handler still uses the same
isDue — both code paths now share one source of truth.
Tests:
- DeviceImageControllerTest: new test asserting an off-schedule poll
returns 304 without rotating; existing wakeTimes tests reworked to
use slot lists that always have a past slot regardless of run time.
- AdvanceRotationMessageHandlerTest: existing AR-04 through AR-07
keep covering isDue's semantics — they now go through the service.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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', [
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user