Skip to content

fix(OPENFRAM-002-9): CU-86akdyq05 MachineHostnameListener leaves the message unacked on any parse or extraction error, risking infinite redelivery - #2079

Draft
flamingo[bot] wants to merge 1 commit into
mainfrom
ai-fix/openfram-002-9-ac22f538-4f0d6344
Draft

flamingo[bot] wants to merge 1 commit into
mainfrom
ai-fix/openfram-002-9-ac22f538-4f0d6344

Conversation

@flamingo

@flamingo flamingo Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Closes findings from rule OPENFRAM-002-9 — MachineHostnameListener leaves the message unacked on any parse or extraction error, risking infinite redelivery.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

# Fix confidence Finding Location
1 🔴 55 low — review closely MachineHostnameListener leaves the message unacked on any parse or extraction error, risking infinite redelivery openframe-client-core/src/main/java/com/openframe/client/listener/MachineHostnameListener.java:84

What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.


Run: https://product-hub.flamingo.so/admin/code-review
Run id: 4f0d6344-55d4-49de-bc86-cac2510f432f

Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.

ClickUp task: CU-86akdyq05 OpenFrame message ack and IAM pattern fixes (4 PRs)

…ed on any parse or extraction error, risking infinite redelivery

@flamingo flamingo Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 What this fix changed, finding by finding

1 finding(s) fixed in this draft — 1 explained inline on the diff; 1 low-confidence hunk(s) need close review before merging.

} catch (JsonProcessingException | IllegalArgumentException e) {
log.error("Non-retryable error processing hostname update, acking to avoid redelivery loop: {}", messagePayload, e);
message.ack();
} catch (Exception e) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 MachineHostnameListener leaves the message unacked on any parse or extraction error, risking infinite redelivery

In handleMessage, added a new catch (JsonProcessingException | IllegalArgumentException e) block (before the generic catch (Exception e)) that logs and calls message.ack() instead of leaving the message unacked. This targets the specific permanent-failure cases called out in the finding: malformed JSON (JsonProcessingException from objectMapper.readValue) and malformed subject (assuming machineIdExtractor.extract throws IllegalArgumentException on an unparseable subject, which is a common convention but not verified against that class's actual implementation in this file). Also added the com.fasterxml.jackson.core.JsonProcessingException import. The remaining generic Exception catch still leaves messages unacked for genuinely transient errors (e.g. machineHostnameService.updateHostname failures), preserving existing redelivery behavior for those. A complete fix would require confirming the actual exception type(s) thrown by NatsTopicMachineIdExtractor.extract for malformed subjects and verifying updateHostname's failure modes to ensure no permanent-failure exceptions leak into the unbounded-retry path.

🤖 Prompt for AI agents
In openframe-client-core/src/main/java/com/openframe/client/listener/MachineHostnameListener.java around line 84, review and complete this code-review fix: MachineHostnameListener leaves the message unacked on any parse or extraction error, risking infinite redelivery.
What the draft fix changed: In `handleMessage`, added a new `catch (JsonProcessingException | IllegalArgumentException e)` block (before the generic `catch (Exception e)`) that logs and calls `message.ack()` instead of leaving the message unacked. This targets the specific permanent-failure cases called out in the finding: malformed JSON (`JsonProcessingException` from `objectMapper.readValue`) and malformed subject (assuming `machineIdExtractor.extract` throws `IllegalArgumentException` on an unparseable subject, which is a common convention but not verified against that class's actual implementation in this file). Also added the `com.fasterxml.jackson.core.JsonProcessingException` import. The remaining generic `Exception` catch still leaves messages unacked for genuinely transient errors (e.g. `machineHostnameService.updateHostname` failures), preserving existing redelivery behavior for those. A complete fix would require confirming the actual exception type(s) thrown by `NatsTopicMachineIdExtractor.extract` for malformed subjects and verifying `updateHostname`'s failure modes to ensure no permanent-failure exceptions leak into the unbounded-retry path.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer

@flamingo flamingo Bot changed the title fix(OPENFRAM-002-9): MachineHostnameListener leaves the message unacked on any parse or extraction error, risking infinite redelivery fix(OPENFRAM-002-9): CU-86akdyq05 MachineHostnameListener leaves the message unacked on any parse or extraction error, risking infinite redelivery Sep 7, 2026
@michaelassraf

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant