Skip to content

Commit 41c54af

Browse files
authored
🤖 refactor: convert OAuthFlowManager flow lifecycle to Effect per-flow Scope (#4033)
## Summary Phase 5 of the progressive Effect migration (first phase of Wave 2, the wave's gating item deferred since #4027): converts `OAuthFlowManager` internals to Effect with real resource safety. Every registered desktop OAuth flow now owns a per-flow `Scope` whose release finalizers guarantee cleanup (registration-timeout clear, deferred settlement, loopback-server close) on every termination path — finish, cancel, caller-timeout race, duplicate registration, `shutdownAll`, and defects. The Promise-based public API is preserved as thin `Effect.runPromise` facades, so the three not-yet-converted OAuth services (`coderOauthService`, `codexOauthService`, `muxGovernorOauthService`) and all existing tests work unchanged. ## Background Wave 1 (#4022, #4025, #4027, #4028, #4030, #4031, #4032) established the house pattern: `Effect.gen` internals, thin `runPromise` facades, `handlerGen` for oRPC procedures. #4027 converted `muxGatewayOauthService` but explicitly deferred the shared flow-lifecycle manager: its resources (loopback `http.Server`, registration `setTimeout`, result deferred) were cleaned up via ad-hoc `try/catch` + fire-and-forget `void closeServer(...)`, and a defect while resolving the deferred silently skipped the server close. This PR is the acquire/release case that deferral pointed at, and unblocks Phase 6 (batch conversion of the sibling OAuth services). ## Implementation **Per-flow Scope design** — `register` creates a `Scope.makeUnsafe()` per flow and moves ownership of the caller-acquired resources into it via one `Effect.acquireRelease` per resource (a combined acquisition would install its finalizer only after every step succeeded, leaking earlier resources on a later defect — the #4031 Codex P2 lesson). Release runs in reverse acquisition order, preserving the pre-Effect `finish` ordering: clear registration timeout → settle deferred (waiters unblock before the async close) → close loopback server (awaited). **Deferred settlement via finalizer** — each `ActiveFlow` carries a mutable `finalResult` staged by the terminating path (finish/cancel/shutdown/replace); the settle finalizer resolves the caller's deferred with it. Settlement is therefore scope-guaranteed rather than an ad-hoc `resolve` call, with a defensive fallback result so waiters can never hang. **Caller-facing timeout race** — `waitFor` maps to `Effect.timeout` over `Effect.promise` on the shared deferred: the local wait timer is fiber-managed (interruption clears it), stays separate from the registration-time timeout, and on any error result runs `finish` for shared cleanup. The cleanup's synchronous bookkeeping (map removal, completed-result recording) runs before `waitFor` resolves — exact parity with the old sync prefix — while the async release runs in an `Effect.forkDetach` fiber, replacing the old `void this.finish(...)` fire-and-forget with a supervised fiber that survives the caller's completion (verified by a live-runtime probe: detached fibers outlive the parent, `runFork`/`runPromise` execute synchronously to first suspension, and a throwing finalizer does not skip its siblings). **shutdownAll contract** — preserved as async (`Promise<void>` facade): `serviceContainer.dispose` awaits it, and loopback-server closes are bounded by the server's force-finish socket handling. It never rejects; release defects are caught (`Effect.catchDefect`) and logged at debug level, per the startup/shutdown-must-never-crash rule. **Effect-native surface** — `waitForEffect` / `cancelEffect` / `finishEffect` / `cancelAllEffect` / `shutdownAllEffect` are public (wire-shaped, never-failing — same shape as #4032's Effect surfaces). `muxGatewayOauthService`'s Effect pipeline now yields `finishEffect` directly instead of `Effect.promise(() => …finish(...))`, and its registration-timeout callback uses `Effect.runFork(finishEffect(...))` instead of `void finish(...)`. **Not converted to Effect `Deferred`** — the result deferred's identity is part of the public caller-owned `OAuthFlowEntry` (the three unconverted services construct entries with `createDeferred`), so swapping it would break the "existing callers unchanged" contract; revisit when Phase 6 converts entry construction. ## Validation - All 18 pre-existing `oauthFlowManager` tests pass byte-identical, plus all OAuth service suites (194 tests: coder/codex/muxGateway/muxGovernor/mcp/copilot/codexOauthAuth) and loopback-server/oauthUtils suites. - Two new behavioral tests for the genuinely-new guarantees: (1) server close + timeout clear still happen when the deferred `resolve` throws (the pre-Effect code skipped the close — this test fails on the old implementation), and (2) the detached cleanup fiber completes after `waitFor` has already returned on the timeout path (guards against accidental child-fiber supervision, where the release would be interrupted with the caller). - A standalone Effect v4 runtime probe validated the semantics the design relies on (finalizer independence under defects, reverse sequential release order, eager sync-prefix execution of `runPromise`/`runFork`, `forkDetach` outliving the parent, `Effect.timeout` + `Effect.catch` over `Effect.promise`). - `make static-check` green. ## Risks Low-to-moderate: this is shared lifecycle code under four OAuth login flows (Gateway, Governor, Codex, Coder). The public API, observable ordering (map removal before `finish` resolves, deferred settlement before server close, synchronous `register`), and error strings are preserved exactly; regressions would surface as leaked loopback listeners, hung `waitFor` calls, or unsettled deferreds — all covered by the existing + new suites. ## Lessons for Phase 6 Phase 6 is the batch conversion of `coderOauthService`, `codexOauthService`, `muxGovernorOauthService`, `copilotOauthService`, plus their ~20 router sites. Notes to make it mechanical: - The manager now exposes never-failing, wire-shaped `waitForEffect`/`cancelEffect`/`finishEffect`/`shutdownAllEffect`, so converted service pipelines can yield them directly (see `desktopCallbackPipeline` in `muxGatewayOauthService` as the template), and registration-timeout callbacks should use `Effect.runFork(manager.finishEffect(...))`. - `beginFinish`'s sync-bookkeeping/async-release split is the pattern to reach for wherever a service needs "unregister now, release in background" semantics. - Each sibling's `startDesktopFlow` should become uninterruptible like the gateway's (#4032): a client abort between loopback acquisition and `register` would otherwise leak the server. - `coderOauthService` is the outlier: it has extra commit-path liveness checks (`has`) and multi-step persist/commit finish calls (~10 `desktopFlows.*` sites vs ~5 in the others) — expect most of the Phase 6 effort there. - **Recommendation: two PRs.** PR A: codex + governor + copilot service internals (near-identical DesktopFlow shape, mechanical) together with their router procedures moving to `handlerGen` (the `waitFor`/`cancel` handlers for the gateway can join here — the router comment at `muxGatewayOauth` already points at this). PR B: `coderOauthService` alone — its commit/persist liveness semantics deserve isolated review, and a combined PR would bury it under the mechanical churn. --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-fable-5` • Thinking: `xhigh`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh -->
1 parent 1a6db60 commit 41c54af

4 files changed

Lines changed: 331 additions & 104 deletions

File tree

‎src/node/orpc/router.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -579,7 +579,7 @@ export const router = (authToken?: string) => {
579579
// startDesktopFlow rides handlerGen; its service pipeline is
580580
// uninterruptible (see startDesktopFlowEffect) so a client abort cannot
581581
// leak the loopback server. waitFor/cancel stay plain handlers until the
582-
// promise-native OAuthFlowManager grows an Effect surface.
582+
// batch OAuth-service conversion migrates the remaining procedures.
583583
startDesktopFlow: t
584584
.input(schemas.muxGatewayOauth.startDesktopFlow.input)
585585
.output(schemas.muxGatewayOauth.startDesktopFlow.output)

‎src/node/services/muxGatewayOauthService.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,9 @@
1111
* is already the exact user-facing string.
1212
*
1313
* Desktop flow bookkeeping (deferreds, loopback server lifetime, timeouts)
14-
* stays promise-based in `OAuthFlowManager`, which is shared with the other
15-
* OAuth services — converting that lifecycle to Effect `Scope` is deferred
16-
* until the shared manager itself migrates.
14+
* lives in `OAuthFlowManager`, which is shared with the other OAuth services
15+
* and is itself Effect-native (per-flow `Scope` lifecycle) — Effect callers
16+
* here use its `*Effect` surface directly.
1717
*/
1818
import * as crypto from "crypto";
1919
import { Effect, Schema } from "effect";
@@ -295,7 +295,9 @@ export class MuxGatewayOauthService {
295295
// Keep server-side timeout tied to flow lifetime so abandoned flows
296296
// (e.g. callers that never invoke waitForDesktopFlow) still self-clean.
297297
timeoutHandle: setTimeout(() => {
298-
void self.desktopFlows.finish(flowId, Err("Timed out waiting for OAuth callback"));
298+
Effect.runFork(
299+
self.desktopFlows.finishEffect(flowId, Err("Timed out waiting for OAuth callback"))
300+
);
299301
}, DEFAULT_DESKTOP_TIMEOUT_MS),
300302
});
301303

@@ -353,7 +355,7 @@ export class MuxGatewayOauthService {
353355
result = Err(`Xum Gateway OAuth error: ${callbackOrDone.error}`);
354356
}
355357

356-
yield* Effect.promise(() => self.desktopFlows.finish(flowId, result));
358+
yield* self.desktopFlows.finishEffect(flowId, result);
357359
});
358360
}
359361

‎src/node/utils/oauthFlowManager.test.ts‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,74 @@ describe("OAuthFlowManager", () => {
292292
});
293293
});
294294

295+
// -----------------------------------------------------------------------
296+
// guaranteed cleanup (per-flow scope)
297+
// -----------------------------------------------------------------------
298+
299+
describe("guaranteed cleanup", () => {
300+
it("closes the server and clears the timeout even when the deferred resolve throws", async () => {
301+
// Per-flow scope guarantee: each resource has an independent release, so
302+
// a defect while settling the deferred cannot leak the loopback server
303+
// or the registration timeout. (The pre-Effect implementation skipped
304+
// the server close when resolve threw.)
305+
let serverClosed = false;
306+
const mockServer = {
307+
close: (cb?: (err?: Error) => void) => {
308+
serverClosed = true;
309+
if (cb) cb();
310+
return mockServer;
311+
},
312+
} as unknown as http.Server;
313+
314+
let timeoutFired = false;
315+
const entry: OAuthFlowEntry = {
316+
server: mockServer,
317+
resultDeferred: {
318+
promise: createDeferred<Result<void, string>>().promise,
319+
resolve: () => {
320+
throw new Error("resolve exploded");
321+
},
322+
},
323+
timeoutHandle: setTimeout(() => {
324+
timeoutFired = true;
325+
}, 10),
326+
};
327+
manager.register("f1", entry);
328+
329+
// Must not reject despite the resolve defect.
330+
await manager.finish("f1", Ok(undefined));
331+
332+
expect(manager.has("f1")).toBe(false);
333+
expect(serverClosed).toBe(true);
334+
335+
await new Promise((resolve) => setTimeout(resolve, 25));
336+
expect(timeoutFired).toBe(false);
337+
});
338+
339+
it("closes the loopback server after a waitFor timeout even though waitFor already returned", async () => {
340+
// The timeout-path cleanup is fire-and-forget (waitFor resolves without
341+
// waiting for the server close); the detached release fiber must still
342+
// complete after waitFor's own fiber has exited.
343+
let serverClosed = false;
344+
const mockServer = {
345+
close: (cb?: (err?: Error) => void) => {
346+
serverClosed = true;
347+
if (cb) cb();
348+
return mockServer;
349+
},
350+
} as unknown as http.Server;
351+
352+
manager.register("f1", createFlowEntry(mockServer));
353+
354+
const result = await manager.waitFor("f1", 10);
355+
expect(result.success).toBe(false);
356+
expect(manager.has("f1")).toBe(false);
357+
358+
await new Promise((resolve) => setTimeout(resolve, 25));
359+
expect(serverClosed).toBe(true);
360+
});
361+
});
362+
295363
// -----------------------------------------------------------------------
296364
// shutdownAll
297365
// -----------------------------------------------------------------------

0 commit comments

Comments
 (0)