Skip to content

NMS-8099: MailTransportMonitor steps all over itself #8212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dino2gnt merged 1 commit into from
Jan 15, 2026
Merged

NMS-8099: MailTransportMonitor steps all over itself #8212

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 19 additions & 17 deletions .../monitors/core/src/main/java/org/opennms/netmgt/poller/monitors/MailTransportMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public PollStatus poll(MonitoredService svc, Map<String, Object> parameters) {
}

parseJavaMailProperties(mailParms);
status = doMailTest(mailParms);
status = doMailTest(mailParms, svc.getNodeId());
} catch (final IllegalStateException ise) {
//ignore this because we don't have to have both a send and read

Expand Down Expand Up @@ -134,10 +134,10 @@ private void parseJavaMailProperties(final MailTransportParameters mailParms) {
*
* @param mailParms
*/
private PollStatus doMailTest(final MailTransportParameters mailParms) {
private PollStatus doMailTest(final MailTransportParameters mailParms, final int nodeId) {
final long beginPoll = System.currentTimeMillis();
PollStatus status = PollStatus.unknown("Beginning poll.");
mailParms.setTestSubjectSuffix(Long.toString(beginPoll));
mailParms.setTestSubjectSuffix(Integer.toString(nodeId) + ": " + Long.toString(beginPoll));
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

The string concatenation should use String.format() or StringBuilder for better readability and maintainability, especially since this is building a composite identifier. Consider: String.format("%d: %d", nodeId, beginPoll)

Suggested change
mailParms.setTestSubjectSuffix(Integer.toString(nodeId) + ": " + Long.toString(beginPoll));
mailParms.setTestSubjectSuffix(String.format("%d: %d", nodeId, beginPoll));

Copilot uses AI. Check for mistakes.

/*
* If both a send and receive test are configured, then were testing the
Expand All @@ -151,19 +151,19 @@ private PollStatus doMailTest(final MailTransportParameters mailParms) {
* matching.
*/
mailParms.setEnd2EndTestInProgress(true);
status = sendTestMessage(mailParms);
status = sendTestMessage(mailParms, nodeId);

if (status.isAvailable()) {
LOG.debug("doMailTest: send test successful.");
status = readTestMessage(mailParms);
status = readTestMessage(mailParms, nodeId);
} else {
LOG.info("doMailTest: send test unsuccessful... skipping read portion of test.");
}

} else if (mailParms.getReadTest() != null) {
status = readTestMessage(mailParms);
status = readTestMessage(mailParms, nodeId);
} else if (mailParms.getSendTest() != null) {
status = sendTestMessage(mailParms);
status = sendTestMessage(mailParms, nodeId);
} else {
throw new IllegalArgumentException("MailTransportMonitor requires either send-host or read-host parameters");
}
Expand All @@ -175,7 +175,7 @@ private PollStatus doMailTest(final MailTransportParameters mailParms) {
return status;
}

private PollStatus readTestMessage(final MailTransportParameters mailParms) {
private PollStatus readTestMessage(final MailTransportParameters mailParms, final int nodeId) {
LOG.debug("readTestMessage: Beginning read mail test.");
PollStatus status = PollStatus.unavailable("Test not completed.");

Expand Down Expand Up @@ -221,7 +221,7 @@ private PollStatus readTestMessage(final MailTransportParameters mailParms) {
}
}
if (mailFolder.isOpen() && (mailParms.getReadTest().getSubjectMatch() != null || mailParms.isEnd2EndTestInProgress())) {
status = processMailSubject(mailParms, mailFolder);
status = processMailSubject(mailParms, mailFolder, nodeId);
if (status.getStatusCode() == PollStatus.SERVICE_AVAILABLE) {
break;
}
Expand Down Expand Up @@ -268,14 +268,14 @@ private void closeStore(final Store mailStore, final Folder mailFolder) {
* @return a PollStatus indicative of the success of matching a subject or just retieving
* mail folder contents... dependent on configuration.
*/
private PollStatus processMailSubject(final MailTransportParameters mailParms, final Folder mailFolder) {
private PollStatus processMailSubject(final MailTransportParameters mailParms, final Folder mailFolder, final int nodeId) {
PollStatus status = PollStatus.unknown();
try {
final String subject = computeMatchingSubject(mailParms);
if (mailFolder.isOpen() && subject != null) {
final Message[] mailMessages = mailFolder.getMessages();
final SearchTerm searchTerm = new SubjectTerm(subject);
final SearchTerm deleteTerm = new HeaderTerm(MTM_HEADER_KEY, m_headerValue);
final SearchTerm deleteTerm = new HeaderTerm(MTM_HEADER_KEY, m_headerValue + "-" + nodeId);
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

The nodeId is concatenated with a hyphen delimiter. For consistency and to avoid potential parsing issues, consider using the same delimiter (': ') as used in the subject line on line 140, or document why different delimiters are used.

Copilot uses AI. Check for mistakes.

LOG.debug("searchMailSubject: searching {} message(s) for subject '{}'", mailMessages.length, subject);

Expand All @@ -298,18 +298,20 @@ private PollStatus processMailSubject(final MailTransportParameters mailParms, f

final boolean deleteAllMail = mailParms.getReadTest().getDeleteAllMail();
final boolean foundMTMHeader = mailMessage.match(deleteTerm);
LOG.debug("searchMailSubject: deleteAllMail = {}, MTM header found = {}", Boolean.toString(deleteAllMail), Boolean.toString(foundMTMHeader));
LOG.debug("searchMailSubject: deleteAllMail = {}, MTM header found = {}, delete = {}", deleteAllMail, foundMTMHeader, delete);
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

The Boolean.toString() calls were removed from the log statement, but the delete variable (a boolean primitive) is now being passed directly. While this works, it's inconsistent with the comment on line 300 which previously called Boolean.toString(). Consider being consistent with how booleans are logged throughout this method.

Suggested change
LOG.debug("searchMailSubject: deleteAllMail = {}, MTM header found = {}, delete = {}", deleteAllMail, foundMTMHeader, delete);
LOG.debug("searchMailSubject: deleteAllMail = {}, MTM header found = {}, delete = {}", Boolean.toString(deleteAllMail), Boolean.toString(foundMTMHeader), Boolean.toString(delete));

Copilot uses AI. Check for mistakes.

if (deleteAllMail) {
if (!delete) LOG.debug("searchMailSubject: flagging message with subject '{}' for deletion because deleteAllMail is set.", subject);
delete = true;
} else if (foundMTMHeader) {
if (!delete) LOG.debug("searchMailSubject: flagging message with subject '{}' for deletion because we sent it (found header {}={})", subject, MTM_HEADER_KEY, m_headerValue);
if (!delete) LOG.debug("searchMailSubject: flagging message with subject '{}' for deletion because we sent it (found header {}={})", subject, MTM_HEADER_KEY, m_headerValue + "-" + nodeId);
delete = true;
}

if (delete) {
LOG.debug("searchMailSubject: Deleting message {} of {}", i, mailMessages.length);
mailMessage.setFlag(Flag.DELETED, true);
delete = false;
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

Resetting the delete flag to false after each deletion is incorrect logic. The delete flag should remain true once set by deleteAllMail or foundMTMHeader conditions. This reset would cause only the first matching message to be deleted when deleteAllMail is true, leaving subsequent messages undeleted.

Copilot uses AI. Check for mistakes.
}

// since we want to delete old messages matchin MTM_HEADER_KEY, we can't break early
Expand Down Expand Up @@ -397,7 +399,7 @@ private Folder retrieveMailFolder(final MailTransportParameters mailParms, final
* @param mailParms
* @return a PollStatus
*/
private PollStatus sendTestMessage(final MailTransportParameters mailParms) {
private PollStatus sendTestMessage(final MailTransportParameters mailParms, final int nodeId) {
PollStatus status = PollStatus.unavailable("Test not completed.");

final long interval = mailParms.getSendTestAttemptInterval();
Expand All @@ -407,7 +409,7 @@ private PollStatus sendTestMessage(final MailTransportParameters mailParms) {
tracker.startAttempt();
LOG.debug("sendTestMessage: sending mail attempt: {}, elapsed time: {}ms", (tracker.getAttempt() + 1), String.format("%.2f", tracker.elapsedTimeInMillis()));
try {
final JavaMailer sendMailer = createMailer(mailParms);
final JavaMailer sendMailer = createMailer(mailParms, nodeId);
overRideDefaultProperties(mailParms, sendMailer);
sendMailer.mailSend();
status = PollStatus.available();
Expand Down Expand Up @@ -471,7 +473,7 @@ private void overRideDefaultProperties(final MailTransportParameters mailParms,
sendMailer.setUseJMTA(mailParms.isSendTestUseJmta());
}

private JavaMailer createMailer(final MailTransportParameters mailParms) throws JavaMailerException {
private JavaMailer createMailer(final MailTransportParameters mailParms, final int nodeId) throws JavaMailerException {
final JavaMailer sendMailer = new JavaMailer(mailParms.getJavamailProperties());
final String mailPropsPrefix = new StringBuilder("mail.").append(mailParms.getSendTestTransport()).append('.').toString();
final Properties props = sendMailer.getSession().getProperties();
Expand Down Expand Up @@ -529,7 +531,7 @@ private JavaMailer createMailer(final MailTransportParameters mailParms) throws
//starttls.enable
props.setProperty(mailPropsPrefix+"starttls.enable", String.valueOf(mailParms.isSendTestStartTls()));
sendMailer.setStartTlsEnabled(mailParms.isSendTestStartTls());
sendMailer.addExtraHeader(MTM_HEADER_KEY, m_headerValue);
sendMailer.addExtraHeader(MTM_HEADER_KEY, m_headerValue+ "-" + nodeId);
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

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

Missing space before the '-' concatenation operator. Should be: m_headerValue + "-" + nodeId for consistency with line 278.

Suggested change
sendMailer.addExtraHeader(MTM_HEADER_KEY, m_headerValue+ "-" + nodeId);
sendMailer.addExtraHeader(MTM_HEADER_KEY, m_headerValue + "-" + nodeId);

Copilot uses AI. Check for mistakes.

sendMailer.setSession(Session.getInstance(props, sendMailer.createAuthenticator()));

Expand Down