mirror of
https://github.com/cpvalente/ontime.git
synced 2026-08-26 09:29:10 +00:00
Fix rundown-switch data loss and clone key divergence found in review
Two real bugs in the clone work, both confirmed with repros before fixing
and re-confirmed by reverting each fix afterwards.
1. Data loss on every rundown switch (blocker).
cachedRundown was a module-level const mutated in place by init(). Once
setRundown stored by reference, db.data.rundowns[loadedId] aliased that
object, so loading a second rundown rewrote the first one's stored record
in place. Repro: after init(A) then init(B), store['rundown-a'] had
id 'rundown-b', title 'B' and order ['b1'] - A's data was gone, and both
keys pointed at one object.
The earlier audit missed this because it only asked whether callers mutate
an object after handing it over, within one rundown's lifetime. It never
considered that cachedRundown is a singleton reused across different
rundowns, which is where the aliasing actually bites.
Fixed at the root: cachedRundown is now reassigned rather than repurposed,
so a stored record keeps pointing at the data it was stored for. Mutating
in place from commit, while it still represents the same rundown, is
unchanged and is what keeps the stored record current. Every
getCurrentRundown() call site was checked - all are locals used within a
single operation, none held across a switch.
2. cloneEntryData was not key-equal to structuredClone.
For an entry without a custom/triggers/entries key, the spread-then-
override form added those as own keys ('custom' as {}, 'triggers' as
undefined). fast-equals deepEqual counts own keys, so a clone never
compared equal to its source, which is the comparison runtime.service.ts
uses to decide whether to re-broadcast - reinstating the per-tick
re-broadcast and restore-save this PR had already fixed once. Dropping
`?? []` earlier corrected the value but not the key.
Containers are now only replaced when the entry carries them.
The regression test was vacuous: toBeUndefined() passes for both an absent
key and an own key holding undefined. It now asserts key equality against
structuredClone plus deepEqual, and was verified to fail against the
previous implementation.
Typecheck, lint, format and the full suite (706 tests) pass.
This commit is contained in:
@@ -1,3 +1,4 @@
|
||||
import { deepEqual } from 'fast-equals';
|
||||
import {
|
||||
EndAction,
|
||||
OntimeEntry,
|
||||
@@ -770,9 +771,17 @@ describe('cloneEntryData()', () => {
|
||||
* change on every comparison, which would have the runtime re-broadcast and re-save the
|
||||
* restore point on every tick. See PR #2178.
|
||||
*/
|
||||
it('leaves an absent triggers or entries value absent', () => {
|
||||
expect(cloneEntryData(makeOntimeEvent({ id: 'partial' })).triggers).toBeUndefined();
|
||||
expect(cloneEntryData(makeOntimeGroup({ id: 'partial', entries: undefined })).entries).toBeUndefined();
|
||||
it.each([
|
||||
['event', makeOntimeEvent({ id: 'partial' })],
|
||||
['group', makeOntimeGroup({ id: 'partial', entries: undefined })],
|
||||
])('gives a partial %s exactly the keys structuredClone would, so it stays deep-equal', (_type, entry) => {
|
||||
const clone = cloneEntryData(entry);
|
||||
// asserting on keys, not values: `toBeUndefined()` cannot tell an absent key from an own
|
||||
// key holding undefined, and it is key presence that decides the deepEqual below
|
||||
expect(Object.keys(clone).sort()).toEqual(Object.keys(structuredClone(entry)).sort());
|
||||
// this is the comparison runtime.service.ts uses to decide whether to re-broadcast an
|
||||
// entry; if the clone gains a key, every tick looks like a change
|
||||
expect(deepEqual(clone, entry)).toBe(true);
|
||||
});
|
||||
|
||||
it('throws on an entry type it does not know how to clone', () => {
|
||||
|
||||
@@ -45,9 +45,14 @@ import {
|
||||
} from './rundown.utils.js';
|
||||
|
||||
/**
|
||||
* The currently loaded rundown in cache
|
||||
* The currently loaded rundown in cache.
|
||||
*
|
||||
* Reassigned - never mutated in place - when a different rundown is loaded: the persistence
|
||||
* layer stores this object by reference, so repurposing it for another rundown would rewrite
|
||||
* the previously loaded rundown's stored record. Mutating it in place while it represents the
|
||||
* same rundown (ie. from commit) is intended, and is what keeps the stored record current.
|
||||
*/
|
||||
const cachedRundown: Rundown = {
|
||||
let cachedRundown: Rundown = {
|
||||
id: '',
|
||||
title: '',
|
||||
order: [],
|
||||
@@ -726,17 +731,21 @@ export function init(initialRundown: Readonly<Rundown>, initialCustomFields: Rea
|
||||
const customFields = structuredClone(initialCustomFields);
|
||||
const processedData = processRundown(rundown, customFields, { mutate: true });
|
||||
|
||||
// update the cache values
|
||||
cachedRundown.id = rundown.id;
|
||||
cachedRundown.title = rundown.title;
|
||||
projectCustomFields = customFields;
|
||||
|
||||
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- we are not interested in the iteration data
|
||||
const { previousEvent, latestEvent, previousEntry, entries, order, ...metadata } = processedData;
|
||||
cachedRundown.entries = entries;
|
||||
cachedRundown.order = order;
|
||||
cachedRundown.flatOrder = metadata.flatEntryOrder;
|
||||
cachedRundown.revision = rundown.revision;
|
||||
|
||||
// a fresh object, so that the record already stored for a previously loaded rundown keeps
|
||||
// pointing at that rundown's data - see the note on cachedRundown
|
||||
cachedRundown = {
|
||||
id: rundown.id,
|
||||
title: rundown.title,
|
||||
entries,
|
||||
order,
|
||||
flatOrder: metadata.flatEntryOrder,
|
||||
revision: rundown.revision,
|
||||
};
|
||||
rundownMetadata = metadata;
|
||||
|
||||
// defer writing to the database
|
||||
|
||||
@@ -506,17 +506,29 @@ export function cloneSimpleRundownEntry(entry: OntimeEntry, newId: EntryId): Ont
|
||||
* `custom` and `entries` hold primitives, so copying the container is enough. `triggers`
|
||||
* holds objects, which are copied too - otherwise a mutable transaction could edit a trigger
|
||||
* of the cached rundown before commit.
|
||||
* An absent container is left absent rather than normalised to an empty one, so that a clone
|
||||
* still compares equal to the entry it came from.
|
||||
* A container is only replaced when the entry actually carries it, so the clone has exactly
|
||||
* the same keys as the source. Adding a key here would make the clone compare unequal to the
|
||||
* entry it came from, and the runtime uses that comparison to decide whether to re-broadcast.
|
||||
*/
|
||||
export function cloneEntryData<T extends OntimeEntry>(entry: T): T {
|
||||
switch (entry.type) {
|
||||
case SupportedEntry.Event:
|
||||
return { ...entry, custom: { ...entry.custom }, triggers: entry.triggers?.map((t) => ({ ...t })) } as T;
|
||||
case SupportedEntry.Group:
|
||||
return { ...entry, custom: { ...entry.custom }, entries: entry.entries?.slice() } as T;
|
||||
case SupportedEntry.Milestone:
|
||||
return { ...entry, custom: { ...entry.custom } } as T;
|
||||
case SupportedEntry.Event: {
|
||||
const clone: OntimeEvent = { ...entry };
|
||||
if (clone.custom) clone.custom = { ...clone.custom };
|
||||
if (clone.triggers) clone.triggers = clone.triggers.map((trigger) => ({ ...trigger }));
|
||||
return clone as T;
|
||||
}
|
||||
case SupportedEntry.Group: {
|
||||
const clone: OntimeGroup = { ...entry };
|
||||
if (clone.custom) clone.custom = { ...clone.custom };
|
||||
if (clone.entries) clone.entries = clone.entries.slice();
|
||||
return clone as T;
|
||||
}
|
||||
case SupportedEntry.Milestone: {
|
||||
const clone: OntimeMilestone = { ...entry };
|
||||
if (clone.custom) clone.custom = { ...clone.custom };
|
||||
return clone as T;
|
||||
}
|
||||
case SupportedEntry.Delay:
|
||||
return { ...entry } as T;
|
||||
default: {
|
||||
|
||||
Reference in New Issue
Block a user