Skip to content

refactor(server): route personal face placement through one face-assignment module - #1156

Merged
Deeds67 merged 3 commits into
open-noodle:mainfrom
luminoso:refactor/face-assignment-module
Oct 6, 2026
Merged

Deeds67 merged 3 commits into
open-noodle:mainfrom
luminoso:refactor/face-assignment-module

Conversation

@luminoso

@luminoso luminoso commented Oct 4, 2026

Copy link
Copy Markdown

Summary

"This face now belongs to this person" takes four writes: move the face, re-point its identity link, drain pending suggestions, and clear rejected or ignored verdicts for that person. Until now that sequence was written out by hand at five call sites across three services. Two of them, the face editor's reassignFaces and reassignFacesById, ran it without a transaction. If a write failed part-way, the face sat on the new person while still carrying the old identity, and FaceIdentityBackfill could then move it back to the old person, silently undoing the user's manual correction (D14). This PR puts the sequence in one fork-only module, so every personal placement is atomic, and gives the suggestion engine and the cleanup engine one shared settlement check.

Changes

  • server/src/services/face-assignment.service.ts (new, fork-only, replaces face-verdict.service.ts). assignFaces({ personGroupId, faceIds, strength, from? }, trx?) handles the move, identity re-link, pending drain and negative-verdict clear. It joins the caller's transaction or opens its own, and returns the ids it assigned. strength is manual (the durable lock) or owner-person (ordinary). from covers three cases: omitted moves the faces unconditionally; another person moves only faces still on it (the cleanup engine's write-time guard); the target itself re-affirms faces where they already sit. getSettledFaceIds(faceIds, target) is the settlement read, built on targetTokens and isSettledForOwner. buildVerdictMaps moved in unchanged.
  • person.service.ts (upstream file, net 4 lines shorter): reassignFaces, reassignFacesById and createFace each make one module call. Feature-photo refresh stays at the call sites, unchanged from upstream.
  • face-suggestion.service.ts: confirm calls the module inside its claim transaction. Both suggestion scans use getSettledFaceIds instead of building person: / space-person: / identity: token strings by hand.
  • face-repair.service.ts: executeRepair and the cleanup lock bucket each make one module call.
  • person.repository.ts (upstream file): reassignFaces takes an optional transaction handle, so the module moves all faces in one statement. The scan prefilter uses BTRIM.
  • face-person-verdict.repository.ts: resolveAssignedFace is removed because drainPendingForFaces covers it. The getPendingForPerson read gate uses BTRIM. SQL docs regenerated.
  • utils/face-repair.ts: isSuggestionScanTarget() is the single scan-eligibility rule for personal and space people, replacing three inline checks and SharedSpaceService.isNamedVisibleSpacePerson.
  • shared-space.service.ts: uses the shared eligibility rule. The space merge drain is one bulk statement instead of one per face.
  • Tests: call-site unit specs now assert one module call instead of call-order mocks. A new medium suite covers the module.

Behaviour changes

  • A personal person whose name is only whitespace is no longer a face-suggestion target. Space people already worked this way. This applies to the scan job, the queue-all prefilter, the re-scan trigger on rename, and the pending-suggestions read. Names arriving through XMP face regions bypass the DTO, which is why the rule lives on the read side rather than as a DTO trim.
  • The SQL side uses BTRIM, which strips spaces only. A name made only of tabs is still queued by the prefilter and then skipped by the job, which is harmless.
  • reassignFaces now commits all faces in one transaction. Before, faces were committed one at a time. A failure now leaves none of them moved instead of some.
  • createFace now also drains pending suggestions and clears negative verdicts for the new face. Both are empty for a face that was just created, so there is no visible change.

Testing

  • pnpm check (tsc): clean.

  • pnpm lint: 0 problems.

  • Full unit suite (pnpm test -- --run): 201 files, 6432 passed, 0 failed.

  • New medium suite test/medium/specs/services/face-assignment.service.spec.ts (12 tests) covers:

    • atomicity on the module's own transaction and on the caller's
    • manual and owner-person strength, including the identity the re-link points at
    • the from guard and the in-place case
    • negative verdicts cleared only for the target
    • pending suggestions drained
    • six fixtures where claimPending's SQL and getSettledFaceIds must agree

    Removing the transaction wrap makes the atomicity test fail.

  • Whitespace-name medium rows in person.repository.spec.ts and face-person-verdict.repository.spec.ts fail when the BTRIM lines are reverted.

  • Related medium specs pass, 20 files and 646 tests: face-repair*, face-suggestion-exclusions, person.service, identity-merge-propagation, face-verdict.merge-durability, face-review-cross-flow, shared-space face specs, people-identity-rbac, and the verdict, identity and decline repository specs. They were run with DOCKER_HOST=unix:///run/user/1002/podman/podman.sock TESTCONTAINERS_RYUK_DISABLED=true pnpm exec vitest --config test/vitest.config.medium.mjs --run <files>.

  • A full medium run has 30 failures in the exif specs, library.service.spec.ts and workflow-core-plugin.spec.ts. The same 30 fail on main with this branch's changes stashed. Under full-suite load, memory.service.spec.ts and database-migration.service.spec.ts failed once and pass on their own.

Follow-ups

  • Space confirm path. confirmSpacePersonFaceSuggestion still writes its own identity, drain and clear sequence. That code is already transactional; it differs in that it writes a shared_space_person_face projection row and uses ensureSpacePersonIdentity. If it should join the module, AssignFacesInput could take the VerdictTarget shape (personGroupId or spacePersonId) that clearNegativeForTarget and targetTokens already accept.
  • Detach and unassign are not routed through the module.
  • replaceFaceIdentities writes source unconditionally on conflict, so an unlocked cleanup move (owner-person) of a face a human placed manually downgrades it from manual. The single-face replaceFaceIdentity uses preserveManualSource instead. This behaviour predates this PR. Whether an unlocked admin move should keep the manual lock is a product call (B2 says an unlocked move stays reviewable), so it is left as is.
  • createAssetFace and the new face's identity link in createFace are still two transactions, because upstream createAssetFace takes no transaction handle.

luminoso and others added 3 commits October 6, 2026 08:54
…gnment module

"This face now belongs to this person" was a four-write sequence (move the
face, re-point its identity link, drain pending suggestions, clear negative
verdicts for the target) hand-written at five call sites. Two of them,
reassignFaces and reassignFacesById, ran it autocommit: a failure part-way
left the face on the new person still carrying the old identity, which
FaceIdentityBackfill can resolve back to the old person and silently revert
the user's correction (D14).

FaceAssignmentService (fork-only, replaces FaceVerdictService) now owns that
sequence behind assignFaces({ personGroupId, faceIds, strength, from? }, trx?).
It joins the caller's transaction or opens its own, so every path is atomic:
reassignFaces, reassignFacesById, createFace, confirmFaceSuggestion,
executeRepair and the cleanup lock bucket. It also owns the read side,
getSettledFaceIds, which replaces the token strings the suggestion scans
built by hand with targetTokens() + isSettledForOwner().

Also:
- one isSuggestionScanTarget() rule for personal and space people, so a
  whitespace-only name no longer makes a personal person scannable; the
  personal SQL prefilter and read gate now use BTRIM like the space ones
- resolveAssignedFace is gone (drainPendingForFaces covers it); the space
  merge drain is one bulk statement instead of one per face
- new medium suite pins the module's atomicity, strengths, negative-verdict
  scoping, and that claimPending's SQL agrees with the TS settlement check
- move faces with one PersonRepository.reassignFaces statement on the
  caller's transaction instead of one UPDATE per face
- pin the personal whitespace-name rule: scannable prefilter and
  getPendingForPerson read gate (medium), personal suggestion scan (unit)
- assert the unlocked-move relink targets the destination identity
- spell out both `from` cases and the BTRIM vs trim() edge in comments;
  inline the one-line space merge drain wrapper
…e same person

executeRepair routes current -> suspected owner, and the two can coincide.
assignFaces treated that as a re-affirm in place, which skips the move
query's filter (ML-sourced, visible, not deleted), so an unlocked route
downgraded a hand-drawn face's manual link to owner-person. The move query
used to skip those faces. executeRepair now asks for the filter in place
too (movableOnly); the lock path keeps re-affirming every face on the person.
@Deeds67
Deeds67 force-pushed the refactor/face-assignment-module branch from f08aeea to 23d9284 Compare October 6, 2026 06:57
@Deeds67 Deeds67 added the changelog:chore Chore/maintenance for changelog label Oct 6, 2026
@Deeds67
Deeds67 merged commit bae804f into open-noodle:main Oct 6, 2026
74 of 76 checks passed
Deeds67 added a commit that referenced this pull request Oct 6, 2026
…gnment module (#1156)

* refactor(server): route personal face placement through one face-assignment module

"This face now belongs to this person" was a four-write sequence (move the
face, re-point its identity link, drain pending suggestions, clear negative
verdicts for the target) hand-written at five call sites. Two of them,
reassignFaces and reassignFacesById, ran it autocommit: a failure part-way
left the face on the new person still carrying the old identity, which
FaceIdentityBackfill can resolve back to the old person and silently revert
the user's correction (D14).

FaceAssignmentService (fork-only, replaces FaceVerdictService) now owns that
sequence behind assignFaces({ personGroupId, faceIds, strength, from? }, trx?).
It joins the caller's transaction or opens its own, so every path is atomic:
reassignFaces, reassignFacesById, createFace, confirmFaceSuggestion,
executeRepair and the cleanup lock bucket. It also owns the read side,
getSettledFaceIds, which replaces the token strings the suggestion scans
built by hand with targetTokens() + isSettledForOwner().

Also:
- one isSuggestionScanTarget() rule for personal and space people, so a
  whitespace-only name no longer makes a personal person scannable; the
  personal SQL prefilter and read gate now use BTRIM like the space ones
- resolveAssignedFace is gone (drainPendingForFaces covers it); the space
  merge drain is one bulk statement instead of one per face
- new medium suite pins the module's atomicity, strengths, negative-verdict
  scoping, and that claimPending's SQL agrees with the TS settlement check

* refactor(server): fold review findings into the face-assignment module

- move faces with one PersonRepository.reassignFaces statement on the
  caller's transaction instead of one UPDATE per face
- pin the personal whitespace-name rule: scannable prefilter and
  getPendingForPerson read gate (medium), personal suggestion scan (unit)
- assert the unlocked-move relink targets the destination identity
- spell out both `from` cases and the BTRIM vs trim() edge in comments;
  inline the one-line space merge drain wrapper

* fix(server): keep the move filter on a cleanup route that stays on the same person

executeRepair routes current -> suspected owner, and the two can coincide.
assignFaces treated that as a re-affirm in place, which skips the move
query's filter (ML-sourced, visible, not deleted), so an unlocked route
downgraded a hand-drawn face's manual link to owner-person. The move query
used to skip those faces. executeRepair now asks for the filter in place
too (movableOnly); the lock path keeps re-affirming every face on the person.

---------

Co-authored-by: Pierre Marais <pierremarais67@gmail.com>
Deeds67 added a commit that referenced this pull request Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:chore Chore/maintenance for changelog 🗄️server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants