From ab9ad00748b7d94d85c54d949db01b73bd8326e8 Mon Sep 17 00:00:00 2001 From: 4gray Date: Mon, 3 Aug 2026 08:41:44 +0200 Subject: [PATCH] fix(stalker): budget the four-request login flow and drain abandoned attempts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two timing defects in the discovery/auth interaction: - The auth budget was written for "two sequential requests", but a status-2 portal costs FOUR — handshake, profile, do_auth, profile retry — and the Electron transport allows each 15 s. A valid but slow login-required portal was aborted before its final profile and reported as auth-rejected. - Aborting a timed-out attempt stops it from SENDING further requests, but a `get_profile` already on the wire is processed regardless, and discovery advanced immediately — so the abandoned attempt could adopt the MAC's token after the next candidate had negotiated its own, invalidating a portal that actually works. The run now drains the abandoned attempt (bounded by one request budget) instead of racing it. The second corrects a claim this branch made in the docs: the in-flight window was described as unclosable from the client. The request cannot be un-sent, but nothing forces us to have a competing session in flight while it lands. Co-Authored-By: Claude Fable 5 --- docs/architecture/stalker-portal.md | 11 ++++ .../stalker-portal-discovery.service.spec.ts | 60 ++++++++++++++++++- .../lib/stalker-portal-discovery.service.ts | 51 ++++++++++++---- 3 files changed, 110 insertions(+), 12 deletions(-) diff --git a/docs/architecture/stalker-portal.md b/docs/architecture/stalker-portal.md index 613a75bf1..876581b61 100644 --- a/docs/architecture/stalker-portal.md +++ b/docs/architecture/stalker-portal.md @@ -361,6 +361,17 @@ regardless of what the client does, so tearing the socket down would not prevent the adoption. Only not sending the request does, which is exactly what the between-calls check guarantees. +That leaves the window where the timer fires while a `get_profile` is already +dispatched — which the check cannot cover, but sequencing can: discovery +**drains** the abandoned attempt (bounded by one request budget) before probing +the next candidate, instead of racing it. The request cannot be un-sent, but +nothing forces us to have a competing session in flight while it lands. + +The budget itself covers the longest real flow: a status-2 portal costs four +sequential requests (handshake, profile, `do_auth`, profile retry) and the +Electron transport allows each 15 s, so a two-request budget would have failed +valid but slow login portals. + ### Watchdog The portal expects `get_events` every `watchdog_timeout` seconds — **120 by diff --git a/libs/portal/stalker/data-access/src/lib/stalker-portal-discovery.service.spec.ts b/libs/portal/stalker/data-access/src/lib/stalker-portal-discovery.service.spec.ts index 29907b88d..44bc89939 100644 --- a/libs/portal/stalker/data-access/src/lib/stalker-portal-discovery.service.spec.ts +++ b/libs/portal/stalker/data-access/src/lib/stalker-portal-discovery.service.spec.ts @@ -379,12 +379,17 @@ describe('StalkerPortalDiscoveryService', () => { await Promise.resolve(); await Promise.resolve(); - jest.advanceTimersByTime(45_000); + jest.advanceTimersByTime(65_000); await Promise.resolve(); // The abandoned attempt is cancelled, so its get_profile never // goes out to adopt the MAC's token behind a later candidate. expect(captured?.aborted).toBe(true); + + // …and discovery does not RACE it: a request already dispatched + // cannot be un-sent, so the run drains it (bounded) instead of + // probing the next candidate while it may still land. + jest.advanceTimersByTime(15_000); // Outcome shape is unchanged: a timed-out confirmation is still // reported as a refusal of that endpoint. await expect(discovery).resolves.toMatchObject({ @@ -395,4 +400,57 @@ describe('StalkerPortalDiscoveryService', () => { jest.useRealTimers(); } }); + + it('drains an abandoned attempt instead of probing the next candidate', async () => { + jest.useFakeTimers(); + try { + // Two candidates both answer "auth required", so a resolved first + // attempt would stop the run — only a timed-out one advances. + mockProbes({ + 'http://slow.example/portal.php': { + resolve: 'Authorization failed.', + }, + 'http://slow.example/server/load.php': { + resolve: 'Authorization failed.', + }, + }); + + let settleFirst: (() => void) | undefined; + authenticate + .mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + settleFirst = () => reject(new Error('late')); + }) + ) + .mockResolvedValue({ token: 'SECOND' }); + + const discovery = service.discover('http://slow.example/c', MAC); + for (let i = 0; i < 6; i += 1) { + await Promise.resolve(); + } + + jest.advanceTimersByTime(65_000); + for (let i = 0; i < 6; i += 1) { + await Promise.resolve(); + } + + // The first attempt is still on the wire: the second candidate + // must NOT have been authenticated yet, or its token could be + // invalidated by the first one's late get_profile. + expect(authenticate).toHaveBeenCalledTimes(1); + + // Once it settles, the run continues immediately. + settleFirst?.(); + for (let i = 0; i < 10; i += 1) { + await Promise.resolve(); + } + expect(authenticate).toHaveBeenCalledTimes(2); + await expect(discovery).resolves.toMatchObject({ + status: 'resolved', + }); + } finally { + jest.useRealTimers(); + } + }); }); diff --git a/libs/portal/stalker/data-access/src/lib/stalker-portal-discovery.service.ts b/libs/portal/stalker/data-access/src/lib/stalker-portal-discovery.service.ts index 7baf5c2de..5a028efc5 100644 --- a/libs/portal/stalker/data-access/src/lib/stalker-portal-discovery.service.ts +++ b/libs/portal/stalker/data-access/src/lib/stalker-portal-discovery.service.ts @@ -59,8 +59,27 @@ export type StalkerPortalDiscoveryOutcome = /** Per-request guard so a hanging host cannot stall discovery forever. */ const PROBE_TIMEOUT_MS = 20_000; -/** authenticate() is two sequential requests; give it a matching budget. */ -const AUTH_TIMEOUT_MS = 45_000; +/** + * `authenticate()` is up to FOUR sequential requests once a portal answers + * `get_profile` with status 2 — handshake, profile, `do_auth`, profile retry — + * and the Electron transport allows each non-`create_link` call 15 s. A budget + * that only covered two would abort a valid but slow login-required portal + * before its final profile and report it as `auth-rejected`. + */ +const AUTH_TIMEOUT_MS = 4 * 15_000 + 5_000; + +/** + * How long to wait for an abandoned attempt to settle before probing the next + * candidate. + * + * Aborting cannot un-send a request: if its `get_profile` was already + * dispatched, the portal adopts that token regardless of what the client does + * to its socket. What we CAN do is refuse to race it — advancing while it is + * still in flight is what lets it invalidate the token the next candidate + * negotiates. Bounded by one request budget so a genuinely hung host cannot + * stall discovery forever; past that the risk is accepted rather than hanging. + */ +const ABANDONED_DRAIN_MS = 15_000; function withTimeout( promise: Promise, @@ -212,16 +231,16 @@ export class StalkerPortalDiscoveryService { // portal call, so a timed-out attempt never sends the `get_profile` // that would adopt the MAC's token behind the next candidate's back. const abandon = new AbortController(); + // Kept so a timed-out attempt can be drained rather than raced. + const pending = this.stalkerSession.authenticate( + candidate, + macAddress, + identity, + { credentials, signal: abandon.signal } + ); try { - const auth = await withTimeout( - this.stalkerSession.authenticate( - candidate, - macAddress, - identity, - { credentials, signal: abandon.signal } - ), - AUTH_TIMEOUT_MS, - () => abandon.abort() + const auth = await withTimeout(pending, AUTH_TIMEOUT_MS, () => + abandon.abort() ); // A handshake can hand out a token whose `get_profile` still // answers a structured denial (`{js:{error:'Invalid token'}}`); @@ -245,6 +264,16 @@ export class StalkerPortalDiscoveryService { timeslotSeconds: auth.timeslotSeconds, }; } catch (error) { + // Do not advance while the abandoned attempt may still be on the + // wire: its `get_profile` adopts the MAC's token portal-side, so + // racing it is exactly what invalidates the next candidate's + // freshly issued session. + await Promise.race([ + pending.catch(() => undefined), + new Promise((resolve) => + setTimeout(resolve, ABANDONED_DRAIN_MS) + ), + ]); return { status: 'auth-rejected', portalUrl: candidate,