---
name: taskengine-recovery-key-stale-execution-trap
description: |
  Fix a class of silent-recovery-failure bug where a scheduled recovery / retry job
  uses TaskEngine's JobDispatcher with a FIXED idempotency key (e.g.
  'webhook-recovery:<id>') and stops actually re-pushing to Redis after the first
  partial failure. Use when: (1) writing a new scheduled TaskEngine Job that
  re-dispatches stranded work by calling `BuyerKiosk\TaskEngine\Application\JobDispatcher::dispatch()`
  with a key derived from the entity id, (2) reviewing such a job that "reports
  success but the work stays undone," (3) auditing existing recovery CLIs like
  `bin/payroll/reprocess-webhooks.php` for production-safe scheduling. Also flags
  the companion in-flight-race issue: recovery queries that just say
  `WHERE processed_at IS NULL` race with healthy worker pickup and need a
  staleness gate.
author: Claude Code
version: 1.0.0
date: 2026-06-03
---

# TaskEngine recovery key → stale-execution silent-no-op trap

## Problem

`BuyerKiosk\TaskEngine\Application\JobDispatcher::dispatch()` is an
**at-most-once** dispatcher. Internally
(`userfrosting/src/BuyerKiosk/TaskEngine/Application/JobDispatcher.php:69-113`):

1. Look up an existing execution by `(jobDefinitionId, idempotencyKey)` via
   `ExecutionRepository::findByIdempotencyKey`. If found, return the existing
   execution **without** calling `pushToQueue()`.
2. Otherwise, create the execution row via `ExecutionRepository::create()`
   (DB INSERT happens first).
3. Then call `pushToQueue()` — the Redis push.

If step 2 succeeds but step 3 throws (Redis down, network glitch, queue
adapter exception), the execution row is now persisted with no message
in Redis. Any future `dispatch()` call with the SAME idempotency key will
hit step 1, return that stale row, and report success — **without ever
pushing to Redis again**. The work stays undone, forever, silently.

Single-shot unit tests with fresh fixtures never see this — it only manifests
across multi-run scenarios (one prior run failed mid-flight, every future
run short-circuits). Recovery jobs scheduled to run every N minutes are
exactly that scenario.

## Context / Trigger Conditions

- Writing or reviewing a class that extends `BaseJob` and calls
  `JobDispatcher::dispatch()` directly (or wraps a `JobDispatchingWebhookProcessor`-
  style service) to re-dispatch existing entities.
- The idempotency key is derived from the entity id alone: e.g.
  `'webhook-recovery:' . $id`, `'invoice-retry:' . $invoiceId`,
  `'qbo-resync:' . $entityId`.
- The job is registered with a cron `schedule` in `task_job_definitions`
  (this multiplies the failure surface — every tick after the failed push
  is a no-op).
- Symptom: the job's task_runs row says `success`, `dispatched=N`, but the
  downstream Process*Job never runs, the entity remains in its "needs
  processing" state, and the recovery CLI manually executed re-pushes
  successfully.
- Companion symptom (a different but co-occurring bug): the recovery query
  is `WHERE processed_at IS NULL AND processing_error IS NULL` with no
  age gate, so it returns events that are legitimately in-flight (just
  enqueued, worker about to pick up), and the recovery cron races with
  healthy queue pickup.

## Solution

### Fix 1 — Scope the idempotency key to the dispatch attempt

Embed a per-tick scope so each scheduled run uses a fresh key namespace.
UTC minute precision is a good default — two cron firings within the same
minute still dedupe (legitimate), but successive runs always get fresh
execution rows so a previously failed Redis push cannot silently
short-circuit them.

```php
protected function currentUtcMinuteScope(): string
{
    return (new \DateTimeImmutable('now', new \DateTimeZone('UTC')))
        ->format('Y-m-d\\TH:i\\Z');
}

// In handle():
$tick = $this->currentUtcMinuteScope();
foreach ($events as $event) {
    $recoveryKey = 'webhook-recovery:' . $event->getId() . ':' . $tick;
    $jobDispatcher->dispatch($jobDef, $recoveryKey, null, null, [
        'webhookEventId' => $event->getId(),
    ]);
}
```

Make `currentUtcMinuteScope()` `protected` so tests can override it with
a fixed string for deterministic key assertions.

### Fix 2 — Staleness gate on the recovery query

Add a min-age filter to the repository method so the cron never races
with healthy in-flight events:

```php
public function findStrandedForRecovery(int $limit, int $minAgeSeconds): array
{
    $stmt = $this->db->prepare('
        SELECT * FROM ...
         WHERE processed_at IS NULL
           AND processing_error IS NULL
           AND received_at < (NOW() - INTERVAL :minAgeSeconds SECOND)
         ORDER BY received_at ASC, id ASC
         LIMIT :limit
    ');
    $stmt->bindValue(':minAgeSeconds', $minAgeSeconds, PDO::PARAM_INT);
    $stmt->bindValue(':limit', $limit, PDO::PARAM_INT);
    $stmt->execute();
    // ...
}
```

Keep the legacy un-gated `findUnprocessed` for manual CLIs / debugging
(operators running `--dry-run` first carry the safety the cron can't),
but the scheduled job must use the gated method.

Default `minAgeSeconds` should be longer than expected healthy queue lag —
300s (5 min) is reasonable for sub-second queue pickup. **Clamp
`<= 0` to default**, not just `< 0`: zero reopens the race.

### Fix 3 — Build the dispatcher only after the JobDefinition lookup

Resolve `JobDefinitionRepository::findByName(TargetJob::getName())` BEFORE
constructing the JobDispatcher. The dispatcher constructor connects to
Redis; a missing migration would otherwise be masked by a Redis connection
failure in production.

```php
public function handle(): JobResult
{
    $jobDef = $this->createJobDefRepo()->findByName(TargetJob::getName());
    if ($jobDef === null) {
        return JobResult::failure('TargetJob definition not found — migration missing?');
    }
    $events = $this->createEventRepo()->findStrandedForRecovery($limit, $minAge);
    if ($events === []) { return JobResult::success([...]); }
    $jobDispatcher = $this->createJobDispatcher(); // only now, only if work exists
    // ...
}
```

## Verification

Add a unit test that proves the recovery key is structurally distinct
from the ingest key AND embeds the per-tick scope:

```php
public function testRecoveryKeyIsDistinctAndIncludesPerTickScope(): void
{
    $job = new class extends MyRecoveryJob {
        protected function currentUtcMinuteScope(): string { return '2026-06-03T12:05Z'; }
    };
    // ... set test dependencies, capture the key passed to dispatcher->dispatch() ...
    $this->assertNotSame('webhook:77', $capturedKey);
    $this->assertSame('webhook-recovery:77:2026-06-03T12:05Z', $capturedKey);
}
```

Add a test that asserts the recovery query is the gated one:

```php
$this->eventRepo->expects($this->once())
    ->method('findStrandedForRecovery')
    ->with(50, 300)
    ->willReturn([]);
$this->eventRepo->expects($this->never())->method('findUnprocessed'); // regression guard
```

Add a test that asserts the dispatcher is NOT built when the JobDefinition
lookup returns null (subclass the SUT and increment a counter inside an
overridden `createJobDispatcher`).

## Example: spec-050 T18 (commit b3073560d)

`userfrosting/src/BuyerKiosk/Payroll/Jobs/ReprocessStrandedWebhooksJob.php`
applies all three fixes for the Everee webhook recovery path. The fix
landed AFTER an independent codex review of the first-pass implementation
flagged both the stale-execution-key bug and the in-flight-race bug as
CRITICAL — the original CLI at `bin/payroll/reprocess-webhooks.php` (T10 R2)
had the same latent bugs and was only safe because it was manual ops
tooling with `--dry-run` supervision.

## Residual gap (not closed by these fixes)

The target Process*Job (`ProcessEvereeWebhookJob` in this case) does not
do an atomic `SELECT ... FOR UPDATE` claim on the entity row before its
side effects. The staleness gate + per-tick key NARROW the
double-process window but a proper atomic claim is what fully closes it.
Plan for that as a separate follow-up when the recovery path becomes
hot enough that audit-row uniqueness matters.

## Notes

- This is a general TaskEngine pattern issue, not payroll-specific. Any
  recovery / retry job in this codebase using a fixed-id idempotency key
  against `JobDispatcher::dispatch()` will hit the same trap.
- The companion class of bugs to watch for: services that wrap the
  dispatcher and hard-code the key prefix (e.g.
  `JobDispatchingWebhookProcessor::dispatch()` uses `'webhook:' . $id`).
  Those are safe for first-time ingest because the execution row's
  lifecycle progresses past `queued` (so a re-attempt sees a different
  state and that path is handled differently), but UNSAFE if reused from
  a scheduled recovery context because a queued-but-never-pushed row
  looks identical to a queued-and-running row.
- For ANY tool using idempotency keys against an at-most-once dispatcher
  where the dispatch step can fail server-side: ask "what does the dedupe
  do on the second attempt if the first succeeded persistence but failed
  the side effect?" If the answer is "skips silently," scope the key.

## References

- `userfrosting/src/BuyerKiosk/TaskEngine/Application/JobDispatcher.php:61-113` —
  the dispatch implementation showing the row-then-push ordering and the
  return-existing-without-push branch.
- `userfrosting/src/BuyerKiosk/Payroll/Jobs/ReprocessStrandedWebhooksJob.php` —
  the fixed reference implementation (spec-050 commit b3073560d).
- `userfrosting/src/BuyerKiosk/Payroll/Repositories/PayrollWebhookEventRepository.php`
  — `findStrandedForRecovery` shows the staleness-gated query pattern next
  to the legacy `findUnprocessed`.