fix(storage): factor write reconcile + consistent lookup - #231
Conversation
Distinguish own factor-id retries from concurrent same-identity winners on ambiguous S3 puts, add exact-match key-only upgrades with ambiguous reconcile, and expose consistent DynamoDB factor lookup after races.
|
@codex review |
|
cursor review |
There was a problem hiding this comment.
Pull request overview
Hardens distributed factor metadata writes and lookup consistency.
Changes:
- Reconciles ambiguous S3 writes using factor IDs.
- Adds idempotent encryption-key-only updates.
- Adds strongly consistent DynamoDB lookups.
No critical issues found; one non-blocking documentation inconsistency was identified.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/backup_storage.rs |
Adds factor/key reconciliation and tests. |
src/factor_lookup.rs |
Adds strongly consistent lookup support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// - Factor present in target scope + non-`412` error → `Inserted` (e.g. lost ACK after a | ||
| /// successful put). | ||
| /// - Factor present with **this request's** `attempted_factor_id` → `Inserted` (put landed, | ||
| /// including `412` after a stale-etag retry). |
There was a problem hiding this comment.
Fixed in 7cf5dae — docs now require a matching attempted_factor_id for Inserted on every ambiguous put error (same kind / different id → Unknown(FactorAlreadyExists)).
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
I recommend simplifying the comments, they're adding information that's obvious from the code and present a source of staleness info, makes it harder to maintain |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e1fb42a. Configure here.
|
I'm worried this reconciliation logic has no bound, wouldn't it be possible to get into an infinite loop? e.g. S3 outage |
Trim redundant commentary and correct resolve_put_object_outcome docs so Inserted requires a matching factor id for every ambiguous put error.
![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: c7223ddfa1
ℹ️ 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".
| Err(err) => match Self::classify_put_object_error::<()>(err) { | ||
| FactorMetadataWrite::NotInserted(e) => Err(e), |
There was a problem hiding this comment.
When an SDK retry follows a timed-out but successful conditional PUT, the next attempt can return S3's documented 409 ConditionalRequestConflict; classify_put_object_error categorizes every 4xx except 412 as NotInserted, so this new key-only path returns immediately without rereading metadata even though the key may already have landed. Treat 409 as ambiguous here, like 412, so reconcile_key_only_after_ambiguous_put can detect the exact stored key instead of reporting a false failure.
Useful? React with 👍 / 👎.
Summary
412/timeouts), exact-matchadd_encryption_key_only, and ambiguous key-only reconcile.FactorLookup::lookup_consistentfor post-ConditionalCheckFailedreads.Split from #136 (PR 1/4 — infra). No add-factor product API changes.
Test plan
cargo test --lib resolve_put_object_outcomecargo test --lib add_encryption_key_onlycargo test --lib reconcile_key_onlycargo test --lib factor_lookupcargo clippy --all-targets -- -D warningsNote
Medium Risk
Changes concurrency semantics for backup metadata and factor-lookup rollback; mistakes could drop lookups or mis-handle duplicate factors, though scope is storage infra with extensive new tests and no public API surface in this PR.
Overview
Factor metadata writes after ambiguous S3
PutObjectfailures (412, timeouts, 5xx) now reconcile using the attempted factor id, not onlyFactorKind. If the same identity is present but the id differs, callers getUnknown(FactorAlreadyExists)instead of treating the write as success—soFactorLookuprollback behavior stays correct under concurrent adds. Retries where the caller’s own id is already in metadata still count as success.Adds
add_encryption_key_onlyfor metadata-only encryption-key upgrades (exact key idempotent; same flattened kind with different material conflicts). Ambiguous puts re-read metadata and reconcile viareconcile_key_only_after_ambiguous_put.FactorLookup::lookup_consistentperforms strongly consistentGetItemreads (via sharedlookup_inner) for use after insert races such asConditionalCheckFailed, avoiding false “missing” on eventually consistent reads.Tests expand coverage for 412 vs timeout races, concurrent key-only writes, and the new reconcile paths.
Reviewed by Cursor Bugbot for commit e1fb42a. Bugbot is set up for automated code reviews on this repo. Configure here.