Repository navigation
quest(m5): key moq-net's model by explicit ids, with no cfg(rust2_ts) twins - #8124
Conversation
… twins Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 703728e
Base: 7a3669b
No actionable findings in this planning-only change.
Direction: explicit construction-time identity for Track and FrameContent removes the dependency on observable pointer addresses, and the same implementation in native and translated builds avoids maintaining divergent lifetime behavior. The plan clearly leaves each remaining unsafe rewrite versus translator dependency to a later measured decision. These choices match the companion rust2 #628 plan inspected at 48edaddfdbe8dd28c8b8b67179b87e2743695f2e; that companion change is not yet reflected in rust2's main model-epic text. This PR changes the plan, not the model implementation or the translator's supported operations.
Verification: GitHub-only static inspection of the complete diff, surrounding quest and companion rust2 plan. No quest checks, tests, cross-language differential runs or performance measurements were executed. Check run 42334808239 was in progress when inspected; the validation reported in the PR description was not independently reproduced.
(Written by review (OpenAI))
|
No actionable comments were generated in the recent review. WalkthroughThe plan now specifies explicit Merge Risk:
|
There was a problem hiding this comment.
Important
The plan records the settled decisions correctly, but two census entries would send the implementation quest after code that is not there: NonNull is listed in two files that contain no unsafe, and kio's log is the one item the "leave it to the unsafe epic" option cannot cover.
Reviewed changes
Reviewed the single-document change to quest/m5/rs2ts-model-subset.md at 703728e:
- Explicit ids for the address keys —
TrackandFrameContenttake au64id at construction, keyingcache.rs's deadlines andframe.rs's per-group frames by it in place of their addresses. cfg(rust2_ts)twins removed — the plan commits to identical native and translated builds, with each remainingunsafeitem decided when the quest starts.- Decision record added — the 2026-11-07 decisions from rust2#628 are captured as a checkmark list, seeded by the closed rust2#596 design.
The two notes below are about the plan's accuracy rather than any executable behavior; the code this PR touches is documentation only.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
| - The internal `unsafe`: `model/group.rs`'s `MaybeUninit` frame ring, `NonNull` in | ||
| `track.rs` and `track/demand/ranges.rs`, `frame.rs`'s `Segment`, and kio's lock-free |
There was a problem hiding this comment.
The model's unsafe census is wrong here: track.rs and track/demand/ranges.rs contain no unsafe and no NonNull at this head; the only NonNull in the model's path is kio/src/log.rs, which the same bullet already lists. The model's other unsafe is model/frame.rs's Segment, so this clause should be dropped.
| - The internal `unsafe`: `model/group.rs`'s `MaybeUninit` frame ring, `NonNull` in | |
| `track.rs` and `track/demand/ranges.rs`, `frame.rs`'s `Segment`, and kio's lock-free | |
| - The internal `unsafe`: `model/group.rs`'s `MaybeUninit` frame ring, `frame.rs`'s | |
| `Segment`, and kio's lock-free |
Technical details
# The `NonNull` census entry points at no code
## Affected sites
- `quest/m5/rs2ts-model-subset.md:22-23` — lists `NonNull` in `track.rs` and `track/demand/ranges.rs`.
- `rs/moq-net/src/model/track.rs` and `rs/moq-net/src/model/track/demand/ranges.rs` — no `unsafe` and no `NonNull` at this head.
- `rs/moq-net/src/model/frame.rs:859` — `Segment` is the only `unsafe` in the model besides `group.rs`'s `MaybeUninit` ring.
- `rs/kio/src/log.rs:27` — the only `NonNull` in the model's path, already covered by the separate kio `log` item.
## Required outcome
- Drop the `NonNull` clause or re-point it at the real `unsafe` sites.| - `Track` and `FrameContent` take a unique id at construction, a `u64` from a counter, and | ||
| key `cache.rs`'s deadlines and `frame.rs`'s per-group frames by it in place of their | ||
| addresses (`Arc::as_ptr(..) as usize`, and `ptr::from_ref(self) as usize` in `Drop`). | ||
| rust2-ts refuses every address read, while `ptr::eq` and `Arc::ptr_eq` translate. |
There was a problem hiding this comment.
The parenthetical names Arc::as_ptr and ptr::from_ref, but frame.rs's Drop uses self as *const Self and SharedWeak::address uses as_ptr, so those two frame.rs reads are not explicit. Naming the four reads makes the plan's checklist complete.
| - `Track` and `FrameContent` take a unique id at construction, a `u64` from a counter, and | |
| key `cache.rs`'s deadlines and `frame.rs`'s per-group frames by it in place of their | |
| addresses (`Arc::as_ptr(..) as usize`, and `ptr::from_ref(self) as usize` in `Drop`). | |
| rust2-ts refuses every address read, while `ptr::eq` and `Arc::ptr_eq` translate. | |
| - `Track` and `FrameContent` take a unique id at construction, a `u64` from a counter, and | |
| key `cache.rs`'s deadlines (`Arc::as_ptr(..) as usize` and `ptr::from_ref(self) as usize`) | |
| and `frame.rs`'s per-group frames (`self as *const Self` and `SharedWeak::address`) by it | |
| in place of their addresses. rust2-ts refuses every address read, while `ptr::eq` and | |
| `Arc::ptr_eq` translate. |
Technical details
# The address-key parenthetical omits two of the four reads
## Affected sites
- `rs/moq-net/src/model/cache.rs:485` — `Arc::as_ptr(track) as usize` in `schedule`.
- `rs/moq-net/src/model/cache.rs:1040` — `std::ptr::from_ref::<Self>(self) as usize` in `Drop`.
- `rs/moq-net/src/model/frame.rs:517` — `self as *const Self as usize` in `Drop`.
- `rs/moq-net/src/model/frame.rs:535` — `self.0.as_ptr() as usize` in `SharedWeak::address`.
## Required outcome
- Name the four sites (or the four forms) so the implementation does not miss the `frame.rs` `Drop` and `SharedWeak::address` reads.| `log`. Each is either rewritten in safe Rust for both builds, at measured native cost, or | ||
| left for rust2-ts's [unsafe epic](https://github.com/moq-dev/rust2/blob/main/quest/m3/ts-unsafe/README.md) |
There was a problem hiding this comment.
The plan offers "left for rust2-ts's unsafe epic to translate as is" for every item, but rust2's model epic says kio's log.rs retires through crossbeam-epoch, whose tagged pointers and inline closures translation refuses "for good"; the log must be rewritten or dropped, not deferred. This is the one item the option cannot cover, so it should be called out separately.
| `log`. Each is either rewritten in safe Rust for both builds, at measured native cost, or | |
| left for rust2-ts's [unsafe epic](https://github.com/moq-dev/rust2/blob/main/quest/m3/ts-unsafe/README.md) | |
| `log`. Each is either rewritten in safe Rust for both builds, at measured native cost, or | |
| left for rust2-ts's [unsafe epic](https://github.com/moq-dev/rust2/blob/main/quest/m3/ts-unsafe/README.md) | |
| to translate as is, except kio's `log`, which that epic refuses for good. |
Technical details
# `kio`'s log cannot be left to the unsafe epic
## Affected sites
- `quest/m5/rs2ts-model-subset.md:24-25` — the plan allows leaving each item to rust2-ts's unsafe epic.
- `rs/kio/src/log.rs:34` — imports `crossbeam_epoch`.
- `rs/kio/Cargo.toml:46` — `crossbeam-epoch = "0.9.21"`.
- rust2's model epic (`quest/m2/ts-model/README.md`) says `kio`'s `log.rs` retires through crossbeam-epoch, whose tagged pointers and inline closures translation refuses "for good".
## Required outcome
- Scope the "left for the unsafe epic" option to the items the epic can model, or state that `kio`'s log must be rewritten or dropped.
Updates Keep moq-net's model in rust2-ts's subset after rust2's maintainer decided on 2026-11-07 that rust2-ts keeps refusing pointer addresses (moq-dev/rust2#596 closed, moq-dev/rust2#628 records it).
Changes
TrackandFrameContentget a unique id at construction, au64from a counter.cache.rs's deadlines andframe.rs's per-group frames are keyed by it instead ofArc::as_ptr(..) as usizeandptr::from_ref(self) as usizeinDrop. This is the same in native and translated builds.cfg(rust2_ts)safe twins are gone from the plan. The internalunsafe(theMaybeUninitframe ring,NonNullintrack.rsandtrack/demand/ranges.rs,frame.rs'sSegment, kio's lock-freelog) is either rewritten in safe Rust for both builds, at measured native cost, or left for rust2-ts's unsafe epic to translate as is.Decisions
Settled with the maintainer on 2026-11-07:
cfg(rust2_ts)twin; rust2-ts translatingArcaddresses).cfg(rust2_ts)twins: ✅ none anywhere (rejected: only dropping the address-key twin).unsafeitems: ✅ decided per item when the quest starts (rejected: wait for rust2-ts; safe rewrite now).Public API: none.
Validation
quest check: 600 documents ok.just checkpasses every lint and check. Itstest ghmodule fails insh/go/publish-wrapper.test.sh: under a globaltag.gpgsign = true, the script'sgit tagopens an editor and exits non-zero ("there was a problem with the editor 'vim'"). The same script fails identically onorigin/main, so the failure predates this change. Suggested follow-up: run the test's git with an isolated config (for exampleGIT_CONFIG_GLOBAL=/dev/null), so the host's signing settings can't reach it.Models
Size: XS
Implementer: Claude Code, claude-opus-5-5, high effort
(Written by Claude Opus 5.5 with high effort)
🤖 Generated with Claude Code
Review
Rewritten by
quest reviewafter each round.01a1b40b-ef1b-7ec3-8731-f02ea0aafa32, round 1The plan records settled identity and shared-implementation decisions without changing executable behavior, and no actionable defects remain at the reviewed head.