-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(serve): Bound ACP HTTP pre-attach buffers by bytes #9007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
2675d34
fix(acp): Account for JSON string escaping in response budgets
doudouOUC ca5cbc7
fix(serve): Bound ACP HTTP pre-attach buffers by bytes
doudouOUC b52fef9
fix(serve): Preserve ACP pre-attach stream scope
doudouOUC ee405e9
fix(serve): Expose ACP guard failures by workspace
doudouOUC 6390b6b
codex: address PR review feedback (#9007)
doudouOUC fdf0042
codex: address PR review feedback (#9007)
doudouOUC f7c7353
codex: address PR review feedback (#9007)
doudouOUC 5a780e5
fix(serve): Preserve ambiguous WebSocket deliveries
doudouOUC c1399db
fix(serve): Report failed ACP response delivery
doudouOUC 5ebe9fb
codex: address PR review feedback (#9007)
doudouOUC cba4ee9
codex: address PR review feedback (#9007)
doudouOUC 2d38291
codex: address PR review feedback (#9007)
doudouOUC 531ef36
codex: address PR review feedback (#9007)
doudouOUC c691733
codex: address PR review feedback (#9007)
doudouOUC bcccd37
fix(cli): make websocket teardown logging safe
doudouOUC e7ea3b2
test(serve): align ACP fork fixtures after rebase
doudouOUC File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
31 changes: 31 additions & 0 deletions
31
.qwen/e2e-tests/2026-08-12-daemon-acp-http-pre-attach-bounds.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # Daemon ACP HTTP pre-attach bounds | ||
|
|
||
| ## Scope | ||
|
|
||
| Verify that connection/session responses produced before an ACP HTTP SSE or WebSocket owner is ready are bounded by serialized bytes and frame count across every workspace mount. The test does not claim to bound ordinary live transport queues or transient `JSON.stringify` amplification. | ||
|
|
||
| ## Baseline | ||
|
|
||
| Run the harness against the parent of this change. Initialize one ACP HTTP connection without attaching its response stream, then make the fake bridge produce 128 distinct 1 MiB results. Confirm retained heap/RSS grows with every payload and that the connection remains registered. Repeat with primary and dynamic workspace connections to confirm their retained buffers add together without a daemon-global boundary. | ||
|
|
||
| ## Verification | ||
|
|
||
| 1. Start `qwen serve` with ACP HTTP enabled, one primary workspace, and one dynamically registered trusted workspace. | ||
| 2. For each workspace, initialize a logical connection but delay its connection/session stream attachment. | ||
| 3. Produce distinct large responses until the per-connection 64 MiB boundary is crossed. Expect only the admitting connection to close; a shared WebSocket must receive close code 1013. Confirm the other workspace can still initialize, open a stream, and complete a small request. | ||
| 4. With several connections below their individual limits, compete for the shared 4,096-frame/256-MiB budget. Expect the connection attempting the global N+1 admission to close without evicting frames from another connection. | ||
| 5. Attach a deliberately stalled SSE writer after frames are buffered. Confirm status moves the frames from buffered to pending delivery while `usedFrames` and `usedBytes` remain charged. Close the socket, settle the write, and confirm all counters return to the pre-test baseline. | ||
| 6. Buffer several successful `session/new`, `session/load`, `session/resume`, or `session/fork` results, then close or overflow the connection before delivery. Confirm fresh sessions and persisted forks are removed, newly attached clients are detached, existing ownership remains intact, and none of the provisional sessions accept a prompt before response delivery. | ||
|
doudouOUC marked this conversation as resolved.
|
||
| 7. Send notification forms of `session/new`, `session/load`, `session/resume`, and `session/fork`. Confirm no session is created, restored, attached, or forked. | ||
| 8. Read `GET /daemon/status?detail=full`. Verify fixed limits, global current/high-water count and bytes, pending-delivery frames, guard failures, per-mount failure attribution, and per-connection owned count/bytes. | ||
| 9. Remove the dynamic workspace and close all test connections. Confirm global budget usage returns to the primary baseline. | ||
|
|
||
| ## Commands | ||
|
|
||
| ```bash | ||
| (cd packages/acp-bridge && npx vitest run src/bridge.test.ts src/spawnChannel.test.ts) | ||
|
doudouOUC marked this conversation as resolved.
|
||
| (cd packages/cli && npx vitest run src/serve/acp-http/pre-attach-budget.test.ts src/serve/acp-http/connection-registry.test.ts src/serve/acp-http/sse-stream.test.ts src/serve/acp-http/ws-stream.test.ts src/serve/acp-http/transport.test.ts src/serve/daemon-status.test.ts) | ||
| (cd packages/sdk-typescript && npx vitest run test/unit/daemon-public-surface.test.ts) | ||
| npm run build && npm run typecheck && npm run lint | ||
| git diff --check | ||
| ``` | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { describe, expect, it, vi } from 'vitest'; | ||
| import { estimateJsonStringBytes } from './json-string-bytes.js'; | ||
|
|
||
| describe('estimateJsonStringBytes', () => { | ||
| it('matches JSON.stringify UTF-8 bytes for every UTF-16 code unit', () => { | ||
| for (let code = 0; code <= 0xffff; code++) { | ||
| const value = String.fromCharCode(code); | ||
|
doudouOUC marked this conversation as resolved.
|
||
| expect(estimateJsonStringBytes(value, Number.MAX_SAFE_INTEGER)).toBe( | ||
| Buffer.byteLength(JSON.stringify(value)), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it('matches JSON.stringify for paired surrogates and mixed escaping', () => { | ||
| const samples = [ | ||
| '"\\\b\f\n\r\t', | ||
| '\u0000\u001f', | ||
| '\ud800', | ||
| '\udc00', | ||
| '\ud83d\ude00', | ||
| 'ASCII é 中 \ud83d\ude00 \ud800', | ||
| ]; | ||
| for (const value of samples) { | ||
| expect(estimateJsonStringBytes(value, Number.MAX_SAFE_INTEGER)).toBe( | ||
| Buffer.byteLength(JSON.stringify(value)), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it('matches JSON.stringify for deterministic random strings', () => { | ||
| let state = 0x5eed1234; | ||
| const nextCodeUnit = () => { | ||
| state = (Math.imul(state, 1664525) + 1013904223) >>> 0; | ||
| return state & 0xffff; | ||
| }; | ||
| for (let sample = 0; sample < 1000; sample++) { | ||
| const length = nextCodeUnit() % 128; | ||
| let value = ''; | ||
| for (let index = 0; index < length; index++) { | ||
| value += String.fromCharCode(nextCodeUnit()); | ||
| } | ||
| expect(estimateJsonStringBytes(value, Number.MAX_SAFE_INTEGER)).toBe( | ||
| Buffer.byteLength(JSON.stringify(value)), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it('returns limit + 1 as soon as the encoded string exceeds the limit', () => { | ||
| expect(estimateJsonStringBytes('\u0001'.repeat(100), 20)).toBe(21); | ||
| }); | ||
|
|
||
| it('uses native byte counting for large strings that need no escaping', () => { | ||
| const charCodeAt = vi.spyOn(String.prototype, 'charCodeAt'); | ||
|
doudouOUC marked this conversation as resolved.
|
||
| try { | ||
| const value = 'x'.repeat(8 * 1024 * 1024); | ||
| expect(estimateJsonStringBytes(value, 1024)).toBe(1025); | ||
| expect(estimateJsonStringBytes(value, Number.MAX_SAFE_INTEGER)).toBe( | ||
| value.length + 2, | ||
| ); | ||
| expect(charCodeAt).not.toHaveBeenCalled(); | ||
| } finally { | ||
| charCodeAt.mockRestore(); | ||
| } | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| export function estimateJsonStringBytes( | ||
|
doudouOUC marked this conversation as resolved.
|
||
| value: string, | ||
| limitBytes: number, | ||
| ): number { | ||
| const unescapedBytes = Buffer.byteLength(value, 'utf8') + 2; | ||
| if (unescapedBytes > limitBytes) return limitBytes + 1; | ||
| if (!/["\\]|[^ -\ud7ff\ue000-\uffff]/u.test(value)) { | ||
| return unescapedBytes; | ||
| } | ||
| let bytes = 2; | ||
| for (let index = 0; index < value.length; index++) { | ||
| const code = value.charCodeAt(index); | ||
| if (code === 0x22 || code === 0x5c) { | ||
| bytes += 2; | ||
| } else if (code <= 0x1f) { | ||
| bytes += | ||
| code === 0x08 || | ||
| code === 0x09 || | ||
| code === 0x0a || | ||
| code === 0x0c || | ||
| code === 0x0d | ||
| ? 2 | ||
| : 6; | ||
| } else if (code >= 0xd800 && code <= 0xdbff) { | ||
| const next = value.charCodeAt(index + 1); | ||
| if (next >= 0xdc00 && next <= 0xdfff) { | ||
| bytes += 4; | ||
| index++; | ||
| } else { | ||
| bytes += 6; | ||
| } | ||
| } else if (code >= 0xdc00 && code <= 0xdfff) { | ||
| bytes += 6; | ||
| } else if (code <= 0x7f) { | ||
| bytes++; | ||
| } else if (code <= 0x7ff) { | ||
| bytes += 2; | ||
| } else { | ||
| bytes += 3; | ||
| } | ||
| if (bytes > limitBytes) return limitBytes + 1; | ||
| } | ||
| return bytes; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.