From 75dbfa4185e404d6d5641034ddb4c93da0c13d87 Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 12:52:05 -0600 Subject: [PATCH 01/12] Better handling of resumeSession errors --- packages/api/src/agent.ts | 23 +++++++++++++++-------- packages/api/src/types.ts | 7 ++++++- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/packages/api/src/agent.ts b/packages/api/src/agent.ts index aea3cce9d4b..f9ac6c54ca3 100644 --- a/packages/api/src/agent.ts +++ b/packages/api/src/agent.ts @@ -1,5 +1,5 @@ import { ErrorResponseBody, errorResponseBody } from '@atproto/xrpc' -import { defaultFetchHandler } from '@atproto/xrpc' +import { defaultFetchHandler, XRPCError, ResponseType } from '@atproto/xrpc' import { isValidDidDoc, getPdsEndpoint } from '@atproto/common-web' import { AtpBaseClient, @@ -159,23 +159,30 @@ export class AtpAgent { try { this.session = session const res = await this.api.com.atproto.server.getSession() - if (!res.success || res.data.did !== this.session.did) { - throw new Error('Invalid session') + if (res.data.did !== this.session.did) { + throw new XRPCError(ResponseType.InvalidRequest, 'Invalid session', 'InvalidDID') } this.session.email = res.data.email this.session.handle = res.data.handle this.session.emailConfirmed = res.data.emailConfirmed this._updateApiEndpoint(res.data.didDoc) + this._persistSession?.('create', this.session) return res } catch (e) { this.session = undefined - throw e - } finally { - if (this.session) { - this._persistSession?.('create', this.session) + + if (e instanceof XRPCError && e.error) { + // `ExpiredToken` and `InvalidToken` are handled by the `this._refreshSession` + if (['AuthMissing', 'InvalidDID'].includes(e.error)) { + this._persistSession?.('expired', undefined) + } else { + this._persistSession?.('network-error', undefined) + } } else { - this._persistSession?.('create-failed', undefined) + this._persistSession?.('network-error', undefined) } + + throw e } } diff --git a/packages/api/src/types.ts b/packages/api/src/types.ts index c0f78bfaafc..7b462c0b7e8 100644 --- a/packages/api/src/types.ts +++ b/packages/api/src/types.ts @@ -3,7 +3,12 @@ import { LabelPreference } from './moderation/types' /** * Used by the PersistSessionHandler to indicate what change occurred */ -export type AtpSessionEvent = 'create' | 'create-failed' | 'update' | 'expired' +export type AtpSessionEvent = + | 'create' + | 'create-failed' + | 'update' + | 'expired' + | 'network-error' /** * Used by AtpAgent to store active sessions From 1d0345514f06828feabdf28262ae58404e06fc94 Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 12:53:09 -0600 Subject: [PATCH 02/12] Update test --- packages/api/tests/agent.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/api/tests/agent.test.ts b/packages/api/tests/agent.test.ts index 807046deb00..45a7b84f02a 100644 --- a/packages/api/tests/agent.test.ts +++ b/packages/api/tests/agent.test.ts @@ -343,7 +343,7 @@ describe('agent', () => { expect(events.length).toEqual(2) expect(events[0]).toEqual('create-failed') - expect(events[1]).toEqual('create-failed') + expect(events[1]).toEqual('network-error') expect(sessions.length).toEqual(2) expect(typeof sessions[0]).toEqual('undefined') expect(typeof sessions[1]).toEqual('undefined') From 48d0b35843252322826af37df986b492739807da Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 12:58:17 -0600 Subject: [PATCH 03/12] Format --- packages/api/src/agent.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/api/src/agent.ts b/packages/api/src/agent.ts index f9ac6c54ca3..ebbc16723e8 100644 --- a/packages/api/src/agent.ts +++ b/packages/api/src/agent.ts @@ -160,7 +160,11 @@ export class AtpAgent { this.session = session const res = await this.api.com.atproto.server.getSession() if (res.data.did !== this.session.did) { - throw new XRPCError(ResponseType.InvalidRequest, 'Invalid session', 'InvalidDID') + throw new XRPCError( + ResponseType.InvalidRequest, + 'Invalid session', + 'InvalidDID', + ) } this.session.email = res.data.email this.session.handle = res.data.handle From 4bfa5ee6da84e1bff2dc606e9b57cd265ee4ec47 Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 14:14:47 -0600 Subject: [PATCH 04/12] Pare back to only necessary --- packages/api/src/agent.ts | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/api/src/agent.ts b/packages/api/src/agent.ts index ebbc16723e8..06cce7f5996 100644 --- a/packages/api/src/agent.ts +++ b/packages/api/src/agent.ts @@ -176,11 +176,20 @@ export class AtpAgent { this.session = undefined if (e instanceof XRPCError && e.error) { - // `ExpiredToken` and `InvalidToken` are handled by the `this._refreshSession` - if (['AuthMissing', 'InvalidDID'].includes(e.error)) { + /* + * `ExpiredToken` and `InvalidToken` are handled in + * `this_refreshSession`, and emit an `expired` event there. + * + * `AuthMissing` is a respose from `getSession`, and happens AFTER the + * initial `expired` event is sent. If we handle that here, we'll + * double-emit `expired`. + * + * `InvalidDID` is emit above, and should be handled here. + * + * Everything else is unexpected. + */ + if (['InvalidDID'].includes(e.error)) { this._persistSession?.('expired', undefined) - } else { - this._persistSession?.('network-error', undefined) } } else { this._persistSession?.('network-error', undefined) From c42140bed009f2a905a3500e19c8f514056f17f4 Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 14:25:01 -0600 Subject: [PATCH 05/12] Update handling for 500s --- packages/api/src/agent.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/api/src/agent.ts b/packages/api/src/agent.ts index 06cce7f5996..e8b65b651c1 100644 --- a/packages/api/src/agent.ts +++ b/packages/api/src/agent.ts @@ -190,6 +190,8 @@ export class AtpAgent { */ if (['InvalidDID'].includes(e.error)) { this._persistSession?.('expired', undefined) + } else if (['InternalServerError'].includes(e.error)) { + this._persistSession?.('network-error', undefined) } } else { this._persistSession?.('network-error', undefined) From acc9c28e5c304ffe5ec12c1bfa42fa6d33259bbd Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 14:30:17 -0600 Subject: [PATCH 06/12] Should really be update --- packages/api/src/agent.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/api/src/agent.ts b/packages/api/src/agent.ts index e8b65b651c1..2a788dfe3aa 100644 --- a/packages/api/src/agent.ts +++ b/packages/api/src/agent.ts @@ -170,7 +170,7 @@ export class AtpAgent { this.session.handle = res.data.handle this.session.emailConfirmed = res.data.emailConfirmed this._updateApiEndpoint(res.data.didDoc) - this._persistSession?.('create', this.session) + this._persistSession?.('update', this.session) return res } catch (e) { this.session = undefined From 595f3982881cc9d3221e6a2a12d79f03a4413f38 Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 15:28:59 -0600 Subject: [PATCH 07/12] Update logic from feedback --- packages/api/src/agent.ts | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/packages/api/src/agent.ts b/packages/api/src/agent.ts index 2a788dfe3aa..5620e3df6a8 100644 --- a/packages/api/src/agent.ts +++ b/packages/api/src/agent.ts @@ -180,18 +180,14 @@ export class AtpAgent { * `ExpiredToken` and `InvalidToken` are handled in * `this_refreshSession`, and emit an `expired` event there. * - * `AuthMissing` is a respose from `getSession`, and happens AFTER the - * initial `expired` event is sent. If we handle that here, we'll - * double-emit `expired`. - * - * `InvalidDID` is emit above, and should be handled here. - * - * Everything else is unexpected. + * Everything else is handled here. */ - if (['InvalidDID'].includes(e.error)) { - this._persistSession?.('expired', undefined) - } else if (['InternalServerError'].includes(e.error)) { + if ( + [1, 408, 425, 429, 500, 502, 503, 504, 522, 524].includes(e.status) + ) { this._persistSession?.('network-error', undefined) + } else { + this._persistSession?.('expired', undefined) } } else { this._persistSession?.('network-error', undefined) From 3126221fe8f95483d2a1cd6503b8b1fdebc8f176 Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 15:30:33 -0600 Subject: [PATCH 08/12] Update tests --- packages/api/tests/agent.test.ts | 2 +- packages/pds/src/account-manager/helpers/auth.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/api/tests/agent.test.ts b/packages/api/tests/agent.test.ts index 45a7b84f02a..cff4e3517a8 100644 --- a/packages/api/tests/agent.test.ts +++ b/packages/api/tests/agent.test.ts @@ -159,7 +159,7 @@ describe('agent', () => { expect(events.length).toEqual(2) expect(events[0]).toEqual('create') - expect(events[1]).toEqual('create') + expect(events[1]).toEqual('update') expect(sessions.length).toEqual(2) expect(sessions[0]?.accessJwt).toEqual(agent1.session?.accessJwt) expect(sessions[1]?.accessJwt).toEqual(agent2.session?.accessJwt) diff --git a/packages/pds/src/account-manager/helpers/auth.ts b/packages/pds/src/account-manager/helpers/auth.ts index 083d5cfd39b..366eee601d7 100644 --- a/packages/pds/src/account-manager/helpers/auth.ts +++ b/packages/pds/src/account-manager/helpers/auth.ts @@ -42,7 +42,7 @@ export const createAccessToken = (opts: { jwtKey, serviceDid, scope = AuthScope.Access, - expiresIn = '120mins', + expiresIn = '5s', } = opts const signer = new jose.SignJWT({ scope }) .setProtectedHeader({ alg: 'HS256' }) // only symmetric keys supported From 3f30a44be96c2e78f37ea8860f68d28f812d93ea Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 18:51:55 -0600 Subject: [PATCH 09/12] Feedback --- packages/api/src/agent.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/api/src/agent.ts b/packages/api/src/agent.ts index 5620e3df6a8..0f6602fc2bb 100644 --- a/packages/api/src/agent.ts +++ b/packages/api/src/agent.ts @@ -175,7 +175,7 @@ export class AtpAgent { } catch (e) { this.session = undefined - if (e instanceof XRPCError && e.error) { + if (e instanceof XRPCError) { /* * `ExpiredToken` and `InvalidToken` are handled in * `this_refreshSession`, and emit an `expired` event there. From 52f7ea6d21a06ea2f7a157ebe2603e53a84a9f7c Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 18:53:09 -0600 Subject: [PATCH 10/12] Revert debug change --- packages/pds/src/account-manager/helpers/auth.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pds/src/account-manager/helpers/auth.ts b/packages/pds/src/account-manager/helpers/auth.ts index 366eee601d7..083d5cfd39b 100644 --- a/packages/pds/src/account-manager/helpers/auth.ts +++ b/packages/pds/src/account-manager/helpers/auth.ts @@ -42,7 +42,7 @@ export const createAccessToken = (opts: { jwtKey, serviceDid, scope = AuthScope.Access, - expiresIn = '5s', + expiresIn = '120mins', } = opts const signer = new jose.SignJWT({ scope }) .setProtectedHeader({ alg: 'HS256' }) // only symmetric keys supported From 2383e039bd58d6ea352e784f04cb2a1fb500cfe6 Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 18:56:24 -0600 Subject: [PATCH 11/12] Changeset --- .changeset/chilled-ghosts-enjoy.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changeset/chilled-ghosts-enjoy.md diff --git a/.changeset/chilled-ghosts-enjoy.md b/.changeset/chilled-ghosts-enjoy.md new file mode 100644 index 00000000000..1e86a85a77f --- /dev/null +++ b/.changeset/chilled-ghosts-enjoy.md @@ -0,0 +1,8 @@ +--- +'@atproto/api': patch +--- + +Improve `resumeSession` event emission. It will no longer double emit when some +requests fail, and the `create-failed` event has been replaced by `expired` +where appropriate, and with a new event `network-error` where appropriate or an +unknown error occurs. From 13847d1347b735814f9bd87e7f160e8353c9e2ba Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Thu, 4 Jan 2024 19:44:19 -0600 Subject: [PATCH 12/12] Bump minor --- .changeset/chilled-ghosts-enjoy.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/chilled-ghosts-enjoy.md b/.changeset/chilled-ghosts-enjoy.md index 1e86a85a77f..29beea82484 100644 --- a/.changeset/chilled-ghosts-enjoy.md +++ b/.changeset/chilled-ghosts-enjoy.md @@ -1,5 +1,5 @@ --- -'@atproto/api': patch +'@atproto/api': minor --- Improve `resumeSession` event emission. It will no longer double emit when some