Skip to content

Comments

Fix: Clear stale relay mappings and remove config block when disabled#296

Open
hikmethankolay wants to merge 1 commit intoBillionmail:devfrom
hikmethankolay:fix/smtp-relay-fix
Open

Fix: Clear stale relay mappings and remove config block when disabled#296
hikmethankolay wants to merge 1 commit intoBillionmail:devfrom
hikmethankolay:fix/smtp-relay-fix

Conversation

@hikmethankolay
Copy link

fixes #293

Purpose

When an SMTP relay is disabled or deleted, the system fails to fall back to the server's own/local SMTP and returns "mail transport unavailable" error. This occurs because:

  1. Stale database entries: The bm_domain_smtp_transport table retains relay transport mappings pointing to non-existent SMTP services
  2. Orphaned Postfix config: The main.cf relay configuration block (sender_dependent_default_transport_maps) remains active even after relay is disabled

This PR ensures proper cleanup when relay functionality is disabled, restoring expected fallback behavior.

Implementation

File: core/internal/service/relay/config_sync.go

Change 1 - Clear stale transport mappings (lines 443-447):

// Clear stale relay transport mappings to ensure fallback to original SMTP
_, err = g.DB().Model("bm_domain_smtp_transport").Where("atype", "relay").Delete()
if err != nil {
    g.Log().Warningf(ctx, "Failed to clear relay transport mappings: %v", err)
}

Change 2 - Remove relay config block from main.cf (lines 808-822):

} else {
    // Remove the configuration block when relay is disabled
    if hasConfigBlock {
        blockEnd := endIndex + len(endMarker)
        if blockEnd < len(content) && (content[blockEnd] == '\n' || content[blockEnd] == '\r') {
            blockEnd++
            if blockEnd < len(content) && content[blockEnd] == '\n' {
                blockEnd++
            }
        }
        content = content[:beginIndex] + content[blockEnd:]
        modified = true
        g.Log().Info(nil, "Removed relay configuration block from main.cf")
    }
}

Testing

  • Unit tests passed (go test)
  • Manual steps:
# 1. Configure and enable an SMTP relay with domain binding
# 2. Send test email → should route via relay
# 3. Disable the relay (set active=0)
# 4. Send test email → should now use local SMTP (fallback)
# 5. Verify main.cf no longer contains relay configuration block
docker exec billionmail-postfix-billionmail-1cat /etc/postfix/main.cf | grep -A4 "RELAY SERVICE"
# 6. Verify transport table is cleared
docker exec billionmail-pgsql-billionmail-1 psql -U billionmail  -d billionmail  -c "SELECT * FROM bm_domain_smtp_transport WHERE atype='relay';"
  • Impact analysis: No changes to relay-enabled functionality; only affects cleanup on disable

This update clears stale relay transport mappings from the database when no active relay configs exist, ensuring fallback to original SMTP. It also removes the relay configuration block from main.cf when relay is disabled, improving cleanup and logging.
Copilot AI review requested due to automatic review settings December 20, 2025 20:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes issue #293 by ensuring proper cleanup when SMTP relay functionality is disabled or deleted. The changes prevent the "mail transport unavailable" error that previously occurred due to stale database entries and orphaned Postfix configuration.

Key changes:

  • Clears relay transport mappings from the database when relay system becomes inactive
  • Removes the relay configuration block from Postfix's main.cf when relay is disabled

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

}
content = content[:beginIndex] + content[blockEnd:]
modified = true
g.Log().Info(nil, "Removed relay configuration block from main.cf")
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

Context parameter inconsistency: The logging call uses nil for the context parameter, but the pattern used elsewhere in this file when context is unavailable is context.Background() (see lines 705, 747, 756, 758). Using context.Background() instead of nil is more idiomatic and consistent with the rest of the codebase.

Suggested change
g.Log().Info(nil, "Removed relay configuration block from main.cf")
g.Log().Info(context.Background(), "Removed relay configuration block from main.cf")

Copilot uses AI. Check for mistakes.
// Include trailing newline if present
if blockEnd < len(content) && (content[blockEnd] == '\n' || content[blockEnd] == '\r') {
blockEnd++
if blockEnd < len(content) && content[blockEnd] == '\n' {
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

Newline handling could be more robust: The logic handles common cases like \n, \n\n, and \r\n, but doesn't consistently handle all newline character combinations. After checking for the first newline character (which could be \n or \r), the second check only looks for \n. Consider checking for any newline character in the second check as well to handle edge cases more consistently.

Suggested change
if blockEnd < len(content) && content[blockEnd] == '\n' {
if blockEnd < len(content) && (content[blockEnd] == '\n' || content[blockEnd] == '\r') {

Copilot uses AI. Check for mistakes.
// Clear stale relay transport mappings to ensure fallback to original SMTP
_, err = g.DB().Model("bm_domain_smtp_transport").Where("atype", "relay").Delete()
if err != nil {
g.Log().Warningf(ctx, "Failed to clear relay transport mappings: %v", err)
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

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

Error handling inconsistency: Similar database delete operation at line 863 returns an error when the delete fails, but here it only logs a warning. This inconsistency could mask database issues. Consider returning the error instead of just logging it to maintain consistent error handling behavior throughout the function.

Suggested change
g.Log().Warningf(ctx, "Failed to clear relay transport mappings: %v", err)
return gerror.Wrap(err, "Failed to clear relay transport mappings")

Copilot uses AI. Check for mistakes.
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.

Server doesnt send emails trough its own smtp when a relay added then deleted/inacitvated

1 participant