OF-3300: Harden against unrecognized error conditions

[guusdk]

The isDeliveryRelatedErrorResponse() method called error.getCondition() without exception handling. When an unrecognized error condition was encountered, it would throw an uncaught IllegalArgumentException.

Wrapped the calls to error.getCondition() in try-catch blocks that:

• Catch IllegalArgumentException when unrecognized conditions are encountered
• Log the error condition for debugging purposes
• Return false to gracefully handle unknown error types (appropriate behavior for unknown delivery-related errors)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7d918c20-e43e-468a-8846-69f8d2aeea59

📥 Commits

Reviewing files that changed from the base of the PR and between acfbbb1 and b202601.

📒 Files selected for processing (1)

• xmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/MultiUserChatServiceImpl.java

🚧 Files skipped from review as they are similar to previous changes (1)

• xmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/MultiUserChatServiceImpl.java

---

📝 Walkthrough

Walkthrough

This PR refines the error detection logic in MultiUserChatServiceImpl.isDeliveryRelatedErrorResponse() to handle edge cases more defensively. For IQ error stanzas, the code now explicitly extracts the PacketError, checks whether the error condition indicates known client connectivity exceptions, and wraps condition inspection in try/catch with debug logging. For non-IQ stanzas, the method adds an explicit null check on PacketError and wraps the condition membership check in try/catch to treat unrecognized error conditions as non-delivery-related while logging debug output. These changes prevent implicit failures when encountering unexpected error conditions.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	The pull request description accurately describes the changes made to handle unrecognized error conditions with exception handling and logging.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR hardens MultiUserChatServiceImpl#isDeliveryRelatedErrorResponse against IllegalArgumentException thrown by PacketError#getCondition() when encountering unrecognized XMPP error conditions, preventing an uncaught exception from interrupting ghost-user detection / ping-response handling in MUC.

Changes:

• Wraps error.getCondition() calls in try/catch (IllegalArgumentException) in the IQ (ping) path.
• Wraps error.getCondition() calls in try/catch (IllegalArgumentException) in the Message/Presence path, returning false for unknown conditions.
• Adds debug logging when an unknown condition is encountered.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/MultiUserChatServiceImpl.java`:
- Around line 1570-1579: The error-handling path must treat unrecognized IQ
error conditions as non-delivery-related to avoid ghost removal: in
MultiUserChatServiceImpl where you inspect stanza.getError() and reference
pingErrorsIndicatingClientConnectivity, change the
catch(IllegalArgumentException) so it does not fall through but explicitly
returns false (or otherwise short-circuits) to indicate "not a ping-related
delivery error"; additionally, harden isPendingPingResponse by guarding the call
to error.getCondition() (wrap it in a try/catch for IllegalArgumentException or
check for null/unknown condition) so it cannot throw and cause the same
fall-through behavior. Ensure you reference the methods isPendingPingResponse
and the pingErrorsIndicatingClientConnectivity lookup when applying these fixes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bf766f9e-fe64-47aa-a649-197195d8aaf5

📥 Commits

Reviewing files that changed from the base of the PR and between f1571a7 and acfbbb1.

📒 Files selected for processing (1)

• xmppserver/src/main/java/org/jivesoftware/openfire/muc/spi/MultiUserChatServiceImpl.java