Skip to content

refactor(providers): share AbortCoordinator across both provider query loops#701

Open
griffinwork40 wants to merge 1 commit into
mainfrom
afk/split-openai-query-file
Open

refactor(providers): share AbortCoordinator across both provider query loops#701
griffinwork40 wants to merge 1 commit into
mainfrom
afk/split-openai-query-file

Conversation

@griffinwork40

Copy link
Copy Markdown
Owner

What

openai-compatible/query.ts hand-rolled the per-session abort slot that anthropic-direct already encapsulates in AbortCoordinator — including six copies of the compare-and-clear invariant:

if (this.abortController === controller) this.abortController = null;

This moves AbortCoordinator to providers/shared/ and wires openai-compatible through it.

Why

That invariant is safety-critical: a leaked stale controller makes compact()'s idle guard misread "turn in flight" as idle and run summarization mid-turn. Maintaining it by hand in two providers is exactly the drift risk shared/tool-loop-cap.ts exists to prevent — and anthropic-direct/query.ts already has zero raw this.abortController references because it solved this.

AbortCoordinator was already provider-neutral: zero imports, no SDK types. Only its docstring said "Anthropic." Copying it into openai-compatible/ would have created the duplication this PR removes, so it moves to shared/ instead.

before after
raw abortController refs in openai query.ts 9 0
duplicated compare-and-clear sites 6 0 (one clear() write path)
abort-related fields on the class 4 1 collaborator

Four fields (abortController, pendingAbortReason, closeResolve, closedPromise) collapse into one abort collaborator. Net −26 lines of logic in query.ts; total file length is ~flat because the freed space went into documenting the contract at each site.

⚠️ One behavioural delta — intentional, convergent

compactHistory's beginAbort now goes through AbortCoordinator.begin(), which drains a pending abort reason.

An interrupt() parked between turns therefore pre-aborts the auto-compaction firing at that boundary and consumes the reason, instead of letting compaction run and deferring the abort to the next turn.

This matches anthropic-direct/query/compact-handler.ts:148 and is arguably the more correct reading (ESC should stop compaction too), but it is a change. Narrow repro window: pending interrupt and auto-compact enabled and the threshold crossing at that exact boundary. Documented inline at the call site.

Preserving the old behaviour would have required adding a non-draining begin variant to the shared class for one caller — working against the anti-drift goal of the PR. Flagging it explicitly rather than burying it.

Tests

Adds shared/abort-coordinator.test.ts (14 cases) — the class previously had no dedicated test, covered only indirectly via concurrent-session-isolation.test.ts. Now that it's load-bearing for two providers that gap was worth closing:

  • scope lifecycle (begin → busy → clear → idle)
  • stale-scope compare-and-clear protection (the highest-risk invariant)
  • pending-abort drain, including drain-exactly-once and park-when-already-aborted
  • the close-promise race the provider main loop depends on

Verification

  • pnpm lint clean (tsc --noEmit, strict)
  • Full suite: 699 files, 13082 passed, 14 skipped
  • audit:sdk:check, audit:env:check, scan:env:check all green
  • pnpm build succeeds

Scope

Part of #360 (god-class decomposition); follows #407, which extracted client/model-params/retry from this same file.

Deliberately not in this PR:

  • SessionState bag — moves no logic on its own; only earns its place as the precondition for extracting the turn engine.
  • Turn engine (_runTurnInner 209 LOC + runIteration 119) — the largest remaining mass and where ~4/5 of recent commits land, but a big diff on the hottest-churn path deserves its own review.
  • compactHistory extraction — looks like a seam, isn't: its logic already lives in compact.ts behind injected callbacks, so the remaining 58 lines are a thin adapter. Moving it would be file-hop, not decomposition.

…y loops

openai-compatible/query.ts hand-rolled the per-session abort slot that
anthropic-direct already encapsulates in AbortCoordinator, including six
copies of the compare-and-clear invariant:

    if (this.abortController === controller) this.abortController = null;

That invariant is safety-critical — a leaked stale controller makes
compact()'s idle guard misread "turn in flight" as idle and run
summarization mid-turn — and maintaining it by hand in two providers is
exactly the drift risk shared/tool-loop-cap.ts exists to prevent.

Move AbortCoordinator to providers/shared/ (it had zero imports and no
SDK types — it was already provider-neutral) and wire openai-compatible
through it. Four fields (abortController, pendingAbortReason,
closeResolve, closedPromise) collapse into one `abort` collaborator;
clear() becomes the only write path to null.

Net -26 lines of logic in query.ts, and the six duplicated invariant
sites become six calls. File length is ~flat because the freed space
went to documenting the contract at each site.

Also adds shared/abort-coordinator.test.ts (14 cases): scope lifecycle,
stale-scope compare-and-clear protection, pending-abort drain, and the
close-promise race the main loop depends on. The class was previously
covered only indirectly via concurrent-session-isolation.test.ts.

BEHAVIOURAL DELTA (one, intentional, convergent):
compactHistory's beginAbort now goes through AbortCoordinator.begin(),
which drains a pending abort reason. An interrupt() parked between turns
therefore pre-aborts the auto-compaction firing at that boundary and
consumes the reason, instead of letting compaction run and deferring the
abort to the next turn. This matches
anthropic-direct/query/compact-handler.ts:148. Documented inline at the
call site.

Part of #360 (god-class decomposition); follows #407, which extracted
client/model-params/retry from the same file.

Verification: pnpm lint clean, full suite 699 files / 13082 passed,
audit:sdk:check + audit:env:check + scan:env:check all green, pnpm build
succeeds.
@vercel

vercel Bot commented Jul 25, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agent-afk-docs Ready Ready Preview, Comment Jul 25, 2026 12:32am

@griffinwork40

Copy link
Copy Markdown
Owner Author

CI note: the Test (windows-latest) red is pre-existing, not from this PR

All gating checks pass — Test (ubuntu-latest) (the required gate) is SUCCESS, as are macOS, Lint & Build, Docs, Publish Bundle Smoke, and PTY.

windows-latest is advisory by design (ci.yml:94 sets continue-on-error for every non-ubuntu matrix OS, with the comment at ci.yml:83-89 noting the Windows-compat grind is in progress). Its failure does not block merge.

I verified it is not caused by this change:

main @ e71fbce (this PR's base) this PR
windows job conclusion failure failure
failing test files 85 86
failing tests 537 537

Same failing-test count, and the failures are the same pre-existing Windows-compat set — memory-loader.test.ts, skill-bridge.test.ts, bash.test.ts, tool-injector.test.ts, launchd.test.ts, etc. The two providers/ files that appear (anthropic-direct/grant-manager.test.ts, openai-compatible/auth.test.ts) also fail on main's Windows job and are untouched here.

Root cause of the sampled memory-loader.test.ts failure, for whoever picks up the compat work: the suite sets process.env['HOME'] to a tmp dir, but getAfkHome() falls back to join(homedir(), '.afk') (src/paths.ts:56) and Node's os.homedir() reads USERPROFILE on Windows — so the fixture directory is never consulted.

The file this PR adds passes on Windows: ✓ src/agent/providers/shared/abort-coordinator.test.ts (14 tests).

Worth flagging for future triage: a run-level conclusion of success on main does not mean the Windows job passed — continue-on-error masks it at the run level. The job-level conclusion has to be read directly.

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