Repository navigation
Make hosted cutovers zero-gap - #186
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds listener transfer and takeover support, updates deployment scripts for resumable listener handoff, and replaces rehearsal/rollback migration phases with one final cutover. Golden continuity now validates listener ownership evidence and supports OAuth account propagation with v0.1.69. ChangesListener handoff migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant switch-front-migration.sh
participant legacyService
participant stableFront
participant validate-deploy-evidence.py
participant finalize-legacy-retirement.sh
switch-front-migration.sh->>legacyService: capture listener PID, FD, and inode
switch-front-migration.sh->>stableFront: activate listener-FD takeover
stableFront->>switch-front-migration.sh: report inherited socket and preserved connections
switch-front-migration.sh->>legacyService: retire source listener
switch-front-migration.sh->>validate-deploy-evidence.py: emit final-cutover evidence
validate-deploy-evidence.py->>finalize-legacy-retirement.sh: validate handoff and retirement evidence
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
cmd/subrouter-transport-observer/golden_options_test.go (1)
39-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a negative case for
v0.1.68.This test accepts
v0.1.69and rejectsv0.1.63. It does not prove that the previousv0.1.68candidate is rejected. Add that case so the test enforces an exact release pin instead of only rejecting the bootstrap candidate.🤖 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_options_test.go` around lines 39 - 47, Add a negative test case in the existing golden-argument test around parseGoldenArgs that replaces the accepted v0.1.69 candidate with v0.1.68 and asserts parsing fails, matching the existing v0.1.63 rejection assertion so the exact release pin is enforced.cmd/subrouter/listener_transfer_unix.go (1)
53-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the no-op
continueand log the failed transfer.The
continueat Line 56 is the last statement in the loop body, so it changes nothing. More important,requestErris discarded without a log line. A rejected or failed descriptor transfer then leaves no trace on the front, which makes a failed cutover hard to diagnose. Log the error at warning level. Do not log the request body.♻️ Proposed change
requestErr := f.handleListenerTransfer(connection) _ = connection.Close() if requestErr != nil { - continue + slog.Warn("front listener transfer failed", "error", requestErr) } }Add
"log/slog"to the imports.🤖 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/listener_transfer_unix.go` around lines 53 - 58, Update the listener transfer loop around handleListenerTransfer to remove the no-op continue and log any non-nil requestErr at warning level using slog, without logging the request body. Add the required log/slog import and preserve connection.Close behavior.cmd/subrouter/listener_address.go (1)
19-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the port comparison and confirm the wildcard rule is intentional.
Two points in this function:
- Line 15 already rejects a port mismatch by string comparison. The integer re-parse at lines 26-28 can therefore never change the port verdict. Its only remaining effect is to reject non-numeric ports. Drop the re-parse and compare hosts directly.
- A wildcard
expectedHostof"",0.0.0.0, or::matches any actual host. A listener bound to127.0.0.1:31416therefore satisfies an expected address of0.0.0.0:31416. The takeover incmd/subrouter/listener_takeover_linux.goLine 50 then reports success for a socket that is not publicly reachable. Confirm that this permissive direction is intended for the handoff validation.♻️ Proposed simplification
actualIP, expectedIP := net.ParseIP(actualHost), net.ParseIP(expectedHost) if actualIP != nil && expectedIP != nil { return actualIP.Equal(expectedIP) } - actualPortNumber, actualErr := strconv.Atoi(actualPort) - expectedPortNumber, expectedErr := strconv.Atoi(expectedPort) - return actualErr == nil && expectedErr == nil && actualPortNumber == expectedPortNumber && - strings.EqualFold(actualHost, expectedHost) + return strings.EqualFold(actualHost, expectedHost) }Remove the
strconvimport if you apply this change.🤖 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/listener_address.go` around lines 19 - 29, Update the listener address comparison to remove the redundant strconv-based port parsing and compare the already-validated port values directly, removing the unused strconv import. Also review the wildcard handling in the listener address function and confirm whether expectedHost values "", "0.0.0.0", and "::" should intentionally match every actual host; if not, restrict the wildcard behavior so takeover validation cannot accept a non-publicly reachable listener.cmd/subrouter/main.go (1)
1364-1364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the
listener-transfersubcommand in the usage text.
runForProgramnow dispatcheslistener-transfer, anddeploy/gcp/install-front-slots.shanddeploy/gcp/smoke-listener-takeover.shinvoke it directly. The usage text documents only thefront --listener-transfer-socketflag. Add one line so operators can discover the helper and its required flags.📝 Proposed usage addition
%[1]s front --backend-id ID --backend-address ADDRESS [--backend-network tcp|unix] [--addr 127.0.0.1:31415] [--control-socket /var/run/subrouter-front.sock] [--listener-transfer-socket /var/run/subrouter-front-listener.sock] + %[1]s listener-transfer --socket PATH --address ADDRESS --source-pid PID --source-fd FD🤖 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/main.go` at line 1364, Update the usage text in runForProgram’s help output to add a dedicated listener-transfer subcommand entry, documenting its required flags, alongside the existing front usage line. Keep the existing front documentation unchanged.deploy/gcp/switch-front-migration.sh (1)
330-332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe persistent sampler leaks if the run fails after the commit point.
cleanupstops${persistent_legacy_sampler_unit}only whentransition_committedis0. This is intentional sofinalize-legacy-retirement.shcan adopt it. If a later step of this script fails after Line 403, the unit and its sentinel stay on the instance with no adopter, and a subsequent run creates another unit. Consider stopping the sampler on a non-zero exit status even after the commit, or documenting the manual cleanup path.🤖 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/switch-front-migration.sh` around lines 330 - 332, Update cleanup in switch-front-migration.sh so a non-zero exit status also stops the persistent legacy sampler after transition_committed is set, preventing orphaned units and sentinels when post-commit steps fail. Preserve the existing adoption behavior for successful committed runs and ensure the cleanup condition uses the script’s status variable.deploy/gcp/validate-deploy-evidence.py (1)
875-875: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the useless conditional.
Both branches return
legacy_url, so the condition has no effect. Ruff reports RUF034. The value is correct for both cases: after listener-fd takeover the front destination still serves the legacy backend URL, and a rollback destination is the legacy backend. Simplify the expression and keep the intent in a comment.♻️ Proposed refactor
- expected_destination_url = legacy_url if destination_kind == "front" else legacy_url + # Listener-fd takeover keeps the legacy backend URL for both destinations. + expected_destination_url = legacy_url🤖 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/validate-deploy-evidence.py` at line 875, Replace the conditional assignment to expected_destination_url with a direct legacy_url assignment, and add a concise comment preserving why both destination kinds use the legacy backend URL after listener-fd takeover or rollback.Source: Linters/SAST tools
cmd/subrouter-transport-observer/golden_acceptance_regression_test.go (1)
55-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the listener-retirement-before-proof rule in isolation.
SourceListenerRetiredAt = "2026-08-02T00:00:01.1Z"is afterActivatedAt = "2026-08-02T00:00:01Z", so the generic timestamp ordering rejects this input before thelistener_retired <= proof_receivedcheck runs. Set retirement after activation but beforeReceivedAt, or add a second case that passes ordering and violates only that rule.🤖 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_acceptance_regression_test.go` around lines 55 - 63, Update TestGoldenMigrationRequiresListenerRetirementBeforeFreshPublicProof so SourceListenerRetiredAt remains after ActivatedAt but occurs before ReceivedAt, ensuring generic timestamp validation passes and only the listener-retirement-before-proof rule causes rejection; alternatively add a separate case with that isolated ordering.
🤖 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_migration_evidence.go`:
- Around line 518-520: Add a lower-bound validation for listenerRetired in the
ordering condition around parseGoldenEvidenceTime: require
listenerRetired.Before(requested) alongside the existing upper-bound checks,
ensuring SourceListenerRetiredAt is not missing or earlier than
TransitionRequestedAt while preserving the other timestamp validations.
In `@cmd/subrouter-transport-observer/golden_options_test.go`:
- Line 27: Replace the personal-looking account identifier in the golden options
test fixture with the synthetic value test-account@example.com everywhere it
appears, including all arguments and assertions associated with the test.
- Around line 82-83: The failure in the golden child environment assertion must
not print the full environment. Update the check around
containsGoldenEnvironment to report only redacted evidence, such as whether the
expected SUBROUTER_CODEX_ACCOUNT_ID key is present, while preserving the
existing failure behavior.
In `@cmd/subrouter/front.go`:
- Around line 199-201: Replace context-free networking calls to satisfy noctx:
update openFreshPublicListener in cmd/subrouter/front.go:199-201 to use
net.ListenConfig.Listen with a context; update httptest.NewRequest at
cmd/subrouter/front_takeover_test.go:37-37 to its context-aware variant; and
replace net.DialTimeout at cmd/subrouter/front_takeover_test.go:90-90,
cmd/subrouter/front_takeover_test.go:182-182,
cmd/subrouter/listener_transfer_unix_test.go:78-78, and
cmd/subrouter/listener_transfer_unix_test.go:82-82 with DialContext using a
configured timeout. Share one dial helper between both test files, placing it in
a file compatible with the Unix-only build constraint, then run go test ./....
- Around line 209-216: Prevent shutdown from deadlocking in the listener
lifecycle around startServingLocked, runOnListeners, and listenerWG.Wait: ensure
retired listener goroutines cannot block when reporting to the bounded
f.listenerResults channel. Either make their result sends non-blocking or drain
f.listenerResults concurrently during shutdown while waiting for listenerWG,
preserving normal result processing.
In `@deploy/gcp/finalize-legacy-retirement.sh`:
- Around line 100-103: Validate accepting_new_public_false_at immediately after
extracting it, rejecting empty or literal "null" values with a clear error via
die before emitting retirement evidence. Keep the existing cutover_run_label
validation unchanged.
- Around line 158-176: Update adopt_legacy_sampler to assert that sampler_unit
is active before accepting sampler_result and sampler_oom_result. Record a
baseline sample count or result-file mtime at adoption, then update
stop_legacy_sampler to verify the corresponding value changed during the drain
window before trusting the metrics; retain the existing numeric validation and
sampler_adopted state handling.
In `@deploy/gcp/install-front-slots.sh`:
- Around line 385-393: Update the listener_status probes in
restore_front_bootstrap so each call runs in a subshell, containing the
die-induced exit and allowing the elif recovery branch to execute. Preserve the
existing checks for ports 31416 and 31415 and the replace-listener behavior.
- Around line 297-302: Declare python3 as a required dependency in both listener
scripts: in deploy/gcp/install-front-slots.sh lines 297-302, add a command check
beside the existing systemd-run check with the specified listener acceptance
probe error; in deploy/gcp/smoke-listener-takeover.sh lines 24-26, include
python3 in the command dependency list used by the script’s guard.
- Around line 360-371: Update the listener acceptance verification around the
existing acceptance loop to capture the pre-transfer
listener.accepted_connections value before systemd-run replaces/transfers the
listener, then require the post-transfer accepted count to be greater than that
baseline. Preserve the existing numeric validation, retry behavior, and failure
message while changing both the loop termination and final check from accepted >
0 to accepted > accepted_before.
In `@deploy/gcp/switch-front-migration.sh`:
- Around line 368-374: Strengthen the jq validation in the listener-status
parsing block near legacy_listener so .inode must match the safe expected inode
format, not merely have string type, before assigning source_listener_inode.
Keep the existing PID and FD numeric checks and service/port validation
unchanged, and ensure the validated inode can be safely interpolated into the
remote command used later.
---
Nitpick comments:
In `@cmd/subrouter-transport-observer/golden_acceptance_regression_test.go`:
- Around line 55-63: Update
TestGoldenMigrationRequiresListenerRetirementBeforeFreshPublicProof so
SourceListenerRetiredAt remains after ActivatedAt but occurs before ReceivedAt,
ensuring generic timestamp validation passes and only the
listener-retirement-before-proof rule causes rejection; alternatively add a
separate case with that isolated ordering.
In `@cmd/subrouter-transport-observer/golden_options_test.go`:
- Around line 39-47: Add a negative test case in the existing golden-argument
test around parseGoldenArgs that replaces the accepted v0.1.69 candidate with
v0.1.68 and asserts parsing fails, matching the existing v0.1.63 rejection
assertion so the exact release pin is enforced.
In `@cmd/subrouter/listener_address.go`:
- Around line 19-29: Update the listener address comparison to remove the
redundant strconv-based port parsing and compare the already-validated port
values directly, removing the unused strconv import. Also review the wildcard
handling in the listener address function and confirm whether expectedHost
values "", "0.0.0.0", and "::" should intentionally match every actual host; if
not, restrict the wildcard behavior so takeover validation cannot accept a
non-publicly reachable listener.
In `@cmd/subrouter/listener_transfer_unix.go`:
- Around line 53-58: Update the listener transfer loop around
handleListenerTransfer to remove the no-op continue and log any non-nil
requestErr at warning level using slog, without logging the request body. Add
the required log/slog import and preserve connection.Close behavior.
In `@cmd/subrouter/main.go`:
- Line 1364: Update the usage text in runForProgram’s help output to add a
dedicated listener-transfer subcommand entry, documenting its required flags,
alongside the existing front usage line. Keep the existing front documentation
unchanged.
In `@deploy/gcp/switch-front-migration.sh`:
- Around line 330-332: Update cleanup in switch-front-migration.sh so a non-zero
exit status also stops the persistent legacy sampler after transition_committed
is set, preventing orphaned units and sentinels when post-commit steps fail.
Preserve the existing adoption behavior for successful committed runs and ensure
the cleanup condition uses the script’s status variable.
In `@deploy/gcp/validate-deploy-evidence.py`:
- Line 875: Replace the conditional assignment to expected_destination_url with
a direct legacy_url assignment, and add a concise comment preserving why both
destination kinds use the legacy backend URL after listener-fd takeover or
rollback.
🪄 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: ef3632c9-a650-48e8-a6f8-7405e8357894
📒 Files selected for processing (31)
cmd/subrouter-transport-observer/golden.gocmd/subrouter-transport-observer/golden_acceptance_regression_test.gocmd/subrouter-transport-observer/golden_counter_continuity.gocmd/subrouter-transport-observer/golden_deployment_action_lifecycle_unix_test.gocmd/subrouter-transport-observer/golden_local_mac_harness_test.gocmd/subrouter-transport-observer/golden_migration_cycle.gocmd/subrouter-transport-observer/golden_migration_evidence.gocmd/subrouter-transport-observer/golden_migration_transition.gocmd/subrouter-transport-observer/golden_options_test.gocmd/subrouter-transport-observer/golden_runtime_continuity_test.gocmd/subrouter-transport-observer/testdata/golden_fake_client/main.gocmd/subrouter/deploy_scripts_test.gocmd/subrouter/front.gocmd/subrouter/front_takeover_test.gocmd/subrouter/listener_address.gocmd/subrouter/listener_takeover_linux.gocmd/subrouter/listener_takeover_other.gocmd/subrouter/listener_takeover_test.gocmd/subrouter/listener_transfer_other.gocmd/subrouter/listener_transfer_unix.gocmd/subrouter/listener_transfer_unix_test.gocmd/subrouter/main.gocmd/subrouter/supervisor.godeploy/gcp/finalize-legacy-retirement.shdeploy/gcp/golden-local-mac-production-continuity.shdeploy/gcp/install-front-slots.shdeploy/gcp/migrate-to-front-slots.shdeploy/gcp/smoke-listener-takeover.shdeploy/gcp/switch-front-migration.shdeploy/gcp/validate-deploy-evidence.pygo.mod
💤 Files with no reviewable changes (1)
- cmd/subrouter-transport-observer/golden_counter_continuity.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
deploy/gcp/deploy-lock.sh (1)
124-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the preserved sentinel set instead of listing targets.
The preserve branch removes only the
front,slot-a, andslot-bmarkers. Any future sampler target gains an unremoved/tmp/subrouter-rss-${owner_cleanup_label}-*.runningmarker on lock release. Invert the rule and remove every marker except the legacy one, so new targets are cleaned by default.♻️ Proposed refactor
- owner_cleanup_command="trap \"if test -f ${owner_cleanup_preserve_path}; then rm -f -- /tmp/subrouter-rss-${owner_cleanup_label}-front.running /tmp/subrouter-rss-${owner_cleanup_label}-slot-a.running /tmp/subrouter-rss-${owner_cleanup_label}-slot-b.running; else rm -f -- /tmp/subrouter-rss-${owner_cleanup_label}-*.running; fi; rm -f -- /tmp/install-front-slots-${owner_cleanup_label}.sh /tmp/deployment-contract-${owner_cleanup_label}.py\" EXIT;" + owner_cleanup_command="trap \"for marker in /tmp/subrouter-rss-${owner_cleanup_label}-*.running; do test -e \\\"\\\${marker}\\\" || continue; if test -f ${owner_cleanup_preserve_path} && test \\\"\\\${marker}\\\" = /tmp/subrouter-rss-${owner_cleanup_label}-legacy.running; then continue; fi; rm -f -- \\\"\\\${marker}\\\"; done; rm -f -- /tmp/install-front-slots-${owner_cleanup_label}.sh /tmp/deployment-contract-${owner_cleanup_label}.py\" EXIT;"🤖 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/deploy-lock.sh` around lines 124 - 128, Update the preserve branch of the owner_cleanup_command trap to remove all /tmp/subrouter-rss-${owner_cleanup_label}-*.running markers except the legacy preserved sentinel, rather than hard-coding front, slot-a, and slot-b. Keep removal of the install and deployment contract files unchanged, and leave the non-preserve branch behavior intact.cmd/subrouter/deploy_scripts_test.go (1)
1146-1160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe fake
gcloudreimplements the preserve rule, so the real trap body is not covered.Lines 1149-1156 decide which sentinels to remove inside the test double. The test therefore proves that
subrouter_acquire_deploy_lockforwards the preserve path, not that the trap emitted bydeploy-lock.shremoves the correct markers. If the trap body dropsslot-aor preserves the wrong marker, this test still passes.Assert on the emitted remote command, or execute the trap body from
remote_commanddirectly.💚 Proposed change to run the real trap body
remote_command="$*" cleanup() { - if [[ "${remote_command}" == *"test -f ${REMOTE_PRESERVE_MARKER}"* && -f "${REMOTE_PRESERVE_MARKER}" ]]; then - unlink "${REMOTE_FRONT_SENTINEL}" 2>/dev/null || true - else - unlink "${REMOTE_FRONT_SENTINEL}" 2>/dev/null || true - unlink "${REMOTE_LEGACY_SENTINEL}" 2>/dev/null || true - fi + # Execute the trap body that deploy-lock.sh generated instead of restating it. + trap_body="${remote_command#*bash -c \'trap \"}" + trap_body="${trap_body%%\" EXIT;*}" + eval "${trap_body//sudo /}" }The rewritten double still needs the sentinel paths to point at the temporary directory. Adjust the generated marker prefix in the harness if you take this direction.
🤖 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/deploy_scripts_test.go` around lines 1146 - 1160, Update the fake gcloud harness in writeExecutableTestFile so it no longer reimplements the preserve-rule cleanup logic. Instead, execute or otherwise assert against the trap body contained in remote_command, ensuring the real deploy-lock.sh trap removes and preserves the correct sentinels; when executing it, rewrite the generated marker prefix so sentinel paths resolve within the temporary test directory.
🤖 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 `@deploy/gcp/finalize-legacy-retirement.sh`:
- Line 241: Before constructing the handoff_checkpoint command, extract
`.front.slot` from CUTOVER_EVIDENCE into a local variable and validate that it
is present, non-null, and matches the expected slot format. Use only the
validated variable when invoking gcloud_ssh, preserving the existing
validate-front-handoff-checkpoint arguments and preventing untrusted text from
being interpolated into the remote shell command.
In `@deploy/gcp/switch-front-migration.sh`:
- Around line 495-500: Update the listener validation in the legacy listener
ownership check before the source_listener_inode assignment to require the
inode’s expected safe format, not merely a JSON string type. Keep the existing
PID and FD validations and ensure only values safe for interpolation into the
single-quoted remote command reach source_listener_inode.
---
Nitpick comments:
In `@cmd/subrouter/deploy_scripts_test.go`:
- Around line 1146-1160: Update the fake gcloud harness in
writeExecutableTestFile so it no longer reimplements the preserve-rule cleanup
logic. Instead, execute or otherwise assert against the trap body contained in
remote_command, ensuring the real deploy-lock.sh trap removes and preserves the
correct sentinels; when executing it, rewrite the generated marker prefix so
sentinel paths resolve within the temporary test directory.
In `@deploy/gcp/deploy-lock.sh`:
- Around line 124-128: Update the preserve branch of the owner_cleanup_command
trap to remove all /tmp/subrouter-rss-${owner_cleanup_label}-*.running markers
except the legacy preserved sentinel, rather than hard-coding front, slot-a, and
slot-b. Keep removal of the install and deployment contract files unchanged, and
leave the non-preserve branch behavior intact.
🪄 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: de277976-f623-4c27-b6d7-7533dbf49ba8
📒 Files selected for processing (13)
cmd/subrouter-transport-observer/golden_acceptance_regression_test.gocmd/subrouter-transport-observer/golden_migration_evidence.gocmd/subrouter-transport-observer/testdata/golden_fake_client/main.gocmd/subrouter/deploy_scripts_test.gocmd/subrouter/front.gocmd/subrouter/front_takeover_test.gocmd/subrouter/listener_address.godeploy/gcp/deploy-lock.shdeploy/gcp/deployment-contract.pydeploy/gcp/finalize-legacy-retirement.shdeploy/gcp/install-front-slots.shdeploy/gcp/switch-front-migration.shdeploy/gcp/validate-deploy-evidence.py
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/subrouter/listener_address.go
- cmd/subrouter-transport-observer/testdata/golden_fake_client/main.go
- deploy/gcp/validate-deploy-evidence.py
- cmd/subrouter-transport-observer/golden_migration_evidence.go
- cmd/subrouter/front.go
- cmd/subrouter-transport-observer/golden_acceptance_regression_test.go
- deploy/gcp/install-front-slots.sh
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
cmd/subrouter/front_fdstore_linux.go (2)
49-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
porttoslot.
frontListenerDescriptorStoreSlotreturns a full slot key that encodes family, address, and port. The nameportdescribes only part of the value.♻️ Proposed rename
func frontListenerStoreName(address net.Addr) (string, error) { - port, err := frontListenerDescriptorStoreSlot(address) + slot, err := frontListenerDescriptorStoreSlot(address) if err != nil { return "", fmt.Errorf("front listener address for descriptor store is invalid: %w", err) } - return "subrouter-front-listener-" + port, nil + return "subrouter-front-listener-" + slot, nil }🤖 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/front_fdstore_linux.go` around lines 49 - 55, Rename the local variable port to slot in frontListenerStoreName, including its declaration, error-path context if applicable, and concatenation when constructing the returned store name; preserve the existing behavior and use of frontListenerDescriptorStoreSlot.
30-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the dead
notifiedbranch.Both branches return
nil, so thenotifiedcheck has no effect.storeFrontListenerignores the bool result. Return the error directly.♻️ Proposed simplification
- notified, err := notifySystemdDescriptorStore("FDSTORE=1\nFDNAME="+name, file) - if err != nil { - return err - } - if !notified { - return nil - } - return nil + _, err = notifySystemdDescriptorStore("FDSTORE=1\nFDNAME="+name, file) + return err🤖 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/front_fdstore_linux.go` around lines 30 - 37, Update storeFrontListener to return the result of notifySystemdDescriptorStore directly, removing the unused notified variable and redundant conditional while preserving the existing error propagation.cmd/subrouter/front_fdstore_linux_test.go (1)
81-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the FDNAME value, not only the verb.
The test checks
FDSTORE=1andFDSTOREREMOVE=1. It does not checkFDNAME.FDSTOREREMOVEremoves a stored descriptor by name, so a wrong or mismatchedFDNAMEwould silently break retention while this test still passes. Compare both messages againstfrontListenerStoreName(listener.Addr()).💚 Proposed assertion
+ wantName, err := frontListenerStoreName(listener.Addr()) + if err != nil { + t.Fatal(err) + } storeMessage, storeDescriptors := readNotification() - if !strings.Contains(storeMessage, "FDSTORE=1") || len(storeDescriptors) != 1 { + if !strings.Contains(storeMessage, "FDSTORE=1") || + !strings.Contains(storeMessage, "FDNAME="+wantName) || len(storeDescriptors) != 1 { t.Fatalf("store notification = %q with %d descriptors", storeMessage, len(storeDescriptors)) }removeMessage, removeDescriptors := readNotification() - if !strings.Contains(removeMessage, "FDSTOREREMOVE=1") || len(removeDescriptors) != 0 { + if !strings.Contains(removeMessage, "FDSTOREREMOVE=1") || + !strings.Contains(removeMessage, "FDNAME="+wantName) || len(removeDescriptors) != 0 { t.Fatalf("remove notification = %q with descriptors %v", removeMessage, removeDescriptors) }As per coding guidelines: "Use
go test ./...before handing off changes."Also applies to: 106-109
🤖 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/front_fdstore_linux_test.go` around lines 81 - 84, Update the assertions in the FDSTORE and FDSTOREREMOVE notification checks to validate that each message contains the expected FDNAME value from frontListenerStoreName(listener.Addr()), in addition to the existing verb checks. Ensure the test verifies both notifications use the same listener-derived name, then run go test ./... before handoff.Source: Coding guidelines
🤖 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/front_handoff_unix.go`:
- Around line 355-361: Update promoteFrontSuccessor to inspect the boolean
result from notifyFrontMainPID and return an error when notification was not
sent, including when NOTIFY_SOCKET is unset. Preserve no-socket success only if
an explicitly supported non-systemd hot-reload path requires it; otherwise
ensure handoffToSuccessor cannot mark reload complete and exit unsupervised.
In `@cmd/subrouter/front_notify_other.go`:
- Around line 1-7: Update the build constraint above notifyFrontMainPID in
cmd/subrouter/front_notify_other.go (lines 1-7) to !linux so the no-op
implementation covers every non-Linux platform. In
cmd/subrouter/front_handoff_other.go (lines 1-20), confirm the fallback compiles
and preserve promoteFrontSuccessor’s explicit unsupported error without
delegating to the no-op notifier.
In `@deploy/gcp/smoke-listener-takeover.sh`:
- Around line 26-28: Update the port validation block in
smoke-listener-takeover.sh so backend_port, test_port, and bootstrap_port are
each validated as numeric and within the valid 0–65535 range before use. Replace
the incorrect bootstrap_port-only arithmetic check with per-variable range
checks, and make each error message identify the port variable being rejected.
---
Nitpick comments:
In `@cmd/subrouter/front_fdstore_linux_test.go`:
- Around line 81-84: Update the assertions in the FDSTORE and FDSTOREREMOVE
notification checks to validate that each message contains the expected FDNAME
value from frontListenerStoreName(listener.Addr()), in addition to the existing
verb checks. Ensure the test verifies both notifications use the same
listener-derived name, then run go test ./... before handoff.
In `@cmd/subrouter/front_fdstore_linux.go`:
- Around line 49-55: Rename the local variable port to slot in
frontListenerStoreName, including its declaration, error-path context if
applicable, and concatenation when constructing the returned store name;
preserve the existing behavior and use of frontListenerDescriptorStoreSlot.
- Around line 30-37: Update storeFrontListener to return the result of
notifySystemdDescriptorStore directly, removing the unused notified variable and
redundant conditional while preserving the existing error propagation.
🪄 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: 52756c29-4034-4de0-ab96-c1461bb5f596
📒 Files selected for processing (22)
cmd/subrouter-transport-observer/golden_acceptance_regression_test.gocmd/subrouter-transport-observer/golden_migration_evidence.gocmd/subrouter-transport-observer/golden_options_test.gocmd/subrouter/deploy_scripts_test.gocmd/subrouter/front.gocmd/subrouter/front_fdstore_linux.gocmd/subrouter/front_fdstore_linux_test.gocmd/subrouter/front_fdstore_other.gocmd/subrouter/front_handoff_other.gocmd/subrouter/front_handoff_unix.gocmd/subrouter/front_notify_linux.gocmd/subrouter/front_notify_other.gocmd/subrouter/front_takeover_test.gocmd/subrouter/listener_address.gocmd/subrouter/listener_transfer_unix.gocmd/subrouter/listener_transfer_unix_test.gocmd/subrouter/sr_test.godeploy/gcp/deployment-contract.pydeploy/gcp/finalize-legacy-retirement.shdeploy/gcp/install-front-slots.shdeploy/gcp/smoke-listener-takeover.shdeploy/gcp/switch-front-migration.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- cmd/subrouter/deploy_scripts_test.go
- deploy/gcp/deployment-contract.py
- deploy/gcp/finalize-legacy-retirement.sh
- cmd/subrouter/listener_transfer_unix.go
- cmd/subrouter-transport-observer/golden_migration_evidence.go
- cmd/subrouter-transport-observer/golden_options_test.go
- deploy/gcp/install-front-slots.sh
- deploy/gcp/switch-front-migration.sh
- cmd/subrouter-transport-observer/golden_acceptance_regression_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_acceptance_regression_test.go`:
- Around line 90-92: Update the validator checks in the relevant test around
validGoldenMigrationTransitionEvidence to first run the unmodified fixture and
fail unless the Python validator exits successfully. Then run the mutated
fixture and require a nonzero validator result, while distinguishing execution
failures from expected validation rejection so missing, invalid, or crashing
validators cannot make the test pass.
In `@cmd/subrouter/front_notify_linux.go`:
- Around line 6-10: Update the MAINPID notification flow around
notifySystemdDescriptorStore and notifySystemdBarrier so a barrier timeout or
unexpected payload remains advisory after the descriptor send succeeds. Prevent
promoteFrontSuccessor from failing before ownership confirmation due solely to
notifyFrontMainPID’s barrier error, or restore parent ownership in the rollback
path before propagating it.
In `@cmd/subrouter/front_takeover_test.go`:
- Around line 485-504: Update the test around the promoteSuccessor callback and
promotions assertion to use an atomic counter, loading it atomically from the
test goroutine. After waiting for retired, poll until the counter reaches 2
(successor promotion and parent restoration) with a timeout, rather than reading
promotions directly; include sync/atomic and preserve the existing failure
behavior. Run go test ./... before handing off.
🪄 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: a98d2141-fe7d-4f35-a0db-e46a6fe8a327
📒 Files selected for processing (13)
cmd/subrouter-transport-observer/golden_acceptance_regression_test.gocmd/subrouter-transport-observer/golden_migration_evidence.gocmd/subrouter/front.gocmd/subrouter/front_fdstore_linux.gocmd/subrouter/front_fdstore_linux_test.gocmd/subrouter/front_handoff_other.gocmd/subrouter/front_handoff_unix.gocmd/subrouter/front_notify_linux.gocmd/subrouter/front_takeover_test.gocmd/subrouter/listener_transfer_unix_test.godeploy/gcp/install-front-slots.shdeploy/gcp/smoke-listener-takeover.shdeploy/gcp/validate-deploy-evidence.py
🚧 Files skipped from review as they are similar to previous changes (7)
- deploy/gcp/validate-deploy-evidence.py
- cmd/subrouter/front_handoff_other.go
- deploy/gcp/smoke-listener-takeover.sh
- cmd/subrouter/front_handoff_unix.go
- cmd/subrouter-transport-observer/golden_migration_evidence.go
- cmd/subrouter/front.go
- deploy/gcp/install-front-slots.sh
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Transfers the live TCP listener from the legacy supervisor to the stable front with pidfd_getfd, preserving existing accepted connections while new connections move without a GCP route change.
The golden gate now proves the same kernel socket is active in the front, retires only the old listener descriptor, and requires held HTTP/WebSocket Codex sessions plus a fresh public Codex session to remain healthy. It retains the slot rehearsal, rollback, and final activation cycle.
Verification:
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Enables zero-gap cutover by duplicating the live TCP listener from the legacy supervisor into the stable front, keeping all connections and avoiding any GCP route change. Adds durable ownership across restarts and safely retires ambiguous successor activations.
New Features
--listener-transfer-socket;listener-transfersends the exact listener FD, validates numeric addresses, and rejects shared or non-absolute sockets.MAINPIDbarrier; Linux-only SIGHUP reload; roll back if a confirmed successor exits; retire ambiguous successor activations gracefully.--takeover-listener-pidand--takeover-listener-fd; added takeover/transfer tests and a production systemd smoke test.Migration
ActiveBackendURL, andSourceListenerRetiredAt; validators enforce a singlefinal-cutoverwith a bounded window and require retirement before destination proof. Candidate bumped to v0.1.69; addedgolang.org/x/sys; golden runner supports--account-id.SUBROUTER_FRONT_ADDRandSUBROUTER_FRONT_TRANSFER_SOCKET. Runmigrate-to-front-slots, thenswitch-front-migration --operation final-cutoverfor descriptor handoff. Deployment lock preserves only the committed legacy sampler when a front handoff checkpoint exists;finalize-legacy-retirementverifies front listener ownership and keeps the disabled legacy process alive until sessions drain.Written for commit ac8e838. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements