Repository navigation
Fix legacy retirement after listener handoff - #194
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe changes filter shutdown metadata from transport classification, preserve upstream timeout detection, and add guarded cleanup for inactive legacy control sockets. Retirement polling invokes cleanup after validating service and socket states. ChangesTransport timeout classification
Legacy control socket cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Retirement as finalize-legacy-retirement.sh
participant Installer as install-front-slots.sh
participant Systemd as systemd
participant SocketCheck as ss
Retirement->>Installer: cleanup-stopped-legacy-control
Installer->>Systemd: verify service and socket state
Installer->>SocketCheck: check socket ownership
SocketCheck-->>Installer: ownership result
Installer-->>Retirement: cleanup result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
d2a1cb1 to
f9b280d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/subrouter-transport-observer/golden_local_daemon.go`:
- Line 46: Update the assignment detection logic around notAssignment so it
skips horizontal whitespace between the key and delimiter before checking for
“=”, while preserving valid timeout parsing and rejecting malformed assignments.
Add a regression case covering “timeout = 10m0s” to ensure it is not recorded as
a transport timeout.
In `@cmd/subrouter/deploy_scripts_test.go`:
- Around line 2533-2544: The test helper run currently uses one enabled value
for both units, preventing coverage of the socket-unit branch in
cleanup-stopped-legacy-control. Update run to accept separate service and socket
enablement values, pass them to LEGACY_ENABLED and LEGACY_SOCKET_ENABLED, and
add a case with the service disabled and socket enabled that verifies the socket
remains.
In `@deploy/gcp/install-front-slots.sh`:
- Around line 713-718: Replace the is-enabled checks for both LEGACY_SERVICE and
LEGACY_SOCKET_SERVICE in deploy/gcp/install-front-slots.sh:713-718 with
list-unit-files --state=enabled queries, failing only when the requested unit is
explicitly reported as enabled and handling query failures separately. Apply the
same unit-state check in deploy/gcp/finalize-legacy-retirement.sh:186. Update
cmd/subrouter/deploy_scripts_test.go:2504-2510 to cover an explicit
query-failure state.
- Line 726: The socket ownership check in deploy/gcp/install-front-slots.sh at
lines 726-726 must query all Unix socket states rather than listening sockets
only; update the ss invocation while preserving ownership detection. In
cmd/subrouter/deploy_scripts_test.go at lines 2514-2518, update the ss mock to
reject listening-only queries for the target socket and add coverage for a
non-listening owned Unix socket.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d52c019b-f24d-4972-823c-18aabccee4ea
📒 Files selected for processing (5)
cmd/subrouter-transport-observer/golden_local_daemon.gocmd/subrouter-transport-observer/golden_runtime_continuity_test.gocmd/subrouter/deploy_scripts_test.godeploy/gcp/finalize-legacy-retirement.shdeploy/gcp/install-front-slots.sh
| end := index + len(token) | ||
| beforeBoundary := index == 0 || !goldenIdentifierByte(text[index-1]) | ||
| afterBoundary := end == len(text) || !goldenIdentifierByte(text[end]) | ||
| notAssignment := end == len(text) || text[end] != '=' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject whitespace-separated assignments.
Line 46 only rejects timeout=.... It accepts timeout = 10m0s and records a false transport timeout. This can fail normal shutdown flows when log formatting includes spaces around =.
Skip horizontal whitespace before checking for =. Add a regression case for timeout = 10m0s.
Proposed fix
- notAssignment := end == len(text) || text[end] != '='
+ assignmentOffset := end
+ for assignmentOffset < len(text) &&
+ (text[assignmentOffset] == ' ' || text[assignmentOffset] == '\t') {
+ assignmentOffset++
+ }
+ notAssignment := assignmentOffset == len(text) || text[assignmentOffset] != '='🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/subrouter-transport-observer/golden_local_daemon.go` at line 46, Update
the assignment detection logic around notAssignment so it skips horizontal
whitespace between the key and delimiter before checking for “=”, while
preserving valid timeout parsing and rejecting malformed assignments. Add a
regression case covering “timeout = 10m0s” to ensure it is not recorded as a
transport timeout.
| if systemctl is-enabled --quiet "${LEGACY_SERVICE}" >/dev/null 2>&1; then | ||
| die "refusing to remove the enabled legacy control socket" | ||
| fi | ||
| if systemctl is-enabled --quiet "${LEGACY_SOCKET_SERVICE}" >/dev/null 2>&1; then | ||
| die "refusing to remove the legacy control socket while ${LEGACY_SOCKET_SERVICE} is enabled" | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
if ! command -v systemctl >/dev/null; then
echo "systemctl is unavailable on this host"
exit 0
fi
set +e
systemctl is-enabled --quiet definitely-missing-subrouter-test.service
status=$?
set -e
printf 'systemctl is-enabled exit status for an unresolved unit: %s\n' "$status"Repository: manaflow-ai/subrouter
Length of output: 197
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Repository files matching targets:\n'
git ls-files | rg '(^deploy/gcp/install-front-slots\.sh$|^deploy/gcp/finalize-legacy-retirement\.sh$|^cmd/subrouter/deploy_scripts_test\.go$)' || true
printf '\nLine count targets:\n'
for f in deploy/gcp/install-front-slots.sh deploy/gcp/finalize-legacy-retirement.sh cmd/subrouter/deploy_scripts_test.go; do
[ -f "$f" ] && wc -l "$f"
done
printf '\nRelevant shell lines:\n'
sed -n '690,730p' deploy/gcp/install-front-slots.sh
printf '\n'
sed -n '165,200p'部署/gcp/finalize-legacy-retirement.sh 2>/dev/null || sed -n '165,200p' deploy/gcp/finalize-legacy-retirement.sh
printf '\nRelevant test lines:\n'
sed -n '2480,2525p' cmd/subrouter/deploy_scripts_test.goRepository: manaflow-ai/subrouter
Length of output: 6563
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Repository files matching targets:\n'
git ls-files | rg '(^deploy/gcp/install-front-slots\.sh$|^deploy/gcp/finalize-legacy-retirement\.sh$|^cmd/subrouter/deploy_scripts_test\.go$)' || true
printf '\nLine count targets:\n'
for f in deploy/gcp/install-front-slots.sh deploy/gcp/finalize-legacy-retirement.sh cmd/subrouter/deploy_scripts_test.go; do
[ -f "$f" ] && wc -l "$f"
done
printf '\nRelevant shell lines:\n'
sed -n '690,730p' deploy/gcp/install-front-slots.sh
printf '\n'
sed -n '165,200p' deploy/gcp/finalize-legacy-retirement.sh
printf '\nRelevant test lines:\n'
sed -n '2480,2525p' cmd/subrouter/deploy_scripts_test.goRepository: manaflow-ai/subrouter
Length of output: 6563
🌐 Web query:
systemd systemctl is-enabled exit status invalid disabled documentation
💡 Result:
The systemctl is-enabled command checks whether the specified unit files are enabled [1][2]. Its exit status is determined by the following rules: 1. Exit Code 0: The command returns 0 if at least one of the specified unit files is considered enabled [3][1][4]. This includes states such as enabled, static, indirect, alias, and transient [1][2]. 2. Non-zero Exit Code: The command returns a non-zero value if none of the specified units are enabled (e.g., states like disabled, masked, or linked) [1][2][4]. Regarding the states mentioned: - Disabled: If a unit is in the disabled state, it generally results in a non-zero exit status (> 0) [1][2]. - Invalid (Bad): The is-enabled command does not return a specific bad state exit code [1][2]. If a unit file is invalid or another error occurs, the command typically prints an error message to standard error instead of returning a status associated with the "bad" state [1][2]. - Not Found: If a unit file does not exist, the command may return an exit code of 4 (in addition to printing "not-found" to standard output) [2]. Important Note on Multiple Arguments: The command's exit status logic remains the same even when multiple unit names are provided; it returns 0 as long as at least one of the units provided is enabled [4]. Documentation for the exact exit codes is available in the official systemctl(1) manual page [3][5].
Citations:
- 1: https://www.freedesktop.org/software/systemd/man/252/systemctl.html
- 2: https://manpages.debian.org/trixie/systemd/systemctl.1.en.html
- 3: https://www.freedesktop.org/software/systemd/man/systemctl.html
- 4: systemctl is-active / is-enabled exit status code is always zero when multiple arguments are given systemd/systemd#11826
- 5: https://man7.org/linux/man-pages/man1/systemctl.1.html
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'All references to legacy enablement checks and tests:\n'
rg -n 'is-enabled|LEGACY_ENABLED|LEGACY_SOCKET_ENABLED|cleanup_stopped_legacy_control|TestFrontSlotInstallerRemovesOnlyInactiveLegacyControlSocket|test.*systemctl.*is-enabled' deploy/gcp cmd/subrouter/deploy_scripts_test.go
printf '\nAll cleanup_stopped_legacy_control definitions/usages:\n'
rg -n 'cleanup_stopped_legacy_control' deploy/gcp cmd/subrouter/deploy_scripts_test.goRepository: manaflow-ai/subrouter
Length of output: 5604
Check the unit file state instead of relying on systemctl is-enabled --quiet failure.
systemctl is-enabled --quiet returns non-zero for many non-enabled states, including errors such as not-found. Use systemctl list-unit-files --state=enabled <unit>, then confirm the result is absent; only fail when enabled is present. Apply the same check to both legacy units in deploy/gcp/install-front-slots.sh#L713-L718 and deploy/gcp/finalize-legacy-retirement.sh#L186; cover an explicit query-failure state in cmd/subrouter/deploy_scripts_test.go.
📍 Affects 3 files
deploy/gcp/install-front-slots.sh#L713-L718(this comment)deploy/gcp/finalize-legacy-retirement.sh#L186-L186cmd/subrouter/deploy_scripts_test.go#L2504-L2510
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gcp/install-front-slots.sh` around lines 713 - 718, Replace the
is-enabled checks for both LEGACY_SERVICE and LEGACY_SOCKET_SERVICE in
deploy/gcp/install-front-slots.sh:713-718 with list-unit-files --state=enabled
queries, failing only when the requested unit is explicitly reported as enabled
and handling query failures separately. Apply the same unit-state check in
deploy/gcp/finalize-legacy-retirement.sh:186. Update
cmd/subrouter/deploy_scripts_test.go:2504-2510 to cover an explicit
query-failure state.
578251c to
310a0b9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/subrouter-transport-observer/golden_local_daemon.go (1)
36-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct coverage for
deadline exceeded.The supplied regression test covers
"timeout","timeouts", and"timeout=true", but not the new"deadline exceeded"branch. Add a case that assertslocal_daemon_transport_issue_timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/subrouter-transport-observer/golden_local_daemon.go` around lines 36 - 38, Add a regression-test case for input containing “deadline exceeded” without an existing timeout marker, and assert that the result includes local_daemon_transport_issue_timeout. Keep the existing timeout, timeouts, and timeout=true cases unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/subrouter-transport-observer/golden_local_daemon.go`:
- Around line 36-38: Add a regression-test case for input containing “deadline
exceeded” without an existing timeout marker, and assert that the result
includes local_daemon_transport_issue_timeout. Keep the existing timeout,
timeouts, and timeout=true cases unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e3e2399-3d54-4b27-aab9-777b6dae59aa
📒 Files selected for processing (5)
cmd/subrouter-transport-observer/golden_local_daemon.gocmd/subrouter-transport-observer/golden_runtime_continuity_test.gocmd/subrouter/deploy_scripts_test.godeploy/gcp/finalize-legacy-retirement.shdeploy/gcp/install-front-slots.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/subrouter/deploy_scripts_test.go
- cmd/subrouter-transport-observer/golden_runtime_continuity_test.go
- deploy/gcp/finalize-legacy-retirement.sh
8d214f7 to
3e829d0
Compare
3e829d0 to
ede46db
Compare
ede46db to
09ca5fa
Compare
1 participant
Fixes the production continuity gate after a successful listener handoff.
Regression history:
f2a6e69adds the failing tests09ca5faimplements the fixVerification:
go test ./...09ca5fa