Repository navigation
id(waiting): add a total cap to the waiting pool (#1567) - #2940
Conversation
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. |
|
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 22 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. |
![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: c1f791db5f
ℹ️ 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".
| pub const fn with_total(self, total: usize) -> Self { | ||
| Self { total, ..self } | ||
| } |
There was a problem hiding this comment.
This changes the public waiting-limit API and default admission behavior, including durable deletion of excess waiting rows on reopen, but no dated design record accompanies it. Repository-wide Markdown searches for with_total, DEFAULT_TOTAL, and total waiting caps found no document covering this change. Add the protocol-required dated document recording the 8,192 default, refusal policy, and restore/drop semantics; module docstrings and the commit message do not satisfy the required documentation convention for API and behavior changes.
AGENTS.md reference: pkgs/id/AGENTS.md:L159-L168
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in cd92cf3 by adding the dated design record in doc/2026-10-31T04-00-00Z_design_waiting_pool_total_cap/2026-10-31T04-00-00Z_design_waiting_pool_total_cap.md covering the 8,192 default, refusal policy, and restore/drop semantics.
| assert_eq!(store.waiting_ids().unwrap().len(), 2); | ||
| assert_eq!(ids.iter().filter(|id| store.is_waiting(id)).count(), 2); | ||
| store.check_invariants().unwrap(); | ||
| let store = reopen(&dir, limits, 0); |
There was a problem hiding this comment.
The new regression test opens the same database again while the previous store still owns its handle. Shadowing a Rust binding does not drop its previous value, so this reopen returns StoreError::InUse and panics at the helper's unwrap() instead of checking durability. The existing test_a_second_opener_is_refused_while_the_first_is_open in event_store/mod.rs explicitly verifies this behavior. Call drop(store) or end its scope before reopening.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9738686 by scoping the middle store.
|
Correction to this PR's test record: the laptop test counts in the PR body and the earlier agent note (waiting 29/29, ingest 102/102) came from a background command delivery that proved unreliable on re-check. Foreground re-runs found one real failure — my new reopen test kept its second store open across the third reopen ( Verified foreground counts now: |
Fixes #1567.
The waiting pool's caps were per-author (256) and age (7 days) only. Author keys are self-certifying, so minting authors is free and the pool total — memory plus the redb waiting rows on disk — was unbounded, contradicting the module doc's claim that the caps bound what a peer can make a node hold.
WaitingLimitsgains a total cap across all authors:DEFAULT_TOTAL= 8,192 (32 × the per-author cap), exposed viatotal()and settable via the newwith_totalbuilder.WaitingLimits::newkeeps its signature and defaults the total.hold()— the one path insert, park and restore all funnel through — refuses with the newWaitRefusal::PoolFullwhen the pool is at the total, checked before the per-author cap. The refusal is distinct fromAuthorFullso pool-level callers can tell a Sybil-shaped flood from one author over its share; the event store's ingest surfaces both as its one waiting-full refusal, since the sender's remedy is the same.reloadin event_store/mod.rs and the mirror restore in waiting_rows.rs) — the earliest arrivals are kept, the excess rows are deleted, and the store opens. It never bricks.Tests (laptop):
cargo test --lib waiting— 29 passed, including three new ones: the (total+1)-th event across distinct authors below their per-author caps is refusedPoolFull; park and restore are bound by the total too; restore over a lowered total keeps the earliest arrivals.cargo test --lib ingest— 102 passed, including a new store test: three authors holding one waiting event each, reopened underwith_total(2), opens with two held, invariants pass, and the drop is durable across another reopen.Queue task 1836.
Need help on this PR? Tag
@codesmith-botwith what you need.