fix: retry legacy inline image repair on startup

scripts/legacy-image-repair.test.js

@@ -0,0 +1,35 @@
+import assert from 'node:assert/strict';
+import { mkdtempSync, readFileSync, rmSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import test, { after } from 'node:test';
+
+const temp = mkdtempSync(join(tmpdir(), 'nt-legacy-images-'));
+process.env.DB_PATH = join(temp, 'test.db');
+process.env.UPLOADS_PATH = join(temp, 'uploads');
+process.env.LOG_LEVEL = 'error';
+const { default: db } = await import('../server/db.js');
+const { migrateDataUrlImages } = await import('../server/lib/img-url-migration.js');
+after(() => { db.close(); rmSync(temp, { recursive: true, force: true }); });
+
+test('legacy repair ignores the old flag, retries failures and advances the sync cursor', async () => {
+  const bytes = Buffer.concat([Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+jRZkAAAAASUVORK5CYII=', 'base64'), Buffer.alloc(64)]);
+  const inline = `data:image/png;base64,${bytes.toString('base64')}`;
+  db.prepare("INSERT OR REPLACE INTO app_config (key,value) VALUES ('img_url_data_urls_migrated_v1','done')").run();
+  const id = db.prepare("INSERT INTO meals (name,img_url,is_recipe,updated_at) VALUES ('Legacy photo',?,1,'2000-01-01 00:00:00')").run(inline).lastInsertRowid;
+  const bad = db.prepare("INSERT INTO foods (name,img_url) VALUES ('Broken legacy','data:image/png;base64,bad')").run().lastInsertRowid;
+  const gone = db.prepare("INSERT INTO meals (name,img_url,is_recipe,deleted_at) VALUES ('Deleted photo',?,1,datetime('now'))").run(inline).lastInsertRowid;
+  db.prepare("UPDATE meals SET changed_at='2000-01-01 00:00:00' WHERE id=?").run(id);
+  assert.deepEqual(await migrateDataUrlImages(), { migrated: 1, failed: 1 });
+  const row = db.prepare('SELECT * FROM meals WHERE id=?').get(id);
+  assert.match(row.img_url, /^\/uploads\//);
+  assert.deepEqual(readFileSync(join(temp, row.img_url)), bytes);
+  assert.equal(row.updated_at, '2000-01-01 00:00:00');
+  assert.ok(row.changed_at > '2000-01-01 00:00:00');
+  assert.deepEqual(await migrateDataUrlImages(), { migrated: 0, failed: 1 });
+  assert.equal(db.prepare('SELECT img_url FROM meals WHERE id=?').get(id).img_url, row.img_url);
+  db.prepare('UPDATE foods SET img_url=? WHERE id=?').run(inline, bad);
+  assert.deepEqual(await migrateDataUrlImages(), { migrated: 1, failed: 0 });
+  assert.deepEqual(await migrateDataUrlImages(), { migrated: 0, failed: 0 });
+  assert.equal(db.prepare('SELECT img_url FROM meals WHERE id=?').get(gone).img_url, inline, 'a deleted row is left alone');
+});

server/index.js

@@ -462,9 +462,9 @@ app.listen(PORT, async () => {
     logger.warn(`[scheduler] failed to start: ${e.message}`);
   });
 
-  // #199 one-shot: localize any data-URL img_urls in foods/meals to
+  // Repair legacy data-URL img_urls in foods/meals on every startup to
   // /uploads/ files so the diary hydrator stops amplifying them.
-  // Guarded by app_config flag; idempotent; fire-and-forget so it
+  // Scans inline images only; idempotent; fire-and-forget so it
   // doesn't delay accepting traffic.
   import('./lib/img-url-migration.js').then(({ migrateDataUrlImages }) => migrateDataUrlImages()).catch(e => {
     logger.warn(`[img-url-migration] failed to start: ${e.message}`);

server/lib/img-url-migration.js

@@ -1,119 +1,37 @@
-/**
- * img-url-migration.js — one-shot boot migration for #199.
- *
- * Foods and meals whose img_url column holds a base64 data URL get
- * localized to /uploads/ files on server startup, so the diary's
- * freshenItemImages hydrator (which was stamping data URLs onto every
- * referencing diary item) stops silently amplifying them into 50 MB
- * /api/diary payloads.
- *
- * Idempotent: guarded by an app_config flag so it runs at most once
- * per instance. Async because localizeImage does file IO; can't sit
- * inside db.js's synchronous migration block. Invoked from server
- * bootstrap after `app.listen` (see server/index.js).
- *
- * Design choices worth naming:
- *
- * - **Runs after listen, not during.** Server accepts traffic while
- *   the migration works. Existing installs with hundreds of data-URL
- *   rows don't delay boot. The freshenItemImages filter (added in the
- *   same fix) already keeps the payload clean during the window
- *   between listen and migration completion, so serving traffic is
- *   safe throughout.
- * - **Does NOT bump updated_at.** Same reasoning as the diary shrink
- *   migration in db.js: a mass updated_at bump would show every row
- *   as changed on the next Android sync/pull. Foods/meals dbUpsert
- *   guards pending edits, but skipping the bump avoids the risk
- *   entirely. Native clients pick up the /uploads/ path on the next
- *   natural edit (their PUT sends the local data URL, server
- *   re-localizes fresh via POST/PUT's own localizeImage path).
- * - **Per-row try/catch.** One malformed data URL (truncated,
- *   unknown mime type, disk full mid-write) can't abort the whole
- *   pass. Failures are counted and logged; the row stays as-is and
- *   the read-side filter keeps it out of the diary payload.
+/** Repair legacy embedded food/recipe photos on each startup.
+ * The old one-shot flag missed later recipe edits and imports. Only data URLs
+ * are scanned; ordinary upload paths and external URLs are never re-downloaded.
+ * Deleted rows are skipped: nobody sees their photo, and converting it would
+ * send them to every phone again for nothing.
  */
 import db from '../db.js';
 import { logger } from '../logger.js';
 import { localizeImage } from './image-localizer.js';
 
-const FLAG_KEY = 'img_url_data_urls_migrated_v1';
-
-let _promise = null;
-
+let pending;
 export function migrateDataUrlImages() {
-  if (_promise) return _promise;
-  _promise = _run();
-  return _promise;
+  if (!pending) pending = run().finally(() => { pending = null; });
+  return pending;
 }
 
-async function _run() {
-  try {
-    const done = db.prepare(`SELECT value FROM app_config WHERE key = ?`).get(FLAG_KEY);
-    if (done) return { skipped: true, reason: 'already-migrated' };
-
-    const foods = db.prepare(
-      `SELECT id, img_url FROM foods WHERE deleted_at IS NULL AND img_url LIKE 'data:%'`
-    ).all();
-    const meals = db.prepare(
-      `SELECT id, img_url FROM meals WHERE deleted_at IS NULL AND img_url LIKE 'data:%'`
-    ).all();
-
-    if (foods.length === 0 && meals.length === 0) {
-      db.prepare(`INSERT OR REPLACE INTO app_config (key, value) VALUES (?, ?)`)
-        .run(FLAG_KEY, new Date().toISOString());
-      return { skipped: true, reason: 'no-data-urls', migrated: 0 };
-    }
-
-    const foodUpdate = db.prepare(`UPDATE foods SET img_url = ? WHERE id = ?`);
-    const mealUpdate = db.prepare(`UPDATE meals SET img_url = ? WHERE id = ?`);
-
-    let migrated = 0, failed = 0;
-    for (const r of foods) {
+async function run() {
+  let migrated = 0, failed = 0;
+  for (const table of ['foods', 'meals']) {
+    const rows = db.prepare(`SELECT id, img_url FROM ${table} WHERE deleted_at IS NULL AND img_url LIKE 'data:%'`).all();
+    // changed_at triggers make the corrected URL visible to Android pulls.
+    // Preserve updated_at (the edit time) and never overwrite a concurrent edit.
+    const update = db.prepare(`UPDATE ${table} SET img_url = ? WHERE id = ? AND img_url = ?`);
+    for (const row of rows) {
       try {
-        const local = await localizeImage(r.img_url);
-        if (local && !local.startsWith('data:')) {
-          foodUpdate.run(local, r.id);
-          migrated++;
-        } else {
-          failed++;
-        }
-      } catch (e) {
+        const image = await localizeImage(row.img_url);
+        if (!image || /^data:/i.test(image)) throw new Error('Image conversion failed');
+        migrated += update.run(image, row.id, row.img_url).changes;
+      } catch {
         failed++;
-        logger.warn(`[img-url-migration] food id=${r.id} failed: ${e?.message || e}`);
+        logger.warn(`[img-url-migration] ${table} id=${row.id}: could not store image; will retry next startup`);
       }
     }
-    for (const r of meals) {
-      try {
-        const local = await localizeImage(r.img_url);
-        if (local && !local.startsWith('data:')) {
-          mealUpdate.run(local, r.id);
-          migrated++;
-        } else {
-          failed++;
-        }
-      } catch (e) {
-        failed++;
-        logger.warn(`[img-url-migration] meal id=${r.id} failed: ${e?.message || e}`);
-      }
-    }
-
-    // Only stamp the flag if EVERY row succeeded. A partial pass leaves
-    // the flag off so the next boot retries the failures. That is safe
-    // because localizeImage is idempotent (data URLs hash to the same
-    // filename), and the already-migrated rows short-circuit via
-    // freshenItemImages's data: filter regardless.
-    if (failed === 0) {
-      db.prepare(`INSERT OR REPLACE INTO app_config (key, value) VALUES (?, ?)`)
-        .run(FLAG_KEY, new Date().toISOString());
-    }
-
-    logger.info(
-      `[img-url-migration] localized ${migrated} data-URL image(s) to /uploads/` +
-      (failed ? `, ${failed} failed (will retry on next boot)` : '')
-    );
-    return { migrated, failed };
-  } catch (e) {
-    logger.warn(`[img-url-migration] pass failed: ${e?.message || e}`);
-    return { failed: true, error: e?.message || String(e) };
   }
+  if (migrated || failed) logger.info(`[img-url-migration] localized ${migrated} image(s), ${failed} failed`);
+  return { migrated, failed };
 }