fix(portals): write a pin and retire its aliases in one transaction

Split across two calls, a half-failure had no honest outcome. Reporting
success left a surviving alias to win the next lookup and start the source the
user had just replaced; reporting failure — which the previous round changed
it to — left the canonical row durable while the UI showed a pin that was no
longer the stored one. Review was right both times, which is the tell that the
two-step shape was the problem.

`setVodSourcePin` now takes the keys to retire and does both inside one
`db.transaction()`, with the synchronous `.run()` form the better-sqlite3
driver requires there (issue #1137's lesson). `retireKeys` rides through the
worker op, the IPC contract, the preload bridge and the service, so there is
one call and one outcome.

Also corrects the architecture doc: the scan path is reached whenever no token
clears the trigram minimum, not only when the whole title is one or two
characters — the claim the previous commit's code change had already falsified.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
4grayandClaude Opus 5 committed 2026-07-28 23:13:12 +02:00
1 parent d89511466c
commit dfbff5b815
14 files changed
+160 -57

No files matched your search

@@ -420,9 +420,11 @@ export const dbPreloadCases: PreloadInvokeCase[] = [
},
{
method: 'dbSetVodSourcePin',
args: [vodSourcePin],
args: [vodSourcePin, ['title:dune:']],
channel: 'DB_SET_VOD_SOURCE_PIN',
forwardedArgs: [vodSourcePin],
// The aliases to retire ride along, so the write and the retirement
// are one transaction rather than two calls that can half-apply.
forwardedArgs: [vodSourcePin, ['title:dune:']],
},
{
method: 'dbClearVodSourcePin',
@@ -885,8 +885,8 @@ const electronApi: ElectronBridgeApi = {
ipcRenderer.invoke('DB_GET_VOD_SOURCE_PIN', matchKeys),
dbListVodSourcePins: (playlistId: string) =>
ipcRenderer.invoke('DB_LIST_VOD_SOURCE_PINS', playlistId),
dbSetVodSourcePin: (pin: VodSourcePin) =>
ipcRenderer.invoke('DB_SET_VOD_SOURCE_PIN', pin),
dbSetVodSourcePin: (pin: VodSourcePin, retireKeys?: string[]) =>
ipcRenderer.invoke('DB_SET_VOD_SOURCE_PIN', pin, retireKeys ?? []),
dbClearVodSourcePin: (matchKeys: string[]) =>
ipcRenderer.invoke('DB_CLEAR_VOD_SOURCE_PIN', matchKeys),
// Playback Positions
@@ -109,7 +109,18 @@ export function createDbMock(selectResultsByCall: unknown[][] = []) {
return query;
});
const insertValues = jest.fn().mockResolvedValue(undefined);
// Same `.run()` requirement as the update below: an insert executed inside
// a synchronous transaction callback has to be the synchronous form.
const insertRun = jest.fn();
const insertChain: Record<string, unknown> = {
run: insertRun,
then: (
resolve: (value: unknown) => void,
reject: (reason: unknown) => void
) => Promise.resolve(undefined).then(resolve, reject),
};
insertChain.onConflictDoUpdate = jest.fn().mockReturnValue(insertChain);
const insertValues = jest.fn().mockReturnValue(insertChain);
const insert = jest.fn().mockReturnValue({ values: insertValues });
// Prepared-statement writes MUST use `.run()` (synchronous) rather than
@@ -138,6 +149,8 @@ export function createDbMock(selectResultsByCall: unknown[][] = []) {
.fn()
.mockReturnValue({ execute: deleteExecute, run: deleteRun });
const deleteResult = {
// Inside a transaction the delete must be the synchronous form too.
run: deleteRun,
prepare: deletePrepare,
then: (
resolve: (value: unknown) => void,
@@ -170,6 +183,7 @@ export function createDbMock(selectResultsByCall: unknown[][] = []) {
deleteFn,
deletePrepare,
deleteRun,
insertRun,
deleteWhere,
insert,
insertValues,
@@ -33,12 +33,16 @@ const titleRow = {
updatedAt: '2026-07-01T10:00:00.000Z',
};
/** `createDbMock` resolves `.values()` directly; pins upsert on top of it. */
/**
* Pins upsert, and run inside a transaction — so the chain has to end in a
* synchronous `.run()` rather than resolving, exactly as the driver requires.
*/
function createUpsertDbMock() {
const mock = createDbMock();
const onConflictDoUpdate = jest.fn().mockResolvedValue(undefined);
const insertRun = jest.fn();
const onConflictDoUpdate = jest.fn().mockReturnValue({ run: insertRun });
mock.insertValues.mockReturnValue({ onConflictDoUpdate });
return { ...mock, onConflictDoUpdate };
return { ...mock, onConflictDoUpdate, insertRun };
}
describe('vod-source-pin.operations', () => {
@@ -162,6 +166,40 @@ describe('vod-source-pin.operations', () => {
);
});
it('retires the aliases in the same transaction as the write', async () => {
const { db, transaction, insertRun, deleteRun, deleteWhere } =
createUpsertDbMock();
await setVodSourcePin(db, pin, [
'title:the matrix:1999',
'title:the matrix:',
]);
// Two calls could half-apply, and neither half-outcome has an
// honest answer: a surviving alias wins the next lookup, while a
// rolled-back-looking failure leaves the canonical row durable.
expect(transaction).toHaveBeenCalledTimes(1);
expect(insertRun).toHaveBeenCalledTimes(1);
expect(deleteRun).toHaveBeenCalledTimes(1);
expect(deleteWhere).toHaveBeenCalled();
});
it('never retires the key it just wrote', async () => {
const { db, deleteRun } = createUpsertDbMock();
await setVodSourcePin(db, pin, ['tmdb:603']);
expect(deleteRun).not.toHaveBeenCalled();
});
it('touches nothing else when there is no alias to retire', async () => {
const { db, deleteRun } = createUpsertDbMock();
await setVodSourcePin(db, pin);
expect(deleteRun).not.toHaveBeenCalled();
});
it('writes an ISO timestamp on both the insert and the update', async () => {
const { db, insertValues, onConflictDoUpdate } =
createUpsertDbMock();
@@ -71,13 +71,28 @@ export async function listVodSourcePinsForPlaylist(
return rows.map(toPin);
}
/**
* Store the pin and retire its old aliases as ONE unit.
*
* Split across two calls there is no honest answer to a half-failure: report
* success and a surviving alias silently wins the next lookup; report failure
* and the canonical row is already durable, so a later reopen selects the
* source the UI said was rejected. A transaction removes the question.
*/
export async function setVodSourcePin(
db: AppDatabase,
pin: VodSourcePin
pin: VodSourcePin,
retireKeys: string[] = []
): Promise<{ success: boolean }> {
const updatedAt = new Date().toISOString();
const retire = (retireKeys ?? [])
.filter(
(key) =>
typeof key === 'string' && key !== '' && key !== pin.matchKey
)
.slice(0, MAX_KEYS_PER_LOOKUP);
await db
const insert = db
.insert(schema.vodSourcePins)
.values({
matchKey: pin.matchKey,
@@ -96,6 +111,19 @@ export async function setVodSourcePin(
},
});
await db.transaction(() => {
// `.run()`, not `.execute()`: on better-sqlite3 the latter defers the
// write to a promise that never settles inside this synchronous
// callback, so the statement would be a silent no-op.
insert.run();
if (retire.length > 0) {
db.delete(schema.vodSourcePins)
.where(inArray(schema.vodSourcePins.matchKey, retire))
.run();
}
});
return { success: true };
}
@@ -20,7 +20,10 @@ handleWorkerRequest('DB_LIST_VOD_SOURCE_PINS', (playlistId: string) => ({
playlistId,
}));
handleWorkerRequest('DB_SET_VOD_SOURCE_PIN', (pin: VodSourcePin) => ({ pin }));
handleWorkerRequest(
'DB_SET_VOD_SOURCE_PIN',
(pin: VodSourcePin, retireKeys: string[] = []) => ({ pin, retireKeys })
);
handleWorkerRequest('DB_CLEAR_VOD_SOURCE_PIN', (matchKeys: string[]) => ({
matchKeys,
@@ -367,8 +367,8 @@ export const workerIpcContractCases: WorkerIpcContractCase[] = [
},
{
operation: 'DB_SET_VOD_SOURCE_PIN',
args: [vodSourcePin],
payload: { pin: vodSourcePin },
args: [vodSourcePin, ['title:dune:']],
payload: { pin: vodSourcePin, retireKeys: ['title:dune:'] },
},
{
operation: 'DB_CLEAR_VOD_SOURCE_PIN',
@@ -811,8 +811,11 @@ async function executeRequest(
}
case 'DB_SET_VOD_SOURCE_PIN': {
const payload = message.payload as { pin: VodSourcePin };
return setVodSourcePin(db, payload.pin);
const payload = message.payload as {
pin: VodSourcePin;
retireKeys?: string[];
};
return setVodSourcePin(db, payload.pin, payload.retireKeys ?? []);
}
case 'DB_CLEAR_VOD_SOURCE_PIN': {
+12 -2
View File
@@ -235,8 +235,11 @@ one that does not exist — the chip simply does not appear. So:
silently decide which valid sources the user may see, and it would not even
buy anything: the GLOB cannot use an index, so SQLite reads every row either
way and a `LIMIT` saves transfer, not work. What bounds the scan instead is
its predicate — reaching it means the movie's entire title is one or two
characters, and only films carrying that exact word come back.
its predicate: reaching it means NO token cleared the trigram minimum — a
one-or-two-character title like "It", but also an all-short multiword one
like "I Am" — and EVERY token must then appear as a word. Matching on the
first token alone would return most of a large catalog for the confirmation
pass to discard, which on the single database worker is real blocking.
Like the `LIKE` it replaces it compares ASCII-lowercased text, so a non-ASCII
short title is no better and no worse served than before.
@@ -496,6 +499,13 @@ downloads sit in their own component-provided services beside it.
## Backup
A pin is written and its old aliases retired in ONE transaction
(`setVodSourcePin(db, pin, retireKeys)`). Split in two there is no honest
outcome for a half-failure: a surviving alias is read before the canonical key
on the next open, so reporting success starts the source the user just
replaced — while reporting failure leaves the canonical row durable and the UI
showing a pin that is no longer the stored one.
Pins ride along with playlist backup, under the playlist they point at, as the
optional `sourcePins` collection. See
`docs/architecture/playlist-backup-restore.md` for the remapping and
@@ -142,9 +142,11 @@ describe('VodMultiSourceHostService — pin persistence', () => {
await service.togglePin(ALT_TWO.id);
expect(pins.set).toHaveBeenCalledWith(
expect.objectContaining({ matchKey: 'tmdb:603' })
expect.objectContaining({ matchKey: 'tmdb:603' }),
expect.any(Array)
);
const [retired] = pins.clear.mock.calls[0] as [string[]];
// The aliases ride along with the write, so the two cannot half-apply.
const [, retired] = pins.set.mock.calls[0] as [unknown, string[]];
// This film's other key goes, so a reopen before enrichment cannot
// read a row still pointing at the source just replaced.
expect(retired).toContain('title:the matrix:1999');
@@ -161,8 +163,7 @@ describe('VodMultiSourceHostService — pin persistence', () => {
await service.togglePin(ALT_TWO.id);
// Retiring first would destroy the stored preference and then fail to
// replace it: nothing persisted, while the row still shows the old pin.
// One transaction: a refused write retires nothing either.
expect(pins.clear).not.toHaveBeenCalled();
expect(rowFor(ALT_TWO.id)?.isPinned).toBe(false);
});
@@ -196,21 +197,19 @@ describe('VodMultiSourceHostService — pin persistence', () => {
expect(rowFor(ALT_TWO.id)?.isPinned).toBe(false);
});
it('does not claim a pin whose stale alias survived', async () => {
// A tmdb id AND a title/year, so the canonical key is written and the
// title alias is left to retire — the case where a failed clear can
// actually strand a stale row.
it('retires the aliases in the same call that writes the pin', async () => {
await loadMovie([ALT_TWO], { ...MOVIE_A, tmdbId: 603 });
pins.set.mockResolvedValue(true);
// The canonical key is written, but retiring the alias fails.
pins.clear.mockResolvedValue(false);
await service.togglePin(ALT_TWO.id);
// Lookups read the alias BEFORE the key just written, so reopening
// this movie before enrichment lands would start the OLD source. The
// icon must not promise otherwise.
expect(rowFor(ALT_TWO.id)?.isPinned).toBe(false);
// Previously a write followed by a clear: if the clear failed, the
// surviving alias was read BEFORE the canonical key on the next open
// and started the source the user had just replaced. Neither
// half-outcome had an honest answer, so there is only one call now.
expect(pins.set).toHaveBeenCalledTimes(1);
expect(pins.clear).not.toHaveBeenCalled();
const [, retired] = pins.set.mock.calls[0] as [unknown, string[]];
expect(retired).toContain('title:the matrix:1999');
});
it('keeps the pin when clearing it fails', async () => {
@@ -357,7 +357,8 @@ describe('VodMultiSourceHostService — pinning', () => {
await service.togglePin(ALT_TWO.id);
expect(pins.set).toHaveBeenCalledWith(pin);
// The aliases to retire travel with the write, in one transaction.
expect(pins.set).toHaveBeenCalledWith(pin, []);
expect(rowFor(ALT_TWO.id)?.isPinned).toBe(true);
await service.togglePin(ALT_TWO.id);
@@ -94,27 +94,21 @@ export async function writePin(
return false;
}
// The write goes FIRST, and the old rows only go once it landed. The
// other order destroys the stored preference and then fails to replace it
// — leaving nothing persisted while the UI still shows the old pin.
// Lookups are most-trusted-first, so a leftover alias never outranks the
// key just written.
const stored = await pins.set({
matchKey,
playlistId: candidate.playlistId,
contentId: candidate.contentId,
portalType: candidate.portalType,
});
if (!stored) {
return false;
}
const retire = retirablePinKeys(keys).filter((key) => key !== matchKey);
// A surviving alias is read BEFORE the canonical key on the next open, so
// a failed retire means reopening the movie picks the OLD source until
// enrichment supplies the tmdb id again. Report that rather than claiming
// the change stuck.
return retire.length === 0 || (await erasePin(pins, retire));
// ONE transaction, not a write followed by a clear. Split in two there is
// no honest outcome for a half-failure: a surviving alias is read BEFORE
// the canonical key on the next open, so reporting success starts the
// source the user just replaced — while reporting failure leaves the
// canonical row durable and the UI showing a pin that is no longer the
// stored one.
return pins.set(
{
matchKey,
playlistId: candidate.playlistId,
contentId: candidate.contentId,
portalType: candidate.portalType,
},
retirablePinKeys(keys).filter((key) => key !== matchKey)
);
}
/** Clears every alias, so unpinning is not undone by a stale row. */
@@ -60,13 +60,20 @@ export class VodSourcePinService {
}
}
async set(pin: VodSourcePin): Promise<boolean> {
/**
* Store the pin, retiring `retireKeys` in the same transaction — a
* half-applied change has no honest outcome to report.
*/
async set(pin: VodSourcePin, retireKeys: string[] = []): Promise<boolean> {
if (!this.isAvailable) {
return false;
}
try {
const result = await window.electron.dbSetVodSourcePin(pin);
const result = await window.electron.dbSetVodSourcePin(
pin,
retireKeys
);
return result?.success === true;
} catch (error) {
console.warn(
@@ -882,7 +882,11 @@ export interface ElectronBridgeApi {
dbGetVodSourcePin: (matchKeys: string[]) => Promise<VodSourcePin | null>;
/** Every pin pointing at this playlist — used by playlist backup. */
dbListVodSourcePins: (playlistId: string) => Promise<VodSourcePin[]>;
dbSetVodSourcePin: (pin: VodSourcePin) => Promise<ElectronBridgeResult>;
/** `retireKeys` are removed in the SAME transaction as the write. */
dbSetVodSourcePin: (
pin: VodSourcePin,
retireKeys?: string[]
) => Promise<ElectronBridgeResult>;
dbClearVodSourcePin: (
matchKeys: string[]
) => Promise<ElectronBridgeResult>;