fix(conduct): close relay lifecycle cleanup gaps - #1693
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Multi-agent review roll call (CodeRabbit and Claude review automatically. Reviewers: post substantive findings only. Authors/agents: address every thread, push fixes to this branch, reply and resolve, then re-request review.) |
|
To use Codex here, create a Codex account and connect to github. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bba9c6a53f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and not ready_published | ||
| and terminal_code != "relay_ready_publication_uncertain" | ||
| ): |
There was a problem hiding this comment.
When _publish_ready_receipt raises relay_ready_publication_uncertain, this condition deliberately skips _rollback_relay_activation, but finish_nonready then terminates the provider process group. The call therefore returns with the activation marker present and the broker session still registered with accepting_work=True, even though its provider is dead; unless another caller explicitly retries launch_reserved_relay, the broker can continue assigning work to that dead session. Roll back the activation in the same cleanup path when termination is guaranteed, while preserving the publication uncertainty separately. This hard cancellation after provider start also conflicts with the repository's cooperative-stop contract.
AGENTS.md reference: AGENTS.md:L80-L82
Useful? React with 👍 / 👎.
| _rollback_relay_activation( | ||
| root, | ||
| current, | ||
| registration=registration, | ||
| deadline_monotonic=cleanup_deadline, | ||
| activation_lock_held=activation_registration_guard_held, |
There was a problem hiding this comment.
When activation rollback encounters slow worktree Git probes, lock acquisition, or broker registration, passing the full cleanup_deadline lets _rollback_relay_activation consume the five seconds supposedly reserved for terminal receipt persistence. finish_nonready then receives an already-expired cleanup_action_deadline, and _terminalize_relay may also have no time left, so the provider and broker state can be mutated without a durable terminal relay receipt. Bound rollback by cleanup_action_deadline and preserve the remaining budget for terminalization.
AGENTS.md reference: AGENTS.md:L375-L378
Useful? React with 👍 / 👎.
| _finish_drains( | ||
| (stdout_thread, stderr_thread), | ||
| deadline=deadline, | ||
| deadline=cleanup_action_deadline, | ||
| ) |
There was a problem hiding this comment.
If the proof descriptors reach EOF but a malformed wrapper or descendant keeps stdout or stderr open, _finish_drains now waits until cleanup_action_deadline, which is 55 seconds after the configured startup deadline. Only after that wait does line 1018 assign relay_startup_timeout, so even timeout_seconds=1 can block for roughly 56 seconds. The relay contract defines output-drain proof as part of the single absolute startup deadline (docs/architecture/campaign-successor-relay.md:38-40,49-50); use deadline for readiness proof, then use the cleanup budget only after the attempt has been declared non-ready and terminated.
Useful? React with 👍 / 👎.
4444J99
added
the
lifecycle:blocked
1 participant
Summary
Follow-up to #1669 closing the five remaining relay lifecycle review findings in one bounded correction batch:
The patch is limited to the relay protocol and its focused effector tests. It does not touch host configuration, credentials, provider sessions, custody state, or public receipt paths.
Verification
Success: no issues found in 1 source file.syntax-changed,diff-hygiene, direct-main writer contract, parameter hygiene, test hygiene, Ruff lint, Ruff format).pytest-cli/pytest-apiadmission was attempted once and denied before execution because the pressure sensor was unavailable (pressure-sensor-unavailable). Per the bounded verification contract it was not retried; remote CI is authoritative for the exact published head.Delivery
Normal review and merge queue only; no admin bypass, direct-main write, force push, or repeated base rewrite.