Repository navigation
fix: keep relay tests from flooding the host and racing the relay - #153
Conversation
- The flood test queued about 1,000 descriptors in flight over Unix sockets. In-flight descriptors are a per-user budget shared with the desktop; a looped run made kitty's Wayland connection fail with ETOOMANYREFS and close every session in it. The per-connection cap is now a Limits field, and the tests use caps of 56. - The shared-budget case assumed the relay had counted the first connection's descriptors; it now waits until they are read. - Relay holders connect upstream only after their pid mapping is recorded, so a compositor never sees an unmapped holder. - A connection reset counts as closed in the test helper.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 3080cb7d-1c09-4a8c-97fe-5f26b17dd506) |
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.
![revuto-review[bot]](https://avatars.githubusercontent.com/in/4488771?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is an auto review done by revuto.
The holder handshake and production limits look consistent, but the shared-budget test still has a synchronization race.
Retry the go-byte read on EINTR, send it with MSG_NOSIGNAL, refuse to proceed when the pid map is poisoned, and make the flood test assert the relay does not close before the cap.
|
Self-review (independent fresh-context reviewer): nothing serious. Production behavior unchanged except the holder handshake, which is async-signal-safe, cannot deadlock or leak the holder, and is bounded by the existing 5 s receive timeout. Its P3 notes are applied in the head commit (EINTR retry, MSG_NOSIGNAL, poisoned pid map refuses, a real assertion in the flood test); the SIOCOUTQ-to-count gap is documented as far shorter than the next connect. Validation: fmt, clippy -D warnings, 254 tests in both TMPDIR modes, 40 consecutive relay-test runs. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: ddf7d04e-749e-4dea-a48c-b7d29f026dc1) |
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.
There was a problem hiding this comment.
This is an auto review done by revuto.
The shared-budget test still has the previously reported race; the new timing assumption does not synchronize the relay threads.
SIOCOUTQ reaches zero before the pump adds the descriptors to the shared budget, so the second connection could still be counted first. The budget is now built outside accept_loop and the test harness waits on its counter.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 0e4fa240-e94d-4ee8-917b-5a531a8eecf9) |
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing. Review the Copilot code review billing settings.
There was a problem hiding this comment.
This is an auto review done by revuto.
Revuto completed the review and found no evidence-backed concerns.
Found while cutting 0.5.0 (#152 CI failed on a relay test).
closes_clients_that_queue_descriptors_without_messagesqueued about 1,000 descriptors in flight. In-flight SCM_RIGHTS descriptors are a per-user budget (ETOOMANYREFS past the sender's RLIMIT_NOFILE) shared with the desktop session: a looped local run made kitty's Wayland connection fail and kitty exited. The per-connection cap is now aLimitsfield (production value unchanged, 1024) and the tests use 56, so no test keeps more than about 100 descriptors in flight.closed()treats ECONNRESET as closed.Validation: fmt, clippy -D warnings, 254 tests (default and symlinked TMPDIR), 40 consecutive relay-test runs without a failure.
Note
Medium Risk
Production relay path changes holder startup synchronization (pid mapping before upstream connect); limits are unchanged but fork/control-socket behavior is security-relevant for window pid attribution.
Overview
Fixes relay correctness and test hygiene in the sandbox Wayland filter.
Holder connect ordering: The parent now inserts the holder→client pid mapping in
PEERSbefore the holder connects upstream, signaled by a one-byte “go” on the control socket. The holder blocks on that byte so the compositor never sees a connection without a mapping (addressing CI failures from #152).Descriptor budgets:
Limitsgainsper_connection_fds(still 1024 in production viafrom_rlimit).Budgetis created once per accept loop and carries connection and per-connection caps;pumpenforces the per-connection limit throughbudget.per_connectioninstead of a hardcoded constant.Tests: Descriptor-flood cases use caps of ~56 in flight instead of ~1000, avoiding shared per-user SCM_RIGHTS limits that could break the desktop session. The shared-budget scenario uses
Harness::wait_queuedso the relay has counted the first connection’s fds before the second connects.closed()treatsECONNRESETas a closed socket.Reviewed by Cursor Bugbot for commit fb5213d. Configure here.