Bound confirmed display-crash cleanup without resurrecting quit sessions

src/xrt/state_trackers/steamvr_drv/ovrd_roomsetup_guard.cpp

@@ -168,8 +168,47 @@ static int unique_runtime_process(const fs::path &executable, pid_t expected = 0
 	return result;
 }
 
+// Read only new compositor diagnostics for this setup attempt. The assertion is
+// visible before the process finishes crash handling and its pidfd becomes ready.
+class OverflowLog
+{
+	fs::path path;
+	std::string needle;
+	uint64_t cursor = 0;
+	std::string suffix;
+public:
+	explicit OverflowLog(fs::path value, std::string match = "Vulkan transfer ring buffer overflow")
+	    : path(std::move(value)), needle(std::move(match)) {}
+	void begin()
+	{
+		std::error_code error;
+		cursor = fs::file_size(path, error);
+		if (error) cursor = 0;
+		suffix.clear();
+	}
+	bool overflowed()
+	{
+		std::error_code error;
+		uint64_t size = fs::file_size(path, error);
+		if (error) return false;
+		if (size < cursor) { cursor = 0; suffix.clear(); }
+		if (size == cursor) return false;
+		std::ifstream stream(path, std::ios::binary);
+		stream.seekg(cursor);
+		std::string bytes(static_cast<size_t>(std::min<uint64_t>(size - cursor, 65536)), '\0');
+		stream.read(bytes.data(), bytes.size());
+		bytes.resize(static_cast<size_t>(stream.gcount()));
+		cursor += bytes.size();
+		suffix += bytes;
+		bool found = suffix.find(needle) != std::string::npos;
+		if (suffix.size() > 128) suffix.erase(0, suffix.size() - 128);
+		return found;
+	}
+};
+
+
 // Replacement compositors can retain a broken direct-display session and grow
-// NVIDIA/SteamVR descriptors. Refresh the whole session after transport returns.
+// NVIDIA/SteamVR descriptors. Drain the crashed session after transport returns.
 // This bounded helper deliberately outlives the old server during shutdown.
 static int recover_display()
 {
@@ -188,8 +227,8 @@ static int recover_display()
 		return true;
 	};
 	if (!wait_session(std::chrono::seconds(10))) {
-		// A wedged old server must not leave an already-stopped monitor without
-		// a replacement. Signal only the pinned old session generations.
+		// A wedged old server must not retain the failed display resources.
+		// Signal only the pinned old session generations.
 		for (int fd : {3, 4, 5})
 			if (!exited(fd)) syscall(SYS_pidfd_send_signal, fd, SIGTERM, nullptr, 0);
 		if (!wait_session(std::chrono::seconds(5))) {
@@ -199,8 +238,10 @@ static int recover_display()
 		}
 	}
 	close(3); close(4); close(5);
-	execlp("steam", "steam", "steam://rungameid/250820", nullptr);
-	return 1;
+	// The guard is gone now, so it cannot observe a later user Quit. Never launch
+	// Steam from this detached helper; the user owns starting the next session.
+	fprintf(stderr, "monado-roomsetup-guard: display-loss session stopped; start SteamVR again\n");
+	return 0;
 }
 
 class DisplayHealth
@@ -222,32 +263,41 @@ class DisplayRecovery
 {
 	int compositor = -1;
 	pid_t requester = -1;
-	bool connected_once = false, lost = false;
+	bool connected_once = false, lost = false, crash_seen = false, quit_requested = false;
+	unsigned loss_streak = 0, attempts = 0;
+	OverflowLog crash_log;
 	std::chrono::steady_clock::time_point next_request{};
 	DisplayHealth display_health;
 public:
+	explicit DisplayRecovery(fs::path log = {})
+	    : crash_log(std::move(log), "Failed Watchdog timeout in thread Render in WaitForPresent")
+	{ crash_log.begin(); }
+	void note_quit() { quit_requested = true; }
 	~DisplayRecovery()
 	{
 		if (compositor >= 0) close(compositor);
-		// The session restart helper must survive this guard/server's shutdown.
+		// The cleanup helper must survive this guard/server's shutdown.
 		if (requester > 0) waitpid(requester, nullptr, WNOHANG);
 	}
 	void update(bool connected, float vsync_age, uint64_t vsync, int server, pid_t server_pid, const char *library)
 	{
 		auto now = std::chrono::steady_clock::now();
+		if (quit_requested) return;
 		if (requester > 0) {
 			int status = 0;
 			pid_t result = waitpid(requester, &status, WNOHANG);
 			if (result == requester || (result < 0 && errno == ECHILD)) {
 				requester = -1;
 				next_request = now + std::chrono::seconds(5);
-				fprintf(stderr, "monado-roomsetup-guard: session restart did not complete; retry in 5 seconds\n");
+				fprintf(stderr, "monado-roomsetup-guard: display-loss cleanup helper exited; retries capped at three\n");
 			}
 		}
+		if (loss_streak || lost) crash_seen |= crash_log.overflowed();
 		fs::path runtime = fs::path(library).parent_path();
 		bool stable = display_health.update(connected && !exited(compositor), vsync_age, vsync, now);
 		if (connected) {
 			connected_once = true;
+			loss_streak = 0;
 			if (exited(compositor)) {
 				// Adopt a replacement compositor if SteamVR already respawned
 				// one: killing the session then would destroy a self-healed
@@ -261,18 +311,30 @@ public:
 			}
 			// A watchdog can exit well after USB returns. Disarm only after native
 			// vsync advances continuously, never merely because the process lives.
-			if (lost && stable) lost = false;
+			if (lost && stable) {
+				lost = crash_seen = false;
+				attempts = 0;
+				crash_log.begin();
+			}
 		} else if (connected_once) {
-			lost = true;
+			if (loss_streak == 0 && !lost) { crash_log.begin(); crash_seen = false; }
+			// Five consecutive 100 ms polls reject one transient connection miss.
+			if (loss_streak < 5) ++loss_streak;
+			if (loss_streak >= 5) lost = true;
 		}
-		if (!connected || !lost || compositor < 0 || !exited(compositor) || requester > 0 ||
+		if (!connected || !lost || !crash_seen || attempts >= 3 || compositor < 0 || !exited(compositor) || requester > 0 ||
 		    now < next_request) return;
 		next_request = now + std::chrono::seconds(5);
 		int current_server = unique_runtime_process(runtime / "vrserver", server_pid);
 		if (current_server < 0) return;
 		close(current_server);
 		int monitor = unique_runtime_process(runtime / "vrmonitor");
-		if (monitor < 0) return;
+		if (monitor < 0) {
+			// There is no monitor generation left to signal. /dev/null is already
+			// readable, so exited() treats this slot as complete.
+			monitor = open("/dev/null", O_RDONLY | O_CLOEXEC);
+			if (monitor < 0) return;
+		}
 		char executable[4096];
 		ssize_t length = readlink("/proc/self/exe", executable, sizeof(executable) - 1);
 		if (length <= 0) { close(monitor); return; }
@@ -300,45 +362,13 @@ public:
 		posix_spawn_file_actions_destroy(&actions);
 		close(server_child); close(monitor_child); close(compositor_child);
 		if (spawned) requester = -1;
-		else fprintf(stderr, "monado-roomsetup-guard: automatically restarting SteamVR after display loss\n");
+		else {
+			++attempts;
+			fprintf(stderr, "monado-roomsetup-guard: draining crashed display-loss session, attempt %u/3; restart SteamVR manually\n", attempts);
+		}
 	}
 };
 
-// Read only new compositor diagnostics for this setup attempt. The assertion is
-// visible before the process finishes crash handling and its pidfd becomes ready.
-class OverflowLog
-{
-	fs::path path;
-	uint64_t cursor = 0;
-	std::string suffix;
-public:
-	explicit OverflowLog(fs::path value) : path(std::move(value)) {}
-	void begin()
-	{
-		std::error_code error;
-		cursor = fs::file_size(path, error);
-		if (error) cursor = 0;
-		suffix.clear();
-	}
-	bool overflowed()
-	{
-		std::error_code error;
-		uint64_t size = fs::file_size(path, error);
-		if (error) return false;
-		if (size < cursor) { cursor = 0; suffix.clear(); }
-		if (size == cursor) return false;
-		std::ifstream stream(path, std::ios::binary);
-		stream.seekg(cursor);
-		std::string bytes(static_cast<size_t>(std::min<uint64_t>(size - cursor, 65536)), '\0');
-		stream.read(bytes.data(), bytes.size());
-		bytes.resize(static_cast<size_t>(stream.gcount()));
-		cursor += bytes.size();
-		suffix += bytes;
-		bool found = suffix.find("Vulkan transfer ring buffer overflow") != std::string::npos;
-		if (suffix.size() > 128) suffix.erase(0, suffix.size() - 128);
-		return found;
-	}
-};
 
 struct Snapshot
 {
@@ -496,7 +526,7 @@ int main(int argc, char **argv)
 	std::string world_warning;
 	vr::VRNotificationId world_notification = 0;
 	auto next_notification_attempt = std::chrono::steady_clock::time_point{};
-	DisplayRecovery display_recovery;
+	DisplayRecovery display_recovery(log_path);
 	auto next_display_check = std::chrono::steady_clock::time_point{};
 	while (running && !exited(parent)) {
 		// Test before consuming Accept's events, including a compositor that exits immediately before Accept.
@@ -569,7 +599,7 @@ int main(int argc, char **argv)
 			    event.eventType == vr::VREvent_RoomViewShown || event.eventType == vr::VREvent_RoomViewHidden ||
 			    event.eventType == vr::VREvent_DashboardActivated || event.eventType == vr::VREvent_DashboardDeactivated))
 				log_coordinates(*system, *setup, event.eventType, step);
-			if (event.eventType == vr::VREvent_Quit) running = false;
+			if (event.eventType == vr::VREvent_Quit) { display_recovery.note_quit(); running = false; }
 		}
 		if (!running) break;
 		if (world_setup && world_cancel_deadline != std::chrono::steady_clock::time_point{} &&

tests/tests_steamvr_roomsetup_guard.cpp

@@ -287,7 +287,7 @@ namespace {
 struct FakeRuntime
 {
 	fs::path dir;
-	fs::path library;
+	fs::path library, log;
 	pid_t compositor = -1, server = -1, monitor = -1;
 	int server_fd = -1;
 
@@ -334,12 +334,15 @@ struct FakeRuntime
 		fs::create_directories(dir, error);
 		REQUIRE_FALSE(error);
 		library = (dir / "libopenvr_api.so").string();
+		log = dir / "vrcompositor.txt";
+		std::ofstream(log) << "old log\n";
 		compositor = spawn(copy_sleeper(dir, "vrcompositor"));
 		server = spawn(copy_sleeper(dir, "vrserver"));
 		monitor = spawn(copy_sleeper(dir, "vrmonitor"));
 		server_fd = static_cast<int>(syscall(SYS_pidfd_open, server, 0));
 		REQUIRE(server_fd >= 0);
 	}
+	void crash_evidence() { std::ofstream(log, std::ios::app) << "Failed Watchdog timeout in thread Render in WaitForPresent\n"; }
 	void stop_compositor()
 	{
 		stop(compositor);
@@ -351,7 +354,7 @@ struct FakeRuntime
 	}
 	// The failed spawn below execs this test binary with an unknown flag, so
 	// the helper child exits without signalling anything. Only the parent's
-	// "automatically restarting" line marks a fired recovery.
+	// "draining crashed" line marks a fired recovery.
 	template <typename Poll> static std::string fired(Poll poll)
 	{
 		int saved = dup(STDERR_FILENO);
@@ -387,25 +390,110 @@ struct FakeRuntime
 TEST_CASE("Display recovery adopts a respawned compositor instead of restarting the session")
 {
 	FakeRuntime runtime("healed");
-	DisplayRecovery recovery;
+	DisplayRecovery recovery(runtime.log);
 	const char *library = runtime.library.c_str();
 	// Pin the first compositor generation, then arm the loss latch.
 	recovery.update(true, .01f, 100, runtime.server_fd, runtime.server, library);
-	recovery.update(false, .01f, 101, runtime.server_fd, runtime.server, library);
+	for (int i = 0; i < 5; ++i) recovery.update(false, .01f, 101, runtime.server_fd, runtime.server, library);
+	runtime.crash_evidence();
 	// SteamVR respawns the compositor on its own before the next check.
 	runtime.stop_compositor();
 	runtime.restart_compositor();
 	std::string output = FakeRuntime::fired([&] {
 		recovery.update(true, .01f, 101, runtime.server_fd, runtime.server, library);
 	});
-	CHECK(output.find("automatically restarting") == std::string::npos);
+	CHECK(output.find("draining crashed") == std::string::npos);
 	// The rig above can fire: with no replacement left, the dead handle triggers.
 	runtime.stop_compositor();
 	output = FakeRuntime::fired([&] {
 		recovery.update(true, .01f, 101, runtime.server_fd, runtime.server, library);
 	});
-	CHECK(output.find("automatically restarting") != std::string::npos);
+	CHECK(output.find("draining crashed") != std::string::npos);
 	// Let the helper child exit so the destructor does not leave a zombie.
 	std::this_thread::sleep_for(std::chrono::milliseconds(200));
 	recovery.update(false, -1, 101, runtime.server_fd, runtime.server, library);
 }
+
+TEST_CASE("Display cleanup requires sustained loss and fresh watchdog proof, and honors quit")
+{
+	FakeRuntime runtime("proof");
+	DisplayRecovery recovery(runtime.log);
+	const char *library = runtime.library.c_str();
+	recovery.update(true, .01f, 100, runtime.server_fd, runtime.server, library);
+	recovery.update(false, -1, 100, runtime.server_fd, runtime.server, library);
+	runtime.crash_evidence();
+	runtime.stop_compositor();
+	auto output = FakeRuntime::fired([&] {
+		recovery.update(true, -1, 100, runtime.server_fd, runtime.server, library);
+	});
+	CHECK(output.find("draining crashed") == std::string::npos);
+	for (int i = 0; i < 5; ++i) recovery.update(false, -1, 100, runtime.server_fd, runtime.server, library);
+	// Prior-episode watchdog evidence cannot authorize this loss.
+	output = FakeRuntime::fired([&] {
+		recovery.update(true, -1, 100, runtime.server_fd, runtime.server, library);
+	});
+	CHECK(output.find("draining crashed") == std::string::npos);
+	runtime.crash_evidence();
+	recovery.note_quit();
+	output = FakeRuntime::fired([&] {
+		recovery.update(true, -1, 100, runtime.server_fd, runtime.server, library);
+	});
+	CHECK(output.find("draining crashed") == std::string::npos);
+}
+
+TEST_CASE("Display cleanup works without a monitor and stops at three attempts")
+{
+	FakeRuntime runtime("bounded");
+	FakeRuntime::stop(runtime.monitor);
+	runtime.monitor = -1;
+	DisplayRecovery recovery(runtime.log);
+	const char *library = runtime.library.c_str();
+	recovery.update(true, .01f, 100, runtime.server_fd, runtime.server, library);
+	for (int i = 0; i < 5; ++i) recovery.update(false, -1, 100, runtime.server_fd, runtime.server, library);
+	runtime.crash_evidence();
+	runtime.stop_compositor();
+	for (unsigned attempt = 1; attempt <= 3; ++attempt) {
+		auto output = FakeRuntime::fired([&] {
+			recovery.update(true, -1, 100, runtime.server_fd, runtime.server, library);
+		});
+		CHECK(output.find("attempt " + std::to_string(attempt) + "/3") != std::string::npos);
+		std::this_thread::sleep_for(std::chrono::milliseconds(200));
+		recovery.update(true, -1, 100, runtime.server_fd, runtime.server, library);
+		std::this_thread::sleep_for(std::chrono::milliseconds(5050));
+	}
+	auto output = FakeRuntime::fired([&] {
+		recovery.update(true, -1, 100, runtime.server_fd, runtime.server, library);
+	});
+	CHECK(output.find("draining crashed") == std::string::npos);
+}
+
+TEST_CASE("Detached cleanup never launches Steam after stopping the pinned session")
+{
+	FakeRuntime runtime("helper");
+	// If the old helper tries exec, this private Steam command records it.
+	auto marker = runtime.dir / "relaunched";
+	auto steam = runtime.dir / "steam";
+	std::ofstream(steam) << "#!/bin/sh\ntouch '" << marker.string() << "'\n";
+	REQUIRE(chmod(steam.c_str(), 0700) == 0);
+	int sources[] = {
+		fcntl(runtime.server_fd, F_DUPFD_CLOEXEC, 10),
+		static_cast<int>(syscall(SYS_pidfd_open, runtime.monitor, 0)),
+		static_cast<int>(syscall(SYS_pidfd_open, runtime.compositor, 0))};
+	for (int fd : sources) REQUIRE(fd >= 0);
+	pid_t helper = fork();
+	REQUIRE(helper >= 0);
+	if (helper == 0) {
+		int copies[3];
+		for (int i = 0; i < 3; ++i) copies[i] = fcntl(sources[i], F_DUPFD_CLOEXEC, 10);
+		for (int i = 0; i < 3; ++i) dup2(copies[i], 3 + i);
+		setenv("PATH", runtime.dir.c_str(), 1);
+		_exit(recover_display());
+	}
+	for (int fd : sources) close(fd);
+	int status = 0;
+	REQUIRE(waitpid(helper, &status, 0) == helper);
+	CHECK(WIFEXITED(status));
+	CHECK(WEXITSTATUS(status) == 0);
+	CHECK_FALSE(fs::exists(marker));
+	CHECK(exited(runtime.server_fd));
+}