fix(add-factor): post-insert FactorLookup ensure reconcile - #237
Conversation
… insert Re-read metadata after a successful ensure insert/adopt and delete the lookup if the factor was removed concurrently, closing the pre-check TOCTOU resurrection window.
Merge identical heal match arms and split ensure into helpers so too_many_lines and match_same_arms pass under pedantic.
…ookup-post-insert-reconcile
![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: 84387d654d
ℹ️ 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".
| factor_lookup | ||
| .delete(FactorScope::Main, factor_to_lookup) | ||
| .await?; |
There was a problem hiding this comment.
If another same-backup /add-factor request restores this factor after the Absent read but before this delete, it can adopt the existing lookup, commit metadata, complete its own ensure while the mapping still exists, and then have that valid mapping deleted here. That successful request leaves the restored factor untraceable, so this cleanup needs a post-delete metadata re-read and lookup heal (as the rollback path already performs), rather than ending immediately after the unconditional delete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 49797e3.
After the reconcile delete we now call heal_main_factor_lookup_if_present, which re-reads metadata and re-inserts the lookup if a concurrent same-backup /add-factor restored the factor (and had adopted this mapping) between the Absent check and the delete.
After deleting a stale ensure mapping on Absent metadata, re-run heal so a concurrent same-backup add-factor that restored the 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: 49797e3c42
ℹ️ 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".
| factor_lookup | ||
| .delete(FactorScope::Main, factor_to_lookup) | ||
| .await?; |
There was a problem hiding this comment.
If /delete-factor removes this backup's lookup after the consistent read and another backup then claims the same factor before this call, the unconditional delete removes that new backup's valid mapping. The subsequent heal checks only the original backup's now-absent metadata, so it cannot restore the new owner's row; use an owner-conditional delete rather than deleting solely by factor key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f48996e.
Reconcile now uses FactorLookup::delete_if_maps_to (#backup_id = :backup_id condition) so we cannot delete another backup's valid mapping if ownership changed after the consistent read.
| factor_lookup | ||
| .delete(FactorScope::Main, factor_to_lookup) | ||
| .await?; |
There was a problem hiding this comment.
When this DynamoDB delete applies but returns a timeout or dispatch error, .await? exits before the new heal call. If a concurrent same-backup /add-factor adopted the mapping and committed metadata before that delete, its successful request is left without a lookup. Fresh evidence after the earlier fix is that the added heal remains reachable only on Ok; run it after both successful and ambiguous delete outcomes, as the rollback path already does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f48996e.
Delete errors are logged and heal still runs (same pattern as lookup rollback), so an ambiguous Dynamo delete ACK cannot skip repair.
| heal_main_factor_lookup_if_present( | ||
| backup_storage, | ||
| factor_lookup, | ||
| factor_to_lookup, | ||
| backup_id, | ||
| new_factor_kind, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
If another /delete-factor removes the newly restored factor after heal_main_factor_lookup_if_present observes it in metadata but before that helper inserts the lookup, the helper returns immediately after the successful insert and recreates the stale row this post-insert reconciliation is intended to prevent. Fresh evidence beyond the earlier heal fix is that the reused helper performs only a pre-insert metadata check; its successful insert also needs a post-insert reconciliation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f48996e.
Reconcile now runs up to two delete+heal rounds: after a heal insert that raced with /delete-factor, the next round re-checks metadata and cleans up / re-heals without async recursion between heal and reconcile.
…ners Use owner-conditional FactorLookup deletes, heal after ambiguous delete errors, and run a second delete+heal round so heal inserts are reconciled without async recursion.
1 participant
Summary
/delete-factorcan remove a factor (and its lookup) between the ensure pre-check and Dynamo insert, leaving a resurrected stale row.Stack
Base: #235 (
uaf/5-lookup-ensure-heal-followups). Sibling to #236; rebase #236 onto updated #235 separately if needed.Test plan
Unknownafter insert does not delete the lookup