Skip to content

fix: share the in-flight sign in so a cold token opens one session - #25

Open
sdbennett wants to merge 1 commit into
soliantconsulting:mainfrom
sdbennett:fix/single-flight-token
Open

fix: share the in-flight sign in so a cold token opens one session#25
sdbennett wants to merge 1 commit into
soliantconsulting:mainfrom
sdbennett:fix/single-flight-token

Conversation

@sdbennett

Copy link
Copy Markdown

The problem

getToken has no in-flight guard:

private async getToken(): Promise<string> {
    if (this.token !== null && Date.now() - this.lastCall < 14 * 60 * 1000) {
        return this.token;
    }
    // POST /sessions ...
}

Every caller arriving while the token is null or older than 14 minutes passes that check and runs its own sign in. N concurrent callers open N Data API sessions. Only the last one is kept in this.token; the rest are never referenced again and never DELETEd, so they sit against the server's session limit until FileMaker times them out.

It is invisible from outside the library, because getToken is private and the retry path that also triggers it lives inside request.

Where we hit it

tracker-api serves a page that fires three concurrent GET /v1/time-entries after every save. On any cold start that opened three sessions and abandoned two, which is every time someone comes back after a quarter of an hour and logs time.

We first worked around it in the consumer by serialising the first request of an idle period, but that meant mirroring this file's private 14 minute constant from outside, which quietly breaks the moment it changes. Fixing it here removes the workaround and covers every other consumer.

The change

Share the pending promise, and clear it when it settles:

this.tokenRequest ??= this.mintToken().finally(() => {
    this.tokenRequest = null;
});

return this.tokenRequest;

One sign in however many callers arrive together. Clearing on settle rather than on success matters: a rejected sign in must not stay cached, or the client can never recover from one bad login.

The body of the old getToken moves to mintToken unchanged, so the sign in itself, its error handling and lastCall bookkeeping are untouched.

This also covers the invalid-token retry. request nulls the token on code 952 and calls itself, so concurrent retries after a server restart or an admin session clear now coalesce the same way. That path cannot be reached from a subclass at all.

Tests

Three added to test/Client.test.ts, in the existing fetchMock style:

  • one sign in for three concurrent callers
  • a failed sign in is not cached, and the next caller recovers
  • every waiter rejects when the shared sign in fails

Verified by mutation rather than assumed: dropping the shared promise fails the first and leaves the other sixteen in that file passing.

npx jest 68 passing across the three suites, npm run lint clean, npm run build succeeds.

Compatibility

No public API change. token, lastCall, the 14 minute window, the sign in request and every error are as before. The only new state is one private field.


Raised from a fork because I do not have push access on this repo. maintainer_can_modify is on.

getToken had no in-flight guard. Every caller arriving while the token was null
or older than 14 minutes ran its own sign in, so N concurrent callers opened N
Data API sessions. Only the last one was kept in this.token; the rest were never
referenced again and never deleted, so they sat against the server's session
limit until it timed them out.

Consumers hit this whenever a page fans out. A tracker-api endpoint that answers
three concurrent requests after every save opened three sessions and abandoned
two on any cold start, which is every time someone comes back after a quarter of
an hour and logs time.

Sharing the pending promise means one sign in however many callers arrive
together. The promise is cleared when it settles, so a failed sign in is not
cached and the next caller retries rather than being handed a rejection for good.

This also covers the invalid-token retry: request() nulls the token on code 952
and calls itself, and concurrent retries now coalesce the same way. That path is
not reachable from a subclass, because the retry happens inside request().

Three tests: one sign in for three concurrent callers, recovery after a failed
sign in, and every waiter rejecting when the shared sign in fails. Verified by
mutation: dropping the shared promise fails the first and leaves the other
sixteen in the file passing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant