diff --git a/mod_api/routes/samples.py b/mod_api/routes/samples.py index 9a6f80b6..29169c3a 100644 --- a/mod_api/routes/samples.py +++ b/mod_api/routes/samples.py @@ -44,6 +44,15 @@ 'pass', 'fail', 'missing_output', 'not_started', }) +# How many result rows /samples/{id}/history will scan when ?status is set. +# Status comes from derive_sample_status, which needs the result files and +# expected outputs, so it can't be pushed into SQL the way branch/platform +# can. Without a bound the endpoint has to load a sample's entire history to +# filter it, which is what made it time out on production. Bounded scans are +# reported with pagination.truncated so a caller can tell a capped page from +# a complete one. +_HISTORY_STATUS_SCAN_LIMIT = 1000 + def _preload_expected_outputs(results): """Map regression_test_id -> [RegressionTestOutput] for the given results. @@ -490,6 +499,86 @@ def _process_history_entries( return entries +def _build_history_entries(results, status_filter): + """Build history entries for exactly ``results``, batching the lookups. + + Every follow-up query here is keyed off the results handed in, so the + work scales with that list. Callers must therefore pass the page they + intend to return, not the sample's whole history. + """ + if not results: + return [] + + test_ids = list({r.test_id for r in results}) + + all_files = TestResultFile.query.options( + joinedload(TestResultFile.regression_test_output) + .joinedload(RegressionTestOutput.multiple_files) + ).filter(TestResultFile.test_id.in_(test_ids)).all() + files_by_result = defaultdict(list) + for f in all_files: + files_by_result[(f.test_id, f.regression_test_id)].append(f) + + # Preload expected outputs so status matches /summary and /samples. + expected_by_rt = _preload_expected_outputs(results) + + # Batch load tests to avoid N+1 in _process_history_entries + unique_tests = Test.query.filter(Test.id.in_(test_ids)).all() + test_map = {t.id: t for t in unique_tests} + + # Batch compute timestamps for all referenced tests + _, timestamps_map = batch_get_run_data(unique_tests) + + return _process_history_entries( + results, + files_by_result, + status_filter, + timestamps_map=timestamps_map, + test_map=test_map, + expected_by_rt=expected_by_rt) + + +def _resolve_history_rt_ids(rt_ids, sample_id): + """Narrow a sample's regression tests to the optional ?regression_test_id. + + Without this, ``limit`` counts rows across every regression test on the + sample, so a caller asking about one test gets roughly + limit / len(rt_ids) runs of it and cannot tell the window was short. + """ + raw = request.args.get('regression_test_id') + if raw is None: + return rt_ids, None + + try: + rt_id = int(raw) + if rt_id < 1 or rt_id > 2147483647: + raise ValueError('Out of bounds') + except (ValueError, TypeError): + return None, make_error_response( + 'validation_error', + 'regression_test_id must be a positive integer ' + 'between 1 and 2147483647.', + details={ + 'fields': { + 'regression_test_id': 'Must be a positive integer ' + 'between 1 and 2147483647.'}}, + http_status=400, + ) + + if rt_id not in rt_ids: + return None, make_error_response( + 'validation_error', + f'Regression test {rt_id} does not belong to sample {sample_id}.', + details={ + 'fields': { + 'regression_test_id': 'Must be a regression test of ' + 'this sample.'}}, + http_status=400, + ) + + return [rt_id], None + + def _apply_history_filters( query, branch, @@ -545,6 +634,15 @@ def get_sample_history( Show how a sample performed across different runs. Use failure_signature to tell apart genuine regressions from infra flakes. + + Pass ?regression_test_id to follow a single regression test, so that + ``limit`` means that many runs of it rather than that many rows spread + across every regression test on the sample. + + ?status is applied after status derivation, which needs result files, so + it can't be pushed into SQL. That path scans the most recent + _HISTORY_STATUS_SCAN_LIMIT results and sets pagination.truncated when a + sample has more history than that. """ sample = Sample.query.options(joinedload(Sample.tags)).filter( Sample.id == sample_id).first() @@ -561,6 +659,10 @@ def get_sample_history( if not rt_ids: return paginated_response([], 0, limit, offset) + rt_ids, err = _resolve_history_rt_ids(rt_ids, sample_id) + if err: + return err + # Validate the status filter up front, before any heavy query. status_filter = request.args.get('status') if status_filter and status_filter not in _VALID_SAMPLE_STATUSES: @@ -582,43 +684,35 @@ def get_sample_history( if err: return err - results = query.order_by(Test.id.desc()).all() - - # Preload TestResultFiles - test_ids = list({r.test_id for r in results}) - all_files = TestResultFile.query.options( - joinedload(TestResultFile.regression_test_output) - .joinedload(RegressionTestOutput.multiple_files) - ).filter( - TestResultFile.test_id.in_(test_ids)).all() if test_ids else [] - files_by_result = defaultdict(list) - for f in all_files: - files_by_result[(f.test_id, f.regression_test_id)].append(f) + # regression_test_id breaks ties so a row can't shift between pages when + # several regression tests share a run. + query = query.order_by(Test.id.desc(), TestResult.regression_test_id.asc()) - # Preload expected outputs so status matches /summary and /samples. - expected_by_rt = _preload_expected_outputs(results) - - # Batch load tests to avoid N+1 in _process_history_entries - unique_tests = Test.query.filter( - Test.id.in_(test_ids)).all() if test_ids else [] - test_map = {t.id: t for t in unique_tests} - - # Batch compute timestamps for all referenced tests - _, timestamps_map = batch_get_run_data(unique_tests) - - entries = _process_history_entries( - results, - files_by_result, - status_filter, - timestamps_map=timestamps_map, - test_map=test_map, - expected_by_rt=expected_by_rt) + if status_filter: + # One row past the cap tells us whether the scan was complete. + scanned = query.limit(_HISTORY_STATUS_SCAN_LIMIT + 1).all() + truncated = len(scanned) > _HISTORY_STATUS_SCAN_LIMIT + entries = _build_history_entries( + scanned[:_HISTORY_STATUS_SCAN_LIMIT], status_filter) + return paginated_response( + entries[offset:offset + limit], + len(entries), + limit, + offset, + schema=SampleHistoryEntrySchema(), + truncated=truncated, + extra_meta={'scan_limit': _HISTORY_STATUS_SCAN_LIMIT} + if truncated else None, + ) - total = len(entries) - paged = entries[offset:offset + limit] + # No status filter: the page can be cut in SQL, so everything below it + # loads one page worth of rows instead of the sample's whole history. + total = query.count() + results = query.offset(offset).limit(limit).all() + entries = _build_history_entries(results, None) return paginated_response( - paged, total, limit, offset, schema=SampleHistoryEntrySchema() + entries, total, limit, offset, schema=SampleHistoryEntrySchema() ) diff --git a/openapi-ci-api.yaml b/openapi-ci-api.yaml index 3e248652..c3c95c92 100644 --- a/openapi-ci-api.yaml +++ b/openapi-ci-api.yaml @@ -862,6 +862,16 @@ paths: Use failure_signature for flake detection: a stable signature across multiple runs on different commits indicates a genuine regression, not infrastructure noise. + + + Entries cover every regression test on the sample, newest run first, + so limit counts rows rather than runs. Pass regression_test_id to + follow one test, where limit then means that many runs of it. + + + With status set, the filter runs over a bounded scan of recent + results; pagination.truncated is true when the sample has more + history than the scan covered, and total then undercounts. security: - bearerAuth: [] x-required-scope: runs:read @@ -869,7 +879,8 @@ paths: - $ref: "#/components/parameters/SampleId" - $ref: "#/components/parameters/Limit" - $ref: "#/components/parameters/Offset" - - $ref: "#/components/parameters/RunStatus" + - $ref: "#/components/parameters/RegressionTestIdFilter" + - $ref: "#/components/parameters/SampleHistoryStatus" - $ref: "#/components/parameters/Branch" - $ref: "#/components/parameters/Platform" - $ref: "#/components/parameters/CreatedAfter" @@ -2366,6 +2377,32 @@ components: type: integer minimum: 1 + RegressionTestIdFilter: + name: regression_test_id + in: query + description: > + Restrict the history to a single regression test of this sample, so + that limit counts runs of that test rather than rows across every + regression test on the sample. Must belong to the sample in the path; + 400 otherwise. + schema: + type: integer + minimum: 1 + maximum: 2147483647 + + SampleHistoryStatus: + name: status + in: query + description: > + Normalized per-sample status, derived from the result files and the + expected outputs of each result. Because it is derived rather than + stored, this filter is applied over a bounded scan of the most recent + results; see pagination.truncated on the response. + schema: + type: string + enum: [pass, fail, missing_output, not_started] + example: fail + UserId: name: user_id in: path diff --git a/tests/api/test_routes_samples.py b/tests/api/test_routes_samples.py index 35b0c50a..0458c697 100644 --- a/tests/api/test_routes_samples.py +++ b/tests/api/test_routes_samples.py @@ -1,3 +1,5 @@ +from unittest.mock import patch + from flask import g from sqlalchemy import event @@ -5,7 +7,8 @@ from mod_regression.models import (Category, InputType, OutputType, RegressionTest, RegressionTestOutput) from mod_sample.models import Sample -from mod_test.models import TestResult, TestResultFile +from mod_test.models import (Test, TestPlatform, TestResult, TestResultFile, + TestType) from tests.api.base import ApiTestCase @@ -39,6 +42,10 @@ def setUp(self): g.db.commit() self.reg_out_id = self.reg_out.id + # Requests below detach ORM objects held by this session, so keep a + # plain id for the tests that add runs after a request. + self.fork_id = self.fork.id + self.test_result = TestResult(self.test_id, self.reg_test_id, 0, 0, 0) g.db.add(self.test_result) g.db.commit() @@ -93,13 +100,13 @@ def test_list_run_samples_missing_output_consistent(self): if s['regression_test_id'] == reg_test2_id) self.assertEqual(entry['status'], 'missing_output') - def _count_queries(self, url, token): - """Return the number of SQL statements one GET request executes.""" + def _capture_statements(self, url, token): + """Return (statement, parameters) for every SQL one GET executes.""" statements = [] def counter(conn, cursor, statement, parameters, context, executemany): - statements.append(statement) + statements.append((statement, parameters)) engine = g.db.get_bind() event.listen(engine, 'before_cursor_execute', counter) @@ -109,7 +116,11 @@ def counter(conn, cursor, statement, parameters, context, finally: event.remove(engine, 'before_cursor_execute', counter) self.assertEqual(res.status_code, 200) - return len(statements) + return statements + + def _count_queries(self, url, token): + """Return the number of SQL statements one GET request executes.""" + return len(self._capture_statements(url, token)) def test_list_run_samples_query_count_is_flat(self): # Guards against reintroducing per-regression-test lazy loads: @@ -181,6 +192,184 @@ def test_get_sample_history(self): self.assertTrue( any(h['run_id'] == self.test_id for h in res.json['data'])) + def _add_history_run(self, commit, results): + """Add a run holding one result per (rt_id, output_id, exit_code).""" + run = Test(TestPlatform.linux, TestType.commit, self.fork_id, + 'master', commit) + g.db.add(run) + g.db.commit() + run_id = run.id + for rt_id, output_id, exit_code in results: + g.db.add(TestResult(run_id, rt_id, 0, exit_code, 0)) + g.db.add(TestResultFile(run_id, rt_id, output_id, + 'expected_hash', None)) + g.db.commit() + return run_id + + def _add_second_regression_test(self): + """Add a second regression test on the same sample, with an output.""" + rt = RegressionTest(self.sample_id, 'command_hist2', InputType.file, + OutputType.file, self.category.id, 0) + g.db.add(rt) + g.db.commit() + rt_id = rt.id + rto = RegressionTestOutput(rt_id, 'expected_hash', '.txt', 'exp2') + g.db.add(rto) + g.db.commit() + return rt_id, rto.id + + def test_get_sample_history_paginates_in_sql(self): + # limit has to bound the work, not just the response. The endpoint + # used to build an entry for every result in the sample's history + # and slice the page out of that list afterwards, which is what made + # it time out on production data. + for i in range(4): + self._add_history_run(f'hist_commit_{i}', + [(self.reg_test_id, self.reg_out_id, 0)]) + + token = self.get_token('samp_user@local.com', 'userpass123', + 'thp', scopes=['runs:read']) + base = f'/api/v1/samples/{self.sample_id}/history?limit=2' + first = self.client.get( + base, headers={'Authorization': f'Bearer {token}'}) + second = self.client.get( + f'{base}&offset=2', headers={'Authorization': f'Bearer {token}'}) + + self.assertEqual(first.status_code, 200) + self.assertEqual(second.status_code, 200) + # 4 added runs plus the one from setUp; total counts the whole + # history even though only a page was loaded. + self.assertEqual(first.json['pagination']['total'], 5) + self.assertEqual(first.json['pagination']['next_offset'], 2) + self.assertEqual(len(first.json['data']), 2) + self.assertEqual(len(second.json['data']), 2) + + first_ids = [e['run_id'] for e in first.json['data']] + second_ids = [e['run_id'] for e in second.json['data']] + # Newest first, and the pages must not overlap. + self.assertEqual(first_ids, sorted(first_ids, reverse=True)) + self.assertEqual(set(first_ids) & set(second_ids), set()) + + def test_get_sample_history_loads_only_the_page(self): + # The follow-up queries are keyed off the results of the page, so + # they must not widen as the sample accumulates history. + for i in range(8): + self._add_history_run(f'bounded_commit_{i}', + [(self.reg_test_id, self.reg_out_id, 0)]) + + token = self.get_token('samp_user@local.com', 'userpass123', + 'tbp', scopes=['runs:read']) + statements = self._capture_statements( + f'/api/v1/samples/{self.sample_id}/history?limit=2', token) + + # The file lookup is the one that fans out through two nested + # joinedloads, so it is the one worth pinning. + file_selects = [params for stmt, params in statements + if 'FROM test_result_file' in stmt] + self.assertTrue(file_selects) + for params in file_selects: + self.assertLessEqual(len(params), 2) + + def test_get_sample_history_regression_test_filter(self): + rt2_id, rto2_id = self._add_second_regression_test() + for i in range(3): + self._add_history_run( + f'multi_rt_commit_{i}', + [(self.reg_test_id, self.reg_out_id, 0), + (rt2_id, rto2_id, 0)]) + + token = self.get_token('samp_user@local.com', 'userpass123', + 'trt', scopes=['runs:read']) + url = (f'/api/v1/samples/{self.sample_id}/history' + f'?limit=3®ression_test_id={self.reg_test_id}') + res = self.client.get(url, headers={'Authorization': f'Bearer {token}'}) + + self.assertEqual(res.status_code, 200) + data = res.json['data'] + self.assertEqual({e['regression_test_id'] for e in data}, + {self.reg_test_id}) + # limit means runs of the requested test: 3 asked for, 3 distinct + # runs back. + self.assertEqual(len({e['run_id'] for e in data}), 3) + # 3 added runs plus the one from setUp, counting only this test. + self.assertEqual(res.json['pagination']['total'], 4) + + # Without the filter the same limit is spread over both regression + # tests, so it covers fewer runs — the reason the filter exists. + unfiltered = self.client.get( + f'/api/v1/samples/{self.sample_id}/history?limit=3', + headers={'Authorization': f'Bearer {token}'}) + self.assertEqual(unfiltered.status_code, 200) + self.assertLess( + len({e['run_id'] for e in unfiltered.json['data']}), 3) + + def test_get_sample_history_regression_test_filter_foreign_id(self): + other_sample = Sample('other_sha', 'txt', 'other_sample') + g.db.add(other_sample) + g.db.commit() + other_rt = RegressionTest(other_sample.id, 'other_command', + InputType.file, OutputType.file, + self.category.id, 0) + g.db.add(other_rt) + g.db.commit() + other_rt_id = other_rt.id + + token = self.get_token('samp_user@local.com', 'userpass123', + 'trtf', scopes=['runs:read']) + # A regression test of another sample would otherwise silently + # return an empty page. + res = self.client.get( + f'/api/v1/samples/{self.sample_id}/history' + f'?regression_test_id={other_rt_id}', + headers={'Authorization': f'Bearer {token}'}) + self.assertEqual(res.status_code, 400) + + def test_get_sample_history_regression_test_filter_invalid(self): + token = self.get_token('samp_user@local.com', 'userpass123', + 'trti', scopes=['runs:read']) + for value in ('abc', '0', '-1'): + res = self.client.get( + f'/api/v1/samples/{self.sample_id}/history' + f'?regression_test_id={value}', + headers={'Authorization': f'Bearer {token}'}) + self.assertEqual(res.status_code, 400, f'value={value}') + + def test_get_sample_history_status_filter(self): + failing_run_id = self._add_history_run( + 'failing_commit', [(self.reg_test_id, self.reg_out_id, 1)]) + + token = self.get_token('samp_user@local.com', 'userpass123', + 'tsf', scopes=['runs:read']) + res = self.client.get( + f'/api/v1/samples/{self.sample_id}/history?status=fail', + headers={'Authorization': f'Bearer {token}'}) + + self.assertEqual(res.status_code, 200) + self.assertEqual([e['run_id'] for e in res.json['data']], + [failing_run_id]) + # The whole history fit inside the scan, so the page is complete. + self.assertNotIn('truncated', res.json['pagination']) + + @patch('mod_api.routes.samples._HISTORY_STATUS_SCAN_LIMIT', 2) + def test_get_sample_history_status_filter_scan_is_bounded(self): + # status is derived in Python, so it can't be pushed into SQL. The + # scan is capped instead, and a capped page says so rather than + # passing itself off as the sample's whole history. + for i in range(3): + self._add_history_run(f'scan_commit_{i}', + [(self.reg_test_id, self.reg_out_id, 0)]) + + token = self.get_token('samp_user@local.com', 'userpass123', + 'tsb', scopes=['runs:read']) + res = self.client.get( + f'/api/v1/samples/{self.sample_id}/history?status=pass', + headers={'Authorization': f'Bearer {token}'}) + + self.assertEqual(res.status_code, 200) + self.assertTrue(res.json['pagination']['truncated']) + self.assertEqual(res.json['meta']['scan_limit'], 2) + self.assertEqual(len(res.json['data']), 2) + def test_list_regression_tests(self): token = self.get_token('samp_user@local.com', 'userpass123', 't6', scopes=['runs:read'])