fix(connect): defensive empty-id guard + admin retry test asserts persistence

Post-A self-review surfaced two gaps:

1. `StripeConnectService.CreateTransfer` trusted Stripe's SDK to
   return a non-empty `tr.ID` on success (`err == nil`). The
   invariant holds in practice, but an empty id silently persisted
   on a completed transfer leaves the row permanently
   un-reversible — which defeats the entire point of item A.
   Added a belt-and-suspenders check that converts `(tr.ID="",
   err=nil)` into a failed transfer.

2. `TestRetryTransfer_Success` (admin handler) exercised the retry
   path but didn't assert that StripeTransferID was persisted after
   a successful retry. The worker path and processSellerTransfers
   both had the assertion; the admin manual-retry path was the
   third entry into the same behavior and lacked coverage. Added
   the assertion.

Decision on scope: v1.0.6.2 added a partial UNIQUE on
stripe_transfer_id (WHERE IS NOT NULL AND <> '') in migration 981,
matching the v1.0.6.1 pattern for refunds.hyperswitch_refund_id.
The combination of (a) the DB partial UNIQUE and (b) this defensive
guard means there is now no code or data path that can persist an
empty transfer id while claiming success.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
senke 2026-04-17 14:03:37 +02:00
parent eedaad9f83
commit e0efdf8210
2 changed files with 13 additions and 1 deletions

View file

@ -164,7 +164,7 @@ func TestGetTransfers_FilterBySeller(t *testing.T) {
func TestRetryTransfer_Success(t *testing.T) {
db := setupAdminTransferTestDB(t)
logger := zap.NewNop()
mock := &mockTransferServiceAdmin{}
mock := &mockTransferServiceAdmin{stripeTransferID: "tr_admin_retry_ok"}
handler := NewAdminTransferHandler(db, mock, 0.10, logger)
transferID := uuid.New()
@ -192,6 +192,11 @@ func TestRetryTransfer_Success(t *testing.T) {
var updated marketplace.SellerTransfer
require.NoError(t, db.First(&updated, transferID).Error)
assert.Equal(t, "completed", updated.Status)
// v1.0.7 item A: admin-triggered retry also persists the Stripe
// transfer id. The worker and processSellerTransfers paths are
// already covered by their own tests; this is the third path into
// the same behavior.
assert.Equal(t, "tr_admin_retry_ok", updated.StripeTransferID)
}
func TestRetryTransfer_NotFailed(t *testing.T) {

View file

@ -208,5 +208,12 @@ func (s *StripeConnectService) CreateTransfer(ctx context.Context, sellerUserID
if err != nil {
return "", fmt.Errorf("create stripe transfer: %w", err)
}
// Defensive: Stripe's Go SDK should never return (tr, nil) with an empty
// tr.ID, but the invariant is load-bearing for item B (reversal needs a
// real id to target). If it's ever violated we'd rather fail the transfer
// than persist an empty string and leave the row permanently un-reversible.
if tr.ID == "" {
return "", fmt.Errorf("create stripe transfer: provider returned empty transfer id")
}
return tr.ID, nil
}