Should reject updating a build request which has an associated build.
authordewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 22 Jan 2018 07:38:16 +0000 (07:38 +0000)
committerdewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 22 Jan 2018 07:38:16 +0000 (07:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181893

Reviewed by Ryosuke Niwa.

Current code does not prevent submitting to same build request multiple times.
This could lead to a build losing its associated build request.
As a result, this build will be visible in charts which is not right.
Added a check when a build request is reported.
Addressed a 'FIXME' for the race condition inside ReportProcessor->resolve_build_id by surrounding
it with a database transaction.

* public/include/report-processor.php:
Wrap adding platform and resolve_build_id with a database transaction.
Add a check to make sure only a build request has no associated build can be updated.
* server-tests/api-report-tests.js: Added unit tests accordingly.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@227283 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/include/report-processor.php
Websites/perf.webkit.org/server-tests/api-report-tests.js

index affdd4e..5b887ea 100644 (file)
@@ -1,3 +1,22 @@
+2018-01-19  Dewei Zhu  <dewei_zhu@apple.com>
+
+        Should reject updating a build request which has an associated build.
+        https://bugs.webkit.org/show_bug.cgi?id=181893
+
+        Reviewed by Ryosuke Niwa.
+
+        Current code does not prevent submitting to same build request multiple times.
+        This could lead to a build losing its associated build request.
+        As a result, this build will be visible in charts which is not right.
+        Added a check when a build request is reported.
+        Addressed a 'FIXME' for the race condition inside ReportProcessor->resolve_build_id by surrounding
+        it with a database transaction.
+
+        * public/include/report-processor.php:
+        Wrap adding platform and resolve_build_id with a database transaction.
+        Add a check to make sure only a build request has no associated build can be updated.
+        * server-tests/api-report-tests.js: Added unit tests accordingly.
+
 2018-01-18  Dewei Zhu  <dewei_zhu@apple.com>
 
         Should allow updating a build-request to 'canceled'.
index ca1a031..f36d94a 100644 (file)
@@ -44,14 +44,18 @@ class ReportProcessor {
         $this->runs->aggregate();
         $this->runs->compute_caches();
 
+        $this->db->begin_transaction() or $this->exit_with_error('FailedToBeginTransaction');
+
         $platform_id = $this->db->select_or_insert_row('platforms', 'platform', array('name' => $report['platform']));
         if (!$platform_id)
-            $this->exit_with_error('FailedToInsertPlatform', array('name' => $report['platform']));
+            $this->rollback_with_error('FailedToInsertPlatform', array('name' => $report['platform']));
 
         // FIXME: Deprecate and unsupport "jobId".
         $build_id = $this->resolve_build_id($build_data, array_get($report, 'revisions', array()),
             array_get($report, 'jobId', array_get($report, 'buildRequest')));
 
+        $this->db->commit_transaction() or $this->exit_with_error('FailedToCommitTransaction');
+
         $this->runs->commit($platform_id, $build_id);
     }
 
@@ -128,31 +132,31 @@ class ReportProcessor {
     }
 
     private function resolve_build_id(&$build_data, $revisions, $build_request_id) {
-        // FIXME: This code has a race condition. See <rdar://problem/15876303>.
         $results = $this->db->query_and_fetch_all("SELECT build_id, build_slave FROM builds
             WHERE build_builder = $1 AND build_number = $2 AND build_time <= $3 AND build_time + interval '1 day' > $3 LIMIT 2",
             array($build_data['builder'], $build_data['number'], $build_data['time']));
         if ($results) {
             $first_result = $results[0];
             if ($first_result['build_slave'] != $build_data['slave'])
-                $this->exit_with_error('MismatchingBuildSlave', array('storedBuild' => $results, 'reportedBuildData' => $build_data));
+                $this->rollback_with_error('MismatchingBuildSlave', array('storedBuild' => $results, 'reportedBuildData' => $build_data));
             $build_id = $first_result['build_id'];
         } else
             $build_id = $this->db->insert_row('builds', 'build', $build_data);
+
         if (!$build_id)
-            $this->exit_with_error('FailedToInsertBuild', $build_data);
+            $this->rollback_with_error('FailedToInsertBuild', $build_data);
 
         if ($build_request_id) {
-            if ($this->db->update_row('build_requests', 'request', array('id' => $build_request_id), array('status' => 'completed', 'build' => $build_id))
+            if ($this->db->update_row('build_requests', 'request', array('id' => $build_request_id, 'build' => null), array('status' => 'completed', 'build' => $build_id))
                 != $build_request_id)
-                $this->exit_with_error('FailedToUpdateBuildRequest', array('buildRequest' => $build_request_id, 'build' => $build_id));
+                $this->rollback_with_error('FailedToUpdateBuildRequest', array('buildRequest' => $build_request_id, 'build' => $build_id));
         }
 
 
         foreach ($revisions as $repository_name => $revision_data) {
             $repository_id = $this->db->select_or_insert_row('repositories', 'repository', array('name' => $repository_name, 'owner' => NULL));
             if (!$repository_id)
-                $this->exit_with_error('FailedToInsertRepository', array('name' => $repository_name));
+                $this->rollback_with_error('FailedToInsertRepository', array('name' => $repository_name));
 
             $commit_data = array('repository' => $repository_id, 'revision' => $revision_data['revision'], 'time' => array_get($revision_data, 'timestamp'));
 
@@ -160,23 +164,28 @@ class ReportProcessor {
                 WHERE build_commit = commit_id AND commit_build = $1 AND commit_repository = $2 AND commit_revision != $3 LIMIT 1',
                 array($build_id, $repository_id, $revision_data['revision']));
             if ($mismatching_commit)
-                $this->exit_with_error('MismatchingCommitRevision', array('build' => $build_id, 'existing' => $mismatching_commit, 'new' => $commit_data));
+                $this->rollback_with_error('MismatchingCommitRevision', array('build' => $build_id, 'existing' => $mismatching_commit, 'new' => $commit_data));
 
             $commit_row = $this->db->select_or_insert_row('commits', 'commit',
                 array('repository' => $repository_id, 'revision' => $revision_data['revision']), $commit_data, '*');
             if (!$commit_row)
-                $this->exit_with_error('FailedToRecordCommit', $commit_data);
+                $this->rollback_with_error('FailedToRecordCommit', $commit_data);
             if ($commit_data['time'] && abs(floatval($commit_row['commit_time']) - floatval($commit_data['time'])) > 1.0)
-                $this->exit_with_error('MismatchingCommitTime', array('existing' => $commit_row, 'new' => $commit_data));
+                $this->rollback_with_error('MismatchingCommitTime', array('existing' => $commit_row, 'new' => $commit_data));
 
             if (!$this->db->select_or_insert_row('build_commits', null,
                 array('commit_build' => $build_id, 'build_commit' => $commit_row['commit_id']), null, '*'))
-                $this->exit_with_error('FailedToRelateCommitToBuild', array('commit' => $commit_row, 'build' => $build_id));
+                $this->rollback_with_error('FailedToRelateCommitToBuild', array('commit' => $commit_row, 'build' => $build_id));
         }
 
         return $build_id;
     }
 
+    private function rollback_with_error($message, $details) {
+        $this->db->rollback_transaction();
+        $this->exit_with_error($message, $details);
+    }
+
     private function recursively_ensure_tests(&$tests, $parent_id = NULL, $level = 0) {
         foreach ($tests as $test_name => $test) {
             $test_id = $this->db->select_or_insert_row('tests', 'test', $parent_id ? array('name' => $test_name, 'parent' => $parent_id) : array('name' => $test_name),
index 1a2eef3..3b40d45 100644 (file)
@@ -6,6 +6,7 @@ const TestServer = require('./resources/test-server.js');
 const addBuilderForReport = require('./resources/common-operations.js').addBuilderForReport;
 const addSlaveForReport = require('./resources/common-operations.js').addSlaveForReport;
 const prepareServerTest = require('./resources/common-operations.js').prepareServerTest;
+const MockData = require('./resources/mock-data.js');
 
 describe("/api/report", function () {
     prepareServerTest(this);
@@ -377,7 +378,7 @@ describe("/api/report", function () {
             }
         }};
 
-    function reportAfterAddingBuilderAndAggregators(report)
+    function reportAfterAddingBuilderAndAggregatorsWithResponse(report)
     {
         return addBuilderForReport(report).then(() => {
             const db = TestServer.database();
@@ -385,9 +386,12 @@ describe("/api/report", function () {
                 db.insert('aggregators', {name: 'Arithmetic'}),
                 db.insert('aggregators', {name: 'Geometric'}),
             ]);
-        }).then(() => {
-            return TestServer.remoteAPI().postJSON('/api/report/', [report]);
-        }).then((response) => {
+        }).then(() => TestServer.remoteAPI().postJSON('/api/report/', [report]));
+    }
+
+    function reportAfterAddingBuilderAndAggregators(report)
+    {
+        return reportAfterAddingBuilderAndAggregatorsWithResponse(report).then((response) => {
             assert.equal(response['status'], 'OK');
             assert.equal(response['failureStored'], false);
             return response;
@@ -735,4 +739,64 @@ describe("/api/report", function () {
             });
         });
     });
+
+    const reportWithBuildRequest = {
+        "buildNumber": "123",
+        "buildTime": "2013-02-28T10:12:03.388304",
+        "builderName": "someBuilder",
+        "builderPassword": "somePassword",
+        "platform": "Mountain Lion",
+        "buildRequest": "700",
+        "tests": {
+            "test": {
+                "metrics": {"FrameRate": { "current": [[[0, 4], [100, 5], [205, 3]]] }}
+            },
+        },
+    };
+
+    const anotherReportWithSameBuildRequest = {
+        "buildNumber": "124",
+        "buildTime": "2013-02-28T10:12:03.388304",
+        "builderName": "someBuilder",
+        "builderPassword": "somePassword",
+        "platform": "Lion",
+        "buildRequest": "700",
+        "tests": {
+            "test": {
+                "metrics": {"FrameRate": { "current": [[[0, 4], [100, 5], [205, 3]]] }}
+            },
+        },
+    };
+
+    it("should allow to report a build request", () => {
+        return MockData.addMockData(TestServer.database()).then(() => {
+            return reportAfterAddingBuilderAndAggregatorsWithResponse(reportWithBuildRequest);
+        }).then((response) => {
+            assert.equal(response['status'], 'OK');
+        });
+    });
+
+    it("should reject the report if the build request in the report has an existing associated build", () => {
+        return MockData.addMockData(TestServer.database()).then(() => {
+            return reportAfterAddingBuilderAndAggregatorsWithResponse(reportWithBuildRequest);
+        }).then((response) => {
+            assert.equal(response['status'], 'OK');
+            return TestServer.database().selectRows('builds', {number: '123'});
+        }).then((results) => {
+            assert.equal(results.length, 1);
+            return TestServer.database().selectRows('platforms', {name: 'Mountain Lion'});
+        }).then((results) => {
+            assert.equal(results.length, 1);
+            return TestServer.remoteAPI().postJSON('/api/report/', [anotherReportWithSameBuildRequest]);
+        }).then((response) => {
+            assert.equal(response['status'], 'FailedToUpdateBuildRequest');
+            return TestServer.database().selectRows('builds', {number: '124'});
+        }).then((results) => {
+            assert.equal(results.length, 0);
+            return TestServer.database().selectRows('platforms', {name: 'Lion'});
+        }).then((results) => {
+            assert.equal(results.length, 0);
+        });
+    });
+
 });