Skip to content

ftsb_redisearch: --pipeline >1 panics; pipelined TX/RX byte accounting is wrong #113

Description

@fcostaoliveira

Surfaced during adversarial review of #112 (the TX/RX label fix). These are pre-existing and independent of #112 — but they live in the same sendFlatCmd/sendIfRequired flush loop and should be fixed together. All are on the --pipeline N (N>1) path; the default pipeline=1 path is correct.

1. --pipeline >1 panics: index out of range (CRITICAL — pipeline mode is unusable)

connectionProcessor reassigns cmds/times from sendFlatCmd's return value, so they accumulate across rows until a flush at len >= pipeline. But replies is passed into sendFlatCmd and never returned — each call appends one element to its own local copy, so the caller's replies stays length 1 while cmds/times grow to pipeline. At flush, the loop for pos, t := range times { rcv := replies[pos] ... } indexes replies[pos] for pos >= 1runtime error: index out of range [1] with length 1, crashing the worker goroutine.

Reproduced: pipeline=3 → panic. So -pipeline >1 currently crashes for any worker that accumulates ≥2 commands.

Fix sketch: thread replies back out of sendFlatCmd (like cmds/times), or build the replies slice at the call site so its length matches times.

2. Per-command TxBytes is fabricated across a pipeline batch (HIGH)

sendIfRequired receives a single scalar txBytesCount (the flushing row's bytelen) and records it as Tx() for every command in the flush loop. So TxBytes becomes Σ_flushes(pipeline × bytelen_of_flushing_row) instead of the sum of each command's own bytes — arbitrarily wrong whenever row sizes vary within a pipeline window. The earlier rows' real sizes are discarded.

Fix sketch: thread a per-command []uint64 of TX sizes parallel to times, and record txSizes[pos] per entry.

3. RX is accumulated cumulatively but recorded per-entry (MEDIUM, latent)

rxBytesCount += getRxLen(rcv) runs inside the flush loop, so entry pos records the running prefix sum, not its own reply size. Dormant today because replies aren't captured (see 4), so getRxLen is always 0 — but real if reply capture is ever wired up.

4. RxBytes is structurally always 0: replies are never captured (MEDIUM)

sendFlatCmd declares var rcv interface{} (a nil interface, not a pointer) and passes it to radix.Cmd(rcv, ...), then appends that nil to replies. radix has no destination to unmarshal into, so the reply is read-and-discarded; getRxLen(nil) returns 0. Note this also means getRxLen's string/[]string branches are unreachable in production.

Making RxBytes meaningful is not a one-liner: it needs (a) a pointer/typed receiver (&rcv) with the replies plumbing from item 1, and (b) extending getRxLen to size the types radix actually returns ([]byte, []interface{}, int64) — it currently only handles string/[]string. (Command-level errors are NOT affected — radix returns top-level -ERR replies from client.Do regardless of the nil receiver, so error accounting is intact.)


Also noticed (separate subsystem, dormant — fold in or file separately)

cmd_processor.go emits/labels cursor reads as READ_CURSOR, but the histogram switch in benchmark_runner.go matches case "CURSOR_READ":. A READ_CURSOR op is therefore dropped from readCursorHistogram, so summary()'s per-label totalOps disagrees with the totalHistogram-based count. Dormant: no generator in the repo currently emits READ_CURSOR.


All confirmed by reading the code and reproducing item 1 with the real code path. None block #112 (which only corrects a metric label on the working pipeline=1 path).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions