Repository navigation
fix(planner): never retry a planner run on the port it just retired - #676
Merged
Merged
Conversation
added 2 commits
October 26, 2026 17:53
… tsc The proof broke the whole-project strict typecheck (58 errors), and CI stops at that Typecheck step before smoke and the full regression, so every PR on main, this one included, lost its regression signal. - Declare the surface the proof drives on the undeclared JS adapter (dependencies, stream options, model info) and type emitted chunks with the harness's own StreamChunk union from @deepseek-ai/dsh-llm. - Type the mock server (RequestListener, ServerResponse, AddressInfo narrowing) and the wire request bodies; make adapterFor's captured argument optional. - Narrow chunks and the tool-call block through small helpers instead of unchecked property reads, and read error codes from unknown via one guard. - Every assertion is unchanged. Fixes #672 Constraint: Test file only; no suppression directive, no src, tsconfig or package change. Rejected: A .d.ts beside dsh-llm-gateway-chat.js | widens the change into src and the shipped package. Confidence: high Scope-risk: narrow Tested: Node v26.9.0: cd ts && npx tsc --noEmit exit 0 (was exit 2, 58 errors); adapter proof exit 0 before and after; smoke.ts exit 0; mutation check: a misspelled chunk field and an un-narrowed tool-call read both fail tsc. Not-tested: full run-all locally; CI runs it on Node 22/24 once Typecheck passes. (cherry picked from commit 3acb2a0)
#669's boot retry, like the older soft-stall retry, retires the run port and asks the task's factory for a fresh one. A factory that hands back one shared instance returns the port just closed; its run() then threw booking_planner_run_port_closed, which no catch recognised, so next() rejected untyped. The DSH readiness proof uses exactly such a factory, and main's full regression has failed on it since 6599423 on Node 22 and 24. - Both fresh-port retry branches refuse a port this planner already retired (closingPorts) and give up with the outcome they use when the deadline cannot fund a retry: PLANNER_BOOT_TIMEOUT for the boot branch; boot timeout or PLANNER_PROVIDER_TIMEOUT for the stall branch. Factories that return a new port, as production does, behave exactly as before. - Readiness proof: the boot-budget case asserts the refused retry under a 24s turn budget that funds it with margin on a slow host; a new real-subprocess phase proves the retry boots a fresh runtime, reaps the first, and reaches the provider exactly once. - Planner proof: one deterministic case per retry branch, with fakes that reject like the real port once closed. - copilot-readiness-report (en + zh-CN) records the rule and both scenarios. Refs #660 Constraint: No behavior change for factories that return a new port; no kernel-pinned file touched. Rejected: Accept the untyped rejection in the proof | it hides a planner contract break. Rejected: Cap the boot retry at one attempt | #669's comment and loop disagree on the intended count; left to the planner owner. Confidence: high Scope-risk: narrow Tested: Node v26.9.0: tsc --noEmit exit 0; all 22 ts suites of run-all §49 exit 0, including readiness (refused retry settled 6091 ms; retry case 15577 ms, 2 runtimes, 1 provider request) and planner proofs; mutation: main's planner fails both proofs with booking_planner_run_port_closed, and removing only the stall guard fails the stall case. Not-tested: full run-all locally (CI runs it on Node 22/24); build-dist and package proof not run outside the bin proof.
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.
Refs #660 · Fixes #672
main's CI "Full-stack regression (§1-§49)" has failed on Node 22 and 24 since6599423(#669, "retry once on a fresh run port after a cold boot timeout"). From #670 on, the earlier Typecheck step failed first (#672), which hid it. This PR fixes the failure at its source.What broke
ts/scripts/booking-copilot-dsh-readiness-proof.tsbuilds its boot-budget planner withrunPortFactory: () => port!, one shared port. #669's boot retry callsretire(runPort), which closes the task-owned port, and then asks the factory for a fresh port. The factory hands back the port that was just closed. Itsrun()throwsbooking_planner_run_port_closed, which no catch recognises, so the planner'snext()rejected untyped instead of returning a typed result:The older soft-stall retry has the same retire-then-refetch shape and the same exposure.
The fix (
ts/src/booking-surface/dsh-planner.ts, +15/−2)A fresh-port retry never runs on a port this planner has already retired (
closingPorts.has(port)). Both retry branches now treat a retired port like a deadline that cannot fund a retry, and settle with the outcome each already uses in that case:PLANNER_BOOT_TIMEOUTfor the boot branch; boot timeout orPLANNER_PROVIDER_TIMEOUTfor the stall branch. Factories that return a new port, as production does, behave exactly as before.Proofs
boot_retry_fresh_port, proves fix(planner): retry once on a fresh run port after a cold boot timeout #669's retry works: the first runtime stays blocked before initialize, the second is released, the turn retries on the fresh runtime, makes exactly one provider request, and settles asPLANNER_PROVIDER_TIMEOUT. The first runtime is reaped before the second starts.docs/evaluation/copilot-readiness-report.mdand its zh-CN mirror describe the new rule and both scenarios.Also in this PR: the #672 strict-typecheck fix (commit
00f5b15, cherry-picked from #675's3acb2a0)Without it, CI stops at Typecheck and never reaches the regression this PR fixes. It is the same change as in #675, so whichever PR lands first makes the other's copy a no-op.
Evidence (local, Node v26.9.0, macOS)
da99c5e:cd ts && npx tsx scripts/booking-copilot-dsh-readiness-proof.tsexits 1 with the CI error above.cd ts && npx tsc --noEmit: exit 0.ts/suites thatrun-all-tests.sh§49 runs: exit 0. They include the readiness, planner, dsh core, warmer, bin, server and managed-dsh proofs.PLANNER_BOOT_TIMEOUTin 6091 ms. The retry case retried after 6165 ms and settled asPLANNER_PROVIDER_TIMEOUTin 15577 ms, with 2 runtimes, 1 provider request and both trees reaped.main'sdsh-planner.ts, the planner proof fails on the new boot case and the readiness proof fails atboot_budget_bound, both withbooking_planner_run_port_closed. Removing only the stall-branch guard fails the new stall case.node scripts/check-docs-i18n.mjsandnode scripts/check-doc-readability.mjs: exit 0; the report's links resolve.run-all-tests.sh(CI runs it on Node 22 and 24), andbuild-distor the package proof outside the bin proof.Open question for the planner owner (not changed here). #669's comment says "one fresh-port retry converges and only a genuinely stuck runtime burns both", but the boot branch retries while at least 12000 ms remain, so a long turn deadline can boot a stuck runtime more than twice. Is that intended?