turbine: end the open transaction before a partition drop - #440
Merged
Merged
Conversation
The drop ran inside whatever transaction the pipeline's connection had open. Between batches that is an idle tick's progress write, begun before the drop took the pass lock. Its snapshot could predate a window pass that had since deleted closed buckets' rows on the manager's own connection and committed; DuckDB then refused the drop's delete of those rows with "Conflict on tuple deletion", and the pipeline stopped over it (#437). The pass lock could not help: it excludes passes from now on, not the one that committed before the snapshot was taken. dropPartitions now commits the open transaction first, so the drop begins one whose snapshot is after every pass the lock excludes. The progress write is rewritten at every commit, so nothing is lost by committing it early. Test: TestTurbine_PartitionDropSucceedsAfterAPassDeletedTheRows drives a real Turbine with a DuckDB dropper over two connections in exactly that order. On main the run stops with the crash log's error; with the fix it continues. internal/core and internal/cli/run pass under -race. Closes #437.
The test left the loop running and let t.Cleanup close its connection and database under it: a use after free in the ADBC driver, which segfaulted on CI's Linux runner and happened to pass on a Mac. The loop is now cancelled and waited for before anything closes.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Closes #437.
The bug
When a
partition_ownedworker's group session failed, it crashed while dropping the partitions it had lost:This was the other half of the network blip in #436: one worker stalled silently, the other crashed.
Why
The window table is written from two DuckDB connections: the pipeline's (batch inserts, the partition drop) and the window manager's (the pass that publishes closed minutes and deletes their rows). Each has its own transaction, and DuckDB's concurrency is optimistic.
The drop ran inside whatever transaction the pipeline's connection already had open. Between batches that's an idle tick's progress write, which opens a transaction and takes its snapshot before the drop takes the pass lock. If a pass then deleted the same rows and committed, the drop's
DELETEhit tuples already gone, and DuckDB refused it. The pass lock couldn't help: it excludes passes from now on, not one that committed before the snapshot.Nine statements across two connections reproduce it exactly.
The fix
dropPartitionscommits the pipeline's open transaction before deleting, so the drop begins a transaction whose snapshot is after every pass the lock excludes. The progress write is rewritten at every commit, so committing it early loses nothing.Test
TestTurbine_PartitionDropSucceedsAfterAPassDeletedTheRowsdrives a realTurbinewith a DuckDB dropper over two connections in exactly that order, then loses the partitions.main: the run stops with the crash log's error, word for word.internal/coreandinternal/cli/runpass under-race.Not reproduced end to end
Unlike #436, this one needs a window pass to land in a ~300ms gap after an idle tick, so it's timing-dependent in the stack. The two-connection test is the proof.