fix: Correct relations pagination and recursion #2465

Henry-Hiles · 2026-11-06T23:00:40Z

Henry-Hiles commented

2026-11-06 23:00:40 +00:00

Ended up rewriting a fair bit, but should work better.

The only actual behavior change that might be confusing should be explained by https://github.com/matrix-org/matrix-spec/pull/2469. However, the actual code changes may not be good, as I'm not very familiar with Continuwuity's codebase, nor Rust in general. Please tell me if anything needs improving :)

Fixes #2467, fixes #2468, fixes #2469, fixes #2470, and fixes some other smaller bugs I didn't open issues for.

Pull request checklist:

This pull request targets the main branch, and the branch is named something other than
main.
I have written an appropriate pull request title and my description is clear.
I understand I am responsible for the contents of this pull request.
I have followed the contributing guidelines:
- My contribution follows the code style, if applicable.
- I ran pre-commit checks before opening/drafting this pull request.
- I have tested my contribution (or proof-read it for documentation-only changes)
  myself, if applicable. This includes ensuring code compiles.
- My commit messages follow the commit message format and are descriptive.

Ended up rewriting a fair bit, but should work better. The only actual behavior change that might be confusing should be explained by https://github.com/matrix-org/matrix-spec/pull/2469. However, the actual code changes *may* not be good, as I'm not very familiar with Continuwuity's codebase, nor Rust in general. Please tell me if anything needs improving :) Fixes #2467, fixes #2468, fixes #2469, fixes #2470, and fixes some other smaller bugs I didn't open issues for.      **Pull request checklist:**  - [x] This pull request targets the `main` branch, and the branch is named something other than `main`. - [x] I have written an appropriate pull request title and my description is clear. - [x] I understand I am responsible for the contents of this pull request. - I have followed the [contributing guidelines][c1]: - [x] My contribution follows the [code style][c2], if applicable. - [ ] I ran [pre-commit checks][c1pc] before opening/drafting this pull request. - [x] I have [tested my contribution][c1t] (or proof-read it for documentation-only changes) myself, if applicable. This includes ensuring code compiles. - [x] My commit messages follow the [commit message format][c1cm] and are descriptive.  [c1]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md [c2]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/docs/development/code_style.mdx [c1pc]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#pre-commit-checks [c1t]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#running-tests-locally [c1cm]: https://forgejo.ellis.link/continuwuation/continuwuity/src/branch/main/CONTRIBUTING.md#commit-messages

Henry-Hiles added 2 commits

2026-11-06 23:00:40 +00:00

fix: Correct relations pagination and recursion 2fc14e72a9

Ended up rewriting a fair bit, but should work better.

chore: Make Clippy happy

Documentation / Build and Deploy Documentation (pull_request) Has been skipped

Details

Auto Labeler / Apply labels based on changed files (pull_request_target) Successful in 5s

Details

Checks / Prek / Check changed files (pull_request) Successful in 6s

Details

Checks / Changelog / Check changelog is added (pull_request_target) Successful in 14s

Details

Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 2m13s

Details

Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 40m51s

Details

0a4aa77950

forgejo-actions added the

Changelog

Missing

label

2026-11-06 23:00:48 +00:00

Henry-Hiles added

and removed

labels

2026-11-06 23:01:03 +00:00

Henry-Hiles added 1 commit

2026-11-07 00:54:59 +00:00

chore: Add changelog to #2465

Documentation / Build and Deploy Documentation (pull_request) Has been skipped

Details

Checks / Prek / Check changed files (pull_request) Successful in 9s

Details

Checks / Changelog / Check changelog is added (pull_request_target) Successful in 6s

Details

Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 21m11s

Details

Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 40m48s

Details

bfe81cfbd0

forgejo-actions added

and removed

labels

2026-11-07 00:55:05 +00:00

nex commented

2026-11-07 02:42:29 +00:00

@Jade You've touched relations somewhat before, do you want to review this?

Aranjedeath approved these changes

2026-11-07 04:21:17 +00:00

Aranjedeath left a comment

no new explosions (threads work better)

Aranjedeath requested review from Jade

2026-11-07 04:21:27 +00:00

eleboucher requested changes

2026-11-07 08:54:45 +00:00

Dismissed

eleboucher left a comment

Looks good, but some perf issue that may arise with this patch

src/api/client/relations.rs

					
				@ -147,28 +151,49 @@ async fn paginate_relations_with_filter(

						.unwrap_or(30)

						.min(100);

limit=0 loops: has_more is true, events is empty after truncate, so next_batch == from. .clamp(1, 100)?

`limit=0` loops: has_more is true, events is empty after truncate, so next_batch == from. `.clamp(1, 100)`?

Uh, can you clarify?

I think this is correct behavior.

For a loop to happen, there would need to be a misbehaving client that passes limit=0 but spams the endpoint until it has events, but I mean, that's not something the server should handle? Clients shouldn't (and don't) pass limit=0 when they want events.

I don't see a reason to always return at least 1.

Uh, can you clarify? I think this is correct behavior. For a loop to happen, there would need to be a misbehaving client that passes `limit=0` but spams the endpoint until it has events, but I mean, that's not something the server should handle? Clients shouldn't (and don't) pass `limit=0` when they want events. I don't see a reason to always return at least 1.

Fair, it only loops with a broken client. Fine as is

Henry-Hiles marked this conversation as resolved

src/service/rooms/pdu_metadata/bundled_aggregations.rs

					
				@ -39,0 +38,4 @@

						let mut replace_events = Vec::new();

						let mut reference_events = Vec::new();

						self.db

							.get_relations(room_id, target, PduCount::max(), Direction::Backward)

No cap anymore, so a message with 5k reactions loads 5k PDUs on every /sync and /messages it shows up in. Can we keep a cap on the raw scan, maybe a few hundred?

The cap didn't properly take effect before, it was there but basically useless. I can add a cap though, maybe a maintainer can also give me an opinion on what to cap it to.

I think the proper fix here is actually indexing this column by relation type, so for stuff like bundled aggregations where it's only edits, we only need to search through edits.

I think a separate PR would be best for this though, and though it looks like there was a cap of 50 before, it never actually applied, so this is not a regression.

I think the proper fix here is actually indexing this column by relation type, so for stuff like bundled aggregations where it's only edits, we only need to search through edits. I think a separate PR would be best for this though, and though it looks like there was a cap of 50 before, it never actually applied, so this is not a regression.

You're right, with max_depth 0 the old code collected everything and never hit the limit check. Not a regression then, fine to do the rel_type index in a follow-up.

Henry-Hiles marked this conversation as resolved

src/service/rooms/pdu_metadata/data.rs

					
				@ -60,2 +57,3 @@

						&self,

						shortroomid: ShortRoomId,

						target: ShortEventId,

						target: u64,

nit: keep ShortEventId here

nit: keep `ShortEventId` here

Why?

This was never a short event Id, it's the event's position in the timeline (a pdu count) which is different data. The two types are just compatible, but it doesn't make sense to use here, as far as I can tell.

Why? This was never a short event Id, it's the event's position in the timeline (a pdu count) which is different data. The two types are just compatible, but it doesn't make sense to use here, as far as I can tell.

Right, it was always a pdu count. Ignore.

Henry-Hiles marked this conversation as resolved

src/service/rooms/pdu_metadata/mod.rs

					
				@ -113,3 +87,3 @@

						}

						pdus

						let scan_from = match dir {

With recurse, this loads every PDU in the tree (3 levels) on every page, then truncates to limit. Forward always scans from min. Could the walk use counts only, then sort and load PDUs lazily until limit + 1 match?

With recurse, this loads every PDU in the tree (3 levels) on every page, then truncates to `limit`. Forward always scans from min. Could the walk use counts only, then sort and load PDUs lazily until `limit + 1` match?

Good idea, I will add this.

Added. All my client's tests still pass.

Henry-Hiles marked this conversation as resolved

Henry-Hiles added 1 commit

2026-11-07 15:00:42 +00:00

chore: Make walk use counts only

Documentation / Build and Deploy Documentation (pull_request) Has been skipped

Details

Checks / Prek / Check changed files (pull_request) Successful in 9s

Details

Checks / Prek / Clippy and Cargo Tests (pull_request) Successful in 8m58s

Details

Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 20m55s

Details

Checks / Changelog / Check changelog is added (pull_request_target) Successful in 7s

Details

eec328ecb4

Henry-Hiles requested review from eleboucher

2026-11-07 15:15:32 +00:00

Jade added the

Bug

Matrix/Client

labels

2026-11-07 15:52:35 +00:00

eleboucher approved these changes

2026-11-07 17:05:35 +00:00

eleboucher left a comment

LGTM

Jade approved these changes

2026-11-08 18:10:09 +00:00

Dismissed

Jade left a comment

TBH this is running into the same things I was when trying to use the existing relations database table, it's fine for now.

src/service/rooms/pdu_metadata/mod.rs

					
				@ -116,0 +99,4 @@

							.await;

						seen.extend(level.iter().copied());

						for _ in 0..max_depth {

							let next: Vec<Vec<_>> = level

Can you break out of this loop when there's nothing in level?

Jade marked this conversation as resolved

Henry-Hiles commented

2026-11-08 19:08:13 +00:00

@Jade wrote in #2465 (comment):

TBH this is running into the same things I was when trying to use the existing relations database table, it's fine for now.

I'm planning to revisit this, with a migration, Ginger has more details.

@Jade wrote in https://forgejo.ellis.link/continuwuation/continuwuity/pulls/2465#issuecomment-39080: > TBH this is running into the same things I was when trying to use the existing relations database table, it's fine for now. I'm planning to revisit this, with a migration, Ginger has more details.

Henry-Hiles added 1 commit

2026-11-08 19:10:22 +00:00

chore: break out of loop when level is empty

Documentation / Build and Deploy Documentation (pull_request) Has been skipped

Details

Checks / Prek / Check changed files (pull_request) Successful in 7s

Details

Checks / Changelog / Check changelog is added (pull_request_target) Successful in 9s

Details

Checks / Prek / Pre-commit & Formatting (pull_request) Successful in 1m1s

Details

Checks / Prek / Clippy and Cargo Tests (pull_request) Failing after 40m47s