From e8f1d4ed4c04b170bf796c3924e66874dc5a78ee Mon Sep 17 00:00:00 2001 From: Anthony Ettinger Date: Thu, 13 Aug 2026 10:40:43 +0000 Subject: [PATCH] fix(sync): retry /save when the app does not answer, and carry feeds.opml Two changes to settings sync. **Retry on 5xx.** `request()` was a single fetch: any non-2xx became a value the caller printed before exiting, so one transient 502 was a visible failure. app.moshcode.sh returns "Application failed to respond" intermittently - a probe of the write path measured 20/20 healthy minutes after a real save had failed with exactly that - and unretried, every blip is a failed save. It felt frequent because every one of them was. Retried only where a retry can be right. 502, 503, 504 and a dead socket say nothing about the request: the platform returned them without the app seeing it, so the same bytes a second later are as likely to work as they were the first time. Every other status is an answer - 400 will be 400 again, 401 wants /login, 409 is another machine having saved first and retrying would only lose the same race again. Safe to repeat a PUT because the write is conditional: `ifRevision` pins the revision the caller last agreed on, so a retry landing after a first attempt secretly succeeded is refused with 409 rather than writing twice. Two retries at 400ms and 1200ms, then it is news. The scan workflow this repository ships already retries `npm install` three times for precisely this reasoning; the sync client had none. **feeds.opml joins SYNCED_FILES.** A feed list is exactly the sort of thing that should follow a person to their next machine, and OPML is the format every reader already imports and exports. `json: false`: moshcode's interest in the file begins and ends with moving it, so unlike aliases.json there is no such thing as a broken one here. The server needs nothing. It validates shape rather than filenames - no `..`, no leading slash, 32 files, 256KB - so it already accepts this. Five tests: OPML travels byte for byte, a non-XML feed list still syncs, a 502 is ridden out without a line about it, a 409 is asked exactly once, and a 502 that never clears is still reported rather than retried forever. Co-Authored-By: Claude Opus 5 (1M context) --- src/settings-sync.mjs | 38 +++++++++++++++++++++- test/settings-sync.test.mjs | 63 ++++++++++++++++++++++++++++++++++++- 2 files changed, 99 insertions(+), 2 deletions(-) diff --git a/src/settings-sync.mjs b/src/settings-sync.mjs index 0ea1f6b..40344b6 100644 --- a/src/settings-sync.mjs +++ b/src/settings-sync.mjs @@ -58,6 +58,12 @@ export const MAX_TOTAL_BYTES = 256 * 1024; export const SYNCED_FILES = [ { path: "aliases.json", json: true, label: "pit aliases" }, { path: "herd/rules.json", json: true, label: "herd state rules" }, + // OPML rather than JSON, so `json: false`: it is the interchange format every + // feed reader already imports and exports, which is the whole reason a feed + // list is worth carrying between machines at all. Written by `tcfeed rss add` + // and read by nothing here — moshcode's interest in it begins and ends with + // moving it, and a file this does not parse cannot be broken by this. + { path: "feeds.opml", json: false, label: "rss feeds" }, ]; /** @@ -349,7 +355,7 @@ function endpoint(creds) { * code; a thrown network error inside the pit's dispatch loop would take the * prompt down instead, which is a lost session over a dropped wifi connection. */ -async function request(method, route, { creds, body = null, fetchImpl = fetch, timeoutMs = 20_000 } = {}) { +async function attempt(method, route, { creds, body, fetchImpl, timeoutMs }) { const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), timeoutMs); try { @@ -374,6 +380,36 @@ async function request(method, route, { creds, body = null, fetchImpl = fetch, t } } +/** + * Retried only where a retry can be right: the app not answering. + * + * 502, 503, 504 and a dead socket say nothing about the request — the platform + * returned them without the app seeing it, so the same bytes sent a second + * later are as likely to work as they were the first time. Every other status + * is an answer: 400 will be 400 again, 401 wants `/login`, and 409 is another + * machine having saved first, which retrying would only re-ask. + * + * Unretried, one blip is a visible failure — and it looked frequent because + * every one of them was. `/save` had no retry at all while the scan workflow + * this same repository ships retries `npm install` three times for precisely + * this reasoning. + * + * Safe to repeat a PUT because the write is conditional: `ifRevision` pins the + * revision the caller last agreed on, so a retry that lands after a first + * attempt secretly succeeded is refused with 409 rather than writing twice. + */ +const RETRY_STATUS = new Set([0, 502, 503, 504]); +const RETRY_BACKOFF_MS = [400, 1200]; + +async function request(method, route, { creds, body = null, fetchImpl = fetch, timeoutMs = 20_000, retries = RETRY_BACKOFF_MS.length } = {}) { + let last; + for (let i = 0; ; i++) { + last = await attempt(method, route, { creds, body, fetchImpl, timeoutMs }); + if (!RETRY_STATUS.has(last.status) || i >= retries) return last; + await new Promise((r) => setTimeout(r, RETRY_BACKOFF_MS[i] ?? RETRY_BACKOFF_MS.at(-1))); + } +} + export const pushSnapshot = (snapshot, { ifRevision = null, ...opts }) => request("PUT", "/api/settings", { ...opts, body: { snapshot, ifRevision } }); diff --git a/test/settings-sync.test.mjs b/test/settings-sync.test.mjs index d05307b..848428f 100644 --- a/test/settings-sync.test.mjs +++ b/test/settings-sync.test.mjs @@ -41,7 +41,7 @@ const DIGEST_FIXTURE = { "herd/rules.json": { content: "{}" }, }; -function home({ aliases = null, rules = null, credentials = true, marker = null } = {}) { +function home({ aliases = null, rules = null, credentials = true, marker = null, feeds = null } = {}) { const dir = mkdtempSync(path.join(tmpdir(), "moshcode-sync-")); const moshcode = path.join(dir, ".moshcode"); fs.mkdirSync(moshcode, { recursive: true }); @@ -50,6 +50,7 @@ function home({ aliases = null, rules = null, credentials = true, marker = null JSON.stringify({ token: "mck_super_secret", email: "a@b.c" })); } if (aliases) fs.writeFileSync(path.join(moshcode, "aliases.json"), aliases); + if (feeds) fs.writeFileSync(path.join(moshcode, "feeds.opml"), feeds); if (rules) { fs.mkdirSync(path.join(moshcode, "herd"), { recursive: true }); fs.writeFileSync(path.join(moshcode, "herd", "rules.json"), rules); @@ -260,6 +261,66 @@ test("/save sends the revision it last saw, and reports a conflict rather than w assert.equal(loadMarker(dir).revision, 7, "a refused save must not move the marker"); }); +const OPML = '\n' + + '' + + "\n"; + +test("/save carries the feed list, and does not try to parse it", async () => { + const dir = home({ aliases: "{}", feeds: OPML }); + const fetchImpl = stubFetch([[200, { revision: 1 }]]); + + const code = await saveCommand([], { home: dir, creds: CREDS, fetchImpl, write: lines(), installed: INSTALLED }); + assert.equal(code, 0); + const sent = fetchImpl.calls[0].body.snapshot.files; + assert.equal(sent["feeds.opml"].content, OPML, "OPML travels byte for byte"); +}); + +test("a feed list that is not XML still syncs — nothing here reads it", async () => { + // The contrast with aliases.json is the point: that one is `json: true` and + // a broken one is held back rather than copied to every machine. OPML is + // moved, never parsed, so there is no such thing as a broken one here. + const dir = home({ aliases: "{}", feeds: "this is not xml at all" }); + const fetchImpl = stubFetch([[200, { revision: 1 }]]); + + await saveCommand([], { home: dir, creds: CREDS, fetchImpl, write: lines(), installed: INSTALLED }); + assert.ok(fetchImpl.calls[0].body.snapshot.files["feeds.opml"], "it went anyway"); +}); + +test("a 502 is the app not answering, so it is retried rather than reported", async () => { + const dir = home({ aliases: "{}" }); + const fetchImpl = stubFetch([[502, { message: "Application failed to respond" }], [200, { revision: 3 }]]); + const write = lines(); + + const code = await saveCommand([], { home: dir, creds: CREDS, fetchImpl, write, installed: INSTALLED }); + assert.equal(code, 0, "the second attempt is the answer"); + assert.equal(fetchImpl.calls.length, 2); + assert.equal(loadMarker(dir).revision, 3); + assert.doesNotMatch(write.text(), /502/, "a blip that was ridden out is not worth a line"); +}); + +test("a 409 is an answer and is never retried", async () => { + // The distinction the retry turns on: 5xx means the app did not see the + // request, everything else means it did. Retrying a conflict would only + // ask the same question again and lose the same race. + const dir = home({ aliases: "{}", marker: { revision: 2, digest: "d", at: 1, files: {} } }); + const fetchImpl = stubFetch([[409, { error: "moved on", revision: 5 }]]); + + const code = await saveCommand([], { home: dir, creds: CREDS, fetchImpl, write: lines(), installed: INSTALLED }); + assert.equal(code, 1); + assert.equal(fetchImpl.calls.length, 1, "asked once"); +}); + +test("a 502 that never clears is still reported rather than retried forever", async () => { + const dir = home({ aliases: "{}" }); + const fetchImpl = stubFetch([[502, {}], [502, {}], [502, {}], [502, {}]]); + const write = lines(); + + const code = await saveCommand([], { home: dir, creds: CREDS, fetchImpl, write, installed: INSTALLED }); + assert.equal(code, 1); + assert.equal(fetchImpl.calls.length, 3, "the first try and two retries, then it is news"); + assert.match(write.text(), /could not save/); +}); + test("/save --force drops the precondition", async () => { const dir = home({ aliases: "{}", marker: { revision: 7, digest: "stale", at: 1, files: {} } }); const fetchImpl = stubFetch([[200, { revision: 10 }]]);