Prevent on_batch_complete from firing before all children are enqueued#4
Open
lovitt wants to merge 3 commits into
Open
Prevent on_batch_complete from firing before all children are enqueued#4lovitt wants to merge 3 commits into
lovitt wants to merge 3 commits into
Conversation
`schedule_pending_jobs` dispatches each child to Cloud Tasks and only then registers it in the batch state, one child at a time. `complete?` considers a batch done as soon as every *currently-registered* child is complete — it has no notion of the parent having finished enqueuing. On a large fan-out with uneven child durations, fast early children can complete while the parent is still scheduling slower siblings. At that point every registered child is complete, so `on_child_complete` fires `on_batch_complete` prematurely — before the slow children have even been scheduled. The one-shot completion guard makes this worse: it fires exactly once, so the early (wrong) result is never corrected. Flag the batch as fully enqueued at the end of `setup`, and require that flag before a child may trigger completion. The parent's own post-`setup` `complete` call remains the backstop for the case where every child finishes before the flag is set, so no batch can hang. Children still register after dispatch, so a partial setup cannot orphan state.
Progressive expansion (a running child adding more jobs to an already-enqueued batch) is a supported workflow. It is unaffected by the premature-completion race — and by the seal — because the expanding child sits in the parent's batch state as `processing` until after it has registered its additions, so the batch cannot be judged complete mid- expansion. Pin that down so the seal can't regress it.
Track setup as in_progress/done instead of a single end-of-setup flag, and treat a marker-less batch (enqueued before this change) as fully enqueued so batches in flight during an upgrade are not stranded.
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
1 participant
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.
The bug
on_batch_completecan fire before the batch has finished enqueuing its children.The parent enqueues children one at a time, and
complete?only checks the children already registered in the batch state — it has no notion of how many there will ultimately be. On a large fan-out, an early child can finish before the parent has registered the rest; at that point every child the batch knows about is complete, socomplete?returnstrueand completion fires while later children are still unscheduled.We hit this in production: a ~300-child batch fired
on_batch_completewhile still enqueuing, and the summary it produced was missing the results the not-yet-run children went on to write.The fix
Flag the batch as fully enqueued at the end of
setup, and require that flag before a child can trigger completion:setupsetsbatch_setup_gidafterschedule_pending_jobs.on_child_completereturns early unlesssetup_complete?.cleanupremoves the key.Only the child-driven path is gated. A batch whose children all finish during enqueuing still completes via the parent's own
completeat the end ofexecute, so no completion is lost. This is separate from the existingbatch_completion_gidguard, which dedupes concurrent finishers on an already-enqueued batch.Progressive batch expansion is unaffected: the seal is set once, at the initial
setup, and an expanding child sits in the batch state asprocessing, socomplete?stays false while it runs.One edge case: if the root fails
setupon every retry until it's marked dead, the batch has registered children but no seal, so it never resolves. This only affects a batch whose root died before it could finish enqueuing; onmasterthe same case instead fireson_batch_completeoff the partial set, reporting a never-assembled batch as complete.Testing
Added a spec reproducing the premature fire (written to fail on
master), plus coverage forsetup_complete?, thesetupseal, andcleanupremoval. Full suite green; RuboCop clean.