fix(queue): preserve occurrence identity across recovery - #294
Conversation
f589d99
into
codex/pr198-producer-5a2-core-diagnostics
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8d911a10e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| this.parseEnvelope(raw) | ||
| } catch { | ||
| throw new Error('Queue retry occurrence is invalid') |
There was a problem hiding this comment.
During an upgrade from the parent version, the retry sorted set can contain pending raw job objects because the old retry() implementation stored JSON.stringify(nextJob) rather than an envelope. Rejecting every such member here leaves it permanently due; runtime.ts awaits promoteDueRetries() without recovery, so one legacy retry terminates the worker and causes the same failure after every restart. Accept legacy jobs for claim-time conversion or migrate them atomically before enforcing envelopes.
Useful? React with 👍 / 👎.
| queueRecoveryTimer = setInterval( | ||
| () => void recoverQueueWork(), | ||
| Math.min(stuckJobRecoveryMs, MAX_QUEUE_RECOVERY_INTERVAL_MS), |
There was a problem hiding this comment.
When processTask, processApproval, or processAnsweredQuestions runs longer than stuckJobRecoveryMs—15 minutes by default—this timer invokes recovery even though claim timestamps are never renewed. Recovery then removes the live claim and requeues its occurrence; the original terminal transition fails as stale, and this or another worker executes the same business operation again, potentially duplicating the local repository edits that Forge workers can apply by default. Add a cross-worker-safe lease heartbeat or otherwise prove owner liveness before periodic recovery.
AGENTS.md reference: AGENTS.md:L30-L35
Useful? React with 👍 / 👎.
| if (!shuttingDown) { | ||
| shuttingDown = true | ||
| clearWorkerTimers() | ||
| } | ||
| await done.catch(() => {}) |
There was a problem hiding this comment.
If Redis becomes unavailable while a claim, promotion, recovery, or transition command is pending, stop() can now wait forever because queue clients use maxRetriesPerRequest: null and are disconnected only after run() reaches its finally block. The previous implementation disconnected them from stop(), so SIGINT/SIGTERM could unblock the loop; preserve graceful in-flight completion, but impose a deadline followed by cancellation or disconnect.
Useful? React with 👍 / 👎.
| job = this.parse(raw) | ||
| } catch { | ||
| throw new Error('Queue recovery found an invalid occurrence envelope') | ||
| } |
There was a problem hiding this comment.
If an invalid ready payload reaches the processing list and the worker exits after BRPOPLPUSH but before its normal discard completes, this branch throws without removing or rotating that entry. Each periodic scan eventually encounters the same poison entry and aborts, leaving valid stale occurrences behind it stranded indefinitely. Quarantine or rotate the malformed entry while retaining fail-closed validation so recovery can continue.
Useful? React with 👍 / 👎.
1 participant
Source Issue
Issue: #179
Summary
This stacked child closes the queue occurrence, ownership, recovery, replay, and shutdown gaps identified in review of PR #293 and PR #294.
Stack
codex/pr198-producer-5a2-core-diagnostics724515723ba8460675ddb1c6bd1a323ab6809e05Review remediation
occurrenceId:claimNonce.already_applied.release()atomically returns an exact owned occurrence to ready, removes its claim marker, records a separate bounded release receipt for exact replay, and writes no acknowledgement receipt.stop()are released without task lookup, attempt creation, logs, or business processing. Existing in-flight drain behavior remains intact.Validation
npx vitest run __tests__/core-diagnostic-output-closure.test.ts— 14/14npx vitest run __tests__/queue-occurrence-recovery.redis.test.ts --reporter=verbose— 3/3, database empty before/afternpm run test:unit:zero-skip— 112 files passed and 2 environment-gated files skipped locally; 1,681 passed and 10 environment-gated tests skipped; the queue Redis file was executed separately 3/3npm run lint -- --max-warnings=0npx tsc --noEmitnpm run buildgit diff --checkReal-Redis markers are emitted only after their matching proof completes:
QUEUE_OCCURRENCE_REDIS_MULTIPLICITY_OKQUEUE_OCCURRENCE_REDIS_RECOVERY_OKQUEUE_OCCURRENCE_REDIS_SHUTDOWN_OKMutation evidence
Temporary local mutations, all restored before commit, proved the focused suite fails when:
Scope
Only the two queue/runtime implementation files, the existing focused closure test, and the narrow real-Redis queue proof are changed. Redis listeners, documentation, workflows, database schema, and parent review threads are untouched.