Repository navigation
Correlate golden migration proof by session - #176
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe migration flow now performs a post-snapshot destination liveness handshake. It records connection, session, snapshot, challenge, and timing data, then validates the evidence across Go and Python validators within ten seconds. ChangesDestination liveness migration flow
Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@deploy/gcp/deployment-contract.py`:
- Around line 439-445: The destination proof validation must enforce the same
64-character lowercase hexadecimal format required by destination liveness.
Update command_validate_destination_proof to validate
destination_proof.connection_id with the existing 64-hex rule before
switch-front-migration.sh forwards it, while preserving the current validation
behavior for other proof fields.
In `@deploy/gcp/switch-front-migration.sh`:
- Around line 465-470: Adjust the polling deadline in the liveness-proof loop so
it reserves a margin for stamping liveness_received_at after the proof appears,
rather than using the full DESTINATION_LIVENESS_LIMIT_MS budget. Keep the
existing missing-response failure behavior, and ensure any accepted proof leaves
enough time for received_at - requested_at to remain strictly below the
validators’ 10-second limit.
In `@deploy/gcp/validate-deploy-evidence.py`:
- Around line 1053-1062: Update the canonical destination projection in
validate_migration_snapshot to hash only generation, generation_connections,
inactive_connections, kind, and public_connections, matching
goldenMigrationSnapshotSHA256. Build the canonical object from those fields
before computing
destination_proof.post_snapshot_liveness.destination_snapshot_sha256, while
preserving the existing sorted JSON serialization and SHA-256 comparison.
🪄 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: 2694c93a-f6b5-4083-90f8-5334401e77fb
📒 Files selected for processing (9)
cmd/subrouter-transport-observer/golden.gocmd/subrouter-transport-observer/golden_acceptance_regression_test.gocmd/subrouter-transport-observer/golden_migration_evidence.gocmd/subrouter-transport-observer/golden_migration_transition.gocmd/subrouter-transport-observer/testdata/golden_fake_client/main.gocmd/subrouter/deploy_scripts_test.godeploy/gcp/deployment-contract.pydeploy/gcp/switch-front-migration.shdeploy/gcp/validate-deploy-evidence.py
| def command_validate_destination_liveness(args: argparse.Namespace) -> None: | ||
| if re.fullmatch(r"[0-9a-f]{64}", args.connection_id) is None: | ||
| fail("destination liveness connection_id is invalid") | ||
| if re.fullmatch(r"[A-Za-z0-9._:-]{1,256}", args.session_id) is None: | ||
| fail("destination liveness session_id is invalid") | ||
| if re.fullmatch(r"[0-9a-f]{64}", args.destination_snapshot_sha256) is None: | ||
| fail("destination liveness snapshot SHA-256 is invalid") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every producer and consumer of the destination proof connection_id.
set -euo pipefail
rg -n -C3 'connection_id' --glob '*.go' --glob '*.py' --glob '*.sh' \
| rg -n -C3 'destination|liveness|proof'Repository: manaflow-ai/subrouter
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,160p'
echo "== connection_id occurrences =="
rg -n -C2 'connection_id' --glob '*.go' --glob '*.py' --glob '*.sh' . || true
echo "== destination/liveness/proof occurrences =="
rg -n -C2 'destination|liveness|proof|switch-front-migration|validate_destination' --glob '*.go' --glob '*.py' --glob '*.sh' . || trueRepository: manaflow-ai/subrouter
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== generate_connection_id definitions/usages =="
rg -n -C3 'generate_connection_id|connection_id|ConnectionID|connectionID' cmd/subrouter cmd/subrouter-transport-observer --glob '*.go'
echo "== production code around ConnectionID =="
sed -n '1,220p' cmd/subrouter-transport-observer/golden_migration_transition.go
echo "== deployment contract relevant sections =="
sed -n '399,470p' deploy/gcp/deployment-contract.py
sed -n '554,580p' deploy/gcp/deployment-conduct.py.bak || true
sed -n '554,580p' deploy/gcp/deployment-contract.py
echo "== switch front relevant sections =="
sed -n '420,478p' deploy/gcp/switch-front-migration.sh
sed -n '508,552p' deploy/gcp/switch-front-migration.sh
echo "== deterministic behavior probe =="
python3 - <<'PY'
import re
ids = ["connection", "64hex" * 8, "64HEX" * 8, "64hex32"]
print("validate_destination_proof_connection_id_valid:", [ids], [bool(id_) for id_ in ids])
print("validate_destination_liveness_connection_id_valid:", [ids], [bool(re.fullmatch(r"[0-9a-f]{64}", id_)) for id_ in ids])
PYRepository: manaflow-ai/subrouter
Length of output: 50378
Validate the destination proof connection_id with the 64-hex rule before switch-front-migration.sh forwards it.
command_validate_destination_proof accepts any non-empty string for destination_proof.connection_id, but command_validate_destination_liveness only accepts 64 lowercase hex characters. switch-front-migration.sh passes .connection_id from the destination proof into the liveness request, so a non-hex connection identifier can pass the proof validation and then fail the post-switch liveness contract.
🤖 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/deployment-contract.py` around lines 439 - 445, The destination
proof validation must enforce the same 64-character lowercase hexadecimal format
required by destination liveness. Update command_validate_destination_proof to
validate destination_proof.connection_id with the existing 64-hex rule before
switch-front-migration.sh forwards it, while preserving the current validation
behavior for other proof fields.
| while [[ ! -f "${liveness_proof_path}" || -L "${liveness_proof_path}" ]]; do | ||
| (( $(epoch_millis) - liveness_requested_ms < DESTINATION_LIVENESS_LIMIT_MS )) \ | ||
| || die "golden destination connection produced no response chunk after the destination snapshot" | ||
| sleep 0.05 | ||
| done | ||
| liveness_received_at="$(utc_now)" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Leave margin between the poll deadline and the evidence deadline.
The loop accepts a proof while elapsed time is under 10000 ms. liveness_received_at is stamped after the loop exits. Both evidence validators reject received_at - requested_at >= 10 s (validate-deploy-evidence.py Lines 999-1000, golden_migration_evidence.go Line 514).
A proof that appears at 9.9 s can therefore produce evidence that both validators reject, and that failure happens after the route switch. Poll against a smaller budget so a late-but-accepted proof cannot produce unvalidatable evidence.
🛡️ Proposed fix to reserve stamping margin
while [[ ! -f "${liveness_proof_path}" || -L "${liveness_proof_path}" ]]; do
- (( $(epoch_millis) - liveness_requested_ms < DESTINATION_LIVENESS_LIMIT_MS )) \
+ (( $(epoch_millis) - liveness_requested_ms < DESTINATION_LIVENESS_LIMIT_MS - 500 )) \
|| die "golden destination connection produced no response chunk after the destination snapshot"
sleep 0.05
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while [[ ! -f "${liveness_proof_path}" || -L "${liveness_proof_path}" ]]; do | |
| (( $(epoch_millis) - liveness_requested_ms < DESTINATION_LIVENESS_LIMIT_MS )) \ | |
| || die "golden destination connection produced no response chunk after the destination snapshot" | |
| sleep 0.05 | |
| done | |
| liveness_received_at="$(utc_now)" | |
| while [[ ! -f "${liveness_proof_path}" || -L "${liveness_proof_path}" ]]; do | |
| (( $(epoch_millis) - liveness_requested_ms < DESTINATION_LIVENESS_LIMIT_MS - 500 )) \ | |
| || die "golden destination connection produced no response chunk after the destination snapshot" | |
| sleep 0.05 | |
| done | |
| liveness_received_at="$(utc_now)" |
🤖 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 465 - 470, Adjust the
polling deadline in the liveness-proof loop so it reserves a margin for stamping
liveness_received_at after the proof appears, rather than using the full
DESTINATION_LIVENESS_LIMIT_MS budget. Keep the existing missing-response failure
behavior, and ensure any accepted proof leaves enough time for received_at -
requested_at to remain strictly below the validators’ 10-second limit.
| canonical_destination = json.dumps( | ||
| destination_after, | ||
| sort_keys=True, | ||
| separators=(",", ":"), | ||
| ).encode("utf-8") | ||
| exact( | ||
| liveness_snapshot_sha, | ||
| hashlib.sha256(canonical_destination).hexdigest(), | ||
| "destination_proof.post_snapshot_liveness.destination_snapshot_sha256", | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Hash the same five snapshot fields that the Go validator hashes.
goldenMigrationSnapshotSHA256 in cmd/subrouter-transport-observer/golden_migration_evidence.go (Lines 528-541) hashes exactly generation, generation_connections, inactive_connections, kind, and public_connections. This code hashes every key that destination.after contains, because validate_migration_snapshot returns the full parsed object.
Both digests agree for the current producers. If any producer adds one field to destination.after, the Go validator still accepts the document and this validator rejects it with a snapshot-hash failure. Project the same five fields to keep the two validators on one contract.
♻️ Proposed fix to align the canonical projection
- canonical_destination = json.dumps(
- destination_after,
- sort_keys=True,
- separators=(",", ":"),
- ).encode("utf-8")
+ canonical_destination = json.dumps(
+ {
+ key: destination_after[key]
+ for key in (
+ "generation",
+ "generation_connections",
+ "inactive_connections",
+ "kind",
+ "public_connections",
+ )
+ },
+ sort_keys=True,
+ separators=(",", ":"),
+ ).encode("utf-8")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| canonical_destination = json.dumps( | |
| destination_after, | |
| sort_keys=True, | |
| separators=(",", ":"), | |
| ).encode("utf-8") | |
| exact( | |
| liveness_snapshot_sha, | |
| hashlib.sha256(canonical_destination).hexdigest(), | |
| "destination_proof.post_snapshot_liveness.destination_snapshot_sha256", | |
| ) | |
| canonical_destination = json.dumps( | |
| { | |
| key: destination_after[key] | |
| for key in ( | |
| "generation", | |
| "generation_connections", | |
| "inactive_connections", | |
| "kind", | |
| "public_connections", | |
| ) | |
| }, | |
| sort_keys=True, | |
| separators=(",", ":"), | |
| ).encode("utf-8") | |
| exact( | |
| liveness_snapshot_sha, | |
| hashlib.sha256(canonical_destination).hexdigest(), | |
| "destination_proof.post_snapshot_liveness.destination_snapshot_sha256", | |
| ) |
🤖 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` around lines 1053 - 1062, Update the
canonical destination projection in validate_migration_snapshot to hash only
generation, generation_connections, inactive_connections, kind, and
public_connections, matching goldenMigrationSnapshotSHA256. Build the canonical
object from those fields before computing
destination_proof.post_snapshot_liveness.destination_snapshot_sha256, while
preserving the existing sorted JSON serialization and SHA-256 comparison.
1 participant
Summary
Testing
go test ./cmd/subrouter-transport-observer -run "^TestGoldenMigrationAllowsConcurrentDestinationConnectionDrain$" -count=1go test ./cmd/subrouter -run "^TestGCPDeploymentEvidenceGateValidatesOutcomes$" -count=1Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Correlate the golden migration proof by session and require a post-snapshot liveness proof on the exact destination connection, waiting for that proof before emitting evidence. This makes cutovers reliable under load and verifies the session stayed live in both Go and Python.
journal_correlatedin destination proof across Go/Python validators.post_snapshot_livenessin evidence: same connection/session, destination snapshot SHA matches, ordered timestamps, and liveness within 10s.deploy/gcp/switch-front-migration.shto issue a liveness challenge, wait for theliveness-proof, and include it; addvalidate-destination-livenesstodeploy/gcp/deployment-contract.py; update the fake client and add regression tests (exact-connection chunk, missing/incorrect liveness, concurrent drains).Written for commit 941214c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes