mirror of
https://github.com/fsolla/teqo.git
synced 2026-11-13 23:53:05 +00:00
fix(contact): C120 — serializa o append de telefone no reuso com lock de ficha (Closes #663)
This commit is contained in:
parent
2393847c9b
commit
b2f353e24e
8 changed files with 510 additions and 6 deletions
1
docs/changelog/2026-08-24-c120.md
Normal file
1
docs/changelog/2026-08-24-c120.md
Normal file
|
|
@ -0,0 +1 @@
|
|||
**C120 — race de append de telefone em `findOrCreateContactByPhone` serializada por lock de ficha (2026-08-24):** lock de ficha `contact-ficha:<id>` 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:<id>`, `campaignInviteRepository.ts:127-132`/`campaignInviteRedemption.ts:66-145`) com `contact-ficha:<id>` + 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`.
|
||||
238
docs/plans/c120-race-append-telefone-ficha-impl.md
Normal file
238
docs/plans/c120-race-append-telefone-ficha-impl.md
Normal file
|
|
@ -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:<id>`)
|
||||
é 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:<id>` 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:<id>` + 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:<id>`, `activity:<id>`,
|
||||
`contact-name:<normalized>`, `person-delete:<id>`).
|
||||
- **~ `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:<id>` 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:<id>`, `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:<id>` e `contact-name:<normalized>`, `invite-redemption-contact:<id>`
|
||||
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
|
||||
85
docs/plans/c121-unificar-chave-invite-redemption-ficha.md
Normal file
85
docs/plans/c121-unificar-chave-invite-redemption-ficha.md
Normal file
|
|
@ -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:<id>`, 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:<id>`); trocar a chave mantém a mesma
|
||||
janela, só que alinhada ao create-flow.
|
||||
|
||||
## Abordagem recomendada
|
||||
|
||||
Trocar `invite-redemption-contact:<id>` → `contact-ficha:<id>` 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:<id>`.
|
||||
- **`src/utilities/campaignInviteRepository.ts:127-132`** — a public key skin de
|
||||
`<id>` (semântica atual) aponta para `contact-ficha:<id>`.
|
||||
- **`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)
|
||||
|
|
@ -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<CampaignUser
|
|||
data.phone.length > 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,
|
||||
|
|
|
|||
|
|
@ -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:<id>`) 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))
|
||||
|
|
|
|||
|
|
@ -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:<id>`) 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<void> => acquireTextAdvisoryLocks(payload, req, contactPhoneLockKeys(phones))
|
||||
|
||||
export const acquireContactFichaLock = (
|
||||
payload: ContactPhonePayload,
|
||||
req: ContactPhoneRequest,
|
||||
id: number | string,
|
||||
): Promise<void> => acquireTextAdvisoryLocks(payload, req, [contactFichaLockKey(id)])
|
||||
|
|
|
|||
126
tests/int/contactPhoneAppendRace.int.spec.ts
Normal file
126
tests/int/contactPhoneAppendRace.int.spec.ts
Normal file
|
|
@ -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<void>((resolve) => {
|
||||
releaseFirstRead = resolve
|
||||
})
|
||||
let markFirstRead = () => {}
|
||||
const firstReadReached = new Promise<void>((resolve) => {
|
||||
markFirstRead = resolve
|
||||
})
|
||||
let markSecondRead = () => {}
|
||||
const secondReadDone = new Promise<void>((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<string, unknown>)
|
||||
: {}
|
||||
const dedupeOn =
|
||||
typeof where['phones.value'] === 'object'
|
||||
? (where['phones.value'] as Record<string, unknown>)['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())
|
||||
})
|
||||
})
|
||||
|
|
@ -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) }
|
||||
|
|
|
|||
Loading…
Reference in a new issue