paul/picpeak
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

test(external): make the fold-collision guard test the real code (#745 follow-up) (#1234)

Browse Source 
The regression test added with #1165 re-implemented the claim ordering and the
claim loop inside the test file and asserted against its own copy. It never
required externalRelpathFold, so changing the real sort left it green — a guard
against silently deleting a client's delivered edit that guarded nothing.

The ordering is now a named, exported claimOrderFor() and the test drives it.
Verified by sabotage: replacing the comparator with `return 0` fails the test,
where before it passed.

Three cases added while the seam existed: the managed row wins from BOTH input
orders (the original bug was that the survivor was whichever came first, so one
order proves nothing), the sort is stable for rows of the same kind, and it does
not mutate the caller's array.

No behaviour change — the comparator is byte-identical, only lifted out.

Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
2026-08-29 12:15:31 +02:00
committed by GitHub
co-authored by Paul Nothaft
parent 0d41fe5bf1
commit 4105c099c9
2 changed files with 80 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/__tests__/services/lightroomRoundtrip.test.js
+49 -16
View File
@@ -222,34 +222,67 @@ describe('migration 193 backfill reaches watcher and external rows', () => {
});
describe('externalRelpathFold — a delivered edit must survive a collision', () => {
it('claims managed rows first, so they win and the external row loses', () => {
// The fold DELETES collision losers, and the survivor used to be
// whichever row was claimed first. A replaced photo keeps its
// external_relpath (so re-scans still dedupe) but holds the edit the
// photographer delivered — losing that to the untouched camera original
// is unrecoverable, where losing the external row is not.
const placements = [
[{ id: 1, source_origin: 'external', external_relpath: 'shoot/IMG_1.jpg' }, 'base'],
[{ id: 2, source_origin: 'managed', external_relpath: 'shoot/IMG_1.jpg' }, 'base'],
];
const claimOrder = placements.slice().sort((a, b) => {
const aManaged = a[0].source_origin === 'managed' ? 0 : 1;
const bManaged = b[0].source_origin === 'managed' ? 0 : 1;
return aManaged - bManaged;
});
// Drives the REAL claimOrderFor. The previous version of this test
// re-implemented the sort and the claim loop inline and asserted against its
// own copy, so it stayed green no matter what the service did — a guard
// against silently deleting a client's delivered edit that guarded nothing.
const { claimOrderFor } = require('../../src/services/externalRelpathFold');
// The same claim loop the fold runs, fed by the real ordering.
const claim = (placements) => {
const claimed = new Map();
const losers = new Map();
for (const [row, chosen] of claimOrder) {
for (const [row, chosen] of claimOrderFor(placements)) {
const next = chosen ? `${chosen}/${row.external_relpath}` : row.external_relpath;
const winner = claimed.get(next);
if (winner != null) { losers.set(row.id, winner); continue; }
claimed.set(next, row.id);
}
return { claimed, losers };
};
it('claims managed rows first, so they win and the external row loses', () => {
// The fold DELETES collision losers. A replaced photo keeps its
// external_relpath (so re-scans still dedupe) but holds the edit the
// photographer delivered — losing that is unrecoverable, where losing the
// external row is not: it is still on the share and a re-scan re-imports it.
const { claimed, losers } = claim([
[{ id: 1, source_origin: 'external', external_relpath: 'shoot/IMG_1.jpg' }, 'base'],
[{ id: 2, source_origin: 'managed', external_relpath: 'shoot/IMG_1.jpg' }, 'base'],
]);
expect([...losers.keys()]).toEqual([1]);
expect([...claimed.values()]).toEqual([2]);
});
it('wins regardless of the order the rows arrive in', () => {
// The bug was that the survivor was whichever row came first, so the
// managed row has to win from BOTH input orders or the fix is a coin flip.
const { losers } = claim([
[{ id: 2, source_origin: 'managed', external_relpath: 'shoot/IMG_1.jpg' }, 'base'],
[{ id: 1, source_origin: 'external', external_relpath: 'shoot/IMG_1.jpg' }, 'base'],
]);
expect([...losers.keys()]).toEqual([1]);
});
it('leaves rows of the same kind in their original order', () => {
// A stable sort matters: reordering external rows among themselves would
// change which one survives an external-vs-external collision for no reason.
const rows = [
[{ id: 10, source_origin: 'external', external_relpath: 'a.jpg' }, ''],
[{ id: 11, source_origin: 'external', external_relpath: 'b.jpg' }, ''],
[{ id: 12, source_origin: null, external_relpath: 'c.jpg' }, ''],
];
expect(claimOrderFor(rows).map(([r]) => r.id)).toEqual([10, 11, 12]);
});
it('does not mutate the caller\'s array', () => {
const rows = [
[{ id: 1, source_origin: 'external', external_relpath: 'a.jpg' }, ''],
[{ id: 2, source_origin: 'managed', external_relpath: 'b.jpg' }, ''],
];
claimOrderFor(rows);
expect(rows.map(([r]) => r.id)).toEqual([1, 2]);
});
});
describe('mergeMarks', () => {
backend/src/services/externalRelpathFold.js
+31 -18
View File
@@ -125,6 +125,35 @@ async function rootUsable(root) {
}
}
/**
* The order rows claim their folded path in — managed rows first (#745).
*
* A replaced photo keeps its external_relpath so the folder re-scan can still
* dedupe on it, but its source_origin is now 'managed' and its file is the edit
* the photographer delivered. Collisions below DELETE the loser, and the
* survivor used to be whichever row happened to be claimed first — so a
* delivered edit could be destroyed in favour of the untouched camera original
* sitting next to it on the share.
*
* Managed rows claim first, which makes them the survivor in any collision. The
* external row that loses is the recoverable one: it is still on the share and
* a re-scan re-imports it. The edit is not.
*
* Exported so the regression test can drive THIS function rather than a copy of
* it — a test that re-implements the ordering passes no matter what this does.
* The sort is stable, so rows of the same kind keep their relative order.
*
* @param {Array<[object, string|null]>} placements [row, chosenBase] pairs
* @returns {Array<[object, string|null]>} the same pairs, managed rows first
*/
function claimOrderFor(placements) {
return placements.slice().sort((a, b) => {
const aManaged = a[0].source_origin === 'managed' ? 0 : 1;
const bManaged = b[0].source_origin === 'managed' ? 0 : 1;
return aManaged - bManaged;
});
}
/**
* @param {import('knex')} knex a knex instance, or a transaction from a caller
* that is already inside one (the restore).
@@ -228,23 +257,7 @@ async function foldExternalRelpaths(knex, log = () => {}) {
const resolved = [];
const losers = new Map();
// A replaced photo keeps its external_relpath so the folder re-scan can
// still dedupe on it (#745), but its source_origin is now 'managed' and
// its file is the edit the photographer delivered. Collisions here DELETE
// the loser, and the survivor was whichever row happened to be claimed
// first — so a delivered edit could be destroyed in favour of the
// untouched camera original sitting next to it on the share.
//
// Managed rows claim first, which makes them the survivor in any
// collision. The external row that loses is the recoverable one: it is
// still on the share and a re-scan re-imports it. The edit is not.
const claimOrder = placements.slice().sort((a, b) => {
const aManaged = a[0].source_origin === 'managed' ? 0 : 1;
const bManaged = b[0].source_origin === 'managed' ? 0 : 1;
return aManaged - bManaged;
});
for (const [row, chosen] of claimOrder) {
for (const [row, chosen] of claimOrderFor(placements)) {
const next = chosen ? `${chosen}/${row.external_relpath}` : row.external_relpath;
const winner = claimed.get(next);
if (winner != null) { losers.set(row.id, winner); collided++; continue; }
@@ -318,4 +331,4 @@ async function foldExternalRelpaths(knex, log = () => {}) {
}
module.exports = { foldExternalRelpaths, MARKER };
module.exports = { foldExternalRelpaths, claimOrderFor, MARKER };