fixed the review comments

Generated with [GitHub Copilot](https://github.com/features/copilot)

Signed-off-by: Prabodh Chaudhari <[email protected]>

Author: prabodhcs

charts/lfx-v2-mailing-list-service/values.local.yaml

@@ -13,10 +13,10 @@ app:
   jwtAuthDisabledMockLocalPrincipal: "test-super-admin"
   # Enable debug logging for local development
   logLevel: debug
-  
+
   # Use mock repository for local testing
   repositorySource: mock
-  
+
   # Use mock GroupsIO integration for local testing
   groupsio:
     source: mock

cmd/mailing-list-api/service/grpsio_webhook_test.go

@@ -330,7 +330,7 @@ func TestWebhook_AllEventTypes(t *testing.T) {
 		service.WithMailingListReader(mockRepo),
 		service.WithMailingListWriter(mock.NewMockGrpsIOMailingListWriter(mockRepo)),
 		service.WithMemberReader(mockRepo),
-		service.WithMemberWriter(mock.NewMockGrpsIOWriter(mockRepo)),
+		service.WithMemberWriter(mock.NewMockGrpsIOMemberWriter(mockRepo)),
 	)
 
 	svc := NewMailingList(
@@ -428,7 +428,7 @@ func TestWebhook_MemberEventMissingMemberInfo(t *testing.T) {
 		service.WithMailingListReader(mockRepo),
 		service.WithMailingListWriter(mock.NewMockGrpsIOMailingListWriter(mockRepo)),
 		service.WithMemberReader(mockRepo),
-		service.WithMemberWriter(mock.NewMockGrpsIOWriter(mockRepo)),
+		service.WithMemberWriter(mock.NewMockGrpsIOMemberWriter(mockRepo)),
 	)
 
 	svc := NewMailingList(

cmd/mailing-list-api/service/mailing_list_service.go

@@ -6,6 +6,7 @@ package service
 
 import (
 	"context"
+	stderrors "errors"
 	"fmt"
 	"log/slog"
 	"time"
@@ -671,11 +672,26 @@ func (s *mailingListService) GroupsioWebhook(ctx context.Context, p *mailinglist
 	})
 
 	if err != nil {
-		slog.ErrorContext(ctx, "webhook processing failed after retries", "error", err)
-		// Note: Still return nil (204) to prevent Groups.io from retrying
+		// Check if this is a validation error (malformed data)
+		var validationErr errors.Validation
+		if stderrors.As(err, &validationErr) {
+			// Validation errors should not trigger retries - log and return success
+			slog.ErrorContext(ctx, "webhook validation failed - returning success to prevent retries",
+				"error", err,
+				"action", p.Action)
+			return nil // Return 204 to prevent GroupsIO from retrying
+		}
+
+		// For other errors (transient failures), return error to trigger GroupsIO retry
+		slog.ErrorContext(ctx, "webhook processing failed after retries",
+			"error", err,
+			"action", p.Action,
+			"retries", constants.WebhookMaxRetries)
+		return &mailinglistservice.InternalServerError{Message: "webhook processing failed"}
 	}
 
-	return nil // Always return nil for 204 No Content
+	// Success - return 204 No Content
+	return nil
 }
 
 // Helper functions

internal/service/grpsio_mailing_list_writer.go

@@ -15,6 +15,7 @@ import (
 	"github.com/linuxfoundation/lfx-v2-mailing-list-service/internal/infrastructure/groupsio"
 	"github.com/linuxfoundation/lfx-v2-mailing-list-service/pkg/constants"
 	"github.com/linuxfoundation/lfx-v2-mailing-list-service/pkg/errors"
+	"github.com/linuxfoundation/lfx-v2-mailing-list-service/pkg/log"
 	"github.com/linuxfoundation/lfx-v2-mailing-list-service/pkg/utils"
 )
 
@@ -158,7 +159,7 @@ func (ml *grpsIOWriterOrchestrator) CreateGrpsIOMailingList(ctx context.Context,
 			if existing, revision, checkErr := ml.ensureMailingListIdempotent(ctx, request); checkErr == nil && existing != nil {
 				slog.InfoContext(ctx, "constraint conflict resolved - returning existing record (race condition)",
 					"mailing_list_uid", existing.UID,
-					"subgroup_id", request.SubgroupID)
+					"subgroup_id", log.LogOptionalInt64(request.SubgroupID))
 				return existing, revision, nil
 			}
 		}
@@ -236,7 +237,7 @@ func (ml *grpsIOWriterOrchestrator) CreateGrpsIOMailingList(ctx context.Context,
 		"mailing_list_uid", createdMailingList.UID,
 		"group_name", createdMailingList.GroupName,
 		"source", createdMailingList.Source,
-		"subgroup_id", createdMailingList.SubgroupID,
+		"subgroup_id", log.LogOptionalInt64(createdMailingList.SubgroupID),
 		"parent_uid", createdMailingList.ServiceUID,
 		"public", createdMailingList.Public,
 		"committee_based", createdMailingList.IsCommitteeBased())

internal/service/grpsio_webhook_processor.go

@@ -108,7 +108,7 @@ func (p *grpsIOWebhookProcessor) ProcessEvent(ctx context.Context, event *model.
 
 func (p *grpsIOWebhookProcessor) handleSubGroupCreated(ctx context.Context, event *model.GrpsIOWebhookEvent) error {
 	if event.Group == nil {
-		return fmt.Errorf("missing group information in created_subgroup event")
+		return errors.NewValidation("missing group information in created_subgroup event")
 	}
 
 	parentGroupID := uint64(event.Group.ParentGroupID)
@@ -233,7 +233,7 @@ func (p *grpsIOWebhookProcessor) handleSubGroupDeleted(ctx context.Context, even
 
 func (p *grpsIOWebhookProcessor) handleMemberAdded(ctx context.Context, event *model.GrpsIOWebhookEvent) error {
 	if event.MemberInfo == nil {
-		return fmt.Errorf("missing member info in added_member event")
+		return errors.NewValidation("missing member info in added_member event")
 	}
 
 	memberID := int64(event.MemberInfo.ID)
@@ -308,7 +308,7 @@ func (p *grpsIOWebhookProcessor) handleMemberAdded(ctx context.Context, event *m
 
 func (p *grpsIOWebhookProcessor) handleMemberRemoved(ctx context.Context, event *model.GrpsIOWebhookEvent) error {
 	if event.MemberInfo == nil {
-		return fmt.Errorf("missing member info in removed_member event")
+		return errors.NewValidation("missing member info in removed_member event")
 	}
 
 	memberID := uint64(event.MemberInfo.ID)
@@ -356,7 +356,7 @@ func (p *grpsIOWebhookProcessor) handleMemberRemoved(ctx context.Context, event
 
 func (p *grpsIOWebhookProcessor) handleMemberBanned(ctx context.Context, event *model.GrpsIOWebhookEvent) error {
 	if event.MemberInfo == nil {
-		return fmt.Errorf("missing member info in ban_members event")
+		return errors.NewValidation("missing member info in ban_members event")
 	}
 
 	slog.InfoContext(ctx, "received ban_members event",

pkg/log/log.go

@@ -117,3 +117,22 @@ func Priority(level string) slog.Attr {
 func PriorityCritical() slog.Attr {
 	return Priority(priorityCritical)
 }
+
+// LogOptionalInt64 creates an slog.Value for optional int64 pointers.
+// Returns nil value if pointer is nil, otherwise logs the dereferenced value.
+// This helper ensures consistent logging of nullable int64 fields like IDs or timestamps.
+//
+// Example usage:
+//
+//	slog.InfoContext(ctx, "operation completed",
+//	    "subgroup_id", utils.LogOptionalInt64(record.SubgroupID))
+//
+// Logs:
+//   - When SubgroupID is nil: "subgroup_id": null
+//   - When SubgroupID is &123: "subgroup_id": 123
+func LogOptionalInt64(val *int64) slog.Value {
+	if val == nil {
+		return slog.AnyValue(nil)
+	}
+	return slog.Int64Value(*val)
+}

pkg/utils/retry_test.go

@@ -151,11 +151,11 @@ func TestRetryWithExponentialBackoff_ExponentialDelay(t *testing.T) {
 	assert.GreaterOrEqual(t, delay3, 40*time.Millisecond)
 
 	// Verify each delay is approximately double the previous
-	// Allow for some timing variance (within 2x range)
-	assert.LessOrEqual(t, delay1, 2*delay1)
+	// Allow for some timing variance (within ~2x range)
 	assert.GreaterOrEqual(t, delay2, delay1)
-	assert.LessOrEqual(t, delay2, 4*delay1)
+	assert.LessOrEqual(t, delay2, 2*delay1)
 	assert.GreaterOrEqual(t, delay3, delay2)
+	assert.LessOrEqual(t, delay3, 2*delay2)
 }
 
 func TestRetryWithExponentialBackoff_MaxDelayRespected(t *testing.T) {