fix(agent)!: restrict package broker pipe access and report busy state

[Benoît Cortier (CBenoit)]

Standard users can no longer open the package broker pipe with generic write access. Package broker clients must open the pipe with GENERIC_READ | FILE_WRITE_DATA, as Devolutions.Now.Policy.Client does starting with the release that includes Devolutions/now-libraries#114, and UniGetUI starting with the release that adopts it.

The broker keeps a pipe instance listening at all times. When all connection slots are in use, or a user already holds 4 concurrent connections, the broker replies 503 Service Unavailable with a Retry-After header and a BrokerPaused error instead of leaving the pipe busy. Clients should retry these replies after the indicated delay. At most 16 busy replies are pending at once; beyond that, further clients are disconnected without a reply and should retry the connection later.

Do not merge before the Devolutions.Now.Policy.Client and UniGetUI releases above are available.

BREAKING CHANGE: package broker clients running as standard users must open the pipe without GENERIC_WRITE.

[Benoît Cortier (CBenoit)]

Implementation notes:

• This PR is a single commit that reverts cc9976a ("defer pipe access and busy-handling changes") from fix(agent): harden package broker input handling #2025. The revert applied cleanly; the resulting tree equals fix(agent): harden package broker input handling #2025 before the deferral, plus the later auth.rs rustdoc fix from fix(agent): harden package broker input handling #2025.
• Two tests that the deferral commit had added are gone again: the WaitNamedPipeW waiter test, which assumes no instance listens at capacity, and client_access_allows_generic_read_write_clients. client_access_rejects_generic_write_clients and the narrowed access-mask, instance-creation, and busy-reply tests are back.
• Kept from fix(agent): harden package broker input handling #2025:
  • instance-creation retry with backoff and first_pipe_instance(true) for the first instance;
  • connect-error backoff with rate-limited logging;
  • the 5 s request-header timeout.
• After startup, an instance always listens. The next instance is created before a connected one is handed off. Rejected clients are disconnected so their instance listens again. If creating the next instance fails, the current instance keeps listening during the backoff. Clients arriving in that window are disconnected without a reply, because sending one would mean handing off the only listening instance (accept_loop_keeps_listening_while_next_instance_creation_backs_off).
• agent-policy-tester already opens the pipe with GENERIC_READ | FILE_WRITE_DATA; only its doc comment changes.
• Validation: cargo +nightly fmt --all, cargo clippy --workspace --tests -- -D warnings, cargo test -p now-package-broker -p agent-policy-tester (all 19 pipe::tests pass).

Note

LLM-assisted content (no human feedback).

[Copilot]

Copilot review overview

🟡 Changes recommended

The listener retains an ERROR_PIPE_BUSY race, and per-user admission occurs too late to prevent global-slot exhaustion.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Acquire per-user slots before blocking client capture · New
• Maintain a spare listener during server handoff · New

What changed in this PR

Restricts package broker pipe permissions and adds explicit busy responses under connection pressure.

Changes:

• Limits standard-user pipe access and adds per-user connection caps.
• Returns HTTP 503 with Retry-After and BrokerPaused.
• Adds Windows access-control and concurrency tests.

File	Description
crates/​now-package-broker/​src/​server/​mod.rs	Adds the busy-response router.
crates/​now-package-broker/​src/​pipe.rs	Revises pipe ACLs, admission, listener handling, and tests.
crates/​agent-policy-tester/​src/​windows.rs	Clarifies required client access in comments.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Two listeners can still be exhausted by a three-client burst, exposing ERROR_PIPE_BUSY instead of HTTP 503.

Review effort: Balanced
Findings: 1

Open (1)

• Two listeners cannot guarantee the promised busy-response 503 · New

Resolved since last review (2)

• Acquire per-user slots before blocking client capture
• Maintain a spare listener during server handoff

[Copilot]

Copilot review overview

🔵 Needs a closer look

Exhausted busy-reply slots disconnect clients instead of returning the promised retryable 503 response.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Two listeners cannot guarantee the promised busy-response 503

Previously missed (1)

In code that hasn't changed since last review

Over-limit clients are disconnected without promised 503 Retry-After

crates/​now-package-broker/​src/​pipe.rs:366

This fallback contradicts the PR's busy-response contract: once 16 busy replies are waiting for request headers, the next over-limit client is disconnected here without the promised 503 and Retry-After. The dependent policy client retries only 503 responses carrying Retry-After, so an incomplete response will not follow the new retry path precisely during a sustained overload. Either preserve a retryable response for this case or document and coordinate the disconnect behavior with clients before release.

[Copilot]

Copilot review overview

🟡 Changes recommended

Overload paths can emit unbounded warning logs, and per-user admission lacks integrated coverage.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Rate-limit warnings for connections rejected by busy-reply cap · New
• Add coverage for user connection admission and slot reuse · New

[Copilot]

Copilot review overview

🔵 Needs a closer look

The security-sensitive concurrency changes have an unresolved misleading log and are explicitly gated on an unreleased client dependency.

Review effort: Balanced
Findings: 1

Open (1)

• Busy warning logged before reply permit acquisition · New

Resolved since last review (2)

• Rate-limit warnings for connections rejected by busy-reply cap
• Add coverage for user connection admission and slot reuse

[Copilot]

Copilot review overview

🔵 Needs a closer look

Synchronous retry waits can stall the accept loop despite an available spare listener.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Busy warning logged before reply permit acquisition

Previously missed (1)

In code that hasn't changed since last review

Replacement retry blocks accept loop and starves the healthy spare

crates/​now-package-broker/​src/​pipe.rs:290

If both disconnecting this failed instance and creating its replacement fail, awaiting the exponential retry pauses the entire accept loop even when spare.instance is still healthy. A client can connect to that spare during the 100 ms–30 s sleep, consume the only listener, and receive no response until the sleep ends; subsequent clients then see ERROR_PIPE_BUSY. Promote/continue polling the spare and schedule replacement asynchronously instead of sleeping in the accept loop.

This issue also appears on line 325 of the same file.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The Windows-only security and concurrency changes require human validation and coordination with the pending client release.

Review effort: Balanced
Findings: None

[Benoît Cortier (CBenoit)]

Changes made after review, beyond the plain revert:

• Per-user admission happens before the expensive identity capture. A cheap pid → token-user lookup takes the per-user slot first, and the full capture must report the same user (UserAdmission::serve, covered by an integration test).
• The accept loop keeps two listening instances and awaits both, so a client arriving during a handoff connects instead of getting ERROR_PIPE_BUSY. Bursts larger than the listener count still see ERROR_PIPE_BUSY briefly; NamedPipeClientStream.ConnectAsync waits that out.
• If a failed instance can't be disconnected or replaced, the spare takes over instead of the loop sleeping.
• Busy replies and rejections are logged at most once per minute, with a suppressed count.
• At most 16 busy replies are pending at once. Clients beyond that are disconnected without a reply; the PR body documents this.

Note

LLM-assisted content (no human feedback).

[Copilot]

Copilot review overview

🔵 Needs a closer look

It changes a Windows security boundary and connection admission state machine while depending on unreleased client support.

Review effort: Balanced
Findings: None