Skip to content

fix: reject a legacy timer envelope whose body is not hex - #95

Merged
lesnik512 merged 2 commits into
mainfrom
fix/reject-corrupt-legacy-body
Sep 27, 2026
Merged

lesnik512 merged 2 commits into
mainfrom
fix/reject-corrupt-legacy-body

Conversation

@lesnik512

@lesnik512 lesnik512 commented Sep 27, 2026 •

Copy link
Copy Markdown
Member

Bug

TimerMessageFormat.parse reads the legacy v0.x envelope ({"b": "<hex>", "ct": "..."}). When b was not valid hex (or not a string at all), it caught the TypeError/ValueError from bytes.fromhex and returned an empty body b"". #94 added a test that pinned this.

That is data loss. The handler gets what looks like a valid message with an empty body. Under the default NACK_ON_ERROR policy a handler that returns normally acks it, the Timer is Committed, and the original payload is removed from Redis. Nothing ever says the payload was corrupt.

Can legacy envelopes still arrive?

Yes. 0.2.0 (7e9092c, "switch envelope to BinaryMessageFormatV1") changed the format. Releases 0.0.1 to 0.1.1 wrote the JSON-of-hex envelope. A Timer scheduled by one of those releases with a distant Activation time can still be in Redis and will be read by the current parser when it becomes Due. test_legacy_envelope_still_parses covers the valid case, and this PR leaves that path unchanged.

Fix

parse now raises ValueError("legacy timer envelope body is not hex"), chained from the original error. ValueError is what this package already raises for bad values (response.py, broker.py). FastStream has no dedicated decode-error exception, and its own BinaryMessageFormatV1.parse never raises; it falls back to the raw bytes. A HandlerException such as RejectMessage would be wrong here. Those are IgnoredExceptions, so the log middleware would skip them, and at parse time no StreamMessage exists yet for them to act on.

What happens to such a message now

In FastStream 0.7.7 the exception is raised from SubscriberUsecase.process_message (via is_suitable), before any handler runs:

  • The handler is not called.
  • The logging middleware logs ValueError: legacy timer envelope body is not hex at ERROR, with the traceback.
  • No ack, nack or reject happens because AcknowledgementMiddleware never received a message. The Timer is not Committed, and its payload stays in Redis.
  • The Lease expires, the Timer becomes Due again, and it is retried and logged again every lease_ttl. It stays there until an operator removes or repairs it, for example with cancel_timer.

I checked this against a real Redis with lease_ttl=1: two ERROR lines about a second apart, the handler never ran, and the payload was still stored afterwards.

Known gap: the ERROR line has no timer_id. The log context is only set after parsing succeeds, so an operator has to find the Timer by looking at Redis.

Tests (red, then green)

  • test_legacy_envelope_with_non_hex_body_parses_as_empty_body is replaced by test_legacy_envelope_with_non_hex_body_is_rejected, which is parametrized over "zz", 12 and null.
  • New integration test test_legacy_envelope_with_non_hex_body_never_reaches_handler: a corrupt legacy Timer that is already Due gets Claimed, never reaches the handler, and its payload is still stored.

Before the fix (tests changed, envelope.py untouched):

E       Failed: DID NOT RAISE ValueError      (x3)
E       AssertionError: assert [b''] == []
4 failed

After the fix: 4 passed.

Checks

  • just lint-ci: all checks passed (eof-fixer, ruff format, ruff check, ty)
  • just test-ci against redis:8: 175 passed, 100.00% coverage

Update: remove instead of retrying forever

Raising alone left a poison message: the parse error happens before FastStream has a message to acknowledge, so nothing removed the timer, and it was re-claimed and ERROR-logged every lease_ttl indefinitely, without its timer_id in the log.

The parser now treats an unparseable payload the way this package already treats reject(): it removes the timer through the same store.remove, logs one ERROR naming the timer, the topic and the payload size (not the payload, which may be large or sensitive), and re-raises so no handler runs.

  • The integration test is rewritten to assert the timer and its payload are gone after one delivery attempt and that exactly one removal line names 'old-timer'.
  • Red first: against the previous commit it failed on assert <score> is None (the timer was still in the timeline). Green: 175 passed, 100 % coverage; just lint-ci clean.

@lesnik512
lesnik512 merged commit 3f000ce into main Sep 27, 2026
13 checks passed
@lesnik512
lesnik512 deleted the fix/reject-corrupt-legacy-body branch September 27, 2026 17:12
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.

1 participant