Files
webhooks/pkg/worker/message_ack_test.go
Maxence MaireauxandGitHub 6504cd9692 fix: prevent webhook message loss, remove panic on unknown filter, stop logging secrets (#142)
* fix: resolve P0/P1 reliability and security issues

- P0: Process messages synchronously to prevent data loss on crash
  The Watermill handler now blocks until webhook delivery completes.
  Messages are only acknowledged after processing, not before.
  Errors on invalid payloads are returned (nack) instead of swallowed.
  Per-config failures use continue instead of return, so one bad
  endpoint no longer blocks delivery to other matching configs.

- P1: Replace panic with error on unknown filter key
  FindManyConfigs now returns an error instead of panicking when
  called with an unrecognized filter key. A single bad API request
  no longer crashes the process.

- P1: Remove environment and secret logging
  Removed os.Environ() dumps from server and worker startup.
  Replaced config %+v logging with safe ID/endpoint-only logging.
  Removed event-payload from OpenTelemetry span attributes.

- Docs: Add architecture, message-processing, and security docs.
  Fix CTE typo in retry-mechanism.md.

No breaking changes to public API, Store interface, or CLI flags.

* chore: add CODEOWNERS assigning @formancehq/backend as default reviewers

* fix: remove dead code flagged by ineffassign linter

* fix: address CodeRabbit review feedback

- Return error (nack) when InsertOneAttempt fails for non-success attempts,
  preventing silent loss of retry records
- Rewrite TestProcessMessages_SynchronousProcessing to be deterministic:
  HTTP handler blocks on a channel, test proves handler is blocked until
  the HTTP response completes
- Add TestProcessMessages_NackOnInsertFailureForRetry for the new nack path
- Expand secret logging test patterns to cover %v and more variable names
- Rename TestConfigStringerDoesNotLeakSecret to TestDocument_PercentPlusVLeaksSecrets
- Fix retry-mechanism.md claim query priority documentation: DISTINCT ON
  does per-webhook ordering, not global priority

* fix: propagate errors to span, avoid err shadowing in defer

- Rename loop-scoped err to attemptErr/insertErr so the defer
  closure captures the correct error for span.RecordError()
- Add explicit span.RecordError() on MakeAttempt and InsertOneAttempt
  failures that previously went unrecorded in traces
- Assign err before return on nack path so the defer sees it
- Update Go version in architecture doc to 1.25+ (matches go.mod)

Addresses review comments from paul-nicolas on PR #142.

* fix: handle transport errors in MakeAttempt as retryable attempts

- MakeAttempt now returns a retryable Attempt (instead of a bare error)
  when httpClient.Do fails due to transport/timeout, preventing event
  loss since the attempt record is persisted for the retry loop
- Replace base64-looking test secret with plain dummy to avoid
  triggering secret scanners in CI

Addresses CodeRabbit review comments on PR #142.
2026-04-08 15:16:21 +02:00

176 lines
4.8 KiB
Go

package worker
import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"sync/atomic"
"testing"
"time"
"github.com/ThreeDotsLabs/watermill/message"
webhooks "github.com/formancehq/webhooks/pkg"
"github.com/formancehq/go-libs/v2/publish"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
// TestProcessMessages_SynchronousProcessing verifies that processMessages
// blocks until the webhook HTTP call completes. The handler must not return
// before delivery finishes — otherwise the broker acks prematurely.
func TestProcessMessages_SynchronousProcessing(t *testing.T) {
var callCount atomic.Int32
started := make(chan struct{}, 1)
release := make(chan struct{})
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
callCount.Add(1)
started <- struct{}{}
<-release // block until the test releases us
w.WriteHeader(http.StatusOK)
}))
defer server.Close()
cfg := webhooks.ConfigUser{
Endpoint: server.URL,
Secret: webhooks.NewSecret(),
EventTypes: []string{"test.event"},
}
configStore := &mockStoreWithConfigs{
mockStore: newMockStore(nil, nil),
configs: []webhooks.Config{
{
ConfigUser: cfg,
ID: "cfg-1",
Active: true,
},
},
}
handler := processMessages(configStore, server.Client(), &noRetryPolicy{})
ev := publish.EventMessage{
Type: "test.event",
}
payload, err := json.Marshal(ev)
require.NoError(t, err)
msg := message.NewMessage("test-uuid", payload)
// Run handler in a goroutine — it should block on the HTTP call
done := make(chan error, 1)
go func() {
done <- handler(msg)
}()
// Wait for the HTTP request to start
select {
case <-started:
// good — the handler reached the HTTP endpoint
case err := <-done:
t.Fatalf("handler returned before the webhook request started: %v", err)
case <-time.After(2 * time.Second):
t.Fatal("handler never reached the webhook endpoint")
}
// The handler should still be blocked (waiting for the HTTP response)
select {
case err := <-done:
t.Fatalf("handler returned before the webhook request completed: %v", err)
default:
// good — handler is still blocked, proving synchronous processing
}
// Release the HTTP handler
close(release)
// Now the handler should complete
require.NoError(t, <-done)
assert.Equal(t, int32(1), callCount.Load(),
"exactly one webhook delivery should have been made")
}
// TestProcessMessages_ErrorReturnedOnFailure verifies that when the message
// handler encounters an unmarshal error, it returns an error (causing watermill
// to nack/retry) instead of silently dropping the message.
func TestProcessMessages_ErrorReturnedOnFailure(t *testing.T) {
store := newMockStore(nil, nil)
handler := processMessages(store, http.DefaultClient, &noRetryPolicy{})
msg := message.NewMessage("test-uuid", []byte(`not-valid-at-all`))
err := handler(msg)
assert.Error(t, err, "handler should return error for invalid messages so watermill can nack/retry")
}
// TestProcessMessages_NackOnInsertFailureForRetry verifies that when the
// attempt needs retry but InsertOneAttempt fails, the handler returns an error
// (nack) instead of silently acking and losing the retry record.
func TestProcessMessages_NackOnInsertFailureForRetry(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusInternalServerError) // trigger "to retry"
}))
defer server.Close()
cfg := webhooks.ConfigUser{
Endpoint: server.URL,
Secret: webhooks.NewSecret(),
EventTypes: []string{"test.event"},
}
configStore := &failingInsertStore{
mockStoreWithConfigs: &mockStoreWithConfigs{
mockStore: newMockStore(nil, nil),
configs: []webhooks.Config{
{
ConfigUser: cfg,
ID: "cfg-1",
Active: true,
},
},
},
}
handler := processMessages(configStore, server.Client(), &noRetryPolicy{})
ev := publish.EventMessage{
Type: "test.event",
}
payload, err := json.Marshal(ev)
require.NoError(t, err)
msg := message.NewMessage("test-uuid", payload)
err = handler(msg)
assert.Error(t, err, "handler should return error when insert fails for a non-success attempt")
assert.Contains(t, err.Error(), "insert attempt")
}
// mockStoreWithConfigs extends mockStore to return pre-configured configs
type mockStoreWithConfigs struct {
*mockStore
configs []webhooks.Config
}
func (m *mockStoreWithConfigs) FindManyConfigs(_ context.Context, _ map[string]any) ([]webhooks.Config, error) {
return m.configs, nil
}
// failingInsertStore always fails on InsertOneAttempt
type failingInsertStore struct {
*mockStoreWithConfigs
}
func (m *failingInsertStore) InsertOneAttempt(_ context.Context, _ webhooks.Attempt) error {
return assert.AnError
}
type noRetryPolicy struct{}
func (n *noRetryPolicy) GetRetryDelay(_ int) (time.Duration, error) {
return 0, nil
}