fix: stop the screen client serving stale or blank content after a degraded pull - #12
Conversation
Review of the rate-limit fix pointed out that the retry layer cannot help with the case that actually freezes a screen: getScreen promoted every pull to the cache together with the server's relationsChecksum, including pulls that had fallen back to stale data. The next pull compared the checksums, found them equal, took the cache branch and served the degraded data - so a region, layout, template or media item stayed stale until somebody edited the content in Admin. That is what made "the next pull will retry it" untrue. - Track which relation groups a pull failed to load and store only the checksums it is entitled to. The checksums move to their own field: ContentService hashes the screen object to decide whether it changed, so rewriting relationsChecksum on the screen itself would re-emit it on every pull. - Treat a missing relationsChecksum as "must refetch" rather than defaulting it to an empty collection. Two empty maps compare equal on every key, and the API really can send nothing - the DTO getter answers null for an empty map. - Refetch a slide's template or media when the cached value is null, rather than when the checksum changed. The checksum cannot carry that signal: with the regions branch served from cache, slide and previousSlide are clones of the same cached object, so their checksums always agree. Clear slide.invalid on recovery too - Region filters invalid slides out, so refetching the template achieves nothing while the flag stands. Also from review: - Wait for a pull to finish before scheduling the next one. setInterval fired regardless, so a slow pull got a second one stacked on top of it, doubling the fan-out exactly when the backend was already struggling. Scheduling happens in a finally, and a chain id keeps a restart from leaving two chains running. This also makes stop() effective during the first pull, which start()'s finally used to ignore - a DataSync that ContentService had already discarded kept polling and dispatching content events. - Give the client config request a timeout. Every request through ApiHelper was already capped, but loadConfig awaited a raw fetch, which was the one unbounded await in a pull. Extracted the AbortController pattern out of ApiHelper into fetchWithTimeout and used it in both places. loadConfig also cleared its in-flight promise only on the fetch path, so a single stalled request pinned every later call to it and stalled every pull behind it. - Jitter a Retry-After the server sent. Every client rejected in the same second gets the same value, so honouring it verbatim re-synchronises the burst the header exists to spread. Added rather than subtracted: RFC 9110 makes it a minimum. Tests: 161 pass. New coverage for recovery after a failed region, layout, template and media request, for a screen that advertises no checksums, for the clean-pull path still using the cache, for the poll not overlapping and not outliving stop() or a restart, and for the request timeout and the config loader. Refs issue 507 (os2display/display-api-service)
…hanges Looking into why the ContentService screen hash forced PullStrategy to keep its checksums in a separate field turned up something worse: the hash was hiding a content-staleness bug, and it is the os2display#507 symptom. contentHandler had two mutually exclusive branches. Screen hash changed - emit the screen and leave delivery to each Region's useEffect([region]) firing regionReady. Screen hash unchanged - push updateRegion directly. Neither covers the common case. A pull whose layout checksum is unchanged reuses lastestScreenData.layoutData by reference, so layoutData.regions[i] are the same objects pull after pull. Edit a slide and the server bumps relationsChecksum.regions, which is inside the hashed screen, so the emit branch is taken - but [region] is Object.is-equal, the effect never runs, regionReady never fires, and updateRegion is never called with the fresh regionData. checkScheduling does not rescue it either; it re-reads the stale cached region. The screen keeps showing the old content until something forces a layout refetch. - Drop the ContentService hash and always emit. Re-emitting is cheap: Screen keys its regions by id, so React reconciles them in place and playback state survives, and Region defers any slide-list swap to the playlist wrap-around. - Push updateRegion for every region on every content event, before the emit, so delivery no longer depends on a component noticing a prop changed. - Add ScheduleService.regionReady, an unconditional replay of the cached slides. The push happens before React has mounted a newly added region, so that dispatch is lost; updateRegion's hash gate would then report no change on every later pull and the region would stay blank. - Make the regionReady effect in Region and TouchRegion mount-only. Hanging it off region object identity is what produced the bug. The ScheduleService hash stays. Unlike the ContentService one it gates state churn into the region components rather than a React re-render, and it is the only thing that notices a feed changed - feedData is refetched every pull and carries no checksum. Its input drops the region, though: findScheduledSlides derives the slides from it, so hashing both serialised the full region payload twice per pull for nothing. Also fixed while wiring this up: - checkScheduling wrote this.regions[regionId].slide, singular. Harmless before, but regionReady reads cached.slides, so a region mounting after a schedule change would have been handed the pre-change set. - App and Screen re-ran their useEffect([screen]) on every emit, costing a /v2/tenants/{id} request per pull - the fan-out os2display#507 is about - and a colour-scheme teardown that strips color-scheme-* off the html element before an async config load puts it back. Both now key on the value they use. - Corrected the lastestScreenChecksums comment, which justified the separate field with a re-emit that would not have happened on every pull anyway. The field is right for a different reason: relationsChecksum reaches the client as the server sent it, not as what a degraded pull is entitled to compare against. ContentService and ScheduleService had no test coverage. The new suites cover the regression directly; 10 of their 14 cases fail against the previous behaviour.
getScreen stored a null layoutData when the layout request failed and emitted it to the client. Screen builds its regions from layoutData.regions, so a null unmounts every region: regionRemoved clears each region's scheduling interval and cached content, and the next successful pull mounts them again from scratch, restarting playback from the first slide. The screen is blank in the meantime. Regions and campaigns already degrade by keeping what the previous pull had - content one pull out of date beats a black region, and a rejected request says nothing about what should be shown. Layout now makes the same trade. The fallback is deliberately narrow. It is skipped when the previous pull was in campaign mode, which holds a synthetic full-screen layout with one hardcoded region that no playlist in a normal pull is scheduled for, and when the screen's layout IRI has changed since, where the cached regions would not match the regionData this pull just fetched. In both cases layoutData stays null, as before. report.layout is unaffected, so the checksum is still withheld and the next pull retries the layout.
tuj
changed the title
Nulling templateData marks the slide invalid, and Region drops invalid slides, so a few seconds of rate limiting during the template phase emptied a region as soon as its playlist wrapped. Keep the template the previous pull loaded instead, the same trade getRegions and the layout branch already make, and only mark a slide invalid when no pull has ever loaded its template. Failed template requests are now cached for the rest of the pull too. Slides share a handful of templates, so without that every slide using an unreachable template paid the full retry budget again, stretching a short outage across the whole pull. The region emptied with the fallback image already suppressed, because checkForEmptyContent counted slides the regions had dropped. Count with the test Region applies when it renders, shared between the two so they cannot drift apart, and re-check when a region is removed - losing the last region with content otherwise left contentEmpty stuck at false.
getRegions hands back playlists mapped straight off the API, which have never carried slidesData, so the cloneDeep in getSlidesForRegions had nothing to preserve: a region that loaded fine but whose /slides request failed lost that playlist's content for a whole pull. Look the playlist up in the previous pull and keep its slides, the same trade getRegions and the layout branch already make. Both lookups match on @id rather than on position. Playlists and slides get added, removed and reordered between pulls, and since the template fallback added in f4fe5c9 reads previousSlide.templateData, a positional pairing could hand a slide another slide's template after an editor reordered a playlist - rendering the wrong template rather than none.
renderSlide was called as an argument in Slide's render, so a template this build cannot resolve threw before Slide's ErrorBoundary mounted. The throw escaped past Slide entirely and hit the region's boundary, which has no error handler and never resets - so one unrenderable slide replaced its whole region with the error fallback until the client was reloaded. Rendering the template from a child component puts the throw inside the boundary meant to contain it: that slide shows the fallback, slideError fires, and the region moves on to the next slide.
Every template's render code and config is bundled into the client, and the only thing rendering wants from templateData is the id it looks the bundled module up by. That id is the last segment of the template IRI the slide already carries, and the API's template resource exposes nothing else a screen can use - the resources column that carried asset paths in 2.7 is deprecated and unexposed. So the request read back a value the pull already had, at the cost of one round trip per template per pull. It also cost a failure mode. A throttled template request nulled templateData, Region drops slides marked invalid, and the region emptied itself as soon as its playlist wrapped (os2display#507). The negative cache, the last-known-good fallback and the invalid-slide bookkeeping added to survive that all go with the request. A slide can still be invalid, but only when its template IRI carries no id at all. An id with no module in this build is the remaining way a template can be missing, and that is the render-time throw the slide's error boundary now contains.
turegjorup
left a comment
turegjorup
left a comment
There was a problem hiding this comment.
Not blocking. Nothing in the diff is wrong, but it collides with os2display#546 in a way that needs a named resolution, and one new shared helper has a hole.
Verified rather than read off the description: task test:unit is 34 files / 193 tests green; prettier and markdownlint clean. I reverted SlideTemplate to an inline renderSlide(...) call and both slide-template-error.test.jsx tests fail (expected null not to be null), so the boundary fix and its tests are real. ScreenProvider.php:60-62 builds screen.regions from $layout->getRegions() — the same layout screen.layout names — which is what makes the mount-only regionReady effects and the unconditional emit safe.
I went looking for a phantom-interval leak in ScheduleService.regions/intervals (a regionData key that never mounts would register a setInterval nothing ever clears, and count as content) and disproved it — the two region sources cannot diverge for the reason above, and the layout fallback guards the one case that could with previous.layout === newScreen.layout. Worth recording since it is the kind of thing that looks reachable from the diff alone.
Items with no diff line to attach to:
ErrorBoundaryhas no reset path (error-boundary.jsx:11-14) —hasErroris set and never cleared. Commit 6 recovers by unmount/remount as the region advances, which works; but the region-level boundary inregion.jsxstill has no handler and no reset, so anything throwing outside theSlidesubtree kills that region until the client reloads. This PR narrows that exposure rather than causing it — follow-up, not a blocker.- The fallback renders
componentStackand a hardcoded Danish heading on a public display (error-boundary.jsx:38-44, "Seneste log hændelser"). Commit 6 makes it appear more often (per slide rather than per region), so it is worth deciding whether a screen in a public space should show a stack trace at all. screen.jsx:103-111—<Fragment key={…}>wrapping children that each carry the samekey; the inner ones are redundant. Pre-existing.touch-region.jsx:132-140—<div role="button" onKeyDown={slideDone}>fires on any key, not just Enter/Space, and "LUK" is hardcoded. Pre-existing; this PR only touched the effect deps.
No src/Dto/ or src/State/ change, so the api-resource reviewer does not apply, and no generated-api.ts change for the RTK cache reviewer. Commit 7 does change how templateData is derived, so the slide-template contract is the one place a second opinion would help — though templates.js:72 reading only slide?.templateData?.id is strong evidence { id } is sufficient.
| > | ||
| <ErrorBoundary errorHandler={handleError}> | ||
| {renderSlide(slide, run, slideDone)} | ||
| <SlideTemplate slide={slide} run={run} slideDone={slideDone} /> |
There was a problem hiding this comment.
Highest-severity item here — merge with os2display#546 has one line where either side alone is wrong.
Not a defect in this diff, but it silently disables a shipped fix. git merge-tree against fix/region-run-id-collision conflicts only in CHANGELOG.md and this file. Everything else auto-merges, including os2display#546's MIN_SLIDE_DWELL_MS and slideDoneAfterMinimumDwell, which land in the merged file alongside SlideTemplate:
<<<<<<< fix/issue-507-extra-fixes
<SlideTemplate slide={slide} run={run} slideDone={slideDone} />
=======
{renderSlide(slide, run, slideDoneAfterMinimumDwell)}
>>>>>>> fix/region-run-id-collision
Taking this side keeps the boundary fix and drops the dwell guard, reinstating the unbounded effect → slideDone → new run id → effect loop os2display#546 exists to stop.
Fix: resolve to <SlideTemplate slide={slide} run={run} slideDone={slideDoneAfterMinimumDwell} /> — neither side verbatim.
Test: os2display#546's four dwell tests in region.test.jsx render through Slide, so a wrong resolution fails the suite. The net exists; whoever merges second just needs to know this line is not a pick-one.
Same file, minor: SlideTemplate's new JSDoc says @param {string} props.run - Timestamp for when to run the slide., which is the wording os2display#546 is sweeping out. Post-merge the file carries {number} Run id… on Slide and {string} Timestamp… on SlideTemplate, five lines apart.
| * @param {object} slide The slide to test. | ||
| * @returns {boolean} True if the slide should be rendered. | ||
| */ | ||
| export default function isRenderableSlide(slide) { |
There was a problem hiding this comment.
Should fix — an absent slide counts as renderable.
slide?.invalid !== true returns true for null and undefined. Probed directly:
isRenderableSlide: {"null":true,"undefined":true,"invalid":false,"valid":true}
region of absent slides counts as content: true
So a region whose slides array holds only absent entries reports content, checkForEmptyContent leaves contentEmpty false, the fallback image stays suppressed and the screen is black — the exact failure this helper was extracted to close. Region also passes the absent entry through its filter to setCurrentSlide(undefined), so it renders nothing while claiming content.
To be straight about the limit: the logic hole is proven, the reachability is not. playlist.slidesData = result.value.results.map((ps) => ps.slide) yields such an array only if the API omits slide on a PlaylistSlide, which I did not confirm it can. The optional chaining here shows absent input was anticipated; the return value just does not act on it.
Fix: return slide != null && slide.invalid !== true;
Test: null/undefined cases on this helper, plus one checkForEmptyContent case with slides: [null].
| this.currentScreen.regionData[regionId], | ||
| ); | ||
| } | ||
| this.scheduleService.regionReady(regionId); |
There was a problem hiding this comment.
Should fix — a region that remounts without an intervening pull shows nothing for a poll interval.
This used to read this.currentScreen.regionData[regionId]; it now delegates to ScheduleService.regionReady, which reads a cache regionRemoved has just deleted. On a component swap for the same region id (default ↔ touch-buttons, both keyed region["@id"] in screen.jsx:103-111) React runs the old cleanup before the new mount, so the order is delete-then-read and the region gets nothing until the next pull — up to interval (30s default) of blank region on a public screen.
I can see this is deliberate and pinned: schedule-service.test.js:233-245 asserts recorder.sends stays at 1 across regionRemoved + regionReady. Raising it anyway because ContentService still holds currentScreen, so closing the window is nearly free, and a blank region is the precise symptom os2display#507 is about. Low frequency (needs a layout region-type change) and self-healing, hence should-fix rather than blocker.
Fix: fall back to this.scheduleService.updateRegion(regionId, this.currentScreen?.regionData?.[regionId]) when regionReady finds no cache.
Test: extend the existing regionRemoved test to assert a remount after removal is served from the current screen.
| layout is kept instead, unless it belongs to a campaign or to a layout the screen has since been | ||
| moved away from. | ||
| - Fixed a failed template request dropping slides that were playing perfectly well, where a few | ||
| seconds of rate limiting emptied a region as soon as its playlist wrapped (#507). The last known |
There was a problem hiding this comment.
Should fix — this entry describes behaviour commit 7 removed.
"The last known good template is kept instead, and a template that cannot be reached is now requested once per pull rather than once per slide." At HEAD there is no template request, no last-known-good fallback and no negative cache — and this contradicts the entry a few lines above, "Removed the screen client's template requests."
The PR description flags the code-level overlap between commits 4 and 7; the changelog was not updated with it.
Fix: drop the template clause, keeping the empty-content half that is still live.
| - Fixed a failed slides request emptying a playlist whose region had loaded fine, which lost that | ||
| playlist's content for a whole pull (#507). The slides the previous pull loaded are kept instead. | ||
| - Fixed the screen client pairing slides and playlists with the previous pull by position rather | ||
| than by id, so reordering a playlist in the admin could pair a slide with another slide's |
There was a problem hiding this comment.
Same cause as the entry above: at HEAD the @id matching only protects media, since the template fallback it was justifying is gone. "another slide's template or media" should lose template or.
| */ | ||
| async fetchConfig(nowTimestamp) { | ||
| try { | ||
| const response = await fetchWithTimeout(`/config/client`); |
There was a problem hiding this comment.
Nit — fetchConfig does not check response.ok, so a 4xx/5xx that returns a JSON body is cached as config for 15 minutes and appStorage.setApiUrl(configData.apiEndpoint) then stores undefined. Pre-existing behaviour, but the function was rewritten here so it is cheap to add: if (!response.ok) throw new Error(response.status) falls straight into the existing last-known-good path.
| } | ||
|
|
||
| if (report.regions) { | ||
| stored.regions = null; |
There was a problem hiding this comment.
Nit — clearing to null makes a group this pull failed indistinguishable from a group the server genuinely sent null for, so a failed pull's degraded data would take the cache branch on the next one. Only reachable when the screen has no regions at all, where there is nothing to serve — so a comment noting that rather than a change.
…kaerme' into fix/issue-507-extra-fixes # Conflicts: # CHANGELOG.md # assets/client/components/slide.jsx
…ontent Addresses the review on PR #12, plus one fault found while tracing it. - Drop playlist rows whose slide relation is absent. They mapped to undefined, and getScreen's relations loop then wrote templateData onto them - a throw that reached pull()'s catch and aborted the whole pull, so one broken row in one playlist froze the screen on its last content, every pull. - Treat an absent slide as unrenderable. isRenderableSlide answered true for null, so a region holding only absent slides counted as having content, which suppressed the fallback image and left the screen black. - Restore a region's content and scheduling interval when it remounts. Changing a region's type swaps the component behind an unchanged region id, and React runs the outgoing cleanup before the incoming effects, so regionReady arrived with the cache regionRemoved had just dropped. - Reject an error response as client config rather than caching the error body for the whole config interval with apiEndpoint undefined. - Withhold a failed group's checksum with a unique symbol rather than null, which compared equal to the null the API answers for an empty relation map and so took the cached branch it exists to prevent. - Drop the changelog's claims about template requests and the template fallback, both removed in f5e0e97.
…kaerme' into fix/issue-507-extra-fixes
2 participants
Link to issue
os2display#507 — Problem med visning af spillelister på skærme.
Stacked on
fix/issue-507-problem-med-visning-af-spillelister-paa-skaerme(nginx rate-limit + retry/fan-out); this is the follow-up work from reviewing those.Description
A screen could go stale — or blank — and stay that way until somebody edited the content in Admin. Two root causes: the client cached pulls that had partly failed, so "the next pull will retry it" was never true, and it relied on React object identity to deliver content it had fetched successfully.
Seven commits, each carrying its full reasoning in the commit message:
b0ebacdRetry-After.f788541useEffect([region]), and a pull serving the layout from cache hands back the same region objects every time — so edited slides never reached the region.617cac2f4fe5c9f5e0e97.)2448a41/slidesrequest fails.f575782f5e0e97No backend change:
/v2/templatesstays for the Admin template picker, so there is no/v2BC question.Checklist
Additional comments or questions
Reading the commits in order tells one story — stop caching what failed, stop depending on object identity to deliver what succeeded, stop letting one failed request blank a region, then stop making the request at all — but they are independently reviewable.
One note if you read commit messages:
2448a41justifies matching on@idvia the template fallback fromf4fe5c9, whichf5e0e97then removes. AtHEADthe reason is the media cache.