refactor(discord): replace Discord.Net bot login with a Discord webhook by prpixie · Pull Request #428 · funfair-tech/BuildBot

prpixie · 2026-10-15T14:11:07Z

Summary

Discord.Net bot login/connection replaced by an HttpClient-based webhook sender (DiscordWebhookClient): a named client with transient retries and explicit 429 retry_after handling. Request logging is removed so the token is never logged.
BuildBot's own immutable embed model (DiscordEmbed, DiscordEmbedField, DiscordColor) replaces EmbedBuilder in the GitHub, CloudFormation and Watchtower publishers.
Release channel removed.
BotService renamed DiscordMessageSender, and its 1s Rx delay removed.
Config is Discord:WebhookUrl (DISCORD__WEBHOOKURL from Secrets Manager key DISCORD_WEBHOOK). Startup fails if the URL is missing or not https. Health reflects configuration only.
Embed URLs that are not http(s) are dropped.
src/publish updated.

Deployment

The DISCORD_WEBHOOK key must exist in the BuildBot secret before deploying. The old DISCORD_TOKEN, DISCORD_SERVER, DISCORD_CHANNEL and DISCORD_RELEASE_CHANNEL keys are unused.

Follow-up

When #426 merges, fold the temporary README ## Discord Configuration row into #426's ## Configuration table.

Closes #423

credfeto · 2026-10-15T14:16:34Z

Roslyn analyzer findings

Source	Rule	Level	File	Line	Suppressed	Message
DotNet	IL2026	error	src/BuildBot/Helpers/Endpoints.CloudFormation.cs	26	yes	Using member 'Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapPost(IEndpointRouteBuilder, String, Delegate)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. This API may perform reflection on the supplied delegate and its parameters. These types may be trimmed if not directly referenced.
DotNet	CA2000	error	src/BuildBot/Helpers/ServerStartup.cs	121	yes	Call System.IDisposable.Dispose on object created by 'CreateLogger()' before all references to it are out of scope
DotNet	IL2026	error	src/BuildBot/Helpers/Endpoints.Test.cs	22	yes	Using member 'Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapGet(IEndpointRouteBuilder, String, Delegate)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. This API may perform reflection on the supplied delegate and its parameters. These types may be trimmed if not directly referenced.
DotNet	IL2026	error	src/BuildBot/Helpers/Endpoints.Watchtower.cs	19	yes	Using member 'Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapPost(IEndpointRouteBuilder, String, Delegate)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. This API may perform reflection on the supplied delegate and its parameters. These types may be trimmed if not directly referenced.
DotNet	IL2026	error	src/BuildBot/Helpers/Endpoints.GitHub.cs	34	yes	Using member 'Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapPost(IEndpointRouteBuilder, String, Delegate)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. This API may perform reflection on the supplied delegate and its parameters. These types may be trimmed if not directly referenced.
DotNet	IL2026	error	src/BuildBot/Helpers/Endpoints.GitHub.cs	44	yes	Using member 'Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapPost(IEndpointRouteBuilder, String, Delegate)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. This API may perform reflection on the supplied delegate and its parameters. These types may be trimmed if not directly referenced.
DotNet	IL2026	error	src/BuildBot/Helpers/Endpoints.GitHub.cs	54	yes	Using member 'Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions.MapPost(IEndpointRouteBuilder, String, Delegate)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. This API may perform reflection on the supplied delegate and its parameters. These types may be trimmed if not directly referenced.
DotNet	IL2026	error	src/BuildBot/obj/Release/net11.0/Microsoft.AspNetCore.Http.RequestDelegateGenerator/Microsoft.AspNetCore.Http.RequestDelegateGenerator.RequestDelegateGenerator/GeneratedRouteBuilderExtensions.g.cs	1014	yes	Using member 'Microsoft.AspNetCore.Http.HttpResponseJsonExtensions.WriteAsJsonAsync(HttpResponse, TValue, JsonSerializerOptions, CancellationToken)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved.

credfeto · 2026-10-15T14:30:43Z

IDE MCP code analysis (Rider) was unavailable for the #423 implementation pass: Rider is connected but the BuildBot window has no solution loaded (only "Miscellaneous Files"), so every file reports "not included in any project of the current solution". The full solution build with warnings as errors (all analyzers) passed instead.

credfeto · 2026-10-15T16:25:47Z

Owner decisions from live chat:

q9 i guess q10 DiscordMessageSender

Q9: remove the 1-second InterMessageDelay from BotService. Rx .Delay only adds 1s of latency per message and does not space messages; Discord rate limiting is handled by the webhook client's 429 retry_after retry. This also removes the fixed real-time waits in BotServiceTests, replacing the earlier Q8 idea of adding a clock.
Q10: rename BotService to DiscordMessageSender.

Both will be done as separate changes on this PR after the current review round.

Owner request Q9 on PR #428. The Rx .Delay(InterMessageDelay) only shifted each message by 1s; it did not space messages. Concat still sends one at a time in order, and Discord rate limiting is handled by DiscordWebhookClient's 429 retry_after retry. BotServiceTests now wait on TaskCompletionSource signals bounded by WaitAsync timeouts instead of fixed 3s sleeps. Pattern Sweep: searched src for Rx .Delay, Task.Delay, Thread.Sleep, Observable.Timer/Interval, Throttle, Sample, SpinWait. Only remaining hit is DiscordWebhookClient's 429 back-off via the injected TimeProvider (different construct, required). Construct: Rx .Delay / fixed sleeps used as inter-message spacing Swept: none Refs #423.

Owner request Q10 on PR #428. With no bot connection left, the hosted service only drains the message queue and sends each message through the webhook client, so it is renamed to DiscordMessageSender (file, tests, DI registration, DI test name and a comment). DiscordBot, IDiscordBot and BotMessage are unchanged. Pattern Sweep: grep for BotService across the repo (excluding bin/obj/.git/CHANGELOG.md) finds no remaining references. Refs #423.

credfeto · 2026-10-15T16:44:16Z

Re the Q9/Q10 owner decisions (#428 (comment)):

Q9 done in f3fe620: removed the Rx 1s InterMessageDelay; BotServiceTests now wait on completion signals with bounded timeouts instead of fixed 3s sleeps.
Q10 done in 1bf8e8b: BotService renamed to DiscordMessageSender (file, tests, DI registration).

credfeto · 2026-10-15T17:29:08Z

Owner decision from live chat on simplify suggestion A (Q11):

q11 is that using IHttpClientFactory? if so you just need to define a policy

It does use IHttpClientFactory (named client in DiscordSetup), so Q11: yes. Discord 429 handling moves into the named client's Polly policy (transient errors plus 429, waiting on the Retry-After header, capped, falling back to the existing backoff), and DiscordWebhookClient.SendAsync becomes a single POST. The hand-written retry loop, RateLimitResponse and the rate-limit logging go. This will be done as its own change.

credfeto · 2026-10-15T17:29:30Z

Owner decision from live chat on simplify suggestion 2:

Q12 yes

Q12: DiscordMessageSender will be rewritten as a BackgroundService that await foreaches over the message queue and catches and logs each failure, so one exception can no longer end the pipeline and nothing runs in the constructor. Done as its own change after the Q11 Polly change.

credfeto · 2026-10-15T17:40:34Z

Owner direction from live chat:

we should raie that as a follow on issue after this merges

Q14: once this PR merges, raise a follow-up issue to enable PublishTrimmed / PublishAot now that Discord.Net (and its Newtonsoft.Json dependency) is gone. Starting point: a trial dotnet publish -r linux-x64 -p:PublishTrimmed=true then -p:PublishAot=true to list the remaining warnings. Likely items: Serilog.Enrichers.Demystifier, Polly v7 (Microsoft.Extensions.Http.Polly, possibly Microsoft.Extensions.Http.Resilience instead, which needs package sign-off), confirming AWSSDK.CloudFormation 4.x, whether System.Reactive is still needed after the Q12 change, and a runtime-deps base image in the Dockerfile.

credfeto · 2026-10-15T17:41:36Z

Correction to my previous comment: the owner clarified in live chat

that was "for after" not wait until it merges to raise

so the AOT/trimming follow-up is raised now as #430, to be worked after this PR merges.

Owner decision Q11 on PR #428. The named IHttpClientFactory client's Polly policy now handles transient errors plus 429 Too Many Requests, waiting on the Retry-After header (delta or date, capped at 60s) and otherwise the existing exponential backoff. DiscordWebhookClient.SendAsync is a single POST; the hand-written retry loop, RateLimitResponse and the rate-limit log message are removed. Pattern Sweep: searched production src for hand-written HTTP retry loops (while(true), attempt counters, Task.Delay, Retry-After, TooManyRequests, AddHttpClient/CreateClient/new HttpClient, WaitAndRetry). The only other HTTP client (CloudFormation subscription confirmation) already uses a Polly policy; nothing else to change. Refs #423.

Owner request Q9 on PR #428. The Rx .Delay(InterMessageDelay) only shifted each message by 1s; it did not space messages. Concat still sends one at a time in order, and Discord rate limiting is handled by DiscordWebhookClient's 429 retry_after retry. BotServiceTests now wait on TaskCompletionSource signals bounded by WaitAsync timeouts instead of fixed 3s sleeps. Pattern Sweep: searched src for Rx .Delay, Task.Delay, Thread.Sleep, Observable.Timer/Interval, Throttle, Sample, SpinWait. Only remaining hit is DiscordWebhookClient's 429 back-off via the injected TimeProvider (different construct, required). Construct: Rx .Delay / fixed sleeps used as inter-message spacing Swept: none Refs #423.

Owner request Q10 on PR #428. With no bot connection left, the hosted service only drains the message queue and sends each message through the webhook client, so it is renamed to DiscordMessageSender (file, tests, DI registration, DI test name and a comment). DiscordBot, IDiscordBot and BotMessage are unchanged. Pattern Sweep: grep for BotService across the repo (excluding bin/obj/.git/CHANGELOG.md) finds no remaining references. Refs #423.

Owner decision Q11 on PR #428. The named IHttpClientFactory client's Polly policy now handles transient errors plus 429 Too Many Requests, waiting on the Retry-After header (delta or date, capped at 60s) and otherwise the existing exponential backoff. DiscordWebhookClient.SendAsync is a single POST; the hand-written retry loop, RateLimitResponse and the rate-limit log message are removed. Pattern Sweep: searched production src for hand-written HTTP retry loops (while(true), attempt counters, Task.Delay, Retry-After, TooManyRequests, AddHttpClient/CreateClient/new HttpClient, WaitAndRetry). The only other HTTP client (CloudFormation subscription confirmation) already uses a Polly policy; nothing else to change. Refs #423.

credfeto · 2026-10-15T18:09:14Z

CI Debugger: re-run started for build-pre-release

Owner decision Q12 on PR #428. The sender now awaits the message channel in ExecuteAsync and sends each message in order, logging (never the webhook URL) and continuing when a send fails, so one exception can no longer end delivery; cancellation of the stopping token ends the loop cleanly. The Rx pipeline, constructor-started work and manual cancellation/dispose are removed. Pattern Sweep: searched src for Rx subscribe chains and constructor-started or unawaited background work (Observable/Subscribe/ToObservable, IHostedService/BackgroundService, Task.Run, timers, ContinueWith, Task.Factory, ThreadPool); DiscordMessageSender was the only hit. Refs #423.

Owner request Q9 on PR #428. The Rx .Delay(InterMessageDelay) only shifted each message by 1s; it did not space messages. Concat still sends one at a time in order, and Discord rate limiting is handled by DiscordWebhookClient's 429 retry_after retry. BotServiceTests now wait on TaskCompletionSource signals bounded by WaitAsync timeouts instead of fixed 3s sleeps. Pattern Sweep: searched src for Rx .Delay, Task.Delay, Thread.Sleep, Observable.Timer/Interval, Throttle, Sample, SpinWait. Only remaining hit is DiscordWebhookClient's 429 back-off via the injected TimeProvider (different construct, required). Construct: Rx .Delay / fixed sleeps used as inter-message spacing Swept: none Refs #423.

Owner request Q10 on PR #428. With no bot connection left, the hosted service only drains the message queue and sends each message through the webhook client, so it is renamed to DiscordMessageSender (file, tests, DI registration, DI test name and a comment). DiscordBot, IDiscordBot and BotMessage are unchanged. Pattern Sweep: grep for BotService across the repo (excluding bin/obj/.git/CHANGELOG.md) finds no remaining references. Refs #423.

Owner decision Q11 on PR #428. The named IHttpClientFactory client's Polly policy now handles transient errors plus 429 Too Many Requests, waiting on the Retry-After header (delta or date, capped at 60s) and otherwise the existing exponential backoff. DiscordWebhookClient.SendAsync is a single POST; the hand-written retry loop, RateLimitResponse and the rate-limit log message are removed. Pattern Sweep: searched production src for hand-written HTTP retry loops (while(true), attempt counters, Task.Delay, Retry-After, TooManyRequests, AddHttpClient/CreateClient/new HttpClient, WaitAndRetry). The only other HTTP client (CloudFormation subscription confirmation) already uses a Polly policy; nothing else to change. Refs #423.

…ettings Refs #423

Rename DiscordColour to DiscordColor, DiscordEmbed.Colour to Color and restore GithubStatusNotificationHandler.GetEmbedColor, per the cs-template Code Identifiers rule. Refs #423

Pattern Sweep for 97c618a (UK-spelled code identifiers for the Discord color concept, against the language rule that identifiers follow platform convention). Renamed the pre-existing ColourMapping record struct, its Colour member and the ColourMappings field to ColorMapping / Color / ColorMappings. Searched the repo for Colour excluding .git/obj/bin/CHANGELOG.md; no Colour identifiers remain in src. Construct: UK-spelled Colour identifiers for the Discord color concept src/BuildBot.GitHub/Publishers/GithubStatusNotificationHandler.cs: declares the ColourMapping record struct, its Colour member and the ColourMappings field. Refs #423.

Owner request Q9 on PR #428. The Rx .Delay(InterMessageDelay) only shifted each message by 1s; it did not space messages. Concat still sends one at a time in order, and Discord rate limiting is handled by DiscordWebhookClient's 429 retry_after retry. BotServiceTests now wait on TaskCompletionSource signals bounded by WaitAsync timeouts instead of fixed 3s sleeps. Pattern Sweep: searched src for Rx .Delay, Task.Delay, Thread.Sleep, Observable.Timer/Interval, Throttle, Sample, SpinWait. Only remaining hit is DiscordWebhookClient's 429 back-off via the injected TimeProvider (different construct, required). Construct: Rx .Delay / fixed sleeps used as inter-message spacing Swept: none Refs #423.

Owner request Q10 on PR #428. With no bot connection left, the hosted service only drains the message queue and sends each message through the webhook client, so it is renamed to DiscordMessageSender (file, tests, DI registration, DI test name and a comment). DiscordBot, IDiscordBot and BotMessage are unchanged. Pattern Sweep: grep for BotService across the repo (excluding bin/obj/.git/CHANGELOG.md) finds no remaining references. Refs #423.

Replaces the placeholder with Changed, Removed and Deployment Changes entries for the Discord.Net to webhook refactor. Refs #423.

Simplify (AI review loop Phase A). RateLimitResponse.Global was deserialised from Discord's 429 body but never read; the serialiser skips the unmapped "global" key, so behaviour is unchanged. Pattern Sweep: RateLimitResponse is the only response-deserialised type in the PR; the Webhook* records are outbound payloads. Nothing else to change. Refs #423.

Simplify (AI review loop Phase A). Startup already fails unless Discord:WebhookUrl is an absolute https URL, and FromWebhookUrl is now the only way to build DiscordBotConfiguration (private constructor), so IsConfigured, the PublishAsync not-configured guard, LogWebhookNotConfigured and the Yes/No startup line could never vary. Health reports Discord Ok. Pattern Sweep: construct = Discord configuration checks always true after FromWebhookUrl validation. Removed the redundant IsAbsoluteUri check in the same file; no other hits in BuildBot.Discord or ServerStartup.LoadDiscordConfig. EmbedUrl's scheme check validates embed URLs (different construct). Refs #423.

Health no longer varies with configuration; the entry now states only that startup fails on a missing or non-https webhook URL. Refs #423.

Simplify (AI review loop Phase A). Both arms of the GetFields ternary rebuilt the ARN/Status field list; it is now built once and the Version field appended when present. Output unchanged. Pattern Sweep: searched src/**/*.cs for other ternaries whose arms are both collection expressions; none found. Refs #423.

Owner decision Q11 on PR #428. The named IHttpClientFactory client's Polly policy now handles transient errors plus 429 Too Many Requests, waiting on the Retry-After header (delta or date, capped at 60s) and otherwise the existing exponential backoff. DiscordWebhookClient.SendAsync is a single POST; the hand-written retry loop, RateLimitResponse and the rate-limit log message are removed. Pattern Sweep: searched production src for hand-written HTTP retry loops (while(true), attempt counters, Task.Delay, Retry-After, TooManyRequests, AddHttpClient/CreateClient/new HttpClient, WaitAndRetry). The only other HTTP client (CloudFormation subscription confirmation) already uses a Polly policy; nothing else to change. Refs #423.

Now that #426 added the README Configuration table, the Discord:WebhookUrl / DISCORD__WEBHOOKURL row moves into it and the temporary Discord Configuration section is removed. Refs #423.

Owner decision Q12 on PR #428. The sender now awaits the message channel in ExecuteAsync and sends each message in order, logging (never the webhook URL) and continuing when a send fails, so one exception can no longer end delivery; cancellation of the stopping token ends the loop cleanly. The Rx pipeline, constructor-started work and manual cancellation/dispose are removed. Pattern Sweep: searched src for Rx subscribe chains and constructor-started or unawaited background work (Observable/Subscribe/ToObservable, IHostedService/BackgroundService, Task.Run, timers, ContinueWith, Task.Factory, ThreadPool); DiscordMessageSender was the only hit. Refs #423.

Simplify (AI review loop Phase A). The http(s) scheme check on embed URLs ran per producer through EmbedUrl.FromString, and CloudFormation's Uri skipped it. EmbedMapper now drops any URL that is not an absolute http or https URL when mapping to the wire model, so Discord never rejects an embed over its URL; EmbedUrl only parses a string safely. Pattern Sweep: searched src for UriScheme, .Scheme, IsAbsoluteUri, UriKind and every EmbedUrl caller. GitHub push/status and CloudFormation producers are now covered centrally with no edits; DiscordBotConfiguration's https check validates the webhook endpoint (different construct). Refs #423.

Simplify (AI review loop Phase A). "Discord Webhook configured" always printed because DiscordBotConfiguration.FromWebhookUrl throws otherwise, so it carried no information. The GitHub and SNS config blocks print nothing at startup either. Pattern Sweep: the PR diff adds no other always-identical startup console line; existing progress markers on main (App Created, Endpoints configured, Configuring ... Endpoint, Validating cultures) are a different construct. Refs #423.

Simplify (AI review loop Phase A). Every CreateBot call passed the static Config, so the parameter is removed. Pattern Sweep: CreateBot exists only in DiscordBotTests; no other test helper takes an always-identical argument. Refs #423.

Simplify (AI review loop Phase A). Send failures were caught twice: in DiscordBot.PublishAsync and again in the DiscordMessageSender loop, whose catch could never run. DiscordBot now lets failures propagate and the sender loop, its only caller, logs and continues; the unused LogSendFailed/LogSendCancelled messages are removed. A send cancelled at shutdown now simply ends the loop without a warning. Construct: duplicate catch-all around DiscordBot.PublishAsync. Pattern Sweep: searched src for catch (Exception; CloudFormationSubscriptionConfirmationNotificationHandler and AwsCloudFormation each catch once with no outer catch, and Program.cs is the top-level host handler. No other double guards. Refs #423.

Simplify (AI review loop Phase A). The public OnRetryAsync method only returned a completed task to fill Polly's onRetryAsync argument; it is now an inline static lambda. Behaviour unchanged. Pattern Sweep: no other no-op public callbacks in the PR. CloudformationSetup.OnRetryAsync on main is the same construct but outside this PR (and has its own test), so it is not changed here. Refs #423.

EmbedLimiter truncates the embed title to 256 characters, the description to 4096, field names to 256 and field values to 1024. Fields are capped at 25 and the total embed length is kept at or under 6000 characters by shrinking the description first, then field values from the last field, then dropping trailing fields. Truncation appends "..." and is surrogate-safe, so no surrogate pair is split. Discord therefore no longer rejects oversized notifications. Push notifications use only the first line of each commit message as the field name. Construct: building Discord embeds without length limits Swept: all embeds go through EmbedMapper.ToWebhookEmbed, so the single choke point covers every publisher (GitHub, CloudFormation, Watchtower); no other occurrences. Refs #423

Refs #423

Retry-After (seconds or date) is now used for every retried response (5xx, 408, 429), not only 429. A negative or past value means retry immediately. A Retry-After over 60s is no longer clamped: the retry is refused and the response returned, so a long rate-limit ban is not hammered. The retry predicate is built by hand (HttpRequestException, 5xx, 408, 429) because HandleTransientHttpError cannot refuse a retry. The named webhook client's HttpClient.Timeout is set to 300s (3 waits of up to 60s plus 4 attempts of 30s) so the retries can complete before the client times out. Tests use MockDateTimeSources in place of a hardcoded date. Construct: Polly retry delay/budget on an IHttpClientFactory named client Swept: none Searched: WaitAndRetry|Retry-After|RetryAfter|AddPolicyHandler|AddTransientHttpErrorPolicy|HandleTransientHttpError|.Timeout|AddHttpClient( in production src. Hits: src/BuildBot.Discord/DiscordSetup.cs (this fix). Referred: src/BuildBot.CloudFormation/CloudformationSetup.cs has the same construct but is outside this change and is referred to the owner (Q22); not fixed here. Refs #423

…cted On a non-success response the webhook client reads at most 1024 characters of the body and LogSendRejected includes it, so Discord's validation message (e.g. which embed field is too long) appears in the logs. The request is sent with HttpCompletionOption.ResponseHeadersRead so an error body is never fully buffered, and the body read is bounded by a 10s TimeProvider timeout because HttpClient.Timeout no longer applies once headers arrive. If the body read fails or times out the status is still reported with "(response body unavailable)"; caller cancellation still propagates. The webhook URL/token is never logged. Pattern Sweep record: Construct: non-success HTTP response body discarded before the failure is logged. Searched IsSuccessStatusCode|EnsureSuccessStatusCode|PostAsync|GetAsync|SendAsync(|ReadAsStringAsync|HttpClient in production src. Hits: DiscordWebhookClient.cs (this fix); src/BuildBot.CloudFormation/Publishers/CloudFormationSubscriptionConfirmationNotificationHandler.cs is the same construct, outside this PR's changes, referred to the owner (Q22), not fixed here. Refs #423

…essage Discord rejects the whole webhook message with 400 when a field name or value is empty (Discord.Net's EmbedFieldBuilder used to reject this before sending). EmbedMapper now replaces an empty or whitespace-only field name or value with "-" and omits a blank title or description, so one blank value (e.g. an empty CloudFormation status or branch name) no longer loses the notification. Construct: blank Discord embed text sent instead of being omitted or replaced. Swept: none Refs #423

Each webhook attempt now has its own 30s Polly timeout inside the retry policy, and a timed-out attempt (TimeoutRejectedException) is retried. A connection that stalls before sending headers no longer uses up the whole 300s HttpClient budget and drops the message while every queued message waits behind it. Caller cancellation still propagates and is not retried. The policy is built by CreateWebhookPolicy so tests can run the exact registered composition with a short attempt timeout and zero backoff, because Polly v7 timeouts and sleeps use its own clock, not TimeProvider. The backoff delegate is held in a static field, avoiding a compiler-cached method-group branch. Construct: Polly HTTP retry policy without a per-attempt timeout - src/BuildBot.Discord/DiscordSetup.cs: the webhook retry policy this fix changes (this fix) - src/BuildBot.Discord.Tests/DiscordSetupTests.cs: tests for the per-attempt timeout and retry composition (this fix) Sweep note: CloudformationSetup.cs SNS client is left as is by owner decision (Q22 on PR #428). Refs #423

The webhook retry policy's onRetry was a no-op, so rate-limit storms and slow delivery left no trace before the final failure. Each retry now logs a Warning with the attempt number, the delay and the reason: the HTTP status code, or the exception type for HttpRequestException/TimeoutRejectedException. Exception messages and the request URL are never logged, because they can contain the webhook token. Construct: Polly retry policy whose onRetry does nothing. Refs #423

IsTransientFailure treated every 5xx as transient, so permanent failures such as 501 Not Implemented and 505 were retried 3 times with backoff while the sequential sender waited. The webhook now retries only 408, 429, 500, 502, 503 and 504 (plus connection failures and per-attempt timeouts); the Retry-After over 60s refusal is unchanged. Construct: HTTP retry rule treating every 5xx as transient Swept: searched >= HttpStatusCode|InternalServerError|TooManyRequests|RequestTimeout|Transient|WaitAndRetry|Handle<HttpRequestException|OrResult in src - src/BuildBot.Discord/DiscordSetup.cs: this fix (IsTransientFailure status code rule) - src/BuildBot.Discord.Tests/DiscordSetupTests.cs: tests for the transient status code rule - CloudformationSetup.cs (AddTransientHttpErrorPolicy): left as is by owner decision (Q22 on PR #428) Refs #423

…dget WebhookTimeout equalled the worst-case retry budget exactly (4 x 30s attempts + 3 x 60s Retry-After = 300s), so scheduling and logging overhead could let HttpClient.Timeout cancel the final attempt and lose the message. A 30s slack is added (330s), and the test now asserts that the timeout is strictly greater than the worst-case budget instead of checking a literal value. Construct: overall HttpClient.Timeout set to exactly the retry budget. Searched `Timeout =` in src; only DiscordSetup.cs (this fix). CloudFormation is left as is by owner decision (Q22 on PR #428). Refs #423

The final-failure log in DiscordMessageSender passed the exception object to the logger. An HttpRequestException message can include the webhook URL, which embeds its token, so any sink that rendered the exception could leak it. Log only the exception type name, as the retry logger already does (EventIds 8/9). EventId 7 and the Error level are unchanged. Construct: LoggerMessage method taking an Exception whose message can carry the webhook URL; searched Discord path log calls in src, no further hits Refs #423

credfeto · 2026-10-16T18:11:50Z

AI Review Loop: Phase B (code review) not converging

Round 3 mostly re-raised findings from earlier rounds. Both new findings are now fixed:

Webhook timeout headroom over the retry budget: 86fea97.
Final-failure log writes only the exception type: 21531fa.

These findings are still open and go forward for human review. Each one has a reply on its own inline comment.

Health reflects configuration only, not delivery. Kept as decided in Q1.
No idempotency or de-duplication of webhook sends. Declined: the queue is in-process, so at-least-once delivery on a retry is acceptable for build notifications.
CloudFormation SNS handling duplication and logging of the subscribe URL. Left as is, as decided in Q22.
Splitting the webhook URL into id and token config. Declined: the issue specifies a single DISCORD_WEBHOOK secret.
Pacing between sends. Declined, as decided in Q23. Discord 429s are covered by the Retry-After handling.
Dockerfile ICU / globalization. Deferred to build(aot): enable PublishTrimmed and PublishAot now Discord.Net is gone #430 (AOT/trimming).
Status notification title vs Branch field mismatch. Pre-existing, tracked in fix(github): Status notifications use different branches in title and Branch field #434.

Moving on to Phase C (security review).

prpixie added the auto-pr Pull request created automatically label Oct 15, 2026

prpixie assigned credfeto Oct 15, 2026

credfeto added the AI-Work Work for an AI Agent label Oct 15, 2026

credfeto changed the title ~~chore(changelog): add placeholder entry for #423~~ refactor(discord): replace Discord.Net bot login with a Discord webhook Oct 15, 2026

credfeto added the Security Security issue, e.g. use of insecure packages, or security fix label Oct 15, 2026

credfeto mentioned this pull request Oct 15, 2026

docs(dotnet): require IHttpClientFactory for HttpClient in production code credfeto/cs-template#1165

Open

This was referenced Oct 15, 2026

build(aot): enable PublishTrimmed and PublishAot now Discord.Net is gone #430

Open

Move to the internal load balancer behind an API Gateway #421

Closed

credfeto force-pushed the refactor/423-discord-webhook branch from 63f6664 to c2e6699 Compare October 15, 2026 17:53

credfeto mentioned this pull request Oct 15, 2026

docs(readme): bring README in line with the readme documentation rule #432

Open

credfeto force-pushed the refactor/423-discord-webhook branch from 9635ade to 8950a0b Compare October 15, 2026 18:33

credfeto added 27 commits October 16, 2026 18:58

fix(publish): pass Discord webhook URL instead of removed bot token s…

ef39f7a

…ettings Refs #423

refactor(discord): use platform US spelling for colour identifiers

9f4f73a

Rename DiscordColour to DiscordColor, DiscordEmbed.Colour to Color and restore GithubStatusNotificationHandler.GetEmbedColor, per the cs-template Code Identifiers rule. Refs #423

docs(changelog): describe Discord webhook replacement for #423

26d831f

Replaces the placeholder with Changed, Removed and Deployment Changes entries for the Discord.Net to webhook refactor. Refs #423.

docs(changelog): correct Discord startup/health entry for #423

ec102c9

Health no longer varies with configuration; the entry now states only that startup fails on a missing or non-https webhook URL. Refs #423.

docs(readme): move Discord webhook setting into the Configuration table

fd897bc

Now that #426 added the README Configuration table, the Discord:WebhookUrl / DISCORD__WEBHOOKURL row moves into it and the temporary Discord Configuration section is removed. Refs #423.

docs(changelog): note Discord embed limits and first-line commit titles

1ddbc55

Refs #423

credfeto force-pushed the refactor/423-discord-webhook branch from 86fea97 to 28145f8 Compare October 16, 2026 17:59

Navigation Menu

Repository navigation

refactor(discord): replace Discord.Net bot login with a Discord webhook - #428

refactor(discord): replace Discord.Net bot login with a Discord webhook#428
prpixie wants to merge 38 commits into
mainfrom
refactor/423-discord-webhook

prpixie commented Oct 15, 2026 •

edited by credfeto

Loading

Uh oh!

credfeto commented Oct 15, 2026 •

edited

Loading

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 16, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

prpixie commented Oct 15, 2026 • edited by credfeto Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Deployment

Follow-up

Uh oh!

credfeto commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Roslyn analyzer findings

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

Uh oh!

credfeto commented Oct 15, 2026

CI Debugger: re-run started for build-pre-release

Uh oh!

credfeto commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review Loop: Phase B (code review) not converging

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

prpixie commented Oct 15, 2026 •

edited by credfeto

Loading

credfeto commented Oct 15, 2026 •

edited

Loading

credfeto commented Oct 16, 2026 •

edited

Loading