feat!: replace the shared token with a session pool

[lart2150]

BREAKING CHANGE: calls are no longer serialized on one shared session, so per-session server state (found sets, script globals) does not persist across separate calls anymore; use withSession() to pin related calls to one session. The client holds server sessions, so call destroy() on shutdown.

This is my alternative to #25

[lavere]

Automated review. Generated automatically when I was requested as a reviewer. It has not been read by a human.

The pool refactor is well built: the token/session lifecycle moves cleanly into Session/SessionPool, the tarn wiring (validate on invalid + TTL, non-rejecting destroyer, unref'd reaper, propagateCreateError override) is deliberate and commented, and the 84 tests, lint, typecheck and rollup build all pass on the head commit. I found no correctness bug in the new pooling logic. Two smaller items: the deprecated clearToken() can resurrect a destroyed client, and the container-URL host check (pre-existing, moved in this diff) is a prefix match that can leak the bearer token to a lookalike host.

• src/Client.ts:85 (medium) The container host check is a raw prefix match, so https://fms.example.com.attacker.net/... passes when uri is https://fms.example.com, and Session.requestContainer then sends Authorization: Bearer <session token> to that host. This is pre-existing behaviour moved into the new structure, but the diff touches the line (new comment above it) and the token is now a pooled session that stays valid for other calls, so leaking one is worth more than before. Comparing parsed new URL(containerUrl).origin against new URL(this.uri).origin closes it without changing any legitimate call.
• src/Client.ts:110 (low) clearToken() unconditionally assigns a brand new SessionPool to this.pool, which silently undoes destroy(). After await client.destroy(); await client.clearToken(); the client is usable again and will sign in fresh server sessions that the shutdown path already believes it released — SessionPool.acquire()'s destroyed guard is bypassed because it is a different pool object. Same hole if a clearToken() lands after a concurrent destroy() resolves. A client-level destroyed flag (or an early return in clearToken when this.pool is already destroyed) keeps the documented "cannot be used afterwards" contract intact.

[sdbennett]

cna-coder review: Comment · round 1 · head 7bd391c

Contract NOT cross-checked against a sibling repo (this is a standalone published library, not an api/frontend pair).

TLDR

Replaces the single shared Data API token with a tarn-backed session pool, so concurrent calls stop interleaving on one FileMaker session. The pooling logic itself is sound — I found no correctness bug in acquire/validate/destroy, and the tarn wiring is deliberate and explained at each non-obvious point. What's left is a cluster of docblocks that describe behaviour the code does not have, plus two gaps where new logic ships without an assertion.

• Must fix before approval (0 blocking): none, nothing blocks approval
• OK to leave for now (9 non-blocking): clearToken() can resurrect a destroyed client (src/Client.ts:110); markReleased docblock states the unsafe order (src/Session.ts:115); container stream outlives its session (src/Client.ts:89); the unref reach and the new TTL check are both unasserted (src/SessionPool.ts:96, :134); four doc/style nits
• Pre-existing, not introduced by this PR (2): the container host prefix match (src/Client.ts:85) and two assertion-free TTL tests (test/Client.test.ts:65) — file them or leave them

Findings (introduced by this PR)

Severity	Location	Issue	Suggested fix
🟡 Medium	src/Client.ts:110	clearToken() unconditionally installs a fresh live pool, so await destroy(); await clearToken(); makes a destroyed client usable again and signs in new FileMaker sessions after shutdown. SessionPool.acquire()'s destroyed guard is bypassed because it is a different pool object, breaking the documented "cannot be used afterwards" contract.	Track a destroyed flag on Client and have clearToken() no-op or throw once destroy() has run.
🟡 Medium	src/Session.ts:115	The markReleased docblock says it is called "once the session has gone back to the pool", but Client.withSession calls it before pool.release. The docblock states the unsafe order, so a later refactor that makes the code match it would introduce a real use-after-release. (#103)	Restate as called while returning the session, before the pooled resource is released.
🟡 Medium	src/Client.ts:89	requestContainer resolves with an unread ReadableStream after withSession has already released the session, so destroy() signs that session out mid-download rather than waiting for the call to finish as README:149 promises.	Hold the session until the stream ends, or document that the buffer must be consumed before destroy().
🟡 Medium	src/SessionPool.ts:96	The unref of tarn's undeclared interval field is unasserted and guarded by optional chaining, so a tarn rename silently reintroduces a timer that keeps the process alive after the last call — exactly the failure the comment says it is defending against.	Add a test asserting the reap interval is unref'd after the first release, so a tarn upgrade fails CI instead of regressing quietly.
🟡 Medium	src/SessionPool.ts:134	The new isUsable TTL check, including the Math.abs clock-step handling, ships with no assertion covering it. The only two tests named for the 14-minute rule are pre-existing and contain no expect at all.	Assert the sign-in count in both TTL tests: one POST to /sessions for the reuse case, two for the expiry case.
🟢 Low	src/Session.ts:103	requestContainer skips the lastUsedAt refresh when the download fails, while request deliberately refreshes it on error. This contradicts the PooledSession docblock's "refreshed on every completed response", and can retire a live session early.	Move this.pooled.lastUsedAt = Date.now() above the !response.ok throw.
🟢 Low	src/SessionPool.ts:21	The min docblock says a non-zero value "keeps the reaping timer alive for the lifetime of the process", which this same constructor's unref at line 94 contradicts. The advice to leave min at 0 is still right for the other reason given.	Say the timer keeps running, and leave the process-lifetime claim to the one place it is accurate.
🟢 Low	README.md:64	Three British spellings introduced into a repo that currently has none: serialises here, favour at README.md:41, recognises at src/Session.ts:55.	Use serializes, favor, recognizes.
🟢 Low	src/Layout.ts:9	LayoutClient is declared as an interface, the only one in src/, where the house convention reserves interface for declaration merging and module augmentation.	Declare it as a type alias.

Pre-existing (not introduced by this PR, does not gate merge)

Severity	Location	Issue	Note
🟡 Medium	src/Client.ts:85	The container host check is a raw startsWith prefix match, so https://fms.example.com.attacker.net/... passes when uri is https://fms.example.com, and the bearer token is then sent to that host.	Byte-identical to main; this PR only adds a comment above the line. Worth its own ticket — comparing new URL(containerUrl).origin against new URL(this.uri).origin closes it without affecting any legitimate call.
🟢 Low	test/Client.test.ts:65	The two 14-minute TTL tests contain no expect at all and pass whether or not the rule is honored.	Byte-identical to main and untouched here. Fixing them is what would give the isUsable finding above its assertion.

Note on the existing automated review

The other automated review on this head raises the same Client.ts:85 host check and argues it matters more now because "the token is now a pooled session that stays valid for other calls". That reasoning is inverted: on main, getToken() caches one process-wide token and reuses it for 14 minutes across every call, so a leak there exposes the shared token for the entire client. Here it exposes one of up to five pooled sessions. The vulnerability is real and worth a ticket, but this PR narrows its blast radius rather than widening it, and is not the right place to gate it.

What's good

injectHeaders now copies rather than mutates the caller's RequestInit, fixing a latent bug on main where the caller's object was modified in place. The README's constructor example is corrected to the real argument order. The tarn wiring is deliberate and commented at each point where it deviates from the defaults — non-rejecting destroyer, propagateCreateError override, unref'd reaper, validate-on-invalid-plus-TTL — which is what made the pooling logic reviewable. Around 30 new tests cover the behaviour that matters: concurrency up to max, single sign-in for concurrent cold starts, retirement on 952, release-on-throw, and rejection after destroy().

Reviewed by /cna-coder:review, round 1. 🔴 High items in the first table gate merge; 🟡 Medium and 🟢 Low are FYI; the pre-existing table is context, not a request. Re-runs update this review when the PR gets new commits.

[DASPRiD]

FYI, semantic release is not able to parse ! after feat. For breaking changes, the commit body needs to container BREAKING CHANGE: ….

[DASPRiD]

The session-pool rework (tarn-backed SessionPool, per-call Session checkout, withSession, destroy()) is well thought out: the pool-swap logic in clearToken(), the 952-retry-once semantics, the request-object cloning fix in Session.injectHeaders, and the unref workaround for the reaper timer are all documented with the reasoning a reader would otherwise have to reconstruct. Tests cover concurrency, pool exhaustion, token invalidation, and shutdown draining well. One real gap: clearToken() can silently resurrect a client after destroy(), contradicting the stated "cannot be used afterwards" contract.

[DASPRiD]

The move to a session pool fixes real problems: cold-start sessions no longer leak and concurrent calls no longer interleave on one token. The tests for pooling, withSession and destroy are solid. One thing has to change before merge. Session is now public, and its requestContainer sends the bearer token to any URL because the host check only lives in Client. The lifecycle code needs another look too. The 952 retry can reuse another stale session, clearToken() can revive a destroyed client or abort calls still waiting for a session, and the idle-TTL logic has no test that can fail. Several docs and comments promise more than the code does, and the upgrade notes don't list the new behavior changes.

Nits:

• README.md:149 await using isn't native on Node 22

  Node 22 has Symbol.asyncDispose, but native await using syntax arrived in Node 24. On Node 22 it only works when TypeScript compiles it down, and plain JavaScript gets a syntax error. The same claim is in the Client docblock. Say "with TypeScript 5.2+, or natively on Node 24" instead.

• README.md:92 min docs overstate the cost of a non-zero value

  This row and the min docblock in src/SessionPool.ts say a non-zero min keeps a timer alive for the lifetime of the process. The constructor unrefs the reaping interval, so the timer doesn't keep the process alive. Kept sessions also still expire on the server after 15 minutes. The real cost is that the pool holds tokens that are already dead, and those are dropped on the next acquire.

• src/SessionPool.ts:176 Comment on the signOut catch describes the wrong case

  fetch doesn't reject on an HTTP error status. A DELETE for an expired session resolves with an error response, and that response is ignored without the catch being involved. The catch only swallows network failures and aborts, so the comment should say that.

• jest.config.js:5 restoreMocks comment doesn't match the tests

  The comment says the Date.now() spies leak into later tests without restoreMocks. The new afterEach in test/Client.test.ts already calls jest.restoreAllMocks() before destroy(), so the spies are restored either way. Drop either the option or the manual restore, and the comment with it.

• test/Client.test.ts:22 Test comments describe the old implementation

  "Sign-out is now driven by the pool" here, and "Without a pool each of the three would race…" at line 463, describe history rather than the code. Rewrite the first in the present tense ("The pool signs sessions out, so a DELETE can happen in any test that signed in."). The second can go: the sessionsIssued() assertion already states the rule.

• src/Session.ts:55 British spellings in new text

  "recognises" here, plus "favour" (README.md:41) and "serialises" (README.md:64). Use "recognizes", "favor" and "serializes".

• src/Client.ts:72 Rename e to error

  catch (e) is used for e.code and throw e a few lines down, and error reads better. The same applies to the new TimeoutError example in the README.

Pre-existing, not introduced by this pull request, so not a condition for merging:

src/Client.ts:85

Non-blocking: Container host check is a string prefix match

containerUrl.toLowerCase().startsWith(this.uri.toLowerCase()) accepts https://fm.example.com.evil.net/... and https://fm.example.com@evil.net/... when uri is https://fm.example.com, and the bearer token is sent to those hosts. The PR touches this hunk. Parse both values with new URL() and compare origin instead.

src/Session.ts:87

Non-blocking: Container redirect recursion has no depth limit

Every 302 with set-cookie recurses into requestContainer again and re-sends the bearer token each time. Nothing caps the number of hops. Under the pool, a server that keeps redirecting also holds a pooled session the whole time, which blocks every other caller at max: 1. Capping the hops at 1 or 2 fixes both.

src/Session.ts:52

Non-blocking: Error handling assumes a JSON body with messages[0]

await response.json() throws SyntaxError on an HTML 502 or 503 page from a proxy, and data.messages[0].code throws TypeError when messages is missing. Either way the 952 handling is skipped and the caller gets an unhelpful error. signIn in src/SessionPool.ts has the same pattern.

test/Client.test.ts:674

Nit: "should do nothing without a token" can no longer fail

The test asserts that nothing was fetched at .../sessions/null. The pool never builds that URL, so the assertion holds whatever clearToken() does. If the intent is "no sign-out when nothing was signed in", assert that no DELETE went to the sessions glob at all.

[DASPRiD]

Blocking: Session.requestContainer sends the bearer token to any host

Client.requestContainer checks that the container URL starts with this.uri before it delegates (src/Client.ts:85). Session.requestContainer has no check of its own, but it's public API now: it's exported from src/index.ts and it's what every withSession callback receives. So client.withSession(session => session.requestContainer(url)) attaches Authorization: Bearer <token> to whatever URL it's given and fetches it.

Container URLs usually come from record field data, so anyone who can write that field decides where the token goes. On the base branch the only public entry point enforced the check.

Move the host check into Session.requestContainer, or into a helper both methods call. It should compare origins parsed with new URL(), not do a string prefix match.

[DASPRiD]

Non-blocking: The 952 retry can pick another stale session instead of signing in

The retry calls withSession again, which calls pool.acquire(). That hands out any free session that passes isUsable, and isUsable only checks the invalid flag and the local 14-minute timer.

After a FileMaker Server restart or an admin disconnect, every free session is dead on the server but still looks fine locally. The retry picks one of them, gets 952 again, and the caller sees the error. The old code always signed in again before retrying.

The comment above this line ("this acquires a freshly signed-in one") and the README line "retried once on a fresh one" don't match the code. Either force a new sign-in for the retry, or mark every free session invalid when any of them returns 952.

[DASPRiD]

Non-blocking: clearToken() aborts waiting calls and can revive a destroyed client

Two problems in the pool swap.

First, the comment says in-flight calls on the old pool run to completion. That holds for a call that already holds a session, which is what the new test covers. It doesn't hold for a call that captured this.pool in withSession and is still inside pool.acquire(), for example while a cold sign-in is running or while it waits for a free slot. Once tarn's pool is destroyed, _tryAcquireOrCreate returns early, and destroy() then aborts every pending acquire, so those callers get tarn's abort error. The same gap applies to destroy(): the docblock and README say it waits for in-flight calls, but queued calls are rejected. The README's "existing calls keep working" doesn't hold for them either.

Second, destroy() only destroys the current pool. A clearToken() after destroy() installs a new, working pool, so the "cannot be used afterwards" contract is broken. If clearToken() runs while destroy() is draining, the new pool is never destroyed. A destroyed flag on Client, checked in clearToken, withSession and destroy, fixes the second problem.

[DASPRiD]

Non-blocking: destroy() or clearToken() inside a withSession callback never returns

tarn's destroy() polls until no resource is checked out, with no timeout. If a withSession callback, or error handling reached from inside one, calls await client.destroy() or await client.clearToken(), it waits for the session the callback is holding, and that session is only released after the callback returns. Before this PR, clearToken() was a single sign-out request.

The docblock lists request, requestContainer and withSession as off limits inside the callback, but not these two. The docblock (and src/Session.ts:17 and the README) also call a nested acquire a deadlock. With the default options it waits acquireTimeoutMillis and then fails with TimeoutError, and the docs should say that.

[DASPRiD]

Non-blocking: Math.abs extends a session's life when the clock steps back a little

The comment says Math.abs makes a backwards clock step retire the session instead of extending its life. That's only true for a step of more than 14 minutes. For a smaller step, lastUsedAt ends up in the future. The measured idle time is then shorter than the real one, so the session is kept longer than the TTL, which is the case the comment says this prevents. request() recovers through the 952 retry, but withSession doesn't retry. Use a monotonic clock (performance.now()) for lastUsedAt, or correct the comment.

[DASPRiD]

Non-blocking: Session TTL and clock handling have no test that can fail

isUsable is the only thing that retires stale sessions, and no test exercises it. The two existing TTL tests look like they do, but neither can fail. "should reuse a token for 14 minutes" (test/Client.test.ts:65) has no assertion, and its POST route isn't Once, so a second sign-in would still pass. In "should request a new token after 14 minutes" (test/Client.test.ts:92), the bar routes are registered after identical foo routes, and authorization is set as a response header, so nothing checks that a new session was used.

The backwards-clock case and the lastUsedAt refresh on non-952 errors (src/Session.ts:65) have no test either. Count the sign-in POSTs, and assert which token the later request sent.

[DASPRiD]

Non-blocking: Upgrade notes miss the new behavior changes

The section above still says modId is "the one change that can break a call site". This PR adds more:

• concurrency is capped at max (5), and calls past that queue and can fail with TimeoutError after 30s, where before they all ran on one token
• clearToken() now waits for in-flight calls
• each client can hold up to 5 FileMaker connections instead of 1
• tarn becomes the package's first runtime dependency, and its TimeoutError class is now part of the public API

The root also now exports Session, PooledSession and LayoutClient, and Session.markReleased is public (tagged @internal, but nothing strips it). List these, or keep the internals out of the root exports.