From 44344c93275ba4be878a5bdd4339cb14fe6108ef Mon Sep 17 00:00:00 2001 From: gramps Date: Sun, 27 Sep 2026 16:02:05 -0700 Subject: [PATCH] fix: Change Account signed out BEFORE the browser opened Reported 2026-09-27: click avatar -> Change Account -> cancel in the Google chooser -> back to a signed-out app, then "cannot sign-in because the callback url was not released". Two independent defects: 1. ChangeAccountAsync ran `await SignOutYouTubeAsync(); await SignInAsync();`. SignOutYouTubeAsync -> TokenStore.Clear() -> deletes ytLlive.auth, so the working account's token was destroyed before the browser even opened. It is now just `await SignInAsync()`: success overwrites the token via the sessionChanged hook, cancel/failure keeps the current session. The explicit sign-out was never needed. 2. Abandoning the browser gives the app no signal, so the OAuth attempt held localhost:8765 for its full 5-minute timeout and a retry could not bind. A new AuthenticateAsync now cancels and awaits the abandoned attempt first. That takeover is serialised by a SemaphoreSlim: the first version cleared the in-flight marker before awaiting, so a caller arriving in that window saw a free port and raced to bind it -- reproducing the very failure it was written to prevent. My own test caught that; reasoning had "proved" it fine. Also adds an `openBrowser` ctor seam so tests never shell out to a real browser, and corrects the ai.md line that documented "Logout / Change Account -> SignOutYouTubeAsync" as intended behaviour. Test: YouTubeAuthServiceTests.RetryingAfterAnAbandonedAttempt_ReleasesTheCallbackPort Full suite 370/370, 0 warnings. Coverage gap noted in HANDOFF.md: the ordering fix has no automated test because _youtubeAuth is hardcoded in MainViewModel and ChangeAccountAsync is private. The reported cross-instance effect is NOT this cause and stays open -- no code path can sign out a second instance (per-instance TokenStore, local delete, no server-side revoke). --- HANDOFF.md | 36 +++++++++++++- MyMistakes.md | 57 ++++++++++++++++++++++ Services/YouTubeAuthService.cs | 64 ++++++++++++++++++++++--- ViewModels/MainViewModel.Account.cs | 14 +++++- ai.md | 8 ++-- ytLive.Tests/YouTubeAuthServiceTests.cs | 34 +++++++++++++ 6 files changed, 200 insertions(+), 13 deletions(-) diff --git a/HANDOFF.md b/HANDOFF.md index 526975c..e836907 100644 --- a/HANDOFF.md +++ b/HANDOFF.md @@ -1,8 +1,40 @@ # HANDOFF β€” current state **Branch:** `main` (pre-1.0, no feature branches β€” creator ruling 2026-08-24). -**Last pushed:** `b2036a3`. This unit (reconciling the pricing ruling + verifying the Store -revenue share) is **docs-only** β€” no code touched, no build, no tests run. +**Last pushed:** `e7cded6` (pricing/Store fee docs). **Local, unpushed:** `536676f` (recording +filename ms) and the Change Account fix in this unit β€” ask before pushing. + +## 🚨 FIXED 2026-09-27 β€” "Change Account" no longer signs you out before the browser opens + +**Reported as:** *"I clicked on my signed-in account pic, then clicked sign-in to another account. It +opened the browser page to login under a different account and I canceled the request. But when +returned to the app, I see logged out β€” and should not have happened! Cannot sign-in because the +callback url was not released."* Also reported as hitting **both** running instances. + +**Two independent defects, and the second one was masking a race:** + +1. **The sign-out ran first.** `ChangeAccountAsync` was `await SignOutYouTubeAsync(); await + SignInAsync();`. `SignOutYouTubeAsync` β†’ `TokenStore.Clear()` β†’ **deletes** `ytLlive.auth`. So the + working account's token was destroyed *before the browser even opened*; cancelling the chooser left + nothing. Now `ChangeAccountAsync` is just `await SignInAsync()` β€” success overwrites the token via + the `sessionChanged` hook, cancel/failure keeps the current session. No sign-out is needed at all. +2. **The retry couldn't rebind the callback port.** Abandoning the browser gives the app no signal, so + the attempt held `localhost:8765` for its full 5-minute timeout. A new `AuthenticateAsync` now + cancels and awaits the abandoned attempt before binding, **serialised by a `SemaphoreSlim`** + (`_authGate`) β€” without the gate, the takeover window let a third caller see a free port and race + to bind it, i.e. the takeover reproduced the original failure. My own test caught this. + +**The cross-instance claim was NOT the cause** and is still open as a *separate* question: no code path +can sign out a second instance (`TokenStore` is per-`InstanceProfile.DataRoot`, `Clear()` is a local +file delete with no server-side revoke, and no second instance had a token at inspection time). Verify +both processes carried distinct `YTLIVE_INSTANCE` values and ran Debug builds before chasing it. + +Test: `YouTubeAuthServiceTests.RetryingAfterAnAbandonedAttempt_ReleasesTheCallbackPort`. Full suite +**370/370**, 0 warnings (build `ytLive.Tests.csproj`, not just the app project, or VSTest runs a stale +test DLL). Added an `openBrowser` ctor seam to `YouTubeAuthService` so tests never launch a real +browser. **Coverage gap:** the *ordering* fix (defect 1) has no automated test β€” `_youtubeAuth` is +hardcoded in `MainViewModel` and `ChangeAccountAsync` is private, so a real test would shell out to +the browser. Wiring an auth seam into `MainViewModel` is a deliberate follow-up, not a bug-fix extra. ## βœ… DECIDED 2026-09-27 β€” pricing: `$10/mo` subscription, or `$400` lifetime, **no annual** diff --git a/MyMistakes.md b/MyMistakes.md index 40df0c0..a7223ee 100644 --- a/MyMistakes.md +++ b/MyMistakes.md @@ -1177,3 +1177,60 @@ well-formed, unverified* claim got to speak without a check β€” a dead EV-certif policy number with no scope, a refund lever I did not control. **Sound reasoning about a mechanism I had not verified is the failure mode, not the safety net.** Before any external mechanism drives a decision, name the one fact that would falsify it, then go get that fact. + +## 2026-09-27 β€” "Change Account" signed out BEFORE the browser opened: destroying state you cannot get back + +**The symptom:** the creator clicked their avatar β†’ Change Account, cancelled in the Google account +chooser, came back **signed out of a perfectly working account**, and then could not sign in again +("the callback url was not released"). They also observed it hitting *both* running instances. + +**The cause, and it is embarrassing in its simplicity.** `ChangeAccountAsync` was: + +```csharp +await SignOutYouTubeAsync(); // β†’ TokenStore.Clear() β†’ File.Delete(ytLlive.auth) +await SignInAsync(); // the browser has not even opened yet +``` + +The sign-out ran **first**, before any possibility of success, and `SignOutYouTubeAsync` does not +"log out" β€” it **deletes the DPAPI token file**. So the moment the creator committed to the intent of +switching accounts, the old account was already unrecoverable. Cancelling the browser flow then had +nothing to fall back to. The explicit sign-out was not merely redundant, it was the bug: success +already overwrites the token through the `sessionChanged β†’ TokenStore.Save` hook. + +**The transferable rule β€” order of operations on unrecoverable state:** + +> **Never destroy the working state before the replacement is secured. Authenticate first, then +> replace.** If the new value can fail, cancel, or be abandoned, the old value is the *only* thing +> standing between the user and a re-authentication loop β€” so it must outlive the attempt, not +> precede it. + +The tell for this class of bug: a call named for an *intent* ("Change Account") that internally +performs a *destructive reset* before doing the requested work. "Change" implies Aβ†’B, and the naive +implementation of Aβ†’B is `Reset(); Set(B)`. The safe one is `B = Compute(); if (B != null) Commit(B)`. + +**The second defect was hiding behind the first, and the fix for it was also a race.** Abandoning the +browser gives the app no signal, so the OAuth attempt just sat on its loopback port `localhost:8765` +for the full 5-minute timeout; a retry could not bind and reported the callback URL as "not released". +Cancel-and-await-the-previous fixed the serial case, but my first version still had a window: +`Interlocked.Exchange(ref _authInFlight, null)` nulls the field *before* awaiting the old attempt, so +a third caller arriving in that window sees "nothing in flight" and races to bind the same port +anyway β€” **the takeover reintroduced the exact failure it was written to prevent.** A `SemaphoreSlim` +around cancelβ†’awaitβ†’bind made it atomic. My own test caught this; reasoning alone had "proved" it +correct. The generalisation: **when you serialise a handover, the whole handover is one critical +section β€” clearing the "who is doing this" marker early is the same bug as not having the lock.** + +**Also worth recording: the cross-instance part of the report was NOT the cause.** The creator +believed one instance's sign-out hit both. It could not have β€” `TokenStore.DefaultPath` resolves +through `InstanceProfile.DataRoot` and `Clear()` only deletes a local file, never a server-side revoke, +and no second instance had a token to clear. The two claims were independent, and believing they were +linked would have sent me to debug the wrong subsystem. **One report can carry two unrelated defects; +split the report into claims and check each against the code before building a theory on it.** + +### Known timing flake: `NotificationAreaIntegrationTests.Info_Auto_Dismisses_And_Error_Sticks` + +Failed once on 2026-09-27 with "info toast never auto-dismissed" after an 8s wait, then passed in +isolation and on a clean full-suite run. It is a real-time WPF toast timer, so it loses under +parallel load β€” **it is not a signal that your change broke notifications.** Before believing it, +re-run it alone (`vstest --Tests:Info_Auto_Dismisses_And_Error_Sticks`); only treat it as yours if it +fails in isolation too. See also the two other suite-level timing traps already recorded above +(the one-tick pipe read, and blocking waits on the shared `RealApp` STA thread). diff --git a/Services/YouTubeAuthService.cs b/Services/YouTubeAuthService.cs index 00ce8b2..f12f2d3 100644 --- a/Services/YouTubeAuthService.cs +++ b/Services/YouTubeAuthService.cs @@ -21,16 +21,36 @@ public class YouTubeAuthService private readonly string _clientSecret; private readonly HttpClient _http; private readonly Action? _sessionChanged; + private readonly Action _openBrowser; + + /// Serialises "take over from the previous attempt". Without this, two + /// overlapping calls both see a free port and race to bind it, which is the same + /// failure the cancel-and-await is meant to prevent. + private readonly SemaphoreSlim _authGate = new(1, 1); + + /// The attempt currently holding the loopback callback port, if any. + /// A creator who abandons the Google account chooser gives the app no signal, so + /// the attempt simply waits out its 5-minute timeout while still holding port + /// 8765 β€” and a retry in that window cannot bind. A new attempt cancels and + /// awaits the abandoned one first, which releases the port (creator report + /// 2026-09-27: "cannot sign-in because the callback url was not released"). + /// Both fields below are only touched while holding . + private Task? _authInFlight; + private CancellationTokenSource? _authCts; public YouTubeChannel? CurrentChannel { get; private set; } public YouTubeAuthService(string clientId, string clientSecret, HttpClient? http = null, - Action? sessionChanged = null) + Action? sessionChanged = null, + Action? openBrowser = null) { _clientId = clientId; _clientSecret = clientSecret; _http = http ?? new HttpClient(); _sessionChanged = sessionChanged; + // Seam: the real flow shells out to the default browser, which a test must not do. + _openBrowser = openBrowser ?? (url => + Process.Start(new ProcessStartInfo(url) { UseShellExecute = true })); } /// Replaces the current session (used when a saved token loads at startup). @@ -65,23 +85,53 @@ public class YouTubeAuthService public async Task AuthenticateAsync() { - const int port = 8765; - var redirectUri = $"http://localhost:{port}/oauth2/callback"; - if (string.IsNullOrWhiteSpace(_clientId) || string.IsNullOrWhiteSpace(_clientSecret)) return null; + // Taking over from an abandoned attempt has to be atomic: cancel it, wait for + // it to actually let go of the port, then bind. Letting the gate go before the + // bind reopens the race the takeover exists to close. + Task attempt; + await _authGate.WaitAsync().ConfigureAwait(false); + try + { + var previousCts = _authCts; + _authCts = null; + previousCts?.Cancel(); + + if (_authInFlight is { } previous) + { + try { await previous.ConfigureAwait(false); } catch { /* its result is irrelevant */ } + _authInFlight = null; + } + + var cts = new CancellationTokenSource(TimeSpan.FromMinutes(5)); + _authCts = cts; + _authInFlight = attempt = RunAuthFlowAsync(cts.Token); + } + finally + { + _authGate.Release(); + } + + return await attempt.ConfigureAwait(false); + } + + private async Task RunAuthFlowAsync(CancellationToken cancellationToken) + { + const int port = 8765; + var redirectUri = $"http://localhost:{port}/oauth2/callback"; + using var listener = new HttpListener(); listener.Prefixes.Add($"{redirectUri}/"); listener.Start(); - Process.Start(new ProcessStartInfo(GetAuthorizationUrl(redirectUri)) { UseShellExecute = true }); + _openBrowser(GetAuthorizationUrl(redirectUri)); HttpListenerContext context; try { - using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(5)); - context = await listener.GetContextAsync().WaitAsync(cts.Token); + context = await listener.GetContextAsync().WaitAsync(cancellationToken); } catch (OperationCanceledException) { diff --git a/ViewModels/MainViewModel.Account.cs b/ViewModels/MainViewModel.Account.cs index f400fac..a668e47 100644 --- a/ViewModels/MainViewModel.Account.cs +++ b/ViewModels/MainViewModel.Account.cs @@ -126,9 +126,21 @@ public partial class MainViewModel await Task.CompletedTask; } + /// Change account: sign in to a different YouTube account WITHOUT + /// destroying the current session first. + /// The bug this fixes (2026-09-27). This used to be + /// await SignOutYouTubeAsync(); await SignInAsync(); β€” it signed out + /// before opening the browser, and SignOutYouTubeAsync calls + /// TokenStore.Clear(), which DELETES the saved token file. The creator + /// clicked "Change Account", cancelled in the Google account chooser, and came + /// back to an app that had already thrown away a working, signed-in account. + /// + /// No sign-out is needed at all: on success + /// replaces the session and the sessionChanged hook overwrites the token + /// file with the new account. On cancel or failure it returns null and leaves + /// the current session untouched β€” which is the behaviour that was wanted. private async Task ChangeAccountAsync() { - await SignOutYouTubeAsync(); await SignInAsync(); } } \ No newline at end of file diff --git a/ai.md b/ai.md index 702ebc1..c4ff3d4 100644 --- a/ai.md +++ b/ai.md @@ -229,7 +229,7 @@ The AI hallucinated through multiple commits that night on background/scene prop ### Current limitations / TODOs -- `Helpers/OAuthCredentials.cs` contains the real ClientId/ClientSecret. Auth is complete and the session **persists via Windows DPAPI** (`Helpers/TokenStore.cs` β†’ `%APPDATA%\ytLlive\ytLlive.auth`, CurrentUser scope), reloaded best-effort at startup with a proactive refresh of a near-expiry access token. Sign-in/Change Account lives **inside the Start Stream dialog** (two-state flow β€” no separate Connect button). Sign-out is **explicit only** (Logout / Change Account β†’ `SignOutYouTubeAsync`): `StopStream()` does NOT clear the session β€” TASK 18's 2026-08-29 reversal ("stopping a recording leaves the creator signed in") β€” the older "graceful End signs out" line here was stale and is corrected against the code (2026-09-01). `YouTubeAuthService` takes an optional `HttpClient` + `sessionChanged` callback (test seam + save hook; services are still constructed in `MainViewModel`) +- `Helpers/OAuthCredentials.cs` contains the real ClientId/ClientSecret. Auth is complete and the session **persists via Windows DPAPI** (`Helpers/TokenStore.cs` β†’ `%APPDATA%\ytLlive\ytLlive.auth`, CurrentUser scope), reloaded best-effort at startup with a proactive refresh of a near-expiry access token. Sign-in/Change Account lives **inside the Start Stream dialog** (two-state flow β€” no separate Connect button). Sign-out is **explicit only**, and that means **Logout β€” NOT Change Account**: `StopStream()` does NOT clear the session β€” TASK 18's 2026-08-29 reversal ("stopping a recording leaves the creator signed in") β€” the older "graceful End signs out" line here was stale and is corrected against the code (2026-09-01). `YouTubeAuthService` takes an optional `HttpClient` + `sessionChanged` + `openBrowser` callback (test seams + save hook; services are still constructed in `MainViewModel`). **Change Account must never sign out first** (fixed 2026-09-27): it used to `await SignOutYouTubeAsync(); await SignInAsync();`, and since `SignOutYouTubeAsync` β†’ `TokenStore.Clear()` **deletes** `ytLlive.auth`, cancelling in the Google chooser left the creator logged out of a working account. It is now just `await SignInAsync()` β€” success overwrites the token via the `sessionChanged` hook, cancel/failure leaves the session untouched. The OAuth flow also **releases its loopback callback port on retry**: abandoning the browser leaves the attempt holding `localhost:8765` for its 5-minute timeout, so a new `AuthenticateAsync` cancels and awaits the abandoned attempt before binding, serialised by a `SemaphoreSlim` (`_authGate`) β€” without that gate, two overlapping calls both see a free port and race to bind it (the exact failure the takeover prevents) - Scene/source/asset layout + the social bar persist (SQLite, schema v8); the OAuth session persists (DPAPI); the paid-unlock state persists (LayoutStore Settings table β€” `LicenseKey`/`LicenseValidatedAt`/`IsPremium`, 14-day offline grace) - `YouTubeStreamService` manages the **variable reusable stream** (shipped 2026-08-16): `GetOrCreateReusableStreamAsync` lists `liveStreams?mine=true` and reuses the existing `cdn.isReusable` stream, creating it only on first use (`resolution=variable`, `frameRate=variable`); the stream is cached via `LayoutStore` (`SaveReusableStream`/`LoadReusableStream`, Settings table) and bound at broadcast insert (`boundStreamId`). Health (shipped 2026-08-16): `GetStreamHealthAsync(streamId)` polls `liveStreams?part=status` for `healthStatus` + `configurationIssues[]` β†’ `StreamHealth`; the real API nests them under `status.healthStatus = {status, lastUpdateTimeSeconds, configurationIssues[]}` β€” the parse accepts that object shape and the legacy flat-string shape (fix 2026-09-22: the object shape crashed every poll with `requires an element of type 'String'`); banner decision in pure `StreamHealthReporter` - Webcam capture is shipped (milestone 1); the live desktop/game backdrop is shipped (ship task #1); **the output compositor (TASK 4 ship step 1) is SHIPPED**, **the FFmpeg locator (TASK 4 ship step 2) is SHIPPED**, **the encoder + RTMP push (TASK 4 ship step 3) is SHIPPED**, **WASAPI audio capture (TASK 4 ship step 4) is SHIPPED**, **frame-pipeline wiring (TASK 4 ship step 5) is SHIPPED**, **health stats (TASK 4 ship step 6) is SHIPPED**, **one-click go-live + private-only enforcement (TASK 4 ship step 7) is SHIPPED** β€” TASK 4 (RTMP Ingest) is fully done; full plan in `TASKS.md` @@ -741,8 +741,10 @@ lights when a session is actually running. branch handled explicitly β€” a falsy result must never fall through into the effect.** - **Folder:** default `%APPDATA%\ytLlive\recordings\`, user-overridable via `ChooseRecordFolderCommand` (`OpenFolderDialog`), persisted through `LayoutStore.Load/SaveRecordFolder` (`RecordFolder` key). -- **Explicit sign-out only:** `StopStream` no longer clears the session/token. Sign out via Logout / - Change Account. Stopping a recording leaves the creator signed in. +- **Explicit sign-out only:** `StopStream` no longer clears the session/token. Sign out via **Logout + only** β€” Change Account re-authenticates *in place* and deliberately does not sign out first + (2026-09-27; signing out first deleted `ytLlive.auth` before the browser even opened, so a + cancelled switch lost a working session). Stopping a recording leaves the creator signed in. - **Stop ends everything; failures roll back (2026-09-01):** `StopStream` also clears both intent pills (`RecordPillOn`/`OnAirPillOn`) β€” a lit pill with no session behind it is a lie. Every frame-pump death and every go-live prep failure now runs the full `StopStream` teardown instead of diff --git a/ytLive.Tests/YouTubeAuthServiceTests.cs b/ytLive.Tests/YouTubeAuthServiceTests.cs index aa1e88c..7258340 100644 --- a/ytLive.Tests/YouTubeAuthServiceTests.cs +++ b/ytLive.Tests/YouTubeAuthServiceTests.cs @@ -83,4 +83,38 @@ public class YouTubeAuthServiceTests Assert.Null(service.CurrentChannel); } + + /// Creator report 2026-09-27: clicking "Change Account", cancelling in the + /// Google account chooser, then clicking again failed with "cannot sign-in because + /// the callback url was not released". Abandoning the browser gives the app no + /// signal, so the first attempt sat on the loopback callback port for its full + /// 5-minute timeout and the second attempt could not bind. + /// Requires nothing else to be listening on port 8765 β€” close a running + /// LlamaCasty that happens to be mid-sign-in, or this test sees a genuine + /// "address already in use" from the host. + [Fact] + public async Task RetryingAfterAnAbandonedAttempt_ReleasesTheCallbackPort() + { + var service = new YouTubeAuthService( + "test-id", "test-secret", + new HttpClient(new FakeHandler("{}", "{}")), + openBrowser: _ => { }); // never shell out to a real browser from a test + + var first = service.AuthenticateAsync(); + + // The creator gives up on the chooser and clicks "Change Account" again. + var second = service.AuthenticateAsync(); + + // The abandoned attempt must be released promptly rather than left holding the + // port for its whole 5-minute timeout. If it weren't, this await would time out. + Assert.Null(await first.WaitAsync(TimeSpan.FromSeconds(15))); + + // Taking over again proves `second` really did bind the port: had it not, + // awaiting it would throw HttpListenerException ("address already in use"). + _ = service.AuthenticateAsync(); + Assert.Null(await second.WaitAsync(TimeSpan.FromSeconds(15))); + + // The last attempt is deliberately left pending β€” nothing can cancel it and the + // test host exits before its 5-minute timer fires. + } }