Leader-selected collection job IDs by tgeoghegan · Pull Request #4897 · divviup/janus

tgeoghegan · 2026-10-13T14:43:24Z

Transfers responsibility for choosing collection job IDs from the collector to the Leader, per incoming DAP changes in 1.

In order to make this work for the Leader-Selected batch mode, we also add an idempotency key to the Leader-Selected batch mode query config (also detailed in 1).

The route/handler for collection job creation:

is now POST instead of PUT
no longer includes a collection job ID
provides colleciton job ID in a Location header
sets HTTP 200 OK status and a retry-after header
checks for replayed creation requests by comparing the query instead
of the collection job ID

And we add an index on (task_id, query) on the collection jobs table to make replayed collection job lookups faster.

jcjones · 2026-10-13T18:29:33Z

+          0) >= $3",
+            )
+            .await?;
+        self.query_opt(


query_opt here breaks if there is more than one matching row, and that's quite feasible here. If two identical POSTs run concurrently, both find no existing job, both insert with different random() IDs, and both commit -- then this will error trying to look up the job later, because our uniqueness guarantee isn't enforced enough.

We could add a UNIQUE (task_id, query) index, or derive the job ID differently.

Combining this with your other comment about extensions being included in uniqueness, here's what I think we need:

derive the collection job ID by hashing the collection job req, including extensions

replace get_collection_job_by_query with get_collection_job_by_request which uses the entire request, byte for byte, to recognize collection jobs

We already have a UNIQUE(task_id, collection_job_id) constraint which will do the job.

Does that sound right?

I don't think it's necessary to get the collection job ID from a hash, we could make things work either with that or with a random ID, and I think I'd prefer using random IDs for consistency. I think we will still need a UNIQUE index that includes the query column to address the problems JC identified, both race conditions and collection jobs with different extensions, and particularly a race condition between two collection job initialization requests with the same query and different sets of extensions. The latter combination would not be caught by UNIQUE(task_id, collection_job_id) with content-derived collection job IDs.

Yeah, that sounds like what I was leaning toward - making those IDs be a predictable function of the whole job.

I'm arguing for deriving collection job IDs by hashing the entire collection job req, including the extensions (note J.C.'s observation that a collection job req's identity now includes every byte of the request, including extensions: https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/blob/e20e3e531765f43228521a5ffbbe9b0fdadef80c/draft-ietf-ppm-dap.md?plain=1#L3366-L3369). If we do that, then I believe the UNIQUE(task_id, collection_job_id) constraint addresses all the races we're concerned about, since the digest will incorporate extensions.

It also absolves us of needing to store collection job extensions on the leader side, which we would otherwise be forced to do in order to check them against new requests.

One consequence of this is that we wouldn't be reporting illegal mutations anymore, but as you noted w.r.t. the aggregation parameter variation, we should be reporting BatchOverlap there.

Thinking about it again, a UNIQUE alone is still going to 500, at least in the degenerate case. I think we need an ON CONFLICT ... RETURNING with a re-select. Or some other retryable mechanism.

I'm also realizing now that we have to handle the idempotency specially. If we get two leader selected collection job requests that differ only in aggregation parameter, then we can't treat them as two distinct collection jobs consuming two batches. We have to flag the fact that we've previously seen that idempotency key.

I wound up re-writing a good bit of handle_create_collection_job_inner to achieve the behavior we want.

First, we query for all collection jobs that intersect the incoming query using a new method on trait CollectableBatchMode. Then we check if any of them exactly matches the incoming request, in which case we have seen a replayed request and can serve the result. If there are intersections that don't match the incoming request, then a request parameter has been mutated and we fail with batchOverlap.

The existing CollectableBatchMode::validate_query_count, which used to be responsible for checking for overlap, is now slated for deletion, once the aggregate share request handler that still uses it gets updated accordingly (which I will do in a later PR, as it's part of the overall draft-ietf-ppm-dap-20 alignment work).

Additionally, I changed the plumbing so that the POST /tasks/{task-id}/collection_jobs route can serve a CollectionJobResp body, in the case where a job already existed.

Looks good... but my gut says that this is still only safe for LeaderSelected modes, unless we have the DB impose uniqueness.

jcjones · 2026-10-13T18:44:49Z

                    // match.
                    if let Some(collection_job) = tx
-                        .get_collection_job::<SEED_SIZE, B, A>(&vdaf, task.id(), &collection_job_id)
+                        .get_collection_job_by_query::<SEED_SIZE, B, A>(


The query plus the agg parameter isn't sufficient anymore:
https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/blob/e20e3e531765f43228521a5ffbbe9b0fdadef80c/draft-ietf-ppm-dap.md?plain=1#L3366-L3369

This was already on my radar, but we should open an issue. Basically, a re-POST with the same query and agg param but different extensions silently returns the existing job, and the new extensions are ignored.

jcjones · 2026-10-13T18:50:34Z

+    client_timestamp_interval, helper_aggregate_share, leader_aggregate_share,
+    aggregate_share_id, query
+FROM collection_jobs
+WHERE collection_jobs.task_id = $1


Should this skip deleted jobs?

After a DELETE, re-POSTing the same time-interval query returns the deleted job's location, and GET on it returns DeletedCollectionJob So this change now blocks re-collecting the same time interval after a delete.

Yeah, this is subtle. At a minimum, we need more test coverage here.

I'm not certain if we should allow un-deletion of collection jobs. Even if we verify that the query, aggregation parameter and extensions haven't changed since it was first created, we can't guarantee that we are aggregating over the same set of reports. Suppose this sequence of events happens:

collection job gets created with some set of parameters P

collection job gets serviced successfully, aggregating over a set of reports R

collection job gets deleted

garbage collection runs, deleting a set of reports G that intersects with R

a "new" collection job gets created with parameters P

Now, if we allow un-deletion, we'd re-run the collection job, but only aggregate the set R \ G, which would leak information, right?

This leads me to conclude that the behavior we want here is that if we notice an attempt to un-delete a collection job, we should synchronously return HTTP 204 No Content (but not redirect the HTTP client to a URI that will yield that same response).

What do we think?

I don't think garbage collection should be a concern, as we should start treating all data that's old enough to be garbage collected as if it doesn't exist already. Plus, when we process the collection job the first time, we scrub all the batch_aggregations rows, so all that's left for the relevant time interval is the aggregate share.

I'm sure that the second attempt at collecting the same batch should fail at some point, but less sure about which request should get an error, and what sort of error should be used. Looking at the "Collection Job Initialization" section of the spec, I suppose we could use the batchOverlap problem type. If we return an error during collection job initialization, then we don't have to decide on what the collection job ID to use in that case.

You're correct that neither garbage collection or report expiry matters here because batch aggregation scrubbing makes the collection job or aggregate share request unserviceable.

I took a quick look at how we're handling this kind of case independent of this change, and it's actually pretty wonky. The aggregate share handler in the helper barfs up an HTTP 500 because it doesn't handle the BatchScrubbed error internally. I'm not sure exactly what happens on the leader side with collection job requests, because at this point I think this question needs enough attention and enough new tests that it should be considered in its own PR independent of this stack.

My plan is to pursue the questions of re-creation of deleted collection jobs and aggregate shares in #4911. In this PR, we'll leave the behavior "undefined", in that we don't have test coverage for it.

divergentdave · 2026-10-13T20:29:01Z

+          0) >= $3",
+            )
+            .await?;
+        self.query_opt(


I don't think it's necessary to get the collection job ID from a hash, we could make things work either with that or with a random ID, and I think I'd prefer using random IDs for consistency. I think we will still need a UNIQUE index that includes the query column to address the problems JC identified, both race conditions and collection jobs with different extensions, and particularly a race condition between two collection job initialization requests with the same query and different sets of extensions. The latter combination would not be caught by UNIQUE(task_id, collection_job_id) with content-derived collection job IDs.

divergentdave · 2026-10-13T20:55:19Z

+        // Assuming we're always using the default idempotency key is a hack, but works for the
+        // tests we currently define.
+        Query::new_leader_selected(LeaderSelectedQueryConfig::test_default())


Half of the uses of this method are not generic, we could replace those TimeInterval::query_for_batch_identifier() instances with Query::new_time_interval() by inlining. That leaves three call sites in generic helpers that populate the database with test fixture rows. We could rephrase this method as either computing or picking a query, given a batch identifier, and make it clear that this should only be called once for each collection job. With that said, I think it should be fine to pick a random idempotency key here.

I'm reluctant to tackle refactoring this test infrastructure in an already large PR. I switched to using a random query.

jcjones · 2026-10-14T18:46:58Z

+                /* task_id */ &task_info.pkey,
+                /* query */ &query.get_encoded()?,
+                /* threshold */
+                &task_info.report_expiry_threshold(self.clock.now())?,


It looks like this COALESCE filter is from get_collection_job. Thinking about it more --

If we add the UNIQUE, a replayed POST whose job is past expiry gets filtered out, and then we fall through to put_collection_job, but that'll fail on the UNIQUE. ON CONFLICT(task_id, collection_job_id) won't catch it since the new ID is random.

If we don't add the UNIQUE, the replay returns the old job's Location, but GET on it would fail because get_collection_job will filter the same way.

I think you're right that we don't want the filter, because both deleted and expired collection jobs are relevant to anti-replay. So I deleted this part of the query. However, I want to defer adding test cases for this kind of behavior to #4911, for the reasons I laid out here.

divergentdave · 2026-10-16T22:55:59Z

+    client_timestamp_interval, helper_aggregate_share, leader_aggregate_share,
+    aggregate_share_id, query
+FROM collection_jobs
+WHERE collection_jobs.task_id = $1


I think we should probably add the same expiry threshold clause as used in other methods here. If some of the batch aggregation rows for this collection job are old enough to be garbage collected, we shouldn't allow for any other further operations on the collection job. My concern is that deleting some batch aggregation rows could allow for breaking important invariants, and sticking to our discipline of treating GC-eligible data as if it doesn't exist is a straightforward way of addressing this.

I think I agree with you, but your observation here conflicts with J.C.'s point here.

The issue is that adding test cases for replay of expired or deleted collection jobs is itself a lot of code. I know because I've already written a lot of it in #4911. This change is big as it is, so in the interest of forward momentum, I'd like to merge it even if it likely has bugs around these edge cases.

So what I'd like to do is take this change with the expiry threshold clause in the query. Then, in #4911 (or perhaps in a narrower PR I'll carve out of #4911), we can come to an agreement on how we want these cases to work and then we'll have tests that assure Janus' correct behavior.

@jcjones does that sound good to you?

The collection job tests are now in #4915.

The simulation test framework could prove useful for building confidence in how we handle these edge cases as well, though those get fixed up later in the PR stack.

Transfers responsibility for choosing collection job IDs from the collector to the Leader, per incoming DAP changes in [1]. In order to make this work for the Leader-Selected batch mode, we also add an idempotency key to the Leader-Selected batch mode query config (also detailed in [1]). The route/handler for collection job creation: - is now POST instead of PUT - no longer includes a collection job ID - provides colleciton job ID in a Location header - sets HTTP 200 OK status and a retry-after header - checks for replayed creation requests by comparing the query instead of the collection job ID And we add an index on `(task_id, query)` on the collection jobs table to make replayed collection job lookups faster. [1]: ietf-wg-ppm/draft-ietf-ppm-dap#797

These tests will be re-enabled in #4899.

jcjones

OK with nit, and some last commentary but I don't think I need to block landing.

jcjones · 2026-10-19T20:52:51Z

                                "collection job already exists"
                            );
-                            return Ok(());
+                            debug_assert!(jobs.next().is_none());


I think we can still hit this. (Obviously it'll be turned off in prod). That said, (task_id, query) really is unique in both batch modes now, so perhaps instead of papering over we should just make that tuple unique in the DB?

Or we could change this from doing jobs.next to doing jobs.find(|job| *job.id() == derived_id)?

Either way we don't have to worry about order anymore, or this assertion.

jcjones · 2026-10-19T20:56:15Z

+          0) >= $3",
+            )
+            .await?;
+        self.query_opt(


Looks good... but my gut says that this is still only safe for LeaderSelected modes, unless we have the DB impose uniqueness.

jcjones · 2026-10-19T20:57:02Z

                        TimeInterval,
                        VdafType,
-                    >(datastore, task, Arc::clone(vdaf), collection_job_id, collection_req_bytes)
+                    >(datastore, task, Arc::clone(vdaf),  collection_req_bytes)


Suggested change

>(datastore, task, Arc::clone(vdaf), collection_req_bytes)

>(datastore, task, Arc::clone(vdaf), collection_req_bytes)

jcjones · 2026-10-19T20:58:26Z

+        }

        // Make sure that there is only ever a single collection job, and that it uses the same
        // batch ID after each PUT


nit: Sweet summer test, it's not PUTs you have to worry about now. You've been hit by
you've been struck by
a smooth POST

tgeoghegan requested a review from a team as a code owner October 13, 2026 14:43

tgeoghegan added the allow-changed-migrations Override the ci-migrations check to allow migrations that have changed. label Oct 13, 2026

tgeoghegan force-pushed the timg/leader-selected-collection-job-idempotency-key-server branch 2 times, most recently from bad032f to b684e8f Compare October 13, 2026 17:31

jcjones reviewed Oct 13, 2026

View reviewed changes

divergentdave reviewed Oct 13, 2026

View reviewed changes

jcjones reviewed Oct 14, 2026

View reviewed changes

tgeoghegan force-pushed the timg/leader-selected-collection-job-idempotency-key-server branch 2 times, most recently from eff7ad0 to 21805ff Compare October 16, 2026 19:51

tgeoghegan requested review from divergentdave and jcjones October 16, 2026 20:06

tgeoghegan force-pushed the timg/leader-selected-collection-job-idempotency-key-server branch from 21805ff to 31358c5 Compare October 16, 2026 20:10

tgeoghegan added this pull request to stack #4914 October 17, 2026 21:40

divergentdave reviewed Oct 19, 2026

View reviewed changes

tgeoghegan force-pushed the timg/leader-selected-collection-job-idempotency-key-server branch 2 times, most recently from 187d6ab to 12f0978 Compare October 19, 2026 17:25

tgeoghegan requested a review from divergentdave October 19, 2026 17:32

tgeoghegan force-pushed the timg/leader-selected-collection-job-idempotency-key-server branch from 12f0978 to b35ecf1 Compare October 19, 2026 17:32

tgeoghegan added 2 commits October 19, 2026 13:33

neuter integration tests temporarily

d0b80eb

These tests will be re-enabled in #4899.

tgeoghegan force-pushed the timg/leader-selected-collection-job-idempotency-key-server branch from b35ecf1 to d0b80eb Compare October 19, 2026 17:36

divergentdave approved these changes Oct 19, 2026

View reviewed changes

jcjones approved these changes Oct 19, 2026

View reviewed changes

	>(datastore, task, Arc::clone(vdaf), collection_req_bytes)
	>(datastore, task, Arc::clone(vdaf), collection_req_bytes)

Conversation

tgeoghegan commented Oct 13, 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!

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!

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!

Uh oh!

jcjones left a comment

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!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants