carlos / platform Public

Report the SNS sink the agent really built; tighten ARN and CLI checks

Review of 0f73987f.

- The status probe derived "sns" from the env, so a box whose SNS client
  could not be built (missing region or profile) showed alerts=sns with zero
  failures while delivering nothing. buildAlertSinks now records the
  initialised state and counts the failed build, and the snapshot reports
  sns_unavailable. `carlos status --live` prints alerts=sns-unavailable.
  buildAlertSinks takes the client factory as a parameter and is tested.
- CARLOS_ALERT_SNS_TOPIC_ARN must be exactly a standard topic ARN: a
  subscription ARN (topic ARN plus :<id>), a FIFO name or any extra colon
  is refused, with negative cases.
- README no longer says sends are "not retried": the SDK retries a
  retryable failure up to three times, and a failed bucket write repeats
  the transition on the next tick. The code comment says the same.
- The statuslive test now requires every expected label to appear exactly
  once before checking its column.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude agent, operated by paul@keymail.dev 98a8a9e6c351b82da7fd62b6d135bc377b88de65 parent 0f73987
8 files changed, +228 −36
  • README.md +7 −2
  • cli.go +10 −1
  • docs/site/cli.md +3 −1
  • internal/host/alertsink.go +66 −17
  • internal/host/alertsink_test.go +108 −0
  • internal/host/metrics_loop.go +1 −1
  • internal/status/snapshot.go +7 −2
  • statuslive_test.go +26 −12

CI passed — run details

diff --git a/README.md b/README.md
index 83084c17c..bee763f04 100644
--- a/README.md
+++ b/README.md
@@ -695,8 +695,13 @@ for the full design.
region is read from the ARN, not from `AWS_REGION`. Set it to the box's
own region's alerts topic and anyone subscribed to that topic gets the
email. Either sink works alone and both may be set. A failed send is
- logged and counted, never fatal, and is not retried. A value that is not
- an SNS topic ARN is logged and ignored.
+ logged and counted, never fatal. The SDK tries a retryable failure up to
+ three times within ten seconds. After that, a later tick does not send
+ that alert again, unless the same tick's bucket write also failed, which
+ repeats the whole transition on the next tick. A value that is not an SNS
+ topic ARN (a subscription ARN, for example) is logged and the sink stays
+ off. So does a client that cannot be built, which `carlos status --live`
+ shows as `alerts=sns-unavailable`.
- With neither sink set, the box says so: the agent logs `NO SINK
CONFIGURED` at start, and the box's private status snapshot carries
`metrics.alerts` with both sinks false, which `carlos status --live` prints
diff --git a/cli.go b/cli.go
index b2f08b6c3..f96163eed 100644
--- a/cli.go
+++ b/cli.go
@@ -3451,10 +3451,19 @@ func alertsWord(a *status.AlertDelivery) string {
if a.SNS {
sinks = append(sinks, "sns")
}
- if len(sinks) == 0 {
+ if len(sinks) == 0 && !a.SNSUnavailable {
return "not-delivered"
}
out := strings.Join(sinks, "+")
+ if a.SNSUnavailable {
+ // A topic is configured but its client could not be built. Alone it
+ // is the whole answer; next to a working sink it is a warning.
+ if out == "" {
+ out = "sns-unavailable"
+ } else {
+ out += " (sns-unavailable)"
+ }
+ }
if a.WebhookFailures > 0 {
out += fmt.Sprintf(" (webhook failed %d)", a.WebhookFailures)
}
diff --git a/docs/site/cli.md b/docs/site/cli.md
index 07cce8eec..c08054481 100644
--- a/docs/site/cli.md
+++ b/docs/site/cli.md
@@ -1567,7 +1567,9 @@ with how long it has been dark, and none of its stale readings.
The last column reads `alerts=sns`, `alerts=webhook` or `alerts=webhook+sns`
when a box can deliver its alerts, and `alerts=not-delivered` when it has no
-sink configured. A dash means the box runs an older binary and did not say.
+sink configured. `alerts=sns-unavailable` means a topic is set but the box
+could not build its SNS client, so nothing is delivered through it. A dash
+means the box runs an older binary and did not say.
```sh
carlos status --live
diff --git a/internal/host/alertsink.go b/internal/host/alertsink.go
index 6428d1a6b..3fe144e02 100644
--- a/internal/host/alertsink.go
+++ b/internal/host/alertsink.go
@@ -7,6 +7,7 @@ import (
"fmt"
"log"
"math"
+ "regexp"
"strings"
"sync/atomic"
"time"
@@ -36,9 +37,12 @@ import (
//
// Delivery is best effort and never fatal. A sink that fails is logged and
// counted, the other sink still runs, and the tick carries on. Failed sends
-// are not retried: Evaluate emits an event only on a transition, so the
-// next tick has no event to resend. The count is what makes that visible,
-// through status.AlertDelivery in the box snapshot.
+// are not rescheduled by a later tick: Evaluate emits an event only on a
+// transition, so the next tick has no event to resend. (The SDK itself tries a
+// retryable failure up to three times inside one publish, and a failed bucket
+// write in alertTick repeats the whole transition next tick, delivery
+// included.) The count is what makes a lost send visible, through
+// status.AlertDelivery in the box snapshot.
// snsPublisher is the one SNS call this file makes.
type snsPublisher interface {
@@ -49,12 +53,23 @@ type snsPublisher interface {
// cannot hold the 60 second sampler tick for the SDK's own default retries.
const alertPublishTimeout = 10 * time.Second
-// alertCounters counts failed sends since the process started.
+// alertCounters counts failed sends since the process started, and holds the
+// one fact the status loop cannot derive from the env: whether the SNS client
+// was actually built. They are separate goroutines, so this is their seam.
type alertCounters struct {
webhook atomic.Int64
sns atomic.Int64
+ // snsInit is snsInitUnknown until buildAlertSinks has run for a valid
+ // topic ARN, then snsInitOK or snsInitFailed.
+ snsInit atomic.Int32
}
+const (
+ snsInitUnknown int32 = iota
+ snsInitOK
+ snsInitFailed
+)
+
// processAlertCounters is the process-wide count the metrics loop writes
// and the status loop reads. They are separate goroutines with no other
// shared state, so this is the seam between them.
@@ -110,6 +125,14 @@ func (a *alertSinks) deliver(ctx context.Context, rules metrics.Rules, at time.T
}
}
+// topicARNPattern is the whole shape of a standard SNS topic ARN: partition,
+// region, a 12 digit account, and a topic name that is only letters, digits,
+// hyphen and underscore. The name has no colon, so a subscription ARN
+// (topic ARN plus ":<subscription-id>") does not match, and no dot, so FIFO
+// topics (which need a message group id this sink does not send) do not
+// either.
+var topicARNPattern = regexp.MustCompile(`^arn:aws(-cn|-us-gov)?:sns:([a-z0-9-]+):[0-9]{12}:[A-Za-z0-9_-]{1,256}$`)
+
// parseAlertTopicARN checks CARLOS_ALERT_SNS_TOPIC_ARN and returns the
// region the topic lives in. SNS is regional and a topic is reached only
// through its own region's endpoint, so the region is read from the ARN and
@@ -128,15 +151,18 @@ func parseAlertTopicARN(s string) (region string, err error) {
if a.Service != "sns" {
return "", fmt.Errorf("service %q, want sns", a.Service)
}
- if a.Region == "" || a.AccountID == "" || a.Resource == "" {
- return "", errors.New("arn lacks region, account or topic name")
+ if !topicARNPattern.MatchString(s) {
+ return "", errors.New("not a topic arn (want arn:aws:sns:<region>:<12 digit account>:<topic name>)")
}
return a.Region, nil
}
// alertDeliveryProbe is the box's answer to "do my alert rules have anywhere
-// to go", read from the same env buildAlertSinks reads, so the snapshot and
-// the sinks cannot disagree. A topic ARN that does not parse is not a sink.
+// to go". The webhook and the ARN's shape come from the same env
+// buildAlertSinks reads; whether the SNS client was actually built comes from
+// counts, which buildAlertSinks fills in. So the snapshot reports the sink the
+// agent really holds: a topic ARN that does not parse is not a sink, and one
+// whose client failed to build reads SNS=false, SNSUnavailable=true.
func alertDeliveryProbe(getenv func(string) string, counts *alertCounters) *status.AlertDelivery {
if counts == nil {
counts = &processAlertCounters
@@ -147,16 +173,36 @@ func alertDeliveryProbe(getenv func(string) string, counts *alertCounters) *stat
SNSFailures: counts.sns.Load(),
}
if _, err := parseAlertTopicARN(getenv("CARLOS_ALERT_SNS_TOPIC_ARN")); err == nil {
- out.SNS = true
+ if counts.snsInit.Load() == snsInitFailed {
+ out.SNSUnavailable = true
+ } else {
+ out.SNS = true
+ }
}
return out
}
+// snsClientFactory builds the publisher for a region. It is a parameter so
+// tests can make client construction fail without an AWS config.
+type snsClientFactory func(ctx context.Context, region string) (snsPublisher, error)
+
+// newSNSClient is the production factory: the default credential chain (the
+// box's instance role), aimed at the topic's own region.
+func newSNSClient(ctx context.Context, region string) (snsPublisher, error) {
+ cfg, err := awsconfig.LoadDefaultConfig(ctx, awsconfig.WithRegion(region))
+ if err != nil {
+ return nil, err
+ }
+ return sns.NewFromConfig(cfg), nil
+}
+
// buildAlertSinks reads the sink env and builds the sinks. A bad topic ARN
-// is logged once and leaves SNS off; it never stops the metrics loop. With
-// nothing configured it says so in the journal, because that is the state
-// that used to be silent.
-func buildAlertSinks(ctx context.Context, getenv func(string) string) *alertSinks {
+// is logged once and leaves SNS off; a client that cannot be built does the
+// same, and is also recorded in counts (state and one counted failure) so the
+// status snapshot reports the sink the agent really has. Neither stops the
+// metrics loop. With nothing configured it says so in the journal, because
+// that is the state that used to be silent.
+func buildAlertSinks(ctx context.Context, getenv func(string) string, counts *alertCounters, newClient snsClientFactory) *alertSinks {
webhook := strings.TrimSpace(getenv("CARLOS_ALERT_WEBHOOK"))
topic := strings.TrimSpace(getenv("CARLOS_ALERT_SNS_TOPIC_ARN"))
@@ -166,11 +212,14 @@ func buildAlertSinks(ctx context.Context, getenv func(string) string) *alertSink
if err != nil {
log.Printf("agent: alerts: CARLOS_ALERT_SNS_TOPIC_ARN is not usable, SNS sink off: %v", err)
topic = ""
- } else if cfg, err := awsconfig.LoadDefaultConfig(ctx, awsconfig.WithRegion(region)); err != nil {
- log.Printf("agent: alerts: load aws config, SNS sink off: %v", err)
+ } else if p, err := newClient(ctx, region); err != nil {
+ log.Printf("agent: alerts: build SNS client, SNS sink off: %v", err)
+ counts.sns.Add(1)
+ counts.snsInit.Store(snsInitFailed)
topic = ""
} else {
- pub = sns.NewFromConfig(cfg)
+ counts.snsInit.Store(snsInitOK)
+ pub = p
}
}
@@ -184,7 +233,7 @@ func buildAlertSinks(ctx context.Context, getenv func(string) string) *alertSink
default:
log.Printf("agent: alerts: delivering to webhook")
}
- return newAlertSinks(webhook, httpPostWebhook, topic, pub, &processAlertCounters)
+ return newAlertSinks(webhook, httpPostWebhook, topic, pub, counts)
}
// formatAlert renders one transition as an email: a one-line subject and a
diff --git a/internal/host/alertsink_test.go b/internal/host/alertsink_test.go
index d55fc49ac..412bd840d 100644
--- a/internal/host/alertsink_test.go
+++ b/internal/host/alertsink_test.go
@@ -283,6 +283,17 @@ func TestParseAlertTopicARN(t *testing.T) {
{"arn:aws:sqs:eu-west-1:271376211898:queue", "", true},
{"arn:aws:sns:::topic", "", true},
{"arn:aws:sns:eu-west-1:271376211898:", "", true},
+ // A subscription ARN is topic ARN plus ":<subscription-id>". It is
+ // an sns ARN with all the right fields, and publishing to it fails.
+ {testTopicARN + ":8f4c0e7a-1b2c-4d3e-9f50-0123456789ab", "", true},
+ {"arn:aws:sns:eu-west-1:271376211898:topic:subscription-id", "", true},
+ {"arn:aws:sns:eu-west-1:271376211898:a:b", "", true},
+ {"arn:aws:sns:eu-west-1:271376211898:name.fifo", "", true},
+ {"arn:aws:sns:eu-west-1:271376211898:bad name", "", true},
+ {"arn:aws:sns:eu-west-1:27137621189:carlos-alerts", "", true},
+ {"arn:aws:sns:eu-west-1:abcdefghijkl:carlos-alerts", "", true},
+ {"arn:aws:sns:eu-west-1:271376211898:carlos-alerts\nx", "", true},
+ {"arn:aws:sns:eu-west-1:271376211898:*", "", true},
}
for _, c := range cases {
region, err := parseAlertTopicARN(c.in)
@@ -326,6 +337,103 @@ func TestAlertDeliveryProbe(t *testing.T) {
}
}
+// fakeClientFactory is the buildAlertSinks seam: it records the region it
+// was asked for and either hands back a fake or fails.
+type fakeClientFactory struct {
+ regions []string
+ err error
+ pub *fakeSNS
+}
+
+func (f *fakeClientFactory) build(_ context.Context, region string) (snsPublisher, error) {
+ f.regions = append(f.regions, region)
+ if f.err != nil {
+ return nil, f.err
+ }
+ return f.pub, nil
+}
+
+func TestBuildAlertSinksInitialisedSNS(t *testing.T) {
+ env := func(m map[string]string) func(string) string { return func(k string) string { return m[k] } }
+ counts := &alertCounters{}
+ f := &fakeClientFactory{pub: &fakeSNS{}}
+ sinks := buildAlertSinks(context.Background(), env(map[string]string{"CARLOS_ALERT_SNS_TOPIC_ARN": testTopicARN}), counts, f.build)
+ if len(f.regions) != 1 || f.regions[0] != "eu-west-1" {
+ t.Fatalf("client built for %v, want the ARN's region eu-west-1", f.regions)
+ }
+ sinks.deliver(context.Background(), metrics.DefaultRules(), sinkTestAt,
+ metrics.Event{Rule: "disk", Firing: true, Value: 0.95, Box: "box-a"})
+ if len(f.pub.in) != 1 {
+ t.Fatalf("publishes = %d, want 1", len(f.pub.in))
+ }
+ got := alertDeliveryProbe(env(map[string]string{"CARLOS_ALERT_SNS_TOPIC_ARN": testTopicARN}), counts)
+ if !got.SNS || got.SNSUnavailable || got.SNSFailures != 0 {
+ t.Errorf("a working sink must read as sns: %+v", got)
+ }
+}
+
+// TestBuildAlertSinksClientFailureIsVisible is the review's finding: with a
+// valid ARN in the env but a client that cannot be built (no region, bad
+// profile), the sink is off. The status probe must say so, not report the
+// env as if it were a working sink with zero failures.
+func TestBuildAlertSinksClientFailureIsVisible(t *testing.T) {
+ envMap := map[string]string{"CARLOS_ALERT_SNS_TOPIC_ARN": testTopicARN}
+ env := func(k string) string { return envMap[k] }
+ counts := &alertCounters{}
+ f := &fakeClientFactory{err: errors.New("failed to get shared config profile")}
+ sinks := buildAlertSinks(context.Background(), env, counts, f.build)
+ // Delivering must neither panic nor publish.
+ sinks.deliver(context.Background(), metrics.DefaultRules(), sinkTestAt,
+ metrics.Event{Rule: "disk", Firing: true, Value: 0.95, Box: "box-a"})
+
+ got := alertDeliveryProbe(env, counts)
+ if got.SNS || !got.SNSUnavailable || got.Delivered() {
+ t.Errorf("failed init must read as unavailable and not delivered: %+v", got)
+ }
+ if got.SNSFailures < 1 {
+ t.Errorf("failed init must be counted, got %d", got.SNSFailures)
+ }
+
+ // With a webhook as well, the box still delivers, and says what is wrong.
+ envMap["CARLOS_ALERT_WEBHOOK"] = "http://example.test/hook"
+ got = alertDeliveryProbe(env, counts)
+ if !got.Webhook || got.SNS || !got.SNSUnavailable || !got.Delivered() {
+ t.Errorf("webhook alone must still deliver: %+v", got)
+ }
+}
+
+func TestBuildAlertSinksBadARNIsNotASinkAndBuildsNoClient(t *testing.T) {
+ counts := &alertCounters{}
+ f := &fakeClientFactory{pub: &fakeSNS{}}
+ env := func(k string) string {
+ if k == "CARLOS_ALERT_SNS_TOPIC_ARN" {
+ return testTopicARN + ":subscription-id"
+ }
+ return ""
+ }
+ buildAlertSinks(context.Background(), env, counts, f.build)
+ if len(f.regions) != 0 {
+ t.Errorf("a subscription ARN must not build a client: %v", f.regions)
+ }
+ got := alertDeliveryProbe(env, counts)
+ if got.SNS || got.Delivered() {
+ t.Errorf("subscription ARN counted as a sink: %+v", got)
+ }
+}
+
+func TestBuildAlertSinksNothingConfigured(t *testing.T) {
+ counts := &alertCounters{}
+ f := &fakeClientFactory{pub: &fakeSNS{}}
+ buildAlertSinks(context.Background(), func(string) string { return "" }, counts, f.build)
+ if len(f.regions) != 0 {
+ t.Errorf("no ARN set, yet a client was built: %v", f.regions)
+ }
+ got := alertDeliveryProbe(func(string) string { return "" }, counts)
+ if got.Delivered() || got.SNSUnavailable {
+ t.Errorf("nothing configured: %+v", got)
+ }
+}
+
func deref(s *string) string {
if s == nil {
return ""
diff --git a/internal/host/metrics_loop.go b/internal/host/metrics_loop.go
index 9b0b820a7..cbd2cc382 100644
--- a/internal/host/metrics_loop.go
+++ b/internal/host/metrics_loop.go
@@ -801,7 +801,7 @@ func metricsLoop(execRoot string, stampedUnits func() []string) {
st := loadAlertState(statePath, box)
rules := metrics.RulesFromEnv(os.Getenv)
logRejectedAlertOverrides(os.Getenv)
- sinks := buildAlertSinks(ctx, os.Getenv)
+ sinks := buildAlertSinks(ctx, os.Getenv, &processAlertCounters, newSNSClient)
log.Printf("agent: metrics loop armed (60s ticks, 5min flush), publishing to %s", target)
diff --git a/internal/status/snapshot.go b/internal/status/snapshot.go
index da7018071..398c399d9 100644
--- a/internal/status/snapshot.go
+++ b/internal/status/snapshot.go
@@ -286,12 +286,17 @@ type AliasReading struct {
// silent, and the flagship's disk, memory and cpu alerts went nowhere for
// weeks (discussion #192). Webhook is CARLOS_ALERT_WEBHOOK set. SNS is
// CARLOS_ALERT_SNS_TOPIC_ARN set to a topic ARN the agent can use; a
-// malformed value reads false, because it is not a sink. Failures count
-// sends since the agent process started, so a role without sns:Publish
+// malformed value reads false, because it is not a sink. SNS is the state
+// of the sink the agent actually built, not of the env: when a topic ARN is
+// set but the agent could not build its SNS client (no region, a missing
+// profile), SNS is false and SNSUnavailable is true, so the box never reads
+// as having a sink it cannot use. Failures count sends (and that failed
+// build) since the agent process started, so a role without sns:Publish
// shows up here and not only in the journal.
type AlertDelivery struct {
Webhook bool `json:"webhook"`
SNS bool `json:"sns"`
+ SNSUnavailable bool `json:"sns_unavailable,omitempty"`
WebhookFailures int64 `json:"webhook_failures,omitempty"`
SNSFailures int64 `json:"sns_failures,omitempty"`
}
diff --git a/statuslive_test.go b/statuslive_test.go
index 3d62b5c82..93677c05e 100644
--- a/statuslive_test.go
+++ b/statuslive_test.go
@@ -102,7 +102,8 @@ func TestPrintLiveBoxesSortsByLabel(t *testing.T) {
// TestPrintLiveBoxesSaysWhetherAlertsAreDelivered is discussion #192's
// visibility ask: a box with no alert sink must read as not delivered in the
// operator's own listing, not as silence. A box that predates the field
-// prints a dash, since it did not say.
+// prints a dash, since it did not say. A sink whose client could not be
+// built reads as unavailable, never as a working sink.
func TestPrintLiveBoxesSaysWhetherAlertsAreDelivered(t *testing.T) {
now := time.Now()
var buf bytes.Buffer
@@ -111,22 +112,35 @@ func TestPrintLiveBoxesSaysWhetherAlertsAreDelivered(t *testing.T) {
{GeneratedAt: now, Label: "b-sns", Metrics: &status.BoxMetrics{Alerts: &status.AlertDelivery{SNS: true}}},
{GeneratedAt: now, Label: "c-both", Metrics: &status.BoxMetrics{Alerts: &status.AlertDelivery{Webhook: true, SNS: true, SNSFailures: 3}}},
{GeneratedAt: now, Label: "d-old", Metrics: &status.BoxMetrics{Tunnel: "connected"}},
+ {GeneratedAt: now, Label: "e-unavail", Metrics: &status.BoxMetrics{Alerts: &status.AlertDelivery{SNSUnavailable: true, SNSFailures: 1}}},
+ {GeneratedAt: now, Label: "f-hook-unavail", Metrics: &status.BoxMetrics{Alerts: &status.AlertDelivery{Webhook: true, SNSUnavailable: true}}},
}, now)
want := map[string]string{
- "a-none": "alerts=not-delivered",
- "b-sns": "alerts=sns",
- "c-both": "alerts=webhook+sns (sns failed 3)",
- "d-old": "alerts=-",
+ "a-none": "alerts=not-delivered",
+ "b-sns": "alerts=sns\n",
+ "c-both": "alerts=webhook+sns (sns failed 3)",
+ "d-old": "alerts=-",
+ "e-unavail": "alerts=sns-unavailable (sns failed 1)",
+ "f-hook-unavail": "alerts=webhook (sns-unavailable)",
}
- for _, line := range strings.Split(strings.TrimRight(buf.String(), "\n"), "\n") {
- for label, frag := range want {
- if strings.HasPrefix(line, label) && !strings.Contains(line, frag) {
- t.Errorf("%s: want %q in %q", label, frag, line)
+ lines := strings.Split(strings.TrimRight(buf.String(), "\n"), "\n")
+ if len(lines) != len(want) {
+ t.Fatalf("got %d rows, want %d:\n%s", len(lines), len(want), buf.String())
+ }
+ for label, frag := range want {
+ var rows []string
+ for _, line := range lines {
+ if strings.HasPrefix(line, label+" ") {
+ rows = append(rows, line)
}
}
- }
- if strings.Contains(buf.String(), "b-sns") && strings.Contains(strings.Split(buf.String(), "\n")[1], "not-delivered") {
- t.Errorf("a box with a sink must not read as not delivered:\n%s", buf.String())
+ if len(rows) != 1 {
+ t.Errorf("%s: appears on %d rows, want exactly 1:\n%s", label, len(rows), buf.String())
+ continue
+ }
+ if !strings.Contains(rows[0]+"\n", frag) {
+ t.Errorf("%s: want %q in %q", label, frag, rows[0])
+ }
}
}