From b35a1903a8e9a1bc4a3493a6540336cb2d1062a4 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 13 Sep 2026 03:04:42 -0400 Subject: [PATCH] feat(sentry): remove the startup probe Boot no longer sends a canary event or waits on a flush window. The probe's flush proved queue drain, not delivery: a refused ingest host drained in milliseconds and reported success while sentryDialCheck already named the host as unreachable. Every container start also cost Sentry one event, and the probe's stall test was the flake in #179. SetupSentry keeps its signature and return semantics. It still runs sentryDialCheck and now prints the enabled line whenever init succeeded. The probe-only test helpers and the reset() call that hid the boot canary from other tests go with it. The dial-check doc comment stands alone, and the boot-delay comments drop from ~8s to 3s. Closes #259 *This was generated by AI* --- rest/sentry.go | 29 +++---- rest/sentry_boot.go | 68 +++-------------- rest/sentry_boot_test.go | 158 +++++++-------------------------------- rest/sentry_test.go | 10 --- servers/server.go | 13 ++-- 5 files changed, 53 insertions(+), 225 deletions(-) diff --git a/rest/sentry.go b/rest/sentry.go index 8bd6b9e..7aa8040 100644 --- a/rest/sentry.go +++ b/rest/sentry.go @@ -19,15 +19,12 @@ package rest // point calls (the error-writer behind writeError; methodNotAllowed // reaches it without writeError): one place, every error. // -// What this file deliberately does NOT reproduce — servers/sentry.go's three -// boot-lifecycle pieces, which the cutover slice (#134) must port before -// deleting that file: sentryDialCheck (warn-only TCP reachability check of -// the DSN ingest host at boot), sentryStartupProbe (one canary event flushed -// through the real transport before any listener opens), and -// flushSentryOnShutdown (the SIGTERM/interrupt flush handler servers.Start -// installs after setupSentry). Dropping them silently would make a -// Sentry-down misconfig invisible at boot and lose every still-buffered -// event on SIGTERM. +// The boot/shutdown lifecycle lives in sentry_boot.go: sentryDialCheck (the +// warn-only TCP reachability check of the DSN ingest host that SetupSentry +// runs at boot) and FlushSentryOnShutdown (the SIGTERM/interrupt flush handler +// servers.Start installs when SetupSentry returns true). Dropping either +// silently would make a Sentry-down misconfig invisible at boot, or lose every +// still-buffered event on SIGTERM. // // Tag discipline (PRD #112): events carry the validated key ID and the // matched route pattern — bearer material NEVER (same rule as the metrics @@ -102,18 +99,12 @@ func SetupSentry(release string) bool { return false } - // Warn-only reachability pre-check: catches the misconfig class the probe - // below structurally cannot (fast send failures drain the queue and so - // still "flush"). Never changes the enabled/degraded semantics. + // Warn-only reachability pre-check. It never changes the enabled state. + // A refused or unresolvable host warns, and capture stays on because the + // network may heal before the first real error. sentryDialCheck(dsn) - // Probe failure still returns true — enabled-degraded, not disabled: a - // slow-network false positive must not turn off capture. Do NOT refactor - // this into `return sentryStartupProbe()`. - if sentryStartupProbe() { - Info.Println("Sentry error capture enabled (errors only), release:", release, - "— startup probe flushed (queue drained; delivery not verified — set SENTRY_DEBUG=true to confirm)") - } + Info.Println("Sentry error capture enabled (errors only), release:", release) return true } diff --git a/rest/sentry_boot.go b/rest/sentry_boot.go index 43ad548..0545eb5 100644 --- a/rest/sentry_boot.go +++ b/rest/sentry_boot.go @@ -1,9 +1,9 @@ package rest -// Sentry boot-lifecycle wiring for the new stack: the three pieces ported from -// Phase 0's servers/sentry.go at the single-listener cutover (#134). SetupSentry -// (sentry.go) calls sentryDialCheck and sentryStartupProbe at boot; the -// composition root installs FlushSentryOnShutdown when capture is enabled. +// Sentry boot-lifecycle wiring for the new stack: the two pieces that survive +// from Phase 0's servers/sentry.go after the single-listener cutover (#134) and +// #259. SetupSentry (sentry.go) calls sentryDialCheck at boot; the composition +// root installs FlushSentryOnShutdown when capture is enabled. // // Kept in their own file so the always-on init (sentry.go) and the boot/shutdown // lifecycle stay legible apart; they share the package's sentryTransport test @@ -24,25 +24,20 @@ import ( // events to reach Sentry before the process exits. const sentryShutdownFlushTimeout = 2 * time.Second -// sentryStartupProbeTimeout bounds the boot-time drain check. A var, not a -// const, only so tests can shrink the window; production never mutates it. -var sentryStartupProbeTimeout = 5 * time.Second - // sentryDialCheckTimeout bounds the boot-time TCP reachability pre-check of -// the DSN ingest host — generous enough for a cold DNS resolve plus a +// the DSN ingest host. Generous enough for a cold DNS resolve plus a // cross-region handshake, small enough to keep the worst-case boot delay -// acceptable (it stacks with the probe window before any listener opens). A +// acceptable. This is the only network wait before any listener opens. A // var, not a const, only so tests can shrink the window; production never // mutates it. var sentryDialCheckTimeout = 3 * time.Second // sentryDialCheck is a boot-time, warn-only TCP reachability check of the DSN -// ingest host. It exists because the startup probe cannot see fast send -// failures — DNS errors and refused connections drain the transport queue in -// milliseconds, so the probe's flush still reports success (see -// sentryStartupProbe). A failed dial here names the unreachable host while -// the probe would stay silent. Warn-only by design: the network may heal, and -// a boot-time blip must not disable capture. +// ingest host. sentry.Init does no network I/O, so without this check a wrong +// host, a DNS typo or a closed egress path stays invisible until the first +// real error fails to send. A failed dial names the unreachable host in the +// log before any listener opens. Warn-only by design: the network may heal, +// and a boot-time blip must not disable capture. func sentryDialCheck(rawDSN string) { dsn, err := sentry.NewDsn(rawDSN) if err != nil { @@ -58,47 +53,6 @@ func sentryDialCheck(rawDSN string) { _ = conn.Close() } -// sentryStartupProbe pushes one canary event through the real transport and -// flushes. sentry.Init does no network I/O, so without this the pipeline is -// first exercised by the first real error. -// -// What a true return PROVES — per the SDK's documented Flush contract (queue -// drained, NOT delivered): the transport finished its send attempts within -// the window. That catches hang-class failures (blackholed egress, connects -// slower than the window) and guarantees one event exercised the full -// pipeline so SENTRY_DEBUG has something to report. What it does NOT prove: -// delivery. The transport worker dequeues on ANY send outcome, so DNS -// failures, refused connections, Sentry-side rejections (bad DSN key → 4xx) -// and rate-limit drops all complete in milliseconds, drain the queue, and -// "flush" successfully — those classes are visible only with -// SENTRY_DEBUG=true (and partially via sentryDialCheck). -func sentryStartupProbe() bool { - // Event hygiene: a fixed fingerprint + info level fold every container - // restart into one low-severity Sentry issue instead of resolve→reopen - // churn; the probe tag makes canaries filterable. Cloned hub so none of - // this leaks into the global scope. - var id *sentry.EventID - hub := sentry.CurrentHub().Clone() - hub.WithScope(func(scope *sentry.Scope) { - scope.SetFingerprint([]string{"sentry-startup-probe"}) - scope.SetTag("probe", "true") - scope.SetLevel(sentry.LevelInfo) - id = hub.CaptureMessage("sentry startup probe") - }) - if id == nil { - // Nil event ID = the client dropped the event before the transport - // saw it (e.g. a BeforeSend veto) — nothing queued, so a "successful" - // flush below would be vacuous. - Warn.Println("sentry startup probe was dropped client-side — no canary reached the transport (check BeforeSend/sampling)") - return false - } - if !hub.Flush(sentryStartupProbeTimeout) { - Warn.Println("sentry enabled but startup probe did not flush — transport still busy after the window; events may not be reaching Sentry (check DSN/egress, or set SENTRY_DEBUG=true)") - return false - } - return true -} - // FlushSentryOnShutdown installs a signal handler that flushes buffered // Sentry events before the process exits. The stack has no graceful shutdown // path (Start blocks on Serve and the process dies by signal); this is the diff --git a/rest/sentry_boot_test.go b/rest/sentry_boot_test.go index ef3cad2..6eaeca9 100644 --- a/rest/sentry_boot_test.go +++ b/rest/sentry_boot_test.go @@ -2,10 +2,8 @@ package rest import ( "bytes" - "context" "net" "os" - "sync" "syscall" "testing" "time" @@ -16,128 +14,27 @@ import ( "github.com/stretchr/testify/require" ) -// drainTransport is an in-memory sentry.Transport with a controllable Flush -// outcome. flushDrains=false simulates a drain timeout (queue still busy when -// the window closes — the hang-class failure mode), NOT a delivery failure: -// per the SDK's Flush contract, fast send failures dequeue and "flush" -// successfully. It complements transportMock (sentry_test.go), whose Flush is -// fixed true — these boot-lifecycle tests need to drive the not-drained path. -type drainTransport struct { - mu sync.Mutex - events []*sentry.Event - flushDrains bool -} - -func (t *drainTransport) Configure(sentry.ClientOptions) {} -func (t *drainTransport) Flush(time.Duration) bool { return t.flushDrains } -func (t *drainTransport) FlushWithContext(context.Context) bool { return t.flushDrains } -func (t *drainTransport) Close() {} - -func (t *drainTransport) SendEvent(event *sentry.Event) { - t.mu.Lock() - defer t.mu.Unlock() - t.events = append(t.events, event) -} - -func (t *drainTransport) Events() []*sentry.Event { - t.mu.Lock() - defer t.mu.Unlock() - return append([]*sentry.Event(nil), t.events...) -} - -// bindDrainClient binds a drain-transport Sentry client to the global hub for -// the duration of the test, restoring the unbound (disabled) state afterwards. -func bindDrainClient(t *testing.T, flushDrains bool) *drainTransport { - t.Helper() - transport := &drainTransport{flushDrains: flushDrains} - client, err := sentry.NewClient(sentry.ClientOptions{Transport: transport}) - require.NoError(t, err) - sentry.CurrentHub().BindClient(client) - t.Cleanup(func() { sentry.CurrentHub().BindClient(nil) }) - return transport -} - -func TestSentryStartupProbe_DrainTimeout_WarnsLoudly(t *testing.T) { - bindDrainClient(t, false) - - var warnBuf bytes.Buffer - Warn.SetOutput(&warnBuf) - defer Warn.SetOutput(os.Stdout) - - ok := sentryStartupProbe() - - assert.False(t, ok) - assert.Contains(t, warnBuf.String(), "startup probe did not flush", - "a hang-class transport stall is the one failure mode the drain probe can see — it must be loud") -} - -func TestSentryStartupProbe_FlushDrains_NoWarning(t *testing.T) { - transport := bindDrainClient(t, true) - - var warnBuf bytes.Buffer - Warn.SetOutput(&warnBuf) - defer Warn.SetOutput(os.Stdout) - - ok := sentryStartupProbe() - - assert.True(t, ok) - assert.Empty(t, warnBuf.String(), "a drained probe must not warn") - events := transport.Events() - require.Len(t, events, 1, "the probe must push exactly one canary event through the transport") - event := events[0] - assert.Equal(t, "sentry startup probe", event.Message) - assert.Equal(t, []string{"sentry-startup-probe"}, event.Fingerprint, - "a fixed fingerprint folds every container restart into one Sentry issue — no resolve→reopen churn") - assert.Equal(t, "true", event.Tags["probe"], "probe events must be filterable") - assert.Equal(t, sentry.LevelInfo, event.Level, "the canary is informational, never an alertable error") -} - -func TestSentryStartupProbe_ClientSideDrop_WarnsAndFails(t *testing.T) { - // A BeforeSend veto makes CaptureMessage return a nil event ID — the - // client dropped the canary before the transport ever saw it, so there is - // nothing to drain and the flush alone would report a false success. - client, err := sentry.NewClient(sentry.ClientOptions{ - Transport: &drainTransport{flushDrains: true}, - BeforeSend: func(*sentry.Event, *sentry.EventHint) *sentry.Event { return nil }, - }) - require.NoError(t, err) - sentry.CurrentHub().BindClient(client) - t.Cleanup(func() { sentry.CurrentHub().BindClient(nil) }) - - var warnBuf bytes.Buffer - Warn.SetOutput(&warnBuf) - defer Warn.SetOutput(os.Stdout) - - ok := sentryStartupProbe() - - assert.False(t, ok, "a client-side drop means no canary exercised the pipeline — probe failed") - assert.Contains(t, warnBuf.String(), "dropped client-side", - "a canary that never reached the transport must be visible, not a silent flush success") -} - -// TestSetupSentry_ProbeStall_WarnsAndWithholdsEnabledLine pins that SetupSentry -// actually INVOKES the startup probe against the client it just configured: a -// drain transport injected through the sentryTransport seam reports -// Flush=not-drained, so the stalled-transport premise holds by construction — -// no socket dial manufactures the stall, and machine load cannot invert the -// outcome (#179: a refused dial is a fast send outcome that drains the queue, -// so the old local-server stall lost the timing race under load). Deleting the -// sentryStartupProbe() call from SetupSentry turns this red (no stall warning, -// the enabled line prints, no canary reaches the transport). -func TestSetupSentry_ProbeStall_WarnsAndWithholdsEnabledLine(t *testing.T) { - transport := &drainTransport{flushDrains: false} +// TestSetupSentry_BootSendsNoEventAndWarnsOnUnreachableHost pins the boot +// contract after #259: with a DSN, SetupSentry initialises the client, runs +// the warn-only dial check, prints the enabled line, and hands the transport +// nothing. Sentry receives no event at boot. +// +// The emptiness check is authoritative, not a race won. With a custom +// Transport the SDK skips its async telemetry processor and hands events to +// the transport on the capturing goroutine (sentry-go client.go, processEvent), +// so a re-added boot event would sit in Events() before SetupSentry returns. +// +// 127.0.0.1:1 refuses instantly, so the dial check resolves no name and opens +// no outbound connection. The test shrinks the dial window anyway so a +// pathological environment cannot stall the suite. +func TestSetupSentry_BootSendsNoEventAndWarnsOnUnreachableHost(t *testing.T) { + transport := &transportMock{} sentryTransport = transport - t.Cleanup(func() { sentryTransport = nil }) - - // 127.0.0.1:1 refuses instantly — the suite's deterministic stand-in for - // the dial-check pre-check, which is frozen and irrelevant to the stall - // premise. Shrink its window anyway so a pathological environment cannot - // stall the suite. The probe window itself no longer needs shrinking: the - // stub's Flush reports not-drained immediately, regardless of load. viper.Set("SENTRY_DSN", "http://public@127.0.0.1:1/1") t.Cleanup(func() { - viper.Set("SENTRY_DSN", "") sentry.CurrentHub().BindClient(nil) + sentryTransport = nil + viper.Set("SENTRY_DSN", "") }) restoreDial := sentryDialCheckTimeout sentryDialCheckTimeout = 500 * time.Millisecond @@ -151,21 +48,18 @@ func TestSetupSentry_ProbeStall_WarnsAndWithholdsEnabledLine(t *testing.T) { enabled := SetupSentry(testRelease) - assert.True(t, enabled, "a stalled probe means degraded, never disabled — capture stays on") - assert.Contains(t, warnBuf.String(), "startup probe did not flush", - "a transport that cannot drain within the window must be loud at boot") - assert.NotContains(t, infoBuf.String(), "Sentry error capture enabled", - "the success line must be withheld when the probe could not drain") - events := transport.Events() - require.Len(t, events, 1, - "the canary must reach the transport of the client SetupSentry just configured — the probe ran against THAT client, not some pre-bound stub") - assert.Equal(t, "sentry startup probe", events[0].Message) + assert.True(t, enabled, "a refused ingest host means degraded, never disabled. Capture stays on") + assert.Empty(t, transport.Events(), + "boot must hand the transport nothing. Sentry sees no event until the first real error") + assert.Contains(t, infoBuf.String(), "Sentry error capture enabled", + "the enabled line prints whenever init succeeded, whatever the dial check found") + assert.Contains(t, warnBuf.String(), "127.0.0.1:1", + "SetupSentry must still run the dial check, and the warning must name the unreachable host") } func TestSentryDialCheck_UnreachableHost_WarnsWithHost(t *testing.T) { - // 127.0.0.1:1 refuses instantly — the deterministic stand-in for the - // wrong-DSN-host misconfig class the startup probe cannot see (fast send - // failures still drain the queue). Shrink the dial window anyway so a + // 127.0.0.1:1 refuses instantly. It is the deterministic stand-in for the + // wrong-DSN-host misconfig class. Shrink the dial window anyway so a // pathological environment cannot stall the suite. restore := sentryDialCheckTimeout sentryDialCheckTimeout = 500 * time.Millisecond diff --git a/rest/sentry_test.go b/rest/sentry_test.go index 786f119..2794fcb 100644 --- a/rest/sentry_test.go +++ b/rest/sentry_test.go @@ -103,15 +103,6 @@ func (t *transportMock) Events() []*sentry.Event { return append([]*sentry.Event(nil), t.events...) } -// reset drops any captured events. enableSentry calls it after SetupSentry so -// the boot-time startup-probe canary (#134: SetupSentry now drives the probe) -// does not count toward a test's own event assertions. -func (t *transportMock) reset() { - t.mu.Lock() - defer t.mu.Unlock() - t.events = nil -} - // testRelease is the build-time version stand-in tests pass to SetupSentry. const testRelease = "v-test-132" @@ -133,7 +124,6 @@ func enableSentry(t *testing.T) *transportMock { viper.Set("SENTRY_DSN", "") }) require.True(t, SetupSentry(testRelease), "SetupSentry must enable capture when SENTRY_DSN is set") - tr.reset() // discard the boot-time startup-probe canary so tests count only their own events return tr } diff --git a/servers/server.go b/servers/server.go index 72e3013..b38b598 100644 --- a/servers/server.go +++ b/servers/server.go @@ -147,13 +147,12 @@ func (server *MicroServer) Start() { } // Observability (PRD #112): errors-only Sentry capture, gated on SENTRY_DSN. - // Disabled (local/dev) nothing is initialised — one Info line, no client, no - // signal handler, so shutdown behaves exactly as before. Enabled, this - // BLOCKS boot before any listener opens: the dial pre-check (≤3s on an - // unreachable host) plus the startup-probe flush window (≤5s) — a worst-case - // ~8s delay on a degraded network, by design, so a broken pipeline is - // visible before traffic flows. The shutdown flush handler is installed only - // when capture is enabled. + // Disabled (local/dev) nothing is initialised: one Info line, no client, no + // signal handler, so shutdown behaves exactly as before. Enabled, the dial + // pre-check BLOCKS boot before any listener opens, for up to 3s on an + // unreachable host. That delay is accepted by design so the log names a + // broken pipeline before traffic flows. Start installs the shutdown flush + // handler only when capture is enabled. if rest.SetupSentry(version) { rest.FlushSentryOnShutdown() }