Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions electron/media/mediaLinksRegistry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,4 +205,90 @@ describe("mediaLinksRegistry", () => {
}
});
});

// `findMediaLinksByFingerprint` is a READ that writes: when the path has
// drifted it refreshes `lastKnownPath` in the background. Not awaiting that
// write is the right call — a lookup should not pay for it — but it means
// nothing is watching the promise, and a rejected promise nobody watches is a
// process-level `unhandledRejection`. Under vitest that fails the run from
// OUTSIDE every test, which is how this was found: 1628 passing tests, a red
// job, and a stack pointing at a temp dir a finished suite had removed.
//
// Both cases below assert the same thing in two ways a real machine produces
// it. Neither asserts that the refresh succeeds — that is precisely what is
// allowed to fail.
describe("the background path refresh", () => {
/** Same bytes at a second path, so a lookup matches by fingerprint and
* then tries to write the new path back. */
async function registerThenMove(): Promise<{ original: string; moved: string }> {
const original = path.join(tempDir, "moved.webm");
await writeFileOfSize(original, 900, "m");
await registerMediaLinks(tempDir, original, { webcamVideoPath: `${original}-cam.webm` });
const moved = path.join(tempDir, "moved-elsewhere.webm");
await fs.copyFile(original, moved);
return { original, moved };
}

async function withoutUnhandledRejections(fn: () => Promise<void>): Promise<unknown[]> {
const rejections: unknown[] = [];
const onRejection = (reason: unknown) => rejections.push(reason);
process.on("unhandledRejection", onRejection);
try {
await fn();
// Node decides a rejection is unhandled a tick after the microtask
// queue drains, so the assertion needs a real timer, not a flush.
await new Promise((resolve) => setTimeout(resolve, 50));
} finally {
process.off("unhandledRejection", onRejection);
}
return rejections;
}

// Running as root defeats the permission bit this case relies on. Skipped
// out loud rather than passing vacuously.
it.skipIf(process.getuid?.() === 0)(
"logs a refresh it cannot write, and still answers the lookup",
async () => {
const { original, moved } = await registerThenMove();
// Registry readable, directory unwritable: only the write can fail.
await fs.chmod(tempDir, 0o555);
const warned = vi.spyOn(console, "warn").mockImplementation(() => {
// swallowed: the test asserts on it, the suite output does not need it
});
try {
const rejections = await withoutUnhandledRejections(async () => {
const resolved = await findMediaLinksByFingerprint(tempDir, moved);
// A refresh that failed is not a lookup that failed.
expect(resolved?.webcamVideoPath).toBe(`${original}-cam.webm`);
});
expect(rejections).toEqual([]);
expect(warned).toHaveBeenCalled();
} finally {
warned.mockRestore();
await fs.chmod(tempDir, 0o755);
}
},
);

it("survives the directory disappearing while the refresh is queued", async () => {
// The CI shape: a suite's `afterEach` removes its temp dir while a write
// is still in the queue. Whoever wins the race is fine — what must not
// happen is a rejection escaping into the process.
const { moved } = await registerThenMove();
const warned = vi.spyOn(console, "warn").mockImplementation(() => {
// may or may not fire: the write is allowed to win the race
});
try {
const rejections = await withoutUnhandledRejections(async () => {
const lookup = findMediaLinksByFingerprint(tempDir, moved);
await fs.rm(tempDir, { recursive: true, force: true });
await lookup;
});
expect(rejections).toEqual([]);
} finally {
warned.mockRestore();
await fs.mkdir(tempDir, { recursive: true });
}
});
});
});
33 changes: 26 additions & 7 deletions electron/media/mediaLinksRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,21 @@ const writeQueues = new Map<string, Promise<unknown>>();
function withWriteLock<T>(baseDir: string, fn: () => Promise<T>): Promise<T> {
const queue = writeQueues.get(baseDir) ?? Promise.resolve();
const result = queue.then(fn, fn);
writeQueues.set(
baseDir,
result.then(
() => undefined,
() => undefined,
),
// The tail swallows the outcome so the NEXT writer runs either way; `result`
// keeps the real one for the caller.
const tail = result.then(
() => undefined,
() => undefined,
);
writeQueues.set(baseDir, tail);
// Drop the key once the chain has drained — but only if nothing queued behind
// us in the meantime, or we would strand a tail a later caller is already
// chained on. The map is keyed by an arbitrary directory path, so without
// this it grows for the life of the process; production has one key, a test
// run has one per temp dir. `tail` never rejects, so this cannot leak either.
void tail.then(() => {
if (writeQueues.get(baseDir) === tail) writeQueues.delete(baseDir);
});
return result;
}

Expand Down Expand Up @@ -300,13 +308,24 @@ export async function findMediaLinksByFingerprint(

// Path drifted from what's on record — refresh it so the next lookup can
// take a cheaper path if one becomes available again.
//
// ponytail: deliberately not awaited — a lookup must not pay for a write it
// does not need — but `void` alone is not fire-and-forget, it is
// fire-and-crash. Nothing was watching this promise, so any failure became an
// unhandled rejection: in the main process that is a process-level event, and
// under vitest it fails the whole run from outside every test (the CI symptom
// was `mkdir ENOENT` when a suite's temp dir was removed while this write was
// still queued, reported after 1628 passing tests). A refresh that cannot
// happen is not worth interrupting anyone over — but it is worth a line.
if (match.lastKnownPath !== videoPath) {
void updateRegistry(baseDir, (file) => ({
version: 1,
entries: file.entries.map((e) =>
fingerprintsMatch(e.fingerprint, fingerprint) ? { ...e, lastKnownPath: videoPath } : e,
),
}));
})).catch((error) => {
console.warn("[media-links] could not refresh the recorded path:", error);
});
}

return {
Expand Down
Loading