fix(restore): treat a 404 restore-result ack as terminal so cross-greffer migration leftovers self-clean#117
Open
omarsy wants to merge 1 commit into
Open
fix(restore): treat a 404 restore-result ack as terminal so cross-greffer migration leftovers self-clean#117omarsy wants to merge 1 commit into
omarsy wants to merge 1 commit into
Conversation
… leftovers self-clean
A cross-greffer migration drives a restore on the target greffer (G2) with NO
manager RestoreRun (the manager polls G2's durable restore-status instead). G2's
restore-result callback therefore never gets a 2xx, and because the greffer only
deleted its on-disk restore-state file after a 2xx ack, that .restore_<id>.json
lingered forever (one orphan per migration; a benign 404 logged on each G2 boot
reconcile).
_post_callback now returns a 3-state outcome instead of a bool:
- _CB_ACKED (2xx): the op is finalized.
- _CB_GONE (404): the manager has no record of the run (or the instance is
gone). Terminal, so the durable state file is safe to drop.
- _CB_RETRY (5xx / 403 / other / network): transient, keep the file for boot
reconciliation.
403 is deliberately RETRY, not terminal: during the LIVE migration window the
instance FK is still on the source greffer, so G2's in-flight restore callback
403s while the manager is actively polling G2's restore-status through exactly
that window. Dropping the state file on 403 would strand that poll and time the
migration out. A migration's 404 only arrives AFTER cutover, when the manager no
longer needs the file. A normal restore can't get a spurious 404 either: the
manager creates the RestoreRun before dispatching, and a replay on a terminal
row is a 200 no-op, so 404 there means the run/instance is genuinely gone.
Tests: classify every status (200/204 -> ACKED, 404 -> GONE, 403/400/5xx/network
-> RETRY), the settle predicate, and reconcile_on_boot dropping on 404 while
KEEPING on 403 (regression guard for the migration poll dependency).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Problem
A cross-greffer migration leaves one orphan restore-state file (
.restore_<id>.json) per migration on the target greffer (G2).The migration cutover drives a restore on G2 with no manager
RestoreRun(by design: the manager polls G2's durablerestore-statusinstead, seemanagermigration_services._cold_restore_and_wait). So when G2 posts itsrestore-resultcallback, the manager has no row to finalize and never returns 2xx. The greffer only deleted its on-disk restore-state file after a 2xx ack, so the file lingered forever. Impact is low and bounded (one harmless leftover per migration; a benign 404 logged on each G2 boot reconcile), but it is a wart worth removing.Fix
_post_callbacknow returns a 3-state outcome instead of a bool, and a new_callback_settledpredicate decides when the durable state file may be dropped:_CB_ACKED(finalized)RestoreRun/ instance gone)_CB_GONE(terminal)_CB_RETRY(transient)Why 404 is terminal but 403 is not (the load-bearing invariant)
During the live migration restore the instance FK is still on the source greffer, so G2's in-flight callback 403s (token/ownership mismatch) and the manager is actively polling G2's
restore-statusthrough that exact window. Dropping the file on a 403 would strand that poll and time the migration out. A migration's 404 only arrives after cutover (FK flipped to G2, still noRestoreRun), observed on a later G2 boot reconcile, when the manager no longer needs the file.A normal (non-migration) restore cannot get a spurious 404 either: the manager creates the
RestoreRunbefore dispatching to the greffer, and a replay on a terminal row is an idempotent 200 no-op, so a 404 there means the run/instance is genuinely gone.This was verified against the manager source (
instance_restore_result->_callback_instance_or_403returns 403 on token mismatch / 404 on missing instance;finalize_restorereturnsNone-> 404 only when no matchingRestoreRun).Tests
test_post_callback_classifies_status: 200/204 -> ACKED, 404 -> GONE, 403/400/500/network -> RETRY.test_callback_settled_only_acked_or_gone: the settle predicate.test_reconcile_drops_restore_state_on_404_gone: the orphan self-cleans on 404.test_reconcile_keeps_restore_state_on_403_retry: regression guard that a 403 keeps the file (protects the migration poll dependency).Full greffer suite green (687 passed).
Scope
Greffer-only. No manager change required. Surfaced while wiring the manager's
reap_stuck_opscommand (feat/reap-stuck-ops); unrelated to it and intentionally split into its own PR.