fix(add-factor): lookup ensure vs delete + heal retries - #235
Conversation
Re-read metadata before ensuring FactorLookup so concurrent deletes do not resurrect stale rows, and retry rollback heal attempts before giving up on restoring a concurrent writer's lookup.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65928c0b9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| backup_id: &str, | ||
| new_factor_kind: &FactorKind, | ||
| ) -> Result<(), ErrorResponse> { | ||
| match backup_storage.get_metadata_by_backup_id(backup_id).await? { |
There was a problem hiding this comment.
If this new metadata read exhausts an S3/AWS SDK transient error after the factor write committed, ? returns immediately without attempting the lookup insertion. In the race this helper is intended to repair—another add-factor request owned the lookup row and deleted it after this request committed metadata—the result is a factor stored in metadata with no lookup entry, so recovery cannot find the backup; the consumed challenge also makes a transparent client replay unlikely. The new metadata checks, including the retry-path check below, need retry/reconciliation behavior rather than abandoning the repair on the first read error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 908c463
Metadata presence checks now retry (up to 3×). If they remain unavailable we still attempt the lookup insert (FactorPresence::Unknown) instead of aborting repair with ?.
Do not abandon lookup repair on the first S3 metadata error; retry the presence check and, if still unknown, proceed with insert so a committed factor is not left untraceable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 908c463a3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match factor_present_in_metadata_with_retry( | ||
| backup_storage, | ||
| backup_id, | ||
| new_factor_kind, | ||
| &factor_pk, |
There was a problem hiding this comment.
When /delete-factor removes the factor from S3 after this presence check but deletes the DynamoDB row before the subsequent insert, ensure_main_factor_lookup recreates the lookup after deletion has completed. The result is a stale mapping that routes recovery to metadata without the factor and prevents the identity from being registered elsewhere; the new pre-read therefore does not actually close the deletion race without coordination, a tombstone/version check, or equivalent post-insert reconciliation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed this needs a post-insert reconcile rather than only tightening the pre-check.
Follow-up: #237 — after a successful ensure insert/adopt, re-read metadata and delete the lookup if the factor is gone (closes the delete-between-check-and-insert resurrection window).
| if attempt == MAX_ATTEMPTS { | ||
| return; |
There was a problem hiding this comment.
When the first heal insert returns ConditionalCheckFailed and the consistent lookup then returns None, the next two attempts re-read S3 before retrying the DynamoDB insert. If those metadata reads transiently fail, this return abandons healing even though the first attempt already confirmed that the factor exists; unlike the parent implementation's immediate retry, the final state is a committed factor with no lookup row. The fresh evidence in this revision is the new terminal return after the retry loop's metadata errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bfe42ae on this PR.
On ConditionalCheckFailed + consistent None, heal now retries the Dynamo insert immediately (without requiring another S3 metadata re-read). Once metadata has confirmed the factor is present, later transient S3 failures also continue insert attempts instead of abandoning heal.
When ConditionalCheckFailed is followed by a consistent miss, retry Dynamo insert immediately (and keep inserting if metadata was already confirmed) so a transient S3 failure cannot abandon heal.
1 participant
Summary
Follow-ups from Codex review on #234:
ensure_main_factor_lookupre-reads metadata (and again before retry insert) and skips restore if the factor/backup is gone.ConditionalCheckFailed, before logging exhaustion.Stacked on #234 (PR 5/5 of the universal-add-factor split).
Test plan
cargo clippy --all-targets -- -D warningscargo test --lib routes::add_factorcargo test --test add_factor_happy_paths test_add_factor_same_oidc_single_session