Persist CollectionJobReq on the Helper, and allow narrower batch selectors by jcjones · Pull Request #4766 · divviup/janus

jcjones · 2026-08-03T18:05:42Z

This enables collection extensions and batch selectors narrower than the query, but only does the work for the latter. Collection extensions are left to #4715 based on being unnecessary at this time.

Resolves #4743

Note

I changed the existing AggregateShareRequestRejected to yield a problem document rather than a blank page, which affects several tests, but feels more right. That said, there's no DAP Problem Type for this (see my note on http_handlers.rs:189) and maybe there should be? But also, then I'd need to break AggregateShareRequestRejected apart a bit since we're using it for several purposes and now I distinguish them based on the detail field.

…ctors This _enables_ collection extensions (#4715) and batch selectors narrower than the query, but only does the work for the latter. Collection extensions are left to #4715 based on being unnecessary at this time. Resolves #4743

tgeoghegan · 2026-08-04T15:06:22Z

 }

+#[test]
+fn collection_job_req_decode_encode_is_byte_identical() {


Did roundtrip_collection_job_req not already prove this? Or perhaps roundtrip_encoded could be extended slightly to check that for all types?

I'll add a case to roundtrip_encoded, good call.

in afefe0e

tgeoghegan · 2026-08-04T15:20:58Z

            | Error::TaskParameters(_) => StatusCode::INTERNAL_SERVER_ERROR.into_response(),
-            Error::AggregateShareRequestRejected(_, _) => StatusCode::BAD_REQUEST.into_response(),
+            // Note: DAP defines no error for this; `batchOverlap` may kinda fit the dupe
+            // cases, but it does not cover the too-late rejection that shares this variant.


I believe this was a case where we (DAP editors) felt that HTTP error semantics were sufficient and we didn't need a DAP-specific problem type. We have an existing Error::ForbiddenMutation used when Janus detects attempts to mutate aggregation jobs or collection jobs that I think would work here. We then serve an HTTP 409 Conflict which seems right to me.

However I do support constructing a problem details. Our precedent here has been to construct a document with a link to the errors page on docs.divviup.org. for example. Error::ForbiddenMutation contains:

{ resource_type: &'static str, identifier: String, }

...so you could use those values to render a nice explanation in the problem details as well as the link.

I think using about:blank as a URI would be fine as well, this case seems like it's fine to categorize as just "bad request".

Axes collection_job_req_decode_encode_is_byte_identical Changes duplicate aggregate share requests to be an HTTP Conflict Adds two new -- as of yet unpublished -- Janus-specific errors: - https://docs.divviup.org/references/janus-errors#aggregate-share-request-rejected - https://docs.divviup.org/references/janus-errors#forbidden-mutation

divergentdave · 2026-08-04T15:29:51Z

            | Error::TaskParameters(_) => StatusCode::INTERNAL_SERVER_ERROR.into_response(),
-            Error::AggregateShareRequestRejected(_, _) => StatusCode::BAD_REQUEST.into_response(),
+            // Note: DAP defines no error for this; `batchOverlap` may kinda fit the dupe
+            // cases, but it does not cover the too-late rejection that shares this variant.


I think using about:blank as a URI would be fine as well, this case seems like it's fine to categorize as just "bad request".

divergentdave · 2026-08-04T22:59:58Z

+                        // DAP requires duplicate requests to be identical.
                        if aggregate_share_job.aggregate_share_id() != &aggregate_share_id
+                            || aggregate_share_job.collection_job_req()
+                                != aggregate_share_req.collection_job_req()


We should check the batch selector as well, just in case. (The report count and checksum are okay to skip, as those are a diagnostic tool in the first place)

The literal check is a no-op, I think. BatchSelector<B> wraps nothing but B::BatchIdentifier so comparing the selectors is comparing the identifiers.

But, uh aggregate_share_id doesn't have anything ensuring its uniqueness. The dedup lookup is keyed on batch+param, so a leader that PUTs the same aggregate share ID against a different batch identifier doesn't hit our dedup path at all; it creates a second row sharing that ID.

The poll path then does WHERE aggregate_share_id = $2 via query_opt, which would return error for more than one row. Which becomes a 500.

I guess it's always been this way, but this PR makes the exposure a bit wider. Before, the helper derived the query from the stored batch identifier. A batch identifier had to equal its query interval exactly. Now that narrower selectors are consistent, one collection_job_req legitimately maps to many batch identifiers, so duping IDs is a more ... achievable? error condition.

I feel like I should guard against this on the cache miss scenario and do ForbiddenMutation, but I haven't figured out where yet.

I was referring to the batch_selector field of AggregateShareReq. That may be different than what is wrapped in the collection job request's query.

Ah, some of this may be left over from before aggregate shares had their own IDs. Doing a separate check by ID for an existing aggregate share would make sense I think.

I think we should also ensure that if a leader sends two identical aggregate share requests under different IDs, we send the same ciphertext in response to the second one instead of returning an error for overlapping batches. This could be useful for recovering from operational issues on the leader side without unnecessary data loss.

Perhaps a real fix would be to add a UNIQUE(task_id, aggregate_share_id) constraint to the aggregate_share_jobs table.

I was referring to the batch_selector field of AggregateShareReq. That may be different than what is wrapped in the collection job request's query.

Reasonably confident here that that would still be doing a comparison against its own clone. The case I think you're guarding against -- same query, different selector -- isn't going to make it here. A different selector is a different key, so it'll go down the cache-miss path.

I'm adding alookup by aggregate share ID there, which rejects binding one ID to two batches (with a 409), and that has turned up three existing tests that are re-using a single ID across different batches, all of which would have caused 500s if they hit the poll path. Which they didn't.

I'm also adding a test aggregate_share_request_same_id_different_batch to explicitly check this. Take a look at afefe0e.

tgeoghegan · 2026-08-05T16:02:12Z

+        take_problem_details(&mut response).await,
+        json!({
+            "status": StatusCode::CONFLICT.as_u16(),
+            "type": "https://docs.divviup.org/references/janus-errors#forbidden-mutation",


I guess a PR on divviup/public-docs is forthcoming to add this? No need to block this PR, FWIW.

Yeah, I have that started.

divviup/public-docs#196

jcjones added this to the draft-ietf-ppm-dap-17 milestone Aug 3, 2026

jcjones added the allow-changed-migrations Override the ci-migrations check to allow migrations that have changed. label Aug 3, 2026

allow migrations

fd2004a

jcjones marked this pull request as ready for review August 3, 2026 18:11

jcjones requested a review from a team as a code owner August 3, 2026 18:11

jcjones mentioned this pull request Aug 3, 2026

Rethink TaskConfiguration source-of-truth #4712

Open

tgeoghegan reviewed Aug 4, 2026

View reviewed changes

jcjones requested a review from tgeoghegan August 4, 2026 22:34

Remove unnecessary note

b73f8d5

divergentdave approved these changes Aug 4, 2026

View reviewed changes

tgeoghegan approved these changes Aug 5, 2026

View reviewed changes

jcjones mentioned this pull request Aug 5, 2026

Add a UNIQUE(task_id, aggregate_share_id) constraint to the aggregate_share_jobs table. #4768

Open

Review updates

afefe0e

jcjones mentioned this pull request Aug 5, 2026

Add "Forbidden Mutation" and "Aggregate Share Request Rejected" docs divviup/public-docs#196

Merged

jcjones merged commit 17cb1c4 into main Aug 5, 2026
8 checks passed

jcjones deleted the jcj/4743-persist-helper branch August 5, 2026 23:14

jcjones mentioned this pull request Aug 7, 2026

Implement draft-ietf-ppm-dap-17 #4402

Closed

13 tasks

Uh oh!

Conversation

jcjones commented Aug 3, 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!

Uh oh!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants