Skip to content

fix(buffer): Hold filtered offsets while messages are buffered - #603

Open
muhammad-musa-ml wants to merge 1 commit into
getsentry:mainfrom
muhammad-musa-ml:fix-reduce-filtered-offsets
Open

muhammad-musa-ml wants to merge 1 commit into
getsentry:mainfrom
muhammad-musa-ml:fix-reduce-filtered-offsets

Conversation

@muhammad-musa-ml

Copy link
Copy Markdown

Fixes #602.

Buffer passed a filtered message to the next step even while earlier messages were still waiting in the buffer, so CommitOffsets could commit past messages that had not been flushed. The issue has the Snuba and Sentry chains and the broker's commit calls.

The offsets of a filtered message are now merged into the batch and go out with it. When Buffer holds no offsets, the message is forwarded at once, as before. The check uses the offsets Buffer holds and not the buffer's is_empty, because a buffer whose compute_batch_size returns 0 reports empty while it holds a message.

Three new tests in tests/processing/strategies/test_reduce.py; all three fail on main and pass here:

  • a filtered message is forwarded only when nothing is waiting, including a batch of size 0
  • filtered offsets are held until the batch is flushed, with two partitions
  • FilterStep -> Reduce -> RunTask -> CommitOffsets commits nothing before the batch is written

Run on Windows with Python 3.13.5 and confluent-kafka 2.16.1:

  • the suite without the Kafka-backed tests (no broker here) and without the multiprocessing tests: 131 passed, 6 skipped, 1 xfailed. The multiprocessing tests give 20 failed, 2 passed and 15 errors on main and with this change.
  • mypy . --strict: only the three signal.SIGCHLD errors that main also gives on Windows.
  • flake8, isort and black (22.3.0 and 26.10.1) are clean on the lines I changed. black would also reformat the flush_reason line in buffer.py on main; I left it.

With BatchStep followed by UnbatchStep, the held offset of a filtered message at the end of a batch is not forwarded, because Unfold uses the batch's committable only when the last message has none. The last commit is then the last accepted message, and the filtered tail is read again after a restart.

Buffer passed a filtered message straight to the next step, even when
earlier messages were still waiting in the buffer. With a FilterStep
that has a commit policy in front of a Reduce, CommitOffsets then
committed an offset past messages that had not been flushed yet. Snuba
does this when a prefilter is set (strategy_factory.py) and sentry's
last_seen_updater.py always does. A crash after that commit could lose
the buffered messages, and the later batch commit moved the offset
backwards.

The offsets of filtered messages are now merged into the batch and go
out with it. When nothing is waiting for a flush, the message is still
forwarded at once. The check uses the offsets the Buffer holds and not
the buffer's is_empty, because a buffer whose compute_batch_size
returns 0 reports empty while it holds a message.

All three new tests fail without the change.
@muhammad-musa-ml
muhammad-musa-ml requested review from a team as code owners November 9, 2026 17:54

This branch has not been deployed

No deployments
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.

Buffer forwards the offsets of filtered messages while earlier messages are still buffered

1 participant