fix: use current time when replaying resync state after CasparCG reconnect#436
Conversation
WalkthroughReplays of device states are now computed using a single captured "now" and prior states are clamped to that current time before being reapplied. Per-channel FPS detection and safer templateData merging added for CasparCG; tests updated; minor lodash -> native array checks in Pharos. Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/timeline-state-resolver/src/conductor.ts (1)
820-837:⚠️ Potential issue | 🟡 MinorFix potential crash in
resyncDeviceStateswhen called before device state is initialized.If
resyncStatesevent fires when a device first connects (before any timeline state has been sent viasetDatastoreorsetTimeline),this._deviceStates[deviceId]will beundefined, causing aTypeErrorwhen spreading on lines 826–827.Add a guard to return early if the device has no state:
🛡️ Proposed fix
private resyncDeviceStates(deviceId: string) { this._actionQueue .add(() => { + if (!this._deviceStates[deviceId]?.length) return const now = this.getCurrentTime()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/conductor.ts` around lines 820 - 837, The resyncDeviceStates method can crash when this._deviceStates[deviceId] is undefined; add a guard at the start of resyncDeviceStates to return early if this._deviceStates[deviceId] is falsy (or not an array) before using spread/reverse/filter, so the subsequent logic (creating toBeFilled, calling _getReplayStateWithCurrentTime and connectionManager.getConnection(deviceId)?.device.handleState) only runs when device state exists.
🧹 Nitpick comments (1)
packages/timeline-state-resolver/src/conductor.ts (1)
797-812:nowis recomputed per device — intentional?
nowis captured inside thefor (const deviceId of affectedDevices)loop (Line 798), so each device may see a slightly different current time. If consistent cross-device timing is desired, moveconst now = this.getCurrentTime()before the loop. This is a minor nit — the difference is negligible in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/conductor.ts` around lines 797 - 812, The current time is recomputed per device inside the for (const deviceId of affectedDevices) loop by calling this.getCurrentTime(), causing tiny inconsistencies; move the timestamp capture outside the loop (compute const now = this.getCurrentTime() before iterating affectedDevices) and then use that same now when calling this._getReplayStateWithCurrentTime(s, now) and when invoking connectionManager.getConnection(deviceId)?.device.handleState(...), ensuring all devices use a consistent reference time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/timeline-state-resolver/src/conductor.ts`:
- Around line 820-837: The resyncDeviceStates method can crash when
this._deviceStates[deviceId] is undefined; add a guard at the start of
resyncDeviceStates to return early if this._deviceStates[deviceId] is falsy (or
not an array) before using spread/reverse/filter, so the subsequent logic
(creating toBeFilled, calling _getReplayStateWithCurrentTime and
connectionManager.getConnection(deviceId)?.device.handleState) only runs when
device state exists.
---
Nitpick comments:
In `@packages/timeline-state-resolver/src/conductor.ts`:
- Around line 797-812: The current time is recomputed per device inside the for
(const deviceId of affectedDevices) loop by calling this.getCurrentTime(),
causing tiny inconsistencies; move the timestamp capture outside the loop
(compute const now = this.getCurrentTime() before iterating affectedDevices) and
then use that same now when calling this._getReplayStateWithCurrentTime(s, now)
and when invoking
connectionManager.getConnection(deviceId)?.device.handleState(...), ensuring all
devices use a consistent reference time.
|
I have updated this with the latest fix. Note, I built the fix and tested it on Release 51 and have ported it from there using AI so we need to verify everything works correctly on this master branch before merging. PR update: additional branch-alignment fixes (fix-only from Release 51) I added a small set of targeted hardening changes that were needed when porting the reconnect-resync fix from Release 51 onto main. These are intentionally narrow and focused on runtime safety/correctness. Core reconnect/resync timing fix (primary change) What changed: replayed “state-before-now” states are normalized to current resolver time during replay/resync. What changed: added a stricter guard before mutating enable.start for “now” triggers. What changed: detect/cache channel FPS from Caspar INFO, use detected FPS when building state, and improve template-data merge guards. What changed: explicit normalization of string-or-array params (names, vars) using Array.isArray. Scope is intentionally small: replay timing correctness + defensive runtime guards. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/timeline-state-resolver/src/integrations/casparCG/index.ts (1)
666-673:clearAllChannelsbypasses the newgetChannelFpspriority chain.Unlike the
connecthandler (line 147),clearAllChannelsstill writesfps: obj.frameRatedirectly and never callsupdateDetectedFpsFromInfo, so after aclearAllChannelsaction the_currentStateFPS doesn't reflectinitOptions.fpsor detected-FPS priority—creating a divergence fromconvertStateToCasparwhich usesgetChannelFps.♻️ Suggested fix for consistency
+ this.updateDetectedFpsFromInfo(response.data as InfoEntry[] | undefined) this.clearStates() this._currentState = { channels: {} } response.data.forEach((obj) => { this._currentState.channels[obj.channel] = { channelNo: obj.channel, videoMode: this.getVideMode(obj), - fps: obj.frameRate, + fps: this.getChannelFps(obj.channel), layers: {}, } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/integrations/casparCG/index.ts` around lines 666 - 673, clearAllChannels currently sets fps directly from obj.frameRate, bypassing the fps priority chain; modify clearAllChannels so it does not assign fps: obj.frameRate but instead invokes the existing priority logic (call updateDetectedFpsFromInfo with the new channel state and the info object, or set fps via getChannelFps/getVideMode flow used elsewhere) so the _currentState FPS matches convertStateToCaspar and the connect handler behavior; update references in clearAllChannels to use updateDetectedFpsFromInfo (and/or getChannelFps) for each channel created.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/timeline-state-resolver/src/integrations/casparCG/index.ts`:
- Around line 666-673: clearAllChannels currently sets fps directly from
obj.frameRate, bypassing the fps priority chain; modify clearAllChannels so it
does not assign fps: obj.frameRate but instead invokes the existing priority
logic (call updateDetectedFpsFromInfo with the new channel state and the info
object, or set fps via getChannelFps/getVideMode flow used elsewhere) so the
_currentState FPS matches convertStateToCaspar and the connect handler behavior;
update references in clearAllChannels to use updateDetectedFpsFromInfo (and/or
getChannelFps) for each channel created.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/timeline-state-resolver/src/conductor.ts (1)
746-751: Pre-existing in-place.reverse()mutation in_setDeviceState.Lines 748 and 750 both call
.reverse()directly onthis._deviceStates[deviceId], which mutates the array in-place. The first.reverse()flips the array for the.find(), and the second.reverse()flips it back for the.filter(). While the double-reverse cancels out, this is fragile — any code that runs concurrently or any interruption between these calls could see the array in a reversed state.The new
_getReplayStatesForDevicehelper correctly uses[...deviceStates].reverse(). Consider applying the same pattern here for consistency, though this is a pre-existing issue outside the PR scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/conductor.ts` around lines 746 - 751, The code in _setDeviceState mutates this._deviceStates[deviceId] by calling .reverse() directly; replace both in-place reversals with non-mutating copies (e.g. [...this._deviceStates[deviceId]].reverse() or this._deviceStates[deviceId].slice().reverse()) so the .find() and .filter() operate on reversed copies instead of mutating the stored array—mirror the approach used in _getReplayStatesForDevice to avoid race/consistency issues.packages/timeline-state-resolver/src/integrations/casparCG/index.ts (1)
1013-1025: FPS fallback chain is reasonable but the "first detected" fallback is non-deterministic.
Object.values(this._detectedChannelFps).find((fps) => fps > 0)on line 1021 returns the first value from the object's iteration order. While V8 iterates integer keys in ascending numeric order, this is an implementation detail. If channels genuinely have different frame rates, this fallback could silently pick the wrong one.This is a low-risk edge case since most CasparCG installations use uniform FPS across channels, but worth documenting with a comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/integrations/casparCG/index.ts` around lines 1013 - 1025, The getChannelFps method uses a fallback chain that ultimately picks the first positive FPS from Object.values(this._detectedChannelFps), which is non-deterministic across JS engines; add a concise comment above the getChannelFps function (and near the Object.values(...) use) stating that the "first detected" fallback depends on object iteration order, may pick an arbitrary channel's FPS if channels differ, and that this is intentional/acceptable for now (or how to change it to a deterministic choice such as preferring the lowest/highest/index 0 if desired); reference getChannelFps and this._detectedChannelFps in the comment so future readers can find and understand the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/timeline-state-resolver/src/integrations/casparCG/index.ts`:
- Around line 999-1025: The interlaced FPS detection in
updateDetectedFpsFromInfo is using channelRate for interlaced content which
yields field-rate (e.g., 50 for 1080i50) instead of frame-rate; update the logic
in updateDetectedFpsFromInfo (which populates this._detectedChannelFps based on
InfoEntry) to always prefer entry.frameRate over entry.channelRate (i.e.,
detected = entry.frameRate || entry.channelRate) and remove the conditional on
entry.interlaced so getChannelFps and downstream frame-based calculations use
canonical frame-rate values.
---
Nitpick comments:
In `@packages/timeline-state-resolver/src/conductor.ts`:
- Around line 746-751: The code in _setDeviceState mutates
this._deviceStates[deviceId] by calling .reverse() directly; replace both
in-place reversals with non-mutating copies (e.g.
[...this._deviceStates[deviceId]].reverse() or
this._deviceStates[deviceId].slice().reverse()) so the .find() and .filter()
operate on reversed copies instead of mutating the stored array—mirror the
approach used in _getReplayStatesForDevice to avoid race/consistency issues.
In `@packages/timeline-state-resolver/src/integrations/casparCG/index.ts`:
- Around line 1013-1025: The getChannelFps method uses a fallback chain that
ultimately picks the first positive FPS from
Object.values(this._detectedChannelFps), which is non-deterministic across JS
engines; add a concise comment above the getChannelFps function (and near the
Object.values(...) use) stating that the "first detected" fallback depends on
object iteration order, may pick an arbitrary channel's FPS if channels differ,
and that this is intentional/acceptable for now (or how to change it to a
deterministic choice such as preferring the lowest/highest/index 0 if desired);
reference getChannelFps and this._detectedChannelFps in the comment so future
readers can find and understand the behavior.
| private updateDetectedFpsFromInfo(infoEntries: InfoEntry[] | undefined) { | ||
| if (!infoEntries?.length) return | ||
|
|
||
| for (const entry of infoEntries) { | ||
| const channelRate = Number(entry.channelRate) || 0 | ||
| const frameRate = Number(entry.frameRate) || 0 | ||
| const detected = entry.interlaced ? channelRate || frameRate : frameRate || channelRate | ||
|
|
||
| if (detected > 0) { | ||
| this._detectedChannelFps[entry.channel] = detected | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private getChannelFps(channel?: number): number { | ||
| if (this.initOptions?.fps && this.initOptions.fps > 0) return this.initOptions.fps | ||
|
|
||
| if (channel !== undefined) { | ||
| const channelFps = this._detectedChannelFps[channel] | ||
| if (channelFps && channelFps > 0) return channelFps | ||
| } | ||
|
|
||
| const firstDetectedFps = Object.values(this._detectedChannelFps).find((fps) => fps > 0) | ||
| if (firstDetectedFps) return firstDetectedFps | ||
|
|
||
| return 25 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's understand the repository structure
git ls-files | grep -E "(casparcg|InfoEntry)" | head -20
# Find InfoEntry type definition
rg -n "interface InfoEntry|type InfoEntry" --type=ts -A 15
# Check imports in the current file
head -50 packages/timeline-state-resolver/src/integrations/casparCG/index.ts | grep -E "import|from"Repository: Sofie-Automation/sofie-timeline-state-resolver
Length of output: 498
🏁 Script executed:
#!/bin/bash
# Get full import statements from the file
sed -n '1,50p' packages/timeline-state-resolver/src/integrations/casparCG/index.ts
# Check the mock to understand InfoEntry structure
cat packages/timeline-state-resolver/src/__mocks__/casparcg-connection.ts
# Check the type definitions
cat packages/timeline-state-resolver-types/src/integrations/casparcg.tsRepository: Sofie-Automation/sofie-timeline-state-resolver
Length of output: 15578
🏁 Script executed:
#!/bin/bash
# Search for uses of _detectedChannelFps
rg -n "_detectedChannelFps" --type=ts -B 2 -A 2
# Search for how getChannelFps is used
rg -n "getChannelFps" --type=ts -B 2 -A 2
# Search for seek-related calculations
rg -n "seek" --type=ts -i -B 2 -A 2 | head -100
# Search for millisecond to frame conversion
rg -n "/ fps|/ this\.fps|* fps|\* this\.fps" --type=ts -B 2 -A 2 | head -50Repository: Sofie-Automation/sofie-timeline-state-resolver
Length of output: 13780
🏁 Script executed:
#!/bin/bash
# Search for fps usage in calculations (fixed regex)
rg -n "fps\s*[\*/]|[\*/]\s*fps" --type=ts -B 2 -A 2 | head -80
# Look for frame-related calculations
rg -n "frame.*fps|fps.*frame" --type=ts -i -B 2 -A 2 | head -80
# Check the CasparCGState type definition for how fps is used
rg -n "class CasparCGState|interface.*CasparCGState" --type=ts -A 20 | head -100Repository: Sofie-Automation/sofie-timeline-state-resolver
Length of output: 1580
🏁 Script executed:
#!/bin/bash
# Check if casparcg-state is imported and how
rg -n "from.*casparcg-state|import.*CasparCGState" --type=ts -B 2 -A 5 | head -50
# Look for actual calculations using channel.fps or this.fps in casparCG integration
rg -n "channel\.fps|this\.fps|getChannelFps\(" packages/timeline-state-resolver/src/integrations/casparCG/index.ts -B 3 -A 3
# Check the casparcg-state package.json or README
fd "casparcg-state" --type d | head -5Repository: Sofie-Automation/sofie-timeline-state-resolver
Length of output: 3055
🏁 Script executed:
#!/bin/bash
# Check package.json for casparcg-state version
cat package.json | jq '.dependencies["casparcg-state"]'
# Look for tests with interlaced content
rg -n "interlaced.*true|channelRate.*frameRate" --type=ts -B 3 -A 3
# Check for tests that verify fps behavior
rg -n "fps.*50|fps.*25|frameRate.*channelRate" packages/timeline-state-resolver/src/integrations/casparCG/__tests__/ -B 2 -A 2
# Look at the test file more carefully for interlaced scenarios
fd "casparcg.spec.ts" -exec wc -l {} \;Repository: Sofie-Automation/sofie-timeline-state-resolver
Length of output: 4219
🏁 Script executed:
#!/bin/bash
# Let's check the test file more closely for any assertions about interlaced fps
cat packages/timeline-state-resolver/src/integrations/casparCG/__tests__/casparcg.spec.ts | head -250
# Also check if there are any issues or TODOs mentioning interlaced or fps semantics
rg -n "TODO|FIXME|BUG|interlaced" packages/timeline-state-resolver/src/integrations/casparCG/ --type=ts -B 1 -A 1Repository: Sofie-Automation/sofie-timeline-state-resolver
Length of output: 7320
Fix interlaced FPS detection logic.
Line 1005 incorrectly uses channelRate (field rate) for interlaced content when it should use frameRate (frame rate). In broadcast terminology, 1080i50 means 50 fields/second, which equals 25 frames/second. Using the field rate (50) for the fps parameter will double all frame-based calculations (seek, duration conversions), producing incorrect playout timing.
Change line 1005 from:
const detected = entry.interlaced ? channelRate || frameRate : frameRate || channelRate
to:
const detected = frameRate || channelRate
Both interlaced and progressive formats should use frameRate as the canonical FPS value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/timeline-state-resolver/src/integrations/casparCG/index.ts` around
lines 999 - 1025, The interlaced FPS detection in updateDetectedFpsFromInfo is
using channelRate for interlaced content which yields field-rate (e.g., 50 for
1080i50) instead of frame-rate; update the logic in updateDetectedFpsFromInfo
(which populates this._detectedChannelFps based on InfoEntry) to always prefer
entry.frameRate over entry.channelRate (i.e., detected = entry.frameRate ||
entry.channelRate) and remove the conditional on entry.interlaced so
getChannelFps and downstream frame-based calculations use canonical frame-rate
values.
There was a problem hiding this comment.
Current seek behavior for CasparCG requires field rate for interlaced content for proper seeking rather than frame rate. I decided the safest thing would be to patch here rather than patch casparcg and break other clients that use seek. I verified this in 1080i5994, but it probably holds true for 1080i50. Someone in PAL land should verify this. If casparcg differs between the two then it needs patched instead.
There was a problem hiding this comment.
@dedicatedbroadcastsolutions, thank you for the clarification! You're absolutely right—CasparCG uses field rate motion for interlaced video formats, so using channelRate (field rate) for interlaced content is the correct approach for proper seek behavior. I apologize for the confusion in my initial review.
Your decision to handle this requirement in TSR rather than patching CasparCG itself makes sense to maintain compatibility with other clients that depend on the current seek behavior.
Suggestion: Consider adding a brief comment above line 1005 to document this non-obvious requirement for future maintainers:
// For interlaced formats, use channelRate (field rate) as CasparCG's seek behavior
// is field-based, not frame-based. E.g., 1080i50 uses 50 (fields/sec) not 25 (frames/sec).
const detected = entry.interlaced ? channelRate || frameRate : frameRate || channelRateThis will help prevent similar confusion in future code reviews.
✏️ Learnings added
Learnt from: dedicatedbroadcastsolutions
Repo: Sofie-Automation/sofie-timeline-state-resolver PR: 436
File: packages/timeline-state-resolver/src/integrations/casparCG/index.ts:999-1025
Timestamp: 2026-02-21T00:34:10.968Z
Learning: In CasparCG integrations within packages/timeline-state-resolver/src/integrations/casparCG/index.ts, when detecting FPS from InfoEntry data for interlaced formats, use `channelRate` (field rate) rather than `frameRate` because CasparCG's seek behavior is field-based for interlaced content. For example, 1080i50 should use 50 fields/second, not 25 frames/second, to ensure correct seek positioning.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
AI assesment of adjustments. We still need to human review and validate this PR. Validation update after addressing CodeRabbit notes: Kept the interlaced CasparCG FPS behavior intentionally field-rate based [channelRate], and added clarifying comments to avoid future confusion. Updated CasparCG test expectations to match the intentional interlaced field-rate behavior. yarn workspace timeline-state-resolver run -T jest casparcg.spec.ts --runInBand --forceExit ✅ (23/23) Fails in OBS tests under Node 24 with obs-websocket-js close-event crash ([TypeError: Cannot read properties of undefined (reading 'reason')] while running [obs.spec.ts] |
|
Hey @dedicatedbroadcastsolutions ! Thanks for this PR. You seem to not have used the PR-template when creating the PR. The template includes a few control questions to ensure the PR is ready for review. Before we spend time reviewing this, could you verify that:
If you haven't verified the functionality of the PR (for example, using Quick-TSR), please mark it as a draft so we know it's not ready for review just yet. |
|
Sorry, I tried to follow the PR template but apparently failed on a few points. I have tested the functionality of the version of this based on release 52 as that is what the develop branch of superconductor currently uses. I have not yet tested this PR directly since rebasing it on main as noted in my comment above. I believe the relevant tests have been updated, but will verify that unit tests are included in that. Code comments have been updated, but I am not familiar with how to update system documentation. I need to look into that and could use some guidance. I will try to set up a Quick TSR test before submitting for official review. |
2 participants
About the Contributor
This pull request is posted on behalf of Dedicated Broadcast Solutions.
Type of Contribution
Bug fix
Related Issue
Fixes #435
Current Behavior
When a CasparCG device disconnects and reconnects, TSR may replay the last state-before-now using a stale historical state time.
For absolute-time schedules, the reconnected device can resume near its old playback position and remain offset from continuously running devices by approximately the outage duration.
New Behavior
During resync/state replay, TSR normalizes replayed past states to current resolver time before sending them to device handlers.
This ensures reconnect playback aligns with current schedule time and uses the most current available resolver offset.
Root Cause
Resync replay reused original state.time for a past state instead of refreshing to current time at replay, causing seek/play calculations to be based on stale timing context.
Implementation Summary
Added replay-time normalization for past states in Conductor resync/datastore replay paths.
Reused a single helper to keep behavior consistent across replay entry points.
Added regression test to verify that resync replay uses current time for state-before-now.
Testing
Ran focused test suite:
yarn run -T jest [conductor.spec.ts] --runInBand --forceExit
Added regression coverage for resync replay timing behavior.
Impact
Eliminates reconnect drift caused by stale replay timestamps.
Most visible improvement in CasparCG absolute-time playout scenarios.
No intended behavior change for future states; only replayed past-state timestamp handling is adjusted.
Risk Assessment
Low-to-moderate risk:
Change is targeted to replay/resync timing logic.
Covered by focused regression test.
Does not alter core timeline resolution algorithm.
Notes
Related PRs #413 and #424 were reviewed and do not address this reconnect stale-time replay path.
Overview
Fixes a bug where the Timeline State Resolver (TSR) could replay stale historical state timestamps during device reconnect/resync (notably CasparCG), causing resumed playback to be offset by the outage duration. Replayed past-state timestamps are now normalized to the current resolver time during Conductor resync/datastore replay paths.
Problem
On device disconnect + reconnect, TSR could replay a prior "state-before-now" using the original historical state.time rather than the current resolver time (including current clock offset). This caused reconnected devices to resume at an old playback position and remain offset from continuously running devices by roughly the disconnect duration.
Solution
Changes
packages/timeline-state-resolver/src/conductor.ts
packages/timeline-state-resolver/src/tests/conductor.spec.ts
packages/timeline-state-resolver/src/integrations/casparCG/index.ts
packages/timeline-state-resolver/src/integrations/pharos/connection.ts
Tests
Impact
Reconnected devices now realign to current schedule time and resolver offset when replaying past states, eliminating reconnect drift for absolute-time playout (e.g., CasparCG). Future-state scheduling and core timeline resolution are unchanged.
Risk
Low-to-moderate: changes are focused and small, limited to replay/resync paths and defensive input guards. Covered by a targeted regression test. Fixes #435.