Add subscription gateway account pool core - #278
happylinks wants to merge 1 commit into
Conversation
🤖 OS review · request changes · confidence 2/5Safe once the persistence failure and cancellation handling below are fixed. The package cleanly separates routing, cooldowns, and account walking, but two partial-failure paths can leave routing state incorrect, and its tests are not included in the required CI suite.
🔁 Not merge-ready and no live session owns this branch — add the |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| until, | ||
| }); | ||
| await this.#persist(); | ||
| return { |
There was a problem hiding this comment.
🔴 P1 — Failed persistence leaves an unrollbackable wedge active
markWedged mutates #records before saving, but returns the rollback token only after the save succeeds. If the JSON write fails because the state directory is read-only or the disk is full, markWedged("account-a", 300_000) rejects while isActive("account-a") remains true. The caller receives no token to clear, so a sole account remains unavailable for five minutes despite the operation reporting failure; a later successful mutation can also silently persist that failed wedge. markExhausted and clearWedge have the same in-memory/disk divergence. Serialize each mutation with its save and restore the prior record when persistence fails, without clobbering a newer queued mutation.
| ): Promise<AccountWalkResult<TAccount, TResult>> { | ||
| const excludedIds = new Set<string>(); | ||
|
|
||
| while (!options.signal?.aborted) { |
There was a problem hiding this comment.
🟠 P2 — Cancellation during account acquisition still starts a provider attempt
The loop checks the signal before await options.acquire, but not after it. Since acquire is explicitly allowed to be asynchronous, a request aborted while account discovery is pending still invokes attempt once acquisition resolves, potentially starting a paid provider request after the client disconnected. Recheck the signal immediately after acquisition and return aborted before examining or attempting the selected account.
| while (!options.signal?.aborted) { | |
| const resolution = await options.acquire(excludedIds); | |
| if (options.signal?.aborted) return { kind: "aborted" }; |
| access: { kind: "shared" }, | ||
| }); | ||
|
|
||
| describe("walkAccounts", () => { |
There was a problem hiding this comment.
🟠 P2 — The new package tests are excluded from required CI
All three new test files live under packages/core/subscription-gateway/src, but scripts/test-unit-isolated.sh, used by both bun run check and the required CI unit-test step, only searches packages/core/opensession-server/src and scripts. The root bun test script has the same exclusion, so these routing, retry, and durability regressions can merge with green required checks. Add the subscription-gateway source directory to find_tests or invoke a package-specific test script from the required check.
Summary
@tellahq/subscription-gatewayworkspace packageTesting
bun run checkStack 1 of 2. The next PR adds the OpenAI Chat Completions wire mapping.
Started by Michiel Westerbeek in this OS session