fix: repair two broken error paths in the sender - #498
Merged
Conversation
Unexpected-close handler: the setTimeout that fires on an unexpected clean
close built a synthetic `error` (network/temporary) but then called
handleError() with the getConnectionWithCache callback param `err`, which is
guaranteed falsy past its own guard -- so `${err.message}` threw a TypeError
inside the unref'd timer instead of deferring the delivery. Pass the
constructed error. Broken since #363.
handleResponseError has-a-code guard: it tested the raw response with
/^\d{3}\b/ while bounces.check() classifies the normalized response
(formatSMTPResponse trims a leading CRLF/space ahead of the code). A 5xx that
arrived with leading whitespace -- e.g. a cache-restored response, where
responseCode is not restored -- failed the raw test, took the network-defer
branch, and deferred forever instead of rejecting. Hoist the smtpResponse
normalization (already computed lower down) above the branch and test that, so
the guard and the classifier see the same string.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GFF2LP1xQR2yV9DB37wPyD
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two correctness fixes in the sender's error-handling paths, both found during review of #497.
1. Unexpected-close handler discarded its own error object
The
setTimeoutthat fires on an unexpected clean connection close builds a syntheticerror(category: 'network',temporary: true) but then calledhandleError(delivery, connection, err)— passingerr, thegetConnectionWithCachecallback param, which is guaranteed falsy past its ownif (err) returnguard. So`Network error: ${err.message}`threw aTypeErrorinside anunref()'d timer instead of deferring the delivery. The constructederrorwas never used. Broken since #363 (Jan 2024).Fix: pass the constructed
errorand readerror.message.2. has-a-code guard tested the raw response, not the normalized one
In
handleResponseError, the "does this carry an SMTP code" guard tested the raw string with/^\d{3}\b/, whilebounces.check()classifies the normalized string (formatSMTPResponsetrims a leading CRLF/space ahead of the code). A 5xx arriving with leading whitespace — e.g. a cache-restored response, whereresponseCodeis not restored — failed the raw test, fell into the network-defer branch, and deferred forever instead of rejecting.Fix: hoist the
smtpResponsenormalization (already computed ~15 lines lower for logging) above the branch and test that, so the guard and the classifier consume the same string. Net −1 line; removes a duplicateformatSMTPResponsecall.Verified
TypeError: Cannot read properties of null (reading 'message'); the fix builds the error and routes it as a network defer through the realhandleResponseError.handleResponseErrorfor TypeScript #2 with the cache-restore shape (responseset, noresponseCode):\r\n550 ...now rejects and\r\n554 ... not allowed to connectnow defers+blacklists, where both previously deferred forever. Non-whitespace cases unchanged.bounces-test.jscases lock in the load-bearing property (formatSMTPResponsestrips leading CRLF/space) that the guard now depends on. Suite + eslint green.🤖 Generated with Claude Code
https://claude.ai/code/session_01GFF2LP1xQR2yV9DB37wPyD