From 152359ed2484180c2417886d37eeca6747d720f9 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Thu, 25 Jun 2026 13:12:23 +0200 Subject: [PATCH 1/4] Add verify-in-obsidian skill (relocated from developer-skills) --- .agents/skills/verify-in-obsidian/SKILL.md | 97 ++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 .agents/skills/verify-in-obsidian/SKILL.md diff --git a/.agents/skills/verify-in-obsidian/SKILL.md b/.agents/skills/verify-in-obsidian/SKILL.md new file mode 100644 index 0000000..a22a57a --- /dev/null +++ b/.agents/skills/verify-in-obsidian/SKILL.md @@ -0,0 +1,97 @@ +--- +name: verify-in-obsidian +description: >- + Verify an Obsidian plugin change/fix actually works by driving the REAL Obsidian app in + a dev vault via the `obsidian` CLI (eval / dev:console / dev:errors / dev:screenshot / + command / plugin:reload) and asserting on persisted state — instead of trusting unit + tests with mocks (jsdom can't load real Obsidian). Use when asked to verify/QA a fix, + confirm a feature works end-to-end, check that something persists, reproduce a + user-reported bug in the real app, or validate before merging — especially stateful UI + (settings, modals, drag-reorder, lists). FIRST read the repo's AGENTS.md/CLAUDE.md for + the vault name, plugin id, dev commands, and any QA matrix. +when_to_use: >- + "verify this works", "QA the fix", "does it persist / save", "reproduce the bug in the + real app", "check end-to-end before merge", "drive the GUI", "test in the dev vault". +--- + +# Verify Obsidian plugin changes in the real app + +Vitest/jsdom can't load real Obsidian, so unit tests run against mocks/stubs — they **pass +while the real flow is broken**. For anything stateful (settings, modals, drag-reorder, +persistence), "the unit test passes" is necessary but **not sufficient**. Verify the real +app and assert on what actually persisted. + +## Step 0 — read the repo's own guide first +`AGENTS.md` / `CLAUDE.md` hold the project specifics: **vault name, plugin id, dev-vault +path, build/test commands, and the QA matrix.** Use those, not guesses. Many Obsidian repos +already mandate CLI-verifiable development — follow it. + +## The `obsidian` CLI (the house tool for this) +Anything you can do in Obsidian you can do from the CLI, including dev tools. + +- **`vault=` is a PREFIX argument**, always first: `obsidian vault=dev …`. + The suffix form (`obsidian vault=dev`) can resolve to the wrong vault — don't use it. +- Useful commands: `plugin:reload id=`, `command id=`, `eval code=…`, + `dev:debug on|off`, `dev:console`, `dev:errors`, `dev:screenshot`, `dev:dom`, `dev:css`, + `dev:cdp`, `dev:mobile`, `devtools`, `tabs`, `workspace` (the repo's guide lists the full set). +- `dev:console` / `dev:errors` are only reliable while debugger capture is attached + (`obsidian vault=dev dev:debug on`). +- For non-trivial `eval` code, pass it via a **heredoc/file to `code=…`** to avoid + shell-quoting corruption. + +## The loop (evidence-first) +1. **Rebuild** the bundle (watch: `bun run dev`; one-off per the repo's build script). Note the + plugin `main.js` is often **symlinked** into the dev vault, so a rebuild updates it in place. +2. **Reload the plugin** — `obsidian vault=dev plugin:reload id=`. A rebuilt `main.js` + does NOT auto-reload in a running Obsidian. **This is the #1 false-negative**: never conclude + a fix "doesn't work" without confirming the running app loaded the fresh build. +3. **Reproduce / seed** deterministic state (edit `data.json` or use the plugin's data API), + then reload so it takes effect. +4. **Trigger** the behavior — `obsidian vault=dev command id=<…>`, or drive the UI via `eval` + (see gotchas). Test BOTH hotkey and direct-command paths when relevant. +5. **Gather evidence** — `dev:debug on` → `dev:console clear`/`dev:errors clear` → act → + `dev:console limit=200`, `dev:errors`, `dev:screenshot`, and `tabs`/`workspace ids` for layout. +6. **Assert on persisted state** — read the on-disk `data.json` (source of truth), not just the + UI. Confirm it's plain JSON with no framework artifacts (e.g. Svelte `$state` Proxy leakage). +7. **Add/adjust a regression test** around a CLI-native seam so it's caught without manual + steps. Structure code so Obsidian deps are injected behind interfaces; unit tests then swap + in adapters / the obsidian stub for the pure logic. +8. **Clean up** — remove seeded data + temp files; `dev:debug off`. + +## Evidence-first triage (for bug reports) +- **Don't assume the reported bug still exists** — it may have been fixed by unrelated changes. + Confirm current behavior in the dev vault before touching code. +- **Reproduce under real user conditions**, not synthetic ones: the actual plugin settings, + workspace/tab layout, and platform — and test BOTH the hotkey and direct-command + (`command id=…`) paths. +- **Capture evidence before AND after** the action (`dev:console`, `dev:errors`, + `dev:screenshot`, `tabs`, `workspace`). For pane/tab layout, `workspace … ids` is the + authoritative evidence; `tabs` is a quick summary. +- **If you can't reproduce** after solid evidence-gathering, report the exact setup you tested + and ask for a fresh issue with versions, config, and repro artifacts — don't guess-fix. + +## Driving UI via `eval` — gotchas (apply to raw `eval` and obsidian-e2e alike) +- `eval` may **return before a long async body finishes**. Don't chain `await sleep(...)` + inside one big eval and trust the return. Instead: **fast evals** (one DOM action + immediate + return), and **sequence timing outside** (shell/Node `sleep` between calls). +- **Return JSON-serializable values** (a string), never a live DOM node/object. +- **Selectors:** reuse components' `aria-label`s — they're stable and intent-named. +- **Stacked modals:** query all matches, act on the **last** (topmost). Count `.modal-container`. +- **State pollutes across runs:** close leftovers first (click all `.modal-close-button`, + close settings). +- **Read results reliably:** from a final UI-stable eval, or have the eval write to a vault + file and read it from disk. + +## Committed regression tests (`obsidian-e2e`, if available) +For flows worth guarding, port the check into the repo's e2e suite: +`createObsidianClient({ vault })`, a plugin handle, `acquireVaultRunLock`/`publishMarker` in +`beforeAll`, and `restoreData()` + lock release in `afterAll`. The handle exposes +`reload`, `data().patch(...)`, `exec(commandId, args)`, and `dev.eval(...)`. +**These are local-only** (CI has no Obsidian) — keep a CI-runnable unit/component test as the +primary guard and use the e2e test as the integration safety net. + +## The two lessons worth internalizing +1. **Stale loaded code** is the most common false negative — reload the plugin before + concluding anything about a fix. +2. **Verify the persisted state, not the UI** — and add the regression test against a + CLI-native seam, because mock-based unit tests will happily pass on a broken real flow. From af6ebe773c51106c14f8e9d8e9bddf8b5fcd4b3f Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Thu, 25 Jun 2026 22:28:07 +0200 Subject: [PATCH 2/4] fix(download): stream to a temp file then move into place Appending each range chunk straight to the final vault path made the growing, half-written media file visible to Obsidian's file watcher: a single download fired ~12 create/modify events on the .mp3. Watcher plugins (Waypoint, Dataview, Obsidian Git, MOC/index generators) re-scan the file on every event, and on a memory-constrained mobile WebView that re-scan storm - racing the chunked writer - OOM-crashes the app. Reported against Waypoint, but the cause is the event storm, not any one plugin. Stream every chunk to a dot-prefixed sibling temp that Obsidian keeps out of the file index (zero watcher events while it grows), then rename the completed, size-verified file into place - exactly one create of the finished file, the same shape the pre-#113 atomic createBinary path had. - prefer adapter.rename (in-place, buffers nothing, preserving the #113 memory win); fall back to copy+remove; never re-buffer the whole file, which would reintroduce the OOM this path exists to avoid - unique-per-attempt temp names + an active-set-guarded folder sweep, so colliding or concurrent downloads can't corrupt each other's temps and partials orphaned by a killed download get reclaimed - size verification still runs before the move; on failure the temp is removed, never the final path Validated on Android (Obsidian 1.12.7 + Waypoint 3.0.1): the matched scenario that OOM-crashed now completes, modify events on the final file drop from ~12 to 0 (one create), byte-perfect across megaphone, simplecast and wnyc, heap bounded, no orphan temp left behind. --- src/download/streaming.test.ts | 131 +++++++++++++++++++++++++++++++++ src/download/streaming.ts | 119 ++++++++++++++++++++++++++++-- src/downloadEpisode.test.ts | 120 ++++++++++++++++++++++++++++++ src/downloadEpisode.ts | 76 +++++++++++++------ 4 files changed, 420 insertions(+), 26 deletions(-) diff --git a/src/download/streaming.test.ts b/src/download/streaming.test.ts index 5aa33e8..4339108 100644 --- a/src/download/streaming.test.ts +++ b/src/download/streaming.test.ts @@ -3,7 +3,11 @@ import { requestUrl } from "obsidian"; import { plugin } from "../store"; import type PodNotes from "../main"; import { + isPartialPath, + moveIntoPlace, + partialPathFor, probeAndFetchFirstChunk, + sweepStalePartials, writeStreamedFile, type RangeProbe, } from "./streaming"; @@ -183,3 +187,130 @@ describe("writeStreamedFile", () => { ).rejects.toThrow(/Range request failed/); }); }); + +describe("partialPathFor / isPartialPath", () => { + it("builds a dot-prefixed sibling temp in the same folder", () => { + const tmp = partialPathFor("Podcasts/Show/Ep 1.mp3"); + expect(tmp).toMatch(/^Podcasts\/Show\/\.Ep 1\.mp3\..*\.podnotes-partial$/); + expect(isPartialPath(tmp)).toBe(true); + }); + + it("handles a vault-root (no folder) path", () => { + const tmp = partialPathFor("Ep 1.mp3"); + expect(tmp).toMatch(/^\.Ep 1\.mp3\..*\.podnotes-partial$/); + expect(tmp).not.toContain("/"); + expect(isPartialPath(tmp)).toBe(true); + }); + + it("is unique per call so concurrent downloads to one final path never clash", () => { + const a = partialPathFor("Podcasts/Ep.mp3"); + const b = partialPathFor("Podcasts/Ep.mp3"); + expect(a).not.toBe(b); + }); + + it("rejects non-partial paths", () => { + expect(isPartialPath("Podcasts/Ep.mp3")).toBe(false); + expect(isPartialPath("Podcasts/.Ep.mp3")).toBe(false); // dotfile, wrong suffix + expect(isPartialPath("Podcasts/Ep.mp3.podnotes-partial")).toBe(false); // no dot prefix + }); +}); + +describe("moveIntoPlace", () => { + function setupMoveAdapter(methods: { + rename?: boolean; + copy?: boolean; + remove?: boolean; + }) { + const adapter: Record = { + writeBinary: vi.fn(), + }; + const rename = vi.fn(async () => {}); + const copy = vi.fn(async () => {}); + const remove = vi.fn(async () => {}); + if (methods.rename) adapter.rename = rename; + if (methods.copy) adapter.copy = copy; + if (methods.remove) adapter.remove = remove; + plugin.set({ app: { vault: { adapter } } } as unknown as PodNotes); + return { rename, copy, remove }; + } + + it("prefers rename (in-place, buffers nothing) when available", async () => { + const m = setupMoveAdapter({ rename: true, copy: true, remove: true }); + await moveIntoPlace("dir/.ep.tok.podnotes-partial", "dir/ep.mp3"); + expect(m.rename).toHaveBeenCalledWith( + "dir/.ep.tok.podnotes-partial", + "dir/ep.mp3", + ); + expect(m.copy).not.toHaveBeenCalled(); + }); + + it("falls back to copy + remove when rename is unavailable", async () => { + const m = setupMoveAdapter({ copy: true, remove: true }); + await moveIntoPlace("dir/.ep.tok.podnotes-partial", "dir/ep.mp3"); + expect(m.copy).toHaveBeenCalledWith( + "dir/.ep.tok.podnotes-partial", + "dir/ep.mp3", + ); + expect(m.remove).toHaveBeenCalledWith("dir/.ep.tok.podnotes-partial"); + }); + + it("throws (never re-buffers the whole file) when neither rename nor copy exists", async () => { + // Re-buffering via readBinary→writeBinary would reintroduce the #113 OOM, so + // an adapter that can do neither must fail loudly instead. + setupMoveAdapter({ remove: true }); + await expect( + moveIntoPlace("dir/.ep.tok.podnotes-partial", "dir/ep.mp3"), + ).rejects.toThrow(/cannot move a completed download/i); + }); +}); + +describe("sweepStalePartials", () => { + function setupSweepAdapter(files: string[]) { + const remove = vi.fn(async () => {}); + const list = vi.fn(async () => ({ files, folders: [] as string[] })); + plugin.set({ + app: { vault: { adapter: { writeBinary: vi.fn(), list, remove } } }, + } as unknown as PodNotes); + return { remove, list }; + } + + it("removes orphaned partials but never an active one or a real file", async () => { + const s = setupSweepAdapter([ + "Podcasts/.Ep.dead.podnotes-partial", // orphan -> remove + "Podcasts/.Ep.live.podnotes-partial", // in flight -> keep + "Podcasts/Ep.mp3", // real file -> keep + ]); + + await sweepStalePartials( + "Podcasts", + (p) => p === "Podcasts/.Ep.live.podnotes-partial", + ); + + expect(s.remove).toHaveBeenCalledTimes(1); + expect(s.remove).toHaveBeenCalledWith("Podcasts/.Ep.dead.podnotes-partial"); + }); + + it("is a no-op when the adapter cannot list", async () => { + const remove = vi.fn(async () => {}); + plugin.set({ + app: { vault: { adapter: { writeBinary: vi.fn(), remove } } }, + } as unknown as PodNotes); + + await sweepStalePartials("Podcasts", () => false); + + expect(remove).not.toHaveBeenCalled(); + }); + + it("never throws when listing fails (best-effort)", async () => { + const list = vi.fn(async () => { + throw new Error("list failed"); + }); + plugin.set({ + app: { vault: { adapter: { writeBinary: vi.fn(), list, remove: vi.fn() } } }, + } as unknown as PodNotes); + + await expect( + sweepStalePartials("Podcasts", () => false), + ).resolves.toBeUndefined(); + }); +}); diff --git a/src/download/streaming.ts b/src/download/streaming.ts index 7078327..e0ad8a0 100644 --- a/src/download/streaming.ts +++ b/src/download/streaming.ts @@ -9,12 +9,32 @@ import { encodeUrlForRequest } from "../utility/encodeUrlForRequest"; // bridge copy — used to OOM-kill Obsidian on iOS. Instead we pull the file in // bounded HTTP Range chunks and append each straight to disk, so peak heap // stays at roughly one chunk regardless of episode size. +// +// ---- Temp-then-move (Waypoint et al. crash) -------------------------------- +// Appending chunks directly to the final vault path makes the growing, half- +// written media file visible to the vault's file watcher: a single download +// fires ~12 create/modify events. Watcher plugins (Waypoint, Dataview, Obsidian +// Git, MOC/index generators) react to each one and re-scan the partial file, +// and that synchronous re-scan storm — racing the chunked writer — OOM-crashes +// the app on mobile. So we stream every chunk to a dot-prefixed sibling temp +// (which Obsidian keeps out of the file index entirely, so it fires zero events +// while it grows), then move the finished, size-verified file into place as a +// single rename. Watchers then see exactly one create of an already-complete +// file — the same shape the pre-#113 atomic createBinary path produced. export const DOWNLOAD_CHUNK_SIZE = 4 * 1024 * 1024; // 4 MiB per range request export interface BinaryAppendAdapter { writeBinary(path: string, data: ArrayBuffer): Promise; appendBinary?(path: string, data: ArrayBuffer): Promise; + // Used to move a completed download from its temp path into the final vault + // path and to sweep orphaned temps. Present on the desktop and mobile + // Capacitor adapters but — like appendBinary — absent from Obsidian's public + // DataAdapter typings, so they live behind the same single cast below. + rename?(from: string, to: string): Promise; + copy?(from: string, to: string): Promise; + remove?(path: string): Promise; + list?(path: string): Promise<{ files: string[]; folders: string[] }>; } export interface RangeProbe { @@ -97,18 +117,20 @@ export async function probeAndFetchFirstChunk( } // Write the already-fetched first chunk, then pull the remaining bytes in -// bounded Range requests, appending each straight to disk. Peak memory is one -// chunk, not the whole file. Returns the total number of bytes written. +// bounded Range requests, appending each straight to `destPath`. Peak memory is +// one chunk, not the whole file. Returns the total number of bytes written. +// `destPath` is the temp path (see partialPathFor); the caller moves the result +// into the final vault path once it is complete and size-verified. export async function writeStreamedFile( url: string, - filePath: string, + destPath: string, probe: RangeProbe, onProgress?: (written: number, total: number | null) => void, chunkSize: number = DOWNLOAD_CHUNK_SIZE, ): Promise { const adapter = appendableAdapter(); - await adapter.writeBinary(filePath, probe.firstChunk); + await adapter.writeBinary(destPath, probe.firstChunk); let written = probe.firstChunk.byteLength; onProgress?.(written, probe.totalSize); @@ -144,7 +166,7 @@ export async function writeStreamedFile( const chunk = response.arrayBuffer; if (chunk.byteLength === 0) break; - await adapter.appendBinary(filePath, chunk); + await adapter.appendBinary(destPath, chunk); written += chunk.byteLength; onProgress?.(written, probe.totalSize); @@ -154,3 +176,90 @@ export async function writeStreamedFile( return written; } + +const PARTIAL_SUFFIX = ".podnotes-partial"; + +// A monotonic counter makes every temp name unique within a session even when two +// downloads resolve to the same final path (distinct episodes can collide on one +// path — #107/#183) and start in the same millisecond, so concurrent downloads +// never share a temp file. +let partialCounter = 0; + +// The sibling temp path a download streams to before being moved into place. It is +// dot-prefixed so Obsidian keeps it out of the file index (no watcher events while +// it grows), lives in the same folder as the final file so the move is a cheap +// in-place rename, and carries a unique token so concurrent downloads can't clash. +export function partialPathFor(filePath: string): string { + const slash = filePath.lastIndexOf("/"); + const dir = slash === -1 ? "" : filePath.slice(0, slash + 1); + const name = slash === -1 ? filePath : filePath.slice(slash + 1); + const token = `${Date.now().toString(36)}-${(partialCounter++).toString(36)}`; + return `${dir}.${name}.${token}${PARTIAL_SUFFIX}`; +} + +export function isPartialPath(path: string): boolean { + const name = path.slice(path.lastIndexOf("/") + 1); + return name.startsWith(".") && name.endsWith(PARTIAL_SUFFIX); +} + +// Move the completed temp file to its final vault path as a single operation, so +// watchers see one create of an already-complete file. Prefer rename: it is in- +// place (buffers zero bytes, preserving #113's memory win) and is the path every +// real adapter — desktop FileSystemAdapter, iOS/Android CapacitorAdapter — takes. +// copy+remove is a non-mobile fallback. We deliberately do NOT fall back to +// readBinary→writeBinary: re-buffering the whole 50-200 MB file would reintroduce +// the #113 OOM (and can trip the Android writeBinary large-file hang) on the exact +// devices this fix targets — so an adapter with neither rename nor copy fails loudly. +export async function moveIntoPlace( + tmpPath: string, + filePath: string, +): Promise { + const adapter = appendableAdapter(); + if (typeof adapter.rename === "function") { + await adapter.rename(tmpPath, filePath); + return; + } + if ( + typeof adapter.copy === "function" && + typeof adapter.remove === "function" + ) { + await adapter.copy(tmpPath, filePath); + await adapter.remove(tmpPath); + return; + } + throw new Error( + "Adapter cannot move a completed download into place (no rename/copy).", + ); +} + +// Remove temp partials orphaned in `folder` by a previous download that was hard- +// killed (OOM, force-quit) before it could move its file into place or clean up — +// otherwise hidden partials accumulate and can replicate via sync. `isActive` +// guards this and any concurrent download's live temp from being swept (temp names +// are unique per attempt, so a live temp would otherwise look like an orphan). +// Best-effort: a listing failure must never block a download. +export async function sweepStalePartials( + folder: string, + isActive: (path: string) => boolean, +): Promise { + const adapter = appendableAdapter(); + if ( + typeof adapter.list !== "function" || + typeof adapter.remove !== "function" + ) { + return; + } + try { + const listing = await adapter.list(folder); + for (const entry of listing.files) { + if (isPartialPath(entry) && !isActive(entry)) { + await adapter.remove(entry); + } + } + } catch (error) { + console.error( + `Failed to sweep stale download temp files in "${folder}":`, + error, + ); + } +} diff --git a/src/downloadEpisode.test.ts b/src/downloadEpisode.test.ts index 9e28f97..847ed3a 100644 --- a/src/downloadEpisode.test.ts +++ b/src/downloadEpisode.test.ts @@ -55,6 +55,29 @@ function setupVault({ streaming = false }: { streaming?: boolean } = {}) { const appendBinary = vi.fn(async (path: string, data: ArrayBuffer) => { written.set(path, (written.get(path) ?? 0) + data.byteLength); }); + // A raw adapter rename moves bytes on disk AND reconciles the destination into + // the vault index (Obsidian fires a create event for it), so the moved file is + // immediately resolvable for playback — mirroring the on-device behaviour. + const rename = vi.fn(async (from: string, to: string) => { + if (written.has(from)) { + written.set(to, written.get(from) ?? 0); + written.delete(from); + } + disk.delete(from); + disk.add(to); + present.add(to); + }); + // Immediate children of `folder`, mirroring DataAdapter.list (returns full + // vault-relative paths). Lets the stale-partial sweep find orphaned temps. + const list = vi.fn(async (folder: string) => { + const prefix = folder ? `${folder}/` : ""; + const files: string[] = []; + for (const p of disk) { + if (!p.startsWith(prefix)) continue; + if (!p.slice(prefix.length).includes("/")) files.push(p); + } + return { files, folders: [] as string[] }; + }); const adapter: Record = { exists: async (path: string) => disk.has(path) || present.has(path), @@ -63,6 +86,8 @@ function setupVault({ streaming = false }: { streaming?: boolean } = {}) { if (streaming) { adapter.writeBinary = writeBinary; adapter.appendBinary = appendBinary; + adapter.rename = rename; + adapter.list = list; } const app = { @@ -91,6 +116,8 @@ function setupVault({ streaming = false }: { streaming?: boolean } = {}) { removeFile, writeBinary, appendBinary, + rename, + list, written, }; } @@ -1241,6 +1268,99 @@ describe("downloadEpisodeWithNotice (streaming range path)", () => { expect(recorded?.size).toBe(16); }); + it("streams to a dot-prefixed temp the watchers don't see, then renames it into place", async () => { + const v = setupVault({ streaming: true }); + requestUrlMock + .mockResolvedValueOnce( + rangeResponse(206, [1, 2, 3, 4, 5, 6, 7, 8], { + "content-type": "audio/mpeg", + "content-range": "bytes 0-7/16", + }), + ) + .mockResolvedValueOnce( + rangeResponse(206, [9, 10, 11, 12, 13, 14, 15, 16], { + "content-range": "bytes 8-15/16", + }), + ); + + await downloadEpisodeWithNotice(makeEpisode(), "Podcasts/{{title}}"); + + // Every chunk went to a single temp path, and it was a hidden sibling partial + // (dot-prefixed, in the same folder) — never the final media path. + const [writePath] = v.writeBinary.mock.calls[0]; + const [appendPath] = v.appendBinary.mock.calls[0]; + expect(writePath).toBe(appendPath); + expect(writePath).toMatch( + /^Podcasts\/\.My Title\.mp3\..*\.podnotes-partial$/, + ); + expect(writePath).not.toBe("Podcasts/My Title.mp3"); + + // Exactly one move into the real path; the temp does not survive. + expect(v.rename).toHaveBeenCalledTimes(1); + expect(v.rename).toHaveBeenCalledWith(writePath, "Podcasts/My Title.mp3"); + expect(v.disk.has(writePath)).toBe(false); + // The moved file is index-resolvable, so playback (getAbstractFileByPath) + // resolves it rather than silently binding src="". + expect(v.present.has("Podcasts/My Title.mp3")).toBe(true); + }); + + it("cleans up the temp (not the final path) when the move into place fails", async () => { + const v = setupVault({ streaming: true }); + v.rename.mockRejectedValueOnce(new Error("rename boom")); + requestUrlMock + .mockResolvedValueOnce( + rangeResponse(206, [1, 2, 3, 4, 5, 6, 7, 8], { + "content-type": "audio/mpeg", + "content-range": "bytes 0-7/16", + }), + ) + .mockResolvedValueOnce( + rangeResponse(206, [9, 10, 11, 12, 13, 14, 15, 16], { + "content-range": "bytes 8-15/16", + }), + ); + + await expect( + downloadEpisodeWithNotice(makeEpisode(), "Podcasts/{{title}}"), + ).rejects.toThrow(/rename boom/); + + const [tmpPath] = v.writeBinary.mock.calls[0]; + // The failed move removed the temp via the adapter, and the final path was + // never created or registered. + expect(v.removeFile).toHaveBeenCalledWith(tmpPath); + expect(v.disk.has(tmpPath)).toBe(false); + expect(v.present.has("Podcasts/My Title.mp3")).toBe(false); + expect(get(downloadedEpisodes)["Pod"]).toBeUndefined(); + }); + + it("sweeps an orphaned partial from a prior killed download before streaming", async () => { + const v = setupVault({ streaming: true }); + // A partial left behind in the target folder by a download that was killed + // mid-stream (the OOM crash this fix addresses). + v.disk.add("Podcasts/.My Title.mp3.dead-orphan.podnotes-partial"); + // An unrelated real file in the same folder must be left untouched. + v.disk.add("Podcasts/Keep Me.mp3"); + requestUrlMock + .mockResolvedValueOnce( + rangeResponse(206, [1, 2, 3, 4, 5, 6, 7, 8], { + "content-type": "audio/mpeg", + "content-range": "bytes 0-7/16", + }), + ) + .mockResolvedValueOnce( + rangeResponse(206, [9, 10, 11, 12, 13, 14, 15, 16], { + "content-range": "bytes 8-15/16", + }), + ); + + await downloadEpisodeWithNotice(makeEpisode(), "Podcasts/{{title}}"); + + expect(v.removeFile).toHaveBeenCalledWith( + "Podcasts/.My Title.mp3.dead-orphan.podnotes-partial", + ); + expect(v.disk.has("Podcasts/Keep Me.mp3")).toBe(true); + }); + it("writes the whole body in one shot when the server ignores Range (200)", async () => { const v = setupVault({ streaming: true }); requestUrlMock.mockResolvedValue( diff --git a/src/downloadEpisode.ts b/src/downloadEpisode.ts index fdbb02c..54c5979 100644 --- a/src/downloadEpisode.ts +++ b/src/downloadEpisode.ts @@ -25,7 +25,10 @@ import { } from "./utility/mediaType"; import { appendableAdapter, + moveIntoPlace, + partialPathFor, probeAndFetchFirstChunk, + sweepStalePartials, writeStreamedFile, } from "./download/streaming"; import { detectAudioFileExtension } from "./download/mediaSignatures"; @@ -81,6 +84,10 @@ function parentFolderPath(filePath: string): string { // into a single transfer so they can't stack N full downloads in memory. const downloadsInFlight = new Map>(); +// Temp paths of downloads currently streaming, so the stale-partial sweep never +// deletes a live temp (its own, or a concurrent download into the same folder). +const activePartialPaths = new Set(); + // Single source of truth for "what extension and on-disk path does this download // get, and is it even playable". Shared by the streaming and legacy paths so the // playability rule (#DL-07 / #213) and path resolution live in exactly one place. @@ -163,30 +170,57 @@ async function downloadEpisodeToDisk( await ensureFolderExists(parentFolderPath(filePath)); - // Clean up a half-written file on any mid-stream failure so a later attempt - // can't mistake a truncated partial for a complete download — the legacy - // createBinary path was atomic, but chunked appendBinary is not. - let total: number; + // Stream to a sibling temp the vault watchers never see, then move the finished + // file into place as one rename — so watcher plugins (Waypoint, Dataview, + // Obsidian Git, ...) don't get a per-chunk modify storm on the growing media + // file and re-scan it into an OOM crash. See streaming.ts for the mechanism. + const tmpPath = partialPathFor(filePath); + activePartialPaths.add(tmpPath); try { - total = await writeStreamedFile( - episode.streamUrl, - filePath, - probe, - onProgress, - ); - } catch (error) { - await deleteEpisodeFile(filePath); - throw error; - } - if (probe.totalSize !== null && total !== probe.totalSize) { - await deleteEpisodeFile(filePath); - throw new Error( - `Incomplete download: got ${total} of ${probe.totalSize} bytes.`, + // Reclaim temps orphaned by a previous download killed mid-stream (the very + // crash this fix addresses). The active set protects this and any concurrent + // download's live temp from being swept. + await sweepStalePartials( + parentFolderPath(filePath), + (path) => activePartialPaths.has(path), ); - } - downloadedEpisodes.addEpisode(episode, filePath, total); - return filePath; + // Clean up the temp (never the final path) on any mid-stream failure so a + // later attempt can't mistake a truncated partial for a complete download — + // the legacy createBinary path was atomic, but chunked appendBinary is not. + let total: number; + try { + total = await writeStreamedFile( + episode.streamUrl, + tmpPath, + probe, + onProgress, + ); + } catch (error) { + await deleteEpisodeFile(tmpPath); + throw error; + } + if (probe.totalSize !== null && total !== probe.totalSize) { + await deleteEpisodeFile(tmpPath); + throw new Error( + `Incomplete download: got ${total} of ${probe.totalSize} bytes.`, + ); + } + + // Only a fully-written, size-verified file is exposed to the vault — as one + // atomic move, so a partial never even momentarily occupies the real path. + try { + await moveIntoPlace(tmpPath, filePath); + } catch (error) { + await deleteEpisodeFile(tmpPath); + throw error; + } + + downloadedEpisodes.addEpisode(episode, filePath, total); + return filePath; + } finally { + activePartialPaths.delete(tmpPath); + } } export default async function downloadEpisodeWithNotice( From efd8ee6cea8f22b66c9f601312ce0cb8f204dc77 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Thu, 25 Jun 2026 22:41:57 +0200 Subject: [PATCH 3/4] fix(download): cap the streaming temp filename to the OS limit (#22) partialPathFor prepended a dot and appended "..podnotes-partial" to the already-length-capped final name, which could push the temp's name segment past the 255-unit ENAMETOOLONG limit for a maxed-out title - so a very long episode that downloaded fine before could fail on the new temp-then-move write path. Put the unique token first (so it survives the tail truncation) and run the temp path through enforceMaxPathLength. --- src/download/streaming.test.ts | 17 +++++++++++++++-- src/download/streaming.ts | 7 ++++++- src/downloadEpisode.test.ts | 2 +- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/download/streaming.test.ts b/src/download/streaming.test.ts index 4339108..2a41726 100644 --- a/src/download/streaming.test.ts +++ b/src/download/streaming.test.ts @@ -191,13 +191,14 @@ describe("writeStreamedFile", () => { describe("partialPathFor / isPartialPath", () => { it("builds a dot-prefixed sibling temp in the same folder", () => { const tmp = partialPathFor("Podcasts/Show/Ep 1.mp3"); - expect(tmp).toMatch(/^Podcasts\/Show\/\.Ep 1\.mp3\..*\.podnotes-partial$/); + // dir + "...podnotes-partial" (token first so it survives the cap) + expect(tmp).toMatch(/^Podcasts\/Show\/\..*\.Ep 1\.mp3\.podnotes-partial$/); expect(isPartialPath(tmp)).toBe(true); }); it("handles a vault-root (no folder) path", () => { const tmp = partialPathFor("Ep 1.mp3"); - expect(tmp).toMatch(/^\.Ep 1\.mp3\..*\.podnotes-partial$/); + expect(tmp).toMatch(/^\..*\.Ep 1\.mp3\.podnotes-partial$/); expect(tmp).not.toContain("/"); expect(isPartialPath(tmp)).toBe(true); }); @@ -208,6 +209,18 @@ describe("partialPathFor / isPartialPath", () => { expect(a).not.toBe(b); }); + it("keeps the temp name within the filesystem limit for a maxed-out title (#22)", () => { + // The final name segment is already capped to ~255; the dot prefix + suffix + // must not push the temp past ENAMETOOLONG on the new write path. + const longFinal = `Podcasts/${"X".repeat(400)}.mp3`; + const tmp = partialPathFor(longFinal); + const lastSegment = tmp.split("/").pop() ?? ""; + expect(lastSegment.length).toBeLessThanOrEqual(255); + expect(isPartialPath(tmp)).toBe(true); + // The unique token (front of the name) must survive the tail truncation. + expect(partialPathFor(longFinal)).not.toBe(tmp); + }); + it("rejects non-partial paths", () => { expect(isPartialPath("Podcasts/Ep.mp3")).toBe(false); expect(isPartialPath("Podcasts/.Ep.mp3")).toBe(false); // dotfile, wrong suffix diff --git a/src/download/streaming.ts b/src/download/streaming.ts index e0ad8a0..68a142c 100644 --- a/src/download/streaming.ts +++ b/src/download/streaming.ts @@ -2,6 +2,7 @@ import { requestUrl } from "obsidian"; import { get } from "svelte/store"; import { plugin } from "../store"; import { encodeUrlForRequest } from "../utility/encodeUrlForRequest"; +import { enforceMaxPathLength } from "../utility/enforceMaxPathLength"; // ---- Streaming download (issue #113) --------------------------------------- // Mobile WebViews have a tight per-process memory budget. Buffering a whole @@ -189,12 +190,16 @@ let partialCounter = 0; // dot-prefixed so Obsidian keeps it out of the file index (no watcher events while // it grows), lives in the same folder as the final file so the move is a cheap // in-place rename, and carries a unique token so concurrent downloads can't clash. +// The token comes first so it survives the length cap (#22), which trims the tail: +// the final name's own segment is already at the byte limit, and prepending a dot + +// appending the suffix would otherwise push the temp past ENAMETOOLONG. The embedded +// final name is only there to make the temp recognisable while debugging. export function partialPathFor(filePath: string): string { const slash = filePath.lastIndexOf("/"); const dir = slash === -1 ? "" : filePath.slice(0, slash + 1); const name = slash === -1 ? filePath : filePath.slice(slash + 1); const token = `${Date.now().toString(36)}-${(partialCounter++).toString(36)}`; - return `${dir}.${name}.${token}${PARTIAL_SUFFIX}`; + return enforceMaxPathLength(`${dir}.${token}.${name}${PARTIAL_SUFFIX}`, PARTIAL_SUFFIX); } export function isPartialPath(path: string): boolean { diff --git a/src/downloadEpisode.test.ts b/src/downloadEpisode.test.ts index 847ed3a..887176e 100644 --- a/src/downloadEpisode.test.ts +++ b/src/downloadEpisode.test.ts @@ -1291,7 +1291,7 @@ describe("downloadEpisodeWithNotice (streaming range path)", () => { const [appendPath] = v.appendBinary.mock.calls[0]; expect(writePath).toBe(appendPath); expect(writePath).toMatch( - /^Podcasts\/\.My Title\.mp3\..*\.podnotes-partial$/, + /^Podcasts\/\..*\.My Title\.mp3\.podnotes-partial$/, ); expect(writePath).not.toBe("Podcasts/My Title.mp3"); From 948ab772d396326799dd154573d22b3182edfee7 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Thu, 25 Jun 2026 22:50:56 +0200 Subject: [PATCH 4/4] refactor(download): simplify temp-then-move to the typed DataAdapter API rename/copy/remove/list are public, required members of Obsidian's DataAdapter - only appendBinary is runtime-only - so the optional guards and the copy+remove fallback in moveIntoPlace guarded a state that cannot occur (anything past the writeBinary+appendBinary gate is a full DataAdapter with rename). Extend the real DataAdapter for the single appendBinary bolt-on, correct the now-accurate comment, finalize with a plain adapter.rename, and drop the unreachable fallback - which also removes a latent "copy succeeded but remove threw, so a complete download is reported failed" bug. Collapse the streaming branch's three catch->delete->rethrow blocks into one finally guard. --- src/download/streaming.test.ts | 58 ++++------------------------------ src/download/streaming.ts | 54 ++++++++----------------------- src/downloadEpisode.ts | 42 ++++++++++-------------- 3 files changed, 37 insertions(+), 117 deletions(-) diff --git a/src/download/streaming.test.ts b/src/download/streaming.test.ts index 2a41726..1b2cec1 100644 --- a/src/download/streaming.test.ts +++ b/src/download/streaming.test.ts @@ -229,51 +229,18 @@ describe("partialPathFor / isPartialPath", () => { }); describe("moveIntoPlace", () => { - function setupMoveAdapter(methods: { - rename?: boolean; - copy?: boolean; - remove?: boolean; - }) { - const adapter: Record = { - writeBinary: vi.fn(), - }; + it("moves the temp into place with a single rename (buffers nothing)", async () => { const rename = vi.fn(async () => {}); - const copy = vi.fn(async () => {}); - const remove = vi.fn(async () => {}); - if (methods.rename) adapter.rename = rename; - if (methods.copy) adapter.copy = copy; - if (methods.remove) adapter.remove = remove; - plugin.set({ app: { vault: { adapter } } } as unknown as PodNotes); - return { rename, copy, remove }; - } + plugin.set({ + app: { vault: { adapter: { writeBinary: vi.fn(), rename } } }, + } as unknown as PodNotes); - it("prefers rename (in-place, buffers nothing) when available", async () => { - const m = setupMoveAdapter({ rename: true, copy: true, remove: true }); - await moveIntoPlace("dir/.ep.tok.podnotes-partial", "dir/ep.mp3"); - expect(m.rename).toHaveBeenCalledWith( - "dir/.ep.tok.podnotes-partial", - "dir/ep.mp3", - ); - expect(m.copy).not.toHaveBeenCalled(); - }); + await moveIntoPlace("dir/.tok.ep.mp3.podnotes-partial", "dir/ep.mp3"); - it("falls back to copy + remove when rename is unavailable", async () => { - const m = setupMoveAdapter({ copy: true, remove: true }); - await moveIntoPlace("dir/.ep.tok.podnotes-partial", "dir/ep.mp3"); - expect(m.copy).toHaveBeenCalledWith( - "dir/.ep.tok.podnotes-partial", + expect(rename).toHaveBeenCalledWith( + "dir/.tok.ep.mp3.podnotes-partial", "dir/ep.mp3", ); - expect(m.remove).toHaveBeenCalledWith("dir/.ep.tok.podnotes-partial"); - }); - - it("throws (never re-buffers the whole file) when neither rename nor copy exists", async () => { - // Re-buffering via readBinary→writeBinary would reintroduce the #113 OOM, so - // an adapter that can do neither must fail loudly instead. - setupMoveAdapter({ remove: true }); - await expect( - moveIntoPlace("dir/.ep.tok.podnotes-partial", "dir/ep.mp3"), - ).rejects.toThrow(/cannot move a completed download/i); }); }); @@ -303,17 +270,6 @@ describe("sweepStalePartials", () => { expect(s.remove).toHaveBeenCalledWith("Podcasts/.Ep.dead.podnotes-partial"); }); - it("is a no-op when the adapter cannot list", async () => { - const remove = vi.fn(async () => {}); - plugin.set({ - app: { vault: { adapter: { writeBinary: vi.fn(), remove } } }, - } as unknown as PodNotes); - - await sweepStalePartials("Podcasts", () => false); - - expect(remove).not.toHaveBeenCalled(); - }); - it("never throws when listing fails (best-effort)", async () => { const list = vi.fn(async () => { throw new Error("list failed"); diff --git a/src/download/streaming.ts b/src/download/streaming.ts index 68a142c..8700c2e 100644 --- a/src/download/streaming.ts +++ b/src/download/streaming.ts @@ -1,4 +1,4 @@ -import { requestUrl } from "obsidian"; +import { type DataAdapter, requestUrl } from "obsidian"; import { get } from "svelte/store"; import { plugin } from "../store"; import { encodeUrlForRequest } from "../utility/encodeUrlForRequest"; @@ -25,17 +25,12 @@ import { enforceMaxPathLength } from "../utility/enforceMaxPathLength"; export const DOWNLOAD_CHUNK_SIZE = 4 * 1024 * 1024; // 4 MiB per range request -export interface BinaryAppendAdapter { - writeBinary(path: string, data: ArrayBuffer): Promise; +// Obsidian's DataAdapter — writeBinary/rename/remove/list, all used below — is +// fully typed and public. Only `appendBinary` exists at runtime on the desktop and +// mobile Capacitor adapters without appearing in the public typings, so we extend +// the real interface with that single bolt-on and keep one cast (appendableAdapter). +export interface BinaryAppendAdapter extends DataAdapter { appendBinary?(path: string, data: ArrayBuffer): Promise; - // Used to move a completed download from its temp path into the final vault - // path and to sweep orphaned temps. Present on the desktop and mobile - // Capacitor adapters but — like appendBinary — absent from Obsidian's public - // DataAdapter typings, so they live behind the same single cast below. - rename?(from: string, to: string): Promise; - copy?(from: string, to: string): Promise; - remove?(path: string): Promise; - list?(path: string): Promise<{ files: string[]; folders: string[] }>; } export interface RangeProbe { @@ -207,34 +202,17 @@ export function isPartialPath(path: string): boolean { return name.startsWith(".") && name.endsWith(PARTIAL_SUFFIX); } -// Move the completed temp file to its final vault path as a single operation, so -// watchers see one create of an already-complete file. Prefer rename: it is in- -// place (buffers zero bytes, preserving #113's memory win) and is the path every -// real adapter — desktop FileSystemAdapter, iOS/Android CapacitorAdapter — takes. -// copy+remove is a non-mobile fallback. We deliberately do NOT fall back to -// readBinary→writeBinary: re-buffering the whole 50-200 MB file would reintroduce -// the #113 OOM (and can trip the Android writeBinary large-file hang) on the exact -// devices this fix targets — so an adapter with neither rename nor copy fails loudly. +// Move the completed temp file to its final vault path with a single rename, so +// watchers see one create of an already-complete file. rename is a standard +// DataAdapter op on every platform and, because the temp is a sibling of the final +// file, an in-place metadata move that buffers zero bytes — preserving #113's +// memory win. (We never finalize by reading the temp back into memory and +// re-writing it: that whole-file buffer is exactly the #113 OOM this path avoids.) export async function moveIntoPlace( tmpPath: string, filePath: string, ): Promise { - const adapter = appendableAdapter(); - if (typeof adapter.rename === "function") { - await adapter.rename(tmpPath, filePath); - return; - } - if ( - typeof adapter.copy === "function" && - typeof adapter.remove === "function" - ) { - await adapter.copy(tmpPath, filePath); - await adapter.remove(tmpPath); - return; - } - throw new Error( - "Adapter cannot move a completed download into place (no rename/copy).", - ); + await appendableAdapter().rename(tmpPath, filePath); } // Remove temp partials orphaned in `folder` by a previous download that was hard- @@ -248,12 +226,6 @@ export async function sweepStalePartials( isActive: (path: string) => boolean, ): Promise { const adapter = appendableAdapter(); - if ( - typeof adapter.list !== "function" || - typeof adapter.remove !== "function" - ) { - return; - } try { const listing = await adapter.list(folder); for (const entry of listing.files) { diff --git a/src/downloadEpisode.ts b/src/downloadEpisode.ts index 54c5979..59b16c2 100644 --- a/src/downloadEpisode.ts +++ b/src/downloadEpisode.ts @@ -85,7 +85,7 @@ function parentFolderPath(filePath: string): string { const downloadsInFlight = new Map>(); // Temp paths of downloads currently streaming, so the stale-partial sweep never -// deletes a live temp (its own, or a concurrent download into the same folder). +// deletes a concurrent download's live temp when it sweeps the shared folder. const activePartialPaths = new Set(); // Single source of truth for "what extension and on-disk path does this download @@ -176,49 +176,41 @@ async function downloadEpisodeToDisk( // file and re-scan it into an OOM crash. See streaming.ts for the mechanism. const tmpPath = partialPathFor(filePath); activePartialPaths.add(tmpPath); + let moved = false; try { // Reclaim temps orphaned by a previous download killed mid-stream (the very - // crash this fix addresses). The active set protects this and any concurrent + // crash this fix addresses). The active set protects any concurrent // download's live temp from being swept. await sweepStalePartials( parentFolderPath(filePath), (path) => activePartialPaths.has(path), ); - // Clean up the temp (never the final path) on any mid-stream failure so a - // later attempt can't mistake a truncated partial for a complete download — - // the legacy createBinary path was atomic, but chunked appendBinary is not. - let total: number; - try { - total = await writeStreamedFile( - episode.streamUrl, - tmpPath, - probe, - onProgress, - ); - } catch (error) { - await deleteEpisodeFile(tmpPath); - throw error; - } + const total = await writeStreamedFile( + episode.streamUrl, + tmpPath, + probe, + onProgress, + ); if (probe.totalSize !== null && total !== probe.totalSize) { - await deleteEpisodeFile(tmpPath); throw new Error( `Incomplete download: got ${total} of ${probe.totalSize} bytes.`, ); } // Only a fully-written, size-verified file is exposed to the vault — as one - // atomic move, so a partial never even momentarily occupies the real path. - try { - await moveIntoPlace(tmpPath, filePath); - } catch (error) { - await deleteEpisodeFile(tmpPath); - throw error; - } + // atomic rename, so a partial never even momentarily occupies the real path. + await moveIntoPlace(tmpPath, filePath); + moved = true; downloadedEpisodes.addEpisode(episode, filePath, total); return filePath; } finally { + // If the file never made it into place (stream, size or move failure), drop + // the temp so a later attempt can't mistake a truncated partial for a + // complete download — the legacy createBinary path was atomic, chunked + // appendBinary is not. deleteEpisodeFile is a no-op when the temp is absent. + if (!moved) await deleteEpisodeFile(tmpPath); activePartialPaths.delete(tmpPath); } }