fix(add-factor): concurrent lookup heal + same-session OIDC - #234
Conversation
Allow one OIDC JWT+session for same-account upgrades, ensure/heal FactorLookup around concurrent rollback deletes, and cover the single-session metadata-only upgrade path.
![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: 82d21ae2bf
ℹ️ 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".
| ) => | ||
| { | ||
| // Another healer (or the concurrent writer) already restored the row. | ||
| } |
There was a problem hiding this comment.
When rollback healing races with registration of the same factor for another backup, this insert can fail conditionally because the lookup now belongs to that other backup. Treating every conditional failure as successful restoration leaves the original backup's metadata containing the factor while its lookup points elsewhere, breaking recovery and uniqueness; perform a consistent read and confirm backup_id, as ensure_main_factor_lookup does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in this PR: d23d611
Heal now consistently re-reads after ConditionalCheckFailed and confirms the row still maps to this backup_id (same pattern as ensure_main_factor_lookup).
| match factor_lookup | ||
| .insert(FactorScope::Main, factor_to_lookup, backup_id.to_string()) | ||
| .await |
There was a problem hiding this comment.
If /delete-factor removes the factor (or the last factor and the entire backup) after the earlier metadata write/read but before this call, the unconditional insert recreates a lookup for metadata that no longer contains the factor. That stale row can route recovery to a missing backup and permanently prevent the credential from being registered elsewhere; the reconciliation must account for a concurrent metadata deletion rather than restoring solely from the earlier result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to a follow-up stacked PR: #235
There, ensure_main_factor_lookup re-reads metadata before (and during) restore so a concurrent /delete-factor / backup delete does not resurrect a stale lookup.
| Err(err) => { | ||
| tracing::error!( | ||
| message = "Failed to heal FactorLookup after concurrent factor write during rollback", | ||
| error = ?err, | ||
| factor_pk = factor_to_lookup.primary_key(), |
There was a problem hiding this comment.
When request A owns the lookup, request B adopts it and successfully commits the factor, and B's final ensure runs before A's rollback delete, B can return success while A subsequently removes the shared row. If this healing insert then exhausts its DynamoDB retries and returns an error, merely logging it leaves B's successfully registered factor permanently untraceable; the rollback path needs a durable retry or another mechanism that guarantees eventual restoration rather than swallowing the failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Deferred to a follow-up stacked PR: #235
Rollback heal now retries up to 3 times (with owner verification on conditional conflicts) before logging exhaustion.
…kFailed Do not treat every conditional insert failure during rollback heal as success; consistently re-read and confirm the row still maps to this backup (matching ensure_main_factor_lookup).
1 participant
Summary
audsafe).ensure_main_factor_lookup; rollback heals after ambiguous deletes.Split from #136 (PR 4/4). Stacked on #233.
Test plan
cargo test --test add_factor_happy_paths test_add_factor_same_oidc_single_sessioncargo test --lib routes::add_factorcargo clippy --all-targets -- -D warnings