Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of resumeSession failures #2010

Merged
merged 13 commits into from
Jan 5, 2024
34 changes: 26 additions & 8 deletions packages/api/src/agent.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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 && e.error) {
estrattonbailey marked this conversation as resolved.
Show resolved Hide resolved
/*
* `ExpiredToken` and `InvalidToken` are handled in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this._refreshSession implicitly called by the retry logic from calling getSession?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little confusing, but we override the static AtpAgent.fetch here so that if we get an ExpiredToken we can try to refresh the session. So calling getSession actually calls this._fetch here, which calls this._refreshSession.

But yeah, basically expired will be emit from that handling + afaict we don't get a ExpiredToken error from getSession.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet checks out 👍

* `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
}
}

Expand Down
7 changes: 6 additions & 1 deletion packages/api/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions packages/api/tests/agent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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')
Expand Down
2 changes: 1 addition & 1 deletion packages/pds/src/account-manager/helpers/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading