Revert "siso: clean up _start_collector function to not modify the env dict."

This reverts commit 02da7602fb.

Reason for revert: Speculatively reverting for http://b/474306065.

Original change's description:
> siso: clean up _start_collector function to not modify the env dict.
>
> Also do not have special status for missing socks file. Clean up socks file before starting the collector.
>
> Bug: b/455433899
> Change-Id: Ib393f00e7a6098fca1b7d1d70f5aa4616a6a6964
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7408697
> Commit-Queue: Alex Ovsienko <ovsienko@google.com>
> Reviewed-by: Junji Watanabe <jwata@google.com>

Bug: b/455433899
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Change-Id: Ie095f91c72bca65102a96fa33e77b87e79cab9ac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/7407117
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Junji Watanabe <jwata@google.com>

siso.py

@@ -119,14 +119,15 @@ def _kill_collector() -> bool:
 
 
 # Start collector when present.
-# Returns boolean whether collector has started successfully.
-def _start_collector(siso_path: str, expected_endpoint: str, project: str,
+# Returns boolean whether collector has started successfully and a potential sockets path.
+def _start_collector(siso_path: str, sockets_file: Optional[str], project: str,
                      env: dict[str, str]) -> bool:
     class Status(Enum):
         HEALTHY = 1
         WRONG_ENDPOINT = 2
-        UNHEALTHY = 3
-        DEAD = 4
+        NO_SOCKETS = 3
+        UNHEALTHY = 4
+        DEAD = 5
 
     def collector_status() -> Status:
         conn = http.client.HTTPConnection(f"localhost:{_OTLP_HEALTH_PORT}")
@@ -144,9 +145,12 @@ def _start_collector(siso_path: str, expected_endpoint: str, project: str,
             return Status.UNHEALTHY
         endpoint = fetch_receiver_endpoint(conn)
 
-        # Collector is liable to drop unix:// part from socks.
-        if not expected_endpoint.endswith(endpoint):
+        expected_endpoint = sockets_file or _OTLP_DEFAULT_TCP_ENDPOINT
+        if endpoint != expected_endpoint:
             return Status.WRONG_ENDPOINT
+        if sockets_file:
+            if not os.path.exists(sockets_file):
+                return Status.NO_SOCKETS
 
         return Status.HEALTHY
 
@@ -163,12 +167,16 @@ def _start_collector(siso_path: str, expected_endpoint: str, project: str,
         except KeyError:
             return ""
 
-    def start_collector() -> None:
+    def start_collector(sockets_file: Optional[str]) -> None:
         # Use Popen as it's non blocking.
         creationflags = 0
         if sys.platform == "win32":
             creationflags = subprocess.CREATE_NEW_PROCESS_GROUP
         cmd = [siso_path, "collector", "--project", project]
+        if sockets_file:
+            env["SISO_COLLECTOR_ADDRESS"] = f"unix://{sockets_file}"
+        else:
+            env["SISO_COLLECTOR_ADDRESS"] = _OTLP_DEFAULT_TCP_ENDPOINT
         subprocess.Popen(
             cmd,
             stdout=subprocess.DEVNULL,
@@ -186,7 +194,18 @@ def _start_collector(siso_path: str, expected_endpoint: str, project: str,
         if not _kill_collector():
             return False
 
-    start_collector()
+    # Delete possibly existing sockets file.
+    if sockets_file and os.path.exists(sockets_file):
+        try:
+            os.remove(sockets_file)
+        except OSError as e:
+            print(f"Failed to remove {sockets_file} due to {e}. " +
+                  "Having existing sockets file is known to cause " +
+                  "permission issues among others. Not using collector.",
+                  file=sys.stderr)
+            return False
+
+    start_collector(sockets_file)
 
     while time.time() - start < 1:
         status = collector_status()
@@ -318,24 +337,12 @@ def _handle_collector(siso_path: str, args: list[str],
     if not project:
         lenv.pop("SISO_COLLECTOR_ADDRESS", None)
         return lenv
+    sockets_file = None
     if sys.platform in ["darwin", "linux"]:
         path, remainder_len = _resolve_sockets_folder(env)
         sockets_file = os.path.join(path, f"{project[:remainder_len]}.sock")
-        if os.path.exists(sockets_file):
-            try:
-                os.remove(sockets_file)
-            except OSError as e:
-                print(f"Failed to remove {sockets_file} due to {e}. " +
-                      "Having existing sockets file is known to cause " +
-                      "permission issues among others. Not using collector.",
-                      file=sys.stderr)
-                return lenv
-        expected_endpoint = f"unix://{sockets_file}"
-    else:
-        expected_endpoint = _OTLP_DEFAULT_TCP_ENDPOINT
-    lenv["SISO_COLLECTOR_ADDRESS"] = expected_endpoint
 
-    started = _start_collector(siso_path, expected_endpoint, project, lenv)
+    started = _start_collector(siso_path, sockets_file, project, lenv)
     if not started:
         print("Collector never came to life", file=sys.stderr)
         lenv.pop("SISO_COLLECTOR_ADDRESS", None)

tests/siso_test.py

@@ -270,7 +270,7 @@ def test_handle_collector_args_starts_linux(siso_test_fixture: Any,
         return "test-project"
 
     def start_collector_side_effect(siso_path, sockets_file, project, env):
-        env["SISO_COLLECTOR_ADDRESS"] = sockets_file
+        env["SISO_COLLECTOR_ADDRESS"] = f"unix://{sockets_file}"
         return True
 
     mock_start_collector.side_effect = start_collector_side_effect
@@ -280,10 +280,9 @@ def test_handle_collector_args_starts_linux(siso_test_fixture: Any,
 
     sockets_file = os.path.join("/tmp", "run", "testuser", "siso",
                                 "test-project.sock")
-    expected_endpoint = f"unix://{sockets_file}"
-    mock_start_collector.assert_called_once_with(siso_path, expected_endpoint,
+    mock_start_collector.assert_called_once_with(siso_path, sockets_file,
                                                  "test-project", res_env)
-    assert res_env["SISO_COLLECTOR_ADDRESS"] == expected_endpoint
+    assert res_env["SISO_COLLECTOR_ADDRESS"] == f"unix://{sockets_file}"
 
 
 def test_handle_collector_args_starts_windows(mocker: Any) -> None:
@@ -305,8 +304,8 @@ def test_handle_collector_args_starts_windows(mocker: Any) -> None:
 
     assert res_env["SISO_COLLECTOR_ADDRESS"] == siso._OTLP_DEFAULT_TCP_ENDPOINT
     mock_fetch.assert_called_once_with(args, env)
-    mock_start_collector.assert_called_once_with(
-        siso_path, siso._OTLP_DEFAULT_TCP_ENDPOINT, "test-project", res_env)
+    mock_start_collector.assert_called_once_with(siso_path, None,
+                                                 "test-project", res_env)
 
 
 @pytest.mark.skipif(sys.platform == "win32",
@@ -334,8 +333,7 @@ def test_handle_collector_args_fails(siso_test_fixture: Any,
     assert "SISO_COLLECTOR_ADDRESS" not in res_env
     sockets_file = os.path.join("/tmp", "run", "testuser", "siso",
                                 "test-project.sock")
-    expected_endpoint = f"unix://{sockets_file}"
-    mock_start_collector.assert_called_once_with(siso_path, expected_endpoint,
+    mock_start_collector.assert_called_once_with(siso_path, sockets_file,
                                                  "test-project", res_env)
 
 
@@ -399,39 +397,68 @@ def _configure_http_responses(
     mock_conn.getresponse.side_effect = getresponse_side_effect
 
 
-def test_handle_collector_removes_existing_socket_file(
-        siso_test_fixture: Any, start_collector_mocks: Dict[str, Any],
-        mocker: Any) -> None:
+def test_start_collector_removes_existing_socket_file(
+        start_collector_mocks: Dict[str, Any], mocker: Any) -> None:
     mocker.patch("sys.platform", new="linux")
     mock_os_path_exists = mocker.patch("os.path.exists", return_value=True)
     mock_os_remove = mocker.patch("os.remove")
-    mocker.patch("siso._fetch_metrics_project", return_value="test-project")
-    mocker.patch("siso._start_collector", return_value=True)
 
     siso_path = "siso_path"
-    sockets_file = os.path.join("/tmp", "testuser", "siso", "test-project.sock")
-
-    siso._handle_collector(siso_path, ["ninja"], {})
-
+    project = "test-project"
+    sockets_file = os.path.join("/tmp", "test.sock")
+    _configure_http_responses(mocker,
+                              start_collector_mocks["mock_conn"],
+                              status_responses=[(404, None), (200, None)],
+                              config_responses=[(200, None), (200, None)])
+    status_healthy = {"healthy": True, "status": "StatusOK"}
+    config = {
+        "receivers": {
+            "otlp": {
+                "protocols": {
+                    "grpc": {
+                        "endpoint": sockets_file
+                    }
+                }
+            }
+        }
+    }
+    mocker.patch("siso.json.loads",
+                 side_effect=[status_healthy, config, config])
+    siso._start_collector(siso_path, sockets_file, project, {})
     mock_os_path_exists.assert_called_with(sockets_file)
     mock_os_remove.assert_called_with(sockets_file)
 
 
-def test_handle_collector_remove_socket_file_fails(siso_test_fixture: Any,
-                                                   start_collector_mocks: Dict[
-                                                       str, Any],
-                                                   mocker: Any) -> None:
+def test_start_collector_remove_socket_file_fails(
+        start_collector_mocks: Dict[str, Any], mocker: Any) -> None:
     mocker.patch("sys.platform", new="linux")
     mock_os_path_exists = mocker.patch("os.path.exists", return_value=True)
     mock_os_remove = mocker.patch("os.remove",
                                   side_effect=OSError("Permission denied"))
     mock_stderr = mocker.patch("sys.stderr", new_callable=io.StringIO)
-    mocker.patch("siso._fetch_metrics_project", return_value="test-project")
 
     siso_path = "siso_path"
-    sockets_file = os.path.join("/tmp", "testuser", "siso", "test-project.sock")
-
-    siso._handle_collector(siso_path, ["ninja"], {})
+    project = "test-project"
+    sockets_file = os.path.join("/tmp", "test.sock")
+    _configure_http_responses(mocker,
+                              start_collector_mocks["mock_conn"],
+                              status_responses=[(404, None), (200, None)],
+                              config_responses=[(200, None), (200, None)])
+    status_healthy = {"healthy": True, "status": "StatusOK"}
+    config = {
+        "receivers": {
+            "otlp": {
+                "protocols": {
+                    "grpc": {
+                        "endpoint": siso._OTLP_DEFAULT_TCP_ENDPOINT
+                    }
+                }
+            }
+        }
+    }
+    mocker.patch("siso.json.loads",
+                 side_effect=[status_healthy, config, config])
+    siso._start_collector(siso_path, sockets_file, project, {})
 
     mock_os_path_exists.assert_called_with(sockets_file)
     mock_os_remove.assert_called_with(sockets_file)
@@ -760,8 +787,7 @@ def test_start_collector_dead_then_healthy(platform: str, creationflags: int,
     mock_json_loads.side_effect = [status_healthy, config]
 
     env = {}
-    result = siso._start_collector(siso_path, siso._OTLP_DEFAULT_TCP_ENDPOINT,
-                                   project, env)
+    result = siso._start_collector(siso_path, None, project, env)
 
     assert result
     m["subprocess_popen"].assert_called_once_with(
@@ -810,8 +836,7 @@ def test_start_collector_unhealthy_then_healthy(
     ]
 
     env = {}
-    result = siso._start_collector(siso_path, siso._OTLP_DEFAULT_TCP_ENDPOINT,
-                                   project, env)
+    result = siso._start_collector(siso_path, None, project, env)
 
     assert result
     m["subprocess_popen"].assert_called_once_with(
@@ -856,8 +881,7 @@ def test_start_collector_already_healthy(start_collector_mocks: Dict[str, Any],
         status_healthy, config_project_full, config_project_full
     ]
 
-    result = siso._start_collector(siso_path, siso._OTLP_DEFAULT_TCP_ENDPOINT,
-                                   project, {})
+    result = siso._start_collector(siso_path, None, project, {})
 
     assert result
     m["subprocess_popen"].assert_not_called()
@@ -875,8 +899,7 @@ def test_start_collector_never_healthy(start_collector_mocks: Dict[str, Any],
                               status_responses=[(404, None)])
 
     env = {}
-    siso._start_collector(siso_path, siso._OTLP_DEFAULT_TCP_ENDPOINT, project,
-                          env)
+    siso._start_collector(siso_path, None, project, env)
 
     m["subprocess_popen"].assert_called_once_with(
         [siso_path, "collector", "--project", project],
@@ -924,8 +947,7 @@ def test_start_collector_healthy_after_retries(start_collector_mocks: Dict[str,
     ]
 
     env = {}
-    result = siso._start_collector(siso_path, siso._OTLP_DEFAULT_TCP_ENDPOINT,
-                                   project, env)
+    result = siso._start_collector(siso_path, None, project, env)
 
     assert result
     m["subprocess_popen"].assert_called_once_with(
@@ -939,20 +961,28 @@ def test_start_collector_healthy_after_retries(start_collector_mocks: Dict[str,
 
 
 @pytest.mark.parametrize(
-    "expected_result, http_status_responses, json_loads_side_effect_values", [
-        (True, [(404, None),
-                (200, None)], ["status_healthy", "config_with_socket"]),
-        (True, [(404, None)] + [(404, None)] * 5 + [(200, None)],
-         ["status_healthy", "config_with_socket"]),
-        (False, [(404, None)] * 30, []),
+    "os_path_exists_side_effect, expected_result, http_status_responses, json_loads_side_effect_values, expected_os_exists_calls",
+    [
+        (itertools.repeat(True), True, [
+            (404, None), (200, None)
+        ], ["status_healthy", "config_with_socket"], 2),
+        ([False] * 8 + [True], True, [(404, None)] + [(200, None)] * 9,
+         ["status_healthy", "config_with_socket"] * 9, 9),
+        ([False] * 10, False, [(404, None)] + [(200, None)] * 9,
+         ["status_healthy", "config_with_socket"] * 9, 10),
     ],
-    ids=["healthy_immediately", "healthy_later", "never_healthy"])
+    ids=["socket_exists", "appears_later", "never_appears"])
 def test_start_collector_with_sockets_file(
         start_collector_mocks: Dict[str, Any], mocker: Any,
-        expected_result: bool, http_status_responses: List[Tuple[int, Any]],
-        json_loads_side_effect_values: List[str]) -> None:
+        os_path_exists_side_effect: Any, expected_result: bool,
+        http_status_responses: List[Tuple[int, Any]],
+        json_loads_side_effect_values: List[str],
+        expected_os_exists_calls: int) -> None:
     mocker.patch("sys.platform", new="linux")
     mock_json_loads = mocker.patch("siso.json.loads")
+    mocker.patch("os.path.isfile", return_value=False)
+    mock_os_exists = mocker.patch("os.path.exists")
+    mocker.patch("os.remove")
     siso_path = "siso_path"
     project = "test-project"
     sockets_file = os.path.join("/tmp", "test-socket.sock")
@@ -978,6 +1008,7 @@ def test_start_collector_with_sockets_file(
     ]
 
     m = start_collector_mocks
+    mock_os_exists.side_effect = os_path_exists_side_effect
     mock_json_loads.side_effect = json_loads_side_effect
 
     _configure_http_responses(mocker,
@@ -997,6 +1028,8 @@ def test_start_collector_with_sockets_file(
         env=env,
         creationflags=0)
     m["kill_collector"].assert_not_called()
+    assert mock_os_exists.call_count == expected_os_exists_calls
+
 
 # Stanza to have pytest be executed.
 if __name__ == "__main__":