fix(service): Make compare_and_write idempotent#401
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
|
|
||
| Ok(matches) | ||
| Ok(matches_current || matches_next) |
There was a problem hiding this comment.
InMemory backend skips writes Bigtable would apply
Medium Severity
The InMemoryBackend only applies the write when matches_current is true, but returns true whenever matches_current || matches_next. In Bigtable, the write mutation always fires when the predicate indicates a "safe" state (including the idempotent/matches_next path). For the (Some(old), None) transition where the row holds an unrelated object, Bigtable's delete mutation fires and removes it, while InMemoryBackend preserves it — both returning true. This means the test double has different side effects from the real backend, which can mask data-loss scenarios in TieredStorage integration tests that rely on InMemoryBackend.
Additional Locations (1)
There was a problem hiding this comment.
TL;DR: Both backends adhere to the spec. The detail is in the check_and_write docs:
Whether the mutation runs again is up to the implementation.
Effectively, this is a race and the trait leaves this as implementation-defined behavior (not undefined behavior!). The contract that matters is that cleanup can happen, hence the trait leaves this up to implementors:
InMemoryBackendchooses the most correct implementation and skips the side-effect since the precondition is violated.BigTableopts for a simpler filter chain and does run the mutation. It may overwrite a concurrent inline-insert, but this is explicitly OK for the contract (both are no tombstone, which is what matters to idempotency).
Give the two CAS predicate builders semantic names that reflect what they test rather than how they're built: `interleave_filter` → `update_filter`, `redirect_compatible_filter` → `optional_target_filter`. Add `## Predicate Matches` / `## Details` doc sections to all four filter helpers so callers know which mutations branch to pair them with. Extract `TieredWrite::target()` to avoid duplicating the match in every call site.
| // `invert`: filters that use false_mutations (predicate_matched == false means safe). | ||
| let (predicate_filter, invert) = match (current, write.target()) { |
There was a problem hiding this comment.
The invert flag existed before and is a necessary evil. I tried to get rid of it, but CheckAndMutate only returns predicate_matched if at least one row matched the predicate. So it's not possible to create a negative match. The workaround is to use false_mutations and invert the flag.
I will eventually follow up by typing this properly. The predicates will become either a trait or a struct that has the necessary methods to generate the correct CheckAndMutateRowRequest along with the correct predicate matching.
This can all be deferred to a follow-up.
1 participant
The
compare_and_writefunction needs to become idempotent to allow for retries on error and after crashes. Previously, if the operation was repeated, its precondition fails and callers receivefalse— signalling the CAS has failed.The fix is to broaden each CAS predicate to also match when the row is already in the target state. The transition
(current, next)— wherenextisSome(target)for tombstone writes andNonefor object/delete writes — determines which row states are safe:!tt == oldt == newt == otherNone → Some(new)!t or t == newSome(old) → Some(new)t == old or t == newSome(old) → None!t or t == oldNone → None!tWhen the predicate matches an already-committed state, the write still fires (Bigtable's CAS API cannot skip it), but it is a no-op: tombstone writes overwrite with identical data, and deletes on an absent row are no-ops.