feat(miner): rfc 0023 green pt1 — bounded template memory (RFC0023.1–.5) - #354
Merged
Conversation
max_node_children (100): full prefix nodes route unseen tokens through a <*> wildcard child; attach below it stays simSeq-gated (routing is not merging). max_templates (20k): a per-tenant leaf ceiling tracked by a new leaf_count (rebuilt on snapshot restore); at the ceiling both mint arms divert to the §6.3 parse-failure path with the body retained — never force-merge (§3.1). max_line_tokens (512): over-long lines fail parse pre-tree, subsuming the old u16::MAX audit-width guard. RFC0023.1-.5 green; .6 stays stubbed for the telemetry slice; the RFC 0004 tunables tripwire classifies the new knobs as tunables inside the invariants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds three RFC 0023 bounded-memory tunables to ChangesRFC 0023 bounded memory caps
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MinerCluster
participant Tree
participant TenantState
participant Parquet
Client->>MinerCluster: ingest log records
MinerCluster->>Tree: descend / descend_mut with max_node_children
Tree-->>MinerCluster: routed template or wildcard path
MinerCluster->>TenantState: check leaf_count and max_templates
alt overflow or ceiling reached
MinerCluster->>MinerCluster: emit parse-failure record with NO_TEMPLATE
else accepted
MinerCluster->>TenantState: increment leaf_count and template_count
end
Client->>Parquet: encode mined records
Parquet-->>Client: read back overflow body bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements RFC 0023 bounded-template-memory controls in the miner by adding three configurable bounds (tree fan-out, per-tenant template ceiling, and per-line token-width) and turning the corresponding RFC §5 scenarios green via new/updated tests.
Changes:
- Add
MinerConfigbounds (max_node_children,max_templates,max_line_tokens) with validation and builders. - Enforce the bounds in the miner: wildcard routing once a prefix node hits the child cap, template mint diversion at the per-tenant leaf ceiling, and early parse-failure for over-tokenized lines.
- Add/upgrade RFC 0023 scenario tests, including an ingester-path Parquet body round-trip for overflow lines.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-core/src/config.rs | Introduces and validates the three new RFC 0023 bound fields on MinerConfig. |
| crates/ourios-miner/src/tree.rs | Adds wildcard-child routing to cap per-node keyed fan-out on both read and write paths. |
| crates/ourios-miner/src/cluster.rs | Enforces max_templates (leaf ceiling) and max_line_tokens, and threads max_node_children into tree traversal. |
| crates/ourios-miner/tests/rfc0023_bounded_memory.rs | Turns RFC0023.1/.3/.4/.5 green with miner-level scenario tests. |
| crates/ourios-ingester/tests/rfc0023_overflow_roundtrip.rs | Implements RFC0023.2 integration test: overflow body round-trips through Parquet. |
| crates/ourios-miner/tests/rfc0004_configuration_policy.rs | Updates the RFC 0004 config-policy compile-time tripwire to include the new tunables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/ourios-miner/src/tree.rs (1)
185-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
max_node_childrencontract in the public doc comments.The
descend_mut/descendrustdoc blocks above these signatures weren't updated to explain the new parameter — its wildcard-routing behavior and the invariant that read and write sides must be called with the same value (or the "children only grow" reasoning indescend_immutable's inline comment breaks) is currently only documented on the private helpers, not on thesepub fnsignatures.Also applies to: 221-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-miner/src/tree.rs` around lines 185 - 204, Update the public rustdoc for descend_mut and descend to document max_node_children, including that it controls wildcard-routing behavior and must be kept identical between read and write calls so the PrefixNode/descend_immutable invariants remain valid. Add the contract directly on the public signatures rather than only on descend_recursively or other private helpers, and make sure the docs reference the same behavior described by descend_mut, descend, and descend_immutable.crates/ourios-miner/tests/rfc0023_bounded_memory.rs (1)
45-100: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a property test for the
max_templatesceiling invariant.RFC0023.1 verifies the ceiling with one fixed 6-line, 3-ceiling example. The invariant itself (
leaf_countnever exceedsmax_templates, and once at ceiling every would-mint line diverts to parse-failure with body retained) generalizes cleanly to arbitrary ceilings and arbitrary distinct-shaped line counts, and is a good proptest candidate per the crate's testing guideline.As per coding guidelines, "Use property tests (
proptest) for anything with an invariant."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-miner/tests/rfc0023_bounded_memory.rs` around lines 45 - 100, The RFC0023.1 test only checks one fixed example, but the `max_templates` ceiling is an invariant and should be covered with a property test. Refactor `rfc0023_1_template_ceiling_holds_and_never_merges` in `crates/ourios-miner/tests/rfc0023_bounded_memory.rs` into a `proptest`-based test that varies the ceiling and the number of distinct-shaped input lines, while asserting `templates_for(...).len()` never exceeds `MinerConfig::with_max_templates(...)` and that any would-mint overflow line is routed to parse-failure with `NO_TEMPLATE`, `lossy_flag`, and retained body. Use the existing `MinerCluster`, `SharedRecordSink`, and `template_set` helpers to keep the same behavior checks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ourios-core/src/config.rs`:
- Around line 331-375: Add tests covering the zero-bound validation in
MinerConfig’s builder methods so the guard clauses don’t regress. In the config
tests for `MinerConfig::with_max_node_children`,
`MinerConfig::with_max_templates`, and `MinerConfig::with_max_line_tokens`,
assert that passing `0` returns `MinerConfigError::BoundZero` with the expected
field name string for each method. Keep the tests focused on these three methods
and their zero-input rejection behavior.
In `@crates/ourios-miner/src/cluster.rs`:
- Around line 1276-1290: The ceiling-divert handling is duplicated in
cluster.rs, so extract the repeated record-envelope construction, overflow
retention, emit, record_parse_failure, and early return logic into a shared
helper in the Cluster implementation. Reuse that helper from both
at_template_ceiling call sites so the behavior stays identical and future
changes only need to be made in one place; keep the helper centered around the
existing record_envelope, apply_overflow_retention, emit_record, and
record_parse_failure flow.
In `@crates/ourios-miner/src/tree.rs`:
- Around line 265-303: Add a proptest-based property test for the bounded-fanout
invariant around `descend_recursively`/`descend_immutable` and `keyed_children`.
Generate arbitrary token sequences and `max_node_children` values, build the
tree through `Tree::descend_mut`, then walk the visited nodes and assert the
keyed child count never exceeds the cap. Cover the wildcard-reuse path too, so
the test exercises the same routing logic that can trigger an off-by-one in
`keyed_children` or `WILDCARD_CHILD` handling.
---
Nitpick comments:
In `@crates/ourios-miner/src/tree.rs`:
- Around line 185-204: Update the public rustdoc for descend_mut and descend to
document max_node_children, including that it controls wildcard-routing behavior
and must be kept identical between read and write calls so the
PrefixNode/descend_immutable invariants remain valid. Add the contract directly
on the public signatures rather than only on descend_recursively or other
private helpers, and make sure the docs reference the same behavior described by
descend_mut, descend, and descend_immutable.
In `@crates/ourios-miner/tests/rfc0023_bounded_memory.rs`:
- Around line 45-100: The RFC0023.1 test only checks one fixed example, but the
`max_templates` ceiling is an invariant and should be covered with a property
test. Refactor `rfc0023_1_template_ceiling_holds_and_never_merges` in
`crates/ourios-miner/tests/rfc0023_bounded_memory.rs` into a `proptest`-based
test that varies the ceiling and the number of distinct-shaped input lines,
while asserting `templates_for(...).len()` never exceeds
`MinerConfig::with_max_templates(...)` and that any would-mint overflow line is
routed to parse-failure with `NO_TEMPLATE`, `lossy_flag`, and retained body. Use
the existing `MinerCluster`, `SharedRecordSink`, and `template_set` helpers to
keep the same behavior checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2890cee8-eee3-48e0-9100-0b3547fd0166
📒 Files selected for processing (6)
crates/ourios-core/src/config.rscrates/ourios-ingester/tests/rfc0023_overflow_roundtrip.rscrates/ourios-miner/src/cluster.rscrates/ourios-miner/src/tree.rscrates/ourios-miner/tests/rfc0004_configuration_policy.rscrates/ourios-miner/tests/rfc0023_bounded_memory.rs
…overflow exit Five duplicated emit blocks (below-floor zone, degenerate-widening rejection, long-line guard, both ceiling diverts) collapse onto one helper — the overflow contract now has a single implementation, and attach_and_maybe_widen drops back under the clippy line budget. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t, zero-bound test Plus canonical format_template rendering in the RFC 0023 oracle helper (Debug output is not a stable form), a drain-length assertion in the long-line scenario, and the §6.4 call site named in the shared parse-failure helper's doc. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
crates/ourios-miner/src/tree.rs:195
Tree::descend_mutnow takesmax_node_children, but it doesn't validate that the cap is non-zero. Passing0makes every node appear “full” and forces wildcard routing, which would collapse routing unexpectedly. Since this is a public API, add a precondition assert (mirroring the config-levelBoundZeroguarantee) so misuse fails fast.
assert!(
!masked.is_empty(),
"descend_mut precondition: masked must be non-empty",
);
crates/ourios-miner/src/tree.rs:231
Tree::descendtakesmax_node_childrenbut doesn’t validate it. A0cap causes the read-side routing rule to degenerate (everything treated as overflow), which can make candidate selection diverge from intent. Add a non-zero precondition assert to fail fast on invalid input.
assert!(
!masked.is_empty(),
"descend precondition: masked must be non-empty",
);
…ms to test The previous input missed the second-level prefix and minted via no-candidate, making the no-merge assertion vacuous. It now shares gamma's exact bucket (wildcard route + worker prefix, 1/4 similarity < the 0.4 floor) and pins NO_TEMPLATE + an unchanged template count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Implements RFC 0023 §3.1's three bounds and turns five of the seven §5 stubs green.
What
max_node_children(default 100) —descend/descend_mutroute unseen tokens through a<*>wildcard child once a prefix node's keyed children hit the cap. Read and write sides share the routing rule (children only grow, so a token's route can never diverge between them); attach below the wildcard child stayssimSeq-gated — routing is not merging.max_templates(default 20,000) — a per-tenant Drain-leaf ceiling on a newTenantState.leaf_count(incremented at the single leaf-push site, rebuilt fromleaves.len()on snapshot restore). At the ceiling, both mint arms (no-candidate and §6.3 lossy-zone) divert to the parse-failure path: body retained bit-for-bit, counted,NO_TEMPLATE. Existing leaves keep widening — the ceiling stops growth, not matching.max_line_tokens(default 512) — over-long lines fail parse before any tree work, subsuming the previousu16::MAXaudit-width guard (theu16config type keeps every accepted line inside the RFC 0001 §6.4 position width by construction).MinerConfigfields (BoundZerorejection,with_*builders, doc-table rows). The RFC 0004 §3.3 tunables tripwire fired as designed and now classifies the knobs: tunables inside the invariants — overflow diverts, never merges, never drops.Scenarios
NO_TEMPLATE+ retained bodies, and the capped template set equals the uncapped run truncated at the ceiling (no silent merge)..6(telemetry) stays stubbed for the semconv slice;.7is the scale-rerun bench criterion.Invariants / hazards (CLAUDE.md §3/§4)
NO_TEMPLATEwith the body retained; RFC0023.1/.3 pin it.docs/benchmarks.md§9.10) — worst-case tree memory becomes a computable product of the caps.Verification
cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warnings,cargo test --all-features(117 suite blocks, 0 failures — including the corpus gates under the new defaults) — all green locally.🤖 Generated with Claude Code
Summary by CodeRabbit