Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d127db109
ℹ️ 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".
| self._set_state(TunnelState.CONNECTING) | ||
|
|
||
| # 1. Get frpc binary | ||
| frpc_path = await asyncio.to_thread(get_frpc_path) |
There was a problem hiding this comment.
Revert state when frpc binary lookup fails
start() moves the tunnel to CONNECTING before resolving the frpc binary, but the get_frpc_path call is outside any cleanup/rollback path. If binary resolution/download raises, the method exits with an exception while state remains CONNECTING, so consumers and callbacks see a tunnel that never transitions back to STOPPED after a failed start.
Useful? React with 👍 / 👎.
| self._started = True | ||
| self._set_state(TunnelState.CONNECTED) |
There was a problem hiding this comment.
Don't unconditionally force CONNECTED after pipe-drain startup
After _start_pipe_drain() returns, start() always sets CONNECTED even though drain threads can already have observed process exit/disconnect and set a later state (e.g., STOPPED). In that interleaving, this assignment overwrites the newer state and leaves state reporting CONNECTED for a dead tunnel, which can mislead reconnection and health logic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8d127db. Configure here.
| if _CONNECTED_SIGNAL in clean: | ||
| self._set_state(TunnelState.CONNECTED) | ||
| return | ||
| if self._state == TunnelState.CONNECTED: |
There was a problem hiding this comment.
Unlocked state read creates race in log handler
Medium Severity
_handle_log_line reads self._state at line 183 without holding _state_lock, while every other read/write of _state (state property, _set_state) correctly acquires the lock. Since _handle_log_line is called from a background drain thread, a TOCTOU race exists: the drain thread can read _state as CONNECTED, then _cleanup or sync_stop on the main thread sets state to STOPPED, and finally the drain thread calls _set_state(DISCONNECTED) — resulting in an invalid STOPPED → DISCONNECTED transition and spurious callbacks.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8d127db. Configure here.


Note
Medium Risk
Adds a new public state/callback API and updates runtime behavior based on parsed subprocess logs, which could affect how clients observe connection status and may be sensitive to frpc log format changes.
Overview
Adds a tunnel lifecycle state machine to
Tunnelvia newTunnelState(STOPPED,CONNECTING,CONNECTED,DISCONNECTED) and aon_state_change/off_state_changecallback API, exported fromprime_tunnel.__init__.Updates
Tunnel.start,sync_stop,_cleanup, and the stdout/stderr drain threads to drive state transitions, including parsing frpc log lines to detect connect/disconnect signals and ensuring callback failures don’t break dispatch. Addstest_state_events.pycovering state transitions, log-driven reconnect patterns, and cleanup/stop integration.Reviewed by Cursor Bugbot for commit 8d127db. Bugbot is set up for automated code reviews on this repo. Configure here.