juspay / juspay/deja

A recorded redis DELETE is not a precondition, so cross-correlation deletes always diverge on replay

Open
#162 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
2
Forks
1
Avg merge
21h 32m
Merged PRs (30d)
67

Description

A recorded redis `DELETE` proves the key existed, but that evidence never reaches the seed plan. Every delete whose key was set by a *different* correlation therefore diverges on replay, blocking, against a candidate that did nothing wrong.

Measured on `rec-6097dd0-09161413-q8`, run `rp-sbx-6097dd0c92-6097dd0-09161413-q8-0916201249692`: 13 of 14 value divergences are this, all blocking.

## What the run shows

```
DEL cyMerchant_151007bd
recorded: KeyDeleted observed: KeyNotDeleted blocking: true
```

Four things were measured and ruled out before the cause was found, all from the run's own artifacts:

| ruled out | evidence |
|---|---|
| in-memory cache warmth | `get_val` is `replay = Substitute`, so an L1 read replays from the tape and cannot change which redis calls happen |
| L1 eviction reaching L2 | an in-memory `remove` cannot touch a redis key |
| concurrent drive | no `CONCURRENT DRIVE` line in the log; `deja-kernel: complete (driven 100, skipped 0)` in recorded order |
| TTL expiry | the whole drive took **7,948 ms** against a 300s `default_ttl` |

Two structural facts point at the cause:

- each diverged DEL and its matched `KeyNotDeleted` partner are in the **same** correlation, adjacent (5 of 6)
- the `set_key` preceding each diverged DEL is in a **different, earlier** correlation — **6 of 6, no exceptions**

## Cause

`RedisConnectionPool::add_prefix` (`crates/redis_interface/src/module/redis_rs.rs:547` in hyperswitch, `6097dd0c92`) namespaces every physical key by the active correlation during replay:

```rust
#[cfg(feature = "deja")]
if let Some(corr) = deja::replay_key_namespace() {
return format!("{corr}:{physical}");
}
```

So a SET in correlation A writes `A:key` and a DEL in correlation B addresses `B:key`. The DEL correctly finds nothing.

That isolation is deliberate and worth keeping — it is what makes it safe to `Execute` stateful redis ops against a seeded store, with no cross-case collision and no read-modify-write double-apply. The defect is not the namespacing. It is that **the precondition the DEL implies is never seeded into B's namespace.**

## Why it is never seeded

`build_seed_plan` (`crates/deja-runtime/src/replay.rs:2983`) derives preconditions from one source:

```rust
for key in &event.read_set {
```

Reads only. And a redis delete carries **no read set** — neither `set_key` nor `delete_key` declares one, they use the bare `deja::redis` preset, and `read_set` is an `Option` in the derive emitted only when declared (`crates/deja-derive/src/instrument.rs:656`).

So the recorded result `KeyDeleted` — which is direct evidence that the key was present — contributes nothing.

## The same function already states the opposite rule

From `build_seed_plan`, a few lines above the loop:

> Once a correlation CREATES rows in a table, we stop seeding its subsequent reads of that table: it reconstructs its own rows via its writes on replay. **UPDATE/DELETE mutate PRE-EXISTING rows, which remain genuine preconditions to seed.**

That rule is honoured on the `db` arm, because a DB `UPDATE`/`DELETE` carries a read set naming the rows it matched. It is silently not honoured on `redis`, because a redis delete carries none. One stated principle, two boundaries, one implementation.

## Fix shape

Declare a `read_set` on `delete_key` naming the key it deletes, so a recorded `KeyDeleted` becomes a precondition the planner materialises into that correlation's namespace.

**The caveat that needs deciding:** a delete's result gives *presence*, not *value* — `DelReply` is effectively a boolean. So the seed would be a placeholder. That is sound for delete semantics, which only need existence, and the surrounding cases are already covered:

- if the correlation **read** the key before deleting it, that read is in the read set and seeds the correct value; the existing "pristine until the first write" rule resolves the overlap
- if it **only** deletes, nothing observes the placeholder
- a read **after** the delete correctly finds absence

What this must **not** do is borrow the value from the earlier correlation's `SET`. The value is on the tape, but reaching across correlations is exactly what the isolation prevents, and the same file records what that costs: seeding a post-image instead of a pre-image previously produced "13 payment_methods timestamp divergences and their 13 readback misses on run-0811".

## Before a PR

This changes what every correlation's store contains, so the blast radius should be measured rather than assumed:

1. how many keys across a real tape would newly be seeded
2. whether any currently-correct divergence is masked by a placeholder appearing where the recording had nothing
3. whether `set_key` needs the symmetric treatment, or deliberately does not

The property the current scheme gives for free is "a seeded key came from a recorded read". Relaxing it should come with the replacement guarantee written down.

## Scope note

Not a candidate defect and not related to any hyperswitch fix under test — the affected calls are merchant-account cache invalidation during test setup (`POST /accounts`, `GET /accounts/{id}`), not a payment path. It is noise a reviewer has to learn to ignore, which is how the ones worth acting on get missed.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with build_seed_plan in crates/deja-runtime/src/replay.rs, then inspect delete_key instrumentation in crates/deja-derive/src/instrument.rs and Redis namespacing in crates/redis_interface/src/module/redis_rs.rs. Measure the affected keys and replay behavior on the referenced runs before deciding whether delete_key should declare a read_set. Done means the placeholder-presence behavior, any set_key symmetry, and the replacement guarantee are documented and covered by evidence or tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
redis, rust
Domain
databases
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.