fix: harden config export/distribution pipeline (JITSU-139 postmortem) - #1433
Conversation
![jitsu-code-review[bot]](https://avatars.githubusercontent.com/in/3391383?s=60&v=4){kind=small}
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Reviewed the range 079fd5da..245f01a0 across config-keeper, logging, in-memory store refresh handling, and the console admin export hardening.
Main findings:
syncsexport currently drops individual sync rows on per-entity exceptions, which can be interpreted by syncctl as deletion during reconciliation.- Per-entity
try/catchblocks also swallow stream write failures, so export processing can continue after the response is no longer writable.
245f01a to
e307cd5
Compare
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…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>
e307cd5 to
735ee4b
Compare
There was a problem hiding this comment.
Reviewed the PR diff for bulker/config-keeper/app.go, bulker/config-keeper/router.go, bulker/jitsubase/logging/global_logger.go, and libs/core-functions-lib/src/lib/inmem-store.ts.
Checked correctness and regression risk around repository payload validation, repository handler initialization path, startup/system-error marker behavior, and in-memory store refresh/init failure handling.
No actionable bugs or security regressions found in the current change set.
There was a problem hiding this comment.
Reviewed the changes in bulker/config-keeper, bulker/jitsubase/logging, and libs/core-functions-lib.
The JSON validation in config-keeper looks like a solid safeguard against caching truncated payloads, and the startup-failure/system-error logging direction is clear.
I left one inline comment about a potential formatting/alerting regression in Fatal(...) logging.
Fatal prepends the marker as an operand and lets logrus join it, but logrus
renders Fatal with fmt.Sprint, which only inserts spaces between operands when
neither is a string. The marker was glued to the message:
Fatal("cannot start") -> "System error:cannot start"
Fatalf("%s", "cannot start") -> "System error: cannot start"
Alerting matches the marker verbatim, so only the Fatalf form would have
matched. Multiple operands ran together for the same reason:
Fatal("cannot start", "boom") -> "System error:cannot startboom"
Switched to log.Fatalln, whose sprintlnn helper is Sprintln minus the trailing
newline and so joins every operand with a single space. Warn just above already
uses the ln variant for this reason.
Latent for now - nothing calls logging.Fatal today, only Fatalf (3 sites) - but
it is exported, and the next caller would have produced an unmatchable marker
with nothing to catch it. Added tests that assert both forms agree and that
operands stay separated; both fail against the previous implementation with
exactly the strings above.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the PR diff for config-keeper JSON payload validation, jitsubase fatal marker handling, and core-functions-lib repository refresh/init logging changes.
I also checked existing review threads (including outdated ones) and did not find new unresolved correctness, security, or user-visible regression issues in the current patch.
1 participant
Summary
Postmortem follow-ups from JITSU-139 (bulker-connections repository breakage): a destination row with null config crashed the streamed
/api/admin/export/bulker-connectionsresponse 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 the layers downstream of the console:
json.Validbefore caching; invalid payloads are rejected and the previous good data keeps being served (p.jsis exempt — it's JavaScript, not JSON).Fatal/Fatalfnow carry theSystem error:marker, so every failure to init/start (e.g. "Cannot serve without repository") is alertable.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 theSystem 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 console-side fix — building each entity's JSON before anything is written, skipping and logging malformed entities, and destroying the socket so a mid-stream failure can't look like a valid 200 — is in #1441.
The matching Datadog monitors + workload-health prober land in a companion jitsu-cloud-infra PR (jitsucom/jitsu-cloud-infra#71).
Testing
go build+go veton jitsubase, config-keeper, bulkerapptsc --noEmitclean for core-functions-lib🤖 Generated with Claude Code