From 146270e22ca164c4994f933a52588e6b62f7a07d Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Thu, 14 May 2026 12:52:54 +0200 Subject: [PATCH 1/4] clarify CQRS handler-error semantics in go-cqrs.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a 'Handler Errors Do Not Cause Kafka Replay' section explaining the result-sender wrapper behavior: handler errors are caught, a single Failure result is emitted, and the offset commits normally. This removes the common misconception that returning err triggers infinite kafka replay. Reframe the err vs ErrCommandObjectSkipped guidance around result-topic noise rather than retry behaviour (both commit the offset; they differ in what they put on the result topic). References bborbe/trading#125 — the 228 visible failures on one actualTrade were 228 distinct kafka messages from a publisher without state pre-filter, not retries of one offset. --- CHANGELOG.md | 4 ++++ docs/go-cqrs.md | 34 ++++++++++++++++++++++++++++++---- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 524b931..c3e9c8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ Please choose versions by [Semantic Versioning](http://semver.org/). * MINOR version when you add functionality in a backwards-compatible manner, and * PATCH version when you make backwards-compatible bug fixes. +## Unreleased + +- docs: clarify CQRS handler-error semantics in `go-cqrs.md` — handler errors do NOT cause kafka replay; the result-sender wrapper emits a single Failure result and commits the offset. Use `ErrCommandObjectSkipped` to suppress noisy Failure results when the caller condition is non-retryable. References bborbe/trading#125. + ## v0.9.8 - chore: extract `check-versions` to `scripts/check-versions.sh`; add `make release-check` (`precommit + check-versions`); unwire `check-versions` from `precommit` so drift during development is allowed and alignment is enforced at release time. Add `docs/releasing-coding.md`. Aligns with `dark-factory` / `vault-cli` / `semantic-search` release-gate shape. diff --git a/docs/go-cqrs.md b/docs/go-cqrs.md index 49483fb..c0ec30e 100644 --- a/docs/go-cqrs.md +++ b/docs/go-cqrs.md @@ -61,25 +61,51 @@ topic := schemaID.ResultTopic(branch) ## Skipping Invalid Commands -Return `cdb.ErrCommandObjectSkipped` when a command should be committed but not processed. Framework advances offset, sends no result. **Why:** `nil` silently swallows; normal error retries forever. +Return `cdb.ErrCommandObjectSkipped` when a command should be committed but not processed. Framework advances offset, sends no result. ```go // BAD — silently swallows, no visibility return nil, nil, nil -// BAD — framework sends failure result + retries +// BAD — emits a Failure on the result topic for every occurrence (noisy if caller is non-retryable) return nil, nil, err -// GOOD — skips with reason, no retry, no result +// GOOD — clean skip: no retry, no result emitted, offset advances return nil, nil, errors.Wrapf(ctx, cdb.ErrCommandObjectSkipped, "reason: %v", err) ``` **Use for:** malformed data, validation failure, duplicates, wrong state, filtered out. -**NOT for:** transient errors (network, disk) — return normal error so framework retries. +**NOT for:** transient errors (network, disk) — return normal error so the failure is visible on the result topic. + +## Handler Errors Do Not Cause Kafka Replay + +A common misconception: "If my handler returns `err`, kafka will replay the message forever." Not true for this framework. + +The result-sender wrapper (`cdb_command-object-executor-tx-result-sender.go`) catches the handler error, emits a `ResultObjectFailure` to the `*-result` topic, and returns `nil` to the outer kafka consumer. The offset commits on the next batch tick. Each error is **one** Failure on the result topic — not an infinite replay. + +``` +Handler returns err + ↓ +Wrapper sends ResultObjectFailure to *-result topic + ↓ +Wrapper returns nil to outer message handler + ↓ +Kafka offset commits → next message processed +``` + +The only path where offsets do NOT commit is the result-sender itself failing to publish (e.g. kafka producer broken). That bubbles a real error and triggers the kafka library's redelivery semantics. + +**Implications:** + +- Returning `err` from a non-retryable condition (wrong state, validation failure) is **functionally safe** — no replay loop — but it produces a `Failure` on the result topic for every occurrence. If a publisher emits N copies of the same command (no state pre-filter, broker confirm retries, etc.) you get N `Failure` entries and N error log lines. Use `ErrCommandObjectSkipped` to avoid that. +- Returning `err` from a **transient** condition (network blip, disk full) is still the right choice — but understand it produces a single Failure result and a single error log, NOT an automatic retry. If you want retry, build it into the handler or the orchestration around it. + +**Real-world reference:** bborbe/trading#125 — `core/actualtrade/controller` handlers returned `InvalidStateError` for commands targeting trades in terminal states. The "228 failures on one trade" in the logs were 228 **distinct** kafka messages from a publisher that did not pre-filter by state, NOT retries of one offset. Fix: handle terminal states as idempotent skip rather than error. ## Rules - Never consume event topic to wait for command results — use result topic - `RunCommandConsumerTx` wraps executors automatically — don't wrap manually - `ErrCommandObjectSkipped` skips silently (no result sent) — use for non-retryable situations +- Normal `err` returns are NOT retried by the framework; they emit one Failure result and commit the offset — same offset behaviour as Skipped, different result-topic behaviour - `SendResultEnabled() == false` + no error → no result sent - Context timeout → `ResultFor()` returns `Success: false` From eb8321c9a42f80daa2151a3d73f031fde1be638a Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Fri, 15 May 2026 10:12:32 +0200 Subject: [PATCH 2/4] docs(go-cqrs): genericize trading-specific examples Address PR #1 review: replace bborbe/trading#125 / actualtrade / InvalidStateError reference block with a generic order-processing example. Drop trading#125 trailer from the changelog entry. Per CLAUDE.md "General-Purpose Content Only" rule. --- CHANGELOG.md | 2 +- docs/go-cqrs.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3e9c8d..26568c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ Please choose versions by [Semantic Versioning](http://semver.org/). ## Unreleased -- docs: clarify CQRS handler-error semantics in `go-cqrs.md` — handler errors do NOT cause kafka replay; the result-sender wrapper emits a single Failure result and commits the offset. Use `ErrCommandObjectSkipped` to suppress noisy Failure results when the caller condition is non-retryable. References bborbe/trading#125. +- docs: clarify CQRS handler-error semantics in `go-cqrs.md` — handler errors do NOT cause kafka replay; the result-sender wrapper emits a single Failure result and commits the offset. Use `ErrCommandObjectSkipped` to suppress noisy Failure results when the caller condition is non-retryable. ## v0.9.8 diff --git a/docs/go-cqrs.md b/docs/go-cqrs.md index c0ec30e..9b8a146 100644 --- a/docs/go-cqrs.md +++ b/docs/go-cqrs.md @@ -98,7 +98,7 @@ The only path where offsets do NOT commit is the result-sender itself failing to - Returning `err` from a non-retryable condition (wrong state, validation failure) is **functionally safe** — no replay loop — but it produces a `Failure` on the result topic for every occurrence. If a publisher emits N copies of the same command (no state pre-filter, broker confirm retries, etc.) you get N `Failure` entries and N error log lines. Use `ErrCommandObjectSkipped` to avoid that. - Returning `err` from a **transient** condition (network blip, disk full) is still the right choice — but understand it produces a single Failure result and a single error log, NOT an automatic retry. If you want retry, build it into the handler or the orchestration around it. -**Real-world reference:** bborbe/trading#125 — `core/actualtrade/controller` handlers returned `InvalidStateError` for commands targeting trades in terminal states. The "228 failures on one trade" in the logs were 228 **distinct** kafka messages from a publisher that did not pre-filter by state, NOT retries of one offset. Fix: handle terminal states as idempotent skip rather than error. +**Example pattern:** an order-processing handler returns an `InvalidStateError` whenever a command targets an order already in a terminal state (`Cancelled`, `Filled`). If the publisher does not pre-filter by state and emits N duplicate commands for the same order, the result topic gets N `Failure` entries and the log gets N error lines — none of them retries, all distinct messages. Fix: treat terminal states as an idempotent skip (`ErrCommandObjectSkipped`) rather than an error. ## Rules From 8681efd2bd1b13284ef6b7e6413e14ca71dbcaba Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Fri, 15 May 2026 10:36:57 +0200 Subject: [PATCH 3/4] docs(go-cqrs): drop internal library filename reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #1 re-review: replace `cdb_command-object-executor-tx-result-sender.go` with the generic "result-sender wrapper" — surfacing an external library's internal filename creates fragile coupling that silently rots on refactor. --- docs/go-cqrs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/go-cqrs.md b/docs/go-cqrs.md index 9b8a146..0739bd7 100644 --- a/docs/go-cqrs.md +++ b/docs/go-cqrs.md @@ -79,7 +79,7 @@ return nil, nil, errors.Wrapf(ctx, cdb.ErrCommandObjectSkipped, "reason: %v", er A common misconception: "If my handler returns `err`, kafka will replay the message forever." Not true for this framework. -The result-sender wrapper (`cdb_command-object-executor-tx-result-sender.go`) catches the handler error, emits a `ResultObjectFailure` to the `*-result` topic, and returns `nil` to the outer kafka consumer. The offset commits on the next batch tick. Each error is **one** Failure on the result topic — not an infinite replay. +The result-sender wrapper catches the handler error, emits a `ResultObjectFailure` to the `*-result` topic, and returns `nil` to the outer kafka consumer. The offset commits on the next batch tick. Each error is **one** Failure on the result topic — not an infinite replay. ``` Handler returns err From b3ed61259caa1ca7202957280475bd6ffd360e60 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Fri, 15 May 2026 11:31:36 +0200 Subject: [PATCH 4/4] docs(go-cqrs): hedge offset-commit claim, drop trading-domain term Address PR #1 round-3 nits: - Line 94: "the only path where offsets do NOT commit" was too absolute; hedge to "in normal error-handling paths" and call out process-level failures (panic, SIGKILL) as a separate concern. - Line 101: replace "Filled" (trading-domain) with "Completed" per the General-Purpose Content Only rule. --- docs/go-cqrs.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/go-cqrs.md b/docs/go-cqrs.md index 0739bd7..cdb9733 100644 --- a/docs/go-cqrs.md +++ b/docs/go-cqrs.md @@ -91,14 +91,14 @@ Wrapper returns nil to outer message handler Kafka offset commits → next message processed ``` -The only path where offsets do NOT commit is the result-sender itself failing to publish (e.g. kafka producer broken). That bubbles a real error and triggers the kafka library's redelivery semantics. +In normal error-handling paths, the only case where offsets do NOT commit is when the result-sender itself fails to publish (e.g. kafka producer broken) — that bubbles a real error and triggers the kafka library's redelivery semantics. Process-level failures (panic escaping the wrapper, SIGKILL, OOM) also skip the commit, but those are infrastructure concerns, not application-level error handling. **Implications:** - Returning `err` from a non-retryable condition (wrong state, validation failure) is **functionally safe** — no replay loop — but it produces a `Failure` on the result topic for every occurrence. If a publisher emits N copies of the same command (no state pre-filter, broker confirm retries, etc.) you get N `Failure` entries and N error log lines. Use `ErrCommandObjectSkipped` to avoid that. - Returning `err` from a **transient** condition (network blip, disk full) is still the right choice — but understand it produces a single Failure result and a single error log, NOT an automatic retry. If you want retry, build it into the handler or the orchestration around it. -**Example pattern:** an order-processing handler returns an `InvalidStateError` whenever a command targets an order already in a terminal state (`Cancelled`, `Filled`). If the publisher does not pre-filter by state and emits N duplicate commands for the same order, the result topic gets N `Failure` entries and the log gets N error lines — none of them retries, all distinct messages. Fix: treat terminal states as an idempotent skip (`ErrCommandObjectSkipped`) rather than an error. +**Example pattern:** an order-processing handler returns an `InvalidStateError` whenever a command targets an order already in a terminal state (`Completed`, `Cancelled`). If the publisher does not pre-filter by state and emits N duplicate commands for the same order, the result topic gets N `Failure` entries and the log gets N error lines — none of them retries, all distinct messages. Fix: treat terminal states as an idempotent skip (`ErrCommandObjectSkipped`) rather than an error. ## Rules