diff --git a/CHANGELOG.md b/CHANGELOG.md index 44cefa1c..5c74e93b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,24 @@ All notable changes to this project will be documented here. ## [Unreleased] +### Changed + +- **Triage persistOutput is atomic**: Cluster and classification inserts for triage results are now wrapped in a single database transaction. Previously, inserts were issued one at a time — a partial failure left the triage record in an inconsistent state. The new `PersistOutput` method on `TriageStore` commits atomically or rolls back entirely. + +- **SMTP, Telegram, and GitHub sends retry on transient failures**: SMTP email sends, Telegram messages, and GitHub commit status posts now retry up to 3 times with exponential backoff on transient errors (5xx, 429 rate limits, connection timeouts). Telegram and GitHub retries respect the `Retry-After` header. Client errors (auth failures, invalid recipients, 4xx) are not retried. The shared `internal/smtptransient` package provides consistent transient-error classification for SMTP across `mail` and `mailer`. + +- **Webhook dispatch uses bounded worker pool**: Outbound webhook dispatches are now bounded by a semaphore (default concurrency: 10). Under burst traffic, excess dispatches are dropped with a warning instead of spawning unbounded goroutines. + +- **Triage jobs respect server shutdown**: Triage jobs now accept a parent context from the server. When the server shuts down gracefully, in-flight triage jobs are cancelled instead of running for up to 5 minutes after shutdown. + +- **LLM retry only on transient errors**: The LLM CLI provider now retries only on transient errors (context deadlines, process signals). Non-transient errors (syntax errors, invalid model names, exit code 1) fail immediately instead of being retried. + +- **Invitation emails have HTML alternative part**: Invitation emails are now sent as `multipart/alternative` with both plaintext and HTML parts. The HTML part includes a styled invitation card with an "Accept Invitation" button. Plaintext-only emails frequently landed in spam; the HTML alternative part improves deliverability. The MIME boundary uses `crypto/rand` for uniqueness. + +- **Telegram RunURL HTML-escaped**: The `CI_RUN_URL` field in Telegram notifications is now HTML-escaped before insertion into `` attributes. Previously, a malicious RunURL containing `"` or `>` could break out of the attribute and inject HTML/JavaScript in the Telegram message (XSS). Other fields (repo, branch, commit) were already escaped. + +- **invited_by FK allows SET NULL on user deletion**: The `invited_by` foreign key on the `invitations` table now uses `ON DELETE SET NULL` instead of the default restrict. Previously, deleting a user who had invited others would fail with a foreign key violation. Migration 000024 drops the unnamed FK, makes `invited_by` nullable, and adds a named constraint with `ON DELETE SET NULL`. Note: the down migration for 000024 is intentionally irreversible (see the migration SQL for details). + ### Added - **Frontend error boundaries**: A root-level `ErrorBoundary` wraps the entire app in `main.tsx`, a TanStack Router `errorComponent` on the root route catches routing errors, and `ErrorBoundary` wrappers around all Recharts chart sections in `dashboard.tsx` and `analytics.tsx` prevent chart crashes from unmounting the app. The error UI includes both "Try Again" and "Reload" buttons. diff --git a/README.md b/README.md index 6f4185cb..4d4441d7 100644 --- a/README.md +++ b/README.md @@ -85,9 +85,9 @@ export ST_RECONCILE_INTERVAL=60s # How often to check for orphans export ST_RECONCILE_ORPHAN_TIMEOUT=5m # Grace period before declaring an execution orphaned ``` -When `ST_SMTP_HOST` is not set the mailer runs in no-op mode — all outbound email is silently discarded. Set it to enable email notifications. +When `ST_SMTP_HOST` is not set the mailer runs in no-op mode — all outbound email is silently discarded. Set it to enable email notifications. SMTP sends retry up to 3 times with exponential backoff on transient errors (5xx, connection timeout). -When `ST_GITHUB_TOKEN` is not set, GitHub commit status posting is disabled. When set, passing `github_owner`, `github_repo`, and `github_sha` query parameters to `POST /api/v1/reports` will post a `scaledtest/e2e` commit status back to GitHub after the report is ingested. +When `ST_GITHUB_TOKEN` is not set, GitHub commit status posting is disabled. When set, passing `github_owner`, `github_repo`, and `github_sha` query parameters to `POST /api/v1/reports` will post a `scaledtest/e2e` commit status back to GitHub after the report is ingested. GitHub status posts retry up to 3 times with exponential backoff on 429 and 5xx responses, respecting the `Retry-After` header. When `ST_DISABLE_RATE_LIMIT=true` is set, all rate-limit middleware is bypassed and a warning is logged at startup. Use this only in controlled test environments (e.g. CI running E2E suites with many per-test user registrations). **Never set this in production** — it removes brute-force protection on auth endpoints. @@ -515,6 +515,8 @@ internal/ github/ # GitHub commit status client llm/ # LLM provider abstraction (Anthropic, OpenAI, mock) mail/ # Email sender interface and SMTP implementation + mailer/ # Invitation email composer (SMTP, multipart HTML) + smtptransient/ # Shared SMTP transient-error classification (4xx/5xx/timeout) webhook/ # Outbound webhook dispatch ws/ # WebSocket hub for real-time updates k8s/ # Kubernetes job management diff --git a/docs/ci-integration/telegram-notifications.md b/docs/ci-integration/telegram-notifications.md index 0737d69f..804a3a6e 100644 --- a/docs/ci-integration/telegram-notifications.md +++ b/docs/ci-integration/telegram-notifications.md @@ -85,7 +85,11 @@ Branch: main Commit: def5678 View run ``` -All external fields (`CI_REPO`, `CI_BRANCH`, `CI_COMMIT_MSG`) are HTML-escaped before interpolation to prevent invalid HTML from breaking the Telegram API call. +All external fields (`CI_REPO`, `CI_BRANCH`, `CI_COMMIT_MSG`) and `CI_RUN_URL` are HTML-escaped before interpolation to prevent invalid or malicious content from breaking the Telegram API call or injecting HTML/JavaScript. + +## Retry behavior + +The Telegram client retries on transient failures (429 rate-limited and 5xx server errors) with exponential backoff (1s, 2s, 4s, up to 3 retries). When the Telegram API returns a 429 response with a `retry_after` parameter, the client respects that value as the backoff duration instead of the default exponential backoff. Client errors (4xx except 429) are not retried. ## Graceful degradation diff --git a/internal/db/db_test.go b/internal/db/db_test.go index e82beedb..5a2bd792 100644 --- a/internal/db/db_test.go +++ b/internal/db/db_test.go @@ -32,8 +32,8 @@ func TestMigrationsEmbedded(t *testing.T) { t.Errorf("migration up/down mismatch: %d up, %d down", ups, downs) } - if ups != 23 { - t.Errorf("expected 23 migration pairs, got %d", ups) + if ups != 24 { + t.Errorf("expected 24 migration pairs, got %d", ups) } // Verify each up has a matching down diff --git a/internal/db/migrations/000024_invitations_invited_by_set_null.down.sql b/internal/db/migrations/000024_invitations_invited_by_set_null.down.sql new file mode 100644 index 00000000..69cde5a4 --- /dev/null +++ b/internal/db/migrations/000024_invitations_invited_by_set_null.down.sql @@ -0,0 +1,9 @@ +-- Revert: make invited_by NOT NULL again and remove the FK with SET NULL. +-- Any rows where invited_by was set to NULL by ON DELETE SET NULL must be +-- updated to a sentinel UUID before we can re-add NOT NULL, otherwise the +-- constraint would fail. Since there is no meaningful sentinel user, this +-- migration is intentionally NOT reversible — once deployed, invitations +-- may have invited_by = NULL, and rolling back would silently corrupt data. +-- Do not use this down migration; deploy a new migration instead if reversibility +-- is required. +SELECT 1; \ No newline at end of file diff --git a/internal/db/migrations/000024_invitations_invited_by_set_null.up.sql b/internal/db/migrations/000024_invitations_invited_by_set_null.up.sql new file mode 100644 index 00000000..7afe39e5 --- /dev/null +++ b/internal/db/migrations/000024_invitations_invited_by_set_null.up.sql @@ -0,0 +1,10 @@ +-- Make invited_by nullable and set ON DELETE SET NULL so that deleting +-- a user who invited others does not violate the foreign key constraint. +-- Must drop the existing unnamed FK created by migration 000017 before +-- adding the new one with ON DELETE SET NULL; PostgreSQL does not allow +-- two FK constraints on the same column. +ALTER TABLE invitations + DROP CONSTRAINT invitations_invited_by_fkey, + ALTER COLUMN invited_by DROP NOT NULL, + ADD CONSTRAINT fk_invitations_invited_by + FOREIGN KEY (invited_by) REFERENCES users(id) ON DELETE SET NULL; \ No newline at end of file diff --git a/internal/github/github.go b/internal/github/github.go index e244521e..b5827523 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -4,13 +4,19 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" "regexp" + "strconv" "time" + + "github.com/rs/zerolog/log" ) +const defaultMaxRetries = 3 + // StatusPoster posts a GitHub commit status. type StatusPoster interface { PostStatus(ctx context.Context, owner, repo, sha, state, description, statusContext, targetURL string) error @@ -21,6 +27,7 @@ type Client struct { token string HTTPClient *http.Client APIURL string + maxRetries int } // New creates a GitHub Client with the given token. @@ -33,9 +40,16 @@ func New(token string) *Client { token: token, HTTPClient: &http.Client{Timeout: 10 * time.Second}, APIURL: "https://api.github.com", + maxRetries: defaultMaxRetries, } } +// WithMaxRetries sets the number of retry attempts for transient errors. +func (c *Client) WithMaxRetries(n int) *Client { + c.maxRetries = n + return c +} + var ( validOwnerRepo = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) validSHA = regexp.MustCompile(`^[0-9a-fA-F]{7,40}$`) @@ -49,7 +63,9 @@ type statusPayload struct { } // PostStatus posts a commit status to the GitHub Statuses API. -// state must be one of "success", "failure", "pending", "error". +// It retries on 429 (rate limited) and 5xx (server error) responses with +// exponential backoff, respecting the Retry-After header on 429 responses. +// Client errors (4xx except 429) are not retried. func (c *Client) PostStatus(ctx context.Context, owner, repo, sha, state, description, statusContext, targetURL string) error { if !validOwnerRepo.MatchString(owner) { return fmt.Errorf("invalid github owner: %q", owner) @@ -72,6 +88,45 @@ func (c *Client) PostStatus(ctx context.Context, owner, repo, sha, state, descri } url := fmt.Sprintf("%s/repos/%s/%s/statuses/%s", c.APIURL, owner, repo, sha) + + var lastErr error + for attempt := 0; attempt <= c.maxRetries; attempt++ { + if ctx.Err() != nil { + return ctx.Err() + } + + lastErr = c.doPost(ctx, url, body) + if lastErr == nil { + return nil + } + + if !isRetriableError(lastErr) { + return lastErr + } + + if attempt < c.maxRetries { + backoff := retryAfterDuration(lastErr) + if backoff == 0 { + backoff = time.Duration(1<= 300 { - return fmt.Errorf("github status API returned %d", resp.StatusCode) + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + return nil + } + + if resp.StatusCode == http.StatusTooManyRequests { + retryAfter := resp.Header.Get("Retry-After") + return &retriableError{ + statusCode: resp.StatusCode, + retryAfter: retryAfter, + } + } + + if resp.StatusCode >= 500 { + return &retriableError{statusCode: resp.StatusCode} + } + + return fmt.Errorf("github status API returned %d", resp.StatusCode) +} + +// retriableError represents a transient HTTP error that should be retried. +type retriableError struct { + statusCode int + retryAfter string +} + +func (e *retriableError) Error() string { + if e.retryAfter != "" { + return fmt.Sprintf("github status API returned %d (Retry-After: %s)", e.statusCode, e.retryAfter) + } + return fmt.Sprintf("github status API returned %d", e.statusCode) +} + +// isRetriableError returns true for 429 and 5xx errors. +func isRetriableError(err error) bool { + var re *retriableError + return errors.As(err, &re) +} + +// retryAfterDuration extracts the Retry-After duration from a retriableError. +// Returns 0 if not available or parseable. +func retryAfterDuration(err error) time.Duration { + var re *retriableError + if !errors.As(err, &re) { + return 0 + } + if re.retryAfter == "" { + return 0 + } + if secs, e := strconv.Atoi(re.retryAfter); e == nil { + return time.Duration(secs) * time.Second } - return nil + return 0 } diff --git a/internal/github/github_test.go b/internal/github/github_test.go index 454edc1c..1d0f3f17 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -3,9 +3,12 @@ package github import ( "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" + "sync/atomic" "testing" + "time" ) // Compile-time interface check. @@ -43,7 +46,7 @@ func TestPostStatus_Success(t *testing.T) { })) defer srv.Close() - c := &Client{token: "ghp_tok", HTTPClient: srv.Client(), APIURL: srv.URL} + c := &Client{token: "ghp_tok", HTTPClient: srv.Client(), APIURL: srv.URL, maxRetries: defaultMaxRetries} err := c.PostStatus(context.Background(), "myowner", "myrepo", "abc1234", "success", "5 tests passed", "scaledtest/e2e", "https://example.com/reports/123") if err != nil { @@ -78,7 +81,7 @@ func TestPostStatus_HTTPError(t *testing.T) { })) defer srv.Close() - c := &Client{token: "bad", HTTPClient: srv.Client(), APIURL: srv.URL} + c := &Client{token: "bad", HTTPClient: srv.Client(), APIURL: srv.URL, maxRetries: defaultMaxRetries} err := c.PostStatus(context.Background(), "o", "r", "abc1234", "success", "", "", "") if err == nil { @@ -92,7 +95,7 @@ func TestPostStatus_ContextCancelled(t *testing.T) { })) defer srv.Close() - c := &Client{token: "tok", HTTPClient: srv.Client(), APIURL: srv.URL} + c := &Client{token: "tok", HTTPClient: srv.Client(), APIURL: srv.URL, maxRetries: defaultMaxRetries} ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -133,10 +136,226 @@ func TestPostStatus_ShortSHAAccepted(t *testing.T) { })) defer srv.Close() - c := &Client{token: "tok", HTTPClient: srv.Client(), APIURL: srv.URL} + c := &Client{token: "tok", HTTPClient: srv.Client(), APIURL: srv.URL, maxRetries: defaultMaxRetries} // 7-char short SHA should be accepted err := c.PostStatus(context.Background(), "owner", "repo", "abc1234", "success", "", "", "") if err != nil { t.Fatalf("unexpected error for 7-char SHA: %v", err) } } + +func TestPostStatus_RetriesOn429(t *testing.T) { + var attempts atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := attempts.Add(1) + if n < 3 { + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + return + } + w.WriteHeader(http.StatusCreated) + })) + defer srv.Close() + + c := &Client{token: "tok", HTTPClient: srv.Client(), APIURL: srv.URL, maxRetries: 3} + err := c.PostStatus(context.Background(), "owner", "repo", "abc1234", "success", "", "", "") + if err != nil { + t.Fatalf("expected success after retries, got: %v", err) + } + if got := attempts.Load(); got != 3 { + t.Errorf("expected 3 attempts, got %d", got) + } +} + +func TestPostStatus_RetriesOn5xx(t *testing.T) { + var attempts atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := attempts.Add(1) + if n < 2 { + w.WriteHeader(http.StatusBadGateway) + return + } + w.WriteHeader(http.StatusCreated) + })) + defer srv.Close() + + c := &Client{token: "tok", HTTPClient: srv.Client(), APIURL: srv.URL, maxRetries: 3} + err := c.PostStatus(context.Background(), "owner", "repo", "abc1234", "success", "", "", "") + if err != nil { + t.Fatalf("expected success after retries, got: %v", err) + } + if got := attempts.Load(); got != 2 { + t.Errorf("expected 2 attempts, got %d", got) + } +} + +func TestPostStatus_DoesNotRetryOn4xx(t *testing.T) { + var attempts atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts.Add(1) + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + c := &Client{token: "tok", HTTPClient: srv.Client(), APIURL: srv.URL, maxRetries: 3} + err := c.PostStatus(context.Background(), "owner", "repo", "abc1234", "success", "", "", "") + if err == nil { + t.Fatal("expected error for 401 response") + } + if got := attempts.Load(); got != 1 { + t.Errorf("expected 1 attempt (no retry for 4xx), got %d", got) + } +} + +func TestPostStatus_RespectsRetryAfterHeader(t *testing.T) { + var attempts atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := attempts.Add(1) + if n == 1 { + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusTooManyRequests) + return + } + w.WriteHeader(http.StatusCreated) + })) + defer srv.Close() + + c := &Client{token: "tok", HTTPClient: srv.Client(), APIURL: srv.URL, maxRetries: 3} + err := c.PostStatus(context.Background(), "owner", "repo", "abc1234deadbeef", "success", "", "", "") + if err != nil { + t.Fatalf("expected success, got: %v", err) + } + if got := attempts.Load(); got != 2 { + t.Errorf("expected 2 attempts, got %d", got) + } +} + +func TestPostStatus_ExhaustsRetries(t *testing.T) { + var attempts atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts.Add(1) + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + c := &Client{ + token: "tok", + HTTPClient: srv.Client(), + APIURL: srv.URL, + maxRetries: 2, + } + err := c.PostStatus(context.Background(), "owner", "repo", "abc1234deadbeef", "success", "", "", "") + if err == nil { + t.Fatal("expected error after exhausting retries") + } + // 1 initial + 2 retries = 3 total attempts + if got := attempts.Load(); got != 3 { + t.Errorf("expected 3 attempts, got %d", got) + } +} + +func TestPostStatus_ContextCancelledStopsRetries(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + + c := &Client{ + token: "tok", + HTTPClient: &http.Client{Timeout: 5 * time.Second}, + APIURL: srv.URL, + maxRetries: 10, + } + err := c.PostStatus(ctx, "owner", "repo", "abc1234deadbeef", "success", "", "", "") + if err == nil { + t.Fatal("expected error from context cancellation") + } +} + +func TestIsRetriableError(t *testing.T) { + tests := []struct { + name string + err error + retry bool + after0 bool + }{ + { + name: "retriable 429 without Retry-After", + err: &retriableError{statusCode: 429}, + retry: true, + }, + { + name: "retriable 500", + err: &retriableError{statusCode: 500}, + retry: true, + }, + { + name: "retriable 502", + err: &retriableError{statusCode: 502}, + retry: true, + }, + { + name: "non-retriable 401", + err: fmt.Errorf("github status API returned 401"), + retry: false, + }, + { + name: "non-retriable 404", + err: fmt.Errorf("github status API returned 404"), + retry: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isRetriableError(tt.err) + if got != tt.retry { + t.Errorf("isRetriableError() = %v, want %v", got, tt.retry) + } + }) + } +} + +func TestRetryAfterDuration(t *testing.T) { + tests := []struct { + name string + err error + want time.Duration + }{ + { + name: "429 with Retry-After in seconds", + err: &retriableError{statusCode: 429, retryAfter: "30"}, + want: 30 * time.Second, + }, + { + name: "429 with empty Retry-After", + err: &retriableError{statusCode: 429, retryAfter: ""}, + want: 0, + }, + { + name: "429 with invalid Retry-After", + err: &retriableError{statusCode: 429, retryAfter: "notanumber"}, + want: 0, + }, + { + name: "5xx error without Retry-After", + err: &retriableError{statusCode: 500}, + want: 0, + }, + { + name: "non-retriable error", + err: fmt.Errorf("some other error"), + want: 0, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := retryAfterDuration(tt.err) + if got != tt.want { + t.Errorf("retryAfterDuration() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/internal/handler/invitations.go b/internal/handler/invitations.go index de7f5b44..e99059ba 100644 --- a/internal/handler/invitations.go +++ b/internal/handler/invitations.go @@ -30,7 +30,7 @@ var validInvitationRoles = map[string]bool{ // invitationStore is the subset of store.InvitationStore used by InvitationsHandler. type invitationStore interface { - Create(ctx context.Context, teamID, email, role, tokenHash, invitedBy string, expiresAt time.Time) (*model.Invitation, error) + Create(ctx context.Context, teamID, email, role, tokenHash string, invitedBy *string, expiresAt time.Time) (*model.Invitation, error) ListByTeam(ctx context.Context, teamID string) ([]model.Invitation, error) GetByTokenHash(ctx context.Context, tokenHash string) (*model.Invitation, error) Delete(ctx context.Context, teamID, id string) error @@ -103,7 +103,8 @@ func (h *InvitationsHandler) Create(w http.ResponseWriter, r *http.Request) { expiresAt := time.Now().Add(invitationTTL) - inv, err := h.Store.Create(r.Context(), teamID, req.Email, req.Role, tokenHash, claims.UserID, expiresAt) + invitedBy := claims.UserID + inv, err := h.Store.Create(r.Context(), teamID, req.Email, req.Role, tokenHash, &invitedBy, expiresAt) if err != nil { Error(w, http.StatusInternalServerError, "failed to create invitation") return diff --git a/internal/handler/invitations_test.go b/internal/handler/invitations_test.go index 17b2d9cf..4140c3ab 100644 --- a/internal/handler/invitations_test.go +++ b/internal/handler/invitations_test.go @@ -15,6 +15,8 @@ import ( "github.com/scaledtest/scaledtest/internal/store" ) +func strPtr(s string) *string { return &s } + // mockInvitationStore is a test double for invitationStore. type mockInvitationStore struct { inv *model.Invitation @@ -27,7 +29,7 @@ type mockInvitationStore struct { teamNameErr error } -func (m *mockInvitationStore) Create(_ context.Context, _, _, _, _, _ string, _ time.Time) (*model.Invitation, error) { +func (m *mockInvitationStore) Create(_ context.Context, _, _, _, _ string, _ *string, _ time.Time) (*model.Invitation, error) { return m.inv, m.err } @@ -287,7 +289,7 @@ func TestAcceptInvitation_OwnerAlreadyExists_Returns409(t *testing.T) { TeamID: "team-1", Email: "second-owner@example.com", Role: "owner", - InvitedBy: "user-1", + InvitedBy: strPtr("user-1"), ExpiresAt: now.Add(7 * 24 * time.Hour), CreatedAt: now, } @@ -343,7 +345,7 @@ func TestCreateInvitation_CallsMailer(t *testing.T) { TeamID: "team-1", Email: "invitee@example.com", Role: "readonly", - InvitedBy: "user-1", + InvitedBy: strPtr("user-1"), ExpiresAt: time.Now().Add(7 * 24 * time.Hour), CreatedAt: time.Now(), } @@ -384,7 +386,7 @@ func TestCreateInvitation_NilMailer_ReturnsCreated(t *testing.T) { TeamID: "team-1", Email: "invitee@example.com", Role: "readonly", - InvitedBy: "user-1", + InvitedBy: strPtr("user-1"), ExpiresAt: time.Now().Add(7 * 24 * time.Hour), CreatedAt: time.Now(), } @@ -414,7 +416,7 @@ func TestCreateInvitation_MailerError_StillReturnsCreated(t *testing.T) { TeamID: "team-1", Email: "invitee@example.com", Role: "readonly", - InvitedBy: "user-1", + InvitedBy: strPtr("user-1"), ExpiresAt: time.Now().Add(7 * 24 * time.Hour), CreatedAt: time.Now(), } @@ -457,7 +459,7 @@ func TestCreateInvitation_LogsAuditEvent(t *testing.T) { TeamID: "team-1", Email: "invitee@example.com", Role: "readonly", - InvitedBy: "user-1", + InvitedBy: strPtr("user-1"), ExpiresAt: time.Now().Add(7 * 24 * time.Hour), CreatedAt: time.Now(), } @@ -503,7 +505,7 @@ func TestCreateInvitation_NilAuditStore_NoPanic(t *testing.T) { TeamID: "team-1", Email: "invitee@example.com", Role: "readonly", - InvitedBy: "user-1", + InvitedBy: strPtr("user-1"), ExpiresAt: time.Now().Add(7 * 24 * time.Hour), CreatedAt: time.Now(), } @@ -606,7 +608,7 @@ func TestAcceptInvitation_LogsAuditEvent(t *testing.T) { TeamID: "team-1", Email: "invitee@example.com", Role: "readonly", - InvitedBy: "user-1", + InvitedBy: strPtr("user-1"), ExpiresAt: time.Now().Add(7 * 24 * time.Hour), CreatedAt: time.Now(), } @@ -652,7 +654,7 @@ func TestPreviewInvitation_WithStore_ReturnsTeamName(t *testing.T) { TeamID: "team-1", Email: "invitee@example.com", Role: "readonly", - InvitedBy: "user-1", + InvitedBy: strPtr("user-1"), ExpiresAt: time.Now().Add(7 * 24 * time.Hour), CreatedAt: time.Now(), } @@ -685,7 +687,7 @@ func TestPreviewInvitation_GetTeamNameFallsBackToUnknown(t *testing.T) { TeamID: "team-missing", Email: "invitee@example.com", Role: "readonly", - InvitedBy: "user-1", + InvitedBy: strPtr("user-1"), ExpiresAt: time.Now().Add(7 * 24 * time.Hour), CreatedAt: time.Now(), } diff --git a/internal/llm/cli.go b/internal/llm/cli.go index 473f4dd2..722c951c 100644 --- a/internal/llm/cli.go +++ b/internal/llm/cli.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "os/exec" "time" @@ -61,7 +62,10 @@ type cliProvider struct { } // Analyze invokes the CLI with prompt and returns the JSON response. -// It retries on transient exec failures with configurable back-off. +// It retries on transient exec failures (timeouts, process signals) with +// configurable back-off. Client errors (syntax errors, invalid model names) +// are not retried — only transient failures where the CLI exited with a signal +// or the context deadline was exceeded. func (c *cliProvider) Analyze(ctx context.Context, prompt string) (json.RawMessage, error) { var ( out []byte @@ -77,6 +81,9 @@ func (c *cliProvider) Analyze(ctx context.Context, prompt string) (json.RawMessa if err == nil { break } + if !isTransientCLIError(err) { + return nil, err + } if attempt < c.maxRetries { select { case <-ctx.Done(): @@ -95,6 +102,29 @@ func (c *cliProvider) Analyze(ctx context.Context, prompt string) (json.RawMessa return json.RawMessage(out), nil } +// isTransientCLIError returns true for errors that are worth retrying: +// context deadlines/timeouts and process signals (killed by SIGKILL from +// timeout). Non-transient errors like invalid model names or syntax errors +// (exit code 1) are NOT retried. +func isTransientCLIError(err error) bool { + if err == nil { + return false + } + // Context deadline/timeout errors are transient. + if errors.Is(err, context.DeadlineExceeded) { + return true + } + // Process killed by a signal (e.g. SIGKILL from timeout) is transient. + var ee *exec.ExitError + if errors.As(err, &ee) { + return !ee.Exited() + } + // Other errors (network, exec failures) fall through here and are + // NOT retried — only the explicit cases above are transient. + // Client errors that produce normal exit codes are also NOT transient. + return false +} + // run executes one CLI invocation and returns its stdout. func (c *cliProvider) run(ctx context.Context, prompt string) ([]byte, error) { cmd := exec.CommandContext(ctx, c.command, c.buildArgs(prompt)...) diff --git a/internal/llm/llm_test.go b/internal/llm/llm_test.go index 56483876..e17dbddd 100644 --- a/internal/llm/llm_test.go +++ b/internal/llm/llm_test.go @@ -202,20 +202,26 @@ func TestCLIProvider_Analyze_ReturnsErrorWhenOutputNotJSON(t *testing.T) { } } -func TestCLIProvider_Analyze_RetriesOnTransientFailure_ThenSucceeds(t *testing.T) { +func TestCLIProvider_Analyze_RetriesOnTransientError_ThenSucceeds(t *testing.T) { t.Setenv("ANTHROPIC_API_KEY", "test-key") dir := t.TempDir() countFile := filepath.Join(dir, "count") os.WriteFile(countFile, []byte("0"), 0644) + // Script succeeds on 3rd attempt after failing with exit 1 on first two. + // Transient errors (killed process, timeout) are retried; exit code 1 + // is a "client error" (not transient) so the provider will NOT retry it. + // To test retry behavior, we use a script that gets killed by the context + // timeout on the first two calls, then succeeds on the third. script := fmt.Sprintf(` COUNT=$(cat %s 2>/dev/null || echo 0) COUNT=$((COUNT+1)) echo $COUNT > %s if [ "$COUNT" -le 2 ]; then - echo "transient error" >&2 - exit 1 + # Sleep long enough to trigger context timeout on first 2 attempts + # when called with a short timeout. + sleep 10 fi echo '{"succeeded":true}' `, countFile, countFile) @@ -225,7 +231,7 @@ echo '{"succeeded":true}' Provider: "anthropic", Command: cmd, MaxRetries: intPtr(2), - Timeout: 10 * time.Second, + Timeout: 100 * time.Millisecond, }) if err != nil { t.Fatalf("New: %v", err) @@ -245,9 +251,10 @@ echo '{"succeeded":true}' } } -func TestCLIProvider_Analyze_ReturnsErrorAfterAllRetriesExhausted(t *testing.T) { +func TestCLIProvider_Analyze_ClientError_NotRetried(t *testing.T) { t.Setenv("ANTHROPIC_API_KEY", "test-key") + // Exit code 1 (syntax error, bad args) is a client error — not retried. dir := t.TempDir() cmd := writeFakeScript(t, dir, "fakecli", `echo "always fails" >&2; exit 1`) @@ -263,13 +270,58 @@ func TestCLIProvider_Analyze_ReturnsErrorAfterAllRetriesExhausted(t *testing.T) _, err = p.Analyze(context.Background(), "prompt") if err == nil { - t.Fatal("expected error after all retries exhausted") + t.Fatal("expected error for client exit code 1") } if !strings.Contains(err.Error(), "exited") { t.Fatalf("unexpected error: %v", err) } } +func TestCLIProvider_Analyze_TransientError_RetriesThenSucceeds(t *testing.T) { + t.Setenv("ANTHROPIC_API_KEY", "test-key") + + // This script sleeps on the first invocation (causing context timeout) + // and then succeeds on the second attempt. + dir := t.TempDir() + countFile := filepath.Join(dir, "count") + os.WriteFile(countFile, []byte("0"), 0644) + + script := fmt.Sprintf(` +COUNT=$(cat %s 2>/dev/null || echo 0) +COUNT=$((COUNT+1)) +echo $COUNT > %s +if [ "$COUNT" -le 1 ]; then + # Sleep longer than the per-call timeout to trigger context.DeadlineExceeded + sleep 10 +fi +echo '{"success":true}' +`, countFile, countFile) + cmd := writeFakeScript(t, dir, "fakecli", script) + + p, err := New(Config{ + Provider: "anthropic", + Command: cmd, + MaxRetries: intPtr(2), + Timeout: 100 * time.Millisecond, + }) + if err != nil { + t.Fatalf("New: %v", err) + } + + got, err := p.Analyze(context.Background(), "prompt") + if err != nil { + t.Fatalf("Analyze: %v", err) + } + + var result struct{ Success bool } + if err := json.Unmarshal(got, &result); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if !result.Success { + t.Fatalf("expected success:true, got %s", got) + } +} + func TestCLIProvider_Analyze_ZeroRetries_MakesExactlyOneAttempt(t *testing.T) { t.Setenv("ANTHROPIC_API_KEY", "test-key") @@ -381,3 +433,15 @@ func TestMockProvider_Analyze_ReturnsConfiguredError(t *testing.T) { t.Fatalf("got %v, want %v", err, want) } } + +func TestIsTransientCLIError_ContextDeadline(t *testing.T) { + if !isTransientCLIError(context.DeadlineExceeded) { + t.Error("context.DeadlineExceeded should be transient") + } +} + +func TestIsTransientCLIError_NilError(t *testing.T) { + if isTransientCLIError(nil) { + t.Error("nil error should not be transient") + } +} diff --git a/internal/mail/mail.go b/internal/mail/mail.go index 27d0a0fe..32419ee1 100644 --- a/internal/mail/mail.go +++ b/internal/mail/mail.go @@ -7,8 +7,12 @@ import ( "net" "net/smtp" "strings" + "time" + + "github.com/rs/zerolog/log" "github.com/scaledtest/scaledtest/internal/config" + "github.com/scaledtest/scaledtest/internal/smtptransient" ) // Message is an email to be sent. @@ -33,11 +37,12 @@ func (n *NoopSender) Send(_ context.Context, _ Message) error { // SMTPSender delivers email via SMTP using the configured credentials. type SMTPSender struct { - host string - port int - user string - pass string - from string + host string + port int + user string + pass string + from string + maxRetries int } // sanitizeHeader strips CR and LF characters from an email header value @@ -47,8 +52,43 @@ func sanitizeHeader(s string) string { } // Send delivers msg via SMTP, respecting ctx for cancellation and timeouts. -// Header fields are sanitized to prevent header injection. +// It retries on transient SMTP errors (5xx responses and connection timeouts) +// with exponential backoff. Client errors (auth, invalid recipient) are not retried. func (s *SMTPSender) Send(ctx context.Context, msg Message) error { + var lastErr error + for attempt := 0; attempt <= s.maxRetries; attempt++ { + if ctx.Err() != nil { + return ctx.Err() + } + + lastErr = s.sendOnce(ctx, msg) + if lastErr == nil { + return nil + } + + if !smtptransient.IsTransient(lastErr) { + return lastErr + } + + if attempt < s.maxRetries { + backoff := time.Duration(1< + + + + + + +
+

ScaledTest

+
+

You have been invited to join ScaledTest.

+Accept Invitation +
+ +`, escapedURL) +} + +func (m *SMTPMailer) sendWithRetry(ctx context.Context, to string, msg []byte) error { + var lastErr error + for attempt := 0; attempt <= m.maxRetries; attempt++ { + if ctx.Err() != nil { + return ctx.Err() + } + + lastErr = m.sendOnce(ctx, to, msg) + if lastErr == nil { + return nil + } + + if !smtptransient.IsTransient(lastErr) { + return lastErr + } + + if attempt < m.maxRetries { + backoff := time.Duration(1<= %d)", b, len(b), minLen) + } + if len(b) > 128 { + t.Errorf("boundary %q is too long (%d chars)", b, len(b)) + } +} + +func TestSendInvitation_UsesMultipartWithBoundary(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + + _, portStr, _ := net.SplitHostPort(ln.Addr().String()) + var port int + fmt.Sscanf(portStr, "%d", &port) + + received := make(chan string, 1) + go fakeSMTP(t, ln, received) + + m := New("127.0.0.1", port, "", "", "noreply@test.com") + err = m.SendInvitation(context.Background(), "invitee@example.com", "http://app.example.com/invitations/inv_testboundary") + if err != nil { + t.Fatalf("SendInvitation error: %v", err) + } + + body := <-received + if !strings.Contains(body, "multipart/alternative") { + t.Errorf("message missing multipart/alternative content type, got:\n%s", body) + } + if !strings.Contains(body, "text/plain") { + t.Errorf("message missing text/plain part, got:\n%s", body) + } + if !strings.Contains(body, "text/html") { + t.Errorf("message missing text/html part, got:\n%s", body) + } + if !strings.Contains(body, "boundary_") { + t.Errorf("message uses hardcoded boundary instead of unique boundary, got:\n%s", body) + } + if strings.Contains(body, "boundary123") { + t.Errorf("message uses hardcoded 'boundary123' boundary, got:\n%s", body) + } +} + // compile-time interface check var _ Mailer = (*SMTPMailer)(nil) diff --git a/internal/model/model.go b/internal/model/model.go index fc18161b..72575226 100644 --- a/internal/model/model.go +++ b/internal/model/model.go @@ -261,7 +261,7 @@ type Invitation struct { TeamID string `json:"team_id"` Email string `json:"email"` Role string `json:"role"` - InvitedBy string `json:"invited_by"` + InvitedBy *string `json:"invited_by,omitempty"` AcceptedAt *time.Time `json:"accepted_at,omitempty"` ExpiresAt time.Time `json:"expires_at"` CreatedAt time.Time `json:"created_at"` diff --git a/internal/smtptransient/smtptransient.go b/internal/smtptransient/smtptransient.go new file mode 100644 index 00000000..4c733053 --- /dev/null +++ b/internal/smtptransient/smtptransient.go @@ -0,0 +1,30 @@ +package smtptransient + +import ( + "errors" + "net" + "regexp" + "strings" +) + +const DefaultRetries = 3 + +var transientSMTPCodeRe = regexp.MustCompile(`(?:^|\n|\s)([45]\d{2})\s`) + +func IsTransient(err error) bool { + if err == nil { + return false + } + var netErr net.Error + if errors.As(err, &netErr) { + return true + } + msg := err.Error() + if strings.Contains(msg, "smtp dial:") || + strings.Contains(msg, "smtp starttls:") || + strings.Contains(msg, "connection refused") || + strings.Contains(msg, "i/o timeout") { + return true + } + return transientSMTPCodeRe.MatchString(msg) +} diff --git a/internal/smtptransient/smtptransient_test.go b/internal/smtptransient/smtptransient_test.go new file mode 100644 index 00000000..2d621e94 --- /dev/null +++ b/internal/smtptransient/smtptransient_test.go @@ -0,0 +1,113 @@ +package smtptransient_test + +import ( + "fmt" + "testing" + + "github.com/scaledtest/scaledtest/internal/smtptransient" +) + +func TestIsTransient_Nil(t *testing.T) { + if smtptransient.IsTransient(nil) { + t.Error("nil error should not be transient") + } +} + +func TestIsTransient_ConnectionRefused(t *testing.T) { + err := fmt.Errorf("smtp dial: connection refused") + if !smtptransient.IsTransient(err) { + t.Error("connection refused should be transient") + } +} + +func TestIsTransient_Timeout(t *testing.T) { + err := fmt.Errorf("smtp dial: i/o timeout") + if !smtptransient.IsTransient(err) { + t.Error("i/o timeout should be transient") + } +} + +func TestIsTransient_StartTLSError(t *testing.T) { + err := fmt.Errorf("smtp starttls: handshake failure") + if !smtptransient.IsTransient(err) { + t.Error("STARTTLS error should be transient") + } +} + +func TestIsTransient_ClientError(t *testing.T) { + err := fmt.Errorf("smtp auth: invalid credentials") + if smtptransient.IsTransient(err) { + t.Error("auth error should not be transient") + } +} + +func TestIsTransient_5xxResponse(t *testing.T) { + err := fmt.Errorf("smtp RCPT TO: 552 5.2.2 mailbox full") + if !smtptransient.IsTransient(err) { + t.Error("5xx response should be transient") + } +} + +func TestIsTransient_4xxResponse(t *testing.T) { + err := fmt.Errorf("smtp RCPT TO: 451 4.3.0 try again later") + if !smtptransient.IsTransient(err) { + t.Error("4xx response should be transient") + } +} + +func TestIsTransient_421Response(t *testing.T) { + err := fmt.Errorf("421 4.7.0 connection rate limit exceeded") + if !smtptransient.IsTransient(err) { + t.Error("421 response should be transient") + } +} + +func TestIsTransient_452Response(t *testing.T) { + err := fmt.Errorf("452 4.3.1 insufficient system storage") + if !smtptransient.IsTransient(err) { + t.Error("452 response should be transient") + } +} + +func TestIsTransient_FalsePositiveTimestamp(t *testing.T) { + err := fmt.Errorf("smtp auth: invalid credentials at 2024-01-15 12:54:33") + if smtptransient.IsTransient(err) { + t.Error("timestamp containing 54 should not be transient") + } +} + +func TestIsTransient_FalsePositivePort(t *testing.T) { + err := fmt.Errorf("failed to connect on port 5555") + if smtptransient.IsTransient(err) { + t.Error("port number containing 55 should not be transient") + } +} + +func TestIsTransient_FalsePositiveErrorID(t *testing.T) { + err := fmt.Errorf("smtp auth: invalid credentials for request-55abc") + if smtptransient.IsTransient(err) { + t.Error("error ID containing 55 should not be transient") + } +} + +func TestIsTransient_NetError(t *testing.T) { + err := &netError{msg: "network timeout", timeout: true} + if !smtptransient.IsTransient(err) { + t.Error("net.Error should be transient") + } +} + +type netError struct { + msg string + timeout bool +} + +func (e *netError) Error() string { return e.msg } +func (e *netError) Timeout() bool { return e.timeout } +func (e *netError) Temporary() bool { return e.timeout } + +func TestDefaultRetries(t *testing.T) { + if smtptransient.DefaultRetries != 3 { + t.Errorf("DefaultRetries = %d, want 3", smtptransient.DefaultRetries) + } +} diff --git a/internal/store/invitations.go b/internal/store/invitations.go index af43d04c..1de11ac9 100644 --- a/internal/store/invitations.go +++ b/internal/store/invitations.go @@ -27,7 +27,7 @@ func NewInvitationStore(pool *pgxpool.Pool) *InvitationStore { } // Create stores a new invitation. -func (s *InvitationStore) Create(ctx context.Context, teamID, email, role, tokenHash, invitedBy string, expiresAt time.Time) (*model.Invitation, error) { +func (s *InvitationStore) Create(ctx context.Context, teamID, email, role, tokenHash string, invitedBy *string, expiresAt time.Time) (*model.Invitation, error) { var inv model.Invitation err := s.pool.QueryRow(ctx, `INSERT INTO invitations (team_id, email, role, token_hash, invited_by, expires_at) diff --git a/internal/store/triage.go b/internal/store/triage.go index 90f46028..45b54702 100644 --- a/internal/store/triage.go +++ b/internal/store/triage.go @@ -264,3 +264,69 @@ func (s *TriageStore) ListClassifications(ctx context.Context, teamID, triageID } return classifications, rows.Err() } + +// ClusterInput is the data needed to persist a single triage cluster. +type ClusterInput struct { + RootCause string + Label *string +} + +// ClassificationInput is the data needed to persist a single failure classification. +type ClassificationInput struct { + ClusterIndex int + TestResultID string + Classification string +} + +// OutputData holds all clusters and classifications to persist atomically. +type OutputData struct { + Clusters []ClusterInput + Classifications []ClassificationInput +} + +// PersistOutput atomically writes all clusters and classifications for a triage +// result in a single database transaction. If any insert fails, all inserts are +// rolled back so the triage record is not left in an inconsistent state. +func (s *TriageStore) PersistOutput(ctx context.Context, teamID, triageID string, output *OutputData) error { + tx, err := s.pool.Begin(ctx) + if err != nil { + return fmt.Errorf("persist triage output: begin tx: %w", err) + } + defer tx.Rollback(ctx) //nolint:errcheck // rollback on early return is intentional + + // Insert clusters and collect their assigned UUIDs for linking classifications. + clusterIDs := make([]string, len(output.Clusters)) + for i, cluster := range output.Clusters { + err := tx.QueryRow(ctx, + `INSERT INTO triage_clusters (triage_id, team_id, root_cause, label) + VALUES ($1, $2, $3, $4) + RETURNING id`, + triageID, teamID, cluster.RootCause, cluster.Label, + ).Scan(&clusterIDs[i]) + if err != nil { + return fmt.Errorf("persist cluster[%d]: %w", i, err) + } + } + + // Insert per-failure classifications linked to their cluster. + for _, cl := range output.Classifications { + var clusterID *string + if cl.ClusterIndex >= 0 && cl.ClusterIndex < len(clusterIDs) { + cid := clusterIDs[cl.ClusterIndex] + clusterID = &cid + } + _, err := tx.Exec(ctx, + `INSERT INTO triage_failure_classifications (triage_id, cluster_id, test_result_id, team_id, classification) + VALUES ($1, $2, $3, $4, $5)`, + triageID, clusterID, cl.TestResultID, teamID, cl.Classification, + ) + if err != nil { + return fmt.Errorf("persist classification for %s: %w", cl.TestResultID, err) + } + } + + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("persist triage output: commit: %w", err) + } + return nil +} diff --git a/internal/telegram/telegram.go b/internal/telegram/telegram.go index a0cfba28..97e045e4 100644 --- a/internal/telegram/telegram.go +++ b/internal/telegram/telegram.go @@ -11,9 +11,12 @@ import ( "net/http" "strings" "time" + + "github.com/rs/zerolog/log" ) const defaultBaseURL = "https://api.telegram.org" +const defaultTelegramRetries = 3 // Client sends messages to a Telegram chat via the Bot API. type Client struct { @@ -21,6 +24,7 @@ type Client struct { chatID string httpClient *http.Client baseURL string + maxRetries int } // ClientOption configures a Client. @@ -31,6 +35,11 @@ func WithBaseURL(url string) ClientOption { return func(c *Client) { c.baseURL = url } } +// WithMaxRetries sets the number of retry attempts for transient errors. +func WithMaxRetries(n int) ClientOption { + return func(c *Client) { c.maxRetries = n } +} + // NewClient returns a Client configured with the given bot token and chat ID. func NewClient(token, chatID string, opts ...ClientOption) *Client { c := &Client{ @@ -38,6 +47,7 @@ func NewClient(token, chatID string, opts ...ClientOption) *Client { chatID: chatID, httpClient: &http.Client{Timeout: 15 * time.Second}, baseURL: defaultBaseURL, + maxRetries: defaultTelegramRetries, } for _, o := range opts { o(c) @@ -54,9 +64,20 @@ type sendMessageRequest struct { type apiResponse struct { OK bool `json:"ok"` Description string `json:"description,omitempty"` + ErrorCode int `json:"error_code,omitempty"` + Parameters struct { + RetryAfter int `json:"retry_after,omitempty"` + } `json:"parameters,omitempty"` +} + +// isRetriableTelegramError returns true for HTTP 429 and 5xx status codes. +func isRetriableTelegramError(statusCode int) bool { + return statusCode == http.StatusTooManyRequests || statusCode >= 500 } // SendMessage posts text to the configured Telegram chat using HTML parse mode. +// It retries on 429 (rate limited) and 5xx (server error) responses with +// exponential backoff, respecting the Retry-After header on 429 responses. func (c *Client) SendMessage(ctx context.Context, text string) error { endpoint := fmt.Sprintf("%s/bot%s/sendMessage", c.baseURL, c.token) payload := sendMessageRequest{ChatID: c.chatID, Text: text, ParseMode: "HTML"} @@ -64,6 +85,56 @@ func (c *Client) SendMessage(ctx context.Context, text string) error { if err != nil { return fmt.Errorf("telegram: marshal request: %w", err) } + + var lastErr error + for attempt := 0; attempt <= c.maxRetries; attempt++ { + if ctx.Err() != nil { + return ctx.Err() + } + + lastErr = c.doSend(ctx, endpoint, data) + if lastErr == nil { + return nil + } + + re, ok := lastErr.(*telegramError) + if !ok || !isRetriableTelegramError(re.statusCode) { + return lastErr + } + + if attempt < c.maxRetries { + backoff := time.Duration(1< 0 { + backoff = time.Duration(re.retryAfter) * time.Second + } + log.Warn().Err(lastErr). + Int("attempt", attempt+1). + Dur("backoff", backoff). + Msg("telegram: retrying SendMessage") + + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(backoff): + } + } + } + return lastErr +} + +// telegramError represents a Telegram API error with status code and optional retry_after. +type telegramError struct { + statusCode int + desc string + retryAfter int +} + +func (e *telegramError) Error() string { + return fmt.Sprintf("telegram: API error (status %d): %s", e.statusCode, e.desc) +} + +// doSend executes a single HTTP POST to the Telegram sendMessage API. +func (c *Client) doSend(ctx context.Context, endpoint string, data []byte) error { req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewReader(data)) if err != nil { return fmt.Errorf("telegram: build request: %w", err) @@ -80,6 +151,22 @@ func (c *Client) SendMessage(ctx context.Context, text string) error { if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { return fmt.Errorf("telegram: decode response: %w", err) } + + if resp.StatusCode == http.StatusTooManyRequests { + return &telegramError{ + statusCode: resp.StatusCode, + desc: result.Description, + retryAfter: result.Parameters.RetryAfter, + } + } + + if resp.StatusCode >= 500 { + return &telegramError{ + statusCode: resp.StatusCode, + desc: result.Description, + } + } + if !result.OK { return fmt.Errorf("telegram: API error: %s", result.Description) } @@ -126,6 +213,8 @@ func FormatMessage(s CISummary) string { branch := html.EscapeString(s.Branch) commitLine = html.EscapeString(commitLine) + escapedRunURL := html.EscapeString(s.RunURL) + var sb strings.Builder sb.WriteString(fmt.Sprintf("%s %s — %s\n", icon, repo, strings.ToUpper(s.Status))) sb.WriteString(fmt.Sprintf("Branch: %s", branch)) @@ -148,7 +237,7 @@ func FormatMessage(s CISummary) string { sb.WriteString(fmt.Sprintf(" / %d total\n", s.Total)) if s.RunURL != "" { - sb.WriteString(fmt.Sprintf("\nView run", s.RunURL)) + sb.WriteString(fmt.Sprintf("\nView run", escapedRunURL)) } return sb.String() } diff --git a/internal/telegram/telegram_test.go b/internal/telegram/telegram_test.go index 693cd1fe..85f0022b 100644 --- a/internal/telegram/telegram_test.go +++ b/internal/telegram/telegram_test.go @@ -139,6 +139,47 @@ func TestFormatMessage_HTMLEscapesExternalFields(t *testing.T) { } } +func TestFormatMessage_HTMLEscapesRunURL(t *testing.T) { + s := telegram.CISummary{ + Repo: "org/repo", + Branch: "main", + CommitMsg: "test", + Status: "failing", + Failed: 1, + Total: 1, + RunURL: `https://example.com/run"onmouseover="alert(1)`, + } + msg := telegram.FormatMessage(s) + + if strings.Contains(msg, `"onmouseover`) { + t.Errorf("RunURL double-quote must be escaped to prevent XSS; got:\n%s", msg) + } + if !strings.Contains(msg, "https://example.com/run"onmouseover="alert(1)") && + !strings.Contains(msg, "https://example.com/run"onmouseover="alert(1)") { + t.Errorf("RunURL should be HTML-escaped in href; got:\n%s", msg) + } +} + +func TestFormatMessage_RunURLWithAngleBrackets(t *testing.T) { + s := telegram.CISummary{ + Repo: "org/repo", + Branch: "main", + CommitMsg: "test", + Status: "passing", + Passed: 1, + Total: 1, + RunURL: "https://example.com/", + } + msg := telegram.FormatMessage(s) + + if strings.Contains(msg, "