From 63ccf052a7c61a1761c36d8f711e8630a32f9a99 Mon Sep 17 00:00:00 2001 From: Alex Luong Date: Fri, 31 Jul 2026 15:16:29 +0700 Subject: [PATCH] fix(destregistry): record the cause of a transport-level delivery failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Of the four failure paths through destwebhook, the transport branch was the only one that left Delivery.Response nil. A connection-level failure — DNS, refused, TLS, dial timeout — therefore stored response_data NULL, and the reason it failed was not recoverable afterwards: the one copy of the error string went onto the publish-attempt error, which the delivery consumer deliberately suppresses. Record the cause on the attempt. Which string is safe depends on the branch: a plain transport error is a *url.Error naming the customer's own destination and nothing of ours, while ErrProxyDestination.Error() appends proxy diagnostics that are operator-side only — so that branch records the classification and destination host alone, matching the rule the adjacent diagnostics block already follows. Also classify EADDRNOTAVAIL as address_unavailable instead of letting source-side ephemeral port exhaustion fall into the network_error catch-all beside genuinely remote failures; the fix for it is connection reuse, not a retry against the destination. Matched on the errno rather than the message, which is not portable — Linux renders it "cannot assign requested address" and darwin "can't". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01D7FsF8MDpWkeZFJdLYahTX --- .../providers/destwebhook/httphelper.go | 34 ++++++++- .../providers/destwebhook/httphelper_test.go | 71 +++++++++++++++++++ 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/internal/destregistry/providers/destwebhook/httphelper.go b/internal/destregistry/providers/destwebhook/httphelper.go index 416d9953..fbf6c7b8 100644 --- a/internal/destregistry/providers/destwebhook/httphelper.go +++ b/internal/destregistry/providers/destwebhook/httphelper.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "strings" + "syscall" "github.com/hookdeck/outpost/internal/destregistry" ) @@ -76,10 +77,26 @@ func ExecuteHTTPRequest(ctx context.Context, client *http.Client, req *http.Requ } } + // The transport branch is the only one that doesn't fill Response, which + // would store response_data NULL and lose the cause entirely. + // + // A plain transport error is a *url.Error naming the customer's own + // destination, safe to store. ErrProxyDestination.Error() appends proxy + // diagnostics, which are operator-side — that branch stores the + // classification instead, matching the block above. + message := err.Error() + if destErr != nil { + message = code + if destErr.DestHost != "" { + message = fmt.Sprintf("%s connecting to %s", code, destErr.DestHost) + } + } + return &HTTPRequestResult{ Delivery: &destregistry.Delivery{ - Status: "failed", - Code: code, + Status: "failed", + Code: code, + Response: map[string]interface{}{"error": message}, }, Error: destregistry.NewErrDestinationPublishAttempt(err, provider, data), Response: nil, @@ -146,6 +163,15 @@ func ClassifyNetworkError(err error) string { return "unknown" } + // Source-side ephemeral port exhaustion. Ours, not the destination's, and the + // fix is connection reuse rather than a retry — so it must not disappear into + // the network_error catch-all among genuinely remote failures. Matched on the + // errno because the message is not portable: Linux renders EADDRNOTAVAIL as + // "cannot assign requested address", darwin as "can't". + if errors.Is(err, syscall.EADDRNOTAVAIL) { + return "address_unavailable" + } + errStr := err.Error() switch { @@ -157,6 +183,10 @@ func ClassifyNetworkError(err error) string { return "connection_reset" case strings.Contains(errStr, "network is unreachable"): return "network_unreachable" + // Same condition arriving as an opaque string (e.g. synthesized from a proxy + // report), where the errno is not available to match on. + case strings.Contains(errStr, "assign requested address"): + return "address_unavailable" case strings.Contains(errStr, "i/o timeout"): return "timeout" case strings.Contains(errStr, "context deadline exceeded"): diff --git a/internal/destregistry/providers/destwebhook/httphelper_test.go b/internal/destregistry/providers/destwebhook/httphelper_test.go index 291272b8..a81fec32 100644 --- a/internal/destregistry/providers/destwebhook/httphelper_test.go +++ b/internal/destregistry/providers/destwebhook/httphelper_test.go @@ -1,14 +1,21 @@ package destwebhook_test import ( + "context" + "errors" "io" + "net" "net/http" + "net/url" + "os" "strings" + "syscall" "testing" "github.com/hookdeck/outpost/internal/destregistry" "github.com/hookdeck/outpost/internal/destregistry/providers/destwebhook" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestParseHTTPResponse_MaxBytes(t *testing.T) { @@ -63,3 +70,67 @@ func TestParseHTTPResponse_MaxBytes(t *testing.T) { }) } } + +// Ephemeral port exhaustion is a source-side failure: the fix is connection +// reuse, not a retry against the destination. Folding it into the +// network_error catch-all alongside genuinely remote failures is what made a +// 7,604-failure episode in the 2026-07-30 benchmark unattributable. +func TestClassifyNetworkError_AddressUnavailable(t *testing.T) { + t.Parallel() + + err := &url.Error{ + Op: "Post", + URL: "https://example.com/hook", + Err: &net.OpError{ + Op: "dial", + Net: "tcp", + Err: &os.SyscallError{Syscall: "connect", Err: syscall.EADDRNOTAVAIL}, + }, + } + assert.Equal(t, "address_unavailable", destwebhook.ClassifyNetworkError(err)) +} + +func TestClassifyNetworkError_Codes(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + want string + }{ + {"dns", errors.New(`dial tcp: lookup nope.invalid: no such host`), "dns_error"}, + {"refused", errors.New(`dial tcp 1.2.3.4:443: connect: connection refused`), "connection_refused"}, + {"timeout", errors.New(`context deadline exceeded`), "timeout"}, + {"unknown", errors.New(`something else entirely`), "network_error"}, + {"nil", nil, "unknown"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, destwebhook.ClassifyNetworkError(tt.err)) + }) + } +} + +// A transport failure must record why it failed. This was the only one of the +// package's four failure paths leaving Delivery.Response nil, so every +// connection-level failure stored response_data NULL. +func TestExecuteHTTPRequest_TransportErrorRecordsCause(t *testing.T) { + t.Parallel() + + // Port 1 on loopback: nothing listens, so client.Do fails at the transport + // layer without involving a server. + req, err := http.NewRequest(http.MethodPost, "http://127.0.0.1:1/hook", strings.NewReader("{}")) + require.NoError(t, err) + + res := destwebhook.ExecuteHTTPRequest(context.Background(), http.DefaultClient, req, "webhook", 0) + + require.NotNil(t, res.Delivery, "transport error must still record a customer-visible attempt") + assert.Equal(t, "failed", res.Delivery.Status) + require.NotNil(t, res.Delivery.Response, "response_data was nil — the cause is unrecoverable after the fact") + + msg, ok := res.Delivery.Response["error"].(string) + require.True(t, ok, "response_data.error missing or not a string: %#v", res.Delivery.Response) + assert.Contains(t, msg, "connection refused") + assert.Contains(t, msg, "127.0.0.1:1", "the destination the customer configured should be identifiable") +}