From 6c582bbb3f13ec336265a5a8afe797547e21a715 Mon Sep 17 00:00:00 2001 From: Francisco Solla Date: Mon, 24 Aug 2026 18:38:19 -0300 Subject: [PATCH] =?UTF-8?q?fix(contact):=20C120=20=E2=80=94=20serializa=20?= =?UTF-8?q?o=20append=20de=20telefone=20no=20reuso=20com=20lock=20de=20fic?= =?UTF-8?q?ha=20(Closes=20#663)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/changelog/2026-08-24-c120.md | 1 + .../c120-race-append-telefone-ficha-impl.md | 238 ++++++++++++++++++ ...-unificar-chave-invite-redemption-ficha.md | 85 +++++++ src/collections/CampaignUser.ts | 6 + src/utilities/contactIdentity.ts | 32 ++- src/utilities/contactPhoneLocks.ts | 21 ++ tests/int/contactPhoneAppendRace.int.spec.ts | 126 ++++++++++ .../int/postgresTransactionLocks.int.spec.ts | 7 +- 8 files changed, 510 insertions(+), 6 deletions(-) create mode 100644 docs/changelog/2026-08-24-c120.md create mode 100644 docs/plans/c120-race-append-telefone-ficha-impl.md create mode 100644 docs/plans/c121-unificar-chave-invite-redemption-ficha.md create mode 100644 tests/int/contactPhoneAppendRace.int.spec.ts diff --git a/docs/changelog/2026-08-24-c120.md b/docs/changelog/2026-08-24-c120.md new file mode 100644 index 00000000..1deea0f2 --- /dev/null +++ b/docs/changelog/2026-08-24-c120.md @@ -0,0 +1 @@ +**C120 — race de append de telefone em `findOrCreateContactByPhone` serializada por lock de ficha (2026-08-24):** lock de ficha `contact-ficha:` AO REDOR do read-modify-write de reuso, com **re-leitura da ficha dentro do lock** (sem ela o lock seria inócuo: o array do find pré-lock fica stale). Débito colhido no /simplify do C112: os locks eram só por telefone — disjuntos entre fluxos que resolvem a MESMA ficha por primários DIFERENTES —, cada fluxo lia o array antes do outro commitar e o last-writer-wins sumia com um número digitado. **`src/utilities/contactPhoneLocks.ts`** ganhou `contactFichaLockKey`/`acquireContactFichaLock` + doc da ordem (telefones em batch primeiro, ficha por último e sozinha — sem ciclo); **`src/utilities/contactIdentity.ts`** adquire a ficha-lock e re-lê (`findByID`, `select: {phones}`) antes de montar o append no reuso; **`src/collections/CampaignUser.ts`** aplica o mesmo lock no sync conta→ficha (2 linhas; collections ⇒ CI int full + e2e curated + build). Sem migration (schema intocado). **Spec int `tests/int/contactPhoneAppendRace.int.spec.ts`**: ficha `{P1,P2}`, fluxos disjuntos [P1,P3]/[P2,P4] com gate determinístico no find — rodado contra o código antigo prova a perda (3 phones), com a fix prova a sobrevivência {P1,P2,P3,P4} (fase TDD vermelho→verde). Pin do key em `postgresTransactionLocks.int.spec.ts`. /simplify (2 revisores): sem achados alta/média bloqueantes; fixes pontuais aplicados (comentário de bypass no re-read, nota READ COMMITTED no header, gate do spec casado por `equals` em vez de substring do where, fixture via `createContact`). Débitos: **C121 (#885) registrado** — unificar a chave da redenção de convite (`invite-redemption-contact:`, `campaignInviteRepository.ts:127-132`/`campaignInviteRedemption.ts:66-145`) com `contact-ficha:` + revalidar spec literal (~1 dia); full-replace/reorder editors e tombstones **Explicitamente fora** (semântica de editor; documentado); extração do helper RMW deferida com gatilho 3º call site; rename `contactPhoneLocks.ts`→`contactLocks.ts` descartado. Verificação: `pnpm gate:fast` verde (lint/typecheck/2458 unit), int full verde (94 arquivos/831 testes), build verde, e2e `campaignContacts` verde (6/6); impl plan `docs/plans/c120-race-append-telefone-ficha-impl.md` + `docs/plans/c121-unificar-chave-invite-redemption-ficha.md`. \ No newline at end of file diff --git a/docs/plans/c120-race-append-telefone-ficha-impl.md b/docs/plans/c120-race-append-telefone-ficha-impl.md new file mode 100644 index 00000000..f206a26a --- /dev/null +++ b/docs/plans/c120-race-append-telefone-ficha-impl.md @@ -0,0 +1,238 @@ +# Impl: C120 — race de append de telefone em findOrCreateContactByPhone (lock de ficha) + +Status: aprovado +Atualizado em: 2026-08-24 +Issue: #663 +Intenção: body da Issue (fonte única — não há `docs/plans/c120-*.md` de intenção; o body é a spec) +Appetite restante: herdar sem appetite declarado (P2) + +## Leitura da intenção + +- **Outcome (aceite):** dois fluxos concorrentes contra a MESMA ficha nunca perdem um + telefone digitado — ou, na alternativa, uma decisão documentada de aceitar a perda com + rationale. O alvo nomeado é `findOrCreateContactByPhone` + (`src/utilities/contactIdentity.ts`). +- **O que NÃO negociar:** granularidade de lock por ficha (não trocar o modelo de + identidade); sem mudança de schema (nenhuma migration; `contact_phones` relacional + intocado); sem mudança de Access/Consent; editar o dono que já possui o concern + (locks de contato), não criar caminho paralelo. +- **Por que não foi no C112:** a granularidade por-telefone é herdada do C111; o C112 + alargou a janela ao introduzir o append-on-reuse (re-registro/import da MESMA pessoa + com 1 ficha e primário inambíguo). Dois fluxos que resolvam a mesma ficha por primários + DIFERENTES (ficha com 2+ números) não disputam nenhum `contact-phone:*` em comum — os + locks por-telefone disjuntos não os serializam, os dois leem o array pré-mudança e o + last-writer-wins perde um append. O lock por ficha é o ponto de serialização correto. +- **O que reavaliar:** (a) o MECANISMO — lock de ficha+re-read (A) vs escrita + determinística SQL (B) vs aceitar/documentar (C); (b) se o RMW do sync conta→ficha + (`CampaignUser.ts`) entra no escopo mesmo com o custo CI de `src/collections/` + (HIGH_RISK); (c) os editors full-replace de staff — entrarem ou serem documentados + como fora do aceite; (d) se a chave do invite redemption (`invite-redemption-contact:`) + é unificada agora ou vira débito; (e) a forma do spec int (chamada direta da unidade vs + através dos flows de action, que exigem consent/municipality etc.). + +## Abordagem recomendada + +O núcleo é lock de ficha `contact-ficha:` AO REDOR do read-modify-write **com +re-leitura da ficha DENTRO do lock**. Sem a re-leitura o lock não resolve nada: o array +lido no `find` pré-lock pode estar stale e o append ainda se perde. Os locks de telefone +de hoje são mantidos (são a serialização cruzada entre fichas distintas no create-path e +a proteção de dedupe de criações concorrentes); o ficha-lock é sempre adquirido POR +ÚLTIMO e sozinho (uma chave de entidade por RMW), então não existe ciclo de lock-order. + +```mermaid +sequenceDiagram + participant A as Fluxo A (tx₁) + participant P as Postgres + participant B as Fluxo B (tx₂) + + Note over A,P: locks de telefone disjuntos: P1,P3 (A) vs P2,P4 (B) + A->>P: acquire(contact-phone:P1, contact-phone:P3) + B->>P: acquire(contact-phone:P2, contact-phone:P4) + A->>P: find contact where phones.value=P1 → ficha F + B->>P: find contact where phones.value=P2 → ficha F + Note over A,B: gate segura os DOIS finds → ambos leem [P1,P2] pré-mudança + A->>P: acquire(contact-ficha:F) %% NOVO — lock de ficha, por último + A->>P: findByID F (re-read, select phones) → [P1,P2] + A->>P: update F phones=[P1,P2,P3] + B->>P: acquire(contact-ficha:F) %% bloqueia até o commit de A (xact lock) + A->>P: commit → libera o lock + B->>P: findByID F (re-read fresco) → [P1,P2,P3] + B->>P: update F phones=[P1,P2,P3,P4] + B->>P: commit + Note over P: F = {P1,P2,P3,P4} — nenhum telefone digitado perdido +``` + +Sem o fix, A e B escrevem `[P1,P2,P3]` e `[P1,P2,P4]` a partir do MESMO array lido +`[P1,P2]` e o último commit vence — final com 3 números, P3 ou P4 perdido +(last-writer-wins). O lock de xact garante commit-then-release: o segundo fluxo a entrar +sempre re-lê o append do primeiro (que já está commitado) e só acrescenta os seus +faltantes. + +**Opções consideradas:** + +- **A — lock de ficha `contact-ficha:` + re-read dentro do lock (recomendada):** + após o `find` retornar exatamente 1 ficha (`totalDocs === 1`), adquirir o lock, re-ler + a ficha via `findByID` (depth 0, `select: { phones: { value: true } }`) dentro do lock e + montar o append de faltantes sobre essa leitura fresca; `update` só se `missing > 0`. + Mantém os phone locks e o `find` de dedupe intactos. Custo: 1 round-trip de lock + 1 + `findByID` seletivo, só no branch de reuso (caminho raro). +- **B — escrita determinística via SQL (rejeitada):** append direto na tabela relacional + `contact_phones` (`_parent_id`/`_order`/`value`) sem read prévio. Rejeitada porque: + `contact_phones` não tem índice UNIQUE nem validação própria — a dedupe intraficha vive + só em memória (`normalizeContactPhones`, `Contact.ts:22-59`); um insert SQL burlaria o + adapter/hook do Payload (validation, `overrideAccess`, transação única) e o commit + atômico com o resto do flow; a mesma garantia custaria algo razoável, mas com blast + radius de camada inteira (SQL + ordenação `_order` + validação), para um problema que um + advisory lock resolve com 3 linhas no padrão já existente do repo. +- **C — aceitar e documentar (rejeitada):** a perda não é benigna em escopo: é um número + digitado que SOME silenciosamente num cadastro de rotina (dois registros da mesma pessoa + via canais/arquivos diferentes é uso real do apoiador/liderança flow, não janela + desprezível), o dado é PII recém-coletado e o custo do fix (A) é baixo e in-pattern. + Aceitar exigiria além disso um rationale de produto que não existe. + +**Recomendação:** A. + +**Rejeitadas:** B (camada errada — contorna o dono), C (dado coletado some), e também o +lock de ficha SEM re-read (insuficiente por construção: serializa a escrita mas não o +conteúdo — a re-leitura dentro do lock é a parte que torna o append correto). + +### Componentes / mudanças + +- **~ `src/utilities/contactPhoneLocks.ts`** — adicionar `contactFichaLockKey(id)` → + `contact-ficha:${id}` e `acquireContactFichaLock(payload, req, id)` (um wrapper de + `acquireTextAdvisoryLocks`). Fica no dono dos locks de contato; a chave segue o padrão + das locks por entidade existentes (`campaign-demand:`, `activity:`, + `contact-name:`, `person-delete:`). +- **~ `src/utilities/contactIdentity.ts:68-87`** — no branch `totalDocs === 1`, após + resolver a ficha: `await acquireContactFichaLock(payload, req, existing.id)`; re-ler a + ficha com `payload.findByID` (depth 0, `select: { phones: { value: true } }`, + `overrideAccess: true`, `req`) DENTRO do lock; montar `missing` e o array final sobre a + RE-LEITURA (não sobre `docs[0]` do find); `update` só com `missing > 0`. Atualizar o + doc-comment do módulo (mencionar a serialização por ficha no reuso). +- **~ `src/collections/CampaignUser.ts:270-291`** — no RMW do sync conta→ficha + (account phone mudou e a ficha existe), adquirir `contact-ficha:` antes do `findByID` + existente (que já é a leitura do RMW: lock antes do read serializa read+write com um lock + só). **DECISÃO: ENTRA no escopo.** Mesma classe de perda (append perdido → número + digitado some no sync), mesmo padrão/dono, e o hook já roda em transação no postgres + (C99/int exercita phone locks dentro desses hooks — `req.transactionID` existe). Custo: + 2 linhas vs 1 camada de risco real no mesmo defeito silencioso. O custo CI (collections = + HIGH_RISK) está registrado em Riscos e em Fases. +- **+ `tests/int/contactPhoneAppendRace.int.spec.ts`** — spec de concorrência (ver Fase 2). +- **Full-replace editors (`updateSupporterContactRecord` em `actions/supporter.ts:216-223`, + `contact.ts`, `leadership.ts` field `phones`/wizard, `advisor.ts`, tombstones e o branch + `phone` do `leadership.ts` inline) — FORA do aceite (documentado, não alterado):** + a semântica de um editor é "o array é agora exatamente isto" — a ficha lock não muda o + last-writer-wins de um replace; e o branch `phone` inline (reorderWithPrimaryPhone) é + edição explícita de staff que define o primário, não append implícito. O lock só faz + sentido onde dois fluxos appendam sem intenção de sobrescrever. Documentar no header do + `contactPhoneLocks.ts` (quem serializa o quê) e no commit. +- **Invite redemption (`invite-redemption-contact:`, `campaignInviteRepository.ts:127-132`)** + — RMW do profile JÁ serializado, mas por chave DIFERENTE → ficha não fica globalmente + serializada entre create-flow e redemption-flow. **DECISÃO: capturar como débito**, não + unificar agora (unificar é renomear a chave e revalidar os specs de redemption — escopo + e risco extras sem relação com o aceite de C120). Nova Issue de débito. +- **Migration:** sem migration (nenhuma mudança de schema; `contact_phones` continua + `_parent_id`/`_order`/`value`, sem índice UNIQUE, dedupe intraficha segue em memória). +- **Access/Consent:** n/a (sem mudança de acesso; `overrideAccess: true` inalterado nos + pontos tocados, coberto pelos gates de fluxo existentes). +- **UI:** n/a. + +## Fases verificáveis + +1. **Schema+server** — editar `contactPhoneLocks.ts`, `contactIdentity.ts` e + `CampaignUser.ts`, conforme Componentes. Nenhuma migration; nenhum tipo novo + (`generate:types` desnecessário — não há campo novo). Verificação local: `tsc --noEmit`. +2. **Teste int** — criar `tests/int/contactPhoneAppendRace.int.spec.ts` (formato abaixo) e + rodar com o db de teste do worktree (`pnpm test:int contactPhoneAppendRace`). **Prova em + duas direções:** rodar o spec contra o código ATUAL (sem o fix) e ver a asserção + `{P1,P2,P3,P4}` falhar com 3 números (prova a perda — o teste não é tautológico); com o + fix, a mesma spec passa (prova a sobrevivência). O teste fixa a regressão. +3. **Gates de CI** — com `src/collections/` no diff (CampaignUser), o CI `checks` roda + int FULL + e2e CURATED + build + unidades changed (OPS86: diff high-risk nunca zero); + `contactIdentity.ts`/`contactPhoneLocks.ts` não têm entrada no manifest nem em + `E2E_RISK_PREFIXES` — mas o HIGH_RISK de collections sobrepõe o fallback + `campaignHomeActions`. Rodar full cascade local: guards → lint → format → typecheck → + knip → cycles → unit → int → build. + +**Formato do spec int de concorrência (Fase 2):** + +- Plantar a ficha alvo `F = {P1, P2}` via `campaignFixtures().createContact({ phones: +[{ value: P1 }, { value: P2 }] })` (P1/P2 de `campaignFixtures().phone()`). +- Dos fluxos CONCORRENTES com primários DIFERENTES e conjuntos de telefone DISJUNTOS + (requisito — se os phone locks se sobrepõem, a serialização por-telefone mascara a race): + fluxo A `[P1, P3]`, fluxo B `[P2, P4]`, cada um chamando `findOrCreateContactByPhone` + direto dentro de `withPayloadTransaction` (que abre transação real e fornece o `req` com + `transactionID` — exigência do advisory lock). Forma DECIDIDA: chamada direta da unidade, + não via `createSupporterRecord`/`createLeadershipRecord` — os flows de action adicionam + consent + municipality + dedupe de liderança/supporter (outras unidades sob teste e mais + fixtures), enquanto a unidade sob teste é exatamente `findOrCreateContactByPhone` e ambos + os flows reais a chamam de forma idêntica dentro das suas transações. +- Gate com `findSpy` sobre `payload.find` segurando o 1º find de AMBOS os fluxos + (`firstReadGate`/`firstReadReached`, padrão de `campaignLeadership.int.spec.ts:181-285`, + porém com gate nos DOIS reads) — garante que ambos leem o estado `[P1,P2]` antes de + qualquer escrita, tornando a perda determinística sem o fix e a sobrevivência + determinística com o fix (o xact lock garante: quem entrou por último re-lê o append já + commitado do primeiro). Disparar via `Promise.allSettled`. +- Asserções: ambos resolvem com `reused: true` e `contactID === F.id`; a ficha F final tem + EXATAMENTE {P1,P2,P3,P4} (comparação por conjunto). O assert do waiter de pg_locks + (`waitForAdvisoryLockWaiter` + PID via `beginSpy`, que funciona no existing spec porque o + waiter vira determinístico quando os dois reads estão gated) pode reforçar o mecanismo, + mas não é exigido pelo aceite — o state final basta. +- Cleanup: `installCampaignFixtures` (markers + owned) cuida do teardown. + +## Rabbit holes / Não escopo + +- **Lock ordering:** em `findOrCreateContactByPhone`, phone locks são adquiridos em batch + ordenado ANTES de qualquer lock de entidade; o ficha-lock é adquirido por ÚLTIMO e sempre + SOZINHO (uma chave de ficha por RMW; o branch `totalDocs !== 1` não faz RMW e não ganha + lock). Nenhum fluxo segura 2 chaves de entidade de contato simultaneamente + (`person-delete:` e `contact-name:`, `invite-redemption-contact:` + seguem com chaves próprias e nunca são combinadas com `contact-ficha:*`) — sem ciclo. +- **Escrita determinística SQL:** rejeitada (opção B) — exigiria validar/ordenar/ + deduplicar fora do hook e do adapter. +- **Full-replace/reorder editors:** fora do aceite, documentados (ver Componentes). +- **Unificar a chave do invite redemption:** débito capturado, não no escopo. +- **Branch de criação (`totalDocs === 0` / `2+`):** sem mudança — a identidade na criação é + a ficha recém-escrita e os phone locks já serializam as criações concorrentes. +- **Não mexer em `supporterImport.ts`** (modelo de "todos os phones locked up front" intacto). +- **Não adicionar lock em `updateSupporterContactRecord`** nem nos demais editors. + +## Riscos e mitigação + +- **Lock de ficha sem re-read ser insuficiente contra a intenção:** não — a re-leitura é + parte integrante do componente (fora do lock o fix seria inócuo). O spec int cobre os dois + fluxos re-lendo dentro do lock. +- **`CampaignUser.ts` sem transação ativa em algum chamador do hook:** no postgres o + adapter envolve cada mutation numa transação e expõe `transactionID` aos hooks (evidência + C99/int: os phone locks já rodam dentro desses hooks). Se um chamador futuro invocar o + update sem transação, `acquireTextAdvisoryLocks` lança com a mensagem fail-closed padrão + — nunca perde dado silenciosamente. Residual baixo. +- **Deadlock entre fluxos:** chaves de ficha e telefone disjuntas entre os fluxos do aceite + (A usa P1,P3 e F; B usa P2,P4 e F — só F em comum, e a ficha é sempre a última); nenhum + fluxo adquire uma chave de outro fluxo depois de segurar a própria. A invariante de + ordenação é única no código (documentada no comment do módulo). +- **Custo CI (collections = HIGH_RISK):** int full + e2e curated + build no PR. Patch + pequeno e de padrão já coberto; o custo é o preço da camada de serialização no sync + conta→ficha (C99 já paga esse preço hoje por incluir o hook em collections). +- **Re-read extra (perf):** 1 `findByID` seletivo a mais, só no branch de reuso; + desprezível vs. o round-trip de lock já existente no mesmo caminho. +- **Ficha deletada entre o `find` e o lock (`person-delete` concorrente):** a re-leitura + falha com erro claro → rollback da transação (fail-closed), sem corrupção. Fora do aceite. +- **Flakiness do spec:** telefones únicos por fixture (`phone()`), gate determinístico nos + dois reads, transações via `withPayloadTransaction` (commit/rollback corretos); + `waitForAdvisoryLockWaiter` só reforça o mecanismo e usa espera ativa com timeout (padrão + já usado no repo). + +## Aceite de engenharia + +- [ ] Aceite da intenção: dois fluxos concorrentes contra a mesma ficha nunca perdem um + telefone digitado (spec int prova a perda sem o fix e a sobrevivência com o fix) +- [ ] Decisão de concorrência registrada (A recomendada) com B e C rejeitadas e rationale +- [ ] Invariantes AGENTS/engineering-standards: sem migration, sem schema, sem + Access/Consent; edita o dono (`contactPhoneLocks.ts`); sem paralelismo de caminho +- [ ] Testes de domínio previstos: `tests/int/contactPhoneAppendRace.int.spec.ts` + (concorrência por ficha, primários distintos, gate nos dois reads) +- [ ] CI previsto: int full + e2e curated + build + unidades changed (blast de + `src/collections/CampaignUser.ts`) — gates verdes antes do merge +- [ ] Débitos registrados em Issue: unificação da chave `invite-redemption-contact:*` + vs `contact-ficha:*`; full-replace/reorder editors documentados como fora do aceite diff --git a/docs/plans/c121-unificar-chave-invite-redemption-ficha.md b/docs/plans/c121-unificar-chave-invite-redemption-ficha.md new file mode 100644 index 00000000..1a27534a --- /dev/null +++ b/docs/plans/c121-unificar-chave-invite-redemption-ficha.md @@ -0,0 +1,85 @@ +# Impl: C121 — unificar a chave de lock da redenção de convite com o lock de ficha + +Status: rascunho +Atualizado em: 2026-08-24 +Issue: #C121 (débito registrado em `pnpm agent:register` pelo fechamento do C120) +Intenção: débito capturado no fechamento do C120 (ver `docs/plans/c120-race-append-telefone-ficha-impl.md` §Componentes, "Invite redemption") +Appetite restante: ~1 dia + +## Leitura da intenção + +- **Outcome:** o read-modify-write do profile na redenção de convite passa a + serializar contra o **mesmo** lock de ficha do create-flow + (`contact-ficha:`, C120): redenção e create nunca mais perdem um telefone + do outro quando resolvem a MESMA ficha. +- **O que NÃO negociar:** sem mudança de schema; sem tocar a semântica de + reorder do primário da redenção (o RMW dele reordena por design); sem mexer + no `invite-redemption-user:*` / `account-username:*` (locks de outra faixa). +- **O que reavaliar:** o lock de redenção hoje cobre a transação INTEIRA da + redenção (`invite-redemption-contact:`); trocar a chave mantém a mesma + janela, só que alinhada ao create-flow. + +## Abordagem recomendada + +Trocar `invite-redemption-contact:` → `contact-ficha:` na redenção +(`campaignInviteRepository.ts:127-132` / `campaignInviteRedemption.ts:66-145`), +usando `acquireContactFichaLock`. O lock já está em `contactPhoneLocks.ts` desde +o C120; aqui é só o call-site novo + revalidar as asserções **literais** do +spec `tests/int/campaignInvite.int.spec.ts:799-898` (que ancora a chave antiga). + +**Opções consideradas:** +- **A — reusar `acquireContactFichaLock` trocando a chave (recomendada):** 1 + call-site + revalidação do spec; alinha com o C120. +- **B — manter chaves separadas e documentar (rejeitada):** é exatamente o gap + que o C120 abriu — dois locks de entidade de contato que não se serializam. +- **Rejeitadas além:** nenhuma (sem alternativa de mecanismo; a semântica de + advisory lock é a mesma já aceita no C120). + +### Componentes / mudanças + +- **`src/utilities/campaignInviteRedemption.ts`** — trocar a chave para + `contact-ficha:`. +- **`src/utilities/campaignInviteRepository.ts:127-132`** — a public key skin de + `` (semântica atual) aponta para `contact-ficha:`. +- **`tests/int/campaignInvite.int.spec.ts:799-898`** — espera literal da chave + atualizada. +- **Migration:** sem migration. **Access/Consent:** n/a. **UI:** n/a. + +## Fases verificáveis + +1. Trocar a chave (2 arquivos) + revalidação do spec literal. +2. `pnpm test:int campaignInvite` + spec do C120 (`contactPhoneAppendRace`). +3. Gates: `pnpm gate:fast`; entrega com `pnpm push`. + +## Já resolvido no simplify/critique (não reabrir) + +- Documentação da dependência do C120 em `contactPhoneLocks.ts` (quem + serializa o quê). +- Full-replace/reorder editors (supporter/contact/leadership/tombstones) — + **Explicitamente fora** (decisão travada no C120; editores têm semântica de + full-replace, lock não muda last-writer-wins). + +## Rabbit holes / Não escopo + +- Não unificar `invite-redemption-user:*` nem `account-username:*` (outra faixa + de lock, outra finalidade). +- Não extrair helper de RMW (duplicação C120 ficou em 2 call sites — regra do + repo: <3 não abstrai). **Gatilho de extração:** 3º call site do padrão + "lock de ficha + re-read + append-faltantes" (se este C121 adotar o + append-form na redenção, vira o próprio 3º call site — revisitar). +- Renomear `contactPhoneLocks.ts` → `contactLocks.ts`: somente SE este item já + tocar o arquivo (fase cosmética opcional por último). + +## Riscos e mitigação + +- Spec ancora a chave literal → atualizar junto na mesma entrega (sem isso o + PR falha com a asserção velha, nunca com falsos verdes). +- Redenção é single-use (token linka uma conta); a coincidência com um + create-append da mesma ficha é rara — mitigada pelo lock compartilhado. + +## Aceite de engenharia + +- [ ] Redenção e create-flow serializam a ficha com a MESMA chave +- [ ] Spec `campaignInvite.int.spec.ts` revalidado com a chave nova +- [ ] Sem migration/schema; Access/Consent intocados +- [ ] CI verde antes do merge (spec + gates) \ No newline at end of file diff --git a/src/collections/CampaignUser.ts b/src/collections/CampaignUser.ts index 0e426654..fa374cf3 100644 --- a/src/collections/CampaignUser.ts +++ b/src/collections/CampaignUser.ts @@ -25,6 +25,7 @@ import { } from '@/utilities/campaignAccess' import { buildCampaignPasswordResetUrl } from '@/utilities/campaignPasswordReset' import { findOrCreateContactByPhone } from '@/utilities/contactIdentity' +import { acquireContactFichaLock } from '@/utilities/contactPhoneLocks' import { APIError, type CollectionAfterReadHook, @@ -272,6 +273,11 @@ const ensureCampaignUserContactIdentity: CollectionBeforeChangeHook 0 && data.phone !== originalDoc?.phone ) { + // C120 — same ficha lock as the find-or-create reuse append: this + // read-modify-write must not read a stale phones array while another + // flow appends to the same ficha. Lock first, then the read below is the + // RMW's read already serialized by it. + await acquireContactFichaLock(req.payload, req, linkedContactID) const contact = await req.payload.findByID({ collection: 'contact', id: linkedContactID, diff --git a/src/utilities/contactIdentity.ts b/src/utilities/contactIdentity.ts index 34d6bbf2..08a3c760 100644 --- a/src/utilities/contactIdentity.ts +++ b/src/utilities/contactIdentity.ts @@ -3,7 +3,7 @@ import 'server-only' import type { Payload, PayloadRequest } from 'payload' import type { Contact } from '@/payload-types' -import { acquireContactPhoneLocks } from '@/utilities/contactPhoneLocks' +import { acquireContactFichaLock, acquireContactPhoneLocks } from '@/utilities/contactPhoneLocks' import type { PayloadTransactionRequest } from '@/utilities/payloadTransaction' type ContactIdentityRequest = PayloadTransactionRequest | PayloadRequest @@ -20,9 +20,15 @@ type ContactIdentityRequest = PayloadTransactionRequest | PayloadRequest * created is the ficha just written; never a guess among the existing ones). * The remaining typed numbers follow the ficha wherever it lands: they fill * the fresh ficha, or are appended (only the ones it lacks, at the END — a - * re-registration never reorders the mesa's priority). Without phones, a - * fresh name-only ficha is created. Every "cria ou vincula a ficha" path - * shares one owner for the "BA default + shared-phone" policy. + * re-registration never reorders the mesa's priority). The reuse append is + * serialized by the ficha lock (`contact-ficha:`) with a fresh re-read of + * the phones INSIDE the lock — C120: two flows reusing the same ficha by + * different primaries must never drop each other's typed number. The re-read + * is what closes the race: `pg_advisory_xact_lock` is held until commit, so + * the peer's append is already committed before the lock grants the re-read + * sees it (READ COMMITTED — each statement takes a fresh snapshot). + * Without phones, a fresh name-only ficha is created. Every "cria ou vincula a ficha" + * path shares one owner for the "BA default + shared-phone" policy. * * Callers must already be inside an active Payload transaction (the advisory * lock requires it) and authorized to own the write (staff creation flows run @@ -67,7 +73,23 @@ export const findOrCreateContactByPhone = async ({ if (contacts.totalDocs === 1) { const existing = contacts.docs[0]! - const existingPhones = (existing.phones ?? []) + // C120 — serialize the append-on-reuse with every other flow writing + // THIS ficha, then RE-READ the phones inside the lock: the dedupe find + // above is a snapshot that can be stale once a concurrent flow reusing + // the same ficha by a different primary commits its own append, and + // appending from a stale array would silently drop its typed number. + await acquireContactFichaLock(payload, req, existing.id) + // Intentional admin bypass: same policy as the dedupe find — the ficha + // lock already serialized this flow; this read only derives the append target. + const current = await payload.findByID({ + collection: 'contact', + id: existing.id, + depth: 0, + select: { phones: { value: true } }, + overrideAccess: true, + req, + }) + const existingPhones = (current.phones ?? []) .map((entry) => entry.value) .filter((value): value is string => Boolean(value)) const missing = phones.filter((phone) => !existingPhones.includes(phone)) diff --git a/src/utilities/contactPhoneLocks.ts b/src/utilities/contactPhoneLocks.ts index 85789279..6a5301bb 100644 --- a/src/utilities/contactPhoneLocks.ts +++ b/src/utilities/contactPhoneLocks.ts @@ -14,12 +14,33 @@ type ContactPhoneRequest = PayloadTransactionRequest | PayloadRequest * look contacts up by phone: find-or-create, CSV import, invite redemption, * LGPD tombstone). They no longer guard uniqueness — they prevent two * concurrent flows from racing the same phone key. + * + * Ficha locks (C120 — `contact-ficha:`) serialize the append-on-reuse + * read-modify-write of ONE ficha: after the dedupe find commits to reusing a + * ficha, the flow locks the ficha itself and RE-READS its phones inside the + * lock before appending, so concurrent flows that reach the same ficha by + * different primaries never drop each other's typed number (the per-phone + * locks are disjoint in that case). Lock-order invariant: phone keys are + * always acquired batched first, any entity/ficha key LAST and alone — no + * flow ever holds a second entity key while requesting another, so there is + * no cycle. Full-replace editors (supporter/leadership/contact phone fields, + * tombstones) are NOT serialized by this key on purpose: an editor's + * semantics is "the array is now exactly this", and a lock would not change + * its last-writer-wins. */ export const contactPhoneLockKeys = (phones: string[]): string[] => [...new Set(phones)].sort().map((phone) => `contact-phone:${phone}`) +export const contactFichaLockKey = (id: number | string): string => `contact-ficha:${id}` + export const acquireContactPhoneLocks = ( payload: ContactPhonePayload, req: ContactPhoneRequest, phones: string[], ): Promise => acquireTextAdvisoryLocks(payload, req, contactPhoneLockKeys(phones)) + +export const acquireContactFichaLock = ( + payload: ContactPhonePayload, + req: ContactPhoneRequest, + id: number | string, +): Promise => acquireTextAdvisoryLocks(payload, req, [contactFichaLockKey(id)]) diff --git a/tests/int/contactPhoneAppendRace.int.spec.ts b/tests/int/contactPhoneAppendRace.int.spec.ts new file mode 100644 index 00000000..2e86bf0a --- /dev/null +++ b/tests/int/contactPhoneAppendRace.int.spec.ts @@ -0,0 +1,126 @@ +// @vitest-environment node + +import type { Payload } from 'payload' +import { getPayload } from 'payload' +import { beforeAll, describe, expect, it, vi } from 'vitest' + +import config from '@/payload.config' +import { findOrCreateContactByPhone } from '@/utilities/contactIdentity' +import { withPayloadTransaction } from '@/utilities/payloadTransaction' + +import { installCampaignFixtures } from '../helpers/campaignFixtures' + +let payload: Payload +const campaignFixtures = installCampaignFixtures({ + getPayload: () => payload, + setPayload: (nextPayload) => { + payload = nextPayload + }, +}) + +describe('findOrCreateContactByPhone — concurrent append on one ficha (C120)', () => { + beforeAll(async () => { + payload = await getPayload({ config: await config }) + }) + + it('never loses a typed phone when two flows reuse the same ficha by different primaries', async () => { + const fixtures = campaignFixtures() + const primaryA = fixtures.phone() // P1 — primary of flow A + const primaryB = fixtures.phone() // P2 — primary of flow B + const extraA = fixtures.phone() // P3 — only typed by flow A + const extraB = fixtures.phone() // P4 — only typed by flow B + + // Plant the shared ficha: two numbers, so every flow resolves it by a + // DIFFERENT primary (disjoint phone locks — the race the issue names). + const ficha = await fixtures.createContact({ + name: fixtures.value('Ficha Compartilhada'), + phones: [{ value: primaryA }, { value: primaryB }], + }) + + // Gate the two dedupe reads: hold flow A's find open until flow B's find + // has ALSO read the pre-change array. Without the fix both flows append + // from the same [P1,P2] snapshot and the last committer wins (3 phones). + const originalFind = payload.find.bind(payload) + let contactReads = 0 + let releaseFirstRead = () => {} + const firstReadGate = new Promise((resolve) => { + releaseFirstRead = resolve + }) + let markFirstRead = () => {} + const firstReadReached = new Promise((resolve) => { + markFirstRead = resolve + }) + let markSecondRead = () => {} + const secondReadDone = new Promise((resolve) => { + markSecondRead = resolve + }) + + const findSpy = vi.spyOn(payload, 'find').mockImplementation(async (args) => { + const result = await originalFind(args) + const where = + args.collection === 'contact' && 'where' in args + ? (args.where as Record) + : {} + const dedupeOn = + typeof where['phones.value'] === 'object' + ? (where['phones.value'] as Record)['equals'] + : undefined + if (typeof dedupeOn === 'string' && (dedupeOn === primaryA || dedupeOn === primaryB)) { + contactReads += 1 + if (contactReads === 1) { + markFirstRead() + await firstReadGate + } else if (contactReads === 2) { + markSecondRead() + } + } + return result + }) + + const flowA = withPayloadTransaction(payload, async ({ req }) => + findOrCreateContactByPhone({ + payload, + req, + phones: [primaryA, extraA], + name: fixtures.value('Fluxo A'), + }), + ) + await firstReadReached + + const flowB = withPayloadTransaction(payload, async ({ req }) => + findOrCreateContactByPhone({ + payload, + req, + phones: [primaryB, extraB], + name: fixtures.value('Fluxo B'), + }), + ) + await secondReadDone + + releaseFirstRead() + + const [resultA, resultB] = await Promise.allSettled([flowA, flowB]) + findSpy.mockRestore() + + expect(resultA.status).toBe('fulfilled') + expect(resultB.status).toBe('fulfilled') + if (resultA.status === 'fulfilled') { + expect(resultA.value).toMatchObject({ contactID: ficha.id, reused: true }) + } + if (resultB.status === 'fulfilled') { + expect(resultB.value).toMatchObject({ contactID: ficha.id, reused: true }) + } + + const persisted = await payload.findByID({ + collection: 'contact', + id: ficha.id, + depth: 0, + overrideAccess: true, + }) + const finalPhones = (persisted.phones ?? []) + .map((entry) => entry.value) + .filter((value): value is string => Boolean(value)) + .sort() + expect(finalPhones).toEqual([primaryA, primaryB, extraA, extraB].sort()) + }) +}) diff --git a/tests/int/postgresTransactionLocks.int.spec.ts b/tests/int/postgresTransactionLocks.int.spec.ts index e2979451..ac164c7f 100644 --- a/tests/int/postgresTransactionLocks.int.spec.ts +++ b/tests/int/postgresTransactionLocks.int.spec.ts @@ -4,7 +4,7 @@ import { getPayload, type Payload } from 'payload' import { beforeAll, describe, expect, it, vi } from 'vitest' import config from '@/payload.config' -import { contactPhoneLockKeys } from '@/utilities/contactPhoneLocks' +import { contactFichaLockKey, contactPhoneLockKeys } from '@/utilities/contactPhoneLocks' import { acquireTextAdvisoryLocks, getPostgresTransactionDatabase, @@ -66,6 +66,11 @@ describe('PostgreSQL transaction advisory locks', () => { ]) }) + it('derives the canonical contact-ficha lock key (C120 append serialization)', () => { + expect(contactFichaLockKey(42)).toBe('contact-ficha:42') + expect(contactFichaLockKey('42')).toBe('contact-ficha:42') + }) + it('uses only the exact Payload transaction session and fails closed without it', async () => { const sessionDatabase = { execute: vi.fn().mockResolvedValue(undefined) } const unrelatedDatabase = { execute: vi.fn().mockResolvedValue(undefined) }