chore(infra): swap Redis for Valkey in dev stack and integration tests by norbert-kulus-blockydevs · Pull Request #1396 · openlinker-project/openlinker

norbert-kulus-blockydevs · 2026-07-07T13:54:37Z

Summary

Retags the pinned redis:7-alpine image to valkey/valkey:8-alpine in docker-compose.yml (dev stack), libs/test-kit/src/containers.ts (shared Testcontainers harness default), and apps/worker/test/integration/harness.ts (worker-local Testcontainers harness).
Switches the docker-compose.yml healthcheck from redis-cli ping to valkey-cli ping.
No production deployment manifests exist in this repo, so this covers the full in-repo scope.

Why

DevOps asked whether Redis 7 can be swapped for Valkey (the open-source BSD fork). A compatibility audit of this codebase's actual Redis usage found no blockers: no Redis Stack modules (RedisJSON/RediSearch/TimeSeries/Bloom), no post-fork Redis 7.4+/8.0+-exclusive commands, full Streams (XADD/XGROUP/XREADGROUP/XACK) parity since the fork, and the one Lua script (compare-and-delete lock in redis-sync-lock.service.ts) uses only plain EVAL/GET/DEL.

Closes #1394

Test plan

pnpm lint — zero errors (pre-existing warnings unrelated to this change)
pnpm type-check — zero errors
pnpm test — unit suite passes (one pre-existing, unrelated failure in apps/web/src/pages/settings/settings-page.test.tsx reproduces identically on origin/main, confirmed via stash)
docker pull valkey/valkey:8-alpine succeeds locally
pnpm test:integration (Testcontainers-backed: event-bus Streams, sync-lock Lua script, shipping rolling-window sorted sets) — deferred to CI, not run locally in this session

Valkey is the open-source BSD-licensed fork of Redis; a compatibility audit found no Redis Stack modules, no post-fork exclusive commands, and full Streams/Lua parity in this codebase, so the pinned redis:7-alpine image is safe to retag across docker-compose, the shared test-kit Testcontainers harness, and the worker's integration-test harness. Closes #1394 Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

piotrswierzy

/pr-review — chore(infra): swap Redis for Valkey

Summary

Retags redis:7-alpine → valkey/valkey:8-alpine across the dev-stack compose file and both Testcontainers harnesses, and switches the compose healthcheck to valkey-cli ping. The compatibility audit is sound and the code surface is complete — but the PR's "full in-repo scope" claim missed three doc references. Approve with one change.

Verified

Compatibility audit holds — Valkey 8 forks Redis 7.2.4 and keeps full RESP + command parity: Streams (XADD/XGROUP/XREADGROUP/XACK), sorted sets (the shipping rolling-window), and plain EVAL/GET/DEL (the compare-and-delete sync lock) all work unchanged; the ioredis/redis node clients connect identically. No Redis Stack modules or 7.4+/8.0-exclusive commands in use. The audit's conclusion is correct. ✅
Code surface complete — docker-compose.yml (image + valkey-cli healthcheck), libs/test-kit/src/containers.ts DEFAULT_REDIS_IMAGE, apps/worker/test/integration/harness.ts RedisContainer(...), and the types.ts doc comment all swapped. ✅

🟡 IMPORTANT — stale image refs in docs

docs/testing-guide.md still documents redis:7-alpine in three places (lines ~174, ~218, ~903 — the Testcontainers "How It Works" snippet and two "Container Images" tables). The PR states "No production deployment manifests exist… this covers the full in-repo scope," but these doc refs now contradict what the harness actually starts. Update them to valkey/valkey:8-alpine (or "Redis-compatible (Valkey 8)") so the docs match the code.

(The bare redis-cli mentions in docs/webhooks/overview.md and the allegro plan are generic CLI examples — redis-cli still talks to Valkey fine, so those can stay.)

🟢 Suggestion

Integration tests (Streams / Lua lock / sorted sets) were deferred to CI, not run locally — reasonable given the parity analysis, but do confirm the CI integration job is green on this branch before merge, since that's the only place the Valkey image gets exercised against real Redis-command paths.

Verdict

🔄 Approve with changes — the swap itself is correct and complete in code; just sync the three docs/testing-guide.md image references so the "full in-repo scope" claim actually holds.

Sync docs/testing-guide.md with the Valkey swap - it still documented redis:7-alpine in three places after the compose file and Testcontainers harnesses moved to valkey/valkey:8-alpine. Addresses IMPORTANT finding from piotrswierzy's review on PR #1396. Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>

norbert-kulus-blockydevs · 2026-07-07T14:33:52Z

Pushed 3400ae4 addressing the IMPORTANT finding: updated the remaining redis:7-alpine references in docs/testing-guide.md (Testcontainers "How It Works" snippet + two "Container Images" tables) to valkey/valkey:8-alpine, so the docs match the compose file and Testcontainers harnesses.

Grepped the whole docs/ tree for stale Redis image tags to confirm nothing else was missed - only those three lines needed the swap. Bare redis-cli mentions elsewhere were left as-is per the review (generic CLI examples, still valid against Valkey).

On the suggestion (confirm CI integration job is green before merge): will check CI status before merging.

piotrswierzy · 2026-07-07T15:10:47Z

Blocked by: #1394 (comment)

piotrswierzy · 2026-07-07T15:28:22Z

@norbert-kulus-blockydevs Blocked by: #1394 (comment)

piotrswierzy · 2026-07-08T08:17:15Z

Holding off on a fresh review/approve here: this PR (Valkey swap) is a draft, and its whole approach is under active reconsideration on the linked issue. See #1394 comment — the case for a plain redis:7-alpine → redis:8-alpine bump instead (Redis 8 is AGPL/OSI-open again, lower-churn, no vocabulary drift, preserves Redis Stack optionality, fully reversible to Valkey later), unless DevOps's driver is an SSPL/AGPL license-scanner gate, a permissive-only policy, a hosted-service model, or a vendor-neutral-governance requirement — in which case Valkey (this PR) is the correct target and we should record that rationale.

Once DevOps confirms the driver on #1394, this either (a) gets repurposed to the smaller 7→8 bump, or (b) proceeds as-is with the real rationale documented. Either way the one gating check is pnpm test:integration green against the chosen image. Not blocking — just not approving a direction that's still being decided.

#2164) Every consumer read with `id: '>'` — never-delivered entries only — and the repo contained zero calls to XPENDING/XCLAIM/XAUTOCLAIM, so no code path ever read a Pending Entries List. A process killed between read and ACK therefore lost its in-flight message permanently, not temporarily. The comment at job-intake.consumer.ts asserted "message will be re-delivered after timeout"; Redis has no such timeout, which is why this went unnoticed. On the webhook path the loss was also invisible: `webhook_deliveries` is stamped `job_enqueued` before the ACK, so a dropped order was indistinguishable from a delivered one. Three primitives in `@openlinker/shared/redis`, wired into all three consumer groups (webhook-handler, master-deletion-offer-pause, job-intake): - Stable consumer identity via `resolveConsumerName` (OL_WORKER_ID, else hostname). `${prefix}-${process.pid}` was wrong in both directions: in a container PID is typically 1, so replicas collided on one PEL; outside one the name changed every restart, so a process could not reach its own history. This is the precondition for everything else. - Startup drain of own pending history before switching to new messages. - Periodic orphan reclaim for work stranded by a replica that never returned, with the idle threshold floored well above p99 handler duration — a reclaim that fires early steals live work and double-runs it. Built on XPENDING + XCLAIM + XRANGE rather than XREADGROUP/XAUTOCLAIM. This is load-bearing, not stylistic: node-redis v1.5.x transforms an XREADGROUP reply through `transformTuplesReply`, which calls `.length` on the field array, so an entry trimmed while its id remained in the PEL makes the client library throw a TypeError before any of our code runs — aborting the drain and leaving the id unackable, permanently blocking that consumer's recovery. XRANGE returns an empty array for a missing id, which is an answer rather than a crash. A trimmed entry is classified as its own outcome and ACKed, never routed into a handler's error path, where it would persist a bogus dead sync_jobs row or a dead-letter entry describing a failure that never happened. Also guards `persistDeadJob`'s unconditional `markDead`: drain and reclaim make redelivery real, so a redelivered message whose job has since run would otherwise flip a live row to 'dead'. Everything holds to the Redis 6.2 command floor, so #1396's Valkey swap stays a retag rather than a redesign. Covered by 20 unit tests and 9 Testcontainers integration tests, including the crash-then-restart case the issue names and the trimmed-PEL path. Closes #2164 Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com>

) OL has three "event" streams and zero fan-out: each has at most one consumer, each consumer does exactly one thing (turn the event into a job), and events.sync.jobs has no consumer at all. By the standard test — if the producer expects a specific outcome, it is a command wearing event clothing — all three are commands. The streams buy indirection, not decoupling. Meanwhile durability sits after four hops on the webhook path. Nine decisions, most of which are a decision NOT to build something, each with an observable reversal gate rather than an argued one: 1. The spine is the work row, written in the same transaction as the business change — so the outbox and the queue are the same table. 2. Build the contract; keep one transport. Do not build a general bus at zero fan-out. Gate: the first stream to acquire a second independent consumer. 3. If a bus: composite cursor plus a visibility barrier, never a scalar `id > cursor`. 4. eventId is derived from the business fact, never minted at insert. 5. No EntityManager in a core port signature. 6. Payload schemas are structurally incapable of carrying PII — redaction on write fails open, structural exclusion fails closed. 7. Registration-time catalog validation, not a central type union that would invert the infrastructure spine. 8. Nothing depends on a stream primitive above the Redis 6.2 floor, so #1396's Valkey swap stays a retag. 9. Redis is never the sole record of a fact — stated because it is currently false in at least three places. Numbered 049 per the #2162 reallocation: 046 was claimed by #2203 and 047 by reserved-numbers note to the #2166 branch that lands first. Also adds the docs/lessons.md entry for the commit-order gap that decision 3 rests on: an id is assigned before its transaction commits, so id order is not visibility order, and a reader advancing a scalar cursor past a gap never sees the earlier row again — silently and permanently. Closes #2165 Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com>

#2164) Every consumer read with `id: '>'` — never-delivered entries only — and the repo contained zero calls to XPENDING/XCLAIM/XAUTOCLAIM, so no code path ever read a Pending Entries List. A process killed between read and ACK therefore lost its in-flight message permanently, not temporarily. The comment at job-intake.consumer.ts asserted "message will be re-delivered after timeout"; Redis has no such timeout, which is why this went unnoticed. On the webhook path the loss was also invisible: `webhook_deliveries` is stamped `job_enqueued` before the ACK, so a dropped order was indistinguishable from a delivered one. Three primitives in `@openlinker/shared/redis`, wired into all three consumer groups (webhook-handler, master-deletion-offer-pause, job-intake): - Stable consumer identity via `resolveConsumerName` (OL_WORKER_ID, else hostname). `${prefix}-${process.pid}` was wrong in both directions: in a container PID is typically 1, so replicas collided on one PEL; outside one the name changed every restart, so a process could not reach its own history. This is the precondition for everything else. - Startup drain of own pending history before switching to new messages. - Periodic orphan reclaim for work stranded by a replica that never returned, with the idle threshold floored well above p99 handler duration — a reclaim that fires early steals live work and double-runs it. Built on XPENDING + XCLAIM + XRANGE rather than XREADGROUP/XAUTOCLAIM. This is load-bearing, not stylistic: node-redis v1.5.x transforms an XREADGROUP reply through `transformTuplesReply`, which calls `.length` on the field array, so an entry trimmed while its id remained in the PEL makes the client library throw a TypeError before any of our code runs — aborting the drain and leaving the id unackable, permanently blocking that consumer's recovery. XRANGE returns an empty array for a missing id, which is an answer rather than a crash. A trimmed entry is classified as its own outcome and ACKed, never routed into a handler's error path, where it would persist a bogus dead sync_jobs row or a dead-letter entry describing a failure that never happened. Also guards `persistDeadJob`'s unconditional `markDead`: drain and reclaim make redelivery real, so a redelivered message whose job has since run would otherwise flip a live row to 'dead'. Everything holds to the Redis 6.2 command floor, so #1396's Valkey swap stays a retag rather than a redesign. Covered by 20 unit tests and 9 Testcontainers integration tests, including the crash-then-restart case the issue names and the trimmed-PEL path. Closes #2164 Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com>

) OL has three "event" streams and zero fan-out: each has at most one consumer, each consumer does exactly one thing (turn the event into a job), and events.sync.jobs has no consumer at all. By the standard test — if the producer expects a specific outcome, it is a command wearing event clothing — all three are commands. The streams buy indirection, not decoupling. Meanwhile durability sits after four hops on the webhook path. Nine decisions, most of which are a decision NOT to build something, each with an observable reversal gate rather than an argued one: 1. The spine is the work row, written in the same transaction as the business change — so the outbox and the queue are the same table. 2. Build the contract; keep one transport. Do not build a general bus at zero fan-out. Gate: the first stream to acquire a second independent consumer. 3. If a bus: composite cursor plus a visibility barrier, never a scalar `id > cursor`. 4. eventId is derived from the business fact, never minted at insert. 5. No EntityManager in a core port signature. 6. Payload schemas are structurally incapable of carrying PII — redaction on write fails open, structural exclusion fails closed. 7. Registration-time catalog validation, not a central type union that would invert the infrastructure spine. 8. Nothing depends on a stream primitive above the Redis 6.2 floor, so #1396's Valkey swap stays a retag. 9. Redis is never the sole record of a fact — stated because it is currently false in at least three places. Numbered 049 per the #2162 reallocation: 046 was claimed by #2203 and 047 by reserved-numbers note to the #2166 branch that lands first. Also adds the docs/lessons.md entry for the commit-order gap that decision 3 rests on: an id is assigned before its transaction commits, so id order is not visibility order, and a reader advancing a scalar cursor past a gap never sees the earlier row again — silently and permanently. Closes #2165 Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com>

#2164) Every consumer read with `id: '>'` — never-delivered entries only — and the repo contained zero calls to XPENDING/XCLAIM/XAUTOCLAIM, so no code path ever read a Pending Entries List. A process killed between read and ACK therefore lost its in-flight message permanently, not temporarily. The comment at job-intake.consumer.ts asserted "message will be re-delivered after timeout"; Redis has no such timeout, which is why this went unnoticed. On the webhook path the loss was also invisible: `webhook_deliveries` is stamped `job_enqueued` before the ACK, so a dropped order was indistinguishable from a delivered one. Three primitives in `@openlinker/shared/redis`, wired into all three consumer groups (webhook-handler, master-deletion-offer-pause, job-intake): - Stable consumer identity via `resolveConsumerName` (OL_WORKER_ID, else hostname). `${prefix}-${process.pid}` was wrong in both directions: in a container PID is typically 1, so replicas collided on one PEL; outside one the name changed every restart, so a process could not reach its own history. This is the precondition for everything else. - Startup drain of own pending history before switching to new messages. - Periodic orphan reclaim for work stranded by a replica that never returned, with the idle threshold floored well above p99 handler duration — a reclaim that fires early steals live work and double-runs it. Built on XPENDING + XCLAIM + XRANGE rather than XREADGROUP/XAUTOCLAIM. This is load-bearing, not stylistic: node-redis v1.5.x transforms an XREADGROUP reply through `transformTuplesReply`, which calls `.length` on the field array, so an entry trimmed while its id remained in the PEL makes the client library throw a TypeError before any of our code runs — aborting the drain and leaving the id unackable, permanently blocking that consumer's recovery. XRANGE returns an empty array for a missing id, which is an answer rather than a crash. A trimmed entry is classified as its own outcome and ACKed, never routed into a handler's error path, where it would persist a bogus dead sync_jobs row or a dead-letter entry describing a failure that never happened. Also guards `persistDeadJob`'s unconditional `markDead`: drain and reclaim make redelivery real, so a redelivered message whose job has since run would otherwise flip a live row to 'dead'. Everything holds to the Redis 6.2 command floor, so #1396's Valkey swap stays a retag rather than a redesign. Covered by 20 unit tests and 9 Testcontainers integration tests, including the crash-then-restart case the issue names and the trimmed-PEL path. Closes #2164 Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com>

) OL has three "event" streams and zero fan-out: each has at most one consumer, each consumer does exactly one thing (turn the event into a job), and events.sync.jobs has no consumer at all. By the standard test — if the producer expects a specific outcome, it is a command wearing event clothing — all three are commands. The streams buy indirection, not decoupling. Meanwhile durability sits after four hops on the webhook path. Nine decisions, most of which are a decision NOT to build something, each with an observable reversal gate rather than an argued one: 1. The spine is the work row, written in the same transaction as the business change — so the outbox and the queue are the same table. 2. Build the contract; keep one transport. Do not build a general bus at zero fan-out. Gate: the first stream to acquire a second independent consumer. 3. If a bus: composite cursor plus a visibility barrier, never a scalar `id > cursor`. 4. eventId is derived from the business fact, never minted at insert. 5. No EntityManager in a core port signature. 6. Payload schemas are structurally incapable of carrying PII — redaction on write fails open, structural exclusion fails closed. 7. Registration-time catalog validation, not a central type union that would invert the infrastructure spine. 8. Nothing depends on a stream primitive above the Redis 6.2 floor, so #1396's Valkey swap stays a retag. 9. Redis is never the sole record of a fact — stated because it is currently false in at least three places. Numbered 049 per the #2162 reallocation: 046 was claimed by #2203 and 047 by reserved-numbers note to the #2166 branch that lands first. Also adds the docs/lessons.md entry for the commit-order gap that decision 3 rests on: an id is assigned before its transaction commits, so id order is not visibility order, and a reader advancing a scalar cursor past a gap never sees the earlier row again — silently and permanently. Closes #2165 Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com>

…K + ADR-049 durability spine (#2223) * fix(core): recover Redis stream messages stranded between read and ACK (#2164) Every consumer read with `id: '>'` — never-delivered entries only — and the repo contained zero calls to XPENDING/XCLAIM/XAUTOCLAIM, so no code path ever read a Pending Entries List. A process killed between read and ACK therefore lost its in-flight message permanently, not temporarily. The comment at job-intake.consumer.ts asserted "message will be re-delivered after timeout"; Redis has no such timeout, which is why this went unnoticed. On the webhook path the loss was also invisible: `webhook_deliveries` is stamped `job_enqueued` before the ACK, so a dropped order was indistinguishable from a delivered one. Three primitives in `@openlinker/shared/redis`, wired into all three consumer groups (webhook-handler, master-deletion-offer-pause, job-intake): - Stable consumer identity via `resolveConsumerName` (OL_WORKER_ID, else hostname). `${prefix}-${process.pid}` was wrong in both directions: in a container PID is typically 1, so replicas collided on one PEL; outside one the name changed every restart, so a process could not reach its own history. This is the precondition for everything else. - Startup drain of own pending history before switching to new messages. - Periodic orphan reclaim for work stranded by a replica that never returned, with the idle threshold floored well above p99 handler duration — a reclaim that fires early steals live work and double-runs it. Built on XPENDING + XCLAIM + XRANGE rather than XREADGROUP/XAUTOCLAIM. This is load-bearing, not stylistic: node-redis v1.5.x transforms an XREADGROUP reply through `transformTuplesReply`, which calls `.length` on the field array, so an entry trimmed while its id remained in the PEL makes the client library throw a TypeError before any of our code runs — aborting the drain and leaving the id unackable, permanently blocking that consumer's recovery. XRANGE returns an empty array for a missing id, which is an answer rather than a crash. A trimmed entry is classified as its own outcome and ACKed, never routed into a handler's error path, where it would persist a bogus dead sync_jobs row or a dead-letter entry describing a failure that never happened. Also guards `persistDeadJob`'s unconditional `markDead`: drain and reclaim make redelivery real, so a redelivered message whose job has since run would otherwise flip a live row to 'dead'. Everything holds to the Redis 6.2 command floor, so #1396's Valkey swap stays a retag rather than a redesign. Covered by 20 unit tests and 9 Testcontainers integration tests, including the crash-then-restart case the issue names and the trimmed-PEL path. Closes #2164 Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * docs(adr): ADR-049 durability spine and the domain-event contract (#2165) OL has three "event" streams and zero fan-out: each has at most one consumer, each consumer does exactly one thing (turn the event into a job), and events.sync.jobs has no consumer at all. By the standard test — if the producer expects a specific outcome, it is a command wearing event clothing — all three are commands. The streams buy indirection, not decoupling. Meanwhile durability sits after four hops on the webhook path. Nine decisions, most of which are a decision NOT to build something, each with an observable reversal gate rather than an argued one: 1. The spine is the work row, written in the same transaction as the business change — so the outbox and the queue are the same table. 2. Build the contract; keep one transport. Do not build a general bus at zero fan-out. Gate: the first stream to acquire a second independent consumer. 3. If a bus: composite cursor plus a visibility barrier, never a scalar `id > cursor`. 4. eventId is derived from the business fact, never minted at insert. 5. No EntityManager in a core port signature. 6. Payload schemas are structurally incapable of carrying PII — redaction on write fails open, structural exclusion fails closed. 7. Registration-time catalog validation, not a central type union that would invert the infrastructure spine. 8. Nothing depends on a stream primitive above the Redis 6.2 floor, so #1396's Valkey swap stays a retag. 9. Redis is never the sole record of a fact — stated because it is currently false in at least three places. Numbered 049 per the #2162 reallocation: 046 was claimed by #2203 and 047 by reserved-numbers note to the #2166 branch that lands first. Also adds the docs/lessons.md entry for the commit-order gap that decision 3 rests on: an id is assigned before its transaction commits, so id order is not visibility order, and a reader advancing a scalar cursor past a gap never sees the earlier row again — silently and permanently. Closes #2165 Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * fix(core): trust the XCLAIM reply and run reclaim on an idle stream (#2164) Review follow-ups on the recovery primitives. Two were real defects. reclaimOrphans discarded the XCLAIM reply and re-read the body with XRANGE. XCLAIM without FORCE cannot resurrect a non-pending id, so there was no PEL leak — but the opposite failure existed: when a claim legitimately does not transfer (the owner ACKed between the XPENDING and the XCLAIM, or touched the entry so it is no longer idle past the threshold), XRANGE still returns the body, because ACK removes an entry from the PEL but not from the stream. The code then processed and ACKed a message belonging to another live consumer — defeating the very re-assertion the comment claimed to make. The claim reply is now the source of truth: node-redis returns one element per requested id and null where the claim did not transfer, so a non-transfer is either a lost race (skip; not ours) or a trimmed entry (report so the caller clears the dangling id), disambiguated by a single XRANGE. The unit spec previously mocked xClaim as resolving undefined while asserting an entry came back — it encoded the bug — and now pins the correct behaviour, with an integration test covering the owner-ACKed-first race against real Redis. maybeReclaimOrphans sat after the loop's empty-batch `continue` in all three consumers, so it only ran when a batch arrived. Orphans are reclaimed precisely when a stream is quiet — events.master.deletion can sit empty for days — so the feature was dead code in the case it exists for. Moved above the check; the existing lastReclaimAt throttle already prevents it firing every block timeout. Also: - The periodic pass now re-drains own pending before the orphan sweep. A handler that throws leaves its entry un-ACKed in this consumer's PEL, and the orphan sweep deliberately skips self-owned rows, so a single transient failure previously stranded a message until the process restarted. - The webhook handler's recovery path now tracks inFlightMessage and honours the abort flag, as the batch path does. Without it a shutdown mid-recovery returned from stopConsumptionLoop immediately and onModuleDestroy quit the client out from under a message still being processed — the regression #1923 fixed, reintroduced on a new path. - The startup drain is bounded by MAX_DRAIN_PAGES and bails on abort. Its termination previously depended on an ACK-or-throw invariant held across three files with no test; a future non-ACKing branch would have hung onModuleInit and with it application boot. - RECLAIM_INTERVAL_MS moved to the shared module rather than being redeclared per consumer. - Corrected docs and comments that described XAUTOCLAIM and an `id: '0'` read. Neither is used, and the module header explains why — an ADR whose purpose is audit-trail accuracy must not misstate its own implementation. - Documented OL_WORKER_ID in both .env.example files: it is a correctness-relevant knob (two replicas sharing a value share one PEL) that existed only in the plan. Rebased onto current main. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * fix(core): stop one poison entry blocking recovery of every stranded sibling (#2164) Review findings on the recovery loops. The first is a real defect in this PR. A HANDLER THROW ABORTED THE WHOLE RECOVERY PASS, PERMANENTLY. `handleRecoveredEntry` delegates to `processMessage`, which rethrows on any non-decode error, and the drain's try/catch spanned the entire page loop — so a single entry whose handler threw aborted the pass and returned. Because `readOwnPending` always pages the PEL from the oldest id, that same entry then led every later drain and every reclaim, and it was never ACKed, so it stayed first forever. One poison message permanently starved recovery of every other stranded message: precisely the failure this recovery path exists to prevent. Each entry now runs inside its own try/catch (`recoverEntrySafely`), so a handler failure logs and the pass continues to the next entry. The failing entry stays un-ACKed and is retried on a later pass; what no longer happens is its siblings being blocked behind it. The outer catch is kept for genuine Redis-level failures. Applied to all three consumers and to both the drain and the reclaim path. REPEATED DELIVERY IS NOW REACHABLE, SO IT NEEDS A SIGNAL. Before #2164 a failing entry was simply never redelivered, so no terminal state was needed; recovery is what makes unbounded retry possible. `XPENDING` already reports a delivery counter and `toPendingRows` was discarding it. It is now surfaced as `PendingRow.deliveryCount`, threaded onto `StreamEntry`, and compared against `MAX_DELIVERY_ATTEMPTS` — crossing it logs at `error` naming the entry as needing intervention. The constant is deliberately generous: it is a terminal-state backstop, not a retry budget, so a handler failing on a transient blip is still allowed to succeed later. A missing counter defaults to 1, never 0, so a reply-shape change cannot make an entry read as never delivered. Auto-dead-lettering is deliberately NOT done, and the reason is recorded in ADR-049 with a reversal gate: two of the three consumers cannot construct their dead-letter payload from a raw pending entry (the webhook handler needs a decoded event, job-intake a parsed job request), and discarding the entry instead would be unrecoverable loss. With per-entry isolation in place the absence of a terminal state no longer blocks anything — it is noise plus a missing alarm, and the alarm now exists. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * fix(core): make recovery terminate, and alarm on a counter that can actually move (#2164) Review findings on the previous commit. Both are defects it introduced. THE DRAIN NO LONGER TERMINATED, AND onModuleInit AWAITS IT. Isolating a handler failure per entry removed the only thing that ended the startup drain: previously the throw escaped to the outer catch and returned. `readOwnPending` always paged the PEL from '-', and a failing entry is never ACKed, so the same page came back every iteration and `entries.length === 0` became unreachable. The abort escape does not help — `abortController` is created by `startConsumptionLoop()`, which runs *after* the awaited drain. So one poison entry meant MAX_DRAIN_PAGES (1000) iterations of up to COUNT (10) handler invocations plus their Redis round-trips, blocking Nest bootstrap, and `drained` reported ~10,000 recovered messages for one stuck entry. The drain now pages forward with an exclusive `(<id>` cursor (Redis 6.2, the same floor XPENDING sits on), so a pass visits each entry once and terminates naturally. A failed entry stays pending for the next recovery pass rather than being re-attempted inside the current one. THE POISON ALARM COULD NEVER FIRE WHERE POISON LIVES. The previous commit keyed the alarm on Redis' `deliveriesCounter`. Redis increments that on delivery — XREADGROUP and XCLAIM — never on XPENDING or XRANGE, which is all the drain path uses. Re-presenting an entry a thousand times therefore left the counter at 1, so the branch was unreachable on exactly the path where a stuck handler accumulates. The reclaim path increments once, then `owner !== consumer` excludes the entry from every later pass, freezing it at 2. Reaching the threshold needed eleven distinct consumer identities each abandoning the entry — not a thing that happens on a stable-hostname deployment, which is the point of `resolveConsumerName`. Worse, the integration test added alongside it asserted the counter rises by calling XCLAIM directly. True, and irrelevant to the drain — it would have gone on passing while production never incremented. Counting is now local: a per-consumer `RecoveryAttemptTracker` keyed by entry id, incremented on the failure path, cleared on success so a transient failure does not leave an entry permanently near the threshold. It fires once, on the crossing, because a poison entry recurs by definition and an unguarded alarm per pass is fatigue on the channel meant to carry real incidents. Redis' `deliveryCount` is kept as diagnostic context — genuinely useful for cross-replica churn — with its limits and its reclaim-path off-by-one documented on the field. Also: - `recoverEntrySafely` rethrows when the abort signal is set, so a shutdown-time failure ends the pass instead of grinding the rest of the page against a quitting client. - The head-of-line fix is now pinned by integration tests, which is what the review said was missing and what would have caught the non-termination above: a page whose first entry is never ACKed still reaches its siblings; the scan ends rather than re-reading; and Redis' own counter is asserted frozen across drain re-reads, which is the evidence for counting locally. - ADR-049's known-gap paragraph corrected — it presented the delivery-count alarm as the compensating control. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * fix(core): stop the recovery log over-reporting, and correct the reclaim delivery count (#2164) Two remaining review findings. `drained += entries.length` counted entries ATTEMPTED, not entries handled. The exclusive-cursor fix removed the 1000x inflation, but a pass that failed on three of a ten-entry page still logged "Recovered 10 pending message(s)". That line is read by an operator during the incident it is describing, so over-reporting there is the same class of dishonesty this epic exists to remove. `recoverEntrySafely` now reports whether the entry was handled and the drain counts only successes. The reclaim path's `reclaimed` count is deliberately left as-is: it reports what `reclaimOrphans` actually did — take ownership of N entries — which is true independently of whether handling then succeeded. `deliveryCount` on the reclaim path was stale by exactly one: the XPENDING listing is read before that pass's own XCLAIM, and XCLAIM is itself a delivery. Now compensated at the point of construction rather than only described in the field's doc comment. Still open, deliberately, and listed so they are not mistaken for oversights: the `trimmed` check is expressed in two places (`resolvePendingEntry` and the reclaim branch), `recoverEntrySafely` is triplicated across the three consumers (the attempt counter it depends on is already single-source in `RecoveryAttemptTracker`, which was the load-bearing part), and the two shared Redis modules colocate their types rather than using `*.types.ts` — a deviation from the written standard that matches roughly half of `libs/shared` today. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * fix(core): report recovery honestly, and hold the shutdown guarantee the code claimed (#2164) Six review findings. Two are defects in the immediately preceding commit; one contradicts a claim that commit's own message made. A DISCARDED ENTRY WAS COUNTED AND LOGGED AS RECOVERED. The previous commit made the drain count successes instead of attempts, using a boolean. A `trimmed` entry ACKs successfully — its payload is gone, destroyed by retention — so it returned true and was counted. A boot finding ten stranded webhook events whose payloads had all been trimmed logged "Recovered 10 pending webhook event(s)". That is worse than the over-count it replaced: it reports permanent loss as successful recovery, on the path where lost work is a dropped order whose delivery row still reads `published`. A boolean cannot express three outcomes, which is why it was wrong on first use. `recoverEntrySafely` now returns `RecoveryOutcome` — recovered / discarded / failed — and the drain counts each, reporting discards separately and at warn. `reclaimed` COUNTED ENTRIES THAT WERE NEVER CLAIMED. The previous commit deliberately left this alone and justified it: "it reports what `reclaimOrphans` actually did — take ownership of N entries". That is false against the code. `reclaimOrphans` also returns a `trimmed` entry on the path where XCLAIM did NOT transfer and XRANGE then found the data gone; no ownership was taken there. Now counts only entries whose claim actually transferred. THE SHUTDOWN GUARDS WERE BOTH DEAD ON THE PATH THAT NEEDED THEM. `onModuleInit` awaits the drain and only then calls `startConsumptionLoop`, which is where `abortController` was created. So during the startup drain — the one pass that can run for many pages — both the loop's abort check and `recoverEntrySafely`'s shutdown rethrow were reading an undefined controller and could never fire. A shutdown arriving mid-drain was invisible to it, which is exactly what the rethrow's comment says it prevents. The controller is now created before the drain, and `startConsumptionLoop` replaces it only when a previous run aborted (the restart-after-backoff path). Also: - `pending-retry` still had the head-of-line defect the drain was fixed for: it re-read the oldest COUNT ids from '-' every tick with no cursor, so poison at the head starved every later own-pending entry for the process lifetime. Now paged with the same exclusive cursor, capped per tick so recovery cannot monopolise the consume loop. - The attempt tracker evicted the id that had been stuck LONGEST, because a Map `set` on an existing key does not reorder. Now delete-then-set, making eviction least-recently-failed. - The alarm fired at MAX_RECOVERY_ATTEMPTS + 1 while the constant is documented as the threshold. Now fires at the documented value. - Redis' `deliveryCount` had no production reader; it is now in the failure log line, which is where an operator distinguishing cross-replica churn from a locally-stuck handler would look. - The frozen-counter integration assertion compared the readings to themselves, so a server reporting 0 every time would have passed while breaking the "defaults to 1, never 0" invariant. Now asserts the absolute value. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> * test(worker): pin the recovery-outcome contract that reported loss as recovery (#2164) The previous round fixed the defect but not the gap that let it ship. The review's finding was specifically that the drain's counting logic has no test — "shipping it with no test means B1 was undetectable and a future regression back to \`drained += entries.length\` is equally undetectable" — and the tracker unit tests added instead cover a different thing. Four cases on the consumer itself: a processed entry reports 'recovered', a trimmed entry reports 'discarded' (never 'recovered' — its payload is gone), a throwing handler reports 'failed' rather than propagating and aborting the pass for its siblings, and a shutdown-time failure rethrows instead of being swallowed as a handler error. Verified by injection rather than assumed: reverting the trimmed branch to return 'recovered' fails the second case, and only that case. A test that cannot fail is the same mistake as the self-referential assertion this round already corrected. Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com> --------- Signed-off-by: Piotr Swierzy <piotr.swierzy@blockydevs.com>

piotrswierzy · 2026-08-21T21:47:59Z

Closing this draft as stale — no reflection on the work, which the audit in the description holds up.

Context for anyone picking it up again: the async-work-layer epic (#2162) has since landed ADR-049/050/051 and Waves 0–5, which touched every Redis consumer in the tree. That work was written deliberately against the Redis 6.2 command floor — the recovery paths use XPENDING / XCLAIM / XRANGE only, and RedisStreamsEventPublisher writes through a bounded xAdd wrapper — precisely so that a Valkey swap stays a retag rather than a redesign. So the compatibility conclusion here is, if anything, better supported now than when this was opened.

What changed underneath it: the streams gained declared retention bounds and an explicit --maxmemory / --maxmemory-policy noeviction startup (#2163), PEL recovery was added (#2164), and the webhook path stopped using a stream consumer entirely (#2280). The three-file retag in this PR is therefore likely still close to correct, but the integration surface it needs to prove — which was already deferred to CI here and never run — is materially larger than it was in July.

Reopen or re-cut against current main whenever the swap is worth scheduling; docs/operations/redis-stream-retention.md is the relevant new reading.

norbert-kulus-blockydevs requested a review from piotrswierzy as a code owner July 7, 2026 13:54

piotrswierzy reviewed Jul 7, 2026

View reviewed changes

piotrswierzy mentioned this pull request Jul 7, 2026

[TASK] Infrastructure — Swap Redis 7 for Valkey 8/9 in dev stack and Testcontainers #1394

Open

6 tasks

norbert-kulus-blockydevs marked this pull request as draft July 7, 2026 19:20

piotrswierzy mentioned this pull request Jul 29, 2026

fix(infra): align demo Dockerfiles/compose with SysOps container standards #1915

Merged

6 tasks

norbert-kulus-blockydevs mentioned this pull request Aug 14, 2026

docs(orders,currency): order-time FX rate snapshot + reporting-currency stamping #2050

Merged

piotrswierzy closed this Aug 21, 2026

Navigation Menu

Uh oh!

chore(infra): swap Redis for Valkey in dev stack and integration tests - #1396

chore(infra): swap Redis for Valkey in dev stack and integration tests#1396
norbert-kulus-blockydevs wants to merge 2 commits into
mainfrom
1394-swap-redis-valkey

norbert-kulus-blockydevs commented Jul 7, 2026

Uh oh!

piotrswierzy left a comment

Uh oh!

norbert-kulus-blockydevs commented Jul 7, 2026

Uh oh!

piotrswierzy commented Jul 7, 2026

Uh oh!

piotrswierzy commented Jul 7, 2026

Uh oh!

piotrswierzy commented Jul 8, 2026

Uh oh!

piotrswierzy commented Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

norbert-kulus-blockydevs commented Jul 7, 2026

Summary

Why

Test plan

Uh oh!

piotrswierzy left a comment

Choose a reason for hiding this comment

/pr-review — chore(infra): swap Redis for Valkey

Summary

Verified

🟡 IMPORTANT — stale image refs in docs

🟢 Suggestion

Verdict

Uh oh!

norbert-kulus-blockydevs commented Jul 7, 2026

Uh oh!

piotrswierzy commented Jul 7, 2026

Uh oh!

piotrswierzy commented Jul 7, 2026

Uh oh!

piotrswierzy commented Jul 8, 2026

Uh oh!

piotrswierzy commented Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants