Share resumed rollout history - #28426
Conversation
967b40f to
ebc8c5f
Compare
ebc8c5f to
90188f2
Compare
| .as_ref() | ||
| .map(|history| history.items.clone()) | ||
| .take() | ||
| .map(|history| history.items) |
There was a problem hiding this comment.
I like the ownership transfer here, but the running-thread resume branch still clones source_thread.history and then clears it immediately. Can we put the same take() there? Otherwise this improvement becomes marginal
There was a problem hiding this comment.
Should be fixed now
90188f2 to
eb2ebf6
Compare
8ff6e1f to
e3e7a90
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3e7a90018
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| semver = "1.0" | ||
| sentry = "0.46.0" | ||
| serde = "1" | ||
| serde = { version = "1", features = ["rc"] } |
There was a problem hiding this comment.
This changes the workspace serde dependency features, but the commit does not update MODULE.bazel.lock. The repo requires running just bazel-lock-update and committing the refreshed lockfile whenever Cargo.toml/Cargo.lock dependencies change; otherwise Bazel lock checks can reject the change or use stale crate-universe metadata for the new rc feature. Please regenerate the Bazel lockfile with this dependency update.
AGENTS.md reference: AGENTS.md:L37-L40
Useful? React with 👍 / 👎.
e3e7a90 to
c85a4f6
Compare
Summary
Resuming a persisted thread currently deep-clones its complete rollout history several times.
InitialHistoryis retained for the app-server response, copied into thread persistence, and copied again by read-only accessors. These copies scale with the complete rollout rather than the bounded model context and add measurable latency for large sessions.This change stores resumed rollout history in
Arc<Vec<RolloutItem>>. Rollout loading wraps the parsed vector once, while app-server response construction, session initialization, and thread persistence share it through inexpensiveArcclones. Read-only history access now returns a borrowed slice, and fork paths useArc::unwrap_or_clonewhere they genuinely need mutable ownership. Rollout reconstruction also consumes its temporary context instead of cloning the reconstructed model history.The serialized representation remains unchanged. In an artificial 123 MB rollout benchmark, sharing resumed history reduced cold resume latency by roughly 9–10%. The affected crates compile with their test targets, all 80 thread-store tests pass, and the Bazel dependency lock remains valid.