dongjoon-hyun commented on PR #57854: URL: https://github.com/apache/spark/pull/57854#issuecomment-5228134724
Both hardenings are real improvements that match this feature's security model (loopback-only, same-user sharing), and treating a failed/timed-out `ps` probe as "not our server" is the right conservative default. A few comments, mostly minor: 1. **Orphaned server when the `ps` probe fails transiently.** `stop()` calls `self._discovery.clear()` regardless of the verification outcome. If the server is actually alive but `ps` transiently fails (e.g. hits the 5s timeout), we skip the signal yet still delete the discovery file and the daemon pid file, so a later `--stop` can never find that server again and the user has to `kill` it manually. It may be worth distinguishing "could not verify" (keep discovery) from "verified as a different process" (clear it), or at least reflecting this case in the `--stop` CLI output. 2. **The `_await_ready` timeout path still signals without verification.** The cleanup path in `_await_ready` SIGTERMs `daemon_pid()` unchecked. That pid was written moments earlier by `spark-daemon.sh`, so reuse risk is negligible — just noting it for consistency with the new guard. 3. **TOCTOU window.** There is still a window between the `ps` check and `os.kill` where the pid could be recycled. Unavoidable without pidfd, and the check shrinks the window substantially — fine as is, just noting the guard is a mitigation, not an elimination. 4. **IPv6 environments.** The client connects to `sc://localhost:<port>` while the server now binds only `127.0.0.1`. On hosts where `localhost` resolves to `::1` first, this relies on the gRPC resolver falling back to IPv4 (it generally does, but IPv6-only setups would fail). The readiness probe is unaffected since it uses an `AF_INET` socket. Might deserve a one-line mention somewhere. 5. **Explicitly-set `binding.address` is silently overridden.** Intentional and consistent with how `binding.port` is handled, but since a user-provided conf is now silently dropped, a line in the module docstring would help. 6. **Test mock breadth (nit).** `test_server_launcher_binds_to_loopback` patches `os.path.isfile` globally to `True`, which also affects `_find_spark_home()` internals when `SPARK_HOME` is unset. Narrowing the patch to the script-existence check would make the test more robust. 7. **PR description (nit).** "The complete PySpark Connect test environment will be run before the staged proposal is published as an OSS PR" in the testing section reads like a leftover from internal tooling — probably worth replacing with the actual test command. None of these are blockers. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
