fix(selfhost): record an error metric when the webhook-dedup Redis cache fails#8415
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
…che fails
isWebhookDeliveryDuplicate and rememberWebhookDelivery swallowed Redis errors
silently, so a sustained outage affecting webhook dedup was invisible in
metrics -- unlike the identical failure mode in redis-token-cache.ts and
redis-response-cache.ts, which both record an error result.
Adds a named recordWebhookDedupCacheMetric recorder over a new
loopover_redis_webhook_dedup_cache_total{result} counter, mirroring both
siblings, and registers it in the metric meta map beside them. Kept separate
from loopover_webhook_dedup_total{backend="redis"}, which counts actual dedup
hits. Fail-open behavior is unchanged.
Closes JSONbored#8363
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8415 +/- ##
==========================================
+ Coverage 89.58% 89.63% +0.05%
==========================================
Files 97 99 +2
Lines 22706 22823 +117
Branches 3872 3888 +16
==========================================
+ Hits 20341 20458 +117
Misses 2187 2187
Partials 178 178
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-24 12:51:10 UTC
Review summary Nits — 2 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Summary
isWebhookDeliveryDuplicateandrememberWebhookDelivery(src/selfhost/redis-cache.ts) wrapped their Redis calls in a catch that swallowed the error with no metric, so a sustained Redis outage affecting webhook dedup was completely invisible in metrics — unlike the identical failure mode in this file's two siblings, which both record an"error"result (redis-token-cache.ts'srecordTokenCacheMetric,redis-response-cache.ts'srecordRedisResponseCacheMetric).recordWebhookDedupCacheMetric(result)recorder over a newloopover_redis_webhook_dedup_cache_total{result}counter — the same named-recorder-over-a-{result}-labeled-counter shape both siblings use — and calls it from both catch blocks. Registers the counter inmetrics.ts's meta map directly besideloopover_redis_token_cache_total/loopover_redis_gh_response_cache_total, matching their naming and help-text convention.isWebhookDeliveryDuplicatestill returnsfalseandrememberWebhookDeliveryis still a no-op on error. This issue is scoped to observability only.loopover_webhook_dedup_total{backend="redis"}, which counts actual dedup hits (and is pre-seeded to 0 inserver.ts) — folding errors into it would conflate "deliveries deduplicated" with "cache unavailable" and corrupt an existing dashboard series.redis-token-cache.tsandredis-response-cache.tsare untouched — they are the reference pattern."returns false without incrementing when Redis get throws") to assert the new error metric alongside its existing fail-open assertion, and adds the matchingrememberWebhookDeliveryset-throws case, which had no error-path test at all.Closes #8363
Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
src/selfhost/redis-cache.ts, one meta-map entry insrc/selfhost/metrics.ts, andtest/unit/selfhost-redis-cache.test.ts, soactionlint(no workflow change),build:mcp/test:mcp-pack(no MCP change),ui:*(no UI/OpenAPI change),test:workers(no worker change), andnpm audit(no dependency change) are not exercised by it.vitest run --coverage --coverage.all=false): every executable changed line inredis-cache.ts(the recorder and both catch-block calls) is hit by the two error-path tests; themetrics.tschange is a single meta-map literal entry with no separately-instrumented executable line.test/unit/selfhost-redis-cache.test.ts(14 tests) andtest/unit/selfhost-metrics.test.tspass in full (52 together),test/unit/alerts-metric-name-references.test.tsstill passes with the new metric registered, and roottsc --noEmitis clean.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.UI Evidence
Not applicable — backend observability fix in
src/selfhost/; no visible UI, frontend, docs, or extension change.Notes
loopover_redis_<cache>_totalshape rather than extending the existing dedup counter, so the new series is additive: no existing metric, label set, or dashboard query changes.