From c96151fe2f7b319ca5c8c9662ff2687d33eae83d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 09:53:18 +0000 Subject: [PATCH] fix(timer): make all pause maths wrap-aware pausedAt is now an instant, but getExpectedFinish, getCurrent and getRuntimeOffset were still deriving the pause duration from the time of day clock (clock - pausedAt), which mixes units and produces a garbage result on every paused timer. - derive the ongoing pause from the instant in a single helper, so the duration is correct also when the pause spans midnight - getCurrent no longer special cases a paused timer: it discounts the ongoing pause from the elapsed time, which keeps the midnight correction that the paused branch was missing - reuse timeCore.elapsedTime instead of a local copy of the same logic - type getCurrent as a Duration and getElapsed as Maybe to document that these are durations, never points in time - keep _now in parity with the clock in roll() - type the restore point pausedAt as an instant and reject restore points which still carry a time of day, resuming those would corrupt the timer Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_019WA6Z38L7VY2oD6B4vG8hn --- .../src/services/__tests__/timerUtils.test.ts | 95 +++++++++++++++++-- .../__tests__/restore.parser.test.ts | 18 +++- .../__tests__/restore.service.test.ts | 12 +-- .../restore-service/restore.parser.ts | 5 +- .../services/restore-service/restore.type.ts | 3 +- apps/server/src/services/timerUtils.ts | 66 ++++++------- .../src/stores/__tests__/runtimeState.test.ts | 4 + apps/server/src/stores/runtimeState.ts | 11 ++- 8 files changed, 159 insertions(+), 55 deletions(-) diff --git a/apps/server/src/services/__tests__/timerUtils.test.ts b/apps/server/src/services/__tests__/timerUtils.test.ts index 41b993bec..587be0578 100644 --- a/apps/server/src/services/__tests__/timerUtils.test.ts +++ b/apps/server/src/services/__tests__/timerUtils.test.ts @@ -1,7 +1,6 @@ import { EndAction, Instant, Playback, TimeOfDay, TimeStrategy, TimerPhase, TimerType } from 'ontime-types'; import { MILLIS_PER_HOUR, MILLIS_PER_MINUTE, MILLIS_PER_SECOND, dayInMs, millisToString } from 'ontime-utils'; -import * as timeCore from '../../lib/time-core/timeCore.js'; import type { RuntimeState } from '../../stores/runtimeState.js'; import { findDayOffset, @@ -17,6 +16,13 @@ import { const asTimeOfDay = (value: number): RuntimeState['clock'] => value as RuntimeState['clock']; +/** + * Instants are epoch based, we anchor them to a fixed day to keep the tests deterministic + */ +const midnight = new Date(2026, 0, 1).getTime(); +const instantToday = (timeOfDay: number): Instant => (midnight + timeOfDay) as Instant; +const instantYesterday = (timeOfDay: number): Instant => (midnight - dayInMs + timeOfDay) as Instant; + describe('getElapsed()', () => { it('returns active elapsed time from startedAt without add-time adjustments', () => { const state = { @@ -54,12 +60,12 @@ describe('getElapsed()', () => { it('uses the current pause start while paused', () => { const state = { clock: 10 * MILLIS_PER_MINUTE, - _now: timeCore.toInstant((10 * MILLIS_PER_MINUTE) as TimeOfDay, timeCore.now()), + _now: instantToday(10 * MILLIS_PER_MINUTE), timer: { startedAt: 2 * MILLIS_PER_MINUTE, }, _timer: { - pausedAt: timeCore.toInstant((7 * MILLIS_PER_MINUTE) as TimeOfDay, timeCore.now()), + pausedAt: instantToday(7 * MILLIS_PER_MINUTE), pausedDuration: 1 * MILLIS_PER_MINUTE, }, } as RuntimeState; @@ -146,6 +152,28 @@ describe('getExpectedFinish()', () => { const calculatedFinish = getExpectedFinish(state); expect(calculatedFinish).toBe(31); }); + it('adds the time of an ongoing pause which spans midnight', () => { + const state = { + eventNow: { + timeEnd: 1 * MILLIS_PER_HOUR, // 01:00 + timerType: TimerType.CountDown, + }, + clock: 3 * MILLIS_PER_MINUTE, // 00:03 (after midnight) + _now: instantToday(3 * MILLIS_PER_MINUTE), + timer: { + addedTime: 0, + duration: 2 * MILLIS_PER_HOUR, + startedAt: 23 * MILLIS_PER_HOUR, // 23:00 + }, + _timer: { + pausedAt: instantYesterday(23 * MILLIS_PER_HOUR + 58 * MILLIS_PER_MINUTE), // 23:58, before midnight + hasFinished: false, + }, + } as RuntimeState; + + // the 5 minute pause pushes the finish from 01:00 to 01:05 + expect(getExpectedFinish(state)).toBe(1 * MILLIS_PER_HOUR + 5 * MILLIS_PER_MINUTE); + }); it('added time could be negative', () => { const state = { eventNow: { @@ -402,6 +430,51 @@ describe('getCurrent()', () => { expect(current).toBe(35); }); + it('is frozen while paused', () => { + const state = { + eventNow: { + timeEnd: 10, + timerType: TimerType.CountDown, + }, + clock: 5, + _now: instantToday(5), + timer: { + addedTime: 0, + duration: 10, + startedAt: 0, + }, + _timer: { + pausedAt: instantToday(2), // we have been paused for 3ms (see clock) + hasFinished: false, + }, + } as RuntimeState; + + // the timer holds the value it had when we paused + expect(getCurrent(state)).toBe(8); + }); + it('is frozen while paused, even if the pause spans midnight', () => { + const state = { + eventNow: { + timeEnd: 1 * MILLIS_PER_HOUR, // 01:00 + timerType: TimerType.CountDown, + }, + clock: 3 * MILLIS_PER_MINUTE, // 00:03 (after midnight) + _now: instantToday(3 * MILLIS_PER_MINUTE), + timer: { + addedTime: 0, + duration: 2 * MILLIS_PER_HOUR, + startedAt: 23 * MILLIS_PER_HOUR, // 23:00 + }, + _timer: { + pausedAt: instantYesterday(23 * MILLIS_PER_HOUR + 58 * MILLIS_PER_MINUTE), // 23:58, before midnight + hasFinished: false, + }, + } as RuntimeState; + + // we ran for 58 minutes before pausing, so the timer holds at 1h02 + expect(getCurrent(state)).toBe(1 * MILLIS_PER_HOUR + 2 * MILLIS_PER_MINUTE); + }); + describe('on timers of type count-to-end', () => { it('current time is the time to end even if it hasnt started, this is weird, but by design', () => { const state = { @@ -957,13 +1030,14 @@ describe('getRuntimeOffset()', () => { dayOffset: 0, }, clock: 150, + _now: instantToday(150), timer: { startedAt: 100, // started on time current: 25, // are 25ms into it addedTime: 0, }, _timer: { - pausedAt: 125, // we have been paused for 25ms (see clock) + pausedAt: instantToday(125), // we have been paused for 25ms (see clock) }, rundown: { actualStart: 100, @@ -986,17 +1060,14 @@ describe('getRuntimeOffset()', () => { dayOffset: 0, }, clock: 3 * MILLIS_PER_MINUTE, // 00:03 (after midnight) - _now: timeCore.toInstant((3 * MILLIS_PER_MINUTE) as TimeOfDay, timeCore.now()), + _now: instantToday(3 * MILLIS_PER_MINUTE), timer: { startedAt: 23 * MILLIS_PER_HOUR, // started on time at 23:00 current: 25, // still counting down addedTime: 0, }, _timer: { - pausedAt: timeCore.toInstant( - (23 * MILLIS_PER_HOUR + 58 * MILLIS_PER_MINUTE) as TimeOfDay, - (timeCore.now() - dayInMs) as Instant, - ), // 23:58, before midnight + pausedAt: instantYesterday(23 * MILLIS_PER_HOUR + 58 * MILLIS_PER_MINUTE), // 23:58, before midnight pausedDuration: 0, }, rundown: { @@ -1007,7 +1078,11 @@ describe('getRuntimeOffset()', () => { _startDayOffset: 0, } as RuntimeState; - // paused from 23:58 to 00:03 -> so elapsed should still be 58 minutes + // paused from 23:58 to 00:03 -> 5 minutes, regardless of the midnight wrap + const { absolute } = getRuntimeOffset(state); + expect(absolute).toBe(5 * MILLIS_PER_MINUTE); + + // and the pause is not active time, elapsed is still the 58 minutes we ran before pausing expect(getElapsed(state)).toBe(58 * MILLIS_PER_MINUTE); }); diff --git a/apps/server/src/services/restore-service/__tests__/restore.parser.test.ts b/apps/server/src/services/restore-service/__tests__/restore.parser.test.ts index 6be1473c1..b73a338cd 100644 --- a/apps/server/src/services/restore-service/__tests__/restore.parser.test.ts +++ b/apps/server/src/services/restore-service/__tests__/restore.parser.test.ts @@ -13,7 +13,7 @@ describe('isRestorePoint()', () => { selectedEventId: '123', startedAt: 1, addedTime: 2, - pausedAt: 3, + pausedAt: asInstant(1754745600000), firstStart: 1, startEpoch: asInstant(1), currentDay: 0, @@ -64,6 +64,22 @@ describe('isRestorePoint()', () => { expect(isRestorePoint({ ...restorePoint, pausedDuration: '3000' })).toBe(false); }); + it('rejects a pausedAt which is not an instant', () => { + const restorePoint = { + playback: Playback.Pause, + selectedEventId: '123', + startedAt: 1, + addedTime: 2, + // restore points from before pausedAt became an instant contain a time of day + pausedAt: 3, + firstStart: 1, + startEpoch: 1, + currentDay: 0, + }; + + expect(isRestorePoint(restorePoint)).toBe(false); + }); + describe('rejects a badly formatted file', () => { it('with invalid playback value', () => { const restorePoint = { diff --git a/apps/server/src/services/restore-service/__tests__/restore.service.test.ts b/apps/server/src/services/restore-service/__tests__/restore.service.test.ts index 9bfba2545..6b9dc68a1 100644 --- a/apps/server/src/services/restore-service/__tests__/restore.service.test.ts +++ b/apps/server/src/services/restore-service/__tests__/restore.service.test.ts @@ -16,7 +16,7 @@ describe('restoreService', () => { selectedEventId: 'da5b4', startedAt: 1234, addedTime: 5678, - pausedAt: 9087, + pausedAt: asInstant(1754745609087), firstStart: 1234, startEpoch: asInstant(1234), currentDay: 0, @@ -55,7 +55,7 @@ describe('restoreService', () => { selectedEventId: 'da5b4', startedAt: 1234, addedTime: 1234, - pausedAt: 1234, + pausedAt: asInstant(1754745601234), groupStartAt: 10, }; @@ -83,7 +83,7 @@ describe('restoreService', () => { selectedEventId: '1234', startedAt: 1234, addedTime: 1234, - pausedAt: 1234, + pausedAt: asInstant(1754745601234), firstStart: 1234, startEpoch: asInstant(1234), currentDay: 0, @@ -101,7 +101,7 @@ describe('restoreService', () => { selectedEventId: '2345', startedAt: 2345, addedTime: 2345, - pausedAt: 2345, + pausedAt: asInstant(1754745602345), firstStart: 2345, startEpoch: asInstant(2345), currentDay: 0, @@ -112,7 +112,7 @@ describe('restoreService', () => { selectedEventId: '3456', startedAt: 3456, addedTime: 3456, - pausedAt: 3456, + pausedAt: asInstant(1754745603456), firstStart: 3456, startEpoch: asInstant(3456), currentDay: 1, @@ -134,7 +134,7 @@ describe('restoreService', () => { selectedEventId: '5678', startedAt: 5678, addedTime: 5678, - pausedAt: 5678, + pausedAt: asInstant(1754745605678), firstStart: 5678, startEpoch: asInstant(5678), currentDay: 0, diff --git a/apps/server/src/services/restore-service/restore.parser.ts b/apps/server/src/services/restore-service/restore.parser.ts index 8016de760..a464f6c4e 100644 --- a/apps/server/src/services/restore-service/restore.parser.ts +++ b/apps/server/src/services/restore-service/restore.parser.ts @@ -1,4 +1,5 @@ import { Playback } from 'ontime-types'; +import { dayInMs } from 'ontime-utils'; import { is } from '../../utils/is.js'; import type { RestorePoint } from './restore.type.js'; @@ -42,7 +43,9 @@ export function isRestorePoint(restorePoint: unknown): restorePoint is RestorePo return false; } - if (!is.number(restorePoint.pausedAt) && restorePoint.pausedAt !== null) { + // pausedAt is an instant, restore points made before this change contained a time of day + // we reject those to avoid resuming with a corrupt pause duration + if (restorePoint.pausedAt !== null && (!is.number(restorePoint.pausedAt) || restorePoint.pausedAt < dayInMs)) { return false; } diff --git a/apps/server/src/services/restore-service/restore.type.ts b/apps/server/src/services/restore-service/restore.type.ts index 0af426bd6..54b0fef11 100644 --- a/apps/server/src/services/restore-service/restore.type.ts +++ b/apps/server/src/services/restore-service/restore.type.ts @@ -5,7 +5,8 @@ export type RestorePoint = { selectedEventId: MaybeString; startedAt: MaybeNumber; addedTime: number; - pausedAt: MaybeNumber; + /** instant the playback was paused at */ + pausedAt: Maybe; pausedDuration?: number; firstStart: MaybeNumber; startEpoch: Maybe; diff --git a/apps/server/src/services/timerUtils.ts b/apps/server/src/services/timerUtils.ts index aed095ecc..e4202f59c 100644 --- a/apps/server/src/services/timerUtils.ts +++ b/apps/server/src/services/timerUtils.ts @@ -1,4 +1,4 @@ -import { Day, MaybeNumber, TimeOfDay, TimerPhase } from 'ontime-types'; +import { Day, Duration, Maybe, MaybeNumber, TimeOfDay, TimerPhase } from 'ontime-types'; import { MILLIS_PER_HOUR, checkIsNow, dayInMs, isPlaybackActive } from 'ontime-utils'; import * as timeCore from '../lib/time-core/timeCore.js'; @@ -32,14 +32,12 @@ export function getExpectedFinish(state: RuntimeState): MaybeNumber { } const { countToEnd, timeEnd } = state.eventNow; - const { pausedAt } = state._timer; - const { clock } = state; if (startedAt === null) { return null; } - const pausedTime = pausedAt != null ? clock - pausedAt : 0; + const pausedTime = getOngoingPauseDuration(state); if (countToEnd) { return timeEnd + addedTime + pausedTime; @@ -58,11 +56,11 @@ export function getExpectedFinish(state: RuntimeState): MaybeNumber { /** * Calculates running countdown + * The result is a duration (time left in the timer), never a point in time * @param {RuntimeState} state runtime state - * @returns {number} current time for timer + * @returns {Duration} time remaining in the running timer */ - -export function getCurrent(state: RuntimeState): number { +export function getCurrent(state: RuntimeState): Duration { // eslint-disable-next-line no-unused-labels -- dev code path DEV: { if (state.eventNow === null || state.timer.duration === null) { @@ -71,54 +69,58 @@ export function getCurrent(state: RuntimeState): number { } const { startedAt, duration, addedTime } = state.timer; const { countToEnd, timeStart, timeEnd } = state.eventNow; - const { pausedAt } = state._timer; const { clock } = state; if (countToEnd) { const isEventOverMidnight = timeStart > timeEnd; const correctDay = isEventOverMidnight ? dayInMs : 0; - return correctDay - clock + timeEnd + addedTime; + return (correctDay - clock + timeEnd + addedTime) as Duration; } if (startedAt === null) { - return duration; + return duration as Duration; } - if (pausedAt != null) { - return startedAt + duration + addedTime - pausedAt; - } + // an ongoing pause freezes the timer, so we discount the time spent in it + const pausedTime = getOngoingPauseDuration(state); + const elapsedSinceStart = timeCore.elapsedTime(clock, startedAt as TimeOfDay); - const hasPassedMidnight = startedAt > clock; - const correctDay = hasPassedMidnight ? dayInMs : 0; - return startedAt + duration + addedTime - clock - correctDay; + return (duration + addedTime - elapsedSinceStart + pausedTime) as Duration; } /** - * Calculates active time elapsed since the timer started. + * Calculates active time elapsed since the timer started + * Time spent paused is not active time, so it is discounted */ -export function getElapsed(state: RuntimeState): MaybeNumber { - const { clock, _now } = state; +export function getElapsed(state: RuntimeState): Maybe { + const { clock } = state; const { startedAt } = state.timer; - const { pausedDuration, pausedAt } = state._timer; + const { pausedDuration } = state._timer; if (startedAt === null) { return null; } - const currentPauseDuration = pausedAt !== null ? timeCore.timeSince(_now, pausedAt) : 0; + const elapsedSinceStart = timeCore.elapsedTime(clock, startedAt as TimeOfDay); + const activeElapsed = elapsedSinceStart - pausedDuration - getOngoingPauseDuration(state); - const elapsedSinceStart = getTimeSinceStart(clock, startedAt); - const activeElapsed = elapsedSinceStart - pausedDuration - currentPauseDuration; - - return Math.max(0, activeElapsed); + return Math.max(0, activeElapsed) as Duration; } -function getTimeSinceStart(clock: TimeOfDay, startedAt: number): number { - if (clock < startedAt) { - return clock + dayInMs - startedAt; +/** + * Calculates how long the current pause has been going on for + * The pause is tracked as an instant, which makes the calculation + * immune to the clock wrapping around midnight + * @returns 0 if the playback is not paused + */ +function getOngoingPauseDuration(state: RuntimeState): Duration { + const { pausedAt } = state._timer; + + if (pausedAt == null) { + return 0 as Duration; } - return clock - startedAt; + return timeCore.timeSince(state._now, pausedAt); } /** @@ -154,7 +156,7 @@ export function skippedOutOfEvent(state: RuntimeState, previousTime: number, ski * Negative offset is under time / ahead of schedule */ export function getRuntimeOffset(state: RuntimeState): { absolute: number; relative: number } { - const { eventNow, clock, _startDayOffset } = state; + const { eventNow, _startDayOffset } = state; const { addedTime, current, startedAt } = state.timer; // nothing to calculate if there are no loaded events or if we havent started if (eventNow === null || startedAt === null || _startDayOffset === null) { @@ -178,8 +180,8 @@ export function getRuntimeOffset(state: RuntimeState): { absolute: number; relat // how long has the event been running over (is a negative number when in over timer so inverted before adding to offset) const overtime = Math.abs(Math.min(current, 0)); - // time the playback was paused, the different from now to when we paused is added to the offset TODO: brakes when crossing midnight - const pausedTime = state._timer.pausedAt === null ? 0 : clock - state._timer.pausedAt; + // time the playback was paused, the difference from now to when we paused is added to the offset + const pausedTime = getOngoingPauseDuration(state); // absolute offset is difference between schedule and playback time // in case of count to end, the absolute offset is overtime and added time diff --git a/apps/server/src/stores/__tests__/runtimeState.test.ts b/apps/server/src/stores/__tests__/runtimeState.test.ts index 1d253e4ad..34eecd12a 100644 --- a/apps/server/src/stores/__tests__/runtimeState.test.ts +++ b/apps/server/src/stores/__tests__/runtimeState.test.ts @@ -285,6 +285,10 @@ describe('mutation on runtimeState', () => { vi.setSystemTime('jan 2 00:01'); update(); expect(getState().timer.elapsed).toBe(8 * MILLIS_PER_MINUTE); + // the timer is frozen at the value it had when we paused + expect(getState().timer.current).toBe(2 * MILLIS_PER_HOUR - 8 * MILLIS_PER_MINUTE); + // we started 50 minutes late and have been paused for 3 minutes + expect(getState().offset.absolute).toBe(53 * MILLIS_PER_MINUTE); // resume 5 minutes after pausing, having crossed midnight (23:58 -> 00:03) vi.setSystemTime('jan 2 00:03'); diff --git a/apps/server/src/stores/runtimeState.ts b/apps/server/src/stores/runtimeState.ts index e6a08c831..ad2b85eba 100644 --- a/apps/server/src/stores/runtimeState.ts +++ b/apps/server/src/stores/runtimeState.ts @@ -63,9 +63,11 @@ export type RuntimeState = { // private properties of the timer calculations _timer: { forceFinish: Maybe; // whether we should declare an event as finished, will contain the finish time + + /** Instant the playback was paused at, an instant so that pause durations survive midnight */ pausedAt: Maybe; - /** Accumulate pause duration but dose not include the current pause */ + /** Accumulated duration of past pauses, does not include an ongoing pause */ pausedDuration: number; secondaryTarget: Maybe; hasFinished: boolean; @@ -78,6 +80,8 @@ export type RuntimeState = { _end: ExpectedMetadata; _startEpoch: Maybe; _startDayOffset: Maybe; + + /** Current instant, kept in parity with the clock (see setClock) */ _now: Instant; }; @@ -655,9 +659,8 @@ export function roll( } // we will need to do some calculations, update the time first - const epoch = timeCore.now(); - const now = timeCore.toTimeOfDay(epoch); - runtimeState.clock = now; + setClock(runtimeState); + const epoch = runtimeState._now; // 2. if there is an event armed, we use it if (runtimeState.timer.playback === Playback.Armed || runtimeState.timer.phase === TimerPhase.Pending) {