Repository navigation
id(encryption): bound Session held messages at 1024 / 16 MiB (#186) - #2216
Merged
Merged
Conversation
Session held every unreadable application message until its group secret arrived, with no cap: a removed or stalled peer could grow the held set without bound. Per the pinned design from triage task 2031: MAX_HELD_MESSAGES = 1024 and MAX_HELD_BYTES = 16 MiB (sum of CBOR-encoded envelope lengths, tracked on the session as messages are held and released). process() checks the caps after the id-dedupe and before the push - and before any crate call, so a refusal never poisons the session - and refuses a new arrival past either cap with the new SchemeError::HeldFull { count, bytes, cap_count, cap_bytes }, wired into reason() as None like TooManyParents. Already-held messages are never evicted or expired in-session; they leave only via release() when their secret arrives, and the tracked totals drop there.
Tests (encryption::tests): count cap (1024 held, the 1025th refused and not held, re-delivery at the cap still Withheld), byte cap (two >half-cap messages cannot both be held; refusal names count/bytes/caps; re-delivery does not double-count), and release (re-add welcome releases the held message, fresh messages read normally, and after a second removal a new big message is held again, proving the totals dropped).
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 17 minutes. View limit details
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. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74c91b7bf9
ℹ️ 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".
Member
Author
|
Agent note — queue task 2976, Lane D, 2026-10-29: CI diagnosis for head Not mergeable. CI failures triaged:
Task 2976 is parked with this resume brief. Worktree (laptop): |
…length (#186) release() re-encoded the whole waiting set to recompute held_bytes on every readable message (Codex P2): held entries now carry the CBOR-encoded length recorded when the message was held, and release() re-sums those retained sizes. The byte-cap test fixtures used 9 MiB plaintext, whose ciphertext CBOR-encodes as an integer array at ~1.9 bytes/byte (~17 MiB) - over the 16 MiB cap for a single message (Codex P2); fixtures are now 6 MiB plaintext (~12 MiB encoded, over half the cap). Also wraps the HeldFull error string per rustfmt.
main is red on cargo-fmt/rustfmt/treefmt/clippy (issue #2151, PR #2335), so every open PR inherits failing checks. This carries the identical two fixes (byte-for-byte the #2335 diff) on this branch: - meta_client.rs: rustfmt the capped_response_error match arm. - tags.rs: the two clippy::expect_used errors in parse_search_query become let-else breaks; the loops guard on i < input.len(). Once #2335 lands, these hunks are a merge no-op.
Member
Author
|
🤖 Agent note · parked
|
… is -D in clippy-lint)
dezren39
added this pull request to the merge queue
Oct 29, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to Branch Protection failures
Oct 29, 2026
You're not authorized to push to this branch. Visit "About protected branches" for more information.
dezren39
added this pull request to the merge queue
Oct 29, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to Branch Protection failures
Oct 29, 2026
You're not authorized to push to this branch. Visit "About protected branches" for more information.
dezren39
added this pull request to the merge queue
Oct 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #186 (queue task 2976), implementing the design pinned by triage task 2031.
Change
Sessioninpkgs/id/src/encryption/scheme.rsheld every unreadable application message until its group secret arrived — no count cap, no byte cap, no expiry — so a removed or stalled peer could grow the held set without bound.MAX_HELD_MESSAGES = 1024andMAX_HELD_BYTES = 16 MiB(sum of CBOR-encoded envelope lengths viaencode_cbor, asEnvelope::newencodes; the total is tracked on the session as messages are held/released, never re-encoded per arrival).Session::process()enforces both caps after the existing id-dedupe and before the push — and before any crate call, so a refusal never poisons the session — refusing a new arrival with the newSchemeError::HeldFull { count, bytes, cap_count, cap_bytes }, styled onTooManyParentsand wired intoSchemeError::reason()the same way (None).release()when their secret arrives; the held field's doc comment states this.Tests (new in
encryption::tests)HeldFullnaming the caps and not held (second delivery refuses again); re-delivery of a held message at the cap still returnsWithheld.Verification is CI on this PR (fleet CI-first rule; no sandbox/laptop builds).
Need help on this PR? Tag
@codesmith-botwith what you need.Branch hygiene (2026-10-29, queue task 2976 resume)
origin/mainmerged in ate00154d1. main is red on cargo-fmt/rustfmt/treefmt/clippy (#2151), which made every PR's checks fail; commit0556d2bacarries the identical two-file fix from #2335 (byte-for-byte the same diff) so this PR is verifiable on its own. When #2335 lands from the merge queue those hunks become a no-op.Local (laptop) verification on this head:
cargo fmt --all -- --check— cleancargo test --lib encryption::— 74 passed, 0 failed (includes the three new held-cap tests)cargo test --lib tags::— 68 passed, 0 failedcargo clippycould not run (staletarget/built with rustc 1.98.1 vs toolchain 1.99.0 — E0514, an environment artifact, not a code finding); CI's clippy-lint covers it, and the previous run's only clippy failures were main's tags.rs expects, fixed here.