Skip to content

test(sync): guard the responder two-stage handoff reservation against leaks - #1698

Merged
branarakic merged 2 commits into
mainfrom
followup/sync-responder-displacement-tests
Jul 14, 2026
Merged

test(sync): guard the responder two-stage handoff reservation against leaks#1698
branarakic merged 2 commits into
mainfrom
followup/sync-responder-displacement-tests

Conversation

@branarakic

Copy link
Copy Markdown
Contributor

Follow-up to #1691. Test-only, plus one export — no runtime behavior change.

Why

#1691 gave the responder a two-stage admission: pre-authorization runs first, and only once the request is authorized does it get scheduled at its Context Graph's priority. To keep that hand-off from breaching the per-peer capacity contract, the authorized stage reserves its queue slot up front, while pre-authorization is still running.

That reservation counts against SYNC_RESPONDER_PER_PEER_QUEUE_LIMIT. Which means a reservation left behind by a request that never reaches the authorized stage is not a leak a peer can wait out:

  • 4 short-circuiting requests (the per-peer limit) leave 4 stale reservations;
  • every later request from that peer is then refused as sync responder peer queue full — while its queue is in fact empty;
  • the peer never recovers, and nothing in the logs points at the reservation.

The release path is correct today, on both routes that skip the authorized stage (an authorization denial, and an authorization that throws). This PR keeps it that way.

What

Two regression tests in sync-responder-protection.test.ts, driving more short-circuiting requests than the per-peer limit through one responder — a fresh handler would get a fresh limiter and hide the leak entirely — then asserting the peer is still served, and that no failure along the way was a capacity rejection.

SYNC_RESPONDER_PER_PEER_QUEUE_LIMIT is exported so the tests bind to the real limit rather than a hardcoded 4, matching the snapshot limits already exported for the same reason.

Verification

These guards were checked for teeth, not just for green: reintroducing the leak (dropping the handoffReservations.delete(...) in PriorityAdmissionQueue's release) fails both tests with exactly the predicted symptom —

AssertionError: expected 'sync responder peer queue full' not to match /queue full/

— and restoring it turns them green again.

  • sync-responder-protection: 20 passed
  • sync-backpressure, sync-requester-priority, sync-responder-snapshot-cache, sync-on-connect-retry: 91 passed
  • agent runtime build: green

Context

This closes the one open item from the #1691 review: displacement/handoff behavior was covered on the requester lane but not on the responder. The other two review findings (the failedPeers peer-cardinality sum, and the per-peer queue overflow on hand-off) were already fixed in f8e295987 — the overflow by this very reservation mechanism, which is what makes it worth pinning down.

🤖 Generated with Claude Code

… leaks

Follow-up to #1691. The responder admits a prioritized request in two stages,
and the authorized stage reserves its queue slot while pre-authorization is
still running. That reservation counts against the per-peer queue limit, so a
reservation left behind by a request that never reaches the authorized stage is
not a leak a peer can wait out: once SYNC_RESPONDER_PER_PEER_QUEUE_LIMIT of them
accumulate, every later request from that peer is refused as "peer queue full"
while its queue is in fact empty, and the peer never recovers.

The reservation is released correctly today. These tests keep it that way: they
drive more short-circuiting requests than the per-peer limit through a single
responder (a fresh handler would get a fresh limiter and hide the leak), for
both paths that skip the authorized stage — an authorization denial and an
authorization that throws — then assert the peer is still served.

Verified to have teeth: reintroducing the leak (dropping the reservation delete
in PriorityAdmissionQueue's release) fails both tests with exactly the predicted
"sync responder peer queue full" on an empty queue.

`SYNC_RESPONDER_PER_PEER_QUEUE_LIMIT` is exported so the tests bind to the real
limit instead of a hardcoded copy, matching the snapshot limits already exported
for the same reason.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
const SYNC_RESPONDER_PER_PEER_CONCURRENCY = 1;
const SYNC_RESPONDER_QUEUE_LIMIT = 64;
const SYNC_RESPONDER_PER_PEER_QUEUE_LIMIT = 4;
export const SYNC_RESPONDER_PER_PEER_QUEUE_LIMIT = 4;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Issue: Avoid exporting a private limiter knob just for one regression test

What's wrong
This change leaks an implementation detail from the responder scheduler into the module's exported surface. The queue limit is not a domain constant like the snapshot policy exports below it; it is a private admission-queue tuning value. Exporting it for a test makes the production module less encapsulated and gives future code a tempting dependency on an internal limit instead of the responder behavior.

Example
A later production caller can now import SYNC_RESPONDER_PER_PEER_QUEUE_LIMIT from sync-handler.ts and couple unrelated code to a private admission-queue tuning value, making it harder to change the responder limiter model or split limits by lane later.

Suggested direction
Keep the per-peer queue limit private unless it is intended to be part of the responder module contract. If the test needs to drive past the limit without hardcoding 4, introduce a small test-owned scheduling configuration boundary or a focused helper rather than exposing this internal default from production code.

Confidence note
This is not a package-level barrel export, but it still widens the sync-handler module contract for test-only knowledge.

For Agents
Look at packages/agent/src/sync/responder/sync-handler.ts and the new protection test. Preserve the regression coverage, but avoid exporting a private limiter constant solely for the test. Prefer a test-local helper/factory path that owns the stress count, or make responder scheduling limits an explicit injected test configuration if the handler already wants that boundary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining — this is an encapsulation preference, not a defect. Adding export to a numeric const has no runtime effect, and it is not package-public: packages/agent/src/index.ts does not re-export sync/responder/sync-handler.js, so the constant is reachable only from inside the agent package (i.e. the tests). It also sits next to the already-exported SYNC_RESPONDER_DURABLE_DATA_SNAPSHOT_LIMIT / _META_SNAPSHOT_LIMIT, so the precedent is right there.

The test it enables is load-bearing, which is the point: it drives LIMIT+1 short-circuiting requests through one limiter and then requires a further request to actually be served. Delete any one releaseFirst() and reservations accumulate until the peer is permanently refused with "sync responder peer queue full" on an empty queue — the exact leak this PR exists to guard.

@branarakic
branarakic merged commit 8a1cecc into main Jul 14, 2026
48 checks passed
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