Skip to content

quest(m5): key moq-net's model by explicit ids, with no cfg(rust2_ts) twins - #8124

Merged
kixelated merged 1 commit into
mainfrom
quest-plan/rs2ts-explicit-ids
Nov 7, 2026
Merged

kixelated merged 1 commit into
mainfrom
quest-plan/rs2ts-explicit-ids

Conversation

@kixelated

@kixelated kixelated commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

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

  • Track and FrameContent get a unique id at construction, a u64 from a counter. cache.rs's deadlines and frame.rs's per-group frames are keyed by it instead of Arc::as_ptr(..) as usize and ptr::from_ref(self) as usize in Drop. This is the same in native and translated builds.
  • The cfg(rust2_ts) safe twins are gone from the plan. The internal unsafe (the MaybeUninit frame ring, NonNull in track.rs and track/demand/ranges.rs, frame.rs's Segment, kio's lock-free log) 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:

  • Address keys: ✅ explicit ids, the same in both builds (rejected: a cfg(rust2_ts) twin; rust2-ts translating Arc addresses).
  • cfg(rust2_ts) twins: ✅ none anywhere (rejected: only dropping the address-key twin).
  • The other unsafe items: ✅ 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 check passes every lint and check. Its test gh module fails in sh/go/publish-wrapper.test.sh: under a global tag.gpgsign = true, the script's git tag opens an editor and exits non-zero ("there was a problem with the editor 'vim'"). The same script fails identically on origin/main, so the failure predates this change. Suggested follow-up: run the test's git with an isolated config (for example GIT_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 review after each round.

  • Reviewer: Codex, gpt-6.1-sol, low effort
  • Session: 01a1b40b-ef1b-7ec3-8731-f02ea0aafa32, round 1
  • Head: 703728e
  • Base: origin/main at 7a3669b
  • Score: 10/10, The plan records settled identity and shared-implementation decisions without changing executable behavior, and no actionable defects remain at the reviewed head.
  • Findings: none

… twins

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review November 7, 2026 03:36
@kixelated
kixelated added this pull request to the merge queue Nov 7, 2026

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

@coderabbitai

coderabbitai Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The plan now specifies explicit u64 IDs for Track and FrameContent in native and translated builds, replacing address-based identity keys. It removes the proposal for cfg(rust2_ts) safe twins and differential tests. The plan states that each listed unsafe item will either be rewritten in safe Rust at measured native cost or deferred to the rust2-ts unsafe epic. It also records decisions against cfg-specific twins and leaves the choice for each unsafe item until the quest starts.

Merge Risk: Minimal · up to 70372

This planning update introduces no runtime changes or material merge-blocking risk. The shared-build identity decision and deferred unsafe-code choices are consistent with the eventual translation goal.

Pre-merge checks: Passed 5

Check name Status Explanation
Title check Passed The title names the main change: keying moq-net's model by explicit ids and dropping the cfg(rust2_ts) twins. The raw summary, description, and objectives all describe this change to the quest plan do…
Description check Passed The description states the purpose of the change. It links the plan file, explains the maintainer decision on pointer addresses, lists the changes to Track, FrameContent, cache.rs and frame.rs, and re…
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: keying moq-net's model by explicit ids and dropping the cfg(rust2_ts) twins. The raw summary, description, and objectives all describe this change to the quest plan document.

Full details: Description check

Explanation

The description states the purpose of the change. It links the plan file, explains the maintainer decision on pointer addresses, lists the changes to Track, FrameContent, cache.rs and frame.rs, and records the decisions and validation steps. It matches the changeset.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)

Finishing Touches

Simplify code

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4778306e-8aa6-4597-b12f-1aaead7b286a
Commits

Reviewing files that changed from the base of the PR and between 7a3669b and 703728e.

Files selected for processing: 1

  • quest/m5/rs2ts-model-subset.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 — Track and FrameContent take a u64 id at construction, keying cache.rs's deadlines and frame.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 remaining unsafe item 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.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

Comment on lines +22 to +23
- 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

@pullfrog pullfrog Bot Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
- 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.

Comment on lines +18 to +21
- `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.

@pullfrog pullfrog Bot Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
- `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.

Comment on lines +24 to +25
`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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
`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.

Merged via the queue into main with commit 17341dc Nov 7, 2026
10 checks passed
@kixelated
kixelated deleted the quest-plan/rs2ts-explicit-ids branch November 7, 2026 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant