fix: reject a legacy timer envelope whose body is not hex - #95
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
TimerMessageFormat.parsereads the legacy v0.x envelope ({"b": "<hex>", "ct": "..."}). Whenbwas not valid hex (or not a string at all), it caught theTypeError/ValueErrorfrombytes.fromhexand returned an empty bodyb"". #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_ERRORpolicy 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_parsescovers the valid case, and this PR leaves that path unchanged.Fix
parsenow raisesValueError("legacy timer envelope body is not hex"), chained from the original error.ValueErroris what this package already raises for bad values (response.py,broker.py). FastStream has no dedicated decode-error exception, and its ownBinaryMessageFormatV1.parsenever raises; it falls back to the raw bytes. AHandlerExceptionsuch asRejectMessagewould be wrong here. Those areIgnoredExceptions, so the log middleware would skip them, and at parse time noStreamMessageexists 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(viais_suitable), before any handler runs:ValueError: legacy timer envelope body is not hexat ERROR, with the traceback.AcknowledgementMiddlewarenever received a message. The Timer is not Committed, and its payload stays in Redis.lease_ttl. It stays there until an operator removes or repairs it, for example withcancel_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_bodyis replaced bytest_legacy_envelope_with_non_hex_body_is_rejected, which is parametrized over"zz",12andnull.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.pyuntouched):After the fix:
4 passed.Checks
just lint-ci: all checks passed (eof-fixer, ruff format, ruff check, ty)just test-ciagainstredis:8: 175 passed, 100.00% coverageUpdate: 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_ttlindefinitely, without itstimer_idin the log.The parser now treats an unparseable payload the way this package already treats
reject(): it removes the timer through the samestore.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.'old-timer'.assert <score> is None(the timer was still in the timeline). Green: 175 passed, 100 % coverage;just lint-ciclean.