juspay / juspay/deja

Renumbering keys streams by recording_run_id alone, so processes sharing one run id collapse silently

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

Description

Found in review of #161 by a session that did not write the code.

## The hole

`Renumbering::apply` (`crates/deja-orchestrator/src/s3/mod.rs`) keys streams by `recording_run_id` alone:

```rust
let entry = high.entry(run.clone().unwrap_or_default()).or_default();
```

That catches **one run id spread across members** — the case the guard in `e33c81d` refuses. It does not catch the mirror: **several processes sharing one run id**. Those collapse into a single stream at offset 0, are never renumbered, and cannot be refused either, because with one stream there is nothing placed above it for the reservation check to collide with.

The colliding sequences the renumbering exists to remove survive untouched, and nothing is said anywhere.

## The fixtures already encode it

`at_boundary` (`s3/mod.rs:1482`) hardcodes `"recording_run_id":"r1"` and varies only `{instance}`. So `a_multi_member_pull_sums_its_members` builds `i1` at gseq 0 and `i2` at gseq 0 onto one tape and asserts counts only.

The fixture family most of the suite runs on is the unfixed shape. That is worse than an untested case — it is a tested case that encodes the defect as normal, which is why the guard was written without noticing it.

## Reachability

Same knob as the overlap case. `resolve_recording_run_id` reads `DEJA_RECORDING_RUN_ID`, then `DEJA_RUN_ID`, then falls back to `run-{now_ns}` per process. No recording deployment on infra `origin/main` sets any of them, nor the typed `CS__DEJA__RUN_ID` / `ROUTER__DEJA__RUN_ID` — but `infra-configurations/vector/sandbox-ucs-deja-s3.yaml:87` documents the typed form as settable.

Set it on a multi-pod recording deployment and every pod stamps one run id. **Of the two sibling shapes, this is the one that then bites**, and it is the one with no guard.

## Remedy, and its true cost

Key by `(recording_run_id, instance_id)`. A genuine resume keeps the same instance, so the deliberate continuation support in `apply` is preserved exactly; only co-run-id processes separate. Where run ids are already unique the pair collapses to today's behaviour.

It also makes the existing doc true as written: the counter belongs to the `RecordingHook`, and the hook is per process — `recording_run_id` identifies a process only because nothing forces uniqueness.

**It is not a one-line key change.** `instance_id` is on the envelope, outside `event`; `renumber_record` reads `global_sequence` from the top level of `raw` (`s3/mod.rs:889`), so `raw` is the inner event and does not carry it. It is available during collation (`s3/mod.rs:402`, the `by_instance` tally) and dropped there. Carrying it through means a fifth element on

```rust
Vec<(Option, &'static str, u64, String)> // s3/mod.rs:1343, :1439
```

which is destructured in roughly twenty places across ingest, the tally and both pull paths.

## Why this is a follow-up rather than part of #161

Widening a tuple used in twenty places, during an active review, on the branch sandbox deploys from, is a different risk from adding a guard beside existing logic. #161 already carries nine commits across five concerns. And because this is the shape that bites when the knob is set, it deserves its own review rather than being appended to a PR a reviewer is already holding whole.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in crates/deja-orchestrator/src/s3/mod.rs at Renumbering::apply, then trace instance_id through collation at the by_instance tally and the tuple uses around lines 1343 and 1439. Compare the at_boundary fixture and a_multi_member_pull_sums_its_members, and inspect resolve_recording_run_id for the shared-ID path. Done means shared run IDs no longer collapse separate processes, genuine resume behavior remains intact, and existing unique-run behavior is preserved.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.