fix(guest): make gateway refresh failover atomic (re-land #962) - #1027
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Re-lands the previously stacked gateway refresh changes into next by making gateway key cache handling and gateway RPC URL normalization more robust and testable, and by restoring regression tests that pin atomic refresh behavior.
Changes:
- Refactors
GatewayKeyStoreto supportload_from/save_to, addsis_cert_valid_at, and centralizes gateway RPC URL normalization. - Updates gateway client construction to use the normalized RPC URL helper.
- Restores/introduces focused regression tests covering URL normalization, key cache persistence/permissions, malformed cache handling, and certificate refresh boundary behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
352
to
+353
| let content = serde_json::to_string(self).context("Failed to serialize gateway cache")?; | ||
| safe_write_with_mode(GATEWAY_CACHE_PATH, &content, 0o600) | ||
| .context("Failed to write gateway cache")?; | ||
| safe_write_with_mode(path, &content, 0o600).context("Failed to write gateway cache")?; |
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
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.
Problem
PR #962 shows as merged on GitHub, but its change is not on the mainline and never was.
#962 was stacked: its base was
fix/guest-local-provider-inventory-master, not the default branch. That parent branch had already been merged to the mainline via #1016 (merge commit87abbcccd). #962 landed on the parent after that, so nothing carried it forward. Itsmerge_commit_shae1745d672is not an ancestor ofnext.The result is a silent regression:
getQuote-adjacent gateway registration refresh indstack-util/src/system_setup.rsstill has the non-atomic behaviour #962 fixed, and the three tests that pinned that behaviour do not exist onnext.Verified by content rather than by commit graph — of the added lines sampled from
a8240ffec,fn load_from(path: &Path) -> Option<Self>andSelf::load_from(Path::new(GATEWAY_CACHE_PATH))are absent fromnext, along with the surrounding body.Fix
Cherry-picks the five commits of #962 onto
next, unchanged and in their original order (-xrecorded, so each commit references the original SHA):fix(guest): make gateway refresh failover atomicstyle(guest): format gateway refresh testsfix(guest): retain gateway keys across failover attemptsstyle(guest): format gateway failover closurerefactor(guest): keep gateway failover loop inlineAll five applied without conflict despite 73 commits having landed on
nextsince the merge base, and despitesystem_setup.rsitself having changed by 21 insertions / 43 deletions over that window — the two sets of edits touch different regions.Net effect is 99 insertions / 8 deletions in
dstack/dstack-util/src/system_setup.rs.Verification
cargo check -p dstack-utilpasses.cargo test -p dstack-util— 82 passed, 0 failed.cargo fmt --all --checkclean;cargo clippy -p dstack-utilproduces no new warnings.nextbeforehand and present after:key_store_round_trip_is_private_and_stable,malformed_replacement_does_not_overwrite_working_cache,gateway_rpc_urls_are_normalized_once.Note on the stacked-PR pattern
This was found while auditing branches for cleanup: every merged-PR branch still alive had commits pushed after its own merge, and three of them were stacked onto another feature branch rather than the mainline. #960 hit the same trap and was rescued manually via #1020; #962 was not.
delete_branch_on_mergeis now enabled, which removes the branch at merge time and makes this specific failure mode much harder to reproduce.