chore: Centralize retry and add to missing places (#34)

* chore: Centralize retry and add to missing places

Co-Authored-By: Claude <noreply@anthropic.com>

* chore: trigger workflows

* chore: Trigger tests on pull request as well

* chore: Add missing needs

* Revert "chore: Trigger tests on pull request as well"

This reverts commit 5a82419.

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: mvicknr

.github/workflows/test_action.yml

@@ -223,6 +223,7 @@ jobs:
           fi
 
   test-docs-flow:
+    needs: setup-test-tag
     name: Test Documentation Flow (MDX Parsing)
     runs-on: ubuntu-latest
     steps:

cmd/agent-metadata-action/main_test.go

@@ -813,9 +813,9 @@ func TestRunAgentFlow_SigningError_ServiceFailure(t *testing.T) {
 
 	outputStr := getStdout()
 
-	// Verify retries occurred (MaxRetries = 3 from sign package)
+	// Verify retries occurred (3 attempts from retry package)
 	assert.Equal(t, 3, requestCount, "Should have made 3 signing requests (retries)")
 	assert.Contains(t, outputStr, "Signing attempt 1 failed")
 	assert.Contains(t, outputStr, "Signing attempt 2 failed")
-	assert.Contains(t, outputStr, "Failed to sign manifest index after 3 attempts")
+	assert.Contains(t, outputStr, "Failed to sign manifest index")
 }

internal/client/instrumentation.go

@@ -11,6 +11,7 @@ import (
 
 	"agent-metadata-action/internal/logging"
 	"agent-metadata-action/internal/models"
+	"agent-metadata-action/internal/retry"
 )
 
 // InstrumentationClient handles instrumentation metadata operations
@@ -69,91 +70,112 @@ func (c *InstrumentationClient) SendMetadata(ctx context.Context, agentType stri
 			"agent.version":   agentVersion,
 		})
 		logging.Errorf(ctx, "Failed to marshal metadata: %v", err)
-		return fmt.Errorf("failed to marshal metadata: %w", err)
+		return retry.NewNonRetryableError(fmt.Errorf("failed to marshal metadata: %w", err))
 	}
 	logging.Debugf(ctx, "JSON payload size: %d bytes", len(jsonBody))
 	logging.Debugf(ctx, "Configuration definitions count: %d", len(metadata.ConfigurationDefinitions))
 	logging.Debugf(ctx, "Agent control entries: %d", len(metadata.AgentControlDefinitions))
 
-	// Create HTTP request
-	logging.Debug(ctx, "Creating HTTP POST request...")
-	req, err := http.NewRequestWithContext(ctx, "POST", url, bytes.NewBuffer(jsonBody))
-	if err != nil {
-		logging.NoticeErrorWithCategory(ctx, err, "metadata.send", map[string]interface{}{
-			"error.operation": "create_http_request",
-			"http.url":        url,
-			"agent.type":      agentType,
-			"agent.version":   agentVersion,
-		})
-		logging.Errorf(ctx, "Failed to create request: %v", err)
-		return fmt.Errorf("failed to create request: %w", err)
+	// Execute request with retry logic
+	retryConfig := retry.Config{
+		MaxAttempts: 3,
+		BaseDelay:   2 * time.Second,
+		Operation:   "Metadata submission",
 	}
 
-	// Set headers
-	logging.Debug(ctx, "Setting request headers...")
-	req.Header.Set("Content-Type", "application/json")
-	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", c.token))
-	// SECURITY: Token is in header but not logged
+	err = retry.Do(ctx, retryConfig, func() error {
+		// Create HTTP request (must be recreated for each retry)
+		logging.Debug(ctx, "Creating HTTP POST request...")
+		req, err := http.NewRequestWithContext(ctx, "POST", url, bytes.NewBuffer(jsonBody))
+		if err != nil {
+			logging.NoticeErrorWithCategory(ctx, err, "metadata.send", map[string]interface{}{
+				"error.operation": "create_http_request",
+				"http.url":        url,
+				"agent.type":      agentType,
+				"agent.version":   agentVersion,
+			})
+			logging.Errorf(ctx, "Failed to create request: %v", err)
+			return retry.NewNonRetryableError(fmt.Errorf("failed to create request: %w", err))
+		}
 
-	// Execute request
-	logging.Debug(ctx, "Sending HTTP request...")
-	startTime := time.Now()
-	resp, err := c.httpClient.Do(req)
-	duration := time.Since(startTime)
+		// Set headers
+		logging.Debug(ctx, "Setting request headers...")
+		req.Header.Set("Content-Type", "application/json")
+		req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", c.token))
+
+		// Execute request
+		logging.Debug(ctx, "Sending HTTP request...")
+		startTime := time.Now()
+		resp, err := c.httpClient.Do(req)
+		duration := time.Since(startTime)
+
+		if err != nil {
+			logging.NoticeErrorWithCategory(ctx, err, "metadata.send", map[string]interface{}{
+				"error.operation": "execute_http_request",
+				"http.url":        url,
+				"http.duration":   duration.String(),
+				"agent.type":      agentType,
+				"agent.version":   agentVersion,
+			})
+			logging.Errorf(ctx, "HTTP request failed after %s: %v", duration, err)
+			return fmt.Errorf("failed to send metadata: %w", err)
+		}
+		defer resp.Body.Close()
 
-	if err != nil {
-		logging.NoticeErrorWithCategory(ctx, err, "metadata.send", map[string]interface{}{
-			"error.operation": "execute_http_request",
-			"http.url":        url,
-			"http.duration":   duration.String(),
-			"agent.type":      agentType,
-			"agent.version":   agentVersion,
-		})
-		logging.Errorf(ctx, "HTTP request failed after %s: %v", duration, err)
-		return fmt.Errorf("failed to send metadata: %w", err)
-	}
-	defer resp.Body.Close()
+		logging.Debugf(ctx, "Response received in %s", duration)
+		logging.Debugf(ctx, "HTTP status code: %d %s", resp.StatusCode, resp.Status)
 
-	logging.Debugf(ctx, "Response received in %s", duration)
-	logging.Debugf(ctx, "HTTP status code: %d %s", resp.StatusCode, resp.Status)
+		// Read response body for error details (with size limit)
+		logging.Debug(ctx, "Reading response body...")
+		body, err := io.ReadAll(resp.Body)
+		if err != nil {
+			logging.Errorf(ctx, "Failed to read response body: %v", err)
+			return fmt.Errorf("failed to read response: %w", err)
+		}
+		logging.Debugf(ctx, "Response body size: %d bytes", len(body))
+
+		// Check for non-2xx status codes
+		if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+			// Log response body for debugging, but truncate if too large
+			responsePreview := string(body)
+			if len(responsePreview) > 500 {
+				responsePreview = responsePreview[:500] + "... (truncated)"
+			}
+
+			err := fmt.Errorf("metadata submission failed with status %d: %s", resp.StatusCode, string(body))
+			logging.NoticeErrorWithCategory(ctx, err, "metadata.send", map[string]interface{}{
+				"error.operation":    "http_non_2xx_response",
+				"http.status_code":   resp.StatusCode,
+				"http.url":           url,
+				"http.response_body": responsePreview,
+				"agent.type":         agentType,
+				"agent.version":      agentVersion,
+			})
+			logging.Errorf(ctx, "Metadata submission failed with status %d", resp.StatusCode)
+			logging.Debugf(ctx, "Error response body: %s", responsePreview)
+
+			// Determine if error is retryable
+			// Retry on: 5xx (server errors), 408 (timeout), 429 (rate limit)
+			// Don't retry: 4xx (client errors, except 408 and 429)
+			isRetryable := resp.StatusCode >= 500 || resp.StatusCode == 408 || resp.StatusCode == 429
+			if !isRetryable {
+				return retry.NewNonRetryableError(err)
+			}
+			return err
+		}
 
-	// Read response body for error details (with size limit)
-	logging.Debug(ctx, "Reading response body...")
-	body, err := io.ReadAll(resp.Body)
-	if err != nil {
-		logging.Errorf(ctx, "Failed to read response body: %v", err)
-		return fmt.Errorf("failed to read response: %w", err)
-	}
-	logging.Debugf(ctx, "Response body size: %d bytes", len(body))
-
-	// Check for non-2xx status codes
-	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		// Log response body for debugging, but truncate if too large
-		responsePreview := string(body)
-		if len(responsePreview) > 500 {
-			responsePreview = responsePreview[:500] + "... (truncated)"
+		// Success logging
+		if len(body) > 0 {
+			logging.Debugf(ctx, "Success response: %s", string(body))
 		}
 
-		err := fmt.Errorf("metadata submission failed with status %d: %s", resp.StatusCode, string(body))
-		logging.NoticeErrorWithCategory(ctx, err, "metadata.send", map[string]interface{}{
-			"error.operation":    "http_non_2xx_response",
-			"http.status_code":   resp.StatusCode,
-			"http.url":           url,
-			"http.response_body": responsePreview,
-			"agent.type":         agentType,
-			"agent.version":      agentVersion,
-		})
-		logging.Errorf(ctx, "Metadata submission failed with status %d", resp.StatusCode)
-		logging.Debugf(ctx, "Error response body: %s", responsePreview)
+		return nil
+	})
 
+	if err != nil {
 		return err
 	}
 
-	// Success logging
 	logging.Notice(ctx, "Metadata successfully submitted to instrumentation service")
-	if len(body) > 0 {
-		logging.Debugf(ctx, "Success response: %s", string(body))
-	}
-
 	return nil
 }

internal/client/instrumentation_test.go

@@ -312,7 +312,7 @@ func TestSendMetadata_ContextCancellation(t *testing.T) {
 	_ = getStdout()
 
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "failed to send metadata")
+	assert.Contains(t, err.Error(), "retry cancelled")
 }
 
 func TestSendMetadata_SuccessWithResponseBody(t *testing.T) {

internal/oci/client.go

@@ -3,6 +3,7 @@ package oci
 import (
 	"agent-metadata-action/internal/logging"
 	"agent-metadata-action/internal/models"
+	"agent-metadata-action/internal/retry"
 	"bytes"
 	"context"
 	"encoding/json"
@@ -58,21 +59,21 @@ func NewClient(ctx context.Context, registry, username, password string) (*Clien
 func (c *Client) UploadArtifact(ctx context.Context, artifact *models.ArtifactDefinition, artifactPath, version string) (string, int64, error) {
 	tempDir, err := os.MkdirTemp("", "oras-upload-*")
 	if err != nil {
-		return "", 0, fmt.Errorf("failed to create temp directory: %w", err)
+		return "", 0, retry.NewNonRetryableError(fmt.Errorf("failed to create temp directory: %w", err))
 	}
 	defer os.RemoveAll(tempDir)
 
 	fs, err := file.New(tempDir)
 	if err != nil {
-		return "", 0, fmt.Errorf("failed to create file store: %w", err)
+		return "", 0, retry.NewNonRetryableError(fmt.Errorf("failed to create file store: %w", err))
 	}
 	defer fs.Close()
 
 	layerAnnotations := CreateLayerAnnotations(artifact, version)
 
 	layerDesc, err := fs.Add(ctx, artifact.Name, artifact.GetMediaType(), artifactPath)
 	if err != nil {
-		return "", 0, fmt.Errorf("failed to add file to store: %w", err)
+		return "", 0, retry.NewNonRetryableError(fmt.Errorf("failed to add file to store: %w", err))
 	}
 
 	layerDesc.Annotations = layerAnnotations
@@ -86,7 +87,7 @@ func (c *Client) UploadArtifact(ctx context.Context, artifact *models.ArtifactDe
 	}
 	configBytes, err := json.Marshal(config)
 	if err != nil {
-		return "", 0, fmt.Errorf("failed to marshal config: %w", err)
+		return "", 0, retry.NewNonRetryableError(fmt.Errorf("failed to marshal config: %w", err))
 	}
 
 	configDesc := ocispec.Descriptor{
@@ -96,7 +97,7 @@ func (c *Client) UploadArtifact(ctx context.Context, artifact *models.ArtifactDe
 	}
 
 	if err = fs.Push(ctx, configDesc, bytes.NewReader(configBytes)); err != nil {
-		return "", 0, fmt.Errorf("failed to push config: %w", err)
+		return "", 0, retry.NewNonRetryableError(fmt.Errorf("failed to push config: %w", err))
 	}
 
 	artifactType := artifact.GetArtifactType()
@@ -108,25 +109,40 @@ func (c *Client) UploadArtifact(ctx context.Context, artifact *models.ArtifactDe
 
 	manifestDesc, err := oras.PackManifest(ctx, fs, oras.PackManifestVersion1_1, artifactType, packOpts)
 	if err != nil {
-		return "", 0, fmt.Errorf("failed to pack manifest: %w", err)
+		return "", 0, retry.NewNonRetryableError(fmt.Errorf("failed to pack manifest: %w", err))
 	}
 
 	// Tag manifest in file store with a temporary tag so it can be referenced during copy
 	tempTag := "temp-manifest"
 	if err = fs.Tag(ctx, manifestDesc, tempTag); err != nil {
-		return "", 0, fmt.Errorf("failed to tag manifest in file store: %w", err)
+		return "", 0, retry.NewNonRetryableError(fmt.Errorf("failed to tag manifest in file store: %w", err))
 	}
 
-	pushCtx, cancel := context.WithTimeout(ctx, 5*time.Minute)
-	defer cancel()
-
 	logging.Debugf(ctx, "Pushing artifact %s to registry by digest (digest: %s)", artifact.Name, manifestDesc.Digest.String())
 
+	// Copy manifest and blobs to remote registry by digest with retry logic
+	retryConfig := retry.Config{
+		MaxAttempts: 3,
+		BaseDelay:   2 * time.Second,
+		Operation:   "OCI artifact upload",
+	}
+
 	// Copy manifest and blobs to remote registry by digest
 	copyOpts := oras.CopyOptions{}
 	digestRef := manifestDesc.Digest.String()
-	if _, err = oras.Copy(pushCtx, fs, tempTag, c.repo, digestRef, copyOpts); err != nil {
-		return "", 0, fmt.Errorf("failed to push artifact to registry: %w", err)
+
+	err = retry.Do(ctx, retryConfig, func() error {
+		pushCtx, cancel := context.WithTimeout(ctx, 5*time.Minute)
+		defer cancel()
+
+		if _, err := oras.Copy(pushCtx, fs, tempTag, c.repo, digestRef, copyOpts); err != nil {
+			return fmt.Errorf("failed to push artifact to registry: %w", err)
+		}
+		return nil
+	})
+
+	if err != nil {
+		return "", 0, err
 	}
 
 	logging.Debugf(ctx, "Successfully uploaded artifact by digest: %s", manifestDesc.Digest.String())

internal/parser/mdx.go

@@ -26,6 +26,7 @@ const (
 	//EBPF   Subject = "" @todo update once eBPF is publishing release notes
 )
 
+// SubjectToAgentTypeMapping list based on these: https://source.datanerd.us/fleet-management/fleet-deployment-api/blob/master/src/main/java/com/newrelic/fleetdeploymentapi/constant/AgentConstants.java
 var SubjectToAgentTypeMapping = map[Subject]string{
 	DotNet:   "NRDotNetAgent",
 	Infra:    "NRInfra", // maps to the same agent type for k8s & host