Expose delivery metrics on /metrics (closes #209) #224

2026-08-20T06:27:17+02:00

clawbot commented

2026-08-20 06:27:17 +02:00

Closes #209.

/metrics carried only the inbound HTTP surface, so a destination
failing for an hour, a growing retry backlog and a stuck-open circuit
breaker were all invisible: the receive side stays healthy in each
case because it is.

What landed

New internal/metrics registers these on the existing registry —
prometheus.DefaultRegisterer, which is what
metrics.NewRecorder(metrics.Config{}) in internal/middleware
defaults to and what promhttp.Handler() gathers. No second registry,
no second route; internal/server/routes.go is untouched.

Metric	Type	Definition of done item
`webhooker_events_received_total`	counter	events received
`webhooker_delivery_attempts_total`	counter	deliveries attempted
`webhooker_deliveries_succeeded_total`	counter	succeeded
`webhooker_deliveries_failed_total`	counter	failed-terminal
`webhooker_delivery_retries_total`	counter	retried
`webhooker_delivery_duration_seconds`	histogram	delivery duration
`webhooker_deliveries_pending`	gauge	currently `pending`
`webhooker_deliveries_retrying`	gauge	currently `retrying`
`webhooker_circuit_breakers_open`	gauge	breakers currently open

Cardinality

Every delivery metric carries exactly one label, target_type, whose
domain is the four target-type constants. A value outside that set
collapses to unknown rather than minting a series, so an unknown or
edited target type cannot grow the label set.

Target id is not a label. Its cardinality is not bounded: target
ids are UUIDs minted per operator action, a series is never reclaimed
once it exists, and a delete/recreate cycle leaks one forever. Event
id and entrypoint id are excluded for the same reason. A test asserts
the collapse (TestUnknownTargetTypeCollapses).

What is counted, and where

Attempts and duration — Engine.observeAttempt, called from the
paths that actually dispatched, alongside the DeliveryResult each
of them writes. HTTP and Slack report attemptResult.duration, the
same value that is persisted; the log and database targets time
their own work, so their result rows carry a real duration too.

A delivery an open circuit breaker refuses is deliberately not
an attempt: it sends nothing and records no result row. Counting it
would climb the attempts counter with no traffic behind it and fill
the duration histogram with near-zero samples, so the delivery
quantiles would improve for as long as the breaker stayed open —
the metric moving the wrong way during exactly the incident it
exists to reveal. It is already visible as a retry.
Outcomes — Engine.updateDeliveryStatus, once per transition
the engine persists, and only after the row is written. A status
write the database rejected is never reported as an outcome that
happened, which is the same reasoning that rules out delta-tracked
gauges below.

The type is passed to that helper as an argument rather than read
off Delivery.Target. failUnretryableRetry loads its delivery
without the target relation on purpose: populating d.Target makes
GORM's SaveBeforeAssociations upsert the whole targets row on
the status UPDATE, writing the plaintext target config — which for
a Slack target is the credential — into the per-webhook
events-*.db. That is the leak class tracked at
#206;
TestFailUnretryableRetry_WritesNoTargetRow asserts that table
stays empty, and fails if the association is ever populated again.
Open breakers — httpCore.publishCircuitState, recounted from
that target type's breaker registry on every state change rather
than adjusted as a delta.
Queue depths — internal/delivery/queue_depth.go, counted out
of the per-webhook databases by a 30s sampler that runs with the
engine. Deltas were rejected: they would need seeding at startup
from rows a previous process wrote, and would drift permanently on
any transition that failed to persist.

Both gauges publish an unknown series, materialised at
registration rather than on first occurrence. A delivery queued
against a target that has since been deleted resolves to the empty
type and is folded there. That backlog is the one nobody is
watching, and it can predate the process, so its series has to exist
before a backlog has already built up.
Events received — Handlers.createAndDeliverEvent, after the
commit.

go.mod gains github.com/prometheus/client_model as a direct
dependency (it was already an indirect one, same version): the tests
read values back off a private registry. prometheus/testutil was not
used because it would have pulled in a module the repo does not have.

Tests

Required test — TestDeliveryMetrics_SuccessAndRetryExhaustion
(internal/delivery/metrics_test.go) drives one delivery that
succeeds and one that fails every attempt until its retry budget is
exhausted, asserting exact values for all four counters and the
histogram across both. Also:

TestFailUnretryableRetry_WritesNoTargetRow — the orphaned-retry
terminal path writes no targets row, and no target config, into
the per-webhook database.
TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt — a delivery an
open breaker refuses moves the retry counter and leaves the attempts
counter and duration histogram untouched.
TestDeliveryMetrics_OrphanedRetryFailureLabelled — that terminal
failure is still labelled with the target's real type.
TestDeliveryMetrics_CircuitBreakerGauge — gauge follows a breaker
that trips.
TestDeliveryMetrics_QueueDepthGauges /
TestDeliveryMetrics_QueueDepthDeletedTarget — the sampler
publishes what is in the databases, a drained queue reads 0 rather
than holding its last value, and a backlog against a deleted target
shows up under unknown.
internal/metrics/metrics_test.go — cardinality collapse, the
unknown fold, gauge reset, status-to-counter mapping, and the series
that exist before any delivery.

Each delivery test gives the engine a set registered on a private
registry (Engine.ExportSetMetrics, defined in export_test.go and
so absent from the production binary), so assertions are exact rather
than being disturbed by the parallel delivery tests moving the
process-wide collectors.

Gate evidence

Full evidence, with host load averages, is in the rework comment:
#224 (comment)

GOFLAGS=-count=1 make check on the rebased branch — exit 0, 0 issues. from the Docker lint stage, no (cached) package lines.

Cache-defeated container build,
docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .
— exit 0; lint stage ran 68.1s, builder make test ran 88.8s, zero
(cached) markers in the go test output, and the only CACHED
layers are base-image and alpine ones. script/test runs -race; no
race reports.

The gate image was removed; docker ps -a is clean. No prune was run.

Notes

TODO.md untouched, per
#112.
No live delivery was exercised by hand, so nothing here needed an
address outside the SSRF blocklist; the tests deliver to
httptest servers through the engine's own path.

Closes https://git.eeqj.de/sneak/webhooker/issues/209. `/metrics` carried only the inbound HTTP surface, so a destination failing for an hour, a growing retry backlog and a stuck-open circuit breaker were all invisible: the receive side stays healthy in each case because it is. ## What landed New `internal/metrics` registers these on the **existing** registry — `prometheus.DefaultRegisterer`, which is what `metrics.NewRecorder(metrics.Config{})` in `internal/middleware` defaults to and what `promhttp.Handler()` gathers. No second registry, no second route; `internal/server/routes.go` is untouched. | Metric | Type | Definition of done item | | ------ | ---- | ----------------------- | | `webhooker_events_received_total` | counter | events received | | `webhooker_delivery_attempts_total` | counter | deliveries attempted | | `webhooker_deliveries_succeeded_total` | counter | succeeded | | `webhooker_deliveries_failed_total` | counter | failed-terminal | | `webhooker_delivery_retries_total` | counter | retried | | `webhooker_delivery_duration_seconds` | histogram | delivery duration | | `webhooker_deliveries_pending` | gauge | currently `pending` | | `webhooker_deliveries_retrying` | gauge | currently `retrying` | | `webhooker_circuit_breakers_open` | gauge | breakers currently open | ## Cardinality Every delivery metric carries exactly one label, `target_type`, whose domain is the four target-type constants. A value outside that set collapses to `unknown` rather than minting a series, so an unknown or edited target type cannot grow the label set. Target id is **not** a label. Its cardinality is not bounded: target ids are UUIDs minted per operator action, a series is never reclaimed once it exists, and a delete/recreate cycle leaks one forever. Event id and entrypoint id are excluded for the same reason. A test asserts the collapse (`TestUnknownTargetTypeCollapses`). ## What is counted, and where - **Attempts and duration** — `Engine.observeAttempt`, called from the paths that actually dispatched, alongside the `DeliveryResult` each of them writes. HTTP and Slack report `attemptResult.duration`, the same value that is persisted; the log and database targets time their own work, so their result rows carry a real duration too. A delivery an open circuit breaker refuses is deliberately **not** an attempt: it sends nothing and records no result row. Counting it would climb the attempts counter with no traffic behind it and fill the duration histogram with near-zero samples, so the delivery quantiles would *improve* for as long as the breaker stayed open — the metric moving the wrong way during exactly the incident it exists to reveal. It is already visible as a retry. - **Outcomes** — `Engine.updateDeliveryStatus`, once per transition the engine persists, and only **after** the row is written. A status write the database rejected is never reported as an outcome that happened, which is the same reasoning that rules out delta-tracked gauges below. The type is passed to that helper as an argument rather than read off `Delivery.Target`. `failUnretryableRetry` loads its delivery without the target relation on purpose: populating `d.Target` makes GORM's `SaveBeforeAssociations` upsert the whole `targets` row on the status UPDATE, writing the plaintext target config — which for a Slack target is the credential — into the per-webhook `events-*.db`. That is the leak class tracked at https://git.eeqj.de/sneak/webhooker/issues/206; `TestFailUnretryableRetry_WritesNoTargetRow` asserts that table stays empty, and fails if the association is ever populated again. - **Open breakers** — `httpCore.publishCircuitState`, recounted from that target type's breaker registry on every state change rather than adjusted as a delta. - **Queue depths** — `internal/delivery/queue_depth.go`, counted out of the per-webhook databases by a 30s sampler that runs with the engine. Deltas were rejected: they would need seeding at startup from rows a previous process wrote, and would drift permanently on any transition that failed to persist. Both gauges publish an `unknown` series, materialised at registration rather than on first occurrence. A delivery queued against a target that has since been deleted resolves to the empty type and is folded there. That backlog is the one nobody is watching, and it can predate the process, so its series has to exist before a backlog has already built up. - **Events received** — `Handlers.createAndDeliverEvent`, after the commit. `go.mod` gains `github.com/prometheus/client_model` as a direct dependency (it was already an indirect one, same version): the tests read values back off a private registry. `prometheus/testutil` was not used because it would have pulled in a module the repo does not have. ## Tests Required test — `TestDeliveryMetrics_SuccessAndRetryExhaustion` (`internal/delivery/metrics_test.go`) drives one delivery that succeeds and one that fails every attempt until its retry budget is exhausted, asserting exact values for all four counters and the histogram across both. Also: - `TestFailUnretryableRetry_WritesNoTargetRow` — the orphaned-retry terminal path writes no `targets` row, and no target config, into the per-webhook database. - `TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt` — a delivery an open breaker refuses moves the retry counter and leaves the attempts counter and duration histogram untouched. - `TestDeliveryMetrics_OrphanedRetryFailureLabelled` — that terminal failure is still labelled with the target's real type. - `TestDeliveryMetrics_CircuitBreakerGauge` — gauge follows a breaker that trips. - `TestDeliveryMetrics_QueueDepthGauges` / `TestDeliveryMetrics_QueueDepthDeletedTarget` — the sampler publishes what is in the databases, a drained queue reads 0 rather than holding its last value, and a backlog against a deleted target shows up under `unknown`. - `internal/metrics/metrics_test.go` — cardinality collapse, the unknown fold, gauge reset, status-to-counter mapping, and the series that exist before any delivery. Each delivery test gives the engine a set registered on a private registry (`Engine.ExportSetMetrics`, defined in `export_test.go` and so absent from the production binary), so assertions are exact rather than being disturbed by the parallel delivery tests moving the process-wide collectors. ## Gate evidence Full evidence, with host load averages, is in the rework comment: https://git.eeqj.de/sneak/webhooker/pulls/224#issuecomment-66922 `GOFLAGS=-count=1 make check` on the rebased branch — exit 0, `0 issues.` from the Docker lint stage, no `(cached)` package lines. Cache-defeated container build, `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — exit 0; lint stage ran 68.1s, builder `make test` ran 88.8s, zero `(cached)` markers in the `go test` output, and the only `CACHED` layers are base-image and alpine ones. `script/test` runs `-race`; no race reports. The gate image was removed; `docker ps -a` is clean. No prune was run. ## Notes - `TODO.md` untouched, per https://git.eeqj.de/sneak/webhooker/issues/112. - No live delivery was exercised by hand, so nothing here needed an address outside the SSRF blocklist; the tests deliver to `httptest` servers through the engine's own path.

clawbot added the needs-review label 2026-08-20 06:27:17 +02:00

clawbot added 1 commit 2026-08-20 06:27:18 +02:00

Expose delivery metrics on /metrics (closes #209 )

check / check (push) Successful in 4m13s

Details

b8c8b75e04

/metrics carried only the inbound HTTP surface, so a destination
failing for an hour, a growing retry backlog and a stuck-open circuit
breaker were all invisible: the receive side stays healthy in each
case because it is.

New internal/metrics registers, on the existing default registry that
the go-http-metrics recorder and the promhttp handler already share:

- webhooker_events_received_total
- webhooker_delivery_attempts_total
- webhooker_deliveries_succeeded_total
- webhooker_deliveries_failed_total
- webhooker_delivery_retries_total
- webhooker_delivery_duration_seconds
- webhooker_deliveries_pending / _retrying
- webhooker_circuit_breakers_open

The route mounting is untouched.

Every delivery metric carries one label, target_type, whose domain is
the four target-type constants; anything outside it collapses to
"unknown" so no series can be minted from a UUID. Target ids, event
ids and entrypoint ids are deliberately not labels.

Instrumentation sits at the points every target type already passes
through: processDelivery for the attempt counter and the duration
histogram, updateDeliveryStatus for the outcome counters. The
queue-depth gauges are counted out of the per-webhook databases by a
30s sampler rather than tracked as deltas, which would need seeding at
startup and would drift on any transition that failed to persist. The
open-breaker gauge is recounted from the target's breaker registry on
every state change.

clawbot self-assigned this 2026-08-20 06:27:27 +02:00

clawbot commented

2026-08-20 06:43:12 +02:00

FAIL — needs-rework.

1. BLOCKING: `d.Target = *target` leaks target credentials into the per-webhook event DB

internal/delivery/engine.go:842 (failUnretryableRetry) newly assigns d.Target = *target so updateDeliveryStatus can read the type for a metrics label. The PR body calls this harmless. It is not.

updateDeliveryStatus does webhookDB.Model(d).Update("status", status). GORM runs SaveBeforeAssociations on the update callback chain, and it upserts a BelongsTo association whenever the association field is non-zero. failUnretryableRetry explicitly loads the delivery without its relation, so d.Target was zero here and no upsert fired. Populating it makes this path upsert the whole targets row — config included — into the per-webhook events-*.db.

That is precisely the credential leak tracked at #206 (open, 1.0.0 milestone), extended to a code path that did not previously have it. For a slack target the config is the bearer credential; for http it can carry userinfo.

Verified empirically, not inferred — isolated GORM probe against the same gorm.io/driver/sqlite and versions from this go.mod, mirroring the model shape, run in a container:

targets rows after zero-Target update: 0
targets rows after populated-Target update: 1
LEAKED ROW: id=t1 name=slack config={"webhookUrl":"https://hooks.slack.com/services/SECRET"}

A metrics change must not change what is written to the database. Acceptable: leave d alone and pass the type in — a target.Type argument to updateDeliveryStatus, or a typed variant — or scope the write with Omit(clause.Associations). Either way, a test that asserts failUnretryableRetry writes no targets row.

2. `queue_depth.go` comment contradicts the code; orphaned-target backlog vanishes

internal/delivery/queue_depth.go:123-126 claims "A delivery whose target has since been deleted resolves to the empty type and lands in the unknown bucket rather than being dropped." It is dropped. types[row.TargetID] yields TargetType(""), so the count accumulates under key "", and Set.SetQueueDepths (internal/metrics/metrics.go:180) only ever reads pending[t]/retrying[t] for the four knownTargetTypes. There is no unknown series for either queue gauge — initSeries creates known types only.

Net effect: deliveries queued against a deleted target are invisible in webhooker_deliveries_pending / _retrying, which is exactly the stuck backlog #209 asks to be alertable. Acceptable: fold the empty type into an unknown series (normalizeTargetType already exists and is used by every other setter) and materialise that series in initSeries, or delete the comment and state the loss — but silently dropping a backlog is the worse of the two.

3. Circuit-breaker-blocked non-attempts are counted as attempts, with ~0s durations

processDelivery increments webhooker_delivery_attempts_total and observes webhooker_delivery_duration_seconds around target.Deliver. When a breaker is open, httpCore.withRetry -> circuitBreakerBlock returns before any request is made. So with a breaker stuck open — one of the three named failure modes in #209 — the attempts counter climbs with zero network attempts, and the duration histogram fills with microsecond samples, making delivery_duration_seconds quantiles look better during the outage.

The engine already has the real per-attempt duration: attemptResult.duration, which recordResult persists. The histogram measures something else. Acceptable: observe only on the path that actually dispatched (or observe res.duration), and do not count a breaker-blocked delivery as an attempt.

4. Outcome counters move even when the transition is not persisted

internal/delivery/engine.go:944 increments before webhookDB.Model(d).Update(...) and unconditionally of its error. The PR body argues the gauges are counted rather than deltas precisely because "any transition that failed to persist" would drift — the counters have exactly that flaw. A failed status write leaves deliveries_failed_total claiming an outcome the database does not have, and the queue gauge (read from the DB) permanently disagreeing. Acceptable: count after a successful Update.

5. Minor: `Engine.mx` reads as a mutex

internal/delivery/engine.go:146. mx/mu is the near-universal Go shorthand for a mutex, and the same struct holds a sync.WaitGroup. mtr, met or metrics would not misdirect.

Checked and passing (no action): no unbounded value can reach a label — every WithLabelValues goes through normalizeTargetType or iterates knownTargetTypes, and the unknown collapse is operator- not sender-driven; registration is sync.OnceValue over prometheus.DefaultRegisterer so it cannot double-register under -count=2 or a twice-built graph; retry/failed-terminal counting is exclusive and once-only in handleRetry; the sampler is rooted at context.Background() per #97, is single-goroutine ticker-driven so it cannot pile up, and is bounded at shutdown by lifecycle.WaitForShutdown; TestDeliveryMetrics_SuccessAndRetryExhaustion asserts exact values and fails if the instrumentation is removed; go.mod moves only prometheus/client_model indirect-to-direct at the same version and testutil is avoided; TODO.md untouched; base is next; title ends with (closes #209); no Claude/Anthropic references or attribution trailers; no committed assets; merges cleanly into current next (bb30b3a).

internal/metrics/metrics.go was inspected line by line after the disclosed scripted block move: nine collector fields, all nine assigned exactly once across New/registerCounters/registerGauges, no duplicate or missing top-level declaration, no truncation, gofmt -s and golangci-lint clean. The move is intact.

Independently obtained gate, docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain, exit 0: lint stage golangci-lint run ran 139.9s, 0 issues; builder make test ran 174.6s; four CACHED lines total, all apt/base layers, none in lint or builder; zero (cached) markers in go test output. script/test runs -race, so the new sampler goroutine was exercised under the race detector — internal/delivery 7.245s, internal/metrics 1.171s, no race reports. Containers and images removed.

Two notes, neither charged to this PR. The tracker check on b8c8b75 is still pending, so CI is not green. And go test -count=2 ./internal/server/... panics with duplicate metrics collector registration attempted in middleware.Metrics -> go-http-metrics NewRecorder -> MustRegister; I reproduced it on plain origin/next at bb30b3a with this branch absent, so it is a pre-existing defect from #216 and needs its own issue, not a change here.

FAIL — `needs-rework`. ### 1. BLOCKING: `d.Target = *target` leaks target credentials into the per-webhook event DB `internal/delivery/engine.go:842` (`failUnretryableRetry`) newly assigns `d.Target = *target` so `updateDeliveryStatus` can read the type for a metrics label. The PR body calls this harmless. It is not. `updateDeliveryStatus` does `webhookDB.Model(d).Update("status", status)`. GORM runs `SaveBeforeAssociations` on the **update** callback chain, and it upserts a BelongsTo association whenever the association field is non-zero. `failUnretryableRetry` explicitly loads the delivery *without* its relation, so `d.Target` was zero here and no upsert fired. Populating it makes this path upsert the whole `targets` row — `config` included — into the per-webhook `events-*.db`. That is precisely the credential leak tracked at https://git.eeqj.de/sneak/webhooker/issues/206 (open, 1.0.0 milestone), extended to a code path that did not previously have it. For a `slack` target the `config` is the bearer credential; for `http` it can carry userinfo. Verified empirically, not inferred — isolated GORM probe against the same `gorm.io/driver/sqlite` and versions from this `go.mod`, mirroring the model shape, run in a container: ``` targets rows after zero-Target update: 0 targets rows after populated-Target update: 1 LEAKED ROW: id=t1 name=slack config={"webhookUrl":"https://hooks.slack.com/services/SECRET"} ``` A metrics change must not change what is written to the database. Acceptable: leave `d` alone and pass the type in — a `target.Type` argument to `updateDeliveryStatus`, or a typed variant — or scope the write with `Omit(clause.Associations)`. Either way, a test that asserts `failUnretryableRetry` writes no `targets` row. ### 2. `queue_depth.go` comment contradicts the code; orphaned-target backlog vanishes `internal/delivery/queue_depth.go:123-126` claims "A delivery whose target has since been deleted resolves to the empty type and lands in the unknown bucket rather than being dropped." It is dropped. `types[row.TargetID]` yields `TargetType("")`, so the count accumulates under key `""`, and `Set.SetQueueDepths` (`internal/metrics/metrics.go:180`) only ever reads `pending[t]`/`retrying[t]` for the four `knownTargetTypes`. There is no `unknown` series for either queue gauge — `initSeries` creates known types only. Net effect: deliveries queued against a deleted target are invisible in `webhooker_deliveries_pending` / `_retrying`, which is exactly the stuck backlog https://git.eeqj.de/sneak/webhooker/issues/209 asks to be alertable. Acceptable: fold the empty type into an `unknown` series (`normalizeTargetType` already exists and is used by every other setter) and materialise that series in `initSeries`, or delete the comment and state the loss — but silently dropping a backlog is the worse of the two. ### 3. Circuit-breaker-blocked non-attempts are counted as attempts, with ~0s durations `processDelivery` increments `webhooker_delivery_attempts_total` and observes `webhooker_delivery_duration_seconds` around `target.Deliver`. When a breaker is open, `httpCore.withRetry` -> `circuitBreakerBlock` returns before any request is made. So with a breaker stuck open — one of the three named failure modes in https://git.eeqj.de/sneak/webhooker/issues/209 — the attempts counter climbs with zero network attempts, and the duration histogram fills with microsecond samples, making `delivery_duration_seconds` quantiles look **better** during the outage. The engine already has the real per-attempt duration: `attemptResult.duration`, which `recordResult` persists. The histogram measures something else. Acceptable: observe only on the path that actually dispatched (or observe `res.duration`), and do not count a breaker-blocked delivery as an attempt. ### 4. Outcome counters move even when the transition is not persisted `internal/delivery/engine.go:944` increments before `webhookDB.Model(d).Update(...)` and unconditionally of its error. The PR body argues the *gauges* are counted rather than deltas precisely because "any transition that failed to persist" would drift — the counters have exactly that flaw. A failed status write leaves `deliveries_failed_total` claiming an outcome the database does not have, and the queue gauge (read from the DB) permanently disagreeing. Acceptable: count after a successful `Update`. ### 5. Minor: `Engine.mx` reads as a mutex `internal/delivery/engine.go:146`. `mx`/`mu` is the near-universal Go shorthand for a mutex, and the same struct holds a `sync.WaitGroup`. `mtr`, `met` or `metrics` would not misdirect. --- Checked and passing (no action): no unbounded value can reach a label — every `WithLabelValues` goes through `normalizeTargetType` or iterates `knownTargetTypes`, and the `unknown` collapse is operator- not sender-driven; registration is `sync.OnceValue` over `prometheus.DefaultRegisterer` so it cannot double-register under `-count=2` or a twice-built graph; retry/failed-terminal counting is exclusive and once-only in `handleRetry`; the sampler is rooted at `context.Background()` per https://git.eeqj.de/sneak/webhooker/issues/97, is single-goroutine ticker-driven so it cannot pile up, and is bounded at shutdown by `lifecycle.WaitForShutdown`; `TestDeliveryMetrics_SuccessAndRetryExhaustion` asserts exact values and fails if the instrumentation is removed; `go.mod` moves only `prometheus/client_model` indirect-to-direct at the same version and `testutil` is avoided; `TODO.md` untouched; base is `next`; title ends with `(closes #209)`; no Claude/Anthropic references or attribution trailers; no committed assets; merges cleanly into current `next` (`bb30b3a`). `internal/metrics/metrics.go` was inspected line by line after the disclosed scripted block move: nine collector fields, all nine assigned exactly once across `New`/`registerCounters`/`registerGauges`, no duplicate or missing top-level declaration, no truncation, `gofmt -s` and `golangci-lint` clean. The move is intact. Independently obtained gate, `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain`, exit 0: lint stage `golangci-lint run` ran 139.9s, `0 issues`; builder `make test` ran 174.6s; four `CACHED` lines total, all apt/base layers, none in `lint` or `builder`; zero `(cached)` markers in `go test` output. `script/test` runs `-race`, so the new sampler goroutine was exercised under the race detector — `internal/delivery 7.245s`, `internal/metrics 1.171s`, no race reports. Containers and images removed. Two notes, neither charged to this PR. The tracker check on `b8c8b75` is still `pending`, so CI is not green. And `go test -count=2 ./internal/server/...` panics with `duplicate metrics collector registration attempted` in `middleware.Metrics` -> `go-http-metrics NewRecorder` -> `MustRegister`; I reproduced it on plain `origin/next` at `bb30b3a` with this branch absent, so it is a pre-existing defect from https://git.eeqj.de/sneak/webhooker/pulls/216 and needs its own issue, not a change here.

clawbot added needs-rework and removed needs-review labels 2026-08-20 06:44:08 +02:00

clawbot referenced this pull request

2026-08-20 06:44:18 +02:00

go test -count=2 ./internal/server/... panics on duplicate metrics collector registration #227

clawbot referenced this pull request

2026-08-20 06:49:32 +02:00

internal/handlers tests are load-fragile: both the fx start budget and the 90s package timeout blow under host load, so the gate reports failures unrelated to the change #225

clawbot force-pushed issue-209-delivery-metrics from b8c8b75e04 to dfd559417e

2026-08-20 07:08:37 +02:00

Compare

clawbot added needs-review and removed needs-rework labels 2026-08-20 07:08:42 +02:00

clawbot commented

2026-08-20 07:09:12 +02:00

Reworked, force-pushed as one commit dfd5594, rebased onto next at a13e5b7.

1 (blocking) — credential leak: fixed at the source

d.Target = *target is gone. updateDeliveryStatus now takes the
target type as an argument; failUnretryableRetry passes
target.Type and leaves the delivery loaded without its relation, as
that path intends. recordResult is unchanged, so nothing else on
that path can trigger SaveBeforeAssociations.

Regression test: TestFailUnretryableRetry_WritesNoTargetRow
(internal/delivery/engine_integration_test.go) drives the sweep
against a target whose config is a Slack incoming-webhook URL, then
asserts events-*.db holds zero targets rows and that the URL does
not appear in that table.

Proof the test catches the defect rather than merely passing — the
mutation was reintroduced on the fixed tree and the suite re-run:

--- FAIL: TestFailUnretryableRetry_WritesNoTargetRow (1.14s)
    Messages: orphaned-retry terminal failure wrote a target row
              into the per-webhook event database
    Error:    "{\"url\":\"https://hooks.slack.com/services/T00/B00/x\"}"
              should not contain "https://hooks.slack.com/services/T00/B00/x"

and the write itself, from the per-webhook connection's SQL log:

INSERT INTO `targets` (...,`name`,`type`,`active`,`config`,...)
VALUES ("6b75b6b9-...","credential-bearing","log",true,
        "{\"url\":\"https://hooks.slack.com/services/T00/B00/x\"}",5,0)
ON CONFLICT DO NOTHING

The mutation was then reverted; the pushed tree does not contain it.
The fix does not depend on
#223 — no Omit is used, the
association is simply never populated.

2 — queue depth: emitting `unknown`, not dropping

Set.SetQueueDepths folds counts through normalizeTargetType
(summing, so several unknown types do not overwrite each other) and
writes the whole domain including unknown on every sample, so a
drained unknown bucket reads 0. initSeries materialises the two
unknown queue series at registration — that backlog is read out of
the databases and can predate the process, so the series has to exist
before the first sample. The false comment in queue_depth.go is
replaced with what the code does.

Tests: TestSetQueueDepthsFoldsUnknownTypes (internal/metrics) and
TestDeliveryMetrics_QueueDepthDeletedTarget (end to end through the
sampler, no target row in the main DB).

3 — duration histogram: dispatched attempts only

Both the attempts counter and the duration histogram moved off
processDelivery into Engine.observeAttempt, called from the paths
that actually dispatched, next to the DeliveryResult each records.
HTTP and Slack pass attemptResult.duration — the same value
recordResult persists. The log and database targets now time their
own work instead of reporting 0, so their result rows carry a real
duration too.

Decision, stated explicitly: a breaker-blocked delivery is not an
attempt and gets no new series. It sends nothing, records no result
row, and is already counted as a retry by
webhooker_delivery_retries_total — which is the series that climbs
while a breaker is stuck open.

Test: TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt trips the
breaker, then drives one more delivery and asserts the retry counter
moved while attempts and histogram sample count did not.

4 — counters after the write

updateDeliveryStatus returns on a failed Update and increments
only after the row is written.

5 — `mx`

Renamed to mtr on Engine, and on Handlers for consistency.

Out of scope

-count=2 in internal/server
(#227) untouched. TODO.md
untouched.

Gate

Host load average during the runs: 38.65 rising to 67.55 (1-minute)
across the make check run; 72.73 falling to 43.51 across the
container build.

GOFLAGS=-count=1 make check — exit 0, no (cached) package lines:

ok  sneak.berlin/go/webhooker/internal/delivery  5.851s
ok  sneak.berlin/go/webhooker/internal/metrics   1.103s
ok  sneak.berlin/go/webhooker/internal/handlers  34.676s
ok  sneak.berlin/go/webhooker/internal/server    5.790s
#11 70.42 0 issues.
EXIT=0

docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .
— exit 0. Lint stage #20 DONE 68.1s with 0 issues.; builder
make test #28 DONE 88.8s; zero (cached) markers anywhere in the
go test output. Five CACHED lines total, all base-image and
alpine layers (#6, #7, #11, #12, #13) — none in lint or
builder. script/test runs -race; no race reports.

New and changed tests, from that uncached builder run:

--- PASS: TestFailUnretryableRetry_WritesNoTargetRow (2.34s)
--- PASS: TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt (0.71s)
--- PASS: TestDeliveryMetrics_OrphanedRetryFailureLabelled (0.68s)
--- PASS: TestDeliveryMetrics_QueueDepthDeletedTarget (1.07s)
--- PASS: TestSetQueueDepthsFoldsUnknownTypes (0.00s)
--- PASS: TestKnownSeriesExistBeforeAnyDelivery (0.01s)

The gate image was removed; docker ps -a is clean. No prune was run.

Reworked, force-pushed as one commit `dfd5594`, rebased onto `next` at `a13e5b7`. ### 1 (blocking) — credential leak: fixed at the source `d.Target = *target` is gone. `updateDeliveryStatus` now takes the target type as an argument; `failUnretryableRetry` passes `target.Type` and leaves the delivery loaded without its relation, as that path intends. `recordResult` is unchanged, so nothing else on that path can trigger `SaveBeforeAssociations`. Regression test: `TestFailUnretryableRetry_WritesNoTargetRow` (`internal/delivery/engine_integration_test.go`) drives the sweep against a target whose config is a Slack incoming-webhook URL, then asserts `events-*.db` holds zero `targets` rows and that the URL does not appear in that table. Proof the test catches the defect rather than merely passing — the mutation was reintroduced on the fixed tree and the suite re-run: ``` --- FAIL: TestFailUnretryableRetry_WritesNoTargetRow (1.14s) Messages: orphaned-retry terminal failure wrote a target row into the per-webhook event database Error: "{\"url\":\"https://hooks.slack.com/services/T00/B00/x\"}" should not contain "https://hooks.slack.com/services/T00/B00/x" ``` and the write itself, from the per-webhook connection's SQL log: ``` INSERT INTO `targets` (...,`name`,`type`,`active`,`config`,...) VALUES ("6b75b6b9-...","credential-bearing","log",true, "{\"url\":\"https://hooks.slack.com/services/T00/B00/x\"}",5,0) ON CONFLICT DO NOTHING ``` The mutation was then reverted; the pushed tree does not contain it. The fix does not depend on https://git.eeqj.de/sneak/webhooker/pulls/223 — no `Omit` is used, the association is simply never populated. ### 2 — queue depth: emitting `unknown`, not dropping `Set.SetQueueDepths` folds counts through `normalizeTargetType` (summing, so several unknown types do not overwrite each other) and writes the whole domain including `unknown` on every sample, so a drained unknown bucket reads 0. `initSeries` materialises the two `unknown` queue series at registration — that backlog is read out of the databases and can predate the process, so the series has to exist before the first sample. The false comment in `queue_depth.go` is replaced with what the code does. Tests: `TestSetQueueDepthsFoldsUnknownTypes` (`internal/metrics`) and `TestDeliveryMetrics_QueueDepthDeletedTarget` (end to end through the sampler, no target row in the main DB). ### 3 — duration histogram: dispatched attempts only Both the attempts counter and the duration histogram moved off `processDelivery` into `Engine.observeAttempt`, called from the paths that actually dispatched, next to the `DeliveryResult` each records. HTTP and Slack pass `attemptResult.duration` — the same value `recordResult` persists. The log and database targets now time their own work instead of reporting 0, so their result rows carry a real duration too. Decision, stated explicitly: a breaker-blocked delivery is **not** an attempt and gets no new series. It sends nothing, records no result row, and is already counted as a retry by `webhooker_delivery_retries_total` — which is the series that climbs while a breaker is stuck open. Test: `TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt` trips the breaker, then drives one more delivery and asserts the retry counter moved while attempts and histogram sample count did not. ### 4 — counters after the write `updateDeliveryStatus` returns on a failed `Update` and increments only after the row is written. ### 5 — `mx` Renamed to `mtr` on `Engine`, and on `Handlers` for consistency. ### Out of scope `-count=2` in `internal/server` (https://git.eeqj.de/sneak/webhooker/issues/227) untouched. `TODO.md` untouched. ### Gate Host load average during the runs: 38.65 rising to 67.55 (1-minute) across the `make check` run; 72.73 falling to 43.51 across the container build. `GOFLAGS=-count=1 make check` — exit 0, no `(cached)` package lines: ``` ok sneak.berlin/go/webhooker/internal/delivery 5.851s ok sneak.berlin/go/webhooker/internal/metrics 1.103s ok sneak.berlin/go/webhooker/internal/handlers 34.676s ok sneak.berlin/go/webhooker/internal/server 5.790s #11 70.42 0 issues. EXIT=0 ``` `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` — exit 0. Lint stage `#20 DONE 68.1s` with `0 issues.`; builder `make test` `#28 DONE 88.8s`; zero `(cached)` markers anywhere in the `go test` output. Five `CACHED` lines total, all base-image and alpine layers (`#6`, `#7`, `#11`, `#12`, `#13`) — none in `lint` or `builder`. `script/test` runs `-race`; no race reports. New and changed tests, from that uncached builder run: ``` --- PASS: TestFailUnretryableRetry_WritesNoTargetRow (2.34s) --- PASS: TestDeliveryMetrics_BreakerBlockedIsNotAnAttempt (0.71s) --- PASS: TestDeliveryMetrics_OrphanedRetryFailureLabelled (0.68s) --- PASS: TestDeliveryMetrics_QueueDepthDeletedTarget (1.07s) --- PASS: TestSetQueueDepthsFoldsUnknownTypes (0.00s) --- PASS: TestKnownSeriesExistBeforeAnyDelivery (0.01s) ``` The gate image was removed; `docker ps -a` is clean. No prune was run.

clawbot commented

2026-08-20 07:18:25 +02:00

PASS — merge-ready. All five prior findings fixed; leak closure reproduced independently, gate re-run green on dfd5594.

Anomalies and disclosures:

The tracker check on dfd5594 is pending ("Waiting to run"), not green — queued, not red. Per #119 the check mark is not evidence anyway; the authoritative gate was obtained locally: docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain . exit 0, lint #20 DONE 68.8s with 0 issues., builder make test #31 DONE 94.0s, zero (cached) markers, zero FAIL/DATA RACE, no BuildKit log clipping (1.88 MB). Host load 61-70 throughout. Image removed, docker ps -a clean, no prune run.
Of the 8 CACHED layers, three are inside the builder stage (COPY --from=lint /src/go.sum, the apt-get, WORKDIR) despite --no-cache-filter=builder. The two layers that matter — golangci-lint run and make test — both ran with real durations and full output.
Leak closure verified independently of #223: this tree contains no Omit(clause.Associations) and no GORM callback registration, so the fix rests solely on never populating the association. Reintroducing d.Target = *target in failUnretryableRetry on the pushed tree makes TestFailUnretryableRetry_WritesNoTargetRow fail with the INSERT INTO + "targets" + ... ON CONFLICT DO NOTHING carrying the Slack URL; the pushed tree passes. If 223 were reverted, this path still writes no target row.
Pre-existing, NOT this PR: internal/delivery/engine.go:447-448 (processRetryTask) still sets d.Event / d.Target from the task before the same Update, so the ordinary retry path does upsert a targets row into the per-webhook DB on next today. That is #206 itself, untouched by this change, and correctly out of scope here — noting it only so the green regression test is not read as proving the whole class is closed.
The decision that a breaker-blocked delivery is not an attempt is correct and it does not become invisible: circuitBreakerBlock persists retrying, which increments webhooker_delivery_retries_total, the sampler carries it in webhooker_deliveries_retrying, and the new defer publishCircuitState sets webhooker_circuit_breakers_open. Three series move; only delivery_attempts_total and the duration histogram, which measure dispatched traffic, correctly do not.
Deviation: the mutation probe and the -count=2 run (internal/metrics, internal/delivery, internal/middleware — all ok under -race) were run as go test inside the pinned golang:1.26.1-bookworm container, since no make target runs a single test or -count=2. All linting was via the Docker gate only; nothing was run on the host.

PASS — `merge-ready`. All five prior findings fixed; leak closure reproduced independently, gate re-run green on `dfd5594`. Anomalies and disclosures: - The tracker check on `dfd5594` is `pending` ("Waiting to run"), not green — queued, not red. Per https://git.eeqj.de/sneak/webhooker/issues/119 the check mark is not evidence anyway; the authoritative gate was obtained locally: `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .` exit 0, lint `#20 DONE 68.8s` with `0 issues.`, builder `make test` `#31 DONE 94.0s`, zero `(cached)` markers, zero `FAIL`/`DATA RACE`, no BuildKit log clipping (1.88 MB). Host load 61-70 throughout. Image removed, `docker ps -a` clean, no prune run. - Of the 8 `CACHED` layers, three are inside the `builder` stage (`COPY --from=lint /src/go.sum`, the apt-get, `WORKDIR`) despite `--no-cache-filter=builder`. The two layers that matter — `golangci-lint run` and `make test` — both ran with real durations and full output. - Leak closure verified independently of https://git.eeqj.de/sneak/webhooker/pulls/223: this tree contains no `Omit(clause.Associations)` and no GORM callback registration, so the fix rests solely on never populating the association. Reintroducing `d.Target = *target` in `failUnretryableRetry` on the pushed tree makes `TestFailUnretryableRetry_WritesNoTargetRow` fail with the `INSERT INTO ` + "`targets`" + ` ... ON CONFLICT DO NOTHING` carrying the Slack URL; the pushed tree passes. If 223 were reverted, this path still writes no target row. - Pre-existing, NOT this PR: `internal/delivery/engine.go:447-448` (`processRetryTask`) still sets `d.Event` / `d.Target` from the task before the same `Update`, so the ordinary retry path does upsert a `targets` row into the per-webhook DB on `next` today. That is https://git.eeqj.de/sneak/webhooker/issues/206 itself, untouched by this change, and correctly out of scope here — noting it only so the green regression test is not read as proving the whole class is closed. - The decision that a breaker-blocked delivery is not an attempt is correct and it does not become invisible: `circuitBreakerBlock` persists `retrying`, which increments `webhooker_delivery_retries_total`, the sampler carries it in `webhooker_deliveries_retrying`, and the new `defer publishCircuitState` sets `webhooker_circuit_breakers_open`. Three series move; only `delivery_attempts_total` and the duration histogram, which measure dispatched traffic, correctly do not. - Deviation: the mutation probe and the `-count=2` run (`internal/metrics`, `internal/delivery`, `internal/middleware` — all `ok` under `-race`) were run as `go test` inside the pinned `golang:1.26.1-bookworm` container, since no make target runs a single test or `-count=2`. All linting was via the Docker gate only; nothing was run on the host.

clawbot merged commit 4cc83b2326 into next

2026-08-20 07:19:05 +02:00

clawbot deleted branch issue-209-delivery-metrics

2026-08-20 07:19:05 +02:00

clawbot referenced this issue from a commit

2026-08-20 07:19:05 +02:00

Expose delivery metrics on /metrics (closes #209) (#224)

clawbot referenced this pull request

2026-08-20 07:26:30 +02:00

Stop target credentials leaking into event databases (closes #206) #223

~~clawbot referenced this pull request 2026-08-20 07:38:15 +02:00~~

Add an egress CIDR allowlist to the SSRF guard (closes #204) #217