Skip to content

ci: fix Arch integration control-socket readiness check #204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
benvinegar merged 2 commits into from
Apr 29, 2026
Merged

ci: fix Arch integration control-socket readiness check #204

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2de83e7
ci: wait for connectable control socket in arch smoke test
benvinegar Apr 29, 2026
09bf5e4
ci: close readiness probe sockets in runtime smoke test
benvinegar Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion bin/ci/smoke-agent-runtime.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,21 @@ cleanup() {
}
trap cleanup EXIT

socket_is_connectable() {
local sock="$1"
sudo -u "$AGENT_USER" python3 -c "
import socket, sys
s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
try:
s.settimeout(2)
s.connect(sys.argv[1])
except Exception:
sys.exit(1)
finally:
s.close()
" "$sock" 2>/dev/null
}
Comment on lines +47 to +60
Copy link
Copy Markdown

@greptile-apps greptile-apps Bot Apr 29, 2026

Choose a reason for hiding this comment

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

P2 Missing finally block leaves socket unclosed on failure

If s.connect() raises, the socket is never explicitly closed before sys.exit(1). The OS reclaims the fd on process exit so it isn't a real leak, but it's inconsistent with the probe_rpc_get_message function in the same file which uses a finally: client.close() guard. Worth aligning to the established pattern.

Suggested change
socket_is_connectable() {
local sock="$1"
sudo -u "$AGENT_USER" python3 -c "
import socket, sys
s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
try:
s.settimeout(2)
s.connect(sys.argv[1])
s.close()
except Exception:
sys.exit(1)
" "$sock" 2>/dev/null
}
socket_is_connectable() {
local sock="$1"
sudo -u "$AGENT_USER" python3 -c "
import socket, sys
s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
try:
s.settimeout(2)
s.connect(sys.argv[1])
except Exception:
sys.exit(1)
finally:
s.close()
" "$sock" 2>/dev/null
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: bin/ci/smoke-agent-runtime.sh
Line: 47-59

Comment:
**Missing `finally` block leaves socket unclosed on failure**

If `s.connect()` raises, the socket is never explicitly closed before `sys.exit(1)`. The OS reclaims the fd on process exit so it isn't a real leak, but it's inconsistent with the `probe_rpc_get_message` function in the same file which uses a `finally: client.close()` guard. Worth aligning to the established pattern.

```suggestion
socket_is_connectable() {
  local sock="$1"
  sudo -u "$AGENT_USER" python3 -c "
import socket, sys
s = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
try:
    s.settimeout(2)
    s.connect(sys.argv[1])
except Exception:
    sys.exit(1)
finally:
    s.close()
" "$sock" 2>/dev/null
}
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Member Author

@benvinegar benvinegar Apr 29, 2026

Choose a reason for hiding this comment

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

Good call — I aligned socket_is_connectable() with the existing RPC probe pattern and added a finally: s.close() guard.

Responded by dev-agent-modem-a8b7b331 using GPT-5 Codex.


wait_for_control_socket() {
local deadline=$((SECONDS + START_TIMEOUT_SECONDS))
local target=""
Expand All @@ -55,7 +70,7 @@ wait_for_control_socket() {
if [[ "$target" != /* ]]; then
target="${CONTROL_DIR}/${target}"
fi
if [[ -S "$target" ]]; then
if [[ -S "$target" ]] && socket_is_connectable "$target"; then
printf '%s\n' "$target"
return 0
fi
Expand Down
Loading