Enhance service bus handling of invalid JSON in receive_message function - #4932
Enhance service bus handling of invalid JSON in receive_message function#4932JC-wk wants to merge 11 commits into
Conversation
…ovide fix for the infinite looped service bus issue
There was a problem hiding this comment.
Pull request overview
Improves the resource processor’s Service Bus session receiver loop to tolerate malformed/non-JSON messages by dead-lettering them and continuing processing, instead of crashing the runner.
Changes:
- Dead-letter malformed Service Bus messages on
json.JSONDecodeErrorand skip further processing of that message. - Add a unit test covering the malformed JSON path in
receive_message. - Bump
resource_processorversion and add an Unreleased changelog entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| resource_processor/vmss_porter/runner.py | Dead-letters invalid JSON messages and continues the receive loop. |
| resource_processor/tests_rp/test_runner.py | Adds a unit test to verify invalid JSON messages are dead-lettered and not completed. |
| resource_processor/_version.py | Patch version bump. |
| CHANGELOG.md | Adds an Unreleased BUG FIXES entry for the change. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Unit Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 7905301. ♻️ This comment has been updated with latest results. |
|
/test 59a309a |
|
🤖 pr-bot 🤖 🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/27306698867 (with refid (in response to this comment from @rudolphjacksonm) |
|
/test e7218a4 |
|
🤖 pr-bot 🤖 🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/28122108599 (with refid (in response to this comment from @rudolphjacksonm) |
marrobi
left a comment
There was a problem hiding this comment.
@JC-wk Opus 4.8 review, let me kniw if agree:
A couple of things before merge:
-
(Optional) Guard the dead-letter call. If a message is already in a state where
dead_letter_messagecan fail (e.g. lock lost / already settled), the raised exception would bubble to the outerexcept Exceptionand we'd re-enter the same situation. Consider wrapping the dead-letter in its own try/except so a settle failure is logged rather than aborting the batch, e.g.:except json.JSONDecodeError as e: logger.error(f"Received bad service bus resource request message: {e}") try: await receiver.dead_letter_message(msg, reason="InvalidJSON", error_description=str(e)) except Exception: logger.exception("Failed to dead-letter malformed message") continue
-
(Nit) Test brittleness.
test_receive_message_bad_jsonasserts the exact exception string"Expecting value: line 1 column 1 (char 0)". That text comes from the stdlib and could change across Python versions. Asserting onreason="InvalidJSON"(and maybe thaterror_descriptionis non-empty) would be more robust, but this is minor.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
CHANGELOG.md:20
- The changelog entry references PR #4932, but this PR is described as resolving issue #4976. The link/number should match the actual issue/PR for traceability.
* Fix to enhance service bus handling of invalid JSON in receive_message function ([#4932](https://github.com/microsoft/AzureTRE/pull/4932))
resource_processor/vmss_porter/runner.py:76
json.loadscan succeed for payloads that are still malformed for this code path (e.g., JSON string/array, or missing required keys). In that casemessage["id"]/etc will raiseTypeError/KeyError, so the receiver can still crash despite the new JSONDecodeError handling. Consider validating the decoded payload is an object with required fields and dead-lettering it similarly before starting the trace span.
try:
message = json.loads(str(msg))
except (json.JSONDecodeError) as e:
logger.error(f"Received bad service bus resource request message: {e}")
await receiver.dead_letter_message(msg, reason="InvalidJSON", error_description=str(e))
resource_processor/tests_rp/test_runner.py:158
- The test asserts an exact
JSONDecodeErrormessage string inerror_description. That message can vary across Python versions/implementations, making the unit test brittle. Prefer asserting that dead-lettering happened and thaterror_descriptioncontains a stable substring (or is non-empty) rather than matching the full text.
mock_receiver.dead_letter_message.assert_awaited_once_with(
"invalid_json_string",
reason="InvalidJSON",
error_description="Expecting value: line 1 column 1 (char 0)"
)
resolves #4976
What is being addressed
Service bus will now skip malformed payloads instead of crashing
How is this addressed
Summary of Changes
• Caught json.JSONDecodeError when deserializing the Service Bus message payload.
• Sent the malformed message to the dead-letter queue via await receiver.dead_letter_message(msg, reason="InvalidJSON") to clear it from the queue and aid in troubleshooting.
• Added a continue statement inside the except block to prevent processing of empty/malformed message structures, avoiding downstream TypeError crashes.
• Added a new unit test test_receive_message_bad_json to simulate receiving a malformed/non-JSON payload, asserting that:
• The message is successfully sent to the dead-letter queue.
• The message is not processed or marked complete (i.e. skipped).