Repository navigation
fix(tests): settle dsh-native attach chains before teardown (#1791) - #1798
Merged
Merged
Conversation
Contributor
📦 Built Package ArtifactBranch: Option A — Install this exact build (recommended)npm install -g billion-context@0.0.1771-pr.1798.381c965This exact version is immutable and reproducible. The rolling Option B — Download artifact
tar xzf billion-context-pr1798.tgz
npm install -g packageInstall instructions are refreshed on each push. |
Owner
Author
[bot] 🏷 PR #1798 review 结论(结构化七问) 已拉取分支逐行核对代码、对照源码验证根因链、独立复跑负载测试。CI 12/12 全绿(含 ubuntu/windows × node 22/24 四个 test 矩阵 + codex/opencode/pi/dsh 四条 e2e lane + build-artifact)。
协调性:#1793 与本 PR 同文件但区域不相交(waitFor 上限 L412-418 一带 + 两处 #1365 循环),两者语义正交(拉长轮询预算 vs teardown 前排空),任意合并顺序均干净。分支基于当前 master(仅领先一个提交),diff 每行都服务于本 PR 目的,整洁。 判定:可合并。 无需补 commit。 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What & why
Pre-existing file-level flakiness in
tests/dsh-native.test.ts, unrelated to #1785: intermittent reds rotating between#983 maybeRetry self-heals a base-less register after a failed respawnandapply() runtime-info (#956): a mid-resolve model switch discards the stale resolve, plus one windows/22 CI occurrence (check-run109983788889) with the fast throwbili: proxy is down — recovery in progress, retry shortlyattests/dsh-native.test.ts:1028.Root cause (reproduced locally with a timestamped console trace): the
#1772 web-profile caveattest fires four attach chains throughapply()and tears down (proxy.close()) without settling any of them. The deferred health probes then hit the closed mock → ECONNREFUSED →verifyAttachAndRecoverruns its spawn fallback inside later tests' bodies (that test installs no spawn stub, so it reaches the realbootstrap()and spawns actual bili child processes, which exit code 1) and clobbers the module-sharedregister.base/register.toolsReady/state.attach/ preset env mid-test. Whichever test's early window the clobber lands in goes red — hence the rotating failures. This is the same pollution class the #1117 test's house comment already documents ("a deferred probe hitting a closed proxy would run the fallback mid-way through a LATER apply() … sequence pollution exposed when #1130's suites follow this one"); #1772 was added later without that discipline.#956itself has no internal race — its assertion sequence is microtask-deterministic by static analysis (the stale flight A's discard chain is pure microtasks and drains before the 20 ms timer; flight B cannot start until a laterheadersForpoll). Its reds are the upstream clobber landing in its window. So: test-hygiene defect, not a production race —src/is untouched.Fix (test file only)
dsh.bundle.patch.yml禁用自动压缩在 web profile 下静默失效 #1772: hermetic fallback seam —_setSpawnForTest(async () => undefined)(cleared infinally) so a deferred probe can never reach a real bootstrap child process; and settle each of the four apply chains while the mock is still open:await _stateToolsReadyForTest()after each phase, with_resetRegisterForTest(proxy.origin)before phases 2/4 so each [Enhancement] E2E native lane: real dsh + bili profile vs deterministic fake upstream (from #1239) #1268 gate strictly waits its own chain (otherwise the previous chain'stoolsReady=truelets the next gate resolve immediately and leaves it dangling again). All original assertions unchanged.apply() /acp-cache (#1146), the threeapply() /acp (#1677)variants, andapply() /acp pre-first-request (#955).sleep(50)becomes settle-based — a counting stub plusawait waitFor(() => spawnCalls >= 1, "initial failed respawn settled"). The stub body is synchronous, so once the counter is observable by a macrotask the base-less state (landed.then(base=undefined)) has already drained as microtasks and the single-flight is cleared; the subsequentheadersFordeterministically takes the maybeRetry respawn branch. This closes the secondary timeout variant where probe + 30 ms routed-evidence grace + jitter exceeded 50 ms under load.No production behavior change:
src/untouched, every existing assertion byte-identical.Evidence
not ok 29 - #983 maybeRetry self-heals…, errorbili: proxy is down — recovery in progress, retry shortly, stackObject.execute (src/agent/dsh-native.ts:348)←tests/dsh-native.test.ts:1028, duration_ms 88.attach target http://127.0.0.1:<port> is not healthy — falling back to a spawned proxylines (one perdsh.bundle.patch.yml禁用自动压缩在 web profile 下静默失效 #1772 apply) landing after the two following tests had already finished, followed by theproxy bootstrap failed — model traffic goes direct (uncompressed)line inside the #983c window.npm run typecheckclean; fullnpm test: 3450 pass / 0 fail / 19 skipped (of 3469);npm run build: success.Coordination
Open PR #1793 edits the same file but only disjoint regions (the
waitFordeadline near L407–418 and the two #1365 loops near L1386/L1433). No textual overlap with this change; semantically additive (longer poll deadlines + settled chains). Either merge order is clean.config surface
None — test-only change; no config fields, env vars, or CLI flags added, renamed, or re-semantics'd.
Fixes #1791