Skip to content

feat(queue): add reliable Redis claims#73

Closed
abnegate wants to merge 4 commits into
mainfrom
dat-1898-reliable-claims
Closed

feat(queue): add reliable Redis claims#73
abnegate wants to merge 4 commits into
mainfrom
dat-1898-reliable-claims

Conversation

@abnegate

Copy link
Copy Markdown
Member

Summary

Adds opt-in reliable Redis queue claims for the DAT-1898 recovery path:

  • atomically claim messages into an in-flight lease set
  • heartbeat active claims and reclaim expired claims in bounded batches
  • commit, reject, and retry with immutable claim tokens
  • preserve claims across Swoole shutdown and bound prefetch
  • retain payload bytes exactly and recover oldest-first across batches
  • distinguish stale acknowledgements with a typed ClaimLost exception
  • reset transport connections after ambiguous failures without replaying mutating commands

This is the queue-library dependency slice of DAT-1898. Cloud queue wiring, package release/upgrade, separate gauge scheduling, and rollout observation remain follow-up gates; this PR does not deploy production.

Verification

  • host unit: 62 tests, 173 assertions
  • host Redis/Swoole end-to-end: 56 tests, 577 assertions
  • Linux unit: 62 tests, 173 assertions
  • Linux Redis/Swoole end-to-end: 55 tests, 566 assertions
  • Pint
  • PHPStan
  • Rector dry-run
  • package syntax and diff checks
  • independent exact-head review: no confirmed findings

The macOS test process was also investigated under LLDB: its exit 139 originates in the loaded Xdebug extension; the same suites exit cleanly with XDEBUG_MODE=off.

Related work

This is additive and intentionally avoids folding in the broader reconnect, KEDA sizing, or publisher-refactor work tracked by open PRs #34, #41, and #44.

Copilot AI review requested due to automatic review settings July 15, 2026 14:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@greptile-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces opt-in reliable Redis queue delivery for the DAT-1898 recovery path. Messages are atomically claimed into an in-flight lease sorted set, heartbeated by the Swoole adapter, and committed or rejected via immutable claimedAt tokens; a background recovery coroutine periodically scans for and reclaims expired leases.

  • New Lua scripts (Script.php) implement CLAIM, COMMIT, REJECT, RETRY, EXPIRED, RECLAIM, and EXTEND as atomic Redis operations, with defensive validation, stat tracking, and a quarantine list for corrupt entries.
  • Redis broker gains receiveReliable, commitReliable, rejectReliable, retryReliable, extend, expired, and reclaim, all routing through a command() helper that resets the shared connection on ambiguous failures without replaying mutating operations.
  • Swoole adapter adds a consumeReliable path that spawns a per-message heartbeat coroutine and a shared recovery coroutine, with correct WaitGroup/Channel lifecycle on coroutine-creation failure.

Confidence Score: 5/5

The change is additive and opt-in; existing non-reliable queue paths are unmodified and the Lua scripts are atomic with layered defensive validation.

All reliable-path operations use atomic Lua scripts that correctly validate state, manage stats, and handle corrupt entries via quarantine. The PHP broker resets connections on ambiguous failures without replaying mutating commands. The Swoole heartbeat and recovery coroutines manage WaitGroup and Channel lifecycles properly, including coroutine-creation failure fallbacks.

packages/queue/src/Queue/Broker/Redis/Script.php for the EXPIRED/RECLAIM ordering issues from prior threads; packages/queue/src/Queue/Broker/Redis.php for the getQueueSize atomic guard inconsistency.

Important Files Changed

Filename Overview
packages/queue/src/Queue/Broker/Redis/Script.php Seven Lua scripts implementing atomic claim lifecycle. EXPIRED uses ZREVRANGEBYSCORE (descending) and RECLAIM uses RPUSH (priority-tier placement) — both flagged in prior review threads. One minor concern: EXPIRED appends every scanned pid regardless of record validity, which can trigger an extra scan pass when corrupt entries fill a batch.
packages/queue/src/Queue/Broker/Redis.php Well-structured reliable path implementations. The command() helper correctly resets connections on ambiguous failures. getQueueSize() calls $this->atomic($this->commands) only for its LogicException side-effect but discards the return value — minor inconsistency.
packages/queue/src/Queue/Adapter/Swoole.php consumeReliable correctly manages heartbeat and recovery coroutines with proper WaitGroup/Channel lifecycle, including coroutine-creation failure paths.
packages/queue/src/Queue/Connection/Locking.php Correctly delegates evaluate() through the mutex synchronize() wrapper with a LogicException guard.
packages/queue/src/Queue/Option/Reliable.php Value object with full constructor validation: heartbeat < visibility, scan/batch > 0, poll bounds ordered and positive.

Reviews (2): Last reviewed commit: "(fix): recover reliable Redis claims saf..." | Re-trigger Greptile

public const string EXPIRED = <<<'LUA'
local now = redis.call('TIME')
local micros = (tonumber(now[1]) * 1000000) + tonumber(now[2])
local pids = redis.call('ZREVRANGEBYSCORE', KEYS[1], micros, '-inf', 'LIMIT', 0, tonumber(ARGV[1]))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 EXPIRED scan order contradicts "oldest-first" intent

ZREVRANGEBYSCORE sorts results descending by score, so the batch returned first contains the items with the highest leaseUntil value — i.e., the messages whose leases expired most recently. Messages that have been stuck the longest (lowest leaseUntil, expired earliest) are pushed to the tail of each scan batch and will wait the longest to be reclaimed. The PR description explicitly states "recover oldest-first across batches", which requires ascending order: ZRANGEBYSCORE KEYS[1] '-inf' micros 'LIMIT' 0 N.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/queue/src/Queue/Broker/Redis/Script.php
Line: 187

Comment:
**EXPIRED scan order contradicts "oldest-first" intent**

`ZREVRANGEBYSCORE` sorts results descending by score, so the batch returned first contains the items with the **highest** `leaseUntil` value — i.e., the messages whose leases expired most recently. Messages that have been stuck the longest (lowest `leaseUntil`, expired earliest) are pushed to the tail of each scan batch and will wait the longest to be reclaimed. The PR description explicitly states "recover oldest-first across batches", which requires ascending order: `ZRANGEBYSCORE KEYS[1] '-inf' micros 'LIMIT' 0 N`.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

.. ',"queue":' .. cjson.encode(replacement.queue)
.. ',"timestamp":' .. tostring(replacement.timestamp)
.. ',"payload":' .. record.payload .. '}'
redis.call('RPUSH', KEYS[3], encoded)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Reclaimed messages silently inherit priority-queue position

RPUSH places the reclaimed envelope at the right end of the pending list — the same end from which RPOP (in the CLAIM script) dequeues. This makes reclaimed messages indistinguishable from explicitly priority-flagged messages, so they will be picked up before any normal-priority messages that were enqueued before the reclaim happened. If a large number of claims expire simultaneously during an outage, repeated reclaims can starve the normal-priority backlog. The RETRY script uses LPUSH (normal priority) for symmetry — consider whether RECLAIM should do the same, or at least document that reclaimed messages are implicitly elevated to priority tier.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/queue/src/Queue/Broker/Redis/Script.php
Line: 268

Comment:
**Reclaimed messages silently inherit priority-queue position**

`RPUSH` places the reclaimed envelope at the right end of the pending list — the same end from which `RPOP` (in the `CLAIM` script) dequeues. This makes reclaimed messages indistinguishable from explicitly priority-flagged messages, so they will be picked up before any normal-priority messages that were enqueued before the reclaim happened. If a large number of claims expire simultaneously during an outage, repeated reclaims can starve the normal-priority backlog. The `RETRY` script uses `LPUSH` (normal priority) for symmetry — consider whether `RECLAIM` should do the same, or at least document that reclaimed messages are implicitly elevated to priority tier.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

@abnegate
abnegate marked this pull request as draft July 15, 2026 14:32

Copy link
Copy Markdown
Member Author

Pausing this PR as draft because DAT-1898 moved to Canceled before publication was reconciled with Linear ownership. I am not reopening the ticket or making further behavioral changes without an active owner.

Current exact-head state:

  • all GitHub Actions checks pass (benchmark intentionally skipped)
  • Greptile is 4/5
  • two current P2 threads remain unresolved

The first thread identifies an ambiguity in the "oldest-first" wording: ZREVRANGEBYSCORE plus RPUSH produces A→B→C only after a full bounded recovery drain, but live claims can interleave between the separate reclaim operations. The second is substantive: RPUSH gives every recovered message immediate dequeue precedence, so a repeatedly expiring message can starve normal work.

The smallest paired remediation for a future active owner is:

  1. scan expired claims with ascending ZRANGEBYSCORE
  2. requeue recovered claims at normal priority with LPUSH
  3. regress bounded A/B/C recovery with pending D/E as reclaim A/B/C and consume D/E/A/B/C
  4. add a poison-message fairness regression
  5. document oldest-first scanning and normal-priority requeue semantics

No production deployment or mutation was performed.

@abnegate
abnegate marked this pull request as ready for review July 16, 2026 08:41
@abnegate

Copy link
Copy Markdown
Member Author

Closing — this was the queue-library slice of DAT-1898 (StatsResources reliable Redis claims), and that approach was cancelled by the maintainer: the stats-resources queue isn't worth the extra claim/lease machinery; if it wedges again we flush it. DAT-1898 is Canceled in Linear and the cloud-side PR (appwrite-labs/cloud#4807) was closed for the same reason. No fault with the implementation — the requirement went away.

@abnegate abnegate closed this Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants