Skip to content

fix: avoid duplicate webhooks during concurrent automod events - #46

Merged
steipete merged 1 commit into
mainfrom
codex/triage-webhook-cache
Sep 16, 2026
Merged

steipete merged 1 commit into
mainfrom
codex/triage-webhook-cache

Conversation

@steipete

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes concurrent automod events creating duplicate webhooks when a channel's cache is empty.

User Impact

Events handled by the same Worker instance share one channel lookup and creation attempt. This avoids wasting Discord webhook slots during a burst. Failures remain retryable, and independent channels proceed independently. Separate Worker instances still have separate caches.

Why This Change Was Made

The cache previously became visible only after network requests completed, allowing every concurrent caller to start creation. Tracking the pending operation at the cache boundary closes that gap, while clearing it in finally preserves retries.

Evidence

A synthetic 20-call reproduction against the production helper issued 20 list calls and 20 create calls before the fix; afterward it issues one of each and returns the same webhook to every caller. Five of six regression tests failed before the fix; all six now pass, including lookup/create/missing-token failures, retry, existing webhook reuse, and channel independence. No live Discord webhook was created.

Independent P0–P2 review is scoped-clean. Worker and forwarder frozen installs/typechecks, the Worker dry-run build, and all 356 tests in the combined tree passed. GitHub CI validates the exact PR head before merge.

Share in-flight channel webhook lookup and creation within a Worker
instance, clearing failures for subsequent retries. Keep unrelated
channels independent and retain the existing successful cache TTL.
@clawsweeper

clawsweeper Bot commented Sep 16, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@steipete
steipete merged commit 0f3b5f8 into main Sep 16, 2026
6 checks passed
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 16, 2026
@steipete
steipete deleted the codex/triage-webhook-cache branch September 16, 2026 03:48
@clawsweeper

clawsweeper Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 15, 2026, 11:48 PM ET / September 16, 2026, 03:48 UTC.

ClawSweeper review

What this changes

Share pending channel webhook lookups and creation across concurrent automod events, with retry cleanup, regression tests, and documentation.

Merge readiness

Blocked before merge - 2 items remain

The fix remains necessary on current main and has no identified blocking code defect. Real transport behavior proof is still required before merge.

Priority: P2
Reviewed head: 577a794157984484b3ace24793bb785f2761c560

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The focused implementation and regression coverage are sound, but supplied proof stops at mocked transport.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The changed webhook helper is exercised by synthetic concurrent calls with mocked REST methods; no evidence shows the real Carbon transport performing shared lookup/creation and recovering after failure. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The changed webhook helper is exercised by synthetic concurrent calls with mocked REST methods; no evidence shows the real Carbon transport performing shared lookup/creation and recovering after failure. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 7 items Applicable repository policy: Read the complete root AGENTS.md and checked src/tests for nested instructions; none were found. The maintainer-notes directory is absent. Existing message construction and listener registration remain unchanged.
Current main still contains the race: The main helper populates its cache only after lookup and creation finish, allowing concurrent cache misses to start independent requests. GitHub's main endpoint confirmed the supplied base SHA remains current.
Bounded concurrency fix: The introduced operation is registered before another caller can proceed, successful results retain the existing cache, and finally removes settled attempts. The active automod caller uses the singleton bot client; no new principal, credential, persistence, or message authorization boundary was identified.
Findings None None.
Security None None.

How this fits together

Hermit handles Discord automoderation events in a Cloudflare Worker. Its webhook helper finds or creates a channel webhook, which the event handler uses to repost moderated content.

flowchart TD
  A[Automod events] --> B[Channel webhook helper]
  B --> C{Cached or pending?}
  C -->|Yes| D[Reuse webhook or await lookup]
  C -->|No| E[Discord lookup or creation]
  E --> F[Cache success and clear pending]
  D --> G[Repost moderated content]
  F --> G
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The changed webhook helper is exercised by synthetic concurrent calls with mocked REST methods; no evidence shows the real Carbon transport performing shared lookup/creation and recovering after failure. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Complete next step (P2) - Provide after-fix real transport proof of concurrent webhook reuse and failure recovery before merge; terminal output or logs are suitable, and screenshots or recordings are welcome when they show those observations. Redact credentials, private endpoints, and other private details. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth production +14 net lines; tests +72 lines Production growth implements shared pending requests and cleanup; six tests cover concurrency, reuse, failure retries, and channel independence.

Technical review

Best possible solution:

Keep deduplication at the existing channel cache boundary, preserving retry behavior and the documented Worker-instance scope.

Do we have a high-confidence way to reproduce the issue?

Yes, source establishes the race: simultaneous empty-cache callers can each finish an empty lookup and create a webhook before any result is cached. This review did not execute that path.

Is this the best way to solve the issue?

Yes, sharing the pending operation in the existing helper is a narrow solution that preserves successful caching and retries without adding configuration or distributed coordination.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 8c7c5e1931b0.

Labels

Label changes:

  • add P2: This repairs a bounded concurrency race that wastes Discord webhook slots during automod bursts.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed webhook helper is exercised by synthetic concurrent calls with mocked REST methods; no evidence shows the real Carbon transport performing shared lookup/creation and recovering after failure. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This repairs a bounded concurrency race that wastes Discord webhook slots during automod bursts.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed webhook helper is exercised by synthetic concurrent calls with mocked REST methods; no evidence shows the real Carbon transport performing shared lookup/creation and recovering after failure. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Applicable repository policy: Read the complete root AGENTS.md and checked src/tests for nested instructions; none were found. The maintainer-notes directory is absent. Existing message construction and listener registration remain unchanged. (AGENTS.md:1, 577a79415798)
  • Current main still contains the race: The main helper populates its cache only after lookup and creation finish, allowing concurrent cache misses to start independent requests. GitHub's main endpoint confirmed the supplied base SHA remains current. (src/utils/channelWebhook.ts:45, 8c7c5e1931b0)
  • Bounded concurrency fix: The introduced operation is registered before another caller can proceed, successful results retain the existing cache, and finally removes settled attempts. The active automod caller uses the singleton bot client; no new principal, credential, persistence, or message authorization boundary was identified. (src/utils/channelWebhook.ts:46, 577a79415798)
  • Production caller and dependency boundary: Automod invokes this helper before sending its repost. The helper directly uses Carbon REST methods and Webhook objects, establishing Carbon as the transport dependency; tests replace its REST methods with mocks. (src/events/autoModerationActionExecution.ts:104, 577a79415798)
  • Proof is synthetic: The complete supplied PR body reports a synthetic 20-call reproduction and explicitly says no live Discord webhook was created. All six added test cases use mocked REST methods; the reported test, typecheck, and build results are supplemental evidence, not an observed real transport run. No tests were executed during this read-only review. (tests/channelWebhook.test.ts:6, 577a79415798)
  • Historical routing: Blame associates the existing webhook request helpers with Shadow's earlier merged history; GitHub commit metadata maps that author to thewilloftheshadow. Raw commit inspection records a parent, so blame alone is not treated as introduction proof. Broader follow-history queries encountered unavailable objects. (src/utils/channelWebhook.ts:23, 8798c6d26c76)

Likely related people:

  • Shadow: Raw commit 8798c6d adds src/utils/channelWebhook.ts:22 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 8798c6d26c76; files: src/utils/channelWebhook.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add a redacted after-fix trace through the production helper and real Carbon transport showing one lookup/creation for concurrent callers and successful retry after a failed attempt.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant