[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 b0828b7.

♻️ 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]

🟢 Approval recommended

The malformed-message handling is correct, focused, and adequately tested.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The focused error-handling change is complete, consistent with message producers, and adequately tested.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Jay W (JayDoubleu)]

/test-force-approve

[github-actions]

🤖 pr-bot 🤖

✅ Marking tests as complete (for commit 0c97da0)

(in response to this comment from Jay W (@JayDoubleu))

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

Review effort: Lite
Findings: 1 · 1

Open (2)

• This PR introduces strict schema enforcement for Service Bus messages (e.g., now requiring name,… · New
• The PR title/description and changelog entry focus on malformed/invalid JSON handling, but the… · New

[Jay W (JayDoubleu)]

Reviewed the two latest Copilot comments against head 0c97da07e61fbb3f75a7c93738112a5c0c429aa4. I consider both non-blocking.

1. Schema compatibility: the production API already supplies name, version and parameters through get_resource_request_message_payload. Both initial requests and subsequent pipeline steps use this method. The existing Porter command builder already consumes these fields. The older receiver tests mocked invoke_porter_action, so their incomplete payloads do not demonstrate a supported production message format. I found no evidence of the proposed compatibility regression and recommend retaining the validation.

2. Documentation scope: the clarification is reasonable, but it does not identify a runtime bug. The implementation intentionally dead-letters both invalid JSON (InvalidJSON) and well-formed JSON with an invalid message structure or field type (InvalidResourceRequest). Suggested wording for a future documentation clarification: "Dead-letter invalid JSON and structurally invalid Service Bus resource-request payloads instead of retrying them indefinitely." This wording change is optional for this merge and has not been applied.

Resolving these two conversations as reviewed and non-blocking, rather than marking them as code fixes. This assessment is based on source inspection, not a live Service Bus test, and does not replace the normal merge gates.

[Jay W (JayDoubleu)]

/test-force-approve

[github-actions]

🤖 pr-bot 🤖

✅ Marking tests as complete (for commit 3235712)

(in response to this comment from Jay W (@JayDoubleu))

[Copilot]

Copilot review overview

🟡 Changes recommended

A plain ValueError from JSON deserialization can still cause malformed messages to be retried indefinitely.

Review effort: Balanced
Findings: 1

Open (1)

• Handle ValueError from oversized JSON integers to prevent infinite retries · New

Resolved since last review (2)

• This PR introduces strict schema enforcement for Service Bus messages (e.g., now requiring name,…
• The PR title/description and changelog entry focus on malformed/invalid JSON handling, but the…

[Copilot]

Copilot review overview

🟢 Approval recommended

The implementation safely settles invalid messages and includes comprehensive tests for the new behavior.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Handle ValueError from oversized JSON integers to prevent infinite retries

[Jay W (JayDoubleu)]

/test-force-approve

[github-actions]

🤖 pr-bot 🤖

✅ Marking tests as complete (for commit b0828b7)

(in response to this comment from Jay W (@JayDoubleu))