fix: harden config export/distribution pipeline (JITSU-139 postmortem) by absorbb · Pull Request #1433 · jitsucom/jitsu

absorbb · 2026-07-30T10:24:18Z

Summary

Postmortem follow-ups from JITSU-139 (bulker-connections repository breakage): a destination row with null config crashed the streamed /api/admin/export/bulker-connections response mid-stream, producing truncated JSON with HTTP 200; config-keeper cached and propagated it, all bulkers failed to parse it, and a routine pod termination turned into a 10.5-hour processing stop for 1/16 of connection topics — all without alerts.

This PR fixes each defense-in-depth layer:

console — one malformed entity can no longer poison a whole /api/admin/export/* export: each entity's JSON is built before anything is written, malformed entities are skipped and logged with the System error: marker (incl. the clickhouseSettings null-deref that triggered the incident). If an export still fails mid-stream, the socket is destroyed so consumers see an aborted transfer instead of valid-looking truncated JSON with status 200.
config-keeper — repository payloads are validated with json.Valid before caching; invalid payloads are rejected and the previous good data keeps being served (p.js is exempt — it's JavaScript, not JSON).
bulker/jitsubase — Fatal/Fatalf now carry the System error: marker, so every failure to init/start (e.g. "Cannot serve without repository") is alertable.
rotor (core-functions-lib) — store refresh failures log Error refreshing repository <name> at error level, matching the Go-side wording, so a single Datadog monitor covers both stacks; init-without-cache failure (a failure to start) carries the System error: marker.

Repository refresh errors deliberately do not carry the System error: marker — they are recoverable (consumers keep serving last-known-good config) and alert via a dedicated repository-refresh monitor instead. Only failures to init/start are system errors.

The matching Datadog monitors + workload-health prober land in a companion jitsu-cloud-infra PR (jitsucom/jitsu-cloud-infra#71).

Testing

go build + go vet on jitsubase, config-keeper, bulkerapp
tsc --noEmit clean for console and core-functions-lib
console unit tests: 30/30 pass
prettier applied

🤖 Generated with Claude Code

jitsu-code-review

Reviewed the full diff range, including the bulker/config-keeper logging changes and the console export hardening. I like the direction overall, but I spotted two correctness risks around per-entity error handling in streamed exports (not blocking, but worth addressing).

jitsu-code-review · 2026-07-30T10:27:22Z

+          writer.write(payload);
+          needComma = true;
+        }
+      } catch (e) {


Potential regression: this catch wraps both transformation and writer.write(...). If a write starts failing (e.g. client disconnected / stream error), we just log and continue scanning all remaining DB rows. Should write failures be rethrown so the export aborts immediately instead of doing expensive work on an unwritable response?

Fixed in e307cd5 — same restructuring across all export loops: build+serialize inside the guard, write outside it, so client disconnects abort the export instead of being logged per-entity while the scan continues.

jitsu-code-review · 2026-07-30T10:27:23Z

+        return exportSyncEntity({ data, from, id, to, updatedAt, workspace });
+      } catch (e) {
+        logExportEntityError("syncs", id, e);
        return [];


Question on sync correctness: returning [] here means a single exception while materializing one sync removes it from this snapshot. For syncctl consumers that reconcile desired state from this export, could that be interpreted as deletion and tear down the CronJob? If yes, should we fail the whole sync export instead of silently omitting the entity?

Fixed in e307cd5 — syncs now fails the whole export on any per-entity error instead of omitting the sync. Omission reads as deletion to syncctl's reconciler, exactly as you describe; failing closed keeps the last known good snapshot on the consumer side.

jitsu-code-review

Reviewed the changes in range 079fd5d..245f01a across config-keeper, shared logging, in-memory store refresh logging, and console export streaming.\n\nI did not find additional actionable bugs/regressions beyond the currently open unresolved review threads already on this PR.

jitsu-code-review

Reviewed the range 079fd5da..245f01a0 across config-keeper, logging, in-memory store refresh handling, and the console admin export hardening.

Main findings:

syncs export currently drops individual sync rows on per-entity exceptions, which can be interpreted by syncctl as deletion during reconciliation.
Per-entity try/catch blocks also swallow stream write failures, so export processing can continue after the response is no longer writable.

jitsu-code-review · 2026-07-30T10:45:58Z

+        return exportSyncEntity({ data, from, id, to, updatedAt, workspace });
+      } catch (e) {
+        logExportEntityError("syncs", id, e);
        return [];


Possible correctness issue: this catch returns [], so one exception while materializing one sync removes that sync from the snapshot. syncctl deletes CronJobs missing from desired state during reconcile, so this can unintentionally tear down a healthy sync because of a transient/malformed row. Should this fail the whole syncs export instead of omitting the entity?

Fixed in e307cd5 — the syncs export now fails closed. Any per-entity failure (materialization or serialization) rethrows and fails the whole export, so syncctl never sees a snapshot with a healthy sync missing; consumers keep their last known good state. Stale is safe, wrong is not — same principle as the incident postmortem this PR follows up on.

jitsu-code-review · 2026-07-30T10:45:58Z

+        if (needComma) {
+          writer.write(",");
+        }
+        writer.write(payload);


Potential regression: this catch also swallows writer.write(...) failures (e.g. disconnect / stream error) and then continues scanning remaining DB rows. That keeps doing expensive work for a response that can no longer be delivered, and the outer exp.data catch at line 1221 never sees the write failure. Can we rethrow write errors and only skip entity-construction/serialization failures?

Fixed in e307cd5. Writes moved outside every per-entity try/catch (all 10 sites, not just this one): the guard now covers only entity materialization and JSON.stringify, and writer.write failures propagate to the outer export error handling, aborting immediately instead of scanning the remaining rows for a dead stream.

…d of failing mid-stream One bad row (e.g. a destination with null config reading clickhouseSettings) crashed the streamed /api/admin/export/* response mid-stream, producing a truncated HTTP 200 that every consumer cached or choked on (JITSU-139). - build each entity's JSON before writing, wrap per-entity work in try/catch: malformed entities are skipped and logged with the 'System error:' marker - guard clickhouseSettings parsing with a typeof check - if an export still fails mid-stream, destroy the socket instead of ending the response, so consumers see an aborted transfer, not valid-looking JSON Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ystem errors Failure-to-start (Fatalf, e.g. 'Cannot serve without repository') and repository refresh failures now carry the 'System error:' log marker used for unified log-based alerting across Go and Node components (JITSU-139). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

config-keeper cached whatever bytes the console export returned. During JITSU-139 the export failed mid-stream and produced truncated JSON with HTTP 200; cfgkpr cached it and served it to every consumer for hours. Reject payloads that are not complete valid JSON — the repository keeps serving the previous good data and the refresh failure is logged as a system error. p.js (JavaScript, not JSON) is exempt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…marker inmem-store refresh failures now log 'System error: Error refreshing repository <name>' at error level, matching the Go-side message from jitsubase appbase, so one Datadog monitor covers both stacks (JITSU-139). Initialize-with-no-cache failure carries the marker too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Refresh failures are recoverable — consumers keep serving last-known-good config. Only failure to init/start an app carries the 'System error:' marker (Fatalf and store-init-without-cache keep it). Refresh errors stay at error level with unified 'Error refreshing repository' wording across Go and Node for the dedicated Datadog monitor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…osed Two review findings on the skip-and-log hardening: Writes no longer live inside the per-entity try/catch. A failing writer.write means the response is gone, and swallowing it kept the export scanning every remaining DB row for a stream nobody would ever read — while the outer error handling never learned about it. Each site now materializes and serializes inside the guard, and writes outside it, so stream failures abort the export immediately. One malformed row still skips only that row. The syncs export now fails closed instead of omitting entities. syncctl reconciles desired state from this export and deletes CronJobs that are absent from it, so skipping a sync whose row failed to materialize (or serialize) would tear down a healthy sync. Any per-entity failure fails the whole export; consumers keep their last known good snapshot — stale is safe, wrong is not. Also rebased onto current newjitsu: the branch was based on 079fd5d, whose export changes were since reverted and re-landed as 3d5ef90. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

jitsu-code-review

Reviewed the diff across , , , and export streaming logic.

I specifically checked the new JSON payload validation, system-error marker changes, and the export-path error handling adjustments (including sync fail-closed behavior and write-failure propagation). I also checked existing review threads so previously-discussed concerns weren’t re-raised.

No additional actionable bugs, security issues, or correctness regressions found in the current patch set.

jitsu-code-review

Reviewed the diff across bulker/config-keeper, bulker/jitsubase/logging, libs/core-functions-lib, and webapps/console export streaming logic.

I specifically checked the new JSON payload validation, system-error marker changes, and the export-path error handling adjustments (including sync fail-closed behavior and write-failure propagation). I also checked existing review threads so previously-discussed concerns weren’t re-raised.

No additional actionable bugs, security issues, or correctness regressions found in the current patch set.

jitsu-code-review Bot previously approved these changes Jul 30, 2026

View reviewed changes

absorbb dismissed jitsu-code-review[bot]’s stale review via 245f01a July 30, 2026 10:39

jitsu-code-review Bot approved these changes Jul 30, 2026

View reviewed changes

jitsu-code-review Bot previously approved these changes Jul 30, 2026

View reviewed changes

absorbb and others added 6 commits August 3, 2026 19:01

absorbb dismissed jitsu-code-review[bot]’s stale review via e307cd5 August 3, 2026 15:06

absorbb force-pushed the fix/jitsu-139-export-resilience branch from 245f01a to e307cd5 Compare August 3, 2026 15:06

jitsu-code-review Bot approved these changes Aug 3, 2026

View reviewed changes

absorbb mentioned this pull request Aug 4, 2026

JITSU-158: explicit typing + unsafe-any lint gate on config exports #1441

Open

Uh oh!

Conversation

absorbb commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

jitsu-code-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

jitsu-code-review Bot Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

absorbb Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

jitsu-code-review Bot Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

absorbb Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

jitsu-code-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

jitsu-code-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

jitsu-code-review Bot Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

absorbb Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

jitsu-code-review Bot Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

absorbb Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

jitsu-code-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

jitsu-code-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

absorbb commented Jul 30, 2026 •

edited

Loading