fix: rework the syncIngest rollback flow

packages/job-worker/src/blueprints/__tests__/context-adlibActions.test.ts

@@ -144,11 +144,6 @@ describe('Test blueprint api context', () => {
 		)
 	}
 
-	// let context: MockJobContext
-	// beforeAll(async () => {
-	// 	context = await setupDefaultJobEnvironment()
-	// })
-
 	async function getActionExecutionContext(jobContext: JobContext, playoutModel: PlayoutModel) {
 		const playlist = playoutModel.Playlist
 		expect(playlist).toBeTruthy()
@@ -1141,7 +1136,7 @@ describe('Test blueprint api context', () => {
 
 					const newPieceInstanceId = (await context.insertPiece('current', { externalId: 'input1' } as any))
 						._id
-					expect(newPieceInstanceId).toMatch(/randomId([0-9]+)_part0_0_instance_randomId([0-9]+)/)
+					expect(newPieceInstanceId).toMatch(/randomId(\d+)_part0_0_instance_randomId(\d+)/)
 					expect(postProcessPiecesMock).toHaveBeenCalledTimes(1)
 					expect(postProcessPiecesMock).toHaveBeenCalledWith(
 						expect.anything(),
@@ -1342,25 +1337,6 @@ describe('Test blueprint api context', () => {
 						'New part must contain at least one piece'
 					)
 
-					// expect(
-					// 	context.queuePart(
-					// 		// @ts-ignore
-					// 		{
-					// 			floated: true,
-					// 		},
-					// 		[{}]
-					// 	).part.floated
-					// ).toBeFalsy()
-					// expect(
-					// 	context.queuePart(
-					// 		// @ts-ignore
-					// 		{
-					// 			invalid: true,
-					// 		},
-					// 		[{}]
-					// 	).part.invalid
-					// ).toBeFalsy()
-
 					expect(postProcessPiecesMock).toHaveBeenCalledTimes(0)
 					expect(insertQueuedPartWithPiecesMock).toHaveBeenCalledTimes(0)
 
@@ -1371,17 +1347,16 @@ describe('Test blueprint api context', () => {
 					expect(postProcessPiecesMock).toHaveBeenCalledTimes(1)
 					expect(insertQueuedPartWithPiecesMock).toHaveBeenCalledTimes(0)
 
-					partInstance.part.autoNext = true
-					partInstance.part.expectedDuration = 700
-					partInstance.timings = {
-						plannedStartedPlayback: getCurrentTime(),
-						plannedStoppedPlayback: undefined,
-						playOffset: 0,
-						take: undefined,
-					}
-					cache.replacePartInstance(new PlayoutPartInstanceModelImpl(partInstance, [], true))
+					const partInstanceModel = cache.getPartInstance(partInstance._id) as PlayoutPartInstanceModel
+					expect(partInstanceModel).toBeTruthy()
+
+					partInstanceModel.updatePartProps({
+						autoNext: true,
+						expectedDuration: 700,
+					})
+					partInstanceModel.setPlannedStartedPlayback(getCurrentTime())
 
-					expect(isTooCloseToAutonext(partInstance, true)).toBeTruthy()
+					expect(isTooCloseToAutonext(partInstanceModel.PartInstance, true)).toBeTruthy()
 					await expect(context.queuePart({} as any, [{}] as any)).rejects.toThrow(
 						'Too close to an autonext to queue a part'
 					)

packages/job-worker/src/ingest/syncChangesToPartInstance.ts

@@ -162,18 +162,18 @@ export async function syncChangesToPartInstances(
 					),
 				}
 
-				const clonedPartInstance = existingPartInstance.clone()
+				const partInstanceSnapshot = existingPartInstance.snapshotMakeCopy()
 
 				const syncContext = new SyncIngestUpdateToPartInstanceContext(
 					context,
 					{
-						name: `Update to ${clonedPartInstance.PartInstance.part.externalId}`,
-						identifier: `rundownId=${clonedPartInstance.PartInstance.part.rundownId},segmentId=${clonedPartInstance.PartInstance.part.segmentId}`,
+						name: `Update to ${existingPartInstance.PartInstance.part.externalId}`,
+						identifier: `rundownId=${existingPartInstance.PartInstance.part.rundownId},segmentId=${existingPartInstance.PartInstance.part.segmentId}`,
 					},
 					context.studio,
 					showStyle,
 					rundownWrapped.Rundown,
-					clonedPartInstance,
+					existingPartInstance,
 					proposedPieceInstances,
 					playStatus
 				)
@@ -186,16 +186,18 @@ export async function syncChangesToPartInstances(
 						newResultData,
 						playStatus
 					)
-
-					// If the blueprint function throws, no changes will be synced to the cache:
-					// TODO - save clonedPartInstance
-					playoutModel.replacePartInstance(clonedPartInstance)
 				} catch (err) {
 					logger.error(`Error in showStyleBlueprint.syncIngestUpdateToPartInstance: ${stringifyError(err)}`)
+
+					// Operation failed, rollback the changes
+					existingPartInstance.snapshotRestore(partInstanceSnapshot)
 				}
 
 				if (playStatus === 'next') {
-					updateExpectedDurationWithPrerollForPartInstance(playoutModel, clonedPartInstance.PartInstance._id)
+					updateExpectedDurationWithPrerollForPartInstance(
+						playoutModel,
+						existingPartInstance.PartInstance._id
+					)
 				}
 
 				// Save notes:
@@ -214,7 +216,7 @@ export async function syncChangesToPartInstances(
 				if (newNotes.length) {
 					// TODO - these dont get shown to the user currently
 					// TODO - old notes from the sync may need to be pruned, or we will end up with duplicates and 'stuck' notes?+
-					clonedPartInstance.appendNotes(newNotes)
+					existingPartInstance.appendNotes(newNotes)
 
 					validateScratchpartPartInstanceProperties(
 						context,
@@ -223,7 +225,7 @@ export async function syncChangesToPartInstances(
 					)
 				}
 
-				if (clonedPartInstance.PartInstance._id === playoutModel.Playlist.currentPartInfo?.partInstanceId) {
+				if (existingPartInstance.PartInstance._id === playoutModel.Playlist.currentPartInfo?.partInstanceId) {
 					// This should be run after 'current', before 'next':
 					await syncPlayheadInfinitesForNextPartInstance(
 						context,

packages/job-worker/src/playout/model/PlayoutModel.ts

@@ -112,13 +112,6 @@ export interface PlayoutModel extends PlayoutModelReadonly, StudioPlayoutModelBa
 
 	removeUntakenPartInstances(): void
 
-	/**
-	 * HACK: This allows for taking a copy of a `PartInstanceWithPieces` for use in `syncChangesToPartInstances`.
-	 * This lets us discard the changes if the blueprint call throws.
-	 * We should look at avoiding this messy/dangerous method, and find a better way to do this
-	 */
-	replacePartInstance(partInstance: PlayoutPartInstanceModel): void
-
 	/**
 	 * Reset the playlist for playout
 	 */

packages/job-worker/src/playout/model/PlayoutPartInstanceModel.ts

@@ -12,11 +12,31 @@ import { IBlueprintMutatablePart, PieceLifespan, Time } from '@sofie-automation/
 import { PartCalculatedTimings } from '@sofie-automation/corelib/dist/playout/timings'
 import { PlayoutPieceInstanceModel } from './PlayoutPieceInstanceModel'
 
+/**
+ * Token returned when making a backup copy of a PlayoutPartInstanceModel
+ * The contents of this type is opaque and will vary fully across implementations
+ */
+export interface PlayoutPartInstanceModelSnapshot {
+	__isPlayoutPartInstanceModelBackup: true
+}
+
 export interface PlayoutPartInstanceModel {
 	readonly PartInstance: ReadonlyDeep<DBPartInstance>
 	readonly PieceInstances: PlayoutPieceInstanceModel[]
 
-	clone(): PlayoutPartInstanceModel
+	/**
+	 * Take a snapshot of the current state of this PlayoutPartInstanceModel
+	 * This can be restored with `snapshotRestore` to rollback to a previous state of the model
+	 */
+	snapshotMakeCopy(): PlayoutPartInstanceModelSnapshot
+
+	/**
+	 * Restore a snapshot of this PlayoutPartInstanceModel, to rollback to a previous state
+	 * Note: It is only possible to restore each snapshot once.
+	 * Note: Any references to child `PlayoutPieceInstanceModel` or `DBPartInstance` may no longer be valid after this operation
+	 * @param snapshot Snapshot to restore
+	 */
+	snapshotRestore(snapshot: PlayoutPartInstanceModelSnapshot): void
 
 	appendNotes(notes: PartNote[]): void
 

packages/job-worker/src/playout/model/implementation/PlayoutModelImpl.ts

@@ -484,28 +484,6 @@ export class PlayoutModelImpl extends PlayoutModelReadonlyImpl implements Playou
 		}
 	}
 
-	/**
-	 * HACK: This allows for taking a copy of a `PartInstanceWithPieces` for use in `syncChangesToPartInstances`.
-	 * This lets us discard the changes if the blueprint call throws.
-	 * We should look at avoiding this messy/dangerous method, and find a better way to do this
-	 */
-	replacePartInstance(partInstance: PlayoutPartInstanceModel): void {
-		if (!(partInstance instanceof PlayoutPartInstanceModelImpl))
-			throw new Error(`Expected PartInstanceWithPiecesImpl`)
-
-		const currentPartInstance = this.CurrentPartInstance
-		const nextPartInstance = this.NextPartInstance
-
-		if (
-			(currentPartInstance && partInstance.PartInstance._id === currentPartInstance.PartInstance._id) ||
-			(nextPartInstance && partInstance.PartInstance._id === nextPartInstance.PartInstance._id)
-		) {
-			this.AllPartInstances.set(partInstance.PartInstance._id, partInstance)
-		} else {
-			throw new Error('Cannot call `replacePartInstance` for arbitrary PartInstances')
-		}
-	}
-
 	/**
 	 * Reset the playlist for playout
 	 */

packages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.ts

@@ -26,12 +26,43 @@ import {
 	PieceLifespan,
 	Time,
 } from '@sofie-automation/blueprints-integration'
-import { PlayoutPartInstanceModel } from '../PlayoutPartInstanceModel'
+import { PlayoutPartInstanceModel, PlayoutPartInstanceModelSnapshot } from '../PlayoutPartInstanceModel'
 import { protectString } from '@sofie-automation/corelib/dist/protectedString'
 import { PlayoutPieceInstanceModel } from '../PlayoutPieceInstanceModel'
 import { PlayoutPieceInstanceModelImpl } from './PlayoutPieceInstanceModelImpl'
 import { EmptyPieceTimelineObjectsBlob } from '@sofie-automation/corelib/dist/dataModel/Piece'
 
+interface PlayoutPieceInstanceModelSnapshotImpl {
+	PieceInstance: PieceInstance
+	HasChanges: boolean
+}
+class PlayoutPartInstanceModelSnapshotImpl implements PlayoutPartInstanceModelSnapshot {
+	readonly __isPlayoutPartInstanceModelBackup = true
+
+	isRestored = false
+
+	readonly PartInstance: DBPartInstance
+	readonly PartInstanceHasChanges: boolean
+	readonly PieceInstances: ReadonlyMap<PieceInstanceId, PlayoutPieceInstanceModelSnapshotImpl | null>
+
+	constructor(copyFrom: PlayoutPartInstanceModelImpl) {
+		this.PartInstance = clone(copyFrom.PartInstanceImpl)
+		this.PartInstanceHasChanges = copyFrom.PartInstanceHasChanges
+
+		const pieceInstances = new Map<PieceInstanceId, PlayoutPieceInstanceModelSnapshotImpl | null>()
+		for (const [pieceInstanceId, pieceInstance] of copyFrom.PieceInstancesImpl) {
+			if (pieceInstance) {
+				pieceInstances.set(pieceInstanceId, {
+					PieceInstance: clone(pieceInstance.PieceInstanceImpl),
+					HasChanges: pieceInstance.HasChanges,
+				})
+			} else {
+				pieceInstances.set(pieceInstanceId, null)
+			}
+		}
+		this.PieceInstances = pieceInstances
+	}
+}
 export class PlayoutPartInstanceModelImpl implements PlayoutPartInstanceModel {
 	PartInstanceImpl: DBPartInstance
 	PieceInstancesImpl: Map<PieceInstanceId, PlayoutPieceInstanceModelImpl | null>
@@ -85,26 +116,33 @@ export class PlayoutPartInstanceModelImpl implements PlayoutPartInstanceModel {
 		}
 	}
 
-	/**
-	 * @deprecated
-	 * What is the purpose of this? Without changing the ids it is going to clash with the old copy..
-	 * TODO - this has issues with deleting instances!
-	 */
-	clone(): PlayoutPartInstanceModel {
-		const cloned = new PlayoutPartInstanceModelImpl(clone(this.PartInstanceImpl), [], this.#PartInstanceHasChanges)
+	snapshotMakeCopy(): PlayoutPartInstanceModelSnapshot {
+		return new PlayoutPartInstanceModelSnapshotImpl(this)
+	}
 
-		for (const [id, pieceInstance] of this.PieceInstancesImpl) {
-			if (!pieceInstance) {
-				cloned.PieceInstancesImpl.set(id, null)
-			} else {
-				cloned.PieceInstancesImpl.set(
-					id,
-					new PlayoutPieceInstanceModelImpl(clone(pieceInstance.PieceInstanceImpl), pieceInstance.HasChanges)
+	snapshotRestore(snapshot: PlayoutPartInstanceModelSnapshot): void {
+		if (!(snapshot instanceof PlayoutPartInstanceModelSnapshotImpl))
+			throw new Error(`Cannot restore a Snapshot from an different Model`)
+
+		if (snapshot.PartInstance._id !== this.PartInstance._id)
+			throw new Error(`Cannot restore a Snapshot from an different PartInstance`)
+
+		if (snapshot.isRestored) throw new Error(`Cannot restore a Snapshot which has already been restored`)
+		snapshot.isRestored = true
+
+		this.PartInstanceImpl = snapshot.PartInstance
+		this.#PartInstanceHasChanges = snapshot.PartInstanceHasChanges
+		this.PieceInstancesImpl.clear()
+		for (const [pieceInstanceId, pieceInstance] of snapshot.PieceInstances) {
+			if (pieceInstance) {
+				this.PieceInstancesImpl.set(
+					pieceInstanceId,
+					new PlayoutPieceInstanceModelImpl(pieceInstance.PieceInstance, pieceInstance.HasChanges)
 				)
+			} else {
+				this.PieceInstancesImpl.set(pieceInstanceId, null)
 			}
 		}
-
-		return cloned
 	}
 
 	appendNotes(notes: PartNote[]): void {

packages/job-worker/src/playout/model/implementation/PlayoutPieceInstanceModelImpl.ts

@@ -1,7 +1,7 @@
 import { PieceInstanceInfiniteId } from '@sofie-automation/corelib/dist/dataModel/Ids'
 import { ReadonlyDeep } from 'type-fest'
 import { PieceInstance, PieceInstancePiece } from '@sofie-automation/corelib/dist/dataModel/PieceInstance'
-import { clone, getRandomId } from '@sofie-automation/corelib/dist/lib'
+import { getRandomId } from '@sofie-automation/corelib/dist/lib'
 import { Time } from '@sofie-automation/blueprints-integration'
 import { PlayoutPieceInstanceModel } from '../PlayoutPieceInstanceModel'
 
@@ -26,15 +26,6 @@ export class PlayoutPieceInstanceModelImpl implements PlayoutPieceInstanceModel
 		this.#HasChanges = hasChanges
 	}
 
-	/**
-	 * @deprecated
-	 * What is the purpose of this? Without changing the ids it is going to clash with the old copy..
-	 * TODO - this has issues with deleting instances!
-	 */
-	clone(): PlayoutPieceInstanceModelImpl {
-		return new PlayoutPieceInstanceModelImpl(clone(this.PieceInstanceImpl), this.#HasChanges)
-	}
-
 	prepareForHold(): PieceInstanceInfiniteId {
 		const infiniteInstanceId: PieceInstanceInfiniteId = getRandomId()
 		this.PieceInstanceImpl.infinite = {