From 1f6e73ad0d77ec2ba28eb0593b0f6433edbf9648 Mon Sep 17 00:00:00 2001 From: Jan Starzak Date: Thu, 24 Apr 2025 11:45:07 +0200 Subject: [PATCH 1/7] fix: if a state older than the `currentState` is put into `handleState`, a B-A-B sequence of state changes can happen Co-authored-by: Mint de Wit --- packages/timeline-state-resolver/src/service/stateHandler.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/timeline-state-resolver/src/service/stateHandler.ts b/packages/timeline-state-resolver/src/service/stateHandler.ts index 25f99e70c4..a66f7ae65d 100644 --- a/packages/timeline-state-resolver/src/service/stateHandler.ts +++ b/packages/timeline-state-resolver/src/service/stateHandler.ts @@ -77,6 +77,8 @@ export class StateHandler { } async handleState(state: Timeline.TimelineState, mappings: Mappings) { + if (this.currentState?.state && this.currentState.state.time > state.time) return // the incoming state is stale, we ignore it + const nextState = this.stateQueue[0] const trace = startTrace('device:convertTimelineStateToDeviceState', { deviceId: this.context.deviceId }) From 888f51bab33a222a756ce29426789665873944e7 Mon Sep 17 00:00:00 2001 From: Jan Starzak Date: Thu, 24 Apr 2025 12:26:42 +0200 Subject: [PATCH 2/7] fix: ensure that nextState is top-of-queue in `calculateNextStateChange` before calling `executeNextStateChange` --- .../timeline-state-resolver/src/service/stateHandler.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/timeline-state-resolver/src/service/stateHandler.ts b/packages/timeline-state-resolver/src/service/stateHandler.ts index a66f7ae65d..5b463114ea 100644 --- a/packages/timeline-state-resolver/src/service/stateHandler.ts +++ b/packages/timeline-state-resolver/src/service/stateHandler.ts @@ -144,7 +144,11 @@ export class StateHandler { nextState.commands = [] } - if (nextState.state.time - (nextState.preliminary ?? 0) <= this.context.getCurrentTime() && this.currentState) { + if ( + nextState === this.stateQueue[0] && + nextState.state.time - (nextState.preliminary ?? 0) <= this.context.getCurrentTime() && + this.currentState + ) { await this.executeNextStateChange() } } From 66e917d83663811ccee6f59bbdd6404a367bccde Mon Sep 17 00:00:00 2001 From: Jan Starzak Date: Thu, 24 Apr 2025 12:29:39 +0200 Subject: [PATCH 3/7] fix: add a `_executingStateChange` check. --- packages/timeline-state-resolver/src/service/stateHandler.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/timeline-state-resolver/src/service/stateHandler.ts b/packages/timeline-state-resolver/src/service/stateHandler.ts index 5b463114ea..cedcea12fc 100644 --- a/packages/timeline-state-resolver/src/service/stateHandler.ts +++ b/packages/timeline-state-resolver/src/service/stateHandler.ts @@ -145,9 +145,9 @@ export class StateHandler { } if ( + this._executingStateChange && // make sure we're not trying to loop onto ourselves nextState === this.stateQueue[0] && - nextState.state.time - (nextState.preliminary ?? 0) <= this.context.getCurrentTime() && - this.currentState + nextState.state.time - (nextState.preliminary ?? 0) <= this.context.getCurrentTime() ) { await this.executeNextStateChange() } From 311aac2347cfaec20b9f06168cecc678be33dfdd Mon Sep 17 00:00:00 2001 From: Jan Starzak Date: Thu, 24 Apr 2025 13:28:52 +0200 Subject: [PATCH 4/7] fix: _executingStateChange check in calculateNextStateChange is inverted --- packages/timeline-state-resolver/src/service/stateHandler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/timeline-state-resolver/src/service/stateHandler.ts b/packages/timeline-state-resolver/src/service/stateHandler.ts index cedcea12fc..66d8e3561e 100644 --- a/packages/timeline-state-resolver/src/service/stateHandler.ts +++ b/packages/timeline-state-resolver/src/service/stateHandler.ts @@ -145,7 +145,7 @@ export class StateHandler { } if ( - this._executingStateChange && // make sure we're not trying to loop onto ourselves + !this._executingStateChange && nextState === this.stateQueue[0] && nextState.state.time - (nextState.preliminary ?? 0) <= this.context.getCurrentTime() ) { From d905c79e710a69bd2c4082067345fab671cbfa12 Mon Sep 17 00:00:00 2001 From: Jan Starzak Date: Thu, 24 Apr 2025 13:34:45 +0200 Subject: [PATCH 5/7] chore: adress SonarLint notes --- packages/timeline-state-resolver/src/service/stateHandler.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/timeline-state-resolver/src/service/stateHandler.ts b/packages/timeline-state-resolver/src/service/stateHandler.ts index 66d8e3561e..90bcca612c 100644 --- a/packages/timeline-state-resolver/src/service/stateHandler.ts +++ b/packages/timeline-state-resolver/src/service/stateHandler.ts @@ -111,8 +111,8 @@ export class StateHandler { this.currentState = { commands: [], deviceState: state, - state: this.currentState?.state || { time: this.context.getCurrentTime(), layers: {}, nextEvents: [] }, - mappings: this.currentState?.mappings || {}, + state: this.currentState?.state ?? { time: this.context.getCurrentTime(), layers: {}, nextEvents: [] }, + mappings: this.currentState?.mappings ?? {}, } await this.calculateNextStateChange() } From 8a365e1100e8abf78e1bcb09ae4c2d5b4f9dae87 Mon Sep 17 00:00:00 2001 From: Jan Starzak Date: Thu, 24 Apr 2025 13:45:00 +0200 Subject: [PATCH 6/7] chore: add test case for handling "stale" states --- .../service/__tests__/stateHandler.spec.ts | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/packages/timeline-state-resolver/src/service/__tests__/stateHandler.spec.ts b/packages/timeline-state-resolver/src/service/__tests__/stateHandler.spec.ts index a33aba3e03..86c7e0f461 100644 --- a/packages/timeline-state-resolver/src/service/__tests__/stateHandler.spec.ts +++ b/packages/timeline-state-resolver/src/service/__tests__/stateHandler.spec.ts @@ -203,6 +203,78 @@ describe('stateHandler', () => { }, }) }) + + test('ignore transitions to states older than current state', async () => { + const stateHandler = getNewStateHandler() + + stateHandler + .setCurrentState({ + entry1: { value: true }, + }) + .catch((e) => { + console.error('Error while setting current state', e) + }) + + stateHandler.handleState(createTimelineState(10000, {}), {}).catch((e) => { + console.error('Error while handling state', e) + }) + + await mockTime.tick() + + expect(MOCK_COMMAND_RECEIVER).toHaveBeenCalledTimes(1) + expect(MOCK_COMMAND_RECEIVER).toHaveBeenCalledWith({ + command: { + type: 'removed', + property: 'entry1', + }, + }) + + stateHandler + .handleState( + createTimelineState(10100, { + entry1: { value: true }, + }), + {} + ) + .catch((e) => { + console.error('Error while handling state', e) + }) + + await mockTime.tick() + + // do not expect to be called because this is in the future + expect(MOCK_COMMAND_RECEIVER).toHaveBeenCalledTimes(1) + + // advance time + MOCK_COMMAND_RECEIVER.mockReset() + await mockTime.advanceTimeTicks(100) + + // now expect to be called with new commands + expect(MOCK_COMMAND_RECEIVER).toHaveBeenCalledTimes(1) + expect(MOCK_COMMAND_RECEIVER).toHaveBeenCalledWith({ + command: { + type: 'added', + property: 'entry1', + }, + }) + + // + MOCK_COMMAND_RECEIVER.mockReset() + + stateHandler.handleState(createTimelineState(10000, {}), {}).catch((e) => { + console.error('Error while handling state', e) + }) + + await mockTime.tick() + + // do not expect to be called because new state is in the past + expect(MOCK_COMMAND_RECEIVER).toHaveBeenCalledTimes(0) + + await mockTime.advanceTimeTicks(100) + + // still no new commands to be received + expect(MOCK_COMMAND_RECEIVER).toHaveBeenCalledTimes(0) + }) }) function createTimelineState( From f12b53eb40ad0ad32a3023a6173d3146ce7877f0 Mon Sep 17 00:00:00 2001 From: Jan Starzak Date: Thu, 24 Apr 2025 13:49:36 +0200 Subject: [PATCH 7/7] chore: update timeline --- packages/timeline-state-resolver/package.json | 2 +- yarn.lock | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/timeline-state-resolver/package.json b/packages/timeline-state-resolver/package.json index 9d5492da2c..29c18ba5e6 100644 --- a/packages/timeline-state-resolver/package.json +++ b/packages/timeline-state-resolver/package.json @@ -109,7 +109,7 @@ "p-timeout": "^3.2.0", "simple-oauth2": "^5.0.0", "sprintf-js": "^1.1.3", - "superfly-timeline": "9.0.2", + "superfly-timeline": "9.1.2", "threadedclass": "^1.2.1", "timeline-state-resolver-types": "9.2.0", "tslib": "^2.6.2", diff --git a/yarn.lock b/yarn.lock index e875e4e1bc..7926bbd209 100644 --- a/yarn.lock +++ b/yarn.lock @@ -11110,12 +11110,12 @@ asn1@evs-broadcast/node-asn1: languageName: node linkType: hard -"superfly-timeline@npm:9.0.2": - version: 9.0.2 - resolution: "superfly-timeline@npm:9.0.2" +"superfly-timeline@npm:9.1.2": + version: 9.1.2 + resolution: "superfly-timeline@npm:9.1.2" dependencies: tslib: ^2.6.0 - checksum: d628d467d5384f5667bc10b877478c5b8b0a91774b5d5c5e9d9d3134b8f1b760225f2fbbb0f9ccd3e55f930c9f3719f81b9347b94ea853fbc0a18bc121d97665 + checksum: c195d3e65fd3d63223dd2a008facb32850b71da0355b258c0a08e1417bd447b144e01088f28e8e71fe8c240885c2bc5f73e8df7c84f131e963521510edf8bde2 languageName: node linkType: hard @@ -11415,7 +11415,7 @@ asn1@evs-broadcast/node-asn1: p-timeout: ^3.2.0 simple-oauth2: ^5.0.0 sprintf-js: ^1.1.3 - superfly-timeline: 9.0.2 + superfly-timeline: 9.1.2 threadedclass: ^1.2.1 timeline-state-resolver-types: 9.2.0 tslib: ^2.6.2