Renumbering keys streams by recording_run_id alone, so processes sharing one run id collapse silently
- 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