diff --git a/migrations/Version20260507230000.php b/migrations/Version20260507230000.php new file mode 100644 index 0000000..1e7d13f --- /dev/null +++ b/migrations/Version20260507230000.php @@ -0,0 +1,27 @@ +addSql('ALTER TABLE device ADD current_rendered_at TIMESTAMP(0) WITHOUT TIME ZONE DEFAULT NULL'); + $this->addSql("COMMENT ON COLUMN device.current_rendered_at IS '(DC2Type:datetime_immutable)'"); + } + + public function down(Schema $schema): void + { + $this->addSql('ALTER TABLE device DROP COLUMN current_rendered_at'); + } +} diff --git a/src/Controller/DeviceImageController.php b/src/Controller/DeviceImageController.php index a8cd102..0a757a1 100644 --- a/src/Controller/DeviceImageController.php +++ b/src/Controller/DeviceImageController.php @@ -68,23 +68,9 @@ class DeviceImageController extends AbstractController return $r; } - // 304: device already has this image — skip the binary transfer and redraw. - // Both the image and the orientation it was last rendered at must match; - // otherwise the device's cached .bin is stale and we must re-send. - if ($image->getId() === $currentImageId - && $device->getCurrentImageOrientation() === $device->getOrientation()) { - $this->logger->info('device.poll.no_change', [ - 'device_id' => $device->getId(), - 'mac' => $mac, - 'image_id' => $image->getId(), - 'interval_ms' => $intervalMs, - ]); - $r = new Response(null, Response::HTTP_NOT_MODIFIED); - $r->headers->set('X-Image-Id', (string) $image->getId()); - $r->headers->set('X-Interval-Ms', (string) $intervalMs); - return $r; - } - + // Asset lookup is needed before the 304 check so we can compare its + // rendered_at timestamp — otherwise a re-cropped image (same id, same + // orientation, new bytes) would be incorrectly served as 304. $asset = $em->getRepository(RenderedAsset::class)->findOneBy([ 'image' => $image, 'deviceModel' => $device->getModel(), @@ -105,6 +91,27 @@ class DeviceImageController extends AbstractController return $r; } + // 304: device already has this image — skip the binary transfer and redraw. + // The image id, the orientation we last served at, AND the asset's + // rendered_at must all match. A re-render (e.g. after re-crop) advances + // rendered_at, so the device's cached bytes are stale and we re-send. + $renderedAt = $asset->getRenderedAt(); + if ($image->getId() === $currentImageId + && $device->getCurrentImageOrientation() === $device->getOrientation() + && $renderedAt !== null + && $device->getCurrentRenderedAt()?->getTimestamp() === $renderedAt->getTimestamp()) { + $this->logger->info('device.poll.no_change', [ + 'device_id' => $device->getId(), + 'mac' => $mac, + 'image_id' => $image->getId(), + 'interval_ms' => $intervalMs, + ]); + $r = new Response(null, Response::HTTP_NOT_MODIFIED); + $r->headers->set('X-Image-Id', (string) $image->getId()); + $r->headers->set('X-Interval-Ms', (string) $intervalMs); + return $r; + } + $binPath = $this->projectDir . '/' . $asset->getFilePath(); if (!file_exists($binPath)) { $this->logger->error('device.poll.file_missing', [ @@ -118,10 +125,10 @@ class DeviceImageController extends AbstractController return $r; } - // Record the orientation we're serving the image at so the next poll's - // 304 check can detect a flip and force a re-fetch. Flushed via the - // controller's EM (markSeen() above already dirties the row). + // Record the orientation and rendered_at we're serving at so the next + // poll's 304 check can detect a flip or a re-render and force a re-fetch. $device->setCurrentImageOrientation($device->getOrientation()); + $device->setCurrentRenderedAt($renderedAt); $em->flush(); $this->logger->info('device.poll.served', [ diff --git a/src/Entity/Device.php b/src/Entity/Device.php index fa2beb6..c434b2b 100644 --- a/src/Entity/Device.php +++ b/src/Entity/Device.php @@ -74,6 +74,15 @@ class Device #[ORM\Column(nullable: true, enumType: Orientation::class)] private ?Orientation $currentImageOrientation = null; + /** + * RenderedAsset.rendered_at timestamp at the moment we last served this + * device its current image. Lets the 304 cache check detect a re-render + * (e.g. user re-cropped the image) — the image id and orientation may + * still match, but the bytes have changed and we must re-send. + */ + #[ORM\Column(nullable: true)] + private ?\DateTimeImmutable $currentRenderedAt = null; + public function __construct() { $this->linkedAt = new \DateTimeImmutable(); @@ -122,4 +131,7 @@ class Device public function getCurrentImageOrientation(): ?Orientation { return $this->currentImageOrientation; } public function setCurrentImageOrientation(?Orientation $o): static { $this->currentImageOrientation = $o; return $this; } + + public function getCurrentRenderedAt(): ?\DateTimeImmutable { return $this->currentRenderedAt; } + public function setCurrentRenderedAt(?\DateTimeImmutable $t): static { $this->currentRenderedAt = $t; return $this; } } diff --git a/tests/Functional/Controller/DeviceImageControllerTest.php b/tests/Functional/Controller/DeviceImageControllerTest.php index 6b30d0c..7c178bf 100644 --- a/tests/Functional/Controller/DeviceImageControllerTest.php +++ b/tests/Functional/Controller/DeviceImageControllerTest.php @@ -66,7 +66,8 @@ class DeviceImageControllerTest extends AppWebTestCase ->setDeviceModel(DeviceModel::V1) ->setOrientation(Orientation::Landscape) ->setStatus(RenderStatus::Ready) - ->setFilePath(self::BIN_PATH); + ->setFilePath(self::BIN_PATH) + ->setRenderedAt(new \DateTimeImmutable()); $this->em()->persist($asset); } @@ -166,10 +167,13 @@ class DeviceImageControllerTest extends AppWebTestCase $image = $setup['image']; $imageId = $image->getId(); - $device->setLockedImage($image); // Simulate the device having already received this image at the current - // orientation — the 304 path now requires this to match too. + // orientation and rendered_at — the 304 path now requires all three + // (image id, orientation, rendered_at) to match. + $asset = $this->em()->getRepository(RenderedAsset::class)->findOneBy(['image' => $image]); + $device->setLockedImage($image); $device->setCurrentImageOrientation(Orientation::Landscape); + $device->setCurrentRenderedAt($asset->getRenderedAt()); $this->em()->flush(); $this->client->request('GET', '/api/device/' . self::MAC . '/image', [], [], [ @@ -211,6 +215,34 @@ class DeviceImageControllerTest extends AppWebTestCase $this->assertResponseStatusCodeSame(200); } + public function test_re_render_returns_200_even_when_image_id_and_orientation_match(): void + { + $setup = $this->createTestSetup(); + $deviceId = $setup['device']->getId(); + $imageId = $setup['image']->getId(); + + // Seed: device receives the image once, server stores currentRenderedAt. + $this->client->request('GET', '/api/device/' . self::MAC . '/image'); + $this->assertResponseStatusCodeSame(200); + + // Simulate a re-render: the asset's rendered_at advances (e.g. user + // re-cropped the image, RenderImageMessageHandler ran again). + $this->em()->clear(); + $asset = $this->em()->getRepository(RenderedAsset::class)->findOneBy([ + 'image' => $this->em()->find(Image::class, $imageId), + 'orientation' => Orientation::Landscape, + ]); + $asset->setRenderedAt(new \DateTimeImmutable('+1 minute')); + $this->em()->flush(); + + // Same image id, same orientation — but the bytes have changed, so + // the 304 cache must invalidate and the device must re-fetch. + $this->client->request('GET', '/api/device/' . self::MAC . '/image', [], [], [ + 'HTTP_X-Current-Image-Id' => (string) $imageId, + ]); + $this->assertResponseStatusCodeSame(200); + } + public function test_poll_advances_current_image(): void { $setup = $this->createTestSetup();