fix(mailroom): reject post requests over capacity by va-an · Pull Request #1509 · payjoin/rust-payjoin

va-an · 2026-04-28T12:54:26Z

Both post_v2 and post_v1_req_and_wait grow the in-memory counters tracked by len(), but only wait_v2 was guarded by has_capacity(). This patch applies the same check to the two write paths so the capacity bound is enforced symmetrically.

Pull Request Checklist

Please confirm the following before requesting review:

I have disclosed my use of AI in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

Both `post_v2` and `post_v1_req_and_wait` grow the in-memory counters tracked by `len()`, but only `wait_v2` was guarded by `has_capacity()`. Apply the same check to the two write paths so the capacity bound is enforced symmetrically.

nothingmuch

utACK, not sure how I missed this (when i wrote it)

i would be happier if this included regression tests similar to the existing capacity tests

nothingmuch · 2026-04-28T16:30:18Z

        id: &ShortId,
        payload: Vec<u8>,
    ) -> Result<Option<oneshot::Receiver<Vec<u8>>>, Error> {
+        if !self.has_capacity().await? {


note that technically in this fn it could be that the v2 removal makes room for the v1 insert.

i think it's better this way since the code is simpler, but technically it implies v1 writers will in some cases see effective capacity of n-1 instead of n

That's not obvious, thanks for pointing it out.
I'll come back to this when I understand the code better.

IMO don't bother fixing this, maybe just add a comment to indicate that this edge case exists

Add regression tests covering `OverCapacity` rejection from each DB method that the capacity check guards: `post_v2_payload`, `post_v1_request_and_wait_for_response`, and `wait_for_v2_payload`. Long-poll tests spawn parallel waiters and poll `Mailboxes::len` until `pending_v1`/`pending_v2` are full before probing. Extract `random_id` and `random_payload` helpers shared across the three new tests.

va-an · 2026-04-29T09:02:56Z

i would be happier if this included regression tests similar to the existing capacity tests

Thanks for the review.
Couldn't find existing capacity tests to mirror, so I added one per guarded DB method.

caarloshenriq · 2026-04-29T19:40:42Z

+
+fn random_id() -> ShortId { ShortId(OsRng.next_u64().to_ne_bytes()) }
+
+fn random_payload() -> Vec<u8> { rand::random::<[u8; 32]>().to_vec() }
+


When a ran using cargo run The compiler returns two warnings about these functions being unused. Since they are only used in tests, you can use #[cfg(test)]

Suggested change

fn random_id() -> ShortId { ShortId(OsRng.next_u64().to_ne_bytes()) }

fn random_payload() -> Vec<u8> { rand::random::<[u8; 32]>().to_vec() }

#[cfg(test)]

fn random_id() -> ShortId { ShortId(OsRng.next_u64().to_ne_bytes()) }

#[cfg(test)]

fn random_payload() -> Vec<u8> { rand::random::<[u8; 32]>().to_vec() }

in the rest of the codebase when we have this kind of stuff we usually add

#[cfg(test)] mod tests { ... }

around all the tests to reduce the boilerplate, i can't decide if it's worth it in this case

Agreed, wrapping everything in a #[cfg(test)] mod tests { } block is cleaner and more consistent with the rest of the codebase. It avoids annotating each helper individually and makes it clear at a glance that all of it is test-only code

I would prefer to wrap tests in files.rs to mod tests {} for consistency with codebase, even we decided to delete this helper functions below.

@nothingmuch should I add commit hash d8afeaa to .git-blame-ignore-revs file? And if so - what's the accepted way to do this: a commit in this PR, a separate PR, etc?

should I add commit hash d8afeaa to .git-blame-ignore-revs file?

no need IMO, git blame has -w option, ignore-revs is more useful for massive reformatting that isn't trivially addressed by git's move/copy detection or -w

And if so - what's the accepted way to do this: a commit in this PR, a separate PR, etc?

in general, just another commit in the PR makes the most sense to me, it's impossible to do it in one commit but that's the closest thing to that

nothingmuch · 2026-04-29T21:17:45Z

Thanks for the review. Couldn't find existing capacity tests to mirror, so I added one per guarded DB method.

test_prune is the test i had in mind FWIW

nothingmuch · 2026-04-29T21:21:08Z

+fn random_id() -> ShortId { ShortId(OsRng.next_u64().to_ne_bytes()) }
+
+fn random_payload() -> Vec<u8> { rand::random::<[u8; 32]>().to_vec() }


i think these functions can be removed, you can just use the loop index as the id

nothingmuch · 2026-04-29T21:28:13Z

+    assert!(matches!(
+        db.post_v2_payload(&random_id(), random_payload()).await,
+        Err(DbError::OverCapacity)
+    ));


all mailboxes share the same capacity, so i think it'd be more correct to assert that whenever there's no capacity, this is true of all request types:

Suggested change

assert!(matches!(

db.post_v2_payload(&random_id(), random_payload()).await,

Err(DbError::OverCapacity)

));

assert!(matches!(

db.post_v2_payload(&random_id(), random_payload()).await,

Err(DbError::OverCapacity)

));

assert!(matches!(db.wait_for_v2_payload(&random_id()).await, Err(DbError::OverCapacity)));

assert!(matches!(

db.post_v1_request_and_wait_for_response(&random_id(), random_payload()).await,

Err(DbError::OverCapacity)

));

same for the lower checks (so this can be a utility function, or even just a single test which always asserts this for a variety of different at-capacity setups)

Move existing tests under a `mod tests {}` submodule. No functional changes.

Drop the `random_id` and `random_payload` helpers in favor of deterministic `ShortId([i; 8])` and a fixed payload.

Capacity is shared across all request types, so once it is full every path must reject with `OverCapacity` regardless of how it was filled. Extract the assertion into a helper and call it from each capacity test.

nothingmuch

utACK

caarloshenriq

tACK 48cc7c7

spacebear21

ACK

spacebear21 · 2026-05-04T15:45:06Z

@va-an sorry for the delay in running the CI workflows, can you fix the lint errors before we merge?

va-an · 2026-05-04T16:27:19Z

@va-an sorry for the delay in running the CI workflows, can you fix the lint errors before we merge?

Done - 24840c5. These lint errors didn't show up locally under devenv.
Want me to squash into a single commit before merging?

coveralls · 2026-05-04T16:28:58Z

Coverage Report for CI Build 25330149521

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.1%) to 85.088%

Details

Coverage increased (+0.1%) from the base build.
Patch coverage: 10 uncovered changes across 2 files (306 of 316 lines covered, 96.84%).
15 coverage regressions across 1 file.

Uncovered Changes

File	Changed	Covered	%
payjoin-mailroom/src/db/files.rs	315	306	97.14%
payjoin-mailroom/src/main.rs	1	0	0.0%

Coverage Regressions

15 previously-covered lines in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
payjoin-cli/src/app/v2/ohttp.rs	15	74.24%

Coverage Stats


Relevant Lines:	13526
Covered Lines:	11509
Line Coverage:	85.09%
Coverage Strength:	396.36 hits per line

💛 - Coveralls

nothingmuch · 2026-05-04T16:46:05Z

These lint errors didn't show up locally under devenv. Want me to squash into a single commit before merging?

i think this is the result of the toolchain (provided by dtolnay/rust-toolchain@nightly action) bumping clippy version, it's not related to your PR at all

spacebear21

reACK

fix(mailroom): reject post requests over capacity

68ae86d

Both `post_v2` and `post_v1_req_and_wait` grow the in-memory counters tracked by `len()`, but only `wait_v2` was guarded by `has_capacity()`. Apply the same check to the two write paths so the capacity bound is enforced symmetrically.

nothingmuch approved these changes Apr 28, 2026

View reviewed changes

nothingmuch reviewed Apr 28, 2026

View reviewed changes

va-an requested a review from nothingmuch April 29, 2026 09:03

caarloshenriq reviewed Apr 29, 2026

View reviewed changes

va-an mentioned this pull request Apr 29, 2026

mailroom: make has_capacity prune lazier #1511

Closed

nothingmuch reviewed Apr 29, 2026

View reviewed changes

va-an added 4 commits April 30, 2026 13:39

refactor(mailroom): wrap tests in mod tests

d8afeaa

Move existing tests under a `mod tests {}` submodule. No functional changes.

test(mailroom): use deterministic test data

70a8ad7

Drop the `random_id` and `random_payload` helpers in favor of deterministic `ShortId([i; 8])` and a fixed payload.

docs(mailroom): note v1 writer capacity edge case

732e1e7

test(mailroom): cover all paths in capacity tests

48cc7c7

Capacity is shared across all request types, so once it is full every path must reject with `OverCapacity` regardless of how it was filled. Extract the assertion into a helper and call it from each capacity test.

va-an requested review from caarloshenriq and nothingmuch April 30, 2026 10:33

nothingmuch approved these changes Apr 30, 2026

View reviewed changes

caarloshenriq approved these changes Apr 30, 2026

View reviewed changes

spacebear21 approved these changes May 4, 2026

View reviewed changes

chore(lint): make contrib/lint.sh happy

24840c5

spacebear21 self-requested a review May 4, 2026 16:53

spacebear21 approved these changes May 4, 2026

View reviewed changes

spacebear21 merged commit e3dec5b into payjoin:master May 4, 2026
16 checks passed

va-an deleted the fix/post-capacity-check branch May 4, 2026 16:56

This was referenced May 4, 2026

Lint CI job should use clippy from nix provided toolchain #1521

Closed

chore: update flake.lock #1518

Closed

benalleng mentioned this pull request Jun 7, 2026

Release payjoin-1.0.0-rc.3 #1613

Closed

17 tasks

spacebear21 mentioned this pull request Jun 24, 2026

Bump payjoin-mailroom version to 0.1.2 #1677

Merged

This was referenced Jul 15, 2026

Release payjoin-cli-1.0.0-rc.0 #1738

Closed

Bump payjoin-cli version to 1.0.0-rc.0 #1739

Merged


		fn random_id() -> ShortId { ShortId(OsRng.next_u64().to_ne_bytes()) }

		fn random_payload() -> Vec<u8> { rand::random::<[u8; 32]>().to_vec() }

Uh oh!

Conversation

va-an commented Apr 28, 2026

Uh oh!

nothingmuch left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

va-an commented Apr 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch commented Apr 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

caarloshenriq left a comment

Choose a reason for hiding this comment

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

spacebear21 commented May 4, 2026

Uh oh!

va-an commented May 4, 2026

Uh oh!

coveralls commented May 4, 2026

Coverage Report for CI Build 25330149521

Coverage increased (+0.1%) to 85.088%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

nothingmuch commented May 4, 2026

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nothingmuch left a comment •

edited

Loading