Skip to content

fix(opencode): keep each request on its own member list - #14353

Merged
diegosouzapw merged 4 commits into
diegosouzapw:release/v3.8.51from
maxmad64bis:fix/opencode-accounts-per-request
Sep 22, 2026
Merged

diegosouzapw merged 4 commits into
diegosouzapw:release/v3.8.51from
maxmad64bis:fix/opencode-accounts-per-request

Conversation

@maxmad64bis

@maxmad64bis maxmad64bis commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ base-red inherited: #13866

Summary

Default change: before, two requests sharing one executor walked one shared member list so one's re-sync could replace the other's picks; after, each request walks its own member list built from its own credentials because state is now keyed by request body identity. It's the same dispatch order for a single request, since the shared pick index is kept and clamped to the local list and per-member health is kept in a named store with write-back.

Related Issues

  • No linked issue — overlap isolation fix carries its own regression test.

Validation

  • Change type: provider
  • Focused tests and category gates from the golden path
  • npm run lint
  • Reconciled with the current active release base; focused checks rerun afterward
  • Production-code changes include a new or updated automated test in this PR

Tests Added Or Updated

  • tests/unit/opencode-accounts-per-request.test.ts — overlap isolation for two interleaved requests with different credentials, shared pick-index continuity across sequential requests, cooldown survival across re-sync, cold default member, and fleet to direct to fleet index reset.

Coverage Notes

  • The changed executor paths in open-sse/executors/opencode.ts and the new open-sse/executors/opencodeAccountScope.ts are covered by the new overlap test plus the existing neighbouring executor tests that guard single-request selection order.
  • There's no coverage drop in the touched files, and any move gets recovered with an added case in the same new test file.

Reviewer Notes

  • New PR cut from release 34113170f4 with no parent dependency; park and pacing arms are kept intact with drivers now taking the per-request list.
  • opencode.ts 1244 lines vs frozen 1247 (margin 3, no rebaseline); file-size, complexity, lint, and typecheck gates green.
  • API Route Typecheck failure is inherited (2 pre-existing TS2677 outside this diff, identical on third-party runs on the same base, base-red 🔴 Release branch not green: release/v3.8.51 #13866).

Reconcile with the release tip: diegosouzapw#14148 landed the request-shape replay wrapper
(execute -> withRequestShapeRetry -> executeOnce). Keep it and wrap it in this
branch's per-request member-list release, so the list is written back once per
request after any shape replay.
@diegosouzapw
diegosouzapw merged commit a61b34d into diegosouzapw:release/v3.8.51 Sep 22, 2026
9 of 16 checks passed
diegosouzapw added a commit to maxmad64bis/OmniRoute that referenced this pull request Sep 22, 2026
…ter the diegosouzapw#14353/diegosouzapw#14213 merge

diegosouzapw#14353 moved the opencode member list off the shared instance, so the
rotation-attribution snapshot/skip enumeration now walks the per-request
list; opencode.ts settles at 1318. The registry gained
FLUSH_EMPTY_RETRY_ENABLED from diegosouzapw#14213, taking the catalog to 77 flags
(Network 19).
diegosouzapw pushed a commit that referenced this pull request Sep 22, 2026
… id, request correlation) (#14223)

Merged. Thank you, @maxmad64bis — and thank you for putting the whole thing behind an off-by-default flag, which is what made it possible to take a 25-file observability change late in a cycle.

"Rotation passes over cooling-down accounts, traffic concentrates, and nothing says why" is a real operator problem, and naming the skipped accounts, exposing per-account rotation state, and carrying the serving account plus request correlation onto proxy log rows is the right set of three. No selection behaviour changes.

This one needed the most reconciliation of the batch, because `opencode.ts` moved four times under it. What was found and fixed, so it is on the record:

**1 — Migration collision that would have stopped the app from booting.** You added `182_proxy_logs_rotation_account.sql`, but `182_request_cost_ledger_and_key_quota.sql` landed on the release while this was open. `getMigrationFiles()` in `src/lib/db/migrationRunner.ts` detects version collisions and **throws**, so a fresh install would have failed at startup — and the `case "182":` arm you added to `isSchemaAlreadyApplied()` would have answered for the *other* migration's schema and skipped it, exactly the hazard your own comment warns about. Renumbered to **183/184**, with `migrationRunner.ts`, the test file name and its contents, and the rebaseline note all moved with them. Proven by `tests/unit/migration-135-numbering-collision.test.ts` (which runs against the *real* migrations directory, unlike the PR's own test which copies into a temp dir), and mutation-proved by re-adding the duplicate and watching it fail.

**2 — A dangling reference git never flagged.** #14353 moved the rotation member list off the shared instance, deleting `this.accounts`. Your `noteRotationAccount` site read `this.accounts.length` and auto-merged with no conflict marker. Fixed to the per-request `accounts`, along with three sibling call sites and `snapshotEntries()`, which had to be re-signed to take the list.

**3 — Two rotation exits were missing the attribution flush.** The `runParkAndReplay` returns in the 429 arm returned without flushing `skippedCooldown`, so a parked-and-replayed request lost its skipped-account line. Added with the same `attributionOn && skippedCooldown.size > 0` guard the other exits use — there are now nine guarded exits. The single-account fast path needs none (`skippedCooldown` is provably empty there).

Validation on the final tree: rotation-attribution suites 4+2+2, `proxy-logger-attribution` 4, `migration-183-184` 3, `migration-135-numbering-collision` 2, `resilience-connections-rotation` 2, `feature-flags-settings` 63, plus the six opencode suites (5+4+34+9+38+8+9) — all green. `opencode-executor` is 58/59 with `"omits accept header when stream is false"` failing identically on a pure-tip control checkout, so inherited. `typecheck:core` clean, `check:file-size` OK, `check:changelog-integrity` OK. `check:open-sse-typecheck` fails with exactly the 8 inherited `auggie.ts` errors (#14496) and nothing naming a file this PR touches.

Flag count reconciled to 77; `ROTATION_ATTRIBUTION` stays `defaultValue: "false"`.

Two things to be aware of going forward:

- `open-sse/executors/opencode.ts` (1318) and `src/sse/handlers/chat.ts` (2559) now sit **exactly at their ceilings**, with zero headroom — the previous 1386 entry was ~68 lines of phantom headroom and was tightened to the measured value. The next PR touching either will need a rebaseline of its own.
- `check:docs-all` reports the migration count drift (README/AGENTS/llm.txt say 178; the tip alone is at 179 and these two take it to 181). Left untouched on purpose: those are agent-instruction surfaces and the count is reconciled on the merge train, not per-PR.
@maxmad64bis
maxmad64bis deleted the fix/opencode-accounts-per-request branch September 24, 2026 21:14
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.

2 participants