Improve handling of resumeSession failures (bluesky-social#2010)

* Better handling of resumeSession errors

* Update test

* Format

* Pare back to only necessary

* Update handling for 500s

* Should really be update

* Update logic from feedback

* Update tests

* Feedback

* Revert debug change

* Changeset

* Bump minor

.changeset/chilled-ghosts-enjoy.md

@@ -0,0 +1,8 @@
+---
+'@atproto/api': minor
+---
+
+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.

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,41 @@ 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?.('update', this.session)
       return res
     } catch (e) {
       this.session = undefined
-      throw e
-    } finally {
-      if (this.session) {
-        this._persistSession?.('create', this.session)
+
+      if (e instanceof XRPCError) {
+        /*
+         * `ExpiredToken` and `InvalidToken` are handled in
+         * `this_refreshSession`, and emit an `expired` event there.
+         *
+         * Everything else is handled here.
+         */
+        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?.('create-failed', undefined)
+        this._persistSession?.('network-error', undefined)
       }
+
+      throw e
     }
   }
 

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

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)
@@ -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')