From b563672e925acc94f93183f8aaa26bdc845c4fc7 Mon Sep 17 00:00:00 2001 From: Barry Schneider Date: Sat, 25 Jul 2026 05:23:29 -0400 Subject: [PATCH 1/2] fix(photo-exif): caption pass must preserve face enrichment (#276) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both enrichment workers now build their ingest payload through one shared lib/photo-payload.js, so the caption pass carries faces_detected/pictured/"Pictured: …" through instead of wiping them via the wholesale extra replace. An absent face count stays absent rather than becoming 0, and the two passes may now run in either order. --- connectors/photo-exif/README.md | 18 ++- connectors/photo-exif/caption-worker.js | 43 ++++-- connectors/photo-exif/face-worker.js | 43 ++---- connectors/photo-exif/lib/photo-payload.js | 69 ++++++++++ connectors/photo-exif/test.mjs | 153 +++++++++++++++++++++ 5 files changed, 280 insertions(+), 46 deletions(-) create mode 100644 connectors/photo-exif/lib/photo-payload.js diff --git a/connectors/photo-exif/README.md b/connectors/photo-exif/README.md index 8196cbd..6ea0e61 100644 --- a/connectors/photo-exif/README.md +++ b/connectors/photo-exif/README.md @@ -117,12 +117,21 @@ powershell -File prep-takeout.ps1 -NoScan # unzip + recycle only (skip the ing **Recycle Bin, never permanent delete** — recoverable, and matching this project's append-only ethos and the box's delete-blocked posture (the `rm`/`Remove-Item` deny); it uses `Microsoft.VisualBasic.FileIO.FileSystem.DeleteFile(SendToRecycleBin)`, not `Remove-Item`. **Caveat:** the Recycle Bin still occupies disk until emptied — to actually reclaim the space after a verified run, empty it manually (the script can't, since permanent delete is blocked). Windows-only. -### Wave order matters +### Wave order — either order is safe (#276) -Run **scan → caption → face** for each photo. The ingest contract requires `text_repr` on every upsert and replaces it (and `extra`) wholesale — there is no deep-merge (doc 04 §3). The face worker therefore reconstructs `text_repr` as *base + caption + "Pictured: …"* by reading the caption cache, so it preserves a caption rather than dropping it. Two ordering caveats follow from the wholesale replace: +Run `scan.js` first (it creates the artifacts). After that, **the caption and face passes may run in either order, or be interleaved**, without losing each other's work. -- If the face worker runs on a photo whose caption isn't in its local cache yet (e.g. the caption was written on a different machine), it will write base-only text and overwrite the server-side caption. Keep the caption cache co-located with the face worker. -- A caption run that lands *after* a face `label` on the same photo drops the "Pictured: …" sentence (the entity link itself is unaffected — that's the primary recall path). In practice each worker processes a photo once (file-state gated), so this only bites if a photo is labeled before it is ever captioned. +The hazard this protects against: the ingest contract requires `text_repr` on every upsert and replaces it (and `extra`) **wholesale** — there is no deep-merge (doc 04 §3). So whichever enrichment pass runs second overwrites the first one's contribution unless both send the *union* of what's known. + +Both workers therefore build their payload through the one shared builder in **`lib/photo-payload.js`**: + +- The **face worker** reads the caption cache and rebuilds *base + caption + "Pictured: …"*. +- The **caption worker** reads the face state + clusters files and carries `faces_detected` / `pictured` / the "Pictured: …" sentence through. + +Two properties worth knowing: + +- **Absent is not zero.** If the face pass hasn't touched a photo, the caption worker omits `faces_detected` and `pictured` entirely rather than sending `0`/`[]` — sending `0` would assert "detection ran and found nothing," which would be false and, thanks to the wholesale replace, permanent. +- **Keep both state directories co-located with the workers.** They read each other's state from `~/.life-context/` (overridable via `PHOTO_EXIF_CAPTION_STATE_PATH`, `PHOTO_EXIF_FACE_STATE_PATH`, `PHOTO_EXIF_FACE_CLUSTERS_PATH`). A worker that can't see the other's state falls back to writing only what it knows — correct, but it will drop the other's contribution, which is the pre-#276 behavior. Running the two passes on *different machines* against the same server is therefore still unsupported. ### Nightly-window scheduling (config, not code) @@ -168,6 +177,7 @@ On Windows, use Task Scheduler with a "Daily, 1:00 AM" trigger running `node cap - `lib/shared.js` — env loading, media walk (`walkImageFiles`/`walkMediaFiles`), the content-hash keying resolver (`keyForMedia`), `mediaType`, ingest client, contact-photos fetch (`suggest-labels`) - `lib/describe.js` — shared EXIF + Takeout-sidecar description logic (`describePhoto`/`readSidecar`/`sidecarPathFor`), used by every script so they can never drift - `lib/caption-cache.js` — caption state (relPath→text map) + `currentTextRepr`, shared by caption + face workers +- `lib/photo-payload.js` — the single ingest-payload builder both enrichment workers use, so neither clobbers the other's `text_repr`/`extra` (#276); also reads the face state into a `relPath → {faces, pictured}` lookup for the caption worker - `lib/face-cluster.js` — pure, IO-free descriptor clustering (euclidean, nearest-centroid, (de)serialization) - `lib/face-align.js` — pure, IO-free SCRFD decode (distance2bbox/kps, anchor centers, NMS) + Umeyama similarity/bilinear warp used to align each 112x112 ArcFace input crop (#268) - `lib/face-detect.js` — lazy ML-model detector + the test fixture detector diff --git a/connectors/photo-exif/caption-worker.js b/connectors/photo-exif/caption-worker.js index f129799..7b71997 100644 --- a/connectors/photo-exif/caption-worker.js +++ b/connectors/photo-exif/caption-worker.js @@ -14,8 +14,9 @@ import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { loadDotEnvIfPresent, walkImageFiles, keyForMedia, isTakeoutRoot, contentHashOfFile, ingestClient } from './lib/shared.js'; -import { describePhoto, buildTextRepr } from './lib/describe.js'; +import { describePhoto } from './lib/describe.js'; import { readCaptionCache, writeCaptionCache } from './lib/caption-cache.js'; +import { buildPhotoPayload, readFaceEnrichment } from './lib/photo-payload.js'; loadDotEnvIfPresent(path.dirname(fileURLToPath(import.meta.url))); @@ -30,8 +31,15 @@ const VLM_PROMPT = process.env.VLM_PROMPT // "not set" (NaN) from "set to zero" (a real, useful value for tests and manual runs). const rawThrottle = Number(process.env.VLM_THROTTLE_MS); const VLM_THROTTLE_MS = Number.isFinite(rawThrottle) ? rawThrottle : 2000; -const STATE_PATH = process.env.PHOTO_EXIF_CAPTION_STATE_PATH - || path.join(os.homedir(), '.life-context', 'photo-exif-captions.json'); +const HOME_STATE = (name) => path.join(os.homedir(), '.life-context', name); +const STATE_PATH = process.env.PHOTO_EXIF_CAPTION_STATE_PATH || HOME_STATE('photo-exif-captions.json'); +// The face worker's state, read (never written) so a caption upsert carries face enrichment +// through instead of wiping it — `extra` is replaced wholesale by ingest (#276). Same env vars and +// defaults as face-worker.js; absent files just mean the face pass hasn't run yet. +const FACE_STATE_PATH = process.env.PHOTO_EXIF_FACE_STATE_PATH || HOME_STATE('photo-exif-faces.json'); +const CLUSTERS_PATH = process.env.PHOTO_EXIF_FACE_CLUSTERS_PATH || HOME_STATE('photo-exif-face-clusters.json'); +const confRaw = Number(process.env.FACE_HINT_CONFIDENCE); +const FACE_HINT_CONFIDENCE = Number.isFinite(confRaw) ? confRaw : 0.6; async function caption(base64Image) { const res = await fetch(`${VLM_BASE_URL}/api/generate`, { @@ -72,6 +80,8 @@ async function main() { // "Pictured: ..." (lib/caption-cache.js). A present key means "already captioned" — same // skip semantics as the old Set, but the text is retained now. const captionCache = readCaptionCache(STATE_PATH); + // Read once, not per photo — the face pass is finished (or absent) by the time this runs. + const faceEnrichment = readFaceEnrichment(FACE_STATE_PATH, CLUSTERS_PATH); let done = 0; let vlmDown = false; @@ -107,22 +117,29 @@ async function main() { break; } - const enrichedText = `${buildTextRepr(dateStr, path.basename(absPath))} ${captionText}`; // Same content-hash key scan.js computed (keyForMedia) so this enriches the SAME artifact — // a Takeout-export photo keys under source='google-photos', everything else under 'photo-exif'. const { source, source_id } = keyForMedia(contentHash, isTakeout); + // Carry through whatever the face pass already found for this photo. Absent (face pass never + // ran) leaves faces_detected/pictured off the payload entirely rather than sending 0/[] — see + // lib/photo-payload.js. This is what makes caption/face order-independent (#276). + const face = faceEnrichment.get(relPath); try { - // Present fields only: text_repr + extra. Per doc 04 §3 upsert merge semantics, everything - // scan.js already stored (occurred_at, GPS, raw_path, content_hash) — plus whatever core - // resolved into place_label from that GPS — is left untouched, since neither is present - // in this payload. This is exactly the "enrichment wave" the contract's upsert exists for. - await postIngest({ + // Present fields only: text_repr + extra (+ hints when someone is pictured). Per doc 04 §3 + // upsert merge semantics, everything scan.js already stored (occurred_at, GPS, raw_path, + // content_hash) — plus whatever core resolved into place_label from that GPS — is left + // untouched, since none of it is present in this payload. This is exactly the "enrichment + // wave" the contract's upsert exists for. + await postIngest(buildPhotoPayload({ source, source_id, - type: 'photo', - text_repr: enrichedText, - extra: { captioned: true }, - }); + dateStr, + filename: path.basename(absPath), + caption: captionText, + faces: face?.faces, + pictured: face?.pictured, + hintConfidence: FACE_HINT_CONFIDENCE, + })); captionCache[relPath] = captionText; writeCaptionCache(STATE_PATH, captionCache); // after every success, not batched — kill-safe done++; diff --git a/connectors/photo-exif/face-worker.js b/connectors/photo-exif/face-worker.js index d70d37b..932f9d1 100644 --- a/connectors/photo-exif/face-worker.js +++ b/connectors/photo-exif/face-worker.js @@ -23,8 +23,9 @@ import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { loadDotEnvIfPresent, walkImageFiles, keyForMedia, isTakeoutRoot, contentHashOfFile, ingestClient, fetchContactPhotos } from './lib/shared.js'; import { describePhoto, readSidecar } from './lib/describe.js'; -import { readCaptionCache, currentTextRepr } from './lib/caption-cache.js'; +import { readCaptionCache } from './lib/caption-cache.js'; import { assignCluster, euclideanDistance, parseClustersFile, serializeClustersFile } from './lib/face-cluster.js'; +import { buildPhotoPayload, picturedNames } from './lib/photo-payload.js'; import { resolveDetector } from './lib/face-detect.js'; const CONNECTOR_DIR = path.dirname(fileURLToPath(import.meta.url)); @@ -97,38 +98,22 @@ function saveClusters(state) { writeFileSync(CLUSTERS_PATH, serializeClustersFile(state.version, state.clusters)); } -// The pictured names on a photo = the labels of the (distinct) clusters its faces fall into. -function picturedNames(clusterIds, clustersById) { - const names = new Set(); - for (const id of clusterIds) { - const label = clustersById.get(id)?.label; - if (label) names.add(label); - } - return [...names].sort(); -} - // Build the enrichment payload for one photo from its stored face entry + current cluster labels. -// text_repr is REQUIRED by the contract on every ingest (IngestPayloadSchema.text_repr is not -// optional), so we always send it — reconstructed as base + caption (from the caption cache) so a -// caption is preserved rather than clobbered — and append "Pictured: …" only when a cluster the -// photo belongs to has been named. Hints are added only for labeled clusters. +// The payload shape itself lives in lib/photo-payload.js so the caption worker emits the identical +// union of fields and the two passes can run in either order without clobbering each other (#276). +// entry.{source,source_id} is the content-hash key scan() computed and persisted (keyForMedia), +// so a labeled photo enriches the SAME artifact scan.js/caption-worker created. function buildPayload(relPath, entry, clustersById, captionCache) { - const pictured = picturedNames(entry.clusters, clustersById); - const caption = captionCache[relPath] ?? null; - const baseText = currentTextRepr(entry.dateStr, path.basename(relPath), caption); - // entry.{source,source_id} is the content-hash key scan() computed and persisted (keyForMedia), - // so a labeled photo enriches the SAME artifact scan.js/caption-worker created. - const payload = { + return buildPhotoPayload({ source: entry.source, source_id: entry.source_id, - type: 'photo', - text_repr: pictured.length ? `${baseText} Pictured: ${pictured.join(', ')}.` : baseText, - extra: { faces_detected: entry.faces, pictured, captioned: caption != null }, - }; - if (pictured.length) { - payload.entity_hints = pictured.map((alias) => ({ alias, alias_type: 'name', role: 'pictured', confidence: FACE_HINT_CONFIDENCE })); - } - return payload; + dateStr: entry.dateStr, + filename: path.basename(relPath), + caption: captionCache[relPath] ?? null, + faces: entry.faces, + pictured: picturedNames(entry.clusters, clustersById), + hintConfidence: FACE_HINT_CONFIDENCE, + }); } // Everything in the payload that can change between runs — used to skip an upsert that would be diff --git a/connectors/photo-exif/lib/photo-payload.js b/connectors/photo-exif/lib/photo-payload.js new file mode 100644 index 0000000..defd366 --- /dev/null +++ b/connectors/photo-exif/lib/photo-payload.js @@ -0,0 +1,69 @@ +// The one place a photo enrichment payload is built. Both enrichment workers (caption + face) +// upsert the SAME (source, source_id) artifact, and the ingest contract replaces `text_repr` and +// `extra` wholesale (doc 04 §3 — no deep-merge). So whichever worker runs second overwrites the +// other's contribution unless BOTH send the union of what's known. Keeping that union in one +// function is what makes the two passes order-independent — the same reason `lib/describe.js` is +// shared: two scripts building the same thing separately is how they drift (#276). +import { parseClustersFile } from './face-cluster.js'; +import { currentTextRepr } from './caption-cache.js'; +import { readFileSync, existsSync } from 'node:fs'; + +// The pictured names on a photo = the labels of the (distinct) clusters its faces fall into. +// Unlabeled clusters contribute nothing — an anonymous pile is not a name (see face-worker.js). +export function picturedNames(clusterIds, clustersById) { + const names = new Set(); + for (const id of clusterIds) { + const label = clustersById.get(id)?.label; + if (label) names.add(label); + } + return [...names].sort(); +} + +// Build the ingest payload for one photo from everything currently known about it. `faces` and +// `pictured` are OPTIONAL and meaningfully so: a caller that has no face data must pass them as +// undefined, NOT as 0/[] — see the extra-field note below. +export function buildPhotoPayload({ source, source_id, dateStr, filename, caption, faces, pictured, hintConfidence }) { + const names = pictured ?? []; + const baseText = currentTextRepr(dateStr, filename, caption); + const payload = { + source, + source_id, + type: 'photo', + text_repr: names.length ? `${baseText} Pictured: ${names.join(', ')}.` : baseText, + // `faces_detected: 0` asserts "we ran detection and found no faces" — a claim a caller that + // never ran detection must not make, and one the wholesale `extra` replace would make + // permanent. So an absent count stays absent rather than defaulting to 0 (#276). + extra: { captioned: caption != null }, + }; + if (faces !== undefined && faces !== null) payload.extra.faces_detected = faces; + if (pictured !== undefined) payload.extra.pictured = names; + if (names.length) { + payload.entity_hints = names.map((alias) => ({ alias, alias_type: 'name', role: 'pictured', confidence: hintConfidence })); + } + return payload; +} + +// Read the face worker's two state files into a relPath -> { faces, pictured } lookup, so the +// caption worker can carry face enrichment through its own upsert without re-running detection. +// Both files absent (face pass never ran) is the normal, non-exceptional case — returns an empty +// map, and every caller then behaves exactly as it did before this existed. +export function readFaceEnrichment(faceStatePath, clustersPath) { + const lookup = new Map(); + if (!faceStatePath || !existsSync(faceStatePath)) return lookup; + let faceState; + try { + faceState = JSON.parse(readFileSync(faceStatePath, 'utf8')); + } catch (err) { + console.error(`photo-exif: unreadable face state at ${faceStatePath}; captioning without face data`, err); + return lookup; + } + const clusters = clustersPath && existsSync(clustersPath) + ? parseClustersFile(readFileSync(clustersPath, 'utf8')).clusters + : []; + const clustersById = new Map(clusters.map((c) => [c.id, c])); + for (const [relPath, entry] of Object.entries(faceState)) { + if (!entry || typeof entry.faces !== 'number') continue; + lookup.set(relPath, { faces: entry.faces, pictured: picturedNames(entry.clusters ?? [], clustersById) }); + } + return lookup; +} diff --git a/connectors/photo-exif/test.mjs b/connectors/photo-exif/test.mjs index 5debcf0..6d46935 100644 --- a/connectors/photo-exif/test.mjs +++ b/connectors/photo-exif/test.mjs @@ -585,6 +585,159 @@ test('caption-worker.js: VLM unreachable stops the run without marking anything assert.throws(() => readFileSync(statePath, 'utf8')); // nothing was captioned }); +// --- #276: caption pass must not clobber face enrichment --------------------------------------- + +// Minimal VLM stand-in for the #276 tests — always answers with the same caption. +async function startVlmStub(responseText) { + const server = http.createServer((req, res) => { + let body = ''; + req.on('data', (c) => (body += c)); + req.on('end', () => { + res.setHeader('content-type', 'application/json'); + res.end(JSON.stringify({ response: responseText })); + }); + }); + const port = await new Promise((resolve) => server.listen(0, '127.0.0.1', () => resolve(server.address().port))); + return { server, port }; +} + +test('caption-worker.js (#276): carries face enrichment through instead of wiping it', async () => { + const tmp = mkdtempSync(path.join(tmpdir(), 'photo-exif-276-preserve-')); + writeFileSync(path.join(tmp, 'photo.jpg'), jpegWithExif({ dateTimeOriginal: '2019:03:04 14:30:00' })); + + // Face pass already ran over this photo and cluster 1 was named. + const faceState = path.join(tmp, 'faces.json'); + writeFileSync(faceState, JSON.stringify({ 'photo.jpg': { faces: 2, clusters: [1, 2], dateStr: '2019-03-04' } })); + const clustersState = path.join(tmp, 'clusters.json'); + writeFileSync(clustersState, serializeClustersFile(1, [ + { id: 1, centroid: [0, 0, 0], count: 3, label: 'Sarah Jones', sample: 'photo.jpg' }, + { id: 2, centroid: [9, 9, 9], count: 1, label: null, sample: 'photo.jpg' }, // unlabeled -> contributes no name + ])); + + const ingestRequests = []; + const { server: ingestServer, port: ingestPort } = await startMockServer((req, body, res) => { + ingestRequests.push(body); + res.end(JSON.stringify({ id: 1, created: false })); + }); + const { server: vlmServer, port: vlmPort } = await startVlmStub('two people cooking pasta in a kitchen'); + + const res = await run('caption-worker.js', { + LIFECONTEXT_URL: `http://127.0.0.1:${ingestPort}`, + LIFECONTEXT_API_KEY: 'test-key', + PHOTO_ROOT: tmp, + PHOTO_EXIF_CAPTION_STATE_PATH: path.join(tmp, 'captions.json'), + PHOTO_EXIF_FACE_STATE_PATH: faceState, + PHOTO_EXIF_FACE_CLUSTERS_PATH: clustersState, + VLM_BASE_URL: `http://127.0.0.1:${vlmPort}`, + VLM_THROTTLE_MS: '0', + FACE_HINT_CONFIDENCE: '0.6', + }); + ingestServer.closeAllConnections(); + ingestServer.close(); + vlmServer.close(); + + assert.equal(res.status, 0, res.stderr); + assert.equal(ingestRequests.length, 1); + const p = ingestRequests[0]; + // The caption landed AND the face pass's work survived — this is the whole point of #276. + assert.equal(p.text_repr, 'Photo taken 2019-03-04 two people cooking pasta in a kitchen Pictured: Sarah Jones.'); + assert.equal(p.extra.captioned, true); + assert.equal(p.extra.faces_detected, 2, 'face count carried through, not wiped'); + assert.deepEqual(p.extra.pictured, ['Sarah Jones'], 'only the NAMED cluster contributes a name'); + assert.deepEqual(p.entity_hints, [{ alias: 'Sarah Jones', alias_type: 'name', role: 'pictured', confidence: 0.6 }]); + // Still upsert-only-what-changed: scan.js's originals are not resent. + assert.equal(p.occurred_at, undefined); + assert.equal(p.latitude, undefined); +}); + +test('caption-worker.js (#276): with no face state, omits faces_detected entirely — never sends 0', async () => { + const tmp = mkdtempSync(path.join(tmpdir(), 'photo-exif-276-noface-')); + writeFileSync(path.join(tmp, 'photo.jpg'), jpegWithExif({ dateTimeOriginal: '2019:03:04 14:30:00' })); + + const ingestRequests = []; + const { server: ingestServer, port: ingestPort } = await startMockServer((req, body, res) => { + ingestRequests.push(body); + res.end(JSON.stringify({ id: 1, created: false })); + }); + const { server: vlmServer, port: vlmPort } = await startVlmStub('a dog on a beach'); + + const res = await run('caption-worker.js', { + LIFECONTEXT_URL: `http://127.0.0.1:${ingestPort}`, + LIFECONTEXT_API_KEY: 'test-key', + PHOTO_ROOT: tmp, + PHOTO_EXIF_CAPTION_STATE_PATH: path.join(tmp, 'captions.json'), + PHOTO_EXIF_FACE_STATE_PATH: path.join(tmp, 'does-not-exist-faces.json'), + PHOTO_EXIF_FACE_CLUSTERS_PATH: path.join(tmp, 'does-not-exist-clusters.json'), + VLM_BASE_URL: `http://127.0.0.1:${vlmPort}`, + VLM_THROTTLE_MS: '0', + }); + ingestServer.closeAllConnections(); + ingestServer.close(); + vlmServer.close(); + + assert.equal(res.status, 0, res.stderr); + const p = ingestRequests[0]; + assert.equal(p.text_repr, 'Photo taken 2019-03-04 a dog on a beach'); // no "Pictured:" sentence + assert.equal(p.extra.captioned, true); + // `0` would assert "we ran detection and found no faces" — a claim this worker cannot make. + assert.ok(!('faces_detected' in p.extra), 'faces_detected absent, not 0'); + assert.ok(!('pictured' in p.extra), 'pictured absent, not []'); + assert.equal(p.entity_hints, undefined); +}); + +test('caption-worker.js (#276): face -> caption -> face round trip converges, nothing lost mid-way', async () => { + const tmp = mkdtempSync(path.join(tmpdir(), 'photo-exif-276-roundtrip-')); + writeFileSync(path.join(tmp, 'photo.jpg'), jpegWithExif({ dateTimeOriginal: '2019:03:04 14:30:00' })); + const fixturePath = path.join(tmp, 'faces-fixture.json'); + writeFileSync(fixturePath, JSON.stringify({ 'photo.jpg': [[0, 0, 0]] })); + + const faceState = path.join(tmp, 'faces.json'); + const clustersState = path.join(tmp, 'clusters.json'); + const captionState = path.join(tmp, 'captions.json'); + const shared = { + LIFECONTEXT_API_KEY: 'test-key', + PHOTO_ROOT: tmp, + PHOTO_EXIF_FACE_FIXTURE: fixturePath, + PHOTO_EXIF_FACE_STATE_PATH: faceState, + PHOTO_EXIF_FACE_CLUSTERS_PATH: clustersState, + PHOTO_EXIF_CAPTION_STATE_PATH: captionState, + FACE_THROTTLE_MS: '0', + FACE_HINT_CONFIDENCE: '0.6', + }; + + const posted = []; + const { server, port } = await startMockServer((req, body, res) => { + posted.push(body); + res.end(JSON.stringify({ id: 1, created: false })); + }); + const { server: vlmServer, port: vlmPort } = await startVlmStub('two people cooking'); + const url = `http://127.0.0.1:${port}`; + + // 1. face scan (cluster is anonymous), 2. name it, 3. caption, 4. face scan again + assert.equal((await run('face-worker.js', { ...shared, LIFECONTEXT_URL: url })).status, 0); + const clusterId = parseClustersFile(readFileSync(clustersState, 'utf8')).clusters[0].id; + assert.equal((await run('face-worker.js', { ...shared, LIFECONTEXT_URL: url }, ['label', String(clusterId), 'Sarah Jones'])).status, 0); + const afterLabel = posted.at(-1); + assert.equal((await run('caption-worker.js', { ...shared, LIFECONTEXT_URL: url, VLM_BASE_URL: `http://127.0.0.1:${vlmPort}`, VLM_THROTTLE_MS: '0' })).status, 0); + const afterCaption = posted.at(-1); + assert.equal((await run('face-worker.js', { ...shared, LIFECONTEXT_URL: url })).status, 0); + const afterRefresh = posted.at(-1); + + server.closeAllConnections(); + server.close(); + vlmServer.close(); + + // The label wave established the face data... + assert.equal(afterLabel.extra.faces_detected, 1); + assert.deepEqual(afterLabel.extra.pictured, ['Sarah Jones']); + // ...the caption wave ADDED the caption without dropping any of it (pre-#276 this wiped both)... + assert.equal(afterCaption.text_repr, 'Photo taken 2019-03-04 two people cooking Pictured: Sarah Jones.'); + assert.equal(afterCaption.extra.faces_detected, 1); + assert.deepEqual(afterCaption.extra.pictured, ['Sarah Jones']); + // ...and a following face pass is now a no-op in content: both workers agree byte-for-byte. + assert.deepEqual(afterRefresh, afterCaption, 'face and caption workers converge on an identical payload'); +}); + // --- #53: face worker ------------------------------------------------------------------------ test('face-cluster: euclidean + nearest-centroid grouping and new-cluster creation', () => { From 32630047c24362d700e288764dbece4904f526cd Mon Sep 17 00:00:00 2001 From: Barry Schneider Date: Sat, 25 Jul 2026 05:30:08 -0400 Subject: [PATCH 2/2] fix(photo-exif): guard the clusters read, drop the "interleaved" claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review on #277: the clusters readFileSync was unguarded while the face-state read beside it was wrapped, so a mid-write/permission error would crash the caption run — it now degrades to no face data (omitting both fields rather than asserting an empty pictured list). Also corrects the README, which overclaimed concurrent runs as safe when each worker snapshots the other's state once at startup. --- connectors/photo-exif/README.md | 6 ++-- connectors/photo-exif/lib/photo-payload.js | 18 ++++++++-- connectors/photo-exif/test.mjs | 41 ++++++++++++++++++++++ 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/connectors/photo-exif/README.md b/connectors/photo-exif/README.md index 6ea0e61..afb2b94 100644 --- a/connectors/photo-exif/README.md +++ b/connectors/photo-exif/README.md @@ -117,9 +117,11 @@ powershell -File prep-takeout.ps1 -NoScan # unzip + recycle only (skip the ing **Recycle Bin, never permanent delete** — recoverable, and matching this project's append-only ethos and the box's delete-blocked posture (the `rm`/`Remove-Item` deny); it uses `Microsoft.VisualBasic.FileIO.FileSystem.DeleteFile(SendToRecycleBin)`, not `Remove-Item`. **Caveat:** the Recycle Bin still occupies disk until emptied — to actually reclaim the space after a verified run, empty it manually (the script can't, since permanent delete is blocked). Windows-only. -### Wave order — either order is safe (#276) +### Wave order — either order is safe, one at a time (#276) -Run `scan.js` first (it creates the artifacts). After that, **the caption and face passes may run in either order, or be interleaved**, without losing each other's work. +Run `scan.js` first (it creates the artifacts). After that, **the caption and face passes may run in either order** — whichever runs second preserves the first one's work. + +**Run them one at a time, not concurrently.** Each worker snapshots the other's state once at startup, so a face `label` applied *while* a caption run is in flight won't be seen by that run, and the caption upsert will overwrite it with the older picture. Sequential runs in either order are safe; overlapping runs are not. The hazard this protects against: the ingest contract requires `text_repr` on every upsert and replaces it (and `extra`) **wholesale** — there is no deep-merge (doc 04 §3). So whichever enrichment pass runs second overwrites the first one's contribution unless both send the *union* of what's known. diff --git a/connectors/photo-exif/lib/photo-payload.js b/connectors/photo-exif/lib/photo-payload.js index defd366..496d7a1 100644 --- a/connectors/photo-exif/lib/photo-payload.js +++ b/connectors/photo-exif/lib/photo-payload.js @@ -57,9 +57,21 @@ export function readFaceEnrichment(faceStatePath, clustersPath) { console.error(`photo-exif: unreadable face state at ${faceStatePath}; captioning without face data`, err); return lookup; } - const clusters = clustersPath && existsSync(clustersPath) - ? parseClustersFile(readFileSync(clustersPath, 'utf8')).clusters - : []; + // An ABSENT clusters file is legitimate (the face pass ran but nothing has been named yet) and + // yields `pictured: []`, matching what face-worker.js itself sends in that state. An + // UNREADABLE one is an anomaly (mid-write on Windows, a directory, bad permissions) — bail to + // the empty lookup rather than claim `pictured: []`, which would assert "nobody named is in + // this photo" and, via the wholesale `extra` replace, make that false claim permanent. Same + // absent-≠-empty reasoning as faces_detected above. + let clusters = []; + if (clustersPath && existsSync(clustersPath)) { + try { + clusters = parseClustersFile(readFileSync(clustersPath, 'utf8')).clusters; + } catch (err) { + console.error(`photo-exif: unreadable cluster state at ${clustersPath}; captioning without face data`, err); + return new Map(); + } + } const clustersById = new Map(clusters.map((c) => [c.id, c])); for (const [relPath, entry] of Object.entries(faceState)) { if (!entry || typeof entry.faces !== 'number') continue; diff --git a/connectors/photo-exif/test.mjs b/connectors/photo-exif/test.mjs index 6d46935..998d013 100644 --- a/connectors/photo-exif/test.mjs +++ b/connectors/photo-exif/test.mjs @@ -685,6 +685,47 @@ test('caption-worker.js (#276): with no face state, omits faces_detected entirel assert.equal(p.entity_hints, undefined); }); +test('caption-worker.js (#276): an unreadable clusters file degrades to no face data, never a crash or a false empty', async () => { + const tmp = mkdtempSync(path.join(tmpdir(), 'photo-exif-276-badclusters-')); + writeFileSync(path.join(tmp, 'photo.jpg'), jpegWithExif({ dateTimeOriginal: '2019:03:04 14:30:00' })); + const faceState = path.join(tmp, 'faces.json'); + writeFileSync(faceState, JSON.stringify({ 'photo.jpg': { faces: 2, clusters: [1] } })); + // A directory where the clusters file should be: exists, but readFileSync throws (EISDIR). + // Portable stand-in for the real hazards — mid-write on Windows, bad permissions. + const clustersState = path.join(tmp, 'clusters.json'); + mkdirSync(clustersState); + + const ingestRequests = []; + const { server: ingestServer, port: ingestPort } = await startMockServer((req, body, res) => { + ingestRequests.push(body); + res.end(JSON.stringify({ id: 1, created: false })); + }); + const { server: vlmServer, port: vlmPort } = await startVlmStub('a dog on a beach'); + + const res = await run('caption-worker.js', { + LIFECONTEXT_URL: `http://127.0.0.1:${ingestPort}`, + LIFECONTEXT_API_KEY: 'test-key', + PHOTO_ROOT: tmp, + PHOTO_EXIF_CAPTION_STATE_PATH: path.join(tmp, 'captions.json'), + PHOTO_EXIF_FACE_STATE_PATH: faceState, + PHOTO_EXIF_FACE_CLUSTERS_PATH: clustersState, + VLM_BASE_URL: `http://127.0.0.1:${vlmPort}`, + VLM_THROTTLE_MS: '0', + }); + ingestServer.closeAllConnections(); + ingestServer.close(); + vlmServer.close(); + + assert.equal(res.status, 0, res.stderr); // degrades, does not crash the run + assert.match(res.stderr, /unreadable cluster state/, 'the anomaly is logged loudly, not swallowed'); + const p = ingestRequests[0]; + assert.equal(p.extra.captioned, true); // the caption still lands + // Names are unknowable here, so claim nothing rather than assert "nobody is pictured" — the + // wholesale `extra` replace would make that false claim permanent. + assert.ok(!('pictured' in p.extra), 'pictured omitted, not []'); + assert.ok(!('faces_detected' in p.extra), 'faces_detected omitted too — partial face data is not sent'); +}); + test('caption-worker.js (#276): face -> caption -> face round trip converges, nothing lost mid-way', async () => { const tmp = mkdtempSync(path.join(tmpdir(), 'photo-exif-276-roundtrip-')); writeFileSync(path.join(tmp, 'photo.jpg'), jpegWithExif({ dateTimeOriginal: '2019:03:04 14:30:00' }));