We originally took heavy inspiration fromAlertmanager's SMTP dispatch functionality due to the number of options we need to support.

In their dispatch code, they defer the call tomessage.Close but in fact this is necessary to complete the SMTP submission and await any errors; we do the same in our code.

NOTE: We useemersion/go-smtp as our SMTP library while Alertmanager usesnet/smtp, but the former extends the latter, so the code that follows is effectively the same.

// coderd/notifications/dispatch/smtp.go// Our client calls Data(), which returns a `dataCloser`.message,err:=c.Data()...// emersion/go-smtp@v0.21.2/client.gofunc (c*Client)Data() (io.WriteCloser,error) {_,_,err:=c.cmd(354,"DATA")iferr!=nil {returnnil,err}return&dataCloser{c:c,WriteCloser:c.text.DotWriter()},nil}...func (d*dataCloser)Close()error {...ifd.c.lmtp {...}else {_,_,err:=d.c.readResponse(250)// <---- this is the crucial lineiferr!=nil {returnerr}}d.closed=truereturnnil}

We cannot defer a call toClose because we depend on the error which is returned to determine if the message was successfully sent or not.

_,_,err:=d.c.readResponse(250)

This line checks if the response was successful (docs); if not, we need to fail this dispatch and mark it for retry.

I discovered this bug by incorrectly configuringCODER_NOTIFICATIONS_EMAIL_FROM as my@coder.com address (which is backed by GSuite) and testing a dispatch via an outlook.com SMTP server. The server was rejecting the message with554 5.2.252 SendAsDenied; <redacted>@outlook.com not allowed to send as <redacted>@coder.com, but the message was being marked successful, because we were not waiting for the actual outcome of the mail dispatch.

This has quite worrying implications for Alertmanager, and I plan to raise a PR in that repo too to address this.