fix(sync): retry /save when the app does not answer, and carry feeds.opml

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 } });
 

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 = '<?xml version="1.0"?>\n<opml version="2.0"><body>' +
+  '<outline type="rss" text="leaddev.com" xmlUrl="https://leaddev.com/feed"/>' +
+  "</body></opml>\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 }]]);