fix: batch parents stay processing after all children terminal — stale cache read + no durable recheck - #3519
Merged
Merged
Conversation
…m durable rechecks The children_complete decision in PipelineBatchScheduler read the parent's engine snapshot through EngineData::retrieve(), which serves a persistent object-cache entry whose miss-fill (SELECT then wp_cache_set) loses races against a concurrent mutate() CAS commit. When that happens to the final chunk's worklist_complete fence, the poisoned entry is served forever — a stranded parent is never written again — and every remaining child callback saw batch_pending=true and early-returned. The parent stayed processing despite the database showing a fully scheduled, fully terminal batch (6-22/day on events.extrachill.com, #3518). - Split the decision into inspectParent() + reconcileParent(): the parent engine snapshot now comes from Jobs::retrieve_engine_data() (database direct), never the object cache. - Persist batch_results through EngineData::mutate() CAS instead of a blind whole-snapshot write, which could clobber concurrent mutations and re-poison the cache. - When children are all terminal but the worklist fence is missing (dead final-chunk worker), or when a persist/complete step fails, re-arm the idempotent chunk action via BatchScheduler::scheduleFinalizeRetry() (now public) so the invariant holds structurally: a fully scheduled, fully terminal batch parent is never left with no future scheduler action. - Expose reconcileParent()/parentCompletionDue() for recovery tooling.
…failing them recover-stuck treated a children_complete pipeline batch parent with a completed worklist as unrecoverable (PathlessBatchRecovery::isRecoverable requires an unfenced worklist) and fell through to the timeout path, marking jobs failed whose children had all succeeded. Pruned Action Scheduler evidence additionally shielded these parents behind the scheduler-ownership guard forever. Route pipeline batch parents through PipelineBatchScheduler before the ownership guard: - dry run reports would_complete_batch_parent with the predicted status - apply reconciles via reconcileParent(); completion, failure to make it durable, and not-due (children still running) fall through to the existing ownership/pathless/timeout handling so nothing regresses - new batch_parents_completed counter in the ability result, message, log context, and CLI output The batch-completion-strategy smoke assertion now tracks the split guard shape in reconcileParent().
…rklist - stale persistent-cache snapshot cannot strand a fully terminal batch parent (reproduces #3518; fails on the old cache-served decision) - unfenced worklist with all children terminal re-arms the idempotent chunk action, and running the recheck completes the parent - recover-stuck dry run reports would_complete_batch_parent and apply completes the parent instead of timing it out
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.
Closes #3518
Root cause (proven, not inferred)
maybeCompleteParent()made its children_complete decision on a cache-served parent engine snapshot.EngineData::retrieve()(inc/Core/EngineData.php:76-91) readswp_cache_get('datamachine_engine_data')— persistent Redis on events — and its miss-fill is non-atomic againstmutate():SELECTreturns a pre-worklist_completesnapshot.finishV2State()CAS commitsworklist_complete=trueand doeswp_cache_set(fresh).wp_cache_set(stale)lands last and poisons the entry.A stranded parent's engine_data is never written again (only completion would rewrite it), so the poisoned entry is served forever. Every subsequent child callback read
batch_pending=trueand early-returned at the guard; the last child's callback — the one that should have completed the parent — read the same poisoned entry and stranded it.Live evidence (events.extrachill.com, blog 7) matched every prediction:
batch_scheduled=100, 100 children, 97 completed / 2 no_items / 1 failed / 0 open,worklist_complete=truein the DB (direct SQL), yet parentprocessinghours later → the decision-time read disagreed with the DB.terminal_accounting_state=4(complete): every child'score_callbacks→onChildCompletedid run, so the bug was inside the decision, not in hook delivery. (This also rules out the callback-before-status-write race: the child row is committed terminal before accounting stages run, Jobs.php:3198.)batch_scheduled == child count == terminal children,worklist_complete=true— the early-return conditions were false in the DB for every one of them, so only a stale read explains the stranding. Hypothesis 2 (dedupe shrinking child count belowbatch_scheduled) is dead: parent 1212793 hasbatch_scheduled=19= exactly its 19 children.What changed
PipelineBatchScheduler(owning layer):maybeCompleteParent()split intoinspectParent()+reconcileParent(). The decision now reads the parent snapshot viaJobs::retrieve_engine_data()— database-direct — never the object cache.batch_resultspersists throughEngineData::mutate()CAS instead of a blind whole-snapshotdatamachine_set_engine_datawrite (which could clobber concurrent mutations and re-poison the cache with the stale snapshot it was handed).BatchScheduler::scheduleFinalizeRetry(), now public): worklist fence missing with zero active children (dead final-chunk worker), counts-query failure, and persist/complete failure. A fully scheduled, fully terminal parent can no longer end up with no future scheduler action — the invariant the docblock promises is now structural, and it also self-heals the dead-final-chunk-worker shape.reconcileParent()/parentCompletionDue()exposed for recovery tooling; thepipeline_batchcore-callback bool contract is preserved (falseonly when a due completion could not be made durable, keeping the accounting stage retryable — existing teststest_parent_engine_write_failure_keeps_child_core_stage_retryable/test_parent_completion_failure_keeps_child_core_stage_retryablepass unchanged).RecoverStuckJobsAbility:reconcileParent()before the scheduler-ownership guard (pruned AS evidence otherwise shields them forever), instead of the timeout path that marked children-succeeded parents as failed. Dry run reportswould_complete_batch_parent+ predicted status;not_dueparents fall through to the existing ownership/pathless/timeout handling unchanged. Newbatch_parents_completedcounter surfaces in the ability result, message, log context, and CLI.Verification
git stash):processingafter all children are terminal — children_complete never lands (~6–22/day on events) #3518 reproduction),tests/Unitsuite (1955 tests) on a MySQL harness: zero new failures vs cleanorigin/main(junit diff — identical failure sets, which are pre-existing environment issues in this local harness).phpcs --standard=WordPresson all changed files: only pre-existing violation classes (camelCase method names matching file conventions, filename sniff); no new docblock or real issues from this change.Notes / follow-ups
EngineData::retrieve()'s miss-fill race exists for every consumer; filling withwp_cache_addinstead ofwp_cache_setwould stop readers from poisoning a fresher writer's value. I kept this PR in the owning layer, but the substrate hardening would shrink the stale window platform-wide.recover-stuckalready requeues v2 batches with an unfenced worklist (PathlessBatchRecovery); after this PR the fenced-but-terminal shape is also handled, so both stranding shapes reconcile.wp --url=https://events.extrachill.com datamachine jobs liveness --limit=200for 24h: no newno_scheduler_pathbatch parents should accumulate; existing 115 can be cleared with onerecover-stuck --limit=100pass (dry run first — it now reportswould_complete_batch_parent).Squash-merge note: conventional commits used; no CHANGELOG or version strings touched.