backlog: file #1210, the connscale FD peak reports an adopted subtree as the engine - #309
Merged
Merged
Conversation
… as the engine test_connscale_smoke_end_to_end red on #298 with fd_count_monotonic: "fixed_per_conn@N=24: 344 < prior 3.56e+04 * 0.75". The suspect number is the PRIOR. Calibrated against this repo's own published at-scale run: ~3.0 handles/connection over an ~833 base (2333 @ N=500, 3835 @ N=1000, 5335 @ N=1500), so N=12 predicts ~870. The observed 344 at N=24 is in band. 35,600 at N=12 is 6.7x the reading for a FIFTEEN HUNDRED connection engine, and no leak produces both minutes apart in one sweep from two separately-spawned engines. Mechanism: _walk_descendants validates nothing -- no creation time, no image name, no cardinality bound. Windows keeps a stale ParentProcessId after the parent exits and recycles PIDs, so any live process pointing at the engine's recycled PID is adopted with its whole subtree. Measured on this box, walking one such stale ppid summed 144,688 handles. And in the connscale smoke the engine spawns NO children, so every descendant the walk finds is by construction not the engine. max() then latches the poisoned tick permanently -- a mean would have diluted it. My first reading was "handles has no PID-set gate like CPU does". An independent pass sharpened it and corrected the remedy: copying #220's gate would be WRONG. Differencing sums over different PID sets is arithmetically invalid, but an INSTANTANEOUS gauge over a genuinely larger subtree is correct. The gap is that the number carries no record of the set it covered. Validating the walk (reject a descendant older than its root) is the fix; gating the aggregate is not. Also recorded: #220's gate is a change-detector, not a binding check -- a first-and-only resolution that adopts a wrong-but-stable subtree passes it. And the test that shipped #220's fix asserts this exposure as correct, on a fixture that is physically unrealizable: it varies cpu_pids while pinning handles=61, though the Windows probe reads both from the same Get-Process rows. Diagnosis only. The fix shape is proposed and ranked but not applied.
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.
Diagnosis only — no code change.
test_connscale_smoke_end_to_endred on #298 with:The suspect number is the prior, not the failure. Calibrated against this repo's own published at-scale run (
docs/benchmarks/results/2026-07-03-adr0066-pooled-atscale/pooled_ab_atscale.txt):No leak produces 35,600 at N=12 and 344 at N=24 minutes later in the same sweep, from two separately-spawned engines. The engine was almost certainly healthy in both arms.
Mechanism: stale-
ParentProcessIdadoption, not a leak_walk_descendantsBFSes the ppid→children map from the engine root and validates nothing — no creation time, no image name, no cardinality bound. Windows does not clearParentProcessIdwhen a parent exits, and it recycles PIDs, so any live process whoseParentProcessIdequals the engine's recycled PID is adopted along with its entire subtree.Measured on the maintainer's box (406 processes, 204,882 handles total): seven dead PIDs still have live processes pointing at them, and walking one as if it were a freshly-recycled engine PID summed 144,688 handles. The observed range is reproducible by this mechanism.
And in the connscale smoke the engine has no children at all —
harness/load/failover.pyspawnsservewith no--shards, and__main__.pyruns uvicorn in-process. So every descendant the walk finds is by construction not the engine. A cardinality bound alone would have caught it.max()then latches the poisoned tick permanently; a mean would have diluted it.Why #220's gate does not cover it — and why copying that gate would be wrong
My first reading was "handles has no PID-set gate like CPU does." An independent verification pass sharpened it and corrected the remedy, which is the part worth keeping:
#220 hardened the CPU path to degrade set-change intervals to gaps. That is right for a difference — differencing two sums taken over different PID sets is arithmetically invalid however legitimate the joiner. But an instantaneous gauge summed over a genuinely larger subtree is correct. So the fix here is not the same gate. The real gap is that the number carries no record of the set it covered, leaving a legitimate growth and a misresolution indistinguishable — and the SLO draws a verdict anyway.
Secondary, and worse: #220's gate is a change-detector, not a binding check. A step whose first and only resolution adopts a wrong-but-stable subtree yields
pa == pbon every interval, so CPU reports too, and the flat-counter guard will not fire because an adopted tree does burn CPU. The smoke profile is exactly that shape.The exposure was asserted as correct by the test that shipped #220's fix
tests/test_connscale_cpu_probe.py::test_a_membership_changed_interval_is_degraded_to_a_gapends:Two problems. The rationale is true of the differencing arithmetic and false of a peak over a set never validated to be the engine. And the fixture is physically unrealizable: it varies
cpu_pids{100}→{100,200}→{100}while pinninghandles=61across all three ticks, but the Windows probe reads handles and CPU from the sameGet-Processrows, so a joining PID necessarily moves both. It constructs the one input shape in which this exposure is invisible, then asserts the pass-through as correct.Blast radius
The SLO is asserted by the smoke test, so this reds PRs that touch nothing near the harness — it red #298, whose diff was two
scripts/asvs/files.Fix shape, ranked (proposed, not applied)
_descendants_windowsalready queriesWin32_Process; addCreationDateand reject any candidate created before the root./proc/<pid>/statfield 22 gives the same on POSIX. This single change would have prevented the observed number.cpu_pidsalready is that set and is simply not consulted._CPU_FLAT_GAP_SPAN_S: for a non-sharded engine, a subtree of cardinality > 1 is a wrong binding and should degrade to a gap, not a number.handlesmoves with the PID set; the current one cannot fail on this class.Not covered by #1101, the sibling SLO defect on the same test — it touches
handles_peakonce, only to dismiss it as a control for time dilation. Correct about dilation, silent on provenance.