fix(orchestrator): preserve conclude DLQ outcomes#426
Draft
albertywu wants to merge 1 commit into
Draft
Conversation
Summary: Intent: - Keep DLQ reconciliation consistent with the terminal outcome already selected by conclude. - Preserve useful failure context in terminal request logs. Changes: - Reuse the parent request termination helper across DLQ reconciliation. - Preserve successful, failed, and cancelled batch outcomes in conclude DLQ fanout. - Record original topic and failure metadata on DLQ terminal logs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a correctness bug in
conclude_dlqrecovery and reuses the shared request terminalization helper introduced in #424.concludeintentionally consumes aBatchIDafter an upstream controller has persisted the batch's terminal outcome. The message carries only the identity; conclude reloads the batch and mapsSucceeded,Failed, orCancelledto the corresponding request outcome. If conclude exhausts its retries, the sameBatchIDreachesconclude_dlqwhile the batch remains terminal.Previously,
conclude_dlqused the generic batch-failure reconciler. That reconciler intentionally skips batches already inSucceededorCancelledto avoid overwriting a competing terminal outcome. This behavior is correct for normal pipeline DLQs, but it is a bug forconclude_dlq, where a terminal batch is the expected input. A partially completed conclude fanout could therefore be acknowledged without reconciling the remaining request states or repairing their logs.This PR gives
conclude_dlqoutcome-preserving recovery:Succeededbatch -> reconcile member requests toLandedFailedbatch -> reconcile member requests toErrorCancelledbatch -> reconcile member requests toCancelledNormal pipeline DLQs remain failure-oriented: requests reconcile to
Error, while batches reconcile toFailedand their member requests toError.Scope: This is a targeted bug fix and code deduplication within the existing architecture. It does not introduce new topics, payloads, stages, stores, or request-log paths, so an RFC is not needed.
reconcileRequest, which callsrequest.ReconcileTerminalState; request-scoped DLQs do this once, while batch-scoped DLQs do it once for each member request.dlq.last_errorintoLastError, but passed nil forRequestLog.Metadata, so structured delivery context was dropped.TerminalOutcometorequest.ReconcileTerminalState, keepingdlq.last_errorinLastErrorand addingdlq.original_topic,dlq.failure_count, anddlq.failed_attoMetadatawhen available.concludeintentionally received IDs for terminal batches, butconclude_dlqused generic batch-failure reconciliation. That logic skippedSucceededandCancelledbatches, so a partially completed fanout could be acknowledged without reconciling the remaining requests or repairing their logs.concludeBatchtreats the existing terminal batch state as the source of truth and callsrequest.ReconcileTerminalStatethroughreconcileRequestfor every member request, completing or repairing the expected fanout.Helper call path: In every After case above, each request terminal-state reconciliation is performed by
request.ReconcileTerminalState. Request-scoped DLQs call it throughreconcileRequest; batch and conclude DLQs callreconcileRequestonce for each member request. Batch failure still usesBatchStore.UpdateState, while conclude DLQ preserves the batch's existing terminal state.ReconcileTerminalStateusagereconcileRequestErrorfailBatchErrorfor each member requestconcludeBatchSucceeded,Failed, orCancelledmaps to requestLanded,Error, orCancelledFuture callers should use this helper only after deciding that a request must enter a terminal state and needs the matching public request log. The caller owns the business decision and supplies
State,LastError, andMetadata; the helper owns the CAS, version advancement, state-to-status mapping, idempotency, and log publication. Do not use it for non-terminal request transitions, batch transitions, or log-only events.Test Plan
✅
make lint && make check-tidy && make check-gazelle && make testRevert Plan
Revert this PR. This restores the previous generic
conclude_dlqbehavior and DLQ-specific request termination logic; #424 can remain independently.Stack