fix: resolve remaining branch review musts (tui barrel, lock toctou, debug leak, killswitch, quota producer)

Author: iceteaSA

ARCHITECTURE.md

@@ -16,6 +16,18 @@
     ".": {
       "import": "./dist/index.js",
       "types": "./dist/index.d.ts"
+    },
+    "./file-lock": {
+      "types": "./dist/file-lock.d.ts",
+      "import": "./dist/file-lock.js"
+    },
+    "./atomic-write": {
+      "types": "./dist/atomic-write.d.ts",
+      "import": "./dist/atomic-write.js"
+    },
+    "./fetch-timeout": {
+      "types": "./dist/fetch-timeout.d.ts",
+      "import": "./dist/fetch-timeout.js"
     }
   },
   "files": [

STRUCTURE.md

@@ -691,6 +691,15 @@ export class AccountManager {
     softQuotaThresholdPercent: number = 100,
     softQuotaCacheTtlMs: number = 10 * 60 * 1000,
     identity?: AccountSessionIdentity,
+    /**
+     * Account indexes the caller has ruled out (e.g. the operator
+     * killswitch pre-filter). Every selection path — pinned session,
+     * round-robin, hybrid, and sticky fallback — skips these indexes
+     * so a killed current account falls through to the next eligible
+     * account instead of collapsing the request into the rate-limit
+     * wait path.
+     */
+    excludeIndexes?: Set<number>,
   ): ManagedAccount | null {
     const quotaKey = getQuotaKey(family, headerStyle, model)
     const effectiveSoftQuotaThreshold = this.getEffectiveSoftQuotaThreshold(
@@ -704,6 +713,7 @@ export class AccountManager {
       if (pinned) {
         clearExpiredRateLimits(pinned, this.now)
         const unavailable =
+          (excludeIndexes?.has(pinned.index) ?? false) ||
           isRateLimitedForHeaderStyle(
             pinned,
             family,
@@ -735,6 +745,7 @@ export class AccountManager {
         effectiveSoftQuotaThreshold,
         softQuotaCacheTtlMs,
         identity,
+        excludeIndexes,
       )
       if (next) {
         this.markTouchedForQuota(next, quotaKey)
@@ -748,7 +759,9 @@ export class AccountManager {
       const tokenTracker = getTokenTracker()
 
       const eligibleAccounts = this.preferAccountOutsideParent(
-        this.accounts.filter((acc) => acc.enabled !== false),
+        this.accounts.filter(
+          (acc) => acc.enabled !== false && !excludeIndexes?.has(acc.index),
+        ),
         family,
         identity,
       )
@@ -825,7 +838,7 @@ export class AccountManager {
     }
 
     const current = this.getCurrentAccountForFamily(family, identity)
-    if (current) {
+    if (current && !excludeIndexes?.has(current.index)) {
       clearExpiredRateLimits(current, this.now)
       const isLimitedForRequestedStyle = isRateLimitedForHeaderStyle(
         current,
@@ -859,6 +872,7 @@ export class AccountManager {
       effectiveSoftQuotaThreshold,
       softQuotaCacheTtlMs,
       identity,
+      excludeIndexes,
     )
     if (next) {
       this.markTouchedForQuota(next, quotaKey)
@@ -874,6 +888,8 @@ export class AccountManager {
     softQuotaThresholdPercent: number = 100,
     softQuotaCacheTtlMs: number = 10 * 60 * 1000,
     identity?: AccountSessionIdentity,
+    /** Indexes ruled out by the caller (e.g. killswitch pre-filter). */
+    excludeIndexes?: Set<number>,
   ): ManagedAccount | null {
     const effectiveSoftQuotaThreshold = this.getEffectiveSoftQuotaThreshold(
       softQuotaThresholdPercent,
@@ -882,6 +898,7 @@ export class AccountManager {
       clearExpiredRateLimits(account, this.now)
       return (
         account.enabled !== false &&
+        !excludeIndexes?.has(account.index) &&
         !isRateLimitedForHeaderStyle(
           account,
           family,

packages/core/package.json

@@ -893,6 +893,116 @@ describe('acquireFencedFileLock — renewal TOCTOU', () => {
     await rm(lockPath, { force: true })
   })
 
+  it('marks the lock lost when a replacement owner arrives between the renewal read and rename', async () => {
+    const target = join(root, 'state.json')
+    const lockPath = `${target}.accounts.lock`
+    const clearIntervalSpy = spyOn(globalThis, 'clearInterval')
+
+    let pausedResolve!: () => void
+    const paused = new Promise<void>((resolve) => {
+      pausedResolve = resolve
+    })
+    let gateResolve!: () => void
+    const gate = new Promise<void>((resolve) => {
+      gateResolve = resolve
+    })
+    let hookFired = false
+
+    try {
+      const lock = await acquireFencedFileLock({
+        path: target,
+        name: 'accounts',
+        ttlMs: 60_000,
+        renewIntervalMs: 10,
+        onStep: async (step) => {
+          if (step === 'renew-read' && !hookFired) {
+            hookFired = true
+            pausedResolve()
+            await gate
+          }
+        },
+      })
+      expect(lock).not.toBeNull()
+
+      // Wait for owner A's renewal tick to reach the read seam, then
+      // owner B takes over between A's read and A's rename.
+      await paused
+      await writeLock(lockPath, 'owner-B', Date.now() + 60_000)
+      gateResolve()
+
+      await lock!.whenLost()
+      expect(lock!.hasLost()).toBe(true)
+      // markLost() cleared the renewal interval exactly once.
+      expect(clearIntervalSpy).toHaveBeenCalledTimes(1)
+
+      // A's pre-rename re-read must catch the takeover before the
+      // rename — B's lock is intact, not clobbered by A's renewal.
+      const contents = await readLock(lockPath)
+      expect(contents?.ownerId).toBe('owner-B')
+
+      await lock!.release()
+      await rm(lockPath, { force: true })
+    } finally {
+      clearIntervalSpy.mockRestore()
+    }
+  })
+
+  it('marks the lock lost when a takeover lands right after the renewal rename (verify-after-commit)', async () => {
+    const target = join(root, 'state.json')
+    const lockPath = `${target}.accounts.lock`
+    const clearIntervalSpy = spyOn(globalThis, 'clearInterval')
+
+    let pausedResolve!: () => void
+    const paused = new Promise<void>((resolve) => {
+      pausedResolve = resolve
+    })
+    let gateResolve!: () => void
+    const gate = new Promise<void>((resolve) => {
+      gateResolve = resolve
+    })
+    let hookFired = false
+
+    try {
+      const lock = await acquireFencedFileLock({
+        path: target,
+        name: 'accounts',
+        ttlMs: 60_000,
+        renewIntervalMs: 10,
+        onStep: async (step) => {
+          if (step === 'renew-committed' && !hookFired) {
+            hookFired = true
+            pausedResolve()
+            await gate
+          }
+        },
+      })
+      expect(lock).not.toBeNull()
+
+      // A's renewal has just renamed its refreshed payload onto the
+      // lock file; owner B's acquisition lands in the post-commit
+      // window, before A's verify re-read.
+      await paused
+      await writeLock(lockPath, 'owner-B', Date.now() + 60_000)
+      gateResolve()
+
+      await lock!.whenLost()
+      expect(lock!.hasLost()).toBe(true)
+      expect(clearIntervalSpy).toHaveBeenCalledTimes(1)
+
+      // A backed off after the verify re-read saw B; B's write is the
+      // final content and A's release refuses to delete it.
+      const contents = await readLock(lockPath)
+      expect(contents?.ownerId).toBe('owner-B')
+
+      await lock!.release()
+      const afterRelease = await readLock(lockPath)
+      expect(afterRelease?.ownerId).toBe('owner-B')
+      await rm(lockPath, { force: true })
+    } finally {
+      clearIntervalSpy.mockRestore()
+    }
+  })
+
   it('whenLost() resolves promptly when the lock is taken over mid-renewal', async () => {
     const target = join(root, 'state.json')
     const lockPath = `${target}.accounts.lock`

packages/core/src/account-manager.ts

@@ -41,6 +41,8 @@ export type FileLockStep =
   | 'stale-marker-claimed'
   | 'stale-lock-confirmed'
   | 'eviction-marker-acquired'
+  | 'renew-read'
+  | 'renew-committed'
 
 export interface FencedFileLockOptions {
   path: string
@@ -62,8 +64,13 @@ export interface FencedFileLockOptions {
   now?: () => number
   /**
    * Hook invoked at well-defined milestones inside the eviction
-   * protocol. Used by tests to inject interleavings and simulate
-   * racing contenders.
+   * protocol and the renewal tick. Used by tests to inject
+   * interleavings and simulate racing contenders:
+   *
+   * - `renew-read` — after the renewal tick reads the lock payload,
+   *   before the ownership checks and the temp-write/rename commit.
+   * - `renew-committed` — after the renewal rename lands, before the
+   *   verify-after-commit re-read.
    */
   onStep?: (step: FileLockStep) => Promise<void> | void
 }
@@ -373,11 +380,12 @@ function buildLock(
       try {
         const observed = await readLockPayload(lockPath)
         if (released || lost) return
+        await options.onStep?.('renew-read')
+        if (released || lost) return
         // If the lock file is gone or carries a different ownerId, the
-        // lock has been taken over. Mark it lost so the next iteration
-        // stops renewing — this is the case the dispatch's TOCTOU test
-        // exercises (owner B evicts and replaces the lock while owner
-        // A's renewal is paused).
+        // lock has been taken over (e.g. owner B evicted and replaced
+        // the lock while this renewal was paused). Mark it lost so the
+        // next iteration stops renewing.
         if (observed === null) {
           markLost()
           return
@@ -424,6 +432,21 @@ function buildLock(
             }
             shouldCommit = true
             await rename(tempPath, lockPath)
+            await options.onStep?.('renew-committed')
+            if (released || lost) return
+            // Verify-after-commit: a replacement owner may have arrived
+            // between the re-read above and the rename — our rename then
+            // just overwrote their fresh lock — or may land a microsecond
+            // after our rename. Re-read and, if the file no longer carries
+            // our ownerId, mark lost and stop renewing so the double-owner
+            // window collapses to this seam; the fresh owner's next
+            // renewal/acquire attempt re-acquires.
+            const committed = await readLockPayload(lockPath)
+            if (released || lost) return
+            if (!committed || committed.ownerId !== ownerId) {
+              markLost()
+              return
+            }
           } catch {
             // Lost the race against a rename/evict — mark lost so the
             // next iteration stops renewing, and clean up any temp draft.

packages/core/src/file-lock.test.ts

@@ -1,6 +1,7 @@
 import { afterEach, describe, expect, it } from 'bun:test'
 import type { AccountMetadataV3 } from './account-types.ts'
 import { createQuotaManager, type FetchAccountQuota } from './quota-manager.ts'
+import type { AccountQuotaResult } from './quota-types.ts'
 
 function makeAccount(
   overrides: Partial<AccountMetadataV3> = {},
@@ -590,6 +591,61 @@ describe('dispose', () => {
     const next = await manager.refreshAccount(account, { index: 0 })
     expect(next.status).toBe('error')
   })
+
+  it('awaits the in-flight refresh before resolving so a post-refresh side effect is fenced', async () => {
+    // A producer (the opencode quota wrapper) fires a fire-and-forget
+    // sidebar write in the continuation after refreshAccount resolves.
+    // dispose() must not resolve until that in-flight refresh has
+    // settled, so the continuation's write is enqueued before the
+    // lifecycle drains the sidebar queue in the following phase. A
+    // side-effect attached to the refresh promise stands in for that
+    // continuation here.
+    const order: string[] = []
+    let resolveFetch: ((result: AccountQuotaResult) => void) | null = null
+    const fetch: FetchAccountQuota = (account) => {
+      order.push('fetch:start')
+      return new Promise<AccountQuotaResult>((resolve) => {
+        resolveFetch = () =>
+          resolve({
+            index: 0,
+            status: 'ok',
+            email: account.email,
+            quota: { groups: {}, modelCount: 0 },
+          })
+      })
+    }
+
+    const manager = createQuotaManager({
+      fetchAccountQuota: fetch,
+      keyOf: keyOfAccount,
+    })
+    const account = makeAccount({ email: 'inflight@example.com' })
+
+    let sideEffectRan = false
+    const pending = manager
+      .refreshAccount(account, { index: 0 })
+      .then((result) => {
+        // Stand-in for the wrapper's fire-and-forget sidebar write.
+        sideEffectRan = true
+        order.push('side-effect')
+        return result
+      })
+
+    // The fetch is now mid-flight (awaiting resolveFetch). Kick off
+    // dispose, then release the fetch — dispose must await the in-flight
+    // refresh and its continuation before resolving.
+    const disposed = manager.dispose().then(() => {
+      order.push('dispose:resolved')
+      // The producer's side-effect was scheduled before dispose resolved.
+      expect(sideEffectRan).toBe(true)
+    })
+    resolveFetch?.()
+
+    await Promise.all([disposed, pending])
+
+    // fetch:start → (in-flight refresh settles + side-effect) → dispose.
+    expect(order).toEqual(['fetch:start', 'side-effect', 'dispose:resolved'])
+  })
 })
 
 describe('refreshAccounts', () => {