Skip to content

Cap QA request size and abort the pipeline on client disconnect - #273

Open
Ayush7614 wants to merge 1 commit into
CopilotKit:mainfrom
Ayush7614:feat/qa-ingress-hardening
Open

Ayush7614 wants to merge 1 commit into
CopilotKit:mainfrom
Ayush7614:feat/qa-ingress-hardening

Conversation

@Ayush7614

Copy link
Copy Markdown

POST /api/qa passed question and conversationHistory straight into the Anthropic pipeline with no length, count, or role validation — a single request could stuff tens of thousands of tokens onto the model bill. It also ignored client disconnects, running generation to completion for nobody.

This PR:

  • Caps question at 4000 chars; history at 20 items, 4000 chars each, 12000 total; roles restricted to user/assistant. Violations return 400 naming the field.
  • Adds an exported sanitizeHistory helper (trims content, enforces all caps).
  • Listens to request.signal abort: destroys the pipeline and closes the stream controller (guarded against double-close).

Verification (all real, run locally):

  • apps/web/src/tests/qa-api.test.ts: 19/19 pass, including 9 new tests (overlong question, non-array/oversized/bad-role/empty/over-budget history, trim passthrough, abort destroys pipeline, sanitizeHistory units)
  • qa-chat-hook + qa-components suites: 24/24 pass (client untouched)
  • pnpm typecheck in apps/web: clean

@NathanTarbert NathanTarbert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation half of this is well built. The abort half does not do what the title says, and that is the main thing below.

Context first, because it explains a lot: CI has not run on any of your five PRs — fork contributions need a maintainer to approve the workflow runs, and that has not happened yet. I ran everything locally.

Blocker 1 — the build

apps/web/src/app/api/qa/route.ts exports sanitizeHistory and four MAX_* constants so the test can import them. Next's App Router only permits HTTP verbs and a fixed set of config fields, so pnpm build and pnpm typecheck both fail:

Type error: "sanitizeHistory" is not a valid Route export field.

pnpm test passes, because vitest imports the module directly and never applies that constraint — which is why this was invisible without CI.

Moving the constants, HistoryItem and sanitizeHistory into something like apps/web/src/lib/qa-limits.ts and importing from both the route and the test fixes it with no logic change. #274 has the same shape with an exported constant.

Blocker 2 — the abort does not reach the model

This one is worth walking through, because the wiring looks right:

  • the abort listener calls pipeline.destroy()
  • destroy() calls pathfinder.disconnect()
  • disconnect() calls reset()
  • reset() sets sessionId = null and sessionCreatedAt = 0

So it clears two fields on the retrieval client. Retrieval has already finished by the time generation is running, so it is a no-op even for Pathfinder. Meanwhile the spend happens in generator.ts at the messages.create call, which is not passed a signal — and PipelineOptions has no field to carry one.

Concretely: a user asks a question, waits three seconds, closes the tab. The local stream closes, and the model request runs to completion and bills the full output. Same cost as before.

The fix is to thread a signal through — add signal?: AbortSignal to PipelineOptions, pass it down to generate, and forward it to messages.create({...}, { signal }). Then the route passes request.signal.

Two honest caveats so this is not oversold once done. This route is not incrementally streamed — the pipeline returns a complete result and the route slices the finished string into chunks, so there is no partial-generation window to salvage on the route side. And aborting an already-dispatched non-streaming request frees the connection without necessarily avoiding the charge; the reliable win is capacity, not a refund.

Given the signal work touches packages/outpost/ai, splitting it out is reasonable — the caps are valuable on their own. If you would rather do that, narrowing the PR title to what it does today would make it accurate.

Worth fixing either way

After an abort, the late completion calls sendEvent on a closed controller and throws Invalid state: Controller is already closed. The catch handler then calls sendEvent again and throws the same thing, this time with no handler. The stream is already closed so the blast radius is small, but it makes the catch block unreachable for genuine errors that land after an abort. A if (settled) return; guard at the top of sendEvent removes the whole class.

What is good

The caps bind before the model call — validation runs, and new AIPipeline() comes after, so nothing reaches the provider before a 400 returns. Your tests assert not.toHaveBeenCalled() on each rejection path, which is the right assertion.

Role injection is closed: anything that is not user or assistant is rejected, so a caller cannot smuggle a system turn into the prompt. There is a test for it.

The limits are sane — 4000 characters is comfortably above a real support question, and 20 items / 12000 characters is a reasonable session, so nothing legitimate breaks.

And the tests are real. I mutation-tested four behaviours — removing the question cap, removing the abort listener, allowing system, and removing the history budget — and every one is caught. The only qualifier is that the abort test asserts destroy() was called, so it pins the current wiring rather than the outcome; it would keep passing after the signal work, and would also pass if destroy() stayed a no-op.

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