fix(watchlist): stale-read clobber, concurrency guard, leaked timeout, quiet catch-up notices

- SyncDeps.listNotes now returns paths only; new readNote(path) reads
  content immediately before parse/diff/write per note, closing the
  window where an edit made between the initial scan and a later write
  got silently overwritten. resolveMissingIds updated to match.
- WatchlistController gains a syncing flag shared by syncNow and
  resolveMissingIds so overlapping invocations short-circuit instead
  of racing on the same vault notes.
- main.ts registers cleanup for the 30s startup catch-up setTimeout so
  it doesn't fire after unload.
- syncNow takes a quiet flag (used by the scheduled catch-up path) so
  zero-change scheduled syncs don't spam a Notice; manual commands are
  unaffected.
This commit is contained in:
afiqzudinhadi 2026-07-30 13:24:33 +08:00
parent b77a189848
commit a4ba1b8e42
5 changed files with 208 additions and 72 deletions

View file

@ -65,7 +65,8 @@ export default class MediaDbPlugin extends Plugin {
this.watchlist = new WatchlistController(this); this.watchlist = new WatchlistController(this);
// catch-up shortly after startup (let vault index settle), then hourly due-check // catch-up shortly after startup (let vault index settle), then hourly due-check
window.setTimeout(() => void this.watchlist.maybeCatchUp(), 30_000); const catchUpTimeout = window.setTimeout(() => void this.watchlist.maybeCatchUp(), 30_000);
this.register(() => window.clearTimeout(catchUpTimeout));
this.registerInterval(window.setInterval(() => void this.watchlist.maybeCatchUp(), 3600_000)); this.registerInterval(window.setInterval(() => void this.watchlist.maybeCatchUp(), 3600_000));
} }
@ -92,7 +93,7 @@ export default class MediaDbPlugin extends Plugin {
private registerRibbonAndFileMenu(): void { private registerRibbonAndFileMenu(): void {
const ribbonIconEl = this.addRibbonIcon('database', 'Add new Media DB entry', () => this.entryHelper.createEntryWithAdvancedSearchModal()); const ribbonIconEl = this.addRibbonIcon('database', 'Add new Media DB entry', () => this.entryHelper.createEntryWithAdvancedSearchModal());
ribbonIconEl.addClass('obsidian-media-db-plugin-ribbon-class'); ribbonIconEl.addClass('media-db-sync-ribbon-class');
this.registerEvent( this.registerEvent(
this.app.workspace.on('file-menu', (menu, file) => { this.app.workspace.on('file-menu', (menu, file) => {

View file

@ -3,7 +3,8 @@ import { buildRecord } from 'packages/obsidian/src/watchlist/build';
import { renderNote } from 'packages/obsidian/src/watchlist/render'; import { renderNote } from 'packages/obsidian/src/watchlist/render';
export interface SyncDeps { export interface SyncDeps {
listNotes(): Promise<{ path: string; content: string }[]>; listNotes(): Promise<{ path: string }[]>;
readNote(path: string): Promise<string>;
writeNote(path: string, content: string): Promise<void>; writeNote(path: string, content: string): Promise<void>;
fetchDetail(tmdbId: string, isMovie: boolean): Promise<any>; fetchDetail(tmdbId: string, isMovie: boolean): Promise<any>;
sleep(ms: number): Promise<void>; sleep(ms: number): Promise<void>;
@ -64,7 +65,10 @@ export async function syncFolder(deps: SyncDeps, opts: SyncOptions = {}): Promis
const notes = await deps.listNotes(); const notes = await deps.listNotes();
for (const note of notes) { for (const note of notes) {
report.scanned++; report.scanned++;
const { frontmatter, body } = parseNote(note.content); try {
// Read immediately before parse/diff so mid-sync edits aren't clobbered by a stale snapshot.
const content = await deps.readNote(note.path);
const { frontmatter, body } = parseNote(content);
const ref = noteTmdbRef(frontmatter); const ref = noteTmdbRef(frontmatter);
if (!ref) { if (!ref) {
report.skippedNoId++; report.skippedNoId++;
@ -74,7 +78,6 @@ export async function syncFolder(deps: SyncDeps, opts: SyncOptions = {}): Promis
report.skippedStatic++; report.skippedStatic++;
continue; continue;
} }
try {
const detail: any = await withRateLimitRetry(() => deps.fetchDetail(ref.tmdbId, ref.isMovie), deps.sleep); const detail: any = await withRateLimitRetry(() => deps.fetchDetail(ref.tmdbId, ref.isMovie), deps.sleep);
const record = buildRecord(detail, ref.isMovie, frontmatter); const record = buildRecord(detail, ref.isMovie, frontmatter);
const rendered = renderNote(record, extractMyNotes(body)); const rendered = renderNote(record, extractMyNotes(body));
@ -82,7 +85,7 @@ export async function syncFolder(deps: SyncDeps, opts: SyncOptions = {}): Promis
if (strip(frontmatter['watch_status']) === 'Watched' && record.watchStatus === 'Unwatched') { if (strip(frontmatter['watch_status']) === 'Watched' && record.watchStatus === 'Unwatched') {
report.flipped.push(note.path); report.flipped.push(note.path);
} }
if (rendered !== note.content) { if (rendered !== content) {
report.written++; report.written++;
if (!opts.dryRun) await deps.writeNote(note.path, rendered); if (!opts.dryRun) await deps.writeNote(note.path, rendered);
deps.log(`${opts.dryRun ? '[dry] ' : ''}updated ${note.path}`); deps.log(`${opts.dryRun ? '[dry] ' : ''}updated ${note.path}`);

View file

@ -25,7 +25,13 @@ function patchFrontmatter(content: string, tmdbId: string, mediaType: string): s
return `---\ntype: watchlist_item\n${insert}\n---\n\n` + content; return `---\ntype: watchlist_item\n${insert}\n---\n\n` + content;
} }
export function shouldNotifySync(quiet: boolean, written: number, errorCount: number): boolean {
return !quiet || written > 0 || errorCount > 0;
}
export class WatchlistController { export class WatchlistController {
private syncing = false;
constructor(private plugin: MediaDbPlugin) {} constructor(private plugin: MediaDbPlugin) {}
private getKey(): string { private getKey(): string {
@ -40,8 +46,8 @@ export class WatchlistController {
const res = await obsidianFetch(new Request(url, { headers })); const res = await obsidianFetch(new Request(url, { headers }));
if (res.status === 429) { if (res.status === 429) {
const err = new TmdbRateLimitError('TMDB 429'); const err = new TmdbRateLimitError('TMDB 429');
const ra = Number(res.headers.get('retry-after') ?? 2); const ra = Number(res.headers.get('retry-after'));
err.retryAfterMs = ra * 1000; err.retryAfterMs = Number.isFinite(ra) && ra > 0 ? ra * 1000 : 2000;
throw err; throw err;
} }
if (res.status !== 200) throw new Error(`TMDB ${res.status} for ${url}`); if (res.status !== 200) throw new Error(`TMDB ${res.status} for ${url}`);
@ -57,9 +63,12 @@ export class WatchlistController {
listNotes: async () => { listNotes: async () => {
if (!(folder instanceof TFolder)) throw new Error(`Watchlist folder not found: ${this.plugin.settings.watchlistFolder}`); if (!(folder instanceof TFolder)) throw new Error(`Watchlist folder not found: ${this.plugin.settings.watchlistFolder}`);
const files = folder.children.filter((f): f is TFile => f instanceof TFile && f.extension === 'md' && !f.name.startsWith('_')); const files = folder.children.filter((f): f is TFile => f instanceof TFile && f.extension === 'md' && !f.name.startsWith('_'));
const out: { path: string; content: string }[] = []; return files.map(f => ({ path: f.path }));
for (const f of files) out.push({ path: f.path, content: await app.vault.read(f) }); },
return out; readNote: async path => {
const f = app.vault.getAbstractFileByPath(path);
if (!(f instanceof TFile)) throw new Error(`Watchlist note not found: ${path}`);
return await app.vault.read(f);
}, },
writeNote: async (path, content) => { writeNote: async (path, content) => {
const f = app.vault.getAbstractFileByPath(path); const f = app.vault.getAbstractFileByPath(path);
@ -71,19 +80,30 @@ export class WatchlistController {
}; };
} }
async syncNow(full: boolean, dryRun = false): Promise<SyncReport> { async syncNow(full: boolean, dryRun = false, quiet = false): Promise<SyncReport> {
if (this.syncing) {
new Notice('Watchlist sync already running');
return { scanned: 0, synced: 0, written: 0, skippedNoId: 0, skippedStatic: 0, flipped: [], errors: [] };
}
this.syncing = true;
try {
const key = this.getKey(); const key = this.getKey();
const report = await syncFolder(this.makeDeps(key), { full, dryRun }); const report = await syncFolder(this.makeDeps(key), { full, dryRun });
const mode = dryRun ? 'DRY-RUN ' : ''; const mode = dryRun ? 'DRY-RUN ' : '';
if (shouldNotifySync(quiet, report.written, report.errors.length)) {
new Notice( new Notice(
`Watchlist ${mode}sync: ${report.synced} checked, ${report.written} updated, ${report.flipped.length} flipped to Unwatched` + `Watchlist ${mode}sync: ${report.synced} checked, ${report.written} updated, ${report.flipped.length} flipped to Unwatched` +
(report.errors.length ? `, ${report.errors.length} errors (see console)` : ''), (report.errors.length ? `, ${report.errors.length} errors (see console)` : ''),
); );
}
if (!dryRun) { if (!dryRun) {
this.plugin.settings.watchlistLastSync = Date.now(); this.plugin.settings.watchlistLastSync = Date.now();
await this.plugin.saveSettings(); await this.plugin.saveSettings();
} }
return report; return report;
} finally {
this.syncing = false;
}
} }
async maybeCatchUp(): Promise<void> { async maybeCatchUp(): Promise<void> {
@ -91,11 +111,17 @@ export class WatchlistController {
if (!s.watchlistEnabled) return; if (!s.watchlistEnabled) return;
const due = s.watchlistLastSync + s.watchlistSyncIntervalHours * 3600_000; const due = s.watchlistLastSync + s.watchlistSyncIntervalHours * 3600_000;
if (Date.now() >= due) { if (Date.now() >= due) {
await this.syncNow(false).catch(e => console.error('[media-db-sync] scheduled sync failed', e)); await this.syncNow(false, false, true).catch(e => console.error('[media-db-sync] scheduled sync failed', e));
} }
} }
async resolveMissingIds(dryRun = false): Promise<ResolveReport> { async resolveMissingIds(dryRun = false): Promise<ResolveReport> {
if (this.syncing) {
new Notice('Watchlist sync already running');
return { scanned: 0, resolved: 0, ambiguous: 0, errors: [] };
}
this.syncing = true;
try {
const key = this.getKey(); const key = this.getKey();
const deps = this.makeDeps(key); const deps = this.makeDeps(key);
const http = this.makeHttp(); const http = this.makeHttp();
@ -104,7 +130,8 @@ export class WatchlistController {
const report: ResolveReport = { scanned: 0, resolved: 0, ambiguous: 0, errors: [] }; const report: ResolveReport = { scanned: 0, resolved: 0, ambiguous: 0, errors: [] };
const notes = await deps.listNotes(); const notes = await deps.listNotes();
for (const note of notes) { for (const note of notes) {
const { frontmatter } = parseNote(note.content); const content = await deps.readNote(note.path);
const { frontmatter } = parseNote(content);
if (noteTmdbRef(frontmatter)) continue; if (noteTmdbRef(frontmatter)) continue;
const type = stripQuotes(frontmatter['type']); const type = stripQuotes(frontmatter['type']);
if (type && type !== 'watchlist_item') continue; // skip type: list dashboards etc. if (type && type !== 'watchlist_item') continue; // skip type: list dashboards etc.
@ -119,7 +146,7 @@ export class WatchlistController {
report.resolved++; report.resolved++;
const mediaType = result.isMovie ? 'Movie' : 'TV Series'; const mediaType = result.isMovie ? 'Movie' : 'TV Series';
deps.log(`${dryRun ? '[dry] ' : ''}resolved ${note.path} -> tmdb_id ${result.tmdbId} (${result.matchedTitle})`); deps.log(`${dryRun ? '[dry] ' : ''}resolved ${note.path} -> tmdb_id ${result.tmdbId} (${result.matchedTitle})`);
if (!dryRun) await deps.writeNote(note.path, patchFrontmatter(note.content, result.tmdbId, mediaType)); if (!dryRun) await deps.writeNote(note.path, patchFrontmatter(content, result.tmdbId, mediaType));
} }
await deps.sleep(250); await deps.sleep(250);
} catch (e) { } catch (e) {
@ -133,5 +160,8 @@ export class WatchlistController {
(report.errors.length ? `, ${report.errors.length} errors (see console)` : ''), (report.errors.length ? `, ${report.errors.length} errors (see console)` : ''),
); );
return report; return report;
} finally {
this.syncing = false;
}
} }
} }

View file

@ -1,5 +1,6 @@
import { describe, expect, test } from 'bun:test'; import { describe, expect, test } from 'bun:test';
import { WatchlistController } from 'packages/obsidian/src/watchlist/WatchlistController'; import { WatchlistController, shouldNotifySync } from 'packages/obsidian/src/watchlist/WatchlistController';
import type { SyncDeps } from 'packages/obsidian/src/watchlist/SyncEngine';
function fakePlugin(overrides: Partial<{ enabled: boolean; last: number; hours: number }> = {}) { function fakePlugin(overrides: Partial<{ enabled: boolean; last: number; hours: number }> = {}) {
return { return {
@ -38,3 +39,80 @@ describe('maybeCatchUp', () => {
expect(called).toBe(false); expect(called).toBe(false);
}); });
}); });
function deferredDeps(): { deps: SyncDeps; listNotesCalls: () => number; release: () => void } {
let listNotesCalls = 0;
let release!: () => void;
const gate = new Promise<void>(resolve => { release = resolve; });
const deps: SyncDeps = {
listNotes: async () => { listNotesCalls++; await gate; return []; },
readNote: async () => '',
writeNote: async () => {},
fetchDetail: async () => ({}),
sleep: async () => {},
log: () => {},
};
return { deps, listNotesCalls: () => listNotesCalls, release };
}
describe('concurrency guard', () => {
test('overlapping syncNow calls: second short-circuits while first is in flight', async () => {
const c = new WatchlistController(fakePlugin());
const { deps, listNotesCalls, release } = deferredDeps();
(c as any).getKey = () => 'fake-key';
(c as any).makeDeps = () => deps;
const first = c.syncNow(false);
const second = await c.syncNow(false);
expect(listNotesCalls()).toBe(1);
expect(second.scanned).toBe(0);
expect(second.written).toBe(0);
release();
const firstResult = await first;
expect(firstResult.scanned).toBe(0);
});
test('syncNow in flight blocks resolveMissingIds (shared guard)', async () => {
const c = new WatchlistController(fakePlugin());
const { deps, release } = deferredDeps();
(c as any).getKey = () => 'fake-key';
(c as any).makeDeps = () => deps;
const first = c.syncNow(false);
const resolveResult = await c.resolveMissingIds();
expect(resolveResult.scanned).toBe(0);
expect(resolveResult.resolved).toBe(0);
release();
await first;
});
test('flag resets after completion → next call runs normally', async () => {
const c = new WatchlistController(fakePlugin());
(c as any).getKey = () => 'fake-key';
let listNotesCalls = 0;
(c as any).makeDeps = () => ({
listNotes: async () => { listNotesCalls++; return []; },
readNote: async () => '',
writeNote: async () => {},
fetchDetail: async () => ({}),
sleep: async () => {},
log: () => {},
});
await c.syncNow(false);
await c.syncNow(false);
expect(listNotesCalls).toBe(2);
});
});
describe('shouldNotifySync', () => {
test('non-quiet → always notifies', () => expect(shouldNotifySync(false, 0, 0)).toBe(true));
test('quiet + no changes + no errors → suppressed', () => expect(shouldNotifySync(true, 0, 0)).toBe(false));
test('quiet + written>0 → notifies', () => expect(shouldNotifySync(true, 3, 0)).toBe(true));
test('quiet + errors>0 → notifies', () => expect(shouldNotifySync(true, 0, 2)).toBe(true));
});

View file

@ -23,16 +23,18 @@ keep me
const AIRING_NOTE = ENDED_NOTE.replace('status: Ended', 'status: Returning Series').replace('last_air_date: 2023-11-09', 'last_air_date: 2023-10-01'); const AIRING_NOTE = ENDED_NOTE.replace('status: Ended', 'status: Returning Series').replace('last_air_date: 2023-11-09', 'last_air_date: 2023-10-01');
function makeDeps(notes: { path: string; content: string }[], detail: any = tvDetail) { function makeDeps(notes: { path: string; content: string }[], detail: any = tvDetail) {
const contents = new Map(notes.map(n => [n.path, n.content]));
const writes: { path: string; content: string }[] = []; const writes: { path: string; content: string }[] = [];
const fetches: string[] = []; const fetches: string[] = [];
const deps: SyncDeps = { const deps: SyncDeps = {
listNotes: async () => notes, listNotes: async () => notes.map(n => ({ path: n.path })),
readNote: async path => contents.get(path)!,
writeNote: async (path, content) => { writes.push({ path, content }); }, writeNote: async (path, content) => { writes.push({ path, content }); },
fetchDetail: async (id) => { fetches.push(id); return detail; }, fetchDetail: async (id) => { fetches.push(id); return detail; },
sleep: async () => {}, sleep: async () => {},
log: () => {}, log: () => {},
}; };
return { deps, writes, fetches }; return { deps, writes, fetches, contents };
} }
describe('withRateLimitRetry', () => { describe('withRateLimitRetry', () => {
@ -133,7 +135,8 @@ describe('syncFolder', () => {
let calls = 0; let calls = 0;
const slept: number[] = []; const slept: number[] = [];
const deps: SyncDeps = { const deps: SyncDeps = {
listNotes: async () => [{ path: 'Loki.md', content: AIRING_NOTE }], listNotes: async () => [{ path: 'Loki.md' }],
readNote: async () => AIRING_NOTE,
writeNote: async () => {}, writeNote: async () => {},
fetchDetail: async () => { fetchDetail: async () => {
calls++; calls++;
@ -160,4 +163,25 @@ describe('syncFolder', () => {
expect(report.errors[0].path).toBe('Bad.md'); expect(report.errors[0].path).toBe('Bad.md');
expect(report.synced).toBe(1); expect(report.synced).toBe(1);
}); });
test('stale-read guard: content edited mid-sync is re-read fresh, not clobbered by early snapshot', async () => {
const { deps, writes, contents } = makeDeps([
{ path: 'A.md', content: AIRING_NOTE },
{ path: 'B.md', content: AIRING_NOTE },
]);
const originalFetch = deps.fetchDetail;
let calls = 0;
deps.fetchDetail = async (id, isMovie) => {
calls++;
if (calls === 1) {
// simulate the user editing B.md's "My Notes" while A.md is still mid-sync,
// i.e. after listNotes() ran but before B.md is actually processed.
contents.set('B.md', AIRING_NOTE.replace('keep me', 'edited during sync'));
}
return originalFetch(id, isMovie);
};
await syncFolder(deps);
const bWrite = writes.find(w => w.path === 'B.md');
expect(bWrite?.content).toContain('edited during sync');
expect(bWrite?.content).not.toContain('keep me');
});
}); });