Skip to content

fix(links): fail health check when Kafka delivery is unavailable - #721

Open
Tyagiquamar wants to merge 10 commits into
databuddy-analytics:mainfrom
Tyagiquamar:fix-links-health-fail-kafka-719
Open

Tyagiquamar wants to merge 10 commits into
databuddy-analytics:mainfrom
Tyagiquamar:fix-links-health-fail-kafka-719

Conversation

@Tyagiquamar

@Tyagiquamar Tyagiquamar commented Sep 4, 2026 •

Copy link
Copy Markdown

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.

tripwire-sh[bot]
tripwire-sh Bot previously requested changes Sep 4, 2026

@tripwire-sh tripwire-sh Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocked — it touches a protected workflow file.

@tripwire-sh

tripwire-sh Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

blocked — @Tyagiquamar, this can't merge yet.

it touches a protected workflow file

superseded — see the newer check below.

@vercel

vercel Bot commented Sep 4, 2026

Copy link
Copy Markdown

@Tyagiquamar is attempting to deploy a commit to the Databuddy OSS Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: b24e2473-9f31-460e-913f-a28907041a73

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The behavioral fix appears sound, but the explicit repository requirement prohibiting postfix non-null assertions must be satisfied before merging.

Findings

  1. P2 Non-null assertion violates rule ▶

Summary

This PR makes Links dependency health fail loudly after a configured Kafka connection attempt fails while preserving direct ClickHouse fallback for production sends.

  • Persists Kafka connection-failure state until a later successful connection.
  • Returns HTTP 503 from /health/status while that known outage remains active.
  • Keeps concurrent sends on the ClickHouse fallback when a health-owned Kafka attempt fails.
  • Adds regression coverage for outages, recovery, concurrency, shutdown, and TLS configuration.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  H["/health/status"] --> R["refreshProducerConnection()"]
  R --> C["Shared Kafka connect attempt"]
  C -->|Success| OK["Clear failure flag"]
  C -->|Failure or cooldown after known failure| F["Keep failure flag set"]
  OK --> READY["Calculate normal readiness"]
  F --> U["503 unavailable"]

  S["sendLinkVisit()"] --> C
  C -->|Unavailable| CH["Persist directly to ClickHouse"]
  C -->|Available| K["Send through Kafka"]
Loading

Reviews (2) · Last reviewed commit: "fix(links): set kafka failure flag only ..."

Comment thread apps/links/src/lib/producer.ts Outdated
Comment thread .github/workflows/health-check.yml Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Loading

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread apps/links/src/lib/producer.ts Outdated
Comment thread .github/workflows/health-check.yml Outdated
Comment thread apps/links/src/lib/producer.ts Outdated
@Tyagiquamar
Tyagiquamar force-pushed the fix-links-health-fail-kafka-719 branch from 27139f5 to 4824956 Compare September 4, 2026 12:25
@vercel

vercel Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated
dashboard Skipped Skipped Oct 9, 2026 3:18pm UTC
documentation Skipped Skipped Oct 9, 2026 3:18pm UTC

@tripwire-sh

tripwire-sh Bot commented Sep 4, 2026 •

Copy link
Copy Markdown

passed — @Tyagiquamar, that's cleared. good to merge.

superseded — see the newer check below.

@vercel
vercel Bot temporarily deployed to Preview – documentation September 4, 2026 12:25 Inactive
@vercel
vercel Bot temporarily deployed to Preview – dashboard September 4, 2026 12:25 Inactive
@tripwire-sh
tripwire-sh Bot dismissed their stale review September 4, 2026 12:25

cleared — this change now passes tripwire's checks.

@Tyagiquamar
Tyagiquamar force-pushed the fix-links-health-fail-kafka-719 branch from 4824956 to 8767e94 Compare September 4, 2026 12:28
@vercel
vercel Bot temporarily deployed to Preview – dashboard September 4, 2026 12:29 Inactive
@vercel
vercel Bot temporarily deployed to Preview – documentation September 4, 2026 12:29 Inactive
tripwire-sh[bot]
tripwire-sh Bot previously requested changes Sep 5, 2026

@tripwire-sh tripwire-sh Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocked — it touches a protected workflow file.

@tripwire-sh

tripwire-sh Bot commented Sep 5, 2026 •

Copy link
Copy Markdown

blocked — @Tyagiquamar, the last push brought something back.

it touches a protected workflow file

superseded — see the newer check below.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread .github/workflows/health-check.yml Outdated
Comment thread apps/links/src/lib/producer.ts Outdated
@vercel
vercel Bot temporarily deployed to Preview – dashboard September 5, 2026 05:19 Inactive
@vercel
vercel Bot temporarily deployed to Preview – documentation September 5, 2026 05:19 Inactive
@tripwire-sh

tripwire-sh Bot commented Sep 5, 2026 •

Copy link
Copy Markdown

passed — @Tyagiquamar, that's cleared. good to merge.

superseded — see the newer check below.

@tripwire-sh
tripwire-sh Bot dismissed their stale review September 5, 2026 05:19

cleared — this change now passes tripwire's checks.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 1 file (changes from recent commits).

Confidence score: 2/5

  • In .github/workflows/health-check.yml, removing REDPANDA_SSL=false while starting Redpanda without TLS makes the links-health-check job deterministically fail; restore the plaintext setting or configure TLS consistently.
  • In .github/workflows/health-check.yml, allowing a degraded status can hide Kafka delivery failures because the producer enters cooldown while the failure flag remains false, allowing /health to 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

@vercel
vercel Bot temporarily deployed to Preview – documentation September 5, 2026 17:28 Inactive
@izadoesdev

Copy link
Copy Markdown
Member

@greptile review

Please review current head 62f865d for correctness and security as part of the requested review of all open PRs. Recheck earlier findings against this head and distinguish unresolved issues from already-fixed feedback.

Comment thread apps/links/src/lib/producer.test.ts Outdated
…ll assertions

Co-authored-by: Cursor <cursoragent@cursor.com>
@vercel
vercel Bot temporarily deployed to Preview – documentation September 22, 2026 07:39 Inactive
@vercel
vercel Bot temporarily deployed to Preview – dashboard September 22, 2026 07:39 Inactive

@izadoesdev izadoesdev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@izadoesdev

Copy link
Copy Markdown
Member

Correction to my review: retract the second half. I claimed a failed Kafka connect can report as a healthy ok via an idle state. That is wrong, and I checked connect() properly this time.

Every failure path in apps/links/src/lib/producer.ts:138-155 sets nextReconnectAt = Date.now() + reconnectCooldownMs before returning false, so getProducerHealthState() returns cooldown, the probe maps cooldown to error, and calculateLinkReadiness already reports degraded. The cooldown early return at line 88 keeps it in cooldown too. idle after an awaited refreshProducerConnection() is only reachable during shutdown. So there is no hidden healthy-on-failure bug, and nothing extra to fix there.

What stands unchanged is the objection to the 503. deliverySinkReady = clickhouse === "ok" || redpanda === "ok" is deliberate, your own test asserts sends still fall back to ClickHouse, and the short-circuit would return 503 for a service that is still redirecting and still delivering.

Given that Kafka-down already surfaces as degraded at HTTP 200, the visibility half of #719 looks already covered. The parts of this PR I would keep are the loud rejection from refreshProducerConnection() and the kafka_health_connect_failed attribute, since a silently-swallowed boolean is worth making explicit. Sorry for the noise on the rest.

@vercel
vercel Bot temporarily deployed to Preview – documentation September 27, 2026 20:47 Inactive
@vercel
vercel Bot temporarily deployed to Preview – dashboard September 27, 2026 20:47 Inactive
…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.
tripwire-sh[bot]
tripwire-sh Bot previously requested changes Sep 28, 2026

@tripwire-sh tripwire-sh Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocked — it touches a protected workflow file.

@tripwire-sh

tripwire-sh Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

blocked — @Tyagiquamar, the last push brought something back.

it touches a protected workflow file

superseded — see the newer check below.

@Tyagiquamar

Copy link
Copy Markdown
Author

Updated in 26c3802.

  • Removed the 503 short-circuit based on didKafkaConnectFail() so calculateLinkReadiness is again the single place deciding HTTP status. Kafka down with ClickHouse up now reports degraded at 200 as designed.
  • Kept the loud rejection from refreshProducerConnection() and the kafka_health_connect_failed attribute unchanged.
  • Left the probe state mapping in apps/links/src/index.ts untouched, per your correction (every failure path already sets nextReconnectAt, so cooldown maps to error).
  • Extended the Links status poll in .github/workflows/health-check.yml from 30 to 90 iterations so the gate outlives the 60s reconnect cooldown.

Validation: bun test apps/links/src/lib/producer.test.ts apps/links/src/lib/health.test.ts (27 pass), bun run --cwd apps/links check-types (tsc clean), biome check apps/links/src/index.ts clean. Ran with host bun 1.3.14 rather than the oven/bun Docker image this time.

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.
@vercel
vercel Bot temporarily deployed to Preview – documentation September 28, 2026 23:35 Inactive
@vercel
vercel Bot temporarily deployed to Preview – dashboard September 28, 2026 23:35 Inactive
@tripwire-sh

tripwire-sh Bot commented Sep 28, 2026

Copy link
Copy Markdown

passed — @Tyagiquamar, that's cleared. good to merge.

View on Tripwire

@tripwire-sh
tripwire-sh Bot dismissed their stale review September 28, 2026 23:35

cleared — this change now passes tripwire's checks.

@Tyagiquamar

Copy link
Copy Markdown
Author

Updated in 5198eb4: withdrawn the .github/workflows/health-check.yml change from this PR (protected workflow file per tripwire). The PR now only touches apps/links/src/:

  • 503 short-circuit removed; calculateLinkReadiness is again the single HTTP-status decision point
  • loud rejection from refreshProducerConnection() and the kafka_health_connect_failed attribute kept
  • probe state mapping untouched per your correction

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: bun test apps/links/src/lib/producer.test.ts apps/links/src/lib/health.test.ts (27 pass), check-types and biome clean.

@vercel
vercel Bot temporarily deployed to Preview – documentation October 2, 2026 02:44 Inactive
@vercel
vercel Bot temporarily deployed to Preview – dashboard October 2, 2026 02:44 Inactive
@Tyagiquamar

Copy link
Copy Markdown
Author

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).

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Loading

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread apps/links/src/lib/producer.test.ts Outdated
Comment thread apps/links/src/lib/producer.ts Outdated
@Tyagiquamar

Copy link
Copy Markdown
Author

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.

@vercel
vercel Bot temporarily deployed to Preview – documentation October 9, 2026 15:18 Inactive
@vercel
vercel Bot temporarily deployed to Preview – dashboard October 9, 2026 15:18 Inactive

This branch was previously deployed

2 inactive deployments
Preview – dashboard — f360a787 Deployed Oct 9, 2026 by vercel[bot]
Preview – documentation — f360a787 Deployed Oct 9, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

links health check no longer exercises Kafka delivery

2 participants