Enhance service bus handling of invalid JSON in receive_message function

[James Chapman (JC-wk)]

resolves #4976

What is being addressed

Service bus will now skip malformed payloads instead of crashing

How is this addressed

Summary of Changes

1. Bug Fix in runner.py:
   • 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.
2. Added Unit Tests in test_runner.py:
   • 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).

• Updated CHANGELOG.md
• Increment version

[Copilot]

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.JSONDecodeError and skip further processing of that message.
• Add a unit test covering the malformed JSON path in receive_message.
• Bump resource_processor version 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.

[github-actions]

Unit Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 7a95e15.

♻️ This comment has been updated with latest results.

[Jack Morris (rudolphjacksonm)]

/test 59a309a

[github-actions]

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/27306698867 (with refid a20534b2)

(in response to this comment from Jack Morris (@rudolphjacksonm))

[Jack Morris (rudolphjacksonm)]

/test e7218a4

[github-actions]

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/28122108599 (with refid a20534b2)

(in response to this comment from Jack Morris (@rudolphjacksonm))

[Marcus Robinson (marrobi)]

James Chapman (@JC-wk) Opus 4.8 review, let me kniw if agree:

A couple of things before merge:

2. (Optional) Guard the dead-letter call. If a message is already in a state where dead_letter_message can fail (e.g. lock lost / already settled), the raised exception would bubble to the outer except Exception and 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

3. (Nit) Test brittleness. test_receive_message_bad_json asserts 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 on reason="InvalidJSON" (and maybe that error_description is non-empty) would be more robust, but this is minor.

[Copilot]

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.loads can succeed for payloads that are still malformed for this code path (e.g., JSON string/array, or missing required keys). In that case message["id"]/etc will raise TypeError/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 JSONDecodeError message string in error_description. That message can vary across Python versions/implementations, making the unit test brittle. Prefer asserting that dead-lettering happened and that error_description contains 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)"
    )

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

resource_processor/vmss_porter/runner.py:77

• This handles only JSON syntax errors. Payloads such as null, [], or "text" deserialize successfully, but the subsequent message["id"] access raises TypeError; the outer catch then reconnects without settling the poison message, so the reported failure remains possible. Validate that the decoded payload is an object with the required resource-request fields and dead-letter invalid structures as well (with a corresponding test).

                            continue

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

resource_processor/vmss_porter/runner.py:82

• This presence-only check still lets malformed requests reach processing. For example, an object containing these four keys but omitting name, version, or parameters passes here, then build_porter_command raises at helpers/commands.py:91 or :132 after an in-progress status has already been sent, so the message remains unsettled and is retried. Invalid field types (such as parameters: null or a list-valued action) have the same problem. Validate the complete resource-request schema, including all consumed fields and their types, before invoking any processing, and dead-letter schema validation failures.

                            missing_fields = REQUIRED_RESOURCE_REQUEST_FIELDS - message.keys()
                            if missing_fields:
                                raise ValueError(f"Resource request message is missing fields: {sorted(missing_fields)}")

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

resource_processor/vmss_porter/runner.py:115

• This handler also catches schema-validation ValueErrors, so valid JSON with missing or mistyped fields is dead-lettered with the misleading reason InvalidJSON. This makes DLQ diagnostics inaccurate. Split parse and schema failures into distinct reasons, or use a broader reason such as InvalidResourceRequest, and update the test assertions accordingly.

                                await receiver.dead_letter_message(msg, reason="InvalidJSON", error_description=str(e))

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (4)

resource_processor/vmss_porter/runner.py:118

• The dead-letter reason "InvalidJSON" is used for both JSON parsing failures and schema/type validation failures (ValueError). This makes DLQ triage/alerting less actionable (schema violations will be misclassified as JSON issues). Consider using distinct reason codes (e.g., "InvalidJSON" for JSONDecodeError and "InvalidResourceRequest" for validation failures) and/or tailoring the log message similarly.

                        except (json.JSONDecodeError, ValueError) 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

resource_processor/tests_rp/test_runner.py:152

• These tests use raw strings as the iterated “message” and assert dead_letter_message is called with that string. In production, async for msg in receiver yields Service Bus message objects, and dead_letter_message is typically called with that message object (not its string/body). To more accurately validate behavior and avoid false confidence, consider having __aiter__ yield a message-like object (e.g., an AsyncMock whose __str__ returns the payload) and assert DLQ is invoked with that object.

    mock_receiver.__aiter__.return_value = ["invalid_json_string"]

resource_processor/tests_rp/test_runner.py:163

• These tests use raw strings as the iterated “message” and assert dead_letter_message is called with that string. In production, async for msg in receiver yields Service Bus message objects, and dead_letter_message is typically called with that message object (not its string/body). To more accurately validate behavior and avoid false confidence, consider having __aiter__ yield a message-like object (e.g., an AsyncMock whose __str__ returns the payload) and assert DLQ is invoked with that object.

    mock_receiver.dead_letter_message.assert_awaited_once()
    dead_letter_args, dead_letter_kwargs = mock_receiver.dead_letter_message.await_args
    assert dead_letter_args == ("invalid_json_string",)

CHANGELOG.md:8

• The changelog entry mentions “invalid JSON”, but the code change also dead-letters messages that are valid JSON yet fail schema/type validation (e.g., missing required fields). Consider updating the wording to reflect both cases (e.g., “malformed or invalid resource request messages”) so the changelog matches the actual behavior change.

* Fix to enhance service bus handling of invalid JSON in receive_message function ([#4932](https://github.com/microsoft/AzureTRE/pull/4932))

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

resource_processor/vmss_porter/runner.py:30

• This change makes name, version, and parameters mandatory (and type-checked). Previously, receive_message only directly depended on id, action, stepId, and operationId, so older producers might now get dead-lettered unexpectedly. If these fields are truly required by the message contract, consider documenting that contract (and/or linking to it). If they are not universally present, make them optional in validation (and validate their types only when provided) or provide defaults before calling downstream processing.

RESOURCE_REQUEST_FIELD_TYPES = {
    "id": str,
    "action": str,
    "stepId": str,
    "operationId": str,
    "name": str,
    "version": str,
    "parameters": dict,
}

resource_processor/vmss_porter/runner.py:110

• Parsing JSON from str(msg) depends on the message object's __str__ implementation, which is not a stable interface for payload decoding and risks mis-parsing (and now dead-lettering) otherwise valid messages. Prefer decoding from the actual message body (e.g., using the SDK-provided body accessors) and then json.loads on that decoded string/bytes.

                            message = json.loads(str(msg))

CHANGELOG.md:8

• The entry text is grammatically awkward ('Fix to enhance ...'). Consider rephrasing to a clearer statement of the change, e.g., 'Enhance Service Bus handling by dead-lettering invalid JSON in receive_message.'

* Fix to enhance service bus handling of invalid JSON in receive_message function ([#4932](https://github.com/microsoft/AzureTRE/pull/4932))

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

resource_processor/tests_rp/test_runner.py:152

• These tests iterate over raw strings, but receive_message is designed to iterate over Service Bus message objects (where str(msg) yields a body representation and the same msg object is passed to dead_letter_message/complete_message). Using plain strings makes the tests less representative and can mask regressions in how messages are stringified/settled. Consider using a lightweight mock message object (e.g., Mock()/MagicMock()) whose __str__ returns the JSON (or invalid JSON) and then assert dead_letter_message/complete_message is awaited with that mock message object.

    mock_receiver.__aiter__.return_value = ["invalid_json_string"]

resource_processor/tests_rp/test_runner.py:190

• These tests iterate over raw strings, but receive_message is designed to iterate over Service Bus message objects (where str(msg) yields a body representation and the same msg object is passed to dead_letter_message/complete_message). Using plain strings makes the tests less representative and can mask regressions in how messages are stringified/settled. Consider using a lightweight mock message object (e.g., Mock()/MagicMock()) whose __str__ returns the JSON (or invalid JSON) and then assert dead_letter_message/complete_message is awaited with that mock message object.

    mock_receiver.__aiter__.return_value = [message]

resource_processor/vmss_porter/runner.py:30

• The validation hard-codes dict for JSON objects (parameters, user). If callers ever pass dict-like mappings (e.g., collections.abc.Mapping implementations) this will be rejected even though they behave like JSON objects. To make the validator more robust and easier to evolve, consider checking against collections.abc.Mapping for object-shaped fields (and updating the constants accordingly).

RESOURCE_REQUEST_FIELD_TYPES = {
    "id": str,
    "action": str,
    "stepId": str,
    "operationId": str,
    "name": str,
    "version": str,
    "parameters": dict,
}
OPTIONAL_RESOURCE_REQUEST_FIELD_TYPES = {"user": dict}