Skip to content

fix(quota-share): give each in-flight request its own lease - #11547

Merged
diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.51from
abhisheksharma2411:fix/quota-share-inflight-lease-v51
Aug 26, 2026
Merged

diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.51from
abhisheksharma2411:fix/quota-share-inflight-lease-v51

Conversation

@abhisheksharma2411

Copy link
Copy Markdown
Contributor

Summary

The quota-share in-flight counter is what P2C reads to pick the less-loaded connection, and its leak bound did not hold.

A slot carried one expiry per connection, and incrementInflight() refreshed it on every increment. So a request that aborted before its release callback ran kept its count alive for as long as the connection kept taking traffic — the leak was unbounded precisely when the connection was busy, which is when P2C's view of load matters most. The module docstring promises the opposite:

the counter can never leak forever, even without touching the generic dispatch

Reproduction

Driven through the injectable clock, so it is deterministic. One orphaned increment, then a request every 60s that increments and releases correctly:

t=0     incrementInflight("conn-X")            // request aborts, never releases
t=60s   increment + decrement                   // normal traffic
t=120s  increment + decrement
...
t=600s  getInflight("conn-X") -> 1

Ten minutes on — five times the 120s DEFAULT_LEASE_MS — the orphan still counts. Every later increment pushed the shared expiresAtMs forward, so it never aged out.

Fix

Each request now carries its own expiry and the count is the number of live leases, so an orphan ages out on its own schedule regardless of what its neighbours do.

Retiring on release needs a rule, because a release does not say which request settled:

  1. If any lease has already expired, retire the newest of those. A request that outlived its own lease is by definition the longest-running one, so an expired lease is the best match for the caller — and consuming it leaves live neighbours alone. Retiring a live lease instead would decrement twice for a single settled request: once when the expiry lapsed, once again now.
  2. Otherwise retire the newest live lease — never the oldest, or a normal request's release would retire an orphan's lease and leave its own newer one behind, and the orphan would never age out.

Rule 1 exists because my first attempt got this wrong, and I would rather show that than hide it. Retiring expired leases and then popping a live one made this case report 1 where the previous code correctly reported 2:

A at t=0 (lease -> 120s), B at t=100s (-> 220s), C at t=110s (-> 230s)
A releases at t=130s, B and C still genuinely in flight

before this PR : 2   correct
first attempt  : 1   regression
this PR        : 2   correct

Bounded at 4096 leases per connection; past that the oldest is dropped, which can only under-count an already-saturated connection — the fail-open direction this module takes everywhere else (getInflight returns 0 for anything unknown).

Scope, and what I am not claiming

I originally thought a second bug was here — that decrementInflight deleting the whole entry on an expired slot would zero out live neighbours. It does not, in practice. Because the shared expiry was refreshed by every increment, it could only lapse after a full lease with no traffic, by which point there are no live neighbours to lose. I checked before writing it up rather than after. The only defect fixed here is the leak.

Tests

Three added to tests/unit/quota-share-strategy.test.ts. Two fail against the current implementation, verified by swapping the base file in and confirming the swap took effect before running:

without the fix:  34 tests, 32 pass, 2 fail
  ✖ an orphaned lease expires even while the connection keeps taking traffic
  ✖ expired leases are retired without an explicit decrement, per request

The third pins neighbour-preservation, which already held and now holds by construction — flagging that so it is not miscounted as a third regression caught.

Full runs on this base: quota-share-strategy 34/34, tests/unit/combo/* 162/162, tests/unit/*quota* 1424 green, tsc -p tsconfig.typecheck-core.json clean.

Notes

The in-flight counter is what P2C reads to pick the less-loaded connection,
and its leak bound did not hold.

A slot carried ONE expiry per connection, and incrementInflight() refreshed
it on every increment. So a request that aborted before its release callback
ran kept its count alive for as long as the connection kept taking traffic —
the leak was unbounded precisely when the connection was busy, which is when
P2C's view of load matters. The module docstring promised the opposite: "the
counter can never leak forever, even without touching the generic dispatch".

Demonstrated with the injectable clock: one orphaned increment, then a
request every 60s that increments and releases correctly. Ten minutes on —
five times the 120s lease — the orphan still counted 1.

Each request now carries its own expiry and the count is the number of live
leases, so an orphan ages out on its own schedule regardless of neighbours.

Retiring on release needs a rule, since a release does not say which request
settled:

  1. If any lease has already expired, retire the newest of those. A request
     that outlived its own lease is the longest-running one, so an expired
     lease is the best match for the caller, and consuming it leaves live
     neighbours alone. Retiring a live lease instead would decrement twice
     for one settled request.
  2. Otherwise retire the newest live lease — never the oldest, or a normal
     release would retire an orphan's lease and leave its own behind, and the
     orphan would never age out.

Rule 1 exists because the first attempt at this got it wrong: retiring an
expired lease AND popping a live one made a three-concurrent-request case
report 1 where the previous code correctly reported 2. That case is now
pinned by the probe in the PR description and matches the old behaviour.

Bounded at 4096 leases per connection; beyond that the oldest is dropped,
which can only under-count an already-saturated connection — the fail-open
direction this module takes everywhere else.

Tests: 3 added. Two fail against the current implementation; the third
documents neighbour-preservation, which already held and now holds by
construction. quota-share 53, combo 162, quota-wide 1424 — all green.
Typecheck clean.
@diegosouzapw
diegosouzapw merged commit b7d91a6 into diegosouzapw:release/v3.8.51 Aug 26, 2026
3 checks passed
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…zapw#11547)

Merged via /merge-batch (lote 2026-08-26, v3.8.51). Boarded no worktree combinado junto com outras ~30 PRs; validação única: typecheck/complexity/cognitive-complexity/changelog-integrity verdes, file-size rebaseado onde necessário (crescimento legítimo), lint com os mesmos 228 achados pré-existentes confirmados via sonda contra o tip puro (não introduzidos por este lote), e ~370 testes focados (unit + vitest) passando. Obrigado pela contribuição.
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.

2 participants