fix(links): fail health check when Kafka delivery is unavailable - #721
fix(links): fail health check when Kafka delivery is unavailable#721Tyagiquamar wants to merge 4 commits into
Conversation
![tripwire-sh[bot]](https://avatars.githubusercontent.com/in/4291944?s=60&v=4){kind=small}
|
superseded — see the newer check below. |
|
@Tyagiquamar is attempting to deploy a commit to the Databuddy OSS Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR makes Kafka health-probe connection failures reject and tightens the Links workflow gate to require fully healthy dependency status. Two blocking interactions remain:
Confidence Score: 3/5The PR is not safe to merge until production sends retain fallback behavior during concurrent health probes and the workflow provisions a Kafka listener compatible with the strict health gate. Two blocking failures remain: shared connection state can turn a health-probe rejection into a lost production visit, and the health-check workflow now requires a status its TLS/plaintext Kafka configuration cannot reach. Files Needing Attention: apps/links/src/lib/producer.ts, .github/workflows/health-check.yml Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
H[Health request] --> C[connect false]
S[Concurrent link visit] --> P[Reuse shared connectPromise]
C --> P
P --> F{Kafka connection}
F -->|Failure| R[Promise rejects]
R --> L[sendLinkVisit exits before ClickHouse fallback]
L --> X[Visit reported lost]
W[CI Links health gate] --> T[TLS Kafka client]
T --> Q[Plaintext Redpanda listener]
Q --> D[Readiness remains degraded]
D --> E[ok-only gate exhausts retries]
Reviews (1): Last reviewed commit: "fix(links): fail health check when Kafka..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Architecture diagram
sequenceDiagram
participant CI as GH Actions Health Gate
participant App as Links App
participant Probe as connect(reportFailure)
participant Kafka as Kafka/Redpanda
participant CH as ClickHouse
Note over CI,CH: Health Check Flow with New Failure Semantics
CI->>App: GET /health/status (poll every 1s, max 30)
App->>App: calculateLinkReadiness()
App->>Probe: refreshProducerConnection() -> connect(false)
alt Kafka unreachable (TLS mismatch in CI)
Probe->>Kafka: TLS handshake (ssl:true)
Kafka-->>Probe: REJECT: tls handshake failed
Probe->>Probe: Bookkeeping: set cooldown, kafka_connected:false, null producer
Probe-->>App: THROW error (NEW: no longer swallowed)
App-->>CI: 200 {"status":"error"}
CI->>CI: jq check .status == "ok"
CI-->>CI: FAIL gate (NEW: "degraded" no longer accepted)
else Kafka reachable
Probe->>Kafka: connect()
Kafka-->>Probe: OK
Probe-->>App: resolve true
App-->>CI: 200 {"status":"ok"}
CI->>CI: jq check .status == "ok"
CI-->>CI: PASS gate
end
Note over App,CH: Production Path (unchanged, still graceful fallback)
App->>Probe: sendLinkVisit() -> connect(true)
alt Kafka down (same unreachable scenario)
Probe->>Kafka: connect attempt
Kafka-->>Probe: REJECT
Probe->>Probe: set cooldown, kafka_connected:false
Probe-->>App: return false (no throw for production)
App->>CH: INSERT link_visit (direct fallback)
CH-->>App: success
else Kafka up
Probe->>Kafka: send message
Kafka-->>Probe: delivered
end
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
27139f5 to
4824956
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
superseded — see the newer check below. |
cleared — this change now passes tripwire's checks.
4824956 to
8767e94
Compare
|
superseded — see the newer check below. |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Confidence score: 3/5
- In
.github/workflows/health-check.yml, the strict.status == "ok"gate can leave CI failing permanently after one startup Kafka Connect failure becauseproducer.tspersistskafkaConnectFailed; add recovery or retry/reset handling before enforcing the final status.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/health-check.yml">
<violation number="1" location=".github/workflows/health-check.yml:761">
P2: The status gate now requires `.status == "ok"`, but a single failed Kafka connect during startup makes this CI job fail with no recovery. On any failed attempt, `producer.ts` sets the persistent `kafkaConnectFailed` flag and arms a 60s reconnect cooldown (`nextReconnectAt = Date.now() + 60000`); during cooldown every `connect()` returns false without trying, so `didKafkaConnectFail()` stays true and `/health/status` returns 503 `"unavailable"` (apps/links/src/index.ts). The poll loop here only retries for ~30s (`for i in {1..30}; sleep 1`), which is shorter than the 60s cooldown, so one transient failure at startup (e.g., Redpanda briefly unreachable during container bring-up) permanently fails the job even though the earlier looser `or .status == "degraded"` gate accepted it. Consider extending the wait past the 60s cooldown (e.g. 90 iterations) so a transient first-connect failure isn't fatal, or confirm the single-attempt behavior is intended.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| STATUS_BODY=$(curl -sS http://localhost:2500/health/status) | ||
| echo "Links /health/status: $STATUS_BODY" | ||
| if echo "$STATUS_BODY" | jq -e '.status == "ok" or .status == "degraded"' > /dev/null; then | ||
| if echo "$STATUS_BODY" | jq -e '.status == "ok"' > /dev/null; then |
There was a problem hiding this comment.
P2: The status gate now requires .status == "ok", but a single failed Kafka connect during startup makes this CI job fail with no recovery. On any failed attempt, producer.ts sets the persistent kafkaConnectFailed flag and arms a 60s reconnect cooldown (nextReconnectAt = Date.now() + 60000); during cooldown every connect() returns false without trying, so didKafkaConnectFail() stays true and /health/status returns 503 "unavailable" (apps/links/src/index.ts). The poll loop here only retries for ~30s (for i in {1..30}; sleep 1), which is shorter than the 60s cooldown, so one transient failure at startup (e.g., Redpanda briefly unreachable during container bring-up) permanently fails the job even though the earlier looser or .status == "degraded" gate accepted it. Consider extending the wait past the 60s cooldown (e.g. 90 iterations) so a transient first-connect failure isn't fatal, or confirm the single-attempt behavior is intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/health-check.yml, line 761:
<comment>The status gate now requires `.status == "ok"`, but a single failed Kafka connect during startup makes this CI job fail with no recovery. On any failed attempt, `producer.ts` sets the persistent `kafkaConnectFailed` flag and arms a 60s reconnect cooldown (`nextReconnectAt = Date.now() + 60000`); during cooldown every `connect()` returns false without trying, so `didKafkaConnectFail()` stays true and `/health/status` returns 503 `"unavailable"` (apps/links/src/index.ts). The poll loop here only retries for ~30s (`for i in {1..30}; sleep 1`), which is shorter than the 60s cooldown, so one transient failure at startup (e.g., Redpanda briefly unreachable during container bring-up) permanently fails the job even though the earlier looser `or .status == "degraded"` gate accepted it. Consider extending the wait past the 60s cooldown (e.g. 90 iterations) so a transient first-connect failure isn't fatal, or confirm the single-attempt behavior is intended.</comment>
<file context>
@@ -757,7 +758,7 @@ jobs:
STATUS_BODY=$(curl -sS http://localhost:2500/health/status)
echo "Links /health/status: $STATUS_BODY"
- if echo "$STATUS_BODY" | jq -e '.status == "ok" or .status == "degraded"' > /dev/null; then
+ if echo "$STATUS_BODY" | jq -e '.status == "ok"' > /dev/null; then
echo "Links dependency health is valid"
break
</file context>
|
passed — @Tyagiquamar, that's cleared. good to merge. |
cleared — this change now passes tripwire's checks.
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Confidence score: 2/5
- In
.github/workflows/health-check.yml, removingREDPANDA_SSL=falsewhile starting Redpanda without TLS makes thelinks-health-checkjob deterministically fail; restore the plaintext setting or configure TLS consistently. - In
.github/workflows/health-check.yml, allowing adegradedstatus can hide Kafka delivery failures because the producer enters cooldown while the failure flag remains false, allowing/healthto pass despite unavailable delivery; require a healthy status or explicitly detect Kafka errors.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/health-check.yml">
<violation number="1">
P2: Removing `REDPANDA_SSL=false` while the CI Redpanda is still started in plaintext (`redpanda start --mode dev-container`, no TLS config) makes the `links-health-check` job deterministically fail. With the env var gone, `producer.ts` defaults `useSsl` to `true`, so KafkaJS performs a TLS handshake against a plaintext listener, which always fails, leaving `kafkaConnectFailed=true` forever. `/health/status` then always returns `503 {"status":"unavailable"}` (the `didKafkaConnectFail()` early return short-circuits before the readiness path that could produce "degraded"), so the retry loop below never sees `ok`/`degraded` and exits 1 after 30 attempts. The "degraded" widening does not help, since "degraded" requires `kafkaConnectFailed === false`. The result is that this job cannot pass its own CI in the merged state; it will stay red until the acknowledged CI-TLS follow-up lands. Land the CI-Redpanda TLS config in this same PR, or keep `REDPANDA_SSL=false` (a genuine Redpanda outage still sets the flag and fails the check loudly, so the goal is preserved).</violation>
<violation number="2">
P2: Accepting `.status == "degraded"` lets the health check pass when Kafka delivery is unavailable. After a Kafka send failure the producer enters cooldown (redpanda `error`), `didKafkaConnectFail()` stays false, and `/health/status` returns `degraded` with HTTP 200. This masks exactly the Kafka delivery outage the PR intends to fail loudly on, once CI Redpanda TLS is enabled and connect() succeeds. Keep the check strictly on `"ok"`.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
1 participant
Root cause
f16085061hardcodedssl: trueon the links Kafka producer, but.github/workflows/health-check.ymlstill stands up a plaintext Redpanda(
--advertise-kafka-addr localhost:9092). The TLS client cannot connect,candidate.connect()throws, and the failure was swallowed: the health pathset
kafka_health_connect_failed: true, nulled the producer, and resolvednormally.
Why CI was falsely green
The swallowed failure leaves the producer in
cooldown, so/health/statusreported
redpanda: "error". With ClickHouse healthy,calculateLinkReadinessreturns
{ 200, "degraded" }— which the workflow gate accepts. Every PRpassed without the Kafka path ever being exercised. (The throw alone would
not suffice either: startup warmup arms the 60s reconnect cooldown, so health
probes during CI are suppressed and never attempt a connection.)
Exact fix (code only, no workflow changes)
apps/links/src/lib/producer.ts: the health probe (refreshProducerConnection)throws
Kafka health probe failedwhen no connection is established, and apersistent
kafkaConnectFailedflag is set on any failed attempt / clearedon any success — so a known outage stays loud across the reconnect-
suppression window. The throw lives in
refreshProducerConnection, not inthe shared
connect(), so a health-owned in-flight attempt can never rejecta concurrent
sendLinkVisit()past its ClickHouse fallback. Productionsend/warmup fallback behavior unchanged; production TLS semantics unchanged;
no
sslflag reintroduced.apps/links/src/index.ts:/health/statusreturns{ 503, "unavailable" }whiledidKafkaConnectFail()is set, instead of adegraded200 that CI accepts. Broker-unconfigured (disabled) behavioris untouched.
Note: with the plaintext CI Redpanda still in place, this job will now fail
loudly until CI gets a TLS listener — that visible failure is the point of
this fix (issue option 3). TLS setup for CI Redpanda is left as a follow-up;
the same gap exists for basket/uptime and is out of scope here.
Regression coverage
apps/links/src/lib/producer.test.ts→health probe failures (issue #719):without the refresh-local throw; guards the shared-promise fallback path
flagged by Greptile/cubic review)
Validation
bun test apps/links/src/lib/producer.test.ts apps/links/src/lib/health.test.ts→ 19 pass, 0 failbunx biome checkon changed files → cleanapps/linkssuite needselysiainstalled;tsc --noEmitneeds bun typesFixes #719
AI disclosure (per AI_POLICY.md): implemented with OpenCode/Muse Spark
(agent-assisted), human-reviewed and human-verified — reproduction, tests,
and validation above were all executed on this machine.