From 3bc540682833d12a39d84bd59019e55553ac30a3 Mon Sep 17 00:00:00 2001 From: Carlos Fernandez Date: Sun, 9 Aug 2026 11:41:49 -0700 Subject: [PATCH] fix(ci): give each upload its own temp file so concurrent tests stop clobbering results upload_type_request() saved every upload to TempFiles/. That name comes from the regression test output, so it is identical for every test running the same regression test. Two tests running at once therefore share one path: A saves TempFiles/X.srt B overwrites TempFiles/X.srt A hashes and renames it to TestResults/.srt B renames -> FileNotFoundError -> HTTP 500 The 500 means no TestResultFile row is ever written, and the platform then shows that regression test as "No output generated but there should be" (got == 'error' in mod_test/controllers.py when a test has no result files but expects output). Contributors see it as their PR breaking tests that it never touched. There is a quieter variant: if B's save lands between A's save and A's hashing, A records the hash of B's output and its verdict silently flips. Fix: allocate a unique temp file per upload with tempfile.mkstemp() and finish with os.replace(), which is atomic. A finally block removes the temp file if anything fails, so failures no longer leak into TempFiles. Observed on ccextractor PR #2309. Tests 9402 (master) and 9410 (the PR) ran concurrently on Linux; their VM logs contain 37 and 21 HTTP 500s, matching the 37 and 21 spurious "No output generated" results almost exactly, against a shared baseline of 24 genuine failures. The Windows runs of the same two commits were staggered, hit zero 500s, and produced identical verdicts. error.log holds 2184 of these failed renames, so this has been corrupting results for a long time; it only became obvious once a backlog started launching two tests at the same moment. --- mod_ci/controllers.py | 43 ++++++++++++------ tests/test_ci/test_controllers.py | 75 +++++++++++++++++++++++++++++-- 2 files changed, 101 insertions(+), 17 deletions(-) diff --git a/mod_ci/controllers.py b/mod_ci/controllers.py index d66c1e62..0ad97ac3 100755 --- a/mod_ci/controllers.py +++ b/mod_ci/controllers.py @@ -8,6 +8,7 @@ import os import re import shutil +import tempfile import time import zipfile from collections import defaultdict @@ -2664,20 +2665,36 @@ def upload_type_request(log, test_id, repo_folder, test, request) -> bool: return False temp_dir = os.path.join(repo_folder, 'TempFiles') os.makedirs(temp_dir, exist_ok=True) - temp_path = os.path.join(temp_dir, filename) - # Save to temporary location - uploaded_file.save(temp_path) - # Get hash and check if it's already been submitted - hash_sha256 = hashlib.sha256() - with open(temp_path, "rb") as f: - for chunk in iter(lambda: f.read(4096), b""): - hash_sha256.update(chunk) - file_hash = hash_sha256.hexdigest() filename, file_extension = os.path.splitext(filename) - results_dir = os.path.join(repo_folder, 'TestResults') - os.makedirs(results_dir, exist_ok=True) - final_path = os.path.join(results_dir, f'{file_hash}{file_extension}') - os.rename(temp_path, final_path) + # The uploaded name is derived from the regression test output, so it is identical + # for every test running that same regression test. Sharing one path across + # concurrent tests races: the second upload overwrites the first, the first rename + # moves the file away, and the second rename then fails with FileNotFoundError -> + # HTTP 500 -> no TestResultFile row -> the run is displayed as "No output generated + # but there should be". Worse, an upload that lands between another test's save and + # its hashing makes that test record the wrong hash, silently flipping its verdict. + # A unique temp file per upload removes both. + temp_fd, temp_path = tempfile.mkstemp(dir=temp_dir, suffix=file_extension) + os.close(temp_fd) + try: + # Save to temporary location + uploaded_file.save(temp_path) + # Get hash and check if it's already been submitted + hash_sha256 = hashlib.sha256() + with open(temp_path, "rb") as f: + for chunk in iter(lambda: f.read(4096), b""): + hash_sha256.update(chunk) + file_hash = hash_sha256.hexdigest() + results_dir = os.path.join(repo_folder, 'TestResults') + os.makedirs(results_dir, exist_ok=True) + final_path = os.path.join(results_dir, f'{file_hash}{file_extension}') + # os.replace is atomic and, unlike leaving a stale temp file behind, keeps + # TempFiles clean when two uploads hash to the same content. + os.replace(temp_path, final_path) + finally: + # No-op once the replace succeeded; on any failure it stops the temp file leaking. + if os.path.exists(temp_path): + os.remove(temp_path) rto = RegressionTestOutput.query.filter( RegressionTestOutput.id == request.form['test_file_id']).first() result_file = TestResultFile(test.id, request.form['test_id'], rto.id, rto.correct, file_hash) diff --git a/tests/test_ci/test_controllers.py b/tests/test_ci/test_controllers.py index b8472f34..8f892e65 100644 --- a/tests/test_ci/test_controllers.py +++ b/tests/test_ci/test_controllers.py @@ -1,4 +1,5 @@ import json +import os import unittest from importlib import reload from unittest import mock @@ -2027,9 +2028,10 @@ def test_upload_type_request_empty(self, mock_filename): @mock.patch('mod_ci.controllers.g') @mock.patch('mod_ci.controllers.iter') @mock.patch('mod_ci.controllers.open') + @mock.patch('mod_ci.controllers.tempfile') @mock.patch('mod_ci.controllers.os') @mock.patch('mod_ci.controllers.secure_filename') - def test_upload_type_request(self, mock_filename, mock_os, mock_open, mock_iter, + def test_upload_type_request(self, mock_filename, mock_os, mock_tempfile, mock_open, mock_iter, mock_g, mock_rto, mock_result_file, mock_hashlib): """Test function upload_type_request.""" from mod_ci.controllers import upload_type_request @@ -2046,19 +2048,22 @@ def test_upload_type_request(self, mock_filename, mock_os, mock_open, mock_iter, } mock_iter.return_value = ['chunk'] mock_os.path.splitext.return_value = "a", "b" + mock_tempfile.mkstemp.return_value = (5, '/tmp/TempFiles/unique-b') upload_type_request(mock_log, 1, MagicMock(), MagicMock(), mock_request) mock_log.debug.assert_called_once() mock_filename.assert_called_once() - # 4 calls: temp_dir, temp_path, results_dir, final_path - self.assertEqual(4, mock_os.path.join.call_count) + # 3 calls: temp_dir, results_dir, final_path. The temp path now comes from mkstemp. + self.assertEqual(3, mock_os.path.join.call_count) # 2 calls: makedirs for TempFiles and TestResults directories self.assertEqual(2, mock_os.makedirs.call_count) + mock_tempfile.mkstemp.assert_called_once_with(dir=mock.ANY, suffix="b") + mock_os.close.assert_called_once_with(5) mock_upload_file.save.assert_called_once() mock_open.assert_called_once_with(mock.ANY, "rb") mock_os.path.splitext.assert_called_once_with(mock.ANY) - mock_os.rename.assert_called_once_with(mock.ANY, mock.ANY) + mock_os.replace.assert_called_once_with(mock.ANY, mock.ANY) mock_rto.query.filter.assert_called_once_with(mock_rto.id == 1) mock_result_file.assert_called_once_with(mock.ANY, 1, mock.ANY, mock.ANY, mock.ANY) mock_g.db.add.assert_called_once_with(mock.ANY) @@ -2066,6 +2071,68 @@ def test_upload_type_request(self, mock_filename, mock_os, mock_open, mock_iter, mock_hashlib.sha256.assert_called_once_with() mock_iter.assert_called_once_with(mock.ANY, b"") + @mock.patch('mod_ci.controllers.TestResultFile') + @mock.patch('mod_ci.controllers.RegressionTestOutput') + @mock.patch('mod_ci.controllers.g') + def test_upload_type_request_uses_unique_temp_path(self, mock_g, mock_rto, mock_result_file): + """Two tests uploading the same filename must not share a temp path. + + The uploaded name is derived from the regression test output, so it is identical + across concurrent tests. A shared temp path let one upload clobber another's file, + which surfaced as HTTP 500s and missing results. + """ + import tempfile as real_tempfile + + from mod_ci.controllers import upload_type_request + + saved_paths = [] + + def make_request(content): + uploaded = MagicMock() + uploaded.filename = 'shared_name.srt' + uploaded.save = lambda path: (saved_paths.append(path), + open(path, 'wb').write(content)) + request = MagicMock() + request.files = {'file': uploaded} + request.form = {'test_id': 1, 'test_file_id': 1} + return request + + with real_tempfile.TemporaryDirectory() as repo_folder: + self.assertTrue(upload_type_request(MagicMock(), 1, repo_folder, MagicMock(), + make_request(b'output of test one'))) + self.assertTrue(upload_type_request(MagicMock(), 2, repo_folder, MagicMock(), + make_request(b'output of test two'))) + + self.assertEqual(2, len(saved_paths)) + self.assertNotEqual(saved_paths[0], saved_paths[1], + "both uploads reused one temp path; concurrent tests will race") + + results = os.listdir(os.path.join(repo_folder, 'TestResults')) + self.assertEqual(2, len(results), "each upload should land as its own result file") + for name in results: + self.assertTrue(name.endswith('.srt')) + # Nothing may be left behind in TempFiles. + self.assertEqual([], os.listdir(os.path.join(repo_folder, 'TempFiles'))) + + @mock.patch('mod_ci.controllers.g') + def test_upload_type_request_cleans_temp_file_on_failure(self, mock_g): + """A failed upload must not leak its temp file.""" + import tempfile as real_tempfile + + from mod_ci.controllers import upload_type_request + + uploaded = MagicMock() + uploaded.filename = 'broken.srt' + uploaded.save = MagicMock(side_effect=OSError("disk went away")) + request = MagicMock() + request.files = {'file': uploaded} + request.form = {'test_id': 1, 'test_file_id': 1} + + with real_tempfile.TemporaryDirectory() as repo_folder: + with self.assertRaises(OSError): + upload_type_request(MagicMock(), 1, repo_folder, MagicMock(), request) + self.assertEqual([], os.listdir(os.path.join(repo_folder, 'TempFiles'))) + @mock.patch('mod_ci.controllers.RegressionTest') @mock.patch('mod_ci.controllers.TestResult') @mock.patch('mod_ci.controllers.g')