Skip to content

impl: recursive rocksdb multi get - #557

Draft
gamesguru wants to merge 8 commits into
matrix-construct:mainfrom
gamesguru:guru/impl/rocksdb-recursive-multi_get
Draft

impl: recursive rocksdb multi get#557
gamesguru wants to merge 8 commits into
matrix-construct:mainfrom
gamesguru:guru/impl/rocksdb-recursive-multi_get

Conversation

@gamesguru

Copy link
Copy Markdown

What does this PR do?

Adds an optimized method which improves the speed of recursive queries (traversing deep forks can sometimes contribute to lag during peak federation hours). Puts an entire recursive query behind a single tokio::spawnBlocking() call and batches key lookups in RocksDB's monotonic order (for sequential reads and hot cache locality).

This PR does NOT hook the method up to anything (it's just there for future use). I don't have any explicit benchmark data yet, but it should be quite significant in many workloads.

Checklist

  • Code is formatted with nightly cargo fmt and satisfies clippy and
    rustc lints; any allowed lint is justified by an obvious reason or a
    comment.
  • Complement compliance changes (new passes or new failures), if any,
    are noted in the description above.
  • Config option changes were made in src/core/config/mod.rs doc
    comments and the regenerated tuwunel-example.toml is committed.
  • User-facing changes are reflected in docs/.
  • I agree that my changes may be licensed under the Apache-2.0 licence
    and my conduct is in line with the Contributor's Covenant and
    Tuwunel's Code of Conduct.

Summary by cubic

Adds Map::recursive_multi_get for breadth-first DAG traversal over RocksDB, returning parsed values, missing keys, and a truncation flag. Improves deep ancestor/fork queries via snapshot reads; not wired to production yet.

  • Ordering and bounds: BFS with per-level key sort, root deduplication, and visited tracking; limits via max_nodes/max_depth. When max_nodes is hit mid-batch, the method now consumes the full multi-get batch to record missing keys and surface RocksDB errors without parsing additional values, and sets truncated = true.
  • Consistency and performance: snapshot-based reads; batched multi-get with SORTED=true; pre-allocated child sink; per-level server shutdown checks; fail-fast error mapping via util::map_err.
  • API and tests: exports RecursiveGetOutput from map.rs and database/mod.rs. Unit tests cover cycles, diamond convergence, missing keys, depth/node truncation, and mid-batch truncation behavior. Migration behavior unchanged (minor Option::zip refactor in service/migrations/injectivity/repair.rs).

Written for commit f249ba9. Summary will update on new commits.

Review in cubic

- Add Map::recursive_multi_get for iterative breadth-first graph traversal over database keys
- Pre-allocate next_batch capacity to prevent heap reallocations during Matrix PDU ancestor fetches
- Enforce strict fail-fast error mapping via util::map_err on RocksDB I/O or block corruption errors
…d tests

- Implement Map::recursive_multi_get with max_nodes/max_depth bounds and RecursiveGetOutput
- Add point-in-time RocksDB snapshot, key sorting, and root deduplication
- Check server shutdown per BFS level and track missing keys
- Add zero-allocation child extraction sink buffer (&mut Vec<K>)
- Add unit tests for cycles, diamond convergence, missing keys, and depth/node truncation
- Restore deferred key conversion in repair.rs migration
Update `recursive_multi_get` in `get_batch.rs` so that when `max_nodes` is hit
mid-batch, remaining results in the batch continue to be iterated to record
missing keys and propagate RocksDB errors without parsing additional values.
Strengthen test assertions in `tests.rs` and add mid-batch truncation tests.
Copilot AI balanced review requested due to automatic review settings August 17, 2026 05:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@jevolk

jevolk commented Aug 17, 2026

Copy link
Copy Markdown
Member

We already have a concurrent multi-threaded recursive traversal for e.g. deep auth-chain forks...

Why would we want the single-threaded sequential version with worst-case tail-latency?

@gamesguru

Copy link
Copy Markdown
Author

In the context of walking live DAGs, this method offers snapshot isolation and cache-friendly bulk reads.

It's possible we need to add intra-level parallelism to make this handle skinny/linear DAGs better.

Potential use cases include recursive relations and path finding (where /backfill fails or stalls indefinitely, e.g., M-HQ or Unrdct Lounge incidents).

@jevolk

jevolk commented Aug 18, 2026

Copy link
Copy Markdown
Member

where /backfill fails or stalls indefinitely, e.g., M-HQ or Unrdct Lounge incidents

I have a strong suspicion you are conflating Tuwunel with other software. Can you file an issue which focuses on the problem and its reproduction rather than offer a questionable solution which we can't take as-is even if it were an effective solution.

We're also going to need exact details for the model, prompt, and base context you're using which came to this solution before we can meaningfully proceed on this.

@jevolk
jevolk marked this pull request as draft August 18, 2026 02:06
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.

3 participants