Conversation
… action A worker killed between completing a step and scheduling the next one leaves its job row processing with no Action Scheduler action that will ever advance it, and nothing in the engine notices until an operator runs recover-stuck. Add a recurring stale-claim reconciliation engine task (five-minute cadence, like high-churn AS cleanup) that detects processing jobs with no live pending/fresh in-progress action and stale run_metrics activity, then resumes them from their last completed step or terminalizes them with an explicit reason. Reuses existing primitives end to end: operation_generation/token claim identity, DirectJobEnqueuer::liveGenerationExecution(), DirectOperationRecoveryPolicy fenced requeue/terminalize for direct jobs, ExecutionPlan + step_results resume-point resolution (same completed-step semantics as JobRetryPolicy), and the shared engine_data.retry.attempts bound (default 3) so a poisonous step terminalizes instead of looping. recover-stuck stays unchanged as the operator escape hatch. Ref: #3478
…s_reason The canonical transition path stores the compound status base in the row and the reason under engine_data.job_status_reason, so assertions read the reason from the engine snapshot.
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.
engine: stale-claim reconciliation for crashed workers (proposal)
Opened as a DRAFT on purpose. The implementation is complete and gated, but the reviewer question this PR really asks is not "is this code correct" — it is "is a reconciler the right shape at all, or is it a daemon that exists to keep two copies of one fact agreeing?" See Is this the right layer? below, answered from what the code actually shows. My conclusion: necessary stopgap, with a concrete follow-up direction that could shrink it. Merge decision is a human call.
Closes #3478
Root cause
A worker that dies between completing step N and scheduling step N+1 leaves the job row
status = 'processing'with no Action Scheduler action that will ever advance it, and nothing in the engine notices.statusand "an AS action exists for this claim" are two copies of one fact — "this run is alive" — written by the same request; when the worker dies mid-request, only AS-side knowledge dies with it, and the row's copy persists forever. Verified on production (events.extrachill.com): jobs 1015119/1015122/1015124 hadstep_resultsfor step 0, frozenrun_lifecycle.updated_at, and zero rows inactionscheduler_actionsdespite being inside the retention window — and AS held nofailedactions either, so the death left no trace on the scheduler side. The Redis outage that killed the workers was incidental; OOM, PHP fatal, timeout, or a deploy mid-flight all produce the same orphan.What this PR does
A recurring engine task (
stale_claim_reconciliation, five-minute cadence — the same shape as the high-churnretention_as_actionsschedule: SystemTask handler +datamachine_recurring_schedulesentry, dispatched through the existingdatamachine/dispatch-system-taskroutine wake; no parallel scheduling path) runs a bounded reconciliation pass:processingjobs with no pending AS action and no fresh in-progress action for the job (exact decoded-args match, mirroringRecoverStuckJobsAbility::getActiveStepActionIdssemantics: pending is live unconditionally, in-progress only inside the freshness window) whoserun_metrics.last_activity_at(fallbackcreated_at) is older than the threshold. Bounded batch of 50 per tick.step_resultsentry (never started → nothing applied → running it is the safe resume), scheduled as a uniquedatamachine_execute_stepaction, job returned topending, attempt recorded.failed - worker_died_mid_stepetc. — stored per the canonical base+job_status_reasonconvention) plus full context underengine_data.stale_claim_reconciliation.wp datamachine jobs recover-stuckis untouched as the operator escape hatch; it is simply no longer the only thing between a crash and a permanent orphan.Existing primitives reused (and what I deliberately did not build)
operation_generation/operation_claim_tokenAS args — no new claim mechanism.DirectJobEnqueuer::liveGenerationExecution()for direct jobs; the LIKE-scan + exact-args-decode pattern fromRecoverStuckJobsAbilityfor flow jobs.DirectOperationRecoveryPolicy::diagnose(),Jobs::commit_missing_direct_operation_requeue()(fenced requeue),Jobs::transition_missing_direct_operation()(fenced terminalize),getProcessingSystemTaskChildren()— the recurring task delegates; it adds no direct-job machinery of its own.ExecutionPlan::from_flow_config()+engine_data.step_resultswith the same completed-step semantics asJobRetryPolicy::stepCompletedSuccessfully().engine_data.retry.attempts— the same counterJobRetryPolicydraws from, so retries and crash-resumes share one bounded budget (default 3, filterdatamachine_stale_claim_max_resume_attempts).SystemTask+RecurringScheduleRegistry+ Agents API Routines, exactly like the retention tasks.Deliberately not built: a new claim/lease table, a heartbeat column, a job-state state machine rewrite, or a second recovery codepath for direct jobs. Also deliberately not touched: the AS retention windows (that is #3479's discussion),
RecoverStuckJobsAbility(the operator pass stays authoritative for overrides, pathless children, and pending-AI deferrals), and both CLI files owned by the sibling PR.Detection rule and threshold
status = 'processing'AND no pending/fresh-in-progress AS action for the job ANDlast_activity_atolder than 15 minutes (filterdatamachine_stale_claim_activity_threshold), candidates prefiltered bycreated_at < now - thresholdso the query stays cheap.Justification: an AS action is the only mechanism that advances a processing job, so an actionless processing row is already beyond the engine's recovery contract — the threshold exists only to stay clear of legitimate scheduling latency (queue backlog, replication lag, the seconds-long gap between one action completing and the next being inserted). Fifteen minutes is an order of magnitude beyond any of those while keeping crash-to-recovery latency inside a few ticks of the five-minute cadence.
Resume-vs-terminalize decision table
source = 'batch')engine_data.job_statusoverride presentcommit_missing_direct_operation_requeue; fence loss ⇒ skip — a newer claim owner advanced it)scheduler_path_lost_after_effects(+ processing system-task children), reusing the operator pass's exact reasonworker_died_mid_step— effects cannot be proven absentstale_claim_resume_exhausted— no infinite loopwaitingstale_claim_unresumable— completion accounting cannot be safely synthesizedThe generation-advanced case from the issue is enforced structurally: resumption only ever happens through a fenced commit on the current row's generation+token, so a stale claim can never resume the job — it loses the fence and skips.
Tests
tests/Unit/Core/StaleClaimReconcilerTest.php(WP_UnitTestCase + real Action Scheduler), covering the four required scenarios plus two:skipped/live_action_exists, row untouched).pending,retry.attempts = 1, resume action present.stale_claim_resume_exhausted) instead of looping.scheduler_path_lost_after_effects), not resumed.worker_died_mid_step.No existing assertions were weakened or removed.
Gates
homeboy review lint data-machine --placement local(per touched file; runs PHPCS (WordPress) + ESLint + PHPStan level 7): pass, no findings oninc/Core/StaleClaimReconciler.php,inc/Engine/AI/System/Tasks/StaleClaimReconciliationTask.php,inc/Engine/AI/System/SystemAgentServiceProvider.php,tests/Unit/Core/StaleClaimReconcilerTest.php.homeboy review test data-machine --placement local(full SQLite suite): pass — 1520 tests, 32 skipped, 0 failures (includes the six new tests; no existing test regressed).homeboy-test.yml) additionally runs the full MySQL suite, which covers the transaction-fenced paths exercised by scenario 4.Is this the right layer?
Asked because the deeper suspicion is fair:
status = 'processing'is a second copy of a fact Action Scheduler already owns, and a reconciler is structurally a daemon whose job is keeping two copies of one fact agreeing. What the code shows:1. Where is job
statuswritten and read? Transitions are centralized inJobs.php(7 canonical methods:create_job,start_job,update_job_status,transition_job_status,complete_job, plus the fencedtransition_missing_direct_operation/transition_recovery_owned_childvariants), but those are called from 21 files / 65 call sites acrossinc/, and 24 files reference theprocessingstate. Reads are heavier: 86get_job()/get_job_metadata()call sites and ~160['status']checks on job rows. Could reads derive liveness from AS instead? Only the live ones. Most read sites are terminal-state or reporting reads — status filters inget_jobs/count_old_jobs,JobsSummaryAbility/RunMetricsAbility/CLI tabulations — which AS cannot answer after its retention prunes history.2. What does the status column give us that AS cannot? Three concrete things. (a) Terminal history with reasons:
execute_step/resume_ai_stepAS actions are pruned after 1 hour (retention_as_actionshook windows:1/24day), while job rows are kept 30 days — the column is the only durable record a job ever ran, how it ended, and why (failed - <reason>compounds,cancelled, plus statuses with no AS analogue at all:completed_no_items,agent_skipped). (b) Cross-request queries: retention deletes, summaries, dashboards, and dedup all queryWHERE status IN (...)directly against the table; deriving that from AS args-JSON scans would be strictly worse. (c) Terminal accounting inputs: theoperation_*/terminal_accounting_*columns that fence recovery live on the same row. So yes — the column earns its place. What it does not earn is theprocessingvalue specifically: that is the one state whose truth AS already owns.3. Would AS-authoritative liveness eliminate the drift class, or move it? The codebase already ran this experiment for direct jobs: the
operation_state/operation_generation/operation_claim_token/operation_action_idcolumns make AS authoritative for direct-job liveness, withDirectOperationRecoveryPolicy+ fenced requeue/terminalize as the enforcement. Drift still happened — the fencing and recovery primitives exist because a duplicated fact drifted (enqueue crashed between schedule and receipt). What the fencing bought was not "no drift" but bounded, detectable, owner-verified drift. Making AS authoritative for flow-job liveness the same way would shrink this orphan class to the same bounded shape — it would not vanish, because every status write still happens in-request on a worker that can die mid-write. Fully eliminating the class would mean never persisting liveness at all: treatprocessingas a derived view (row is either pending-with-an-action or terminal), which is a real architectural change across those 24 files, the status machine, and terminal accounting — not a PR-sized change.4. Recommendation. Necessary stopgap, right layer for now — with a follow-up worth filing. The reconciler is the flow-job counterpart of the direct-job recovery machinery that already exists; it reuses that machinery rather than parallel-building, it is bounded (50 candidates/tick, shared attempt bound), and it converts an invisible permanent orphan into a resume or a loud, reasoned terminal state within minutes. That logic — resume from last completed step vs terminalize loudly — is worth having under any architecture, because even a derived-liveness engine still needs to decide what to do with a dead run's partial effects. The durable fix direction is the one point 3 points at: extend the direct-job operation fencing to flow jobs so "is anything scheduled" becomes owner-verifiable per claim, shrinking the reconciler to a small safety net. I would rather land this stopgap behind the draft flag and file that follow-up than hold crash recovery hostage to the larger refactor. Happy to close this in favor of the architectural approach if that is the call — the decision table and tests transfer.
This PR was authored by an AI coding agent (Extra Chill Bot minion) and is pending human review.