fix(app,recording): Save Diagnostics UI + faster #460 shutdown

electron/ipc/handlers.ts

@@ -1664,6 +1664,66 @@ async function resolveMediaLinksForVideo(videoPath: string): Promise<{
 	return { resolvedVia: "none" };
 }
 
+/**
+ * Writes the diagnostic bundle a bug report needs: app/OS facts, the native
+ * helpers' raw stdout/stderr (which is where `[stop-timing]` and
+ * `encoder-selection` land — see nativeWindowsCaptureStop.ts), and the main
+ * process's own recent console output. Shared by the renderer's IPC call and
+ * the menu/tray "Save Diagnostics" entry point in main.ts, which has no
+ * renderer-side `projectState`/`logs` to offer and does not need to.
+ */
+export async function exportDiagnosticFile(payload: {
+	error: string;
+	stack?: string;
+	projectState: unknown;
+	logs: string[];
+}) {
+	const { filePath, canceled } = await dialog.showSaveDialog({
+		title: "Save Diagnostic File",
+		defaultPath: `openscreen-diagnostic-${Date.now()}.json`,
+		filters: [{ name: "JSON", extensions: ["json"] }],
+	});
+
+	if (canceled || !filePath) return { success: false, canceled: true };
+
+	const HELPER_OUTPUT_MAX_BYTES = 64 * 1024;
+	const tail = (s: string, max: number) => (s.length <= max ? s : s.slice(s.length - max));
+
+	const diagnostic = {
+		timestamp: new Date().toISOString(),
+		appVersion: app.getVersion(),
+		platform: process.platform,
+		arch: process.arch,
+		// The same fact the About box leads with, and for the same reason: it is what
+		// explains why a copy does or does not offer an update check. This file is the
+		// artifact users actually attach, so it must not be the one that omits it.
+		channel: getInstallChannel(),
+		osRelease: os.release(),
+		osVersion: os.version(),
+		totalMemoryMB: Math.round(os.totalmem() / 1024 / 1024),
+		nodeVersion: process.versions.node,
+		electronVersion: process.versions.electron,
+		chromeVersion: process.versions.chrome,
+		error: payload.error,
+		stack: payload.stack,
+		projectState: payload.projectState,
+		recentLogs: payload.logs,
+		helperOutput: {
+			windows: tail(nativeWindowsCaptureOutput, HELPER_OUTPUT_MAX_BYTES),
+			mac: tail(nativeMacCaptureOutput, HELPER_OUTPUT_MAX_BYTES),
+		},
+		mainProcessLogs: mainLogBuffer.snapshot(),
+	};
+
+	try {
+		await fs.writeFile(filePath, JSON.stringify(diagnostic, null, 2), "utf-8");
+		return { success: true, path: filePath };
+	} catch (error) {
+		console.error("Failed to write diagnostic file:", error);
+		return { success: false, error: String(error) };
+	}
+}
+
 export function registerIpcHandlers(
 	createEditorWindow: () => void,
 	createSourceSelectorWindow: () => BrowserWindow,
@@ -4092,55 +4152,8 @@ export function registerIpcHandlers(
 
 	ipcMain.handle(
 		"save-diagnostic",
-		async (
-			_,
-			payload: { error: string; stack?: string; projectState: unknown; logs: string[] },
-		) => {
-			const { filePath, canceled } = await dialog.showSaveDialog({
-				title: "Save Diagnostic File",
-				defaultPath: `openscreen-diagnostic-${Date.now()}.json`,
-				filters: [{ name: "JSON", extensions: ["json"] }],
-			});
-
-			if (canceled || !filePath) return { success: false, canceled: true };
-
-			const HELPER_OUTPUT_MAX_BYTES = 64 * 1024;
-			const tail = (s: string, max: number) => (s.length <= max ? s : s.slice(s.length - max));
-
-			const diagnostic = {
-				timestamp: new Date().toISOString(),
-				appVersion: app.getVersion(),
-				platform: process.platform,
-				arch: process.arch,
-				// The same fact the About box leads with, and for the same reason: it is what
-				// explains why a copy does or does not offer an update check. This file is the
-				// artifact users actually attach, so it must not be the one that omits it.
-				channel: getInstallChannel(),
-				osRelease: os.release(),
-				osVersion: os.version(),
-				totalMemoryMB: Math.round(os.totalmem() / 1024 / 1024),
-				nodeVersion: process.versions.node,
-				electronVersion: process.versions.electron,
-				chromeVersion: process.versions.chrome,
-				error: payload.error,
-				stack: payload.stack,
-				projectState: payload.projectState,
-				recentLogs: payload.logs,
-				helperOutput: {
-					windows: tail(nativeWindowsCaptureOutput, HELPER_OUTPUT_MAX_BYTES),
-					mac: tail(nativeMacCaptureOutput, HELPER_OUTPUT_MAX_BYTES),
-				},
-				mainProcessLogs: mainLogBuffer.snapshot(),
-			};
-
-			try {
-				await fs.writeFile(filePath, JSON.stringify(diagnostic, null, 2), "utf-8");
-				return { success: true, path: filePath };
-			} catch (error) {
-				console.error("Failed to write diagnostic file:", error);
-				return { success: false, error: String(error) };
-			}
-		},
+		async (_, payload: { error: string; stack?: string; projectState: unknown; logs: string[] }) =>
+			exportDiagnosticFile(payload),
 	);
 
 	// One instance each, not one per call. DocumentService serialises saves of a

electron/main.ts

@@ -41,7 +41,11 @@ import {
 } from "./globalShortcut";
 import { mainT, setMainLocale } from "./i18n";
 import { getInstallChannel, offersUpdateCheck, platformOwnsUpdates } from "./install-channel";
-import { getSelectedDesktopSource, registerIpcHandlers } from "./ipc/handlers";
+import {
+	exportDiagnosticFile,
+	getSelectedDesktopSource,
+	registerIpcHandlers,
+} from "./ipc/handlers";
 import { installMainProcessErrorGuards } from "./main-process-errors";
 import { registerSttIpc, shutdownStt } from "./stt";
 import { checkLatestRelease } from "./update-checker";
@@ -211,6 +215,11 @@ function setupApplicationMenu() {
 					role: "about",
 					label: mainT("common", "actions.about") || "About OpenScreen",
 				},
+				{ type: "separator" as const },
+				{
+					label: mainT("common", "actions.saveDiagnostics") || "Save Diagnostics",
+					click: runSaveDiagnostics,
+				},
 				// Omitted entirely — here, in the Help menu and in the tray — where a package
 				// manager owns the update. See `canOfferUpdateCheck`.
 				...(canOfferUpdateCheck()
@@ -369,6 +378,11 @@ function setupApplicationMenu() {
 					label: mainT("common", "actions.about") || "About OpenScreen",
 					click: runAboutDialog,
 				},
+				{ type: "separator" as const },
+				{
+					label: mainT("common", "actions.saveDiagnostics") || "Save Diagnostics",
+					click: runSaveDiagnostics,
+				},
 			],
 		});
 	}
@@ -519,6 +533,47 @@ function runUpdateCheck() {
 	});
 }
 
+/**
+ * Menu and tray entry point for exporting a diagnostic bundle. The backend
+ * (`exportDiagnosticFile`) and its "Save Diagnostics" label already existed —
+ * nothing in the app ever called it (getopenscreen/openscreen#460). Reveals
+ * the written file on success, the same confirmation the export flow's "Show
+ * in folder" gives, so there is no need for a second dialog on top of the
+ * native Save dialog the user already went through.
+ *
+ * No renderer `projectState`/`logs` to attach from here, unlike the in-app
+ * crash path this shares a payload shape with — the diagnostic value for a
+ * capture bug is almost entirely `helperOutput`/`mainProcessLogs`, which
+ * `exportDiagnosticFile` reads straight from the main process regardless.
+ */
+function runSaveDiagnostics() {
+	exportDiagnosticFile({ error: "Manual diagnostic export", projectState: null, logs: [] })
+		.then((result) => {
+			if (result.canceled) return;
+			if (!result.success) {
+				// exportDiagnosticFile resolves rather than rejects on a write
+				// failure, so this is the branch that turns "user picked a save
+				// location and got silence" into a visible error instead of a
+				// menu action that looks like it did nothing.
+				showMessageBox({
+					type: "error",
+					title: PRODUCT_NAME,
+					message: mainT("dialogs", "export.failed") || "Export Failed",
+					detail: result.error,
+				}).catch((error) => {
+					console.error("[diagnostics] failure dialog failed", error);
+				});
+				return;
+			}
+			if (result.path) {
+				shell.showItemInFolder(result.path);
+			}
+		})
+		.catch((error) => {
+			console.error("[diagnostics] save failed", error);
+		});
+}
+
 /** Mirrors the flag that already drives the tray icon. An update must never interrupt a take —
  *  and on Windows it physically cannot, because the capture helpers spawn from inside the
  *  install directory and NSIS cannot overwrite a running .exe. */
@@ -730,6 +785,14 @@ function updateTrayMenu(recording: boolean = false) {
 							label: mainT("common", "actions.about") || "About OpenScreen",
 							click: runAboutDialog,
 						},
+				// Right next to About, and reachable without opening any window: this is the
+				// one place in the app most likely to still be usable right after a recording
+				// failed to stop, which is exactly when the [stop-timing]/encoder-selection
+				// lines this exports are worth the most (getopenscreen/openscreen#460).
+				{
+					label: mainT("common", "actions.saveDiagnostics") || "Save Diagnostics",
+					click: runSaveDiagnostics,
+				},
 				{ type: "separator" as const },
 				{
 					label: mainT("common", "actions.quit") || "Quit",

electron/native/wgc-capture/src/main.cpp

@@ -682,6 +682,16 @@ int main(int argc, char* argv[]) {
     // ordinary hardware, so the stop path can be regression-tested at all.
     const int testStallReadbackMs =
         std::max(0, readEnvInt("OPENSCREEN_WGC_TEST_STALL_READBACK_MS", 0));
+    // Test-only: stall the WGC frame *callback* itself while it holds the
+    // same frame lock, rather than the writer's readback -- the shape
+    // getopenscreen/openscreen#460 actually reproduced on Intel HD 520
+    // ("A WGC frame callback did not finish"). Distinct from
+    // testStallReadbackMs above because quiesceCapture()'s drain only ever
+    // sees the callback side: a stall placed in the writer instead leaves
+    // callbacksInFlight_ at zero and wgcDrained true, which cannot exercise
+    // the video-writer-join skip this stall exists to test.
+    const int testStallFrameCallbackMs =
+        std::max(0, readEnvInt("OPENSCREEN_WGC_TEST_STALL_FRAME_CALLBACK_MS", 0));
 
     std::cout << "{\"event\":\"ready\",\"schemaVersion\":2}" << std::endl;
 
@@ -931,6 +941,18 @@ int main(int argc, char* argv[]) {
             }
         }
 
+        // Gated on an already-arrived first frame: main() blocks up to 10s
+        // waiting for firstFrameWritten before it will even print
+        // recording-started, a startup budget this stall is meant to outlast
+        // (it needs to still be asleep when `stop` arrives, seconds later).
+        // Stalling the first frame trips that unrelated timeout instead of
+        // reaching the steady-state shutdown path this exists to test, and
+        // does not match the real report either -- getopenscreen/openscreen
+        // #460's diagnostic shows recording-started succeeding before the
+        // hang.
+        if (testStallFrameCallbackMs > 0 && firstFrameWritten.load()) {
+            std::this_thread::sleep_for(std::chrono::milliseconds(testStallFrameCallbackMs));
+        }
         session.context()->CopyResource(latestFrameTexture.Get(), texture);
         latestFrameTimestampHns = timestampHns;
         if (!firstFrameWritten.exchange(true)) {
@@ -1420,8 +1442,47 @@ int main(int argc, char* argv[]) {
     }
     logStopStep("audio-mixer");
     beginStopStep("video-writer-join", stepBudgetMs);
-    stopVideoWriter();
-    logStopStep("video-writer-join");
+    if (wgcDrained) {
+        stopVideoWriter();
+        logStopStep("video-writer-join");
+    } else {
+        // wgc-quiesce already reported the frame callback stuck inside the
+        // driver (getopenscreen/openscreen#460 on Intel HD 520: a
+        // CopyResource that never returns), still holding the same
+        // frame-state `mutex` writeVideoFrames takes for its own
+        // per-iteration wait -- the one it also needs to notice
+        // stopRequested. Joining is not a step that can time out here, it is
+        // one that cannot ever succeed, and this is not the only step that
+        // assumed it would: encoder.finalize() below resets the very D3D
+        // device/context a still-blocked writer thread might resume touching
+        // the moment that lock frees, and quiesceCapture()/stop() already
+        // treat "leave everything alone and let process exit reclaim it" as
+        // the only safe response to exactly this state. So this ends the
+        // process here, on this thread, rather than pretending the rest of a
+        // clean shutdown is reachable -- which cost nothing extra before
+        // today: the same TerminateProcess happened anyway, just
+        // stepBudgetMs later, once this step's own watchdog gave up waiting
+        // on a join that could never return. detach() first, not because
+        // TerminateProcess needs it (it does not touch the C++ runtime, no
+        // std::thread destructor runs), but so nothing between here and the
+        // kill can trip over a still-joinable thread.
+        //
+        // The fragmented sink writes moof+mdat incrementally, roughly once a
+        // second, so this is not a new source of loss: whatever was already
+        // on disk before the callback wedged is on disk regardless of
+        // whether Finalize() ever runs, on this path or the slower one it
+        // replaces.
+        videoWriterThread.detach();
+        std::cerr << "[stop-timing] step=video-writer-join elapsed_ms=" << stopElapsedMs()
+                  << " phase=abandoned encode_stage=" << encoder.encodeStage()
+                  << " audio_stage=" << encoder.audioStage() << " reason=frame-callback-stuck"
+                  << std::endl;
+        std::cout << "{\"event\":\"stop-timeout\",\"schemaVersion\":2,\"step\":\"video-writer-join\"}"
+                  << std::endl;
+        std::cout.flush();
+        std::cerr.flush();
+        TerminateProcess(GetCurrentProcess(), 3);
+    }
     if (usesDxgiInput) {
         std::cerr << "[frame-drops] gpu_bridge_contended=" << contendedFrames.load() << std::endl;
     }
@@ -1430,7 +1491,9 @@ int main(int argc, char* argv[]) {
     // the encoder's GPU readback, and audioMixer->stop() joined the only other
     // thread that writes to it. MFEncoder's own writerMutex_ deliberately does
     // NOT cover copyFrameToBuffer, so finalizing before those joins would race
-    // the staging texture -- do not reorder these.
+    // the staging texture -- do not reorder these. Reaching this line at all
+    // means wgcDrained was true above: the branch that was not is a
+    // TerminateProcess call, not a fallthrough.
     beginStopStep("encoder-finalize", shutdownBudgetMs);
     const bool screenFinalized = encoder.finalize();
     logStopStep("encoder-finalize");

electron/native/wgc-capture/src/mf_encoder.cpp

@@ -406,16 +406,17 @@ bool resolveStreamSinkIndex(IMFMediaSink* mediaSink, const GUID& majorType, DWOR
 
 // Did the video stream's encoder MFT actually land on hardware?
 //
-// BeginWriting() succeeding says nothing about this: on the "default" path
-// (see kVideoEncoderRuntime* in mf_encoder.h) no attribute asked for hardware
-// transforms, so Media Foundation is free to hand the sink writer a software
-// MFT even when a hardware one is registered and would have worked. The only
-// way to know which one it actually picked is to ask the pipeline it built,
-// after the fact -- IMFSinkWriterEx::GetTransformForStream walks the MFTs the
-// sink writer inserted for a stream, and a hardware MFT instance is required
-// to expose MFT_ENUM_HARDWARE_URL_Attribute on its own attribute store (not
-// just on the IMFActivate MFTEnumEx returns), which is what distinguishes it
-// from a software one at this point.
+// BeginWriting() succeeding says nothing about this: even on the "default"
+// path (see kVideoEncoderRuntime* in mf_encoder.h), which does now ask for
+// MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS, that is a request and not a
+// guarantee -- Media Foundation is still free to hand the sink writer a
+// software MFT when no hardware one is registered or the driver refuses it.
+// The only way to know which one it actually picked is to ask the pipeline it
+// built, after the fact -- IMFSinkWriterEx::GetTransformForStream walks the
+// MFTs the sink writer inserted for a stream, and a hardware MFT instance is
+// required to expose MFT_ENUM_HARDWARE_URL_Attribute on its own attribute
+// store (not just on the IMFActivate MFTEnumEx returns), which is what
+// distinguishes it from a software one at this point.
 //
 // Every failure path here returns "unknown" rather than guessing: this runs
 // after the sink writer is already committed to, so it must never be able to