fix(segments): surface ClickHouse errors in segment cache (EVO-1924)#100
Conversation
There was a problem hiding this comment.
Sorry @daniloleonecarneiro, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
dpaes
left a comment
There was a problem hiding this comment.
Changes requested — EVO-1924
The re-throw conversion (3 catches: return []/false → throw) is syntactically clean and does touch the file named in the card — but that file, src/modules/segments/services/segment-clickhouse-cache.service.ts, is orphaned dead code: it is imported nowhere in src, is not a provider, and is not dynamically loaded. So the change has zero runtime effect, and the real segment-read path still swallows ClickHouse errors (the card's actual impact is unaddressed). Same pattern as EVO-1901.
What's missing: apply the error-propagation on the segment cache/read service actually used at runtime, with a test covering that live path. If this file is in fact loaded somewhere I missed, point me to it and I'll revisit.
…1924) segment-clickhouse-cache.service.ts is dead: grep -rn 'segment-clickhouse-cache|SegmentClickHouseCacheService' src returns zero hits - never imported, not a provider, no dynamic load, so its swallowing catches (getContactSegmentAssignments/isContactInSegment/getSegmentContacts) have zero runtime effect. The previous fix re-threw inside this orphan, which the review correctly rejected. The live ClickHouse contact-read swallow is entirely in segment-computation.service.ts (getSegmentContacts/getContactSegments/isContactInSegment), already fixed to log+propagate by EVO-1901. The live SegmentCacheService (cache/services/segment-cache.service.ts) is a Redis entity cache with no ClickHouse contact-assignment reads, and the live journey segment trigger keys off segment_entered/segment_exited events, not a membership query - so there is no separate live cache-layer swallow to fix. Remove the dead duplicate instead of fabricating a fix; the runtime defect is covered by EVO-1901, which supersedes this card.
0aa69c2 to
1c907e7
Compare
Correção do review (re-trabalho EVO-1924)O review estava certo: o re-throw foi aplicado em A classe nunca é importada, não é provider em nenhum módulo, não tem load dinâmico → os catches que engolem ( Onde está o swallow VIVOInvestiguei o caminho vivo de leitura de assignment no ClickHouse:
Ou seja, o órfão não tem contraparte viva; o defeito real de runtime é coberto pela EVO-1901. O que mudou
RecomendaçãoEste card é superado pela EVO-1901 quanto à correção de runtime. Esta PR fica como remoção de código morto. Rebase em |
dpaes
left a comment
There was a problem hiding this comment.
Hold — sequencing, not a code problem. The deletion itself is correct and pre-verified: segment-clickhouse-cache.service.ts is orphan (0 refs; not a provider in segments.module.ts; the 10 imports of the colliding SegmentCacheService name all resolve to cache/services/segment-cache.service.ts), so removing it is the right call and build-safe.
But merging this standalone would imply D12/N9 is resolved while the live swallow still persists on develop at segment-computation.service.ts:283/308/338 (return []/false). That real fix is EVO-1901/#86, which I just sent back to Todo (two requirements). So this PR is coupled: it should land with/after #86, not before.
Two options: keep #100 open and I'll merge it right after #86 is re-approved, or just fold this 1-file deletion into #86 so the dead-code removal and the live fix ship together. Your call. Card stays In Review.
dpaes
left a comment
There was a problem hiding this comment.
✅ Approved — EVO-1924 (evo-flow-community#100, base develop)
Clean dead-code removal. Verified segment-clickhouse-cache.service.ts is an orphan: zero imports, not a provider in segments.module.ts (the live SegmentCacheService resolves to cache/services/segment-cache.service.ts), so the error-swallow it contained had zero runtime effect. Branch CLEAN; deletion is build-safe (0 external refs).
Scope note (important): this PR closes EVO-1924's literal subject — that specific orphan file — by deleting it. It is NOT the fix for the live ClickHouse error-swallow (D12/N9): that lives in segment-computation.service.ts (return [] / false at ~283/306/338) and is owned by EVO-1901 / #86, which is still OPEN/unmerged. Merging this does not change runtime behavior and makes no claim about that file.
Minor: the PR body still describes a "log + throw" edit rather than the deletion it actually performs — worth trimming, no code impact.
Approving and squash-merging as the EVO-1924 cleanup.
Resumo
Follow-up do EVO-1901 (D12 + N9). Aquele card fez 3 catches de
segment-computation.service.tspropagarem erro do ClickHouse em vez de retornar vazio silencioso.segment-clickhouse-cache.service.tstinha catches análogos (return[]/false) que ficaram fora do escopo — mesma classe N9: query inválida ou ClickHouse indisponível virava "sem contatos / não pertence" silencioso, mascarando a falha.Este PR aplica o MESMO padrão (log ERROR +
throw error, com comentário explicativo) nos 3 catches de leitura.Catches alterados (read-path → agora propagam)
getContactSegmentAssignments— antes retornava{ assignments: [], fromCache: false }; agora propaga.isContactInSegment— antes retornavafalse; agora propaga.getSegmentContacts— antes retornava{ contactIds: [], total: 0, fromCache: false }; agora propaga.Catches preservados (já propagavam / não são leitura de segmento)
updateSegmentAssignment,batchUpdateSegmentAssignments,invalidateSegmentCache— write/command paths, já faziamthrow. Inalterados.Não há caminho de "cache miss legítimo" nos catches alterados: um miss real retorna array vazio sem exceção; o catch só dispara em erro de ClickHouse de fato.
Testes
jest src/modules/segments→ 10 passed, 3 suites.tsc -p tsconfig.json --noEmit→ exit 0, sem erros.EVO-1924