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).
This commit is contained in:
+34
-2
@@ -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**
|
||||
|
||||
|
||||
@@ -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).
|
||||
|
||||
@@ -21,16 +21,36 @@ public class YouTubeAuthService
|
||||
private readonly string _clientSecret;
|
||||
private readonly HttpClient _http;
|
||||
private readonly Action<YouTubeChannel>? _sessionChanged;
|
||||
private readonly Action<string> _openBrowser;
|
||||
|
||||
/// <summary>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.</summary>
|
||||
private readonly SemaphoreSlim _authGate = new(1, 1);
|
||||
|
||||
/// <summary>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 <see cref="_authGate"/>.</summary>
|
||||
private Task<YouTubeChannel?>? _authInFlight;
|
||||
private CancellationTokenSource? _authCts;
|
||||
|
||||
public YouTubeChannel? CurrentChannel { get; private set; }
|
||||
|
||||
public YouTubeAuthService(string clientId, string clientSecret, HttpClient? http = null,
|
||||
Action<YouTubeChannel>? sessionChanged = null)
|
||||
Action<YouTubeChannel>? sessionChanged = null,
|
||||
Action<string>? 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 }));
|
||||
}
|
||||
|
||||
/// <summary>Replaces the current session (used when a saved token loads at startup).</summary>
|
||||
@@ -65,23 +85,53 @@ public class YouTubeAuthService
|
||||
|
||||
public async Task<YouTubeChannel?> 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<YouTubeChannel?> 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<YouTubeChannel?> 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)
|
||||
{
|
||||
|
||||
@@ -126,9 +126,21 @@ public partial class MainViewModel
|
||||
await Task.CompletedTask;
|
||||
}
|
||||
|
||||
/// <summary>Change account: sign in to a different YouTube account WITHOUT
|
||||
/// destroying the current session first.
|
||||
/// <para><b>The bug this fixes (2026-09-27).</b> This used to be
|
||||
/// <c>await SignOutYouTubeAsync(); await SignInAsync();</c> — it signed out
|
||||
/// <i>before</i> opening the browser, and <c>SignOutYouTubeAsync</c> calls
|
||||
/// <c>TokenStore.Clear()</c>, 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.
|
||||
/// </para>
|
||||
/// <para>No sign-out is needed at all: on success <see cref="SignInAsync"/>
|
||||
/// replaces the session and the <c>sessionChanged</c> 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.</para></summary>
|
||||
private async Task ChangeAccountAsync()
|
||||
{
|
||||
await SignOutYouTubeAsync();
|
||||
await SignInAsync();
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -83,4 +83,38 @@ public class YouTubeAuthServiceTests
|
||||
|
||||
Assert.Null(service.CurrentChannel);
|
||||
}
|
||||
|
||||
/// <summary>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.
|
||||
/// <para>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.</para></summary>
|
||||
[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.
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user