feat: implement acknowledgment tracking for pipeline records - #79
Draft
Mahesh Kamble (ma-gk) wants to merge 3 commits into
Draft
feat: implement acknowledgment tracking for pipeline records#79Mahesh Kamble (ma-gk) wants to merge 3 commits into
Mahesh Kamble (ma-gk) wants to merge 3 commits into
Conversation
Contributor
|
Nice approach! Then we can simply call these funcs. |
| s.SendData(ack.WithContext(ctx, msgAck), []byte(*m.Body), output) | ||
|
|
||
| wg.Add(1) | ||
| go s.deleteOnComplete(msgAck, m.MessageId, m.ReceiptHandle, inFlight, wg) |
Contributor
There was a problem hiding this comment.
For larger record counts, it is going to create unbounded count of go routines as each is going to create a channel in blocking state, instead we can use a optimised approach on callback func, and the sink task then executes it.
Same can be done for kafka as well.
// In getMessages — no goroutines, no waiting, no channels:
for _, m := range receiveMessageOutput.Messages {
msgAck := ack.New()
// capture receipt handle; delete runs inline on whichever
// goroutine calls the final Done/Fail — zero extra goroutines.
receipt := m.ReceiptHandle
msgId := m.MessageId
msgAck.Done(func(failed bool) {
if !failed {
s.client.DeleteMessage(ctx, &qs.DeleteMessageInput{
QueueUrl: &s.QueueURL,
ReceiptHandle: receipt,
})
}
})
s.SendData(ack.WithContext(ctx, msgAck), []byte(*m.Body), output)
}
And then, in the end on termination you can call
msgAck.Done(true)
With this approach, now deletion would not be enqueued.
Contributor
There was a problem hiding this comment.
Something same can be done for kafka.
We can define an OnComplete function in interface, which can do cleanup
Addresses review feedback on the acknowledgment tracking added in 4fb7139. Auditing the goroutine-per-message design surfaced two deadlocks and a class of ack leaks, all reachable from pipeline shapes that run in production today. Deadlocks - The source gated its receive loop on a semaphore of Concurrency*5 that was released only on downstream completion. Any fan-in that accumulates records before emitting closed the cycle: the source stalls at the limit, the fan-in never reaches its flush threshold, nothing acks, no slot frees. The bound is gone; messages in flight are bounded by channel_size, which is what applies backpressure anyway. - tracker.Wait() ran inside Run, but a task's output channel closes only after Run returns. join, archive pack and sample tail/random emit only when their input closes, so waiting inside Run waited for a flush that could not happen until the wait ended. The wait moved to a new optional task.Finisher hook the pipeline calls after closing the output channel. Ack leaks - Base.SendRecord dropped the ack when output was nil, so any task terminating a pipeline without its own guard never settled it. Settled centrally now. jq and replace additionally skipped their whole loop when terminal, never draining their input; they now drain and drop. - 14 sites across 13 tasks abandoned the record they were holding on a mid-loop error, stranding its ack. New ack.Reject/ack.Rejected settle it as failed so the broker redelivers instead of the pipeline hanging. One malformed CSV row was enough to hang a pipeline indefinitely. - The pipeline rejects anything left in a task's input once every worker has returned. Doing this inside the task instead discarded records that healthy sibling workers would have written - measured at 997 of 1998 lost on a file-source pipeline with task_concurrency 4. Data loss - converter and xpath emitted one record per csv row / container node without adjusting the ack, so the first output to complete settled the whole record and the message was deleted while the rest were still in flight. Both now count before sending, matching split and archive. Also - join's duration: never fired. The select had a default case, so it never blocked and the ticker was only serviced between records - never while input was stalled, which is exactly when a partial batch needs flushing. - archive tar unpack reads the archive once instead of twice. - concurrency now sizes the pool of concurrent deletions, restoring the meaning it has on main. No pipeline YAML changes. Verified against origin/main: 20 fixtures with matching exit codes (15 byte-identical), 6 converter goldens byte-identical, race detector clean, 10k records at 38-118 MB, exactly-once with 0 duplicates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
What this does
Adds at-least-once acknowledgment for pipeline records: a message is deleted from its
source only once every downstream task has finished with the record produced from it. A
crash mid-pipeline leaves the message in the queue for redelivery instead of losing it.
An
ack.Ackis created per source record and rides onrecord.Record.Context, so tasksthat simply transform a record need no wiring. Tasks that change a record's branch count
adjust it explicitly —
Fanoutfor one-to-many,Joinedfor many-to-one,Dropforfiltered-out,
Rejectfor failed.Review feedback addressed
archive/tar.godeleteOnComplete?output == nilbranch (no consumer, so no downstream ack to wait for). Both paths now share onedeleteMessageOnCompletein an interface for cleanupack.AcknowledgerwithAck(failed bool). SQS implements it asmessageAck; Kafka can adopt the same trackerDefects found while auditing, and fixed here
Auditing the original design surfaced problems beyond the review comments. All of these
are reachable from pipeline shapes that exist in
data-airflowtoday.Two deadlocks. The receive loop was gated on a semaphore of
Concurrency*5releasedonly on downstream completion, which deadlocks against any fan-in that must accumulate
records before it can emit. Separately,
tracker.Wait()ran insideRun, but a task'soutput channel closes only after
Runreturns — andjoin,archive packandsample tail/randomemit only when their input closes. So the wait waited for a flushthat could not happen until the wait ended. The semaphore is gone; the wait moved to a new
optional
task.Finisherhook the pipeline calls after closing the output channel.Silent data loss in
converterandxpath. Both emitted one record per CSV row /container node without adjusting the ack, so the first output to complete settled the
whole record — the message was deleted while the remaining rows were still in flight. Both
now count before sending, matching
splitandarchive.Ack leaks that hang the pipeline.
Base.SendRecorddropped the ack whenoutputwasnil, so any task terminating a pipeline without its own guard never settled it. Separately,
14 sites across 13 tasks abandoned the record they were holding on a mid-loop error. One
malformed CSV row was enough to hang a pipeline indefinitely. New
ack.Reject/ack.Rejectedsettle such a record as failed so the broker redelivers it. Leftovers in atask's input are rejected by the pipeline once every worker has returned — doing it
inside the task instead discarded records that healthy sibling workers would have written
(measured: 997 of 1998 lost with
task_concurrency: 4).join'sduration:never fired. Theselecthad adefaultcase, so it neverblocked; the ticker was only serviced between records — never while input was stalled,
which is exactly when a partial batch needs flushing.
Goroutine model
One goroutine per unacknowledged message, parked on the ack, bounded by
channel_sizerather than by a pool. The
concurrencysemaphore is taken after the ack settles, aroundthe delete only, so releasing it depends on
DeleteMessagereturning and never on pipelineprogress — that is what keeps it from recreating the deadlock.
A pool sized by
concurrencywas implemented first and then reverted: it needed a mutexand a completion hook on
Ack(the least-tested code in the tree) to save goroutines thatwere never the problem — 10,000 in flight measured at 118 MB peak, dominated by record
data rather than acks.
Compatibility
No pipeline YAML changes.
concurrencykeeps the meaning it has onmain:mainrans.ConcurrencyprocessReceiptsgoroutines behind ans.Concurrency * 1000channel; itnow sizes concurrent deletions. Only the trigger moved — from on-receive to on-completion,
which is the feature.
Verification
Against a binary built from
origin/main:echotimestamps (incl.
join,xpath,xpath_with_index,file,file_with_glob, botharchive and both compress fixtures). The 5 that differ are nondeterministic on
mainalone (random sampling, UUIDs, DAG interleaving) with matching record counts.
*_test_results.jsonreproduced byte-for-byte; xls, emland protobuf outputs identical.
-race(4000 iterations of registration racing settle) and was deleted.
Against LocalStack 4.0 — seven shapes that hung on the previous commit now exit cleanly
with the queue fully drained: terminal
jq, terminalreplace,sample(tail),join(number: 1000, duration: 2s),archive(pack), unwritable sink, and the new fan-outfixture. Plus three mocks of real
data-airflowshapes (ocr/pull_ocr_results,catalog_sources/catalog_listings,keepa/products/hourly) — all three hung before thischange.
Scale: 10,000 messages in 28s at 38 MB; 10,000 records via fan-out at 57 MB; 10,000
messages all unacknowledged simultaneously (
archive pack) at 118 MB. Exactly-onceconfirmed at 2,000 records — 2,000 distinct, 0 duplicates.
New fixture
test/pipelines/sqs_fanout_fanin_dag.yamlcoversjqexplode, DAG parallelbranches and
joinfan-in in one DAG, with a batch size deliberately indivisible by therecord count so records are always left buffered when the source stops — the case that
deadlocks. It hangs on the previous commit and produces exactly 29 files / 200 records /
100-100 branch split here.
Before deploying — two operational checks
Deferred acknowledgment holds a message in flight for longer, and every real config's
joinis unbounded in practice, so nothing is acked until the source stops.VisibilityTimeoutmust exceed each pipeline'send_after. Otherwise SQSredelivers mid-run and the output contains duplicates. Measured: at a 10s timeout with a
20s window, output was 100% duplicated; at 300s, exact.
message_group_id, or a boundedjoin. Anunacknowledged message blocks its message group, so with few groups throughput caps at
max_messagesper group per run — measured 10 of 40 versus 40 of 40 onmain. Raisingthe visibility timeout does not help this; a
duration:on thejoinrestores40 of 40. Nine of the sixteen SQS configs in
data-airflowuse FIFO; the tightest iskeepa/products/hourly(max_messages: 1,end_after: 50m). Caterpillar's own SQSwriter defaults to a UUID per message, so caterpillar-fed queues are likely unaffected.
Known gaps
Fanoutbefore send, exactly oneDone/Failper branch,Joinedon the output — are unguarded, so a future regressionis silent data loss rather than a test failure. All verification above was manual.
archive(pack)livelocks if the visibility timeout is shorter than the full draintime: it acks nothing until the whole stream is consumed, so redelivery keeps the queue
non-empty and
exit_on_emptynever fires. AChangeMessageVisibilityheartbeat wouldfix this and the duplication above, needs no new YAML key, and is the natural follow-up.
httppagination emits one record per page without aFanout— same data-loss shape asconverter/xpathwas. Not fixed here; the naive per-pageAddBranchpanics and itneeds a branch reserved for the loop.
heimdall'sresult.gosends with a background context rather than the record's, so itsacks leak. Fixing it makes its existing terminal-mode guard a double-ack, so both must
change together.
sample.draincan leak the buffered records' acks ifcrypto/randfails; fixing itproperly needs the sampler interface to expose its buffer.
🤖 Generated with Claude Code