Skip to content

Revert the 32KB packet size — it caused a fleet-wide error burst (#2164) - #2179

Merged
erikdarlingdata merged 1 commit into
devfrom
revert-packet-size-2164
Aug 11, 2026
Merged

Revert the 32KB packet size — it caused a fleet-wide error burst (#2164)#2179
erikdarlingdata merged 1 commit into
devfrom
revert-packet-size-2164

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

Reverts #2178, which I merged and deployed without ever opening a connection at the new packet size.

What happened

The test I wrote asserted the connection string contained PacketSize=32768 and that the value sat inside the driver's accepted range. Both passed. Neither proves a connection works — and that was the only risk that mattered, because a packet-size change can't be wrong "a little": it either negotiates or the transport breaks.

Measured on the dogfood fleet, with the previous deploy as a clean control:

box time event failures
08:38 open/drain deploy, no packet change 0
09:22 32KB service start
09:23:32 – 09:24:49 44 transport-level errors across 44 servers 77-second burst
after 09:24:49, still 32KB 506 successes, 0 failures

A transport-level error has occurred when receiving results from the server, once per server, on first connect at the new size — then clean on retry. Self-healing, but a fleet-wide error burst on every service start is not shippable, and on upgrade it would have hit every operator's fleet at once, not just ours.

Rolled back on the box already (restored the pre-change binaries; fleet is healthy, 53 fresh, no new errors). This takes it off dev so it cannot reach 3.5.0.

What survives

The measurement that motivated it is unaffected and still on #2164: drain is 94–97% of a query_store pass, and drain throughput scales with LOB size per row rather than total bytes (256 KB/s at ~180 KB/row versus 1,080 KB/s at ~41 KB/row, same link and code). Per-packet overhead remains the best explanation. The idea isn't dead — the way I shipped it was wrong.

What the next attempt has to do differently

  1. Prove a real connection at the new size before merge — a live connect against at least one representative instance, not a string assertion.
  2. Don't jump to the protocol maximum. 32768 is the ceiling; something in the path (RDS, the encryption layer, an MTU interaction) objects to it on first negotiation. An intermediate value, or negotiating upward with fallback, is the shape that can be rolled out safely.
  3. Consider whether this belongs behind a per-server setting at all, so one unhappy instance can't produce a fleet-wide burst.

Reverts #2178. Part of #2164.

Reverts #2178. I shipped a connection-level change without ever opening
a connection with it: the test asserted the connection STRING contained
PacketSize=32768 and that the value was in the driver's accepted range,
which both passed while proving nothing about a real connect.

Measured on the fleet, with the prior deploy as a clean control:

  08:38  open/drain deploy, no packet change   ->  0 failures
  09:22  32KB service start
  09:23:32-09:24:49                            -> 44 transport-level
                                                  errors, 44 servers
  after 09:24:49, still 32KB                   -> 506 successes, 0 fail

So every server failed a collection once on its first 32KB connect and
succeeded on retry. Self-healing, but a fleet-wide error burst on every
service start is not shippable, and it would have hit every operator's
fleet on upgrade.

The measurement that motivated it stands (drain is 94-97% of a pass and
scales with LOB size, not total bytes). The next attempt has to prove a
real connection at the new size before merge, and probably negotiate
upward rather than jumping to the protocol maximum.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review: Revert 32KB packet size (#2179)

Reviewed the full diff (CHANGELOG.md, both MonitoredServerConnection.cs/ServerConnection.cs builders, CollectorTdsTuning.cs, PacketSizeTuningTests.cs) against the stated goal of a clean revert of the packet-size portion of #2164.

Correctness — clean revert, no dangling references:

  • Both connection-string builders (Lite and Darling) have the PacketSize assignment and its comment removed, restoring MultiSubnetFailover as the trailing initializer property with correct comma placement (no syntax break).
  • The unused using PerformanceMonitor.Common; is correctly dropped from Darling's MonitoredServerConnection.cs.
  • CollectorTdsTuning.cs and its only consumer, PacketSizeTuningTests.cs, are both deleted — grepped the whole tree for CollectorTdsTuning, PacketSize, and MonitoredServerPacketSize and found no other references, so nothing is left pointing at the removed type.
  • The CHANGELOG edit removes only the one bullet describing the packet-size change; the surrounding #2164 entries (open/drain timing split, memory-bound knobs) are untouched, correctly scoping the revert to just the packet-size piece rather than all of [PERF] QS collector drain-mode passes cost the full byte budget every cycle — make the budget a knob, skip already-shipped plans by hash #2164.

Lite/Darling parity — good: the revert is symmetric. Both builders lose the identical PacketSize line/comment. Confirmed there was never a Lite-side equivalent of PacketSizeTuningTests.cs, so there's no orphaned Lite test to also remove.

Security / performance — no concerns; this is a pure removal of a client-side TDS tuning knob, no input handling or secrets involved. The PR description's own data (44 transport errors across 44 servers in a 77-second burst at 32KB) is solid justification for reverting before the packet-size change reaches a wider fleet.

No issues found — this looks like a safe, well-scoped revert.

@erikdarlingdata
erikdarlingdata merged commit f2d5799 into dev Aug 11, 2026
5 checks passed
@erikdarlingdata
erikdarlingdata deleted the revert-packet-size-2164 branch August 11, 2026 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant