CI/testing sweep: compliance assertion scoping, Marten segmentation, dispose leaks - #3775
Merged
Merged
Conversation
…ose leaks Addresses the open testing/CI issues, plus the two red jobs on main today. GH-3726 -- CIRabbitMQ quorum_queue_compliance (the current failure) The transport compliance error-handling assertions picked their "ending activity" by message TYPE: session.AllRecordsInOrder() .Where(x => x.Envelope!.Message is ErrorCausingMessage) .LastOrDefault(... MessageSucceeded or MovedToErrorQueue ...) Broker queues are shared across the test methods in a compliance class, so a redelivery still in flight when one test's tracked session completed lands in the *next* test's session and becomes its ending record. Today's log shows it directly: message 08def019-82b1-43da arrives at 27ms in one test and 28ms in another -- before either had sent anything -- under two different node ids, and its MessageSucceeded at 3913ms is what both assertions latched onto. ErrorCausingMessage now carries an Id and the three helpers scope to it. That is immune to residue by construction; purging cannot help here, because an unacked in-flight redelivery is not in the queue to purge. GH-3771/GH-3762 -- CIMarten runner deaths CIMarten was killed outright by the runner twice in a row at ~15 minutes on an unrelated diff, the OOM signature rather than a test failure. The worker-lane split is the cause: several Marten hosts plus Postgres does not fit a 4-vCPU/16GB hosted runner. Lanes are gone. The suite is segmented across three CI jobs the way CIPolecat is (#3350), each strictly sequential against exactly ONE database -- concurrency moves off the runner VM and onto the matrix. Verified disjoint and exhaustive against a real `--list-tests` (159 classes / 545 tests). This also closes GH-3762: with a single lane per job the sibling databases (tenant1-3, players, things) have one owner again, so no lane-scoping sweep is needed. A memory sampler stays on the three Marten jobs to confirm the footprint actually dropped. It prints to the live log on purpose: when the runner dies, later steps are skipped, so an artifact upload would never run. GH-3763 -- the `public new DisposeAsync()` shape 34 compliance fixtures declared `public new DisposeAsync()`. It reads like an override and is not one: TransportCompliance<T> disposes through a T-typed reference, which binds statically to the base. Every one of them was dead code. Seven carried real cleanup that had therefore never executed -- a SQLite database, shared-memory queues, and four local MQTT brokers, each leaking a listening socket for the life of the test process. Four more were infinitely self-recursive. Real cleanup moved to AfterDisposeAsync (virtual, and actually runs); the rest deleted; the trap documented on the base method. GH-3751 -- Testcontainers fixtures leak one container per process The Redis/MQTT/Mqtt5/Pulsar [ModuleInitializer] fixtures never disposed their container, relying entirely on Ryuk. With ryuk.disabled=true every test process leaks one permanently, and the process count is rising (per-class isolation, retry-in-fresh-process). Took the small-diff option from the issue -- a ProcessExit hook that keeps the static-property ergonomics -- rather than the invasive IAsyncLifetime conversion. Also honours a WOLVERINE_* connection string when one is set, so a caller can supply a warm broker and skip the start, which sits before Main on every process launch. GH-3725 -- finish the xUnit1051 rollout TracingTests opted in (clean, no diagnostics), the Directory.Build.targets opt-in block deleted, and the now-redundant property dropped from all 72 projects. Full `wolverine.slnx` builds clean with the analyzer enforced everywhere. Verified: full solution builds clean; LightweightTcpTransportCompliance 22/22, which exercises all four changed error-handling assertions and proves the new message Id round-trips through serialize/transport/deserialize. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013eR4GL278688VhyhrGcttJ
…phan release `multi_node_exclusive_listener_recovery.rows_released_after_the_listener_is_already_running_are_still_recovered` failed ~40% of runs. #3729 measured the rate, could not decide whether the listener's ownership check was racy or the window was too tight, and was closed on the grounds that the retry harness rescues it. Measured again here: 3 failures in 8 runs. It is neither racy ownership nor a tight window. The test seeded its rows owned by a fabricated node number (987654) standing in for a departed node — and a fabricated owner is by definition an orphan. Releasing orphans is a job the product does on purpose: -- ReleaseOrphanedMessagesOperation, on the durability agent's schedule update wolverine_incoming_envelopes set owner_id = 0 where owner_id != 0 and owner_id not in (select node_number from wolverine_nodes) So the rows stopped being "owned by another node" on their own, the exclusive listener's 250ms recovery poll then claimed them perfectly correctly, and tracking.Count.ShouldBe(0, "The listener must not touch inbox rows that are still owned by another node") failed on intended behaviour. Instrumenting the 2-second window shows the order directly — on a failing run globallyOwned flips 0 -> 5 at t=900ms and only then does tracked go to 5: DIAG t=800ms globallyOwned=0 tracked=0 DIAG t=900ms globallyOwned=5 tracked=0 <- orphan release DIAG t=1400ms globallyOwned=0 tracked=5 <- listener, correctly ListenerInboxRecovery only ever loads owner_id = 0 rows, so the listener never touched a row owned by anyone. The product is right; the test's premise was wrong. Fix: seed the rows owned by another LIVE node in the cluster (the durability agent's host). Live rows are not orphans, so nothing is entitled to free them, and the assertion becomes a real statement about the listener instead of a race against the durability agent. The stale bump — the other thing that frees owned rows — keys off InboxStaleTime, which is null unless explicitly configured and this test does not set it. Verified: 12/12 consecutive passes against a 3-in-8 baseline (P(chance) ~ 0.35%), and the full class 3x. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013eR4GL278688VhyhrGcttJ
The first cut of this split was balanced by test-CLASS count, on the PolecatTests precedent that
per-class fixture cost dominates. The first CI run showed that does not hold for MartenTests:
CIMartenWorkflow 162 tests 3m03s test phase
CIMarten 190 tests 2m55s
CIMartenSagas 204 tests 6m13s <- 1.83 s/test vs ~1.0 elsewhere
Profiled the full suite with `MartenTests --output Detailed` (533 tests, 880s, zero failures).
Class count turned out to be very nearly an *inverted* predictor of cost — the expensive
namespaces are the ones with few, slow tests:
Distribution 192.6s / 49 tests 21.9% multi-node, real leader election
MultiTenancy 148.4s / 53 tests 16.9% provisions tenant databases
Bugs 126.5s / 53 tests 14.4%
AggregateHandlerWorkflow 73.4s / 90 tests 8.3% <- most tests, a third of Distribution
Distribution and MultiTenancy are 39% of the suite between them, and the class-count split had put
both in the same shard. That is the whole of the imbalance.
Rebalanced by brute force over all 3-way assignments of the measured totals (the root namespace is
pinned to the catch-all, since every display name starts with it):
CIMartenDistribution 170 tests 293.6s
CIMarten 231 tests 293.5s
CIMartenTenancy 132 tests 293.4s
A 0.2s spread, verified disjoint and exhaustive against all 533 measured tests.
Renamed the two shards because the balance does not respect theme: a job called CIMartenSagas that
does not run MartenTests.Saga is worse than no name at all. They are now named for what dominates
them, and the comment says so.
Worth stating plainly: CIMarten is no longer the workflow's long pole — on the previous run the
matrix was set by CIRabbitMQ at 13m10s, with the worst Marten shard at 7m56s. Per #3752's own
argument, work on a job below the pole moves the workflow wall clock by zero. This buys
runner-minutes and headroom under the 20m cap, not elapsed time.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013eR4GL278688VhyhrGcttJ
This was referenced Aug 2, 2026
This was referenced Aug 4, 2026
This was referenced Aug 11, 2026
This was referenced Aug 20, 2026
This was referenced Aug 28, 2026
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
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.
Addresses the open testing/CI issues, plus the two red jobs on
maintoday. Closes #3726, #3729, #3771, #3762, #3751, #3725; partially addresses #3763.The two findings that were not what the issues said
#3729 — closed as a "genuine 40% flake"; it is a test bug with a traced mechanism
multi_node_exclusive_listener_recovery.rows_released_after_the_listener_is_already_running_are_still_recoveredseeded its rows owned by a fabricated node number (987654) standing in for a departed node. A fabricated owner is an orphan, and freeing orphans is a job the product does on purpose:So the rows stopped being "owned by another node" on their own, the exclusive listener's 250ms recovery poll then claimed them perfectly correctly, and
failed on intended behaviour. Instrumenting the 2-second window shows the order directly — on a failing run:
ListenerInboxRecoveryonly ever loadsowner_id = 0rows, so the listener never touched a row owned by anyone. The product is right; the test's premise was wrong.Fixed by seeding from another live node — live rows are not orphans, so nothing is entitled to free them, and the assertion becomes a real statement about the listener. (
InboxStaleTime, the other thing that bumps owned rows, is null unless explicitly configured and this test does not set it.)Verified: 12/12 consecutive passes against a 3-in-8 baseline (P(chance) ≈ 0.35%), plus the full class 3×.
#3726 — the three tests it names are all already resolved
Verified locally: item 1 passes (fixed by merged #3728), item 3 passes (that was #3729), and item 2 was only ever a missing local SQL Server container. A full
CIRabbitMQ --disable-test-retryrun was 435 passed / 1 failed, and the one failure was #3729 above.That run surfaced the current chronic failure instead — which is what took CI red today.
quorum_queue_compliance— today's red CIRabbitMQThe transport compliance error-handling helpers picked their "ending activity" by message type:
Broker queues are shared across the test methods in a compliance class, so a redelivery still in flight when one test's tracked session completed lands in the next test's session and becomes its ending record. Today's log shows it directly: message
08def019-82b1-43daarrives at 27ms in one test and 28ms in another — before either had sent anything — under two different node ids, and itsMessageSucceededat 3913ms is what both assertions latched onto.ErrorCausingMessagenow carries anIdand the three helpers scope to it. That is immune to residue by construction; purging cannot help, because an unacked in-flight redelivery is not in the queue to purge.#3771 / #3762 — CIMarten runner deaths
CIMarten was killed outright by the runner twice in a row at ~15 minutes on an unrelated diff — the OOM signature, not a test failure. The worker-lane split is the cause: several Marten hosts plus Postgres does not fit a 4-vCPU/16GB hosted runner. The build already recorded that 4 lanes had killed a runner once; the clamp to 2 still died.
Lanes are gone. The suite is segmented across three CI jobs the way CIPolecat is (#3350), each strictly sequential against exactly one database — concurrency moves off the runner VM and onto the matrix, where every shard gets its own 16GB.
CIMartenWorkflowCIMartenSagasCIMartenVerified disjoint and exhaustive against a real
MartenTests --list-tests(159 classes / 545 tests). The catch-all is the negation of the two named shards, so a brand-new namespace lands somewhere instead of being silently dropped. (The root namespace can never be an include-list entry — every display name starts with it — which is exactly why the catch-all has to be the negation.)This also closes #3762: with a single lane per job the sibling databases (
tenant1-3,players,things) have one owner again, so no lane-scoping sweep is needed.A memory sampler stays on the three Marten jobs to confirm the footprint actually dropped. It prints to the live job log on purpose — when the runner dies, later steps are skipped, so an artifact upload would never run.
#3763 — the
public new DisposeAsync()shape34 compliance fixtures declared
public new DisposeAsync(). It reads like an override and is not one:TransportCompliance<T>disposes through aT-typed reference, which binds statically to the base. Every one of them was dead code.LocalMqttBrokers, each leaking a listening socket for the life of the test process. Moved toAfterDisposeAsync(virtual, and actually runs).await DisposeAsync()calling itself).The trap is now documented on the base method. This is the same shape #3758 fixed for Azure Service Bus; that sweep just never covered the other transports.
#3751 — Testcontainers fixtures leak one container per process
The Redis/MQTT/Mqtt5/Pulsar
[ModuleInitializer]fixtures never disposed their container, relying entirely on Ryuk. Withryuk.disabled=trueevery test process leaks one permanently, and the process count is rising. Took the small-diff option from the issue — aProcessExithook that keeps the static-property ergonomics — rather than the invasiveIAsyncLifetimeconversion. Each also honours aWOLVERINE_*connection string when set, so a caller can supply a warm broker and skip a start that sits beforeMainon every process launch.#3725 — finish the xUnit1051 rollout
TracingTests opted in (clean, no diagnostics), the
Directory.Build.targetsopt-in block deleted, and the now-redundant property dropped from all 72 projects. Fullwolverine.slnxbuilds clean with the analyzer enforced everywhere.Verification
wolverine.slnxbuilds clean (0 warnings, 0 errors).LightweightTcpTransportCompliance22/22 — exercises all four changed error-handling assertions and proves the new messageIdround-trips through serialize/transport/deserialize.CIRabbitMQ --disable-test-retry: 435/436, the one failure being multi_node_exclusive_listener_recovery: listener touches inbox rows still owned by a departed node (~40% failure) #3729, now fixed.Two things I could not verify locally, called out honestly:
mainpassed on a fast machine with a fresh broker. It needs CI's slower, loaded conditions. CIRabbitMQ on this PR is the real verification.CIOtelhere is the outstanding evidence.Not addressed: the #3763
Category=Flakyledger itself (~50 tests), and #3752/#3761 (CIAzureServiceBus wall clock).🤖 Generated with Claude Code
https://claude.ai/code/session_013eR4GL278688VhyhrGcttJ