stream: consumer-group counters can exceed INT64_MAX and break clients decoding RESP integers as signed

Abierto
  • ¿Algo no está bien?
#3,579 1 comentario 1 reacción 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
4/5
Tiempo estimado
3-5 días
Aptitud para principiantes
45/100
Tipo de issue
Error
Claridad
Bastante claro
Estado de actividad
Tranquilo
Stack tecnológico
cpp, redis
Área
backend-api-design, databases

Línea de trabajo

Empieza en src/types/redis_stream.cc siguiendo las rutas de decremento de consumer-group indicadas y, después, inspecciona src/server/redis_reply.h y los serializadores de stream/group/consumer de XINFO. Confirma cómo obtiene XPENDING su summary count y compara los posibles enfoques de saturación y de tratamiento en el lado de emisión. Se considera terminado cuando los unsigned counters y las diferencias de timestamps restantes no puedan producir RESP integers fuera del signed 64-bit range, con el comportamiento acordado antes de la implementación.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

Summary

Several Redis-stream consumer-group counters are stored as uint64_t and emitted directly via redis::Integer (std::to_string, src/server/redis_reply.h). If any of them holds a value above INT64_MAX, the reply is a decimal like :18446744073709551615\r\n, which clients that decode RESP : integers as signed 64-bit (e.g. the Rust fred client) fail to parse — the connection is dropped/poisoned rather than returning group info.

PR #3578 fixes the two concrete instances we hit in production. This issue tracks the remaining, related hardening so it can be discussed before a broader patch (per the contributing guide's "discuss first / prefer focused patches").

Already addressed by #3578
  • XAUTOCLAIM deleting a dangling PEL entry without decrementing pending_number (counter drifts up, later wraps).
  • XINFO GROUPS lag wrapping when entries_read > entries_added (unsigned subtraction).
Remaining, same failure class

  1. Non-saturating pending_number decrements. After #3578, AutoClaim is the only saturating decrement; these still subtract unconditionally, so any invariant break wraps a client-visible uint64 counter (all src/types/redis_stream.cc):

    • DeleteConsumer (XGROUP DELCONSUMER): group.pending_number -= deleted_pel — cross-level (subtracts a consumer's count from the group's count), highest risk.
    • DeletePelEntries (XACK): group.pending_number -= *acknowledged and consumer.pending_number -= ack_count.
    • ClaimPelEntries (XCLAIM): original_consumer.pending_number -= 1.

  2. lag can exceed INT64_MAX even without a wrap. entries_added is settable to any uint64 via XSETID ... ENTRIESADDED n, so a correctly-computed lag = entries_added - entries_read can be a legitimate value > INT64_MAX that CheckLagValid passes through to the reply. #3578's guard only covers the entries_read > entries_added wrap.

  3. No emit-side clamp for already-corrupt data. #3578 stops future drift but doesn't repair or clamp existing values, and other unsigned fields are emitted raw: group/consumer pending_number (XINFO GROUPS/CONSUMERS), entries-added (XINFO STREAM), and the idle/inactive timestamp deltas (now - stored_ts, which wrap under clock skew / a future ts injected via XCLAIM). (The XPENDING summary count appears to be recomputed live from the PEL scan — worth confirming whether it reads the cached counter on any path.)

Suggested direction
  • Make all consumer-group counter decrements saturating (x >= n ? x - n : 0), matching #3578's AutoClaim decrement.
  • Either treat lag > INT64_MAX as unknown (nil, via the existing UINT64_MAX sentinel) in CheckLagValid, or add an emit-side clamp helper (min(v, INT64_MAX)) applied to the counter/timestamp serializations so the wire value is always within i64 for strict clients. Redis already treats these as signed, so clamping is behavior-compatible.

Happy to send a PR once there's agreement on the preferred shape (per-site saturation vs. a shared emit-side clamp).

AI assistance: the analysis behind this issue was done with AI help; I've reviewed and verified the findings against the source.

Lenguaje dominante
C++
Estrellas
4.5k
Forks
686
Merge medio
2 d 13 h
PR fusionados (30 d)
40

Preparar el entorno

Abrir en Codespaces

Inicia el contenedor de desarrollo del proyecto en tu navegador, con tu propia cuenta de GitHub.

  • Incluye un Dockerfile o un archivo de Docker Compose
  • Sin plantilla de pull request
  • Sin guía de contribución

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de apache/kvrocks

Todos los issues de apache/kvrocks

Issues similares

Más issues de C++

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.