1387147 - job_id__in='' on the JobDetail table will create a query that takes forever

Status: RESOLVED FIXED

(Tree Management :: Treeherder: API, defect, P1)

Description

[Cameron Dawson [:camd]]

Need to prevent that API call causing an unbounded query
Also not allow a query to last for so long(10 minutes)  Perhaps limit to 1 or 2 minutes?

Comment 1

[Cameron Dawson [:camd]]

Need to Fix the retrigger UI to stop trying to make this query

Comment 2

[Ed Morley [:emorley]]

The SQL being run is:
https://emorley.pastebin.mozilla.org/9028800

The EXPLAIN of that query:
https://emorley.pastebin.mozilla.org/9028802

Notably:
"USING TEMPORARY; USING filesort"

We need to:
* Fix the UI to not trigger these broken requests (looks like its possible for retriggers to occur when zero jobs selected: https://github.com/mozilla/treeherder/blob/2e341fa06f1b912dbdfb214e5f0ca86e272dcebd/ui/plugins/controller.js#L353-L356)
* Make the API reject an empty `job_id__in` parameter (https://github.com/mozilla/treeherder/blob/2e341fa06f1b912dbdfb214e5f0ca86e272dcebd/treeherder/webapp/api/jobs.py#L605-L648)

Cameron I don't suppose you could take this part?

There's some additional cleanup of this API endpoint that can take place in bug 1343239, plus additional action items for this production issue in bug 1386331 comment 8 - however they are out of scope for this bug.

Comment 3

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:retrigger-job-details-query-fix > mozilla:master

Comment 4

[Cameron Dawson [:camd]]

Comment on attachment 8894000 [details] [review]
[treeherder] mozilla:retrigger-job-details-query-fix > mozilla:master

This will prevent that unbounded query on either push or job_id.  I actually couldn't figure out a way to get the UI to CREATE this query.  All my attempts bailed out before it even hit the job_detail table.  So perhaps this is someone's script doing this and not our UI at all?  Anyway, I added a UI safeguard anyway, just in case, to give a good error message.  

But the API change should make it safe now.

Comment 5

[Ed Morley [:emorley]]

Comment on attachment 8894000 [details] [review]
[treeherder] mozilla:retrigger-job-details-query-fix > mozilla:master

Awesome, thank you!

Comment 6

[Ed Morley [:emorley]]

Ah ha! Using GitHub code search I've found the cause of the requests to the API - try (with space removed):
https://screenshots.mattn.ca /compare/?oldProject=mozilla-central&oldRev=bc829569880635c52d6e3d54f51cd7d3df180186&newProject=mozilla-central&newRev=bc829569880635c52d6e3d54f51cd7d3df180186

It makes a request of form:
https://treeherder.mozilla.org /api/jobdetail/?repository=mozilla-central&job_id__in=

Via:
https://github.com/mnoorenberghe/mozscreenshots/blob/5fee890dfefba998c9d2adcdb1e131a117d6634d/web/compare.js#L162

So perhaps the Treeherder UI part of the PR is unneeded and can be removed? I'll leave it up to you.

Comment 7

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/ae033a0ebfab7eebee93eecb33e45ab4c82339c6
Bug 1387147 - Prevent unbounded /jobdetail query (#2683)

This is a huge table and the unbounded query (no job ids) takes too long
and is useless anyway.

Comment 8

[Cameron Dawson [:camd]]

Awesome sleuthing.  OK, yeah.  I removed the UI code.  This API change should protect this endpoint just fine.

Comment 9

[Ed Morley [:emorley]]

This is now deployed on production and working well :-)

eg:
https://rpm.newrelic.com/accounts/677903/applications/14179757/filterable_errors#/show/a867a2eb-79da-11e7-931c-0242ac110012_4243_9509/stack_trace?top_facet=transactionUiName&primary_facet=error.class&barchart=barchart&_k=9p47mn

(Note also the referrer there matches the conclusion in comment 6)

However I'm now thinking that we should make this an HTTP400 like other API validation does? Sorry for not thinking of this during review. Would you mind switching it over?

I've also filed an issue against the screenshots tool to get that fixed too:
https://github.com/mnoorenberghe/mozscreenshots/issues/36

Comment 10

[Ed Morley [:emorley]]

(In reply to Ed Morley [:emorley] from comment #9)
> However I'm now thinking that we should make this an HTTP400 like other API
> validation does? Sorry for not thinking of this during review. Would you
> mind switching it over?

Comment 11

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:in-filter-fixes > mozilla:master

Comment 12

[Cameron Dawson [:camd]]

Good call.  Here's the fix for that.

Comment 13

[Ed Morley [:emorley]]

Many thanks!

Comment 14

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/045a895d9a12969ba791c2aa7a3cc9f8e1c67e83
Bug 1387147 - Return a 400 for JobDetail bad params instead of 500

If an invalid param value, such as ``job_id__id=‘’`` is passed in, we
will return a 400 response instead of a 500.