Repository navigation
Fix duplicate LoggerMessage event ids; add a guard test - #3266
Merged
Merged
Conversation
Three source-generated log events shared an EventId with another event: 116 and 117 (maintenance notifications vs. slot-map/undialled-server events, landed a day apart on parallel branches) and 99 (the two TLS "connection established" overloads, with different templates). LogErrorConnectionFailed had no explicit id at all and was generated as 0. Renumber the newer side of each pair (124, 125, 126) and give the connection-failed event 127. Add LoggerEventIdTests, which reflects over LoggerExtensions and fails if any [LoggerMessage] lacks an explicit EventId or if any id is used twice, reporting every problem at once and logging the next free id.
mgravell
added a commit
that referenced
this pull request
Oct 9, 2026
Absorbs the drift up to the 3.4.0 cut. Most of main's changes landed in v3's PhysicalBridge/PhysicalConnection/CursorEnumerable, which v4 has deleted, so several are re-expressed in v4 terms rather than merged textually: - #3266 duplicate LoggerMessage event ids: taken as-is. 124/125 (maintenance), 126 (TLS established) and 127 (ConnectionFailed) shipped in 3.4.0, so the v4-only events that had claimed those ids move to 128-131; the guard test (LoggerEventIdTests) covers both. - #3268 create-implementation-plan skill: taken as-is. - #3260/#3267 scan faults after a sync timeout or dispose: the fix targets CursorEnumerable's page prefetch, which v4 does not have (no prefetch; the sync face uses the synchronous Send). The intent is kept as tests on v4's scans: a faulted page surfaces as its own exception, sync and async, and nothing is fetched once the enumeration is let go. - #3257 ConnectionAttemptCompleted: v4 has no PhysicalConnection until an attempt succeeds, so ConnectionAttempt carries the record through the transport factory and the handshake instead - stages Connect, Tunnel, Tls, Handshake, Established; client/server certificate observation and the TLS host; one outcome per attempt. Created only when someone subscribes. Abandoned (executor disposal) reports ConnectionDisposed; outliving the connect timeout reports UnableToConnect with the timeout. A refused AUTH is reported as Handshake + AuthenticationFailure although v4 keeps the connection (parity with v3's fire-and-forget AUTH). - #3264 dedicated pub/sub connection under RESP3 by default: v4's "RESP3, so share" becomes SharesSubscriptions (RESP3 *and* SharedSubscriptionConnection). v4 keeps pay-per-play dialling: the subscription socket is dialled up front only for the configuration channel, as under RESP2. The dedicated subscription connection now raises ConnectionRestored (Subscription) when it (re)connects, as v3's subscription bridge did - previously only reachable under RESP2, and silent there too (Issue922_ReconnectRaised). It does not trigger a reconfigure: topology is read over the interactive leg, and the subscription leg connects just after the initial connect. - 3.4 bump: version.json stays 4.0; the 3.4.0 API surface is reconciled into PublicAPI.Shipped.txt. Also fixed while here: the auth-refusal callback added for #3257 was a lambda capturing `this`, which landed in the closure scope OnPush chains to and let the socket root the multiplexer (GarbageCollectionTests.MuxerIsCollected). Closes ledger rows D6 and D7.
8 of 37 tasks
FloatingMilkshake
pushed a commit
to FloatingMilkshake/MechanicalMilkshake
that referenced
this pull request
Oct 9, 2026
![Renovate [bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [StackExchange.Redis](https://seredis.dev/) ([source](https://github.com/StackExchange/StackExchange.Redis)) | nuget | minor | `3.3.1` → `3.4.0` | --- ### Release Notes <details> <summary>StackExchange/StackExchange.Redis (StackExchange.Redis)</summary> ### [`v3.4.0`](https://github.com/StackExchange/StackExchange.Redis/releases/tag/3.4.0) [Compare Source](StackExchange/StackExchange.Redis@3.3.1...3.4.0) #### Major Changes - In RESP3, by default the library now uses **separate** connections for pub/sub; see [#​3264](StackExchange/StackExchange.Redis#3264) - Smart-client-handoffs is now enabled by default on Redis Enterprise and AMR; see [#​3248](StackExchange/StackExchange.Redis#3248) - Some logging events incorrectly used duplicated ids - resolved; see [#​3266](StackExchange/StackExchange.Redis#3266) #### All Changes - Recover promptly from a cluster failover; fix the configuration channel under RESP3 and with a channel prefix ([#​3254](StackExchange/StackExchange.Redis#3254)) by [@​mgravell](https://github.com/mgravell) in [#​3255](StackExchange/StackExchange.Redis#3255) - Preserve binary members as VectorSetRange bounds by [@​Laurianti](https://github.com/Laurianti) in [#​3256](StackExchange/StackExchange.Redis#3256) - Smart client handoffs: reinstate auto-enlistment for the hosted providers and remove \[Experimental] by [@​mgravell](https://github.com/mgravell) in [#​3248](StackExchange/StackExchange.Redis#3248) - docs: fix retry-category test in the custom RetryPolicy sample by [@​mgravell](https://github.com/mgravell) in [#​3265](StackExchange/StackExchange.Redis#3265) - Fix duplicate LoggerMessage event ids; add a guard test by [@​mgravell](https://github.com/mgravell) in [#​3266](StackExchange/StackExchange.Redis#3266) - Scan: observe the faults of abandoned page tasks after a sync timeout or dispose ([#​3260](StackExchange/StackExchange.Redis#3260)) by [@​mgravell](https://github.com/mgravell) in [#​3267](StackExchange/StackExchange.Redis#3267) - Report the outcome of every physical connection attempt, identifying the client certificate used by [@​mgravell](https://github.com/mgravell) in [#​3257](StackExchange/StackExchange.Redis#3257) - Use a dedicated pub/sub connection under RESP3 by default by [@​mgravell](https://github.com/mgravell) in [#​3264](StackExchange/StackExchange.Redis#3264) #### Tooling - docs(agents): add create-implementation-plan-for-redis-api-change skill by [@​uglide](https://github.com/uglide) in [#​3268](StackExchange/StackExchange.Redis#3268) #### New Contributors - [@​Laurianti](https://github.com/Laurianti) made their first contribution in [#​3256](StackExchange/StackExchange.Redis#3256) **Full Changelog**: <StackExchange/StackExchange.Redis@3.3.1...3.4.0> </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4xNDkuMCIsInVwZGF0ZWRJblZlciI6IjQ0LjE0OS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Reviewed-on: https://git.milkshake.services/FloatingMilkshake/MechanicalMilkshake/pulls/9 Reviewed-by: Milkshake <floatingmilkshake@noreply.milkshake.services>
FloatingMilkshake
pushed a commit
to FloatingMilkshake/Azusa
that referenced
this pull request
Oct 9, 2026
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [StackExchange.Redis](https://seredis.dev/) ([source](https://github.com/StackExchange/StackExchange.Redis)) | nuget | minor | `3.3.1` → `3.4.0` | --- ### Release Notes <details> <summary>StackExchange/StackExchange.Redis (StackExchange.Redis)</summary> ### [`v3.4.0`](https://github.com/StackExchange/StackExchange.Redis/releases/tag/3.4.0) [Compare Source](StackExchange/StackExchange.Redis@3.3.1...3.4.0) #### Major Changes - In RESP3, by default the library now uses **separate** connections for pub/sub; see [#​3264](StackExchange/StackExchange.Redis#3264) - Smart-client-handoffs is now enabled by default on Redis Enterprise and AMR; see [#​3248](StackExchange/StackExchange.Redis#3248) - Some logging events incorrectly used duplicated ids - resolved; see [#​3266](StackExchange/StackExchange.Redis#3266) #### All Changes - Recover promptly from a cluster failover; fix the configuration channel under RESP3 and with a channel prefix ([#​3254](StackExchange/StackExchange.Redis#3254)) by [@​mgravell](https://github.com/mgravell) in [#​3255](StackExchange/StackExchange.Redis#3255) - Preserve binary members as VectorSetRange bounds by [@​Laurianti](https://github.com/Laurianti) in [#​3256](StackExchange/StackExchange.Redis#3256) - Smart client handoffs: reinstate auto-enlistment for the hosted providers and remove \[Experimental] by [@​mgravell](https://github.com/mgravell) in [#​3248](StackExchange/StackExchange.Redis#3248) - docs: fix retry-category test in the custom RetryPolicy sample by [@​mgravell](https://github.com/mgravell) in [#​3265](StackExchange/StackExchange.Redis#3265) - Fix duplicate LoggerMessage event ids; add a guard test by [@​mgravell](https://github.com/mgravell) in [#​3266](StackExchange/StackExchange.Redis#3266) - Scan: observe the faults of abandoned page tasks after a sync timeout or dispose ([#​3260](StackExchange/StackExchange.Redis#3260)) by [@​mgravell](https://github.com/mgravell) in [#​3267](StackExchange/StackExchange.Redis#3267) - Report the outcome of every physical connection attempt, identifying the client certificate used by [@​mgravell](https://github.com/mgravell) in [#​3257](StackExchange/StackExchange.Redis#3257) - Use a dedicated pub/sub connection under RESP3 by default by [@​mgravell](https://github.com/mgravell) in [#​3264](StackExchange/StackExchange.Redis#3264) #### Tooling - docs(agents): add create-implementation-plan-for-redis-api-change skill by [@​uglide](https://github.com/uglide) in [#​3268](StackExchange/StackExchange.Redis#3268) #### New Contributors - [@​Laurianti](https://github.com/Laurianti) made their first contribution in [#​3256](StackExchange/StackExchange.Redis#3256) **Full Changelog**: <StackExchange/StackExchange.Redis@3.3.1...3.4.0> </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4xNDkuMCIsInVwZGF0ZWRJblZlciI6IjQ0LjE0OS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Reviewed-on: https://git.milkshake.services/FloatingMilkshake/Azusa/pulls/9 Reviewed-by: Milkshake <floatingmilkshake@noreply.milkshake.services>
mgravell
added a commit
that referenced
this pull request
Oct 13, 2026
* Investigate Redis Enterprise lag-aware availability checks Confirms the premise: this is the client-side geographic failover feature family, and Lettuce, Jedis and redis-py have all shipped it. It feeds exactly the health checks our Availability/ namespace already has behind SER007. The abstractions line up almost one-to-one with redis-py's - probe, policy (all/any/majority), circuit breaker, per-member config - and two details make it land cleanly: HealthCheckProbe has no internal abstract members, so it is already subclassable from outside the assembly, and ConnectionGroupMember already carries a per-member HealthCheck override, which is what a per-cluster REST endpoint needs. Where we are better placed: HealthCheckResult has Inconclusive, which Lettuce's two-state HealthStatus lacks. Lettuce maps a failed REST call to UNHEALTHY, so a management-plane outage is indistinguishable from the data plane being down and can fail over a database that is serving fine. We can say "could not determine" instead. The awkward part is that src/ has no HTTP client and no JSON parser. JSON turns out to be avoidable if the bdb uid is configured rather than discovered, since the availability call is a status-code check. Also records the defaults disagreeing across clients: 100ms everywhere - server, docs, redis-py - but 5000ms in Lettuce. * Record what a Tunnel API for the management plane should look like mgravell offered to add one; the shape is what decides whether it helps. A member returning HttpMessageHandler would drag System.Net.Http into StackExchange.Redis's public API for the sake of one commercial deployment target - which is the thing keeping this in a separate package was meant to avoid, decided the expensive way. A stream-shaped member instead composes into SocketsHttpHandler.ConnectCallback, which takes exactly a ValueTask<Stream>, so the Enterprise side gets a complete HTTP hijack and core gains no HTTP types at all. Tunnel already speaks that language via BeforeAuthenticateAsync and ConnectTransportAsync, and adding a virtual is binary-safe for subclasses. Two functional arguments for Tunnel over a separate knob, beyond testing: a CONNECT proxy configured for Redis almost certainly applies to the Enterprise REST API too, so co-locating makes that correct by construction; and the test harness already assigns Tunnel, so one assignment would spoof both planes. Caveats recorded: ConnectCallback is net5+, the existing members are EndPoint/ConnectionType-shaped where HTTP wants a Uri, and widening Tunnel's remit means HttpProxyTunnel and LoggingTunnel each need an opinion about whether they apply to the management plane. Also retires open question 5: RawConfig and ConfigurationOptions.Tunnel are both already public, so a probe reaches the tunnel from the context it already gets - no HealthCheckContext change needed. * Real clusters and the fault injector, and a correction I had written that inducing "reachable but stale" on demand is not something anyone can do in CI, so the fakes were the only route to the scenario that matters. That is wrong. Redis ships a fault injection service that the client suites drive over HTTP - POST /action with a type and parameters, poll the returned action_id - and Lettuce's ActiveActiveFailoverScenarioTest injects network_latency against a bdb_id for a bounded duration, which is exactly the mechanism for pushing replication lag past the tolerance and watching the lag-aware check flip while the plain check stays green. So the authoritative end-to-end test is real cluster plus fault injector, on a road Lettuce has already built. The fakes are demoted rather than removed: they still cover machines with no Enterprise cluster, they are far faster than an injector action that budgets minutes plus a stabilisation wait, and they cover what the injector does not obviously reach - specific error_code bodies, 401/403 credential expiry, slow-but-successful responses for probe timeouts. Also records what the real clusters are uniquely good for: everything in section 1 is read off documentation, and a short session settles whether extend_check=lag is actually supported, what failure bodies really contain, whether the Host header is required, what the bdbs listing really returns - which decides whether the JSON-avoidance argument holds - and what lag real geo links show, which is the empirical way to settle 100ms versus 5000ms. * We already have a fault-injector tier; it answers several of the open questions It is not merged - tests/StackExchange.Redis.FaultInjector.Tests lives on marc/maint-optin-server, open as #3191 (107 files, +13160) - which is presumably why I did not find it when I grepped a branch taken fresh from main. Two pieces of it bear directly on this. FaultInjectorClient wraps the same POST /action + GET /action/{id} contract go-redis, redis-py and node-redis converged on, and adds GetValidTriggersAsync, so we can ask a deployment which lag-inducing effects it supports instead of guessing at an action name. And ClusterRestClient is already a Redis Enterprise REST client on 9443 - Basic auth, System.Text.Json, GET /v1/bdbs?fields=uid,name - so the availability call is a method on an existing class, not a new project. It also settles by observation what I had listed as open and sourced only from documentation: the management certificate is self-signed per environment and pinned to a CA from the config directory, so the probe needs CA configuration and cannot rely on the ambient trust store; auth really is Basic; and the bdbs listing returns the requested fields, so discovery is easy - which weakens the avoid-JSON argument to "it keeps JSON out of src/", not "discovery is hard". Sequencing consequence recorded: an end-to-end test is downstream of #3191. And #3191's own summary is the best argument for validating before building - same vendor, same class of API: the specifications are prose, the payloads were never published, and nearly every starting assumption was wrong. * Add the plan, blocked on #3191 Step 0 is to wait for #3191 to merge and build from there, rather than writing a second Redis Enterprise REST client against the same cluster and reconciling the two later - its ClusterRestClient already does Basic auth, CA pinning and GET /v1/bdbs, and its FaultInjectorClient can be asked which lag-inducing effects a deployment supports. The one thing that can start earlier is the API validation, because it produces notes rather than code. Step 1 is that validation, before any implementation: a checklist of things currently taken from documentation, including the measurement that settles 100ms versus 5000ms, with captured bodies saved as fixtures so the stubbed tests replay real payloads. Step 2 deliberately does not decide where the code lives, but records that the dependency argument is weaker than it looked - System.Net.Http needs no package reference on any target we ship - so the question is judgement, not constraint. Also records that this branch becomes the feature branch when step 0 clears, and that the way to take the merge is to rebase onto main rather than merge main in, since #3191 squash-merges. * Step 0 is cleared; note that step 1 needs the machine with cluster access #3191 merged as dc915bb, so this branch is rebased onto it - which is what step 0 asked for, and rebased rather than merged because #3191 squash-merged and a merge would have moved the merge base without bringing its ancestry. The fault-injector tier is present here now. Also recorded where the work can happen. Step 1 is validation against a real deployment plus the fault injector, and both live on the machine holding the environment directory; this box has no ~/aws and no env_output.json anywhere, so nothing in step 1 could be run or verified here. Worth writing down rather than rediscovering, since the environment being absent looks identical to the environment being offline. Steps 2 onward are ordinary code and are not tied to a machine. Noted too that the directory is the AWS multi-cluster template, so env_output.json nests its outputs under .clusters.value[0] - a shape FaultInjectorEnvironment already handles, so it needs no work, just no surprise. * Pin the cluster REST certificate per host rather than chaining to the config CA The 9443 certificate is self-signed by the cluster itself; the CA in the config directory only signs the RESP proxy certificates, so the chain build could never pass. Pin the leaf on first use, keyed by host so multi-cluster templates work. * Step 1 measured: the lag check is Active-Active only, and does what it says there Records the real-deployment results (findings section 10): off Active-Active the lag check fails unconditionally; on it, the stale member flips within seconds of sync pausing and the tolerance is honoured on a running link. Also the two integration problems found in Availability/, which mean src/ needs a change. * Correct the client defaults: Lettuce, Jedis and redis-py all default to 5000 ms Read from source: redis-py's 5000 contradicts its own docstring's 100. None of the three checks for Active-Active, and none has a third result state. * Measure what a probe can discover: not Active-Active over RESP, db_viewer suffices Active-Active membership has no RESP signal, so it comes from REST (fields=crdt) or configuration. db_viewer can call everything a probe needs, but unfiltered bdb/crdb reads expose database passwords even to it, so only filtered requests. The REST endpoint derives from the configured database hostname. * TLS reuse, the cross-client survey, and the local form routed by database host TrustIssuer(leaf) via TlsOptions validates the 9443 call unchanged. No other client activates lag checks automatically or checks crdt; go-redis routes the local form through each database host, which makes it truthful, and scopes the lag verdict to failback only. * The local form flips under paused sync exactly as the database form does Same verdict, same timing, stale member only, tolerance ignored while paused. Its failure code is bdb_endpoint_unavailable, shared with the wrong-node case, so the choice between forms is about call shape, not information. * Sketch the lag-aware API for review A failback-only health check per member rather than a new result value, a member-scoped probe, and a LagAware factory whose only required input is the management credential. Compared against Lettuce, Jedis, redis-py and go-redis. * API sketch: System.Text.Json decided; open questions compared with other clients Keep LagAware naming and failback (not fallback) terminology; rotating NetworkCredential callback plus a separate client-certificate option; and keep the liveness fallback, which no client has, because a dead peer probably makes the survivor's lag check fail. * Partition test: the lag check is blind for a minute, then fails on both sides Partitioning the clusters (tagged iptables rules, self-healing) showed no lag for ~60 s while sync had stopped, then tolerance-governed failure on both members; a tolerance of 0 is honoured. Supports the liveness fallback, and adds a caveat: failback in the first minute after a link breaks is not protected. * AllSuccess: a run of only inconclusive probes is not a pass The default policy reported Healthy when no probe failed, even if none succeeded. Harmless for liveness, where both count as connected, but wrong for any caller that distinguishes them. * Failback health checks: ask whether a member has caught up before moving to it (SER011) ConnectionGroupMember.FailbackHealthCheck (and a group default) is consulted only for members that are not active, and only Healthy makes them eligible. It never evicts the active member, and when no live member is eligible, selection falls back to liveness, since a partition leaves every region behind at once. HealthCheckProbe.Scope lets a probe run once per member rather than per endpoint. * Notes: core change implemented behind SER011 (SER007 is retired) * HealthCheck.LagAware: ask Redis Enterprise whether a member is available and caught up (SER011) A member-scoped probe of the database availability API. Only the management credentials are required: the REST endpoint derives from the configured host, the database is discovered with filtered requests only, the lag question is asked only of Active-Active databases, and no tolerance is sent unless set. Could-not-ask is Inconclusive, never Unhealthy. System.Text.Json on the down-level targets; custom certificate validation is unavailable on net461. * Notes: probe implemented; RawConfig is not public after all * LagAware: ask the cluster name, and never follow redirects Found against live clusters: non-master nodes answer the management routes (discovery, the crdt lookup) with a 307 to the master's internal address, so deriving the REST endpoint from the database host timed out for three of four members. Derive the cluster FQDN (strip redis-{port}.), which resolves to the master, and report a redirect as Inconclusive rather than following it. * Notes: management routes redirect to the master's internal address * Fault-injector scenario: refuse to fail back onto a stale Active-Active member Pauses sync into the preferred member through its cluster's REST API while writes go to the other, and asserts the group stays off it for 15s although it is alive, then fails back once it has caught up. The tier now reads every cluster's credentials, and shares its certificate pin with the probe. * Notes: tier-3 scenario test passing * LagAware: client certificates for the REST call LagAwareOptions.ClientCertificate, with SetUserPfxCertificate and (net6.0+) SetUserPemCertificate mirroring ConfigurationOptions and sharing its loaders. Wired into SocketsHttpHandler on .NET and HttpClientHandler down-level; not supported on net461. Every other client offers mTLS for this call. * Failback: a member chosen by the liveness fallback is provisional Other clients never select a member that fails its lag check; the nearest equivalent that still serves when every member is behind is to leave such a member for an eligible one as soon as one is available, whatever the weights, and to keep checking it so it becomes an ordinary active member once caught up. * Notes: fallback choice is provisional, as decided by client parity * Move the lag-aware log message to a free event id main now uses 124 and 125 (#3257); the guard test from #3266 rejects duplicates. * AnySuccess: a run of only inconclusive probes is not a failure AnySuccess reported Unhealthy whenever nothing succeeded, even when nothing failed either - so a probe that cannot tell (a key-write probe on a replica, say) marked its member unhealthy. Now Inconclusive, matching AllSuccess and MajoritySuccess. * Move the lag-aware log message to a free event id (again) main has since taken 128 as well. * Failback: a zero FailbackDelay clears a failure in the same clock tick; no ValueTuple UpdateState compared the last failure strictly before the cutoff. With FailbackDelay zero the cutoff is now, and on .NET Framework's ~15.6ms clock a failure and the passing check that clears it often share a tick, holding the member out for another interval. Compare inclusively. Also replace a named tuple in the failback-check pass: the library must not reference System.ValueTuple. * Ship lag-aware failback without an experiment Remove SER011: the attributes, the constant and NoWarn entry, the [SER011] API prefixes and docs/exp/SER011.md. It never shipped, so it is not a reserved id. Also restores main's using directives and encoding in the files that already existed there, which earlier edits had disturbed. * Notes: no experiment * Notes: drop the SER011 history
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Three source-generated
[LoggerMessage]events inLoggerExtensions.csshared anEventIdwith another event, and one had no id at all:LogInformationRequestingMaintenanceNotificationsLogInformationRegisteringSlotMapNodeLogInformationMaintenanceNotificationsAcceptedLogInformationActivatingUndialledServerLogInformationTLSConnectionEstablished(cipher-suite overload,#if NET)SslProtocols-only overload, with a different message templateLogErrorConnectionFailed116/117 came from two branches landing a day apart that both took "the next free number". Nothing enforced uniqueness.
Fix
LogErrorConnectionFailedan explicit id, 127. Note this is a visible change from the implicit 0 it has shipped with; unused gaps (80, 103, 104) were deliberately not reused.LoggerEventIdTests.EventIdsAreExplicitAndUnique, which reflects overLoggerExtensions, fails if any[LoggerMessage]lacks an explicitEventIdor any id is used more than once, reports every problem in one message, and logs the highest and next free id. Against the pre-fix code it reports exactly the four rows above.Verification
main, passes with the renumbering./p:CI=true.