Repository navigation
fix(links): fail health check when Kafka delivery is unavailable - #721
Tyagiquamar wants to merge 10 commits into
Conversation
|
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 configuration
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 |
|
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.
All reported issues were addressed across 3 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
|
superseded — see the newer check below. |
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
…ll assertions Co-authored-by: Cursor <cursoragent@cursor.com>
izadoesdev
left a comment
There was a problem hiding this comment.
The diagnosis in #719 is real, but the 503 is the wrong lever and it contradicts a deliberate existing design.
calculateLinkReadiness in apps/links/src/lib/health.ts already encodes the right policy:
const deliverySinkReady = services.clickhouse === "ok" || services.redpanda === "ok";
if (!(admissionReady && deliverySinkReady)) return { httpStatus: 503, status: "unavailable" };
const degraded = services.clickhouse === "error" || services.redpanda === "error";
return { httpStatus: 200, status: degraded ? "degraded" : "ok" };That || is intentional: Kafka down with ClickHouse up is degraded at HTTP 200, because delivery still works. Your own test asserts exactly that ("production sends still fall back to ClickHouse when Kafka is down"). The new short-circuit runs before this and returns 503 whenever didKafkaConnectFail(), so a Redpanda blip would take a links service that is still redirecting and still delivering out of rotation. That turns a past degradation into an outage, and Redpanda connectivity here has been flaky before.
The actual gap is narrower, in the probe's state mapping at apps/links/src/index.ts:158-174. It maps cooldown to error, but getProducerHealthState() only returns cooldown while Date.now() < nextReconnectAt. A connect failure that leaves nextReconnectAt unset or already past yields idle, which the probe maps to pending, and degraded only tests for error. So a failed Kafka connect can be reported as a fully healthy ok. That is the bug worth fixing.
Suggested shape: keep the loud rejection from refreshProducerConnection() and the kafka_health_connect_failed attribute, both of which are good, then make a failed probe map to error rather than pending so readiness reports degraded. Drop the 503 short-circuit and let calculateLinkReadiness stay the single place that decides HTTP status.
Unrelated to your change: the three red Vercel checks are the fork-preview authorization step, not builds.
|
Correction to my review: retract the second half. I claimed a failed Kafka connect can report as a healthy Every failure path in What stands unchanged is the objection to the 503. Given that Kafka-down already surfaces as |
…ss decide HTTP status Removes the health endpoint short-circuit that returned 503 whenever didKafkaConnectFail() was set. Kafka down with ClickHouse up is a degradation (HTTP 200), which calculateLinkReadiness already encodes. The loud rejection from refreshProducerConnection and the kafka_health_connect_failed attribute are unchanged. Also extends the links CI status poll past the 60s producer reconnect cooldown so one transient first-connect failure does not fail the job.
|
superseded — see the newer check below. |
|
Updated in 26c3802.
Validation: |
External pull requests cannot modify workflow files in this repo (tripwire protected-file rule). The links status poll extension is dropped from this PR; a maintainer can land it separately if wanted.
|
passed — @Tyagiquamar, that's cleared. good to merge. |
cleared — this change now passes tripwire's checks.
|
Updated in 5198eb4: withdrawn the
The links status-poll extension in the workflow (30 -> 90 iterations past the 60s reconnect cooldown) is a maintainer-side follow-up if you want it. Tests still green: |
|
Updated probe state mapping in apps/links/src/index.ts so that a failed Redpanda probe (probeResult.status === 'error') maps to 'error' rather than 'pending'. This ensures calculateLinkReadiness reports status: 'degraded' (HTTP 200) when Kafka connection fails while ClickHouse remains healthy, while preserving ClickHouse delivery fallback semantics. Verified with bun test src/lib/producer.test.ts src/lib/health.test.ts (27 tests passing). |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Architecture diagram
sequenceDiagram
participant CI as CI Health Check
participant Health as /health/status Endpoint
participant Probe as refreshProducerConnection()
participant Connect as connect()
participant Kafka as Kafka/Redpanda
participant Send as sendLinkVisit()
participant CH as ClickHouse
Note over CI,CH: Kafka Health Probe Flow
CI->>Health: GET /health/status
Health->>Probe: Trigger health probe
alt Kafka reachable
Probe->>Connect: connect(reportFailure=false)
Connect->>Kafka: Establish TLS connection
Kafka-->>Connect: Connected
Connect-->>Probe: true
Probe-->>Health: Probe success
Health-->>CI: 200 "ok"
else Kafka unreachable (TLS mismatch or broker down)
Probe->>Connect: connect(reportFailure=false)
Connect->>Kafka: Attempt TLS connection
Kafka-->>Connect: Connection failed
Connect->>Connect: Set kafkaConnectFailed=true
Connect-->>Probe: false
Probe-->>Health: Throw "Kafka health probe failed"
alt kafkaConnectFailed flag set
Health-->>CI: 503 "unavailable"
else Flag not set (first failure, broker unconfigured)
Health-->>CI: 200 "degraded"
end
end
Note over Send,CH: Production Send Flow (unchanged fallback)
Send->>Connect: connect(reportFailure=true)
alt Kafka available
Connect->>Kafka: Connect
Kafka-->>Connect: Success
Connect-->>Send: true
Send->>Kafka: Send visit event
else Kafka unavailable
Connect->>Kafka: Connect attempt
Kafka-->>Connect: Failure
Connect-->>Send: false (swallowed)
Send->>CH: Insert visit record
CH-->>Send: Acknowledged
Send-->>Client: true (fallback success)
end
Note over Connect: Concurrent health probe and send
alt Health probe in-flight while send arrives
Send->>Connect: Joins existing connectPromise
Probe-->>Connect: Fails
Connect-->>Send: false
Send->>CH: Fallback insert
CH-->>Send: Acknowledged
Send-->>Client: true
end
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
|
Links health checks attempt Kafka delivery and retain the degraded HTTP200 response when the healthy ClickHouse fallback handles a Kafka failure. Kafka SSL settings accept trimmed, case-insensitive false values consistently, and their tests clear inherited SSL environment flags. Codex assisted with the code, tests, validation and description repair. Docker Bun1.4.1 checks passed: 31 focused producer/health tests, Biome2.5.1, Links type checking and compiled Links build. |

Fixes #719.
Links health checks attempt Kafka delivery and retain the degraded HTTP200 response when the healthy ClickHouse fallback handles a Kafka failure. Kafka SSL settings accept trimmed, case-insensitive false values consistently, and their tests clear inherited SSL environment flags.
Codex assisted with the code, tests, validation and description repair. Docker Bun1.4.1 checks passed: 31 focused producer/health tests, Biome2.5.1, Links type checking and compiled Links build. The contributor confirmed review and manual verification under AI_POLICY.md before this update was published.