-
Notifications
You must be signed in to change notification settings - Fork 35
feat(redis): durable dequeue via claims for the sorted-set transport #412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Abhinav-kodes
wants to merge
29
commits into
llm-d:main
Choose a base branch
from
Abhinav-kodes:fix/404-claim-lease-ack
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 6 commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
833fc1f
feat(metrics): claim lifecycle metrics
Abhinav-kodes 76ba34b
feat(redis): durable dequeue via claims (#404)
Abhinav-kodes 346526e
feat(server): expose durable-dequeue tuning flags
Abhinav-kodes e3e7fc7
test(integration): durability scenarios for claim dequeue (#404)
Abhinav-kodes 285bc33
docs: durable dequeue guide
Abhinav-kodes ad74c82
docs: add release note fragment for 412
Abhinav-kodes 8871357
refactor(redis): shrink to minimal durable slice per review
Abhinav-kodes 40d4803
docs(redis): align durable-dequeue guide with shrunk slice, drop stal…
Abhinav-kodes 271a1da
style: gofmt claim/metrics whitespace
Abhinav-kodes 222b0a9
fix(redis): address blocking review items 1-5 small slice
Abhinav-kodes 0fb95b7
fix(redis): fence missing owners, generation-scoped handles, defer fl…
Abhinav-kodes 4ee2310
fix(redis): remove over-defensive :score and metrics for minimal slice
Abhinav-kodes 3c7edcf
fix(redis): drop originalScore param, :score already removed
Abhinav-kodes 5e1b4b2
fix(redis): release claim on retry/result persistence failure to avoi…
Abhinav-kodes 1079845
fix(redis): make Start context-cancellable so HardKill test is real
Abhinav-kodes d15a037
docs(redis): clarify token-guarded renew and generation-scoped map fo…
Abhinav-kodes fd2c355
fix(docs,redis): address stale :score doc and hard-kill test wiring
Abhinav-kodes 6778e59
test(integration): narrow HardKill to single claim expiry takeover
Abhinav-kodes 37951be
docs(release): update 412 note to fencing, not dedup collapse
Abhinav-kodes 243ef2f
test(integration): rename graceful_shutdown_loss_test.go -> claim_exp…
Abhinav-kodes 6db9397
docs(redis): clarify retry double trade-off wording
Abhinav-kodes 0ae3e2e
fix(server): omit claim defaults in compat shim, let Load apply them
Abhinav-kodes 93dc938
fix(redis): validate non-negative durations and batch sizes in Sorted…
Abhinav-kodes 9509245
fix(redis): require exact owner match in ackResultScript without empt…
Abhinav-kodes 382838e
test(redis): add sustained outage tests and clarify delivery docs
Abhinav-kodes 0b4da71
Merge branch 'upstream/main' into fix/404-claim-lease-ack
Abhinav-kodes 38331c5
test(redis): expand sustained outage and takeover tests; refine deliv…
Abhinav-kodes 1e6f586
fix(redis): key durable claim state by generation and ensure full flo…
Abhinav-kodes 30c1f5c
chore(redis): polish takeover test cleanup and claim key comments
Abhinav-kodes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| # Durable Dequeue (Claim / Lease / Ack) | ||
|
|
||
| Related: | ||
| - [Bug #404: Accepted Redis requests can be lost when an Async pod is hard-killed](https://github.com/llm-d/llm-d-async/issues/404) | ||
| - [batch-gateway #644: Async results can be lost with multiple Batch Processor replicas](https://github.com/llm-d/llm-d-batch-gateway/issues/644) (result-side counterpart) | ||
| - [batch-gateway #645: Resume in-progress batches after Processor pod or node loss](https://github.com/llm-d/llm-d-batch-gateway/issues/645) | ||
|
|
||
| ## The problem | ||
|
|
||
| The redis-sortedset transport used to dequeue with `ZPOPMIN`: the request was | ||
| removed from Redis before any processing happened. Between that pop and the | ||
| result being pushed back to Redis, the request existed only in process memory. | ||
| Any hard stop in that window — SIGKILL, OOM, node loss — silently lost every | ||
| accepted request it held. Graceful shutdown covered only the requests still | ||
| sitting on a single channel send. | ||
|
|
||
| ## The model | ||
|
|
||
| Dequeue is now **peek → claim → ack**: | ||
|
|
||
| 1. **Peek** — each poll reads up to `batch_size` entries with `ZRANGEBYSCORE` | ||
| (non-destructive). Nothing leaves the pending sorted set yet. | ||
| 2. **Claim** — for each entry that passes deadline/cancellation/gate checks, a | ||
| Lua script atomically moves it out of the pending set into claim | ||
| bookkeeping: | ||
| - `<queue>:claimed` — hash of the original member JSON (plus its original | ||
| sort score under a `<id>:score` field), | ||
| - `<queue>:claim-owners` — a random ownership token per claim, | ||
| - `<queue>:claims-idx` — zset of claims scored by lease expiry | ||
| (`min(claim_lease_ttl, deadline + 30s)`). | ||
| 3. **Process as before** — claimed requests flow through the same channels, | ||
| merge policy, and workers. No downstream change. | ||
| 4. **Ack** — when a terminal result is flushed, one Lua script pushes the | ||
| record to the result list, writes a `result-terminal:<id>` dedup marker, | ||
| and drops the claim — atomically. A crash between "inference done" and | ||
| "result written" therefore redelivers the request instead of losing it. | ||
|
|
||
| While a request is held, a background **heartbeater** renews its lease every | ||
| `claim_lease_ttl / 3` (clamped to 1s–30s), so slow-but-healthy inference is | ||
| never mistaken for a dead owner. The lease TTL is therefore the crash | ||
| *detection* window, not a processing-time budget. | ||
|
|
||
| Every exit path is paired with exactly one claim outcome: | ||
|
|
||
| | Path | Claim outcome | | ||
| |---|---| | ||
| | Result produced (success/error/cancelled/deadline/drop) | acked after the record is durably pushed | | ||
| | Request parked for retry | lease renewed; shielded from the shutdown sweep | | ||
| | Consumer context cancelled mid-hand-off | released back to pending at its original sort score | | ||
| | Graceful shutdown with unacked claims | swept back to pending immediately (except retry-owned) | | ||
| | Owner dies (lease expires) | reclaimer redelivers to pending; another instance picks it up | | ||
| | Gate refuses / gate error | no claim was taken; entry simply stays pending | | ||
|
|
||
| ## Delivery guarantees | ||
|
|
||
| - **At-least-once execution**: a request whose owner crashed is re-run by the | ||
| survivor. Expensive inference may execute twice across a failure. | ||
| - **Exactly-once terminal records**: the `result-terminal:<id>` marker makes | ||
| duplicate results collapse — only the first ack pushes a record; later ones | ||
| clean up their claim and no-op. Consumers observe one terminal record per | ||
| accepted request, keyed by the internal request ID (`custom_id` is | ||
| user-supplied and deliberately not used). | ||
| - Ordering within a queue remains earliest-deadline-first; release and | ||
| redelivery restore the original sort score. | ||
|
|
||
| ## Configuration | ||
|
|
||
| | Flag | Config JSON field | Default | Meaning | | ||
| |---|---|---|---| | ||
| | `--claim-lease-ttl` | `claim_lease_ttl_seconds` | `300` | Crash-detection window: how long a claim survives without a heartbeat before survivors redeliver the request. | | ||
| | `--claim-reclaim-interval` | `claim_reclaim_interval_ms` | `15000` | How often expired claims are scanned for redelivery. This bounds how long a crashed instance's work stalls. | | ||
| | `--result-dedup-ttl` | `result_dedup_ttl_seconds` | `21600` | Lifetime of per-request dedup markers; must exceed the longest possible redelivery chain. | | ||
|
|
||
| The heartbeat interval is derived (`lease TTL / 3`, clamped to 1s–30s) and is | ||
| not separately configurable. Flags apply to the redis-sortedset transport and | ||
| are ignored when `--transport-config`/`--transport-config-file` supplies its | ||
| own values. | ||
|
|
||
| Metrics: `async_claim_depth` (claimed per queue), `async_claims_expired_total` | ||
| (redeliveries — spikes indicate crashes or too-short leases), | ||
| `async_duplicate_results_suppressed_total` (duplicate records collapsed). | ||
|
|
||
| ## Operational requirements | ||
|
|
||
| - **Redis persistence is part of the durability contract.** Claims, dedup | ||
| markers, and queued requests all live in Redis; run it with AOF (`appendonly | ||
| yes`, e.g. `appendfsync everysec`) and/or replication. Without persistence a | ||
| Redis restart reintroduces a loss window this feature cannot close. | ||
| - Multiple Async replicas may share one queue: atomic claims prevent double | ||
| dispatch, and lease expiry hands work over automatically when a replica | ||
| disappears. | ||
| - Rolling upgrades are safe: pending members are unchanged, so old-version and | ||
| new-version pods can interleave during rollout (entries popped by old pods | ||
| do not get claim protection). | ||
|
|
||
| ## Known limitations | ||
|
|
||
| - Heartbeats are not token-guarded: in the rare window after a lease lapse | ||
| and takeover, the old owner's final heartbeat can extend the new owner's | ||
| claim by at most one TTL, slightly delaying the next reclaim. | ||
| - Graceful shutdown hands unacked claims back to pending but cannot return | ||
| requests held inside plugin goroutines that ignore context cancellation; | ||
| those wait out their lease like hard-kill losses. | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stale documentation here, since we removed it