fix(coordinator): harden IFC intake + graceful dispose + 移除 _bim-control 死碼 (CH-2: #9 #7 #19) - #142
Conversation
CH-2 收 coordinator 3 風險(#9 IFC strict 接線 / #7 graceful dispose signal / #19 退役 _bim-control 死碼)。proposal/design/tasks + spec delta(local-coordinator-ifc-ready-intake-boundary ADD 1、project-risks-mitigation ADD 1;#19 tasks-only);openspec validate --strict 通過。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rol 死碼 (#9 #7 #19) CH-2 apply: - #9 IFC strict 接線: config 加 ifcDownloadStrict(parseBooleanEnv IFC_DOWNLOAD_STRICT default false);app downloadIfcToSharedVolume 加 fallbackOnFetchError=!strict;compose 加 IFC_DOWNLOAD_STRICT(production 設 true→non-2xx 回 502 不靜默 placeholder) - #7 graceful dispose: index.ts 接 SIGTERM/SIGINT → shutdown(dispose drain→server.close→io.close→exit 0),補實作 RISK-IN-MEMORY-QUEUE-PERSISTENCE graceful shutdown - #19 退役死碼: 刪 BimControlClient + safeArtifacts + config bimControlApiBase + compose BIM_CONTROL_API_BASE 死 env;保留 Artifact type 與簽章;清 11 個 test makeApp 驗證: npm run verify 260 passed(baseline 257+3 新,tsc 0 error); root pytest 65 passed(零回歸)。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 44 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR hardens the coordinator with three coordinated changes: configuration-driven IFC download strictness that returns 502 on non-2xx HTTP responses (vs. silent placeholder fallback), graceful shutdown signal wiring that drains the in-memory conversion queue on SIGTERM/SIGINT, and removal of the deprecated BimControlClient artifact-fetching service. ChangesCoordinator hardening: strict IFC downloads, graceful shutdown, artifact removal
Sequence Diagram(s)sequenceDiagram
participant index.ts as Node Process
participant handler as Signal Handler
participant shutdown as createGracefulShutdown
participant dispose as dispose()
participant io as io.close
participant server as server.close
participant exit as process.exit
index.ts->>handler: register SIGTERM/SIGINT
Note over handler: signal received
handler->>shutdown: invoke shutdown function
shutdown->>dispose: await dispose()
dispose-->>shutdown: queue drained, jobs marked
shutdown->>io: io.close(callback)
io->>server: callback triggers
server->>server: server.close(callback)
server->>exit: call exit(0)
exit-->>shutdown: process terminates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7b41a3e08
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # false(此 dev default):fetch 失敗時 fallback 寫 placeholder,demo loop 不中斷。 | ||
| # production MUST 設 "true":strict 下 HTTP 拿不到 IFC(non-2xx)直接回 502 + | ||
| # download_status=failed,不靜默產 placeholder,避免假資料流入下游 conversion。 | ||
| IFC_DOWNLOAD_STRICT: "false" |
There was a problem hiding this comment.
In the compose-based deployment this literal value overrides anything supplied via .env/--env-file, so the production instruction immediately above cannot actually be applied with the documented compose workflow. When an operator sets IFC_DOWNLOAD_STRICT=true, the coordinator container still receives false and continues the placeholder fallback on non-2xx IFC downloads; use an interpolated default such as ${IFC_DOWNLOAD_STRICT:-false} here and in the host-kit override so production can opt in.
Useful? React with 👍 / 👎.
| storageRoot: config.storageRoot, | ||
| storageHostRoot: config.storageHostRoot, | ||
| timeoutMs: config.ifcDownloadTimeoutSeconds * 1000, | ||
| fallbackOnFetchError: !config.ifcDownloadStrict, |
There was a problem hiding this comment.
When strict mode is enabled for production but the incoming source_ifc.ref uses the contract’s minio://... style reference, this new flag still does not prevent placeholder dispatch: downloadIfcToSharedVolume returns placeholder success for every non-http(s) URL before consulting fallbackOnFetchError. That means strict intake can still mark the job downloaded and enqueue conversion with no real IFC bytes, so the strict path needs to reject or resolve non-http schemes instead of only covering HTTP fetch failures.
Useful? React with 👍 / 👎.
| }); | ||
|
|
||
| const shutdown = async (): Promise<void> => { | ||
| await dispose(); |
There was a problem hiding this comment.
If SIGTERM/SIGINT arrives while an /api/external/ifc-ready request is already accepted and still downloading, this calls dispose() before server.close(), so the queue is drained before that request can enqueue its conversion job. The request can then finish and enqueue after the only drain pass, and the subsequent process.exit(0) can drop that job without marking it dropped_on_restart; stop accepting/wait for active intake (or block enqueue) before the final drain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR (CH-2 risk-triage change set) hardens bim-review-coordinator by (1) wiring an explicit strict mode for IFC intake downloads, (2) wiring graceful shutdown to process signals, and (3) retiring dead _bim-control client code + config/env plumbing, while keeping the Artifact types/signatures intact.
Changes:
- Add
IFC_DOWNLOAD_STRICTconfig wiring and passfallbackOnFetchError: !ifcDownloadStrictinto the IFC downloader; add tests covering strict 502 + no-dispatch. - Add SIGTERM/SIGINT shutdown wiring in
src/index.tsto trigger queue drain/dispose behavior and close network listeners. - Remove retired
_bim-controlclient code paths (client module, config field, compose env var) and adjust coordinator tests accordingly.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| openspec/changes/harden-coordinator-ifc-intake/tasks.md | Adds the OpenSpec task checklist for this change. |
| openspec/changes/harden-coordinator-ifc-intake/proposal.md | Documents motivation and intended behavioral changes (#9/#7/#19). |
| openspec/changes/harden-coordinator-ifc-intake/design.md | Captures decisions + validation strategy for strict IFC intake and shutdown wiring. |
| openspec/changes/harden-coordinator-ifc-intake/specs/local-coordinator-ifc-ready-intake-boundary/spec.md | Adds strict-mode requirement for IFC-ready intake to return 502 on non-2xx fetch. |
| openspec/changes/harden-coordinator-ifc-intake/specs/project-risks-mitigation/spec.md | Adds signal-wired graceful shutdown requirement for the in-memory queue risk. |
| compose.runtime-manager.yml | Removes BIM_CONTROL_API_BASE; adds IFC_DOWNLOAD_STRICT with guidance comments. |
| compose.host-kit.yml | Adds IFC_DOWNLOAD_STRICT with guidance comments. |
| bim-review-coordinator/src/config.ts | Adds ifcDownloadStrict env parsing; removes bimControlApiBase. |
| bim-review-coordinator/src/app.ts | Removes _bim-control usage; wires downloader strictness; stream-config now passes empty artifacts. |
| bim-review-coordinator/src/index.ts | Adds SIGTERM/SIGINT shutdown handler. |
| bim-review-coordinator/src/services/bimControlClient.ts | Deletes retired _bim-control client implementation. |
| bim-review-coordinator/tests/config.test.ts | Adds env coverage for IFC_DOWNLOAD_STRICT parsing. |
| bim-review-coordinator/tests/external-ifc-ready.test.ts | Adds strict-mode 502 + no-dispatch test with an HTTP non-2xx IFC source stub. |
| bim-review-coordinator/tests/* (multiple) | Removes now-deleted bimControlApiBase from app config fixtures; updates defaults. |
Comments suppressed due to low confidence (1)
bim-review-coordinator/src/app.ts:405
- After removing
BimControlClient, this handler now hardcodesconst artifacts: Artifact[] = [];, sochooseReadyUsdc(artifacts)is guaranteed to returnundefined. KeepingreadyUsdchere is misleading and makes it look like there is still an automatic USDC selection path when there isn't.
const readyUsdc = chooseReadyUsdc(artifacts);
const artifactBindings = buildArtifactBindings(input.model_version_id, artifacts, input.artifact_bindings, input.routing_policy);
const kitInstanceBindings = allocateKitInstanceBindings(
config,
artifactBindings,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const shutdown = async (): Promise<void> => { | ||
| await dispose(); | ||
| server.close(() => { | ||
| io.close(() => { | ||
| process.exit(0); | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| process.on("SIGTERM", shutdown); | ||
| process.on("SIGINT", shutdown); |
| ### Requirement: Graceful shutdown SHALL be wired to process termination signals | ||
|
|
||
| `bim-review-coordinator` 進程 SHALL 在收到 `SIGTERM` 或 `SIGINT` 時執行 graceful shutdown,呼叫已實作的 dispose 流程(`ConversionDispatchQueue.drain()` 把 queued job 標 `dropped_on_restart`、取消 in-flight poller、關閉 HTTP server 與 Socket.IO),再以正常退出碼結束。此為 `RISK-IN-MEMORY-QUEUE-PERSISTENCE` 既有 graceful-shutdown-drain requirement 的接線實作;dispose 本體行為 SHALL NOT 改變,in-flight job SHALL NOT 被中斷(drain 只移除尚未起跑的 queued job)。 | ||
|
|
||
| #### Scenario: SIGTERM or SIGINT triggers graceful dispose | ||
|
|
||
| - **WHEN** coordinator 進程在有 queued 轉檔 job 時收到 `SIGTERM` 或 `SIGINT` | ||
| - **THEN** 進程 SHALL 執行 dispose:queued job 透過 `ConversionDispatchQueue.drain()` 標為 `dropped_on_restart` | ||
| - **AND** SHALL 關閉 HTTP server 與 Socket.IO | ||
| - **AND** SHALL 以正常退出碼(`0`)結束 | ||
| - **AND** in-flight job SHALL NOT 被中斷 | ||
|
|
||
| #### Scenario: Dispose body remains the single graceful-shutdown path | ||
|
|
||
| - **WHEN** 本 change 接上 signal handler | ||
| - **THEN** 既有 `dispose()` 的 drain / mark / poller-cancel / server-close 行為 SHALL NOT 被改變 | ||
| - **AND** signal handler SHALL 僅作為觸發既有 dispose 的進入點,不複製或分岔 shutdown 邏輯 |
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
opus 對抗式 review 4 major: - [major] #7 shutdown 死鎖: io.close 嵌在 server.close callback 內,Socket.IO keep-alive 連線讓 server.close callback 永不觸發 → exit(0) 到不了。抽 createGracefulShutdown(src/shutdown.ts)修正順序 io→server + 可注入 deps;補 shutdown.test.ts 3 unit - [major] #9 strict test 沒 pin 502 原因: 加 expect(reason=http_status),擋 fallback-mode regression 假綠 - [major] #9 non-strict fallback 零覆蓋: 補對稱測試(non-strict + http non-2xx → 202 + dispatch),鎖 strict 接線不破壞 demo loop - #7 signal handler 接線由 shutdown.test 直接覆蓋(取代只測 dispose 本體) 驗證: npm run verify 264 passed(260 + 4 新,tsc 0 error)。 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openspec/changes/harden-coordinator-ifc-intake/proposal.md (1)
1-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winproposal.md 需統一為繁體中文撰寫。
目前仍有多個英文主標與段落（例如 Line 1, Line 9, Line 17, Line 29），不符合 OpenSpec 變更文件語言規範。
As per coding guidelines, "
openspec/changes/!(archive)/**/*.md: MUST write proposal / design / tasks / spec in Traditional Chinese; preserve OpenSpec parser required headers ..."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/harden-coordinator-ifc-intake/proposal.md` around lines 1 - 52, The proposal uses several English section headers and paragraphs (e.g., "Why", "What Changes", "Capabilities", "Impact", "Non-goals") but the OpenSpec rule requires the entire proposal.md to be written in Traditional Chinese; update all English headings and any English paragraphs to equivalent Traditional Chinese text while preserving structure, content and required parser headers (do not remove or alter section semantics), ensuring terms like "Why", "What Changes", "Capabilities", "Impact", "Non-goals" and any inline English phrases are translated into Traditional Chinese consistently throughout the document.openspec/changes/harden-coordinator-ifc-intake/tasks.md (1)
1-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick wintasks.md 請改為全繁體中文任務敘述。
目前仍有多處英文章節名稱與詞彙（如 Line 1, Line 6, Line 28），不符 OpenSpec 對變更文件語言的要求。
As per coding guidelines, "
openspec/changes/!(archive)/**/*.md: MUST write proposal / design / tasks / spec in Traditional Chinese; preserve OpenSpec parser required headers ..."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/harden-coordinator-ifc-intake/tasks.md` around lines 1 - 33, 文件 openspec/changes/harden-coordinator-ifc-intake/tasks.md 包含多處英文標題與詞彙（例如 "OpenSpec artifacts", "IFC strict", "graceful dispose" 等），違反必須以繁體中文撰寫變更任務的規範；請將整份檔案的所有英文字串與章節標題翻譯為繁體中文（例如把 "OpenSpec artifacts" 改為「OpenSpec 工件/產出」、"IFC strict" 改為「IFC 嚴格模式」、"graceful dispose" 改為「優雅釋放」等），同時保留原有的 OpenSpec 解析器所需的標頭格式與任務清單結構（例如保留 "##" 標題、項目符號與代號如 `local-coordinator-ifc-ready-intake-boundary`、`CoordinatorConfig.ifcDownloadStrict`、`downloadIfcToSharedVolume`、`createCoordinatorApp` 等識別符），確保中文內容語意一致並保存所有程式與設定項的原始識別符號不被改動。
🧹 Nitpick comments (1)
bim-review-coordinator/tests/external-ifc-ready.test.ts (1)
345-372: 💤 Low valueConsider using
waitForDispatchEndfor more robust verification.The 50ms sleep at line 370 followed by the
streaming.bodies.lengthcheck could be made more deterministic by awaitingwaitForDispatchEnd(app, res.body.ifc_ready_job_id, ["dispatched"])before asserting, similar to the pattern used elsewhere in this test file. This would ensure the dispatch has fully completed rather than relying on a fixed delay.However, the current approach is acceptable since the dispatch queue is in-memory and processes synchronously.
♻️ Optional: More robust dispatch verification
- // fallback 走 placeholder → 照常 enqueue dispatch；給一小段時間讓 in-process queue 派工。 - await new Promise((resolve) => setTimeout(resolve, 50)); + // fallback 走 placeholder → 照常 enqueue dispatch；等 worker 推進到 dispatched。 + await waitForDispatchEnd(app, res.body.ifc_ready_job_id as string, ["dispatched"]); expect(streaming.bodies.length).toBeGreaterThan(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bim-review-coordinator/tests/external-ifc-ready.test.ts` around lines 345 - 372, Replace the ad-hoc 50ms sleep used to wait for dispatch with the deterministic helper: call await waitForDispatchEnd(app, res.body.ifc_ready_job_id, ["dispatched"]) after receiving the response and before asserting streaming.bodies.length; this uses the existing waitForDispatchEnd helper to wait for the job (res.body.ifc_ready_job_id) to reach the "dispatched" state on the test app instance instead of relying on setTimeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bim-review-coordinator/src/shutdown.ts`:
- Around line 19-26: The shutdown function currently awaits deps.dispose()
unguarded so a rejection aborts the rest of shutdown; wrap the dispose call in a
try/catch (or attach .catch) to capture and log any error from deps.dispose()
but always proceed to call deps.io.close, deps.server.close and finally
deps.exit(0); keep the existing close callback nesting (deps.io.close(() => {
deps.server.close(() => { deps.exit(0); }); })) so shutdown completes even if
dispose fails, and ensure the catch uses your logger or console to record the
dispose failure.
- Around line 21-25: The nested call to deps.server.close after deps.io.close is
redundant because Socket.IO (new Server(server,...)) closes the underlying HTTP
server; modify shutdown logic in shutdown.ts so deps.io.close(...) invokes
deps.exit(0) directly unless the HTTP server still needs closing—i.e., inside
the deps.io.close callback check if deps.server?.listening (or similar runtime
flag) and only call deps.server.close(...) when true, otherwise call
deps.exit(0) immediately; update uses of deps.io.close, deps.server.close, and
deps.exit accordingly.
In `@openspec/changes/harden-coordinator-ifc-intake/design.md`:
- Around line 1-32: 檔案內文與章節標題需全部改為繁體中文：把 "Context", "Goals / Non-Goals", "Key
Decisions", "Control flow / Source of truth", "Validation Strategy",
"Environment constraints" 等英文字串與段落說明翻成繁中，同時保留並不翻譯程式相關識別字串（例如
IFC_DOWNLOAD_STRICT、config.ifcDownloadStrict、ifcDownloader、fallbackOnFetchError、dispose()、index.ts、buildArtifactBindings、Artifact
type、BIM_CONTROL_API_BASE 等）及任何程式碼片段、環境變數/簽章與日期；保持 OpenSpec
解析器所需的標頭格式不變並確保語意與決策（#9/#7/#19）與驗證步驟說明完整對應原文內容。
In
`@openspec/changes/harden-coordinator-ifc-intake/specs/local-coordinator-ifc-ready-intake-boundary/spec.md`:
- Around line 1-5: 把檔頭的英文描述段落改成繁體中文，保留 OpenSpec 解析器必要的標頭原文不變；具體做法是在檔案中標題 "#
local-coordinator-ifc-ready-intake-boundary — Spec Delta
(harden-coordinator-ifc-intake)" 及緊接的英文描述（目前位於開頭 1-5 行）改寫為繁體中文敘述，同時確保 parser 標頭如
"## ADDED Requirements"、"## MODIFIED Requirements"、"### Requirement:"、"####
Scenario:" 等字串保持原文不變以免影響解析。
In
`@openspec/changes/harden-coordinator-ifc-intake/specs/project-risks-mitigation/spec.md`:
- Around line 1-5: 將檔案開頭英文標題與說明（目前第 1–5 行）改為繁體中文，並保留既有 OpenSpec
所需的標頭格式；具體來說，保留文件中的 `## ADDED Requirements`、`### Requirement:`、`#### Scenario:`
等欄位不變，並將檔案標題及前導描述（包含對 `RISK-IN-MEMORY-QUEUE-PERSISTENCE` 的說明、graceful shutdown 與
signal 接線說明）翻譯為繁體中文以符合路徑規範，同時確保原有語意與技術細節不變。
---
Outside diff comments:
In `@openspec/changes/harden-coordinator-ifc-intake/proposal.md`:
- Around line 1-52: The proposal uses several English section headers and
paragraphs (e.g., "Why", "What Changes", "Capabilities", "Impact", "Non-goals")
but the OpenSpec rule requires the entire proposal.md to be written in
Traditional Chinese; update all English headings and any English paragraphs to
equivalent Traditional Chinese text while preserving structure, content and
required parser headers (do not remove or alter section semantics), ensuring
terms like "Why", "What Changes", "Capabilities", "Impact", "Non-goals" and any
inline English phrases are translated into Traditional Chinese consistently
throughout the document.
In `@openspec/changes/harden-coordinator-ifc-intake/tasks.md`:
- Around line 1-33: 文件 openspec/changes/harden-coordinator-ifc-intake/tasks.md
包含多處英文標題與詞彙（例如 "OpenSpec artifacts", "IFC strict", "graceful dispose"
等），違反必須以繁體中文撰寫變更任務的規範；請將整份檔案的所有英文字串與章節標題翻譯為繁體中文（例如把 "OpenSpec artifacts"
改為「OpenSpec 工件/產出」、"IFC strict" 改為「IFC 嚴格模式」、"graceful dispose"
改為「優雅釋放」等），同時保留原有的 OpenSpec 解析器所需的標頭格式與任務清單結構（例如保留 "##" 標題、項目符號與代號如
`local-coordinator-ifc-ready-intake-boundary`、`CoordinatorConfig.ifcDownloadStrict`、`downloadIfcToSharedVolume`、`createCoordinatorApp`
等識別符），確保中文內容語意一致並保存所有程式與設定項的原始識別符號不被改動。
---
Nitpick comments:
In `@bim-review-coordinator/tests/external-ifc-ready.test.ts`:
- Around line 345-372: Replace the ad-hoc 50ms sleep used to wait for dispatch
with the deterministic helper: call await waitForDispatchEnd(app,
res.body.ifc_ready_job_id, ["dispatched"]) after receiving the response and
before asserting streaming.bodies.length; this uses the existing
waitForDispatchEnd helper to wait for the job (res.body.ifc_ready_job_id) to
reach the "dispatched" state on the test app instance instead of relying on
setTimeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04e9640d-1b01-4b31-bf00-d06bdb68deee
📒 Files selected for processing (25)
bim-review-coordinator/src/app.tsbim-review-coordinator/src/config.tsbim-review-coordinator/src/index.tsbim-review-coordinator/src/services/bimControlClient.tsbim-review-coordinator/src/shutdown.tsbim-review-coordinator/tests/app/viewerLogIntake.test.tsbim-review-coordinator/tests/auto-poll-conversion.test.tsbim-review-coordinator/tests/cloud-callback-outbox.test.tsbim-review-coordinator/tests/config.test.tsbim-review-coordinator/tests/conversion-dispatch-queue.test.tsbim-review-coordinator/tests/dev-console.test.tsbim-review-coordinator/tests/external-ifc-ready.test.tsbim-review-coordinator/tests/host-native-conversion-ingest.test.tsbim-review-coordinator/tests/local-web-view.test.tsbim-review-coordinator/tests/sessions.test.tsbim-review-coordinator/tests/shadow-metadata.test.tsbim-review-coordinator/tests/shutdown.test.tsbim-review-coordinator/tests/unit_kitpool.test.tscompose.host-kit.ymlcompose.runtime-manager.ymlopenspec/changes/harden-coordinator-ifc-intake/design.mdopenspec/changes/harden-coordinator-ifc-intake/proposal.mdopenspec/changes/harden-coordinator-ifc-intake/specs/local-coordinator-ifc-ready-intake-boundary/spec.mdopenspec/changes/harden-coordinator-ifc-intake/specs/project-risks-mitigation/spec.mdopenspec/changes/harden-coordinator-ifc-intake/tasks.md
💤 Files with no reviewable changes (10)
- bim-review-coordinator/tests/shadow-metadata.test.ts
- bim-review-coordinator/tests/app/viewerLogIntake.test.ts
- bim-review-coordinator/tests/local-web-view.test.ts
- bim-review-coordinator/tests/conversion-dispatch-queue.test.ts
- bim-review-coordinator/tests/cloud-callback-outbox.test.ts
- bim-review-coordinator/src/services/bimControlClient.ts
- bim-review-coordinator/tests/host-native-conversion-ingest.test.ts
- bim-review-coordinator/tests/dev-console.test.ts
- bim-review-coordinator/tests/sessions.test.ts
- bim-review-coordinator/tests/auto-poll-conversion.test.ts
| deps.io.close(() => { | ||
| deps.server.close(() => { | ||
| deps.exit(0); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether io is attached to the same http server (io.close would then close it)
rg -nP -C4 'new\s+Server\s*\(|io\s*=\s*|\.attach\s*\(|createServer' bim-review-coordinator/src/app.tsRepository: monkey1sai/AI-BIM-governance
Length of output: 396
🌐 Web query:
In socket.io 4.8.1, does Server.close() also close the underlying HTTP server it was attached to?
💡 Result:
In Socket.IO 4.8.1, calling Server.close does close the underlying HTTP server that it was attached to [1][2]. The Socket.IO server's close method is designed to perform a cleanup that includes both disconnecting all connected clients and closing the associated HTTP server [1][2]. This behavior is explicitly documented in the official Socket.IO Server API, which notes that it "closes the Socket.IO server and disconnects all clients" and "also closes the underlying HTTP server" [2]. If you need to manage the HTTP server separately (for instance, if you have other long-running connections or need a specific shutdown sequence), you should be aware that triggering Socket.IO's close method will automatically initiate the shutdown of the HTTP server [1]. Some developers have noted that this can lead to blocking behavior if the HTTP server waits for all active connections to finish before closing, which may impact graceful shutdown procedures in frameworks like NestJS [1].
Citations:
- 1: Socket.IO Adapter Close causes premature HTTP Server close() call and blocks application shutdown nestjs/nest#13910
- 2: https://socket.io/docs/v4/server-api/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shutdown.ts excerpt =="
sed -n '1,120p' bim-review-coordinator/src/shutdown.ts | nl -ba | sed -n '1,140p'
echo "== app.ts wiring around io/server =="
sed -n '240,340p' bim-review-coordinator/src/app.ts | nl -ba | sed -n '1,140p'Repository: monkey1sai/AI-BIM-governance
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shutdown.ts excerpt =="
sed -n '1,120p' bim-review-coordinator/src/shutdown.ts | nl -ba | sed -n '1,140p'
echo "== app.ts wiring around io/server =="
sed -n '240,340p' bim-review-coordinator/src/app.ts | nl -ba | sed -n '1,140p'Repository: monkey1sai/AI-BIM-governance
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shutdown.ts lines 1-80 =="
cat -n bim-review-coordinator/src/shutdown.ts | sed -n '1,120p'
echo "== app.ts wiring around io/server (270-320) =="
cat -n bim-review-coordinator/src/app.ts | sed -n '250,330p'Repository: monkey1sai/AI-BIM-governance
Length of output: 5116
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find createGracefulShutdown usage =="
rg -n "createGracefulShutdown|ShutdownDeps" -S bim-review-coordinator/src bim-review-coordinator/tests || true
echo "== Inspect shutdown call site (deps wiring) =="
rg -n "deps\.io\.close|deps\.server\.close|io:.*server" -S bim-review-coordinator/src || true
echo "== Find tests mentioning signal/dispose/shutdown =="
rg -n "SIGTERM|SIGINT|dispose\(\)|exit\(0\)|createGracefulShutdown" -S bim-review-coordinator/tests || true
echo "== List shutdown-related files =="
fd -e "ts" "shutdown" bim-review-coordinator | head -n 50Repository: monkey1sai/AI-BIM-governance
Length of output: 3017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find createGracefulShutdown usage =="
rg -n "createGracefulShutdown|ShutdownDeps" -S bim-review-coordinator/src bim-review-coordinator/tests || true
echo "== Inspect shutdown call site (deps wiring) =="
rg -n "deps\.io\.close|deps\.server\.close|io:.*server" -S bim-review-coordinator/src || true
echo "== Find tests mentioning signal/dispose/shutdown =="
rg -n "SIGTERM|SIGINT|dispose\(\)|exit\(0\)|createGracefulShutdown" -S bim-review-coordinator/tests || true
echo "== List shutdown-related files =="
fd -e "ts" "shutdown" bim-review-coordinator | head -n 50Repository: monkey1sai/AI-BIM-governance
Length of output: 3017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/index.ts (shutdown wiring) =="
sed -n '1,120p' bim-review-coordinator/src/index.ts | cat -n
echo "== tests/shutdown.test.ts =="
sed -n '1,120p' bim-review-coordinator/tests/shutdown.test.ts | cat -n
echo "== Any docs/README mention of shutdown order? =="
rg -n "io\.close|server\.close|createGracefulShutdown" -S bim-review-coordinator/docs bim-review-coordinator/tests bim-review-coordinator/src || trueRepository: monkey1sai/AI-BIM-governance
Length of output: 8166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/index.ts (shutdown wiring) =="
sed -n '1,120p' bim-review-coordinator/src/index.ts | cat -n
echo "== tests/shutdown.test.ts =="
sed -n '1,120p' bim-review-coordinator/tests/shutdown.test.ts | cat -n
echo "== Any docs/README mention of shutdown order? =="
rg -n "io\.close|server\.close|createGracefulShutdown" -S bim-review-coordinator/docs bim-review-coordinator/tests bim-review-coordinator/src || trueRepository: monkey1sai/AI-BIM-governance
Length of output: 8166
🌐 Web query:
Node.js http.Server.close() second call or when server not running ERR_SERVER_NOT_RUNNING callback
💡 Result:
In Node.js, calling server.close when a server is not currently running will result in an ERR_SERVER_NOT_RUNNING error [1]. If you pass a callback to server.close, and the server is not running (i.e., it has no internal handle), the error will be passed to that callback [1]. Specifically, the implementation ensures that if the server is not running, the callback is invoked with the ERR_SERVER_NOT_RUNNING error [1]. Key points regarding this behavior: - ERR_SERVER_NOT_RUNNING is a standard Node.js error code used across net.Server instances, including HTTP, HTTPS, and HTTP/2 servers [1]. - This error occurs if you attempt to close a server that was never started (listen was never called) or has already been closed [1]. - To avoid this, ensure your application logic only calls close when you are certain the server is active, or wrap the call in a check. - In complex applications or test environments (such as Karma or libraries like socket.io), double-closing servers can trigger this error if multiple components attempt to manage the server's lifecycle simultaneously [2]. In such cases, verifying the server's state or using synchronization logic (like awaiting a single exit promise) is recommended [2]. If you are experiencing issues with server shutdowns, note that server.close only stops the server from accepting new connections; it does not force existing connections to close immediately [3][4]. If you need to force a shutdown, newer Node.js versions (v18.2.0+) provide server.closeAllConnections and server.closeIdleConnections to assist with graceful termination [5][6].
Citations:
- 1: nodejs/node@b98aaa312e
- 2: 4.8.2 seems to be able to cause ERR_SERVER_NOT_RUNNING socketio/socket.io#5431
- 3: https://dev.to/gajus/how-to-terminate-a-http-server-in-node-js-ofk
- 4: https://stackoverflow.com/questions/77247270/closing-a-node-http-server-gracefully-server-close-callback-not-firing
- 5: https://stackoverflow.com/questions/14626636/how-do-i-shut-down-a-node-js-https-server-immediately
- 6:
http.server.close()closes idle connections since 18.19.0 nodejs/node#51677
🌐 Web query:
socket.io v4 Server.close implementation closes httpServer and what effect calling httpServer.close again has
💡 Result:
In Socket.IO v4, calling server.close is designed to close both the Socket.IO server and the underlying HTTP server [1]. Specifically, it performs a cleanup of the Socket.IO engine (disconnecting all clients) and subsequently triggers the close method on the attached HTTP server [1][2]. Regarding the effect of calling httpServer.close again: 1. Redundant Calls and Errors: Because server.close has already closed the underlying HTTP server, calling httpServer.close a second time will typically result in an error [3]. The standard Node.js http.Server.close method throws an ERR_SERVER_NOT_RUNNING error if the server is already closed or was never listening [3]. 2. Best Practices for Multiple Servers: If you have multiple Socket.IO server instances attached to the same HTTP server, calling server.close on one instance will close the shared HTTP server, potentially causing errors when other Socket.IO instances subsequently attempt to use it or when you manually try to close the HTTP server again [3]. In such scenarios, it is recommended to manage the HTTP server lifecycle explicitly rather than relying on individual Socket.IO server instances to close it [3]. In summary, server.close is a comprehensive shutdown method that terminates the Socket.IO connections and the HTTP server [1][4]. Subsequent manual attempts to close the same HTTP server should be avoided to prevent runtime errors [3].
Citations:
- 1: https://socket.io/docs/v4/server-api/
- 2: no way to shutdown a server cleanly socketio/socket.io#1602
- 3: Close error on httpServer close when attaching multiple Server instance to same httpServer socketio/socket.io#4002
- 4: Socket disconnect race condition leads to orphaned HTTP connections and stop() callback not running socketio/socket.io#2975
🌐 Web query:
Node.js http.Server.close() called when server not running ERR_SERVER_NOT_RUNNING callback
💡 Result:
The ERR_SERVER_NOT_RUNNING error in Node.js occurs when the server.close method is called on an instance of net.Server (including http, https, and http/2 servers) that is not currently listening or has already been closed [1][2]. When you provide a callback to server.close, Node.js invokes it after the server has fully shut down (i.e., after the 'close' event has been emitted) [3]. If the server was not running at the time server.close was invoked, the provided callback will be executed with an ERR_SERVER_NOT_RUNNING error as its argument [1][3]. Common causes for this error include: 1. Race Conditions: Attempting to close the server before it has finished initializing its listening handle [4]. For instance, calling server.close immediately after server.listen without waiting for the server to successfully bind to the port [4]. 2. Redundant Closures: Calling server.close multiple times on the same instance, where the first call successfully closes the server, and subsequent calls trigger the error [5]. 3. Lifecycle Mismanagement: Trying to shut down a server that was never successfully started (e.g., due to an earlier bind error like EADDRINUSE) [3]. To resolve or prevent this error: - Ensure server.close is only called after you have confirmed the server is running, ideally by invoking it within logic triggered by the server's lifecycle events (such as the listening callback) [4]. - Check the server's status or use flags to track whether the server is currently listening before attempting to close it. - If using third-party libraries, check if they manage server lifecycles automatically, as double-closing—where the library closes the server and then your code attempts to close it again—is a common scenario for this error [5].
Citations:
- 1: nodejs/node@b98aaa312e
- 2: https://nodejs.org/api/errors.html
- 3: https://docs.deno.com/api/node/net/~/Server
- 4: HTTP server.close() does not shut down, process hangs nodejs/node#41895
- 5: 4.8.2 seems to be able to cause ERR_SERVER_NOT_RUNNING socketio/socket.io#5431
🌐 Web query:
socket.io v4 Server.close httpServer.close behavior if httpServer already closing
💡 Result:
In Socket.IO v4, the server.close method is designed to close both the Socket.IO server and the underlying HTTP server [1]. Historically, calling io.close while the HTTP server was already closed or closing could result in an ERR_SERVER_NOT_RUNNING error because the internal call to httpServer.close would fail [2][3]. As of recent updates to Socket.IO, this behavior has been addressed to prevent the library from throwing an error if the underlying HTTP server is not running [3]. The internal implementation now handles the HTTP server closure more gracefully, ensuring that calls to io.close do not crash the application if the HTTP server has already been shut down [3]. Key takeaways: 1. Behavior: Calling io.close triggers an attempt to close the attached HTTP server [1]. If the HTTP server is already closed, modern versions of Socket.IO handle this safely without throwing an exception [3]. 2. Best Practice: If you are managing the HTTP server manually, it is standard to call io.close to ensure all Socket.IO connections are properly disconnected and cleaned up [1]. Simply closing the HTTP server is often insufficient for Socket.IO, as active WebSocket connections may persist until they are explicitly terminated [4][1]. 3. Multiple Instances: If you have multiple Socket.IO server instances attached to the same HTTP server, be aware that each io.close call attempts to close the shared HTTP server [2]. You may need to coordinate your shutdown logic to ensure the HTTP server is closed only once, or use individual io.engine.close calls if you need to disconnect Socket.IO instances without closing the shared transport [2]. For robust graceful shutdowns, ensure your logic explicitly handles the disconnect of active sockets to avoid orphaned connections [4][5].
Citations:
- 1: https://socket.io/docs/v4/server-api/
- 2: Close error on httpServer close when attaching multiple Server instance to same httpServer socketio/socket.io#4002
- 3: socketio/socket.io@9581f9b
- 4: no way to shutdown a server cleanly socketio/socket.io#1602
- 5: https://oneuptime.com/blog/post/2026-02-02-websocket-graceful-shutdown/view
🌐 Web query:
Node.js http.Server.close() called when server not running ERR_SERVER_NOT_RUNNING callback
💡 Result:
The ERR_SERVER_NOT_RUNNING error in Node.js occurs when the server.close method is called on a net.Server instance (which includes http.Server) that is not currently running or listening for connections [1][2][3]. Key facts about this error: 1. Cause: This error is explicitly triggered when you attempt to close a server that has not been started via server.listen, or if the server has already been closed and you attempt to close it again [1][4][3]. 2. Callback behavior: If a callback is provided to server.close, and the server is not running, the callback will be invoked with this error passed as an argument [1][3]. 3. Common scenarios: - Race conditions: If server.close is called immediately after server.listen before the server has had time to fully initialize and start listening, the server may not yet be considered "running" [4]. - Double closure: Attempting to close the same server instance multiple times can trigger this error if the first closure has already completed [5][6]. - Improper sequence: Logic that triggers a shutdown sequence before ensuring the server successfully entered a listening state [4][6]. To resolve or prevent this error: - Ensure server.close is only invoked after the server has successfully started. A common practice is to place the shutdown logic inside the callback of the server.listen method or ensure it is guarded by a state check [7][4]. - If you are experiencing this due to a race condition (e.g., during testing or initialization), verify the server's state or wait for the 'listening' event before allowing any close operations [4]. - If using third-party libraries (like Socket.IO or testing frameworks), ensure they are not attempting to perform redundant shutdown operations on the same server instance [5][6].
Citations:
- 1: nodejs/node@b98aaa312e
- 2: https://nodejs.org/api/errors.html
- 3: https://docs.deno.com/api/node/net/~/Server
- 4: HTTP server.close() does not shut down, process hangs nodejs/node#41895
- 5: 4.8.2 seems to be able to cause ERR_SERVER_NOT_RUNNING socketio/socket.io#5431
- 6: Sudden Error [ERR_SERVER_NOT_RUNNING]: Server is not running on angular builds nodejs/node#61159
- 7: https://stackoverflow.com/questions/35471247/error-not-running-on-server-close
🌐 Web query:
socket.io v4 Server.close httpServer.close behavior if httpServer already closing
💡 Result:
In Socket.IO v4, calling io.close (Server.close) will also close the underlying HTTP server, which means you can run into Node’s httpServer.close behavior when that HTTP server is already closing/stopped. 1) What Socket.IO v4 documents / does - io.close “disconnect[s] all clients” and “also closes the underlying HTTP server.” [1] So, even if you call io.close again or you separately call httpServer.close, Socket.IO may still attempt to close the same httpServer. 2) If httpServer.close was already called / server not running - Socket.IO had issues where calling io.close/Server.close when the underlying httpServer had already been closed would raise ERR_SERVER_NOT_RUNNING. This was addressed: “Following [socket.io]… calling both io.close and httpServer.close would throw an ERR_SERVER_NOT_RUNNING exception” and the fix changed the behavior so it doesn’t throw in that case. [2] - The fix’s intent is explicit: “do not throw when the underlying HTTP server is not running.” [2] 3) Practical implication for “httpServer already closing” - Node’s httpServer.close is a “stop accepting new connections” call; if the server is already not running/stopping, the call can produce ERR_SERVER_NOT_RUNNING. Socket.IO’s updated close behavior avoids throwing when it detects the underlying server is already stopped. [2] 4) Recommended shutdown sequence (to avoid races) - Best effort: call io.close once, and do not separately call httpServer.close unless you truly own the server lifecycle. - If you must handle both: rely on Socket.IO’s disconnect-all-clients + underlying close behavior (io.close), and wrap any separate httpServer.close calls in try/catch / error-ignore for ERR_SERVER_NOT_RUNNING, since prior Socket.IO versions could throw without the fix. [1][2] Answer to your exact question (behavior if httpServer already closing): - In current Socket.IO v4 (after the 2025 fix), calling io.close when the underlying HTTP server is already stopped will not throw; earlier versions could throw ERR_SERVER_NOT_RUNNING because Socket.IO calls httpServer.close as part of io.close. [1][2]
Citations:
shutdown.ts: remove/guard redundant server.close after io.close
app.tscreatesserverand constructs Socket.IO asnew Server(server, ...), sodeps.io.close()also closes the underlying HTTP server in Socket.IO v4.- With that, the nested
deps.server.close(...)is likely redundant and may result inERR_SERVER_NOT_RUNNINGpassed to the callback (currently ignored), while tests only cover call ordering via mocks.
deps.io.close(() => {
deps.server.close(() => {
deps.exit(0);
});
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bim-review-coordinator/src/shutdown.ts` around lines 21 - 25, The nested call
to deps.server.close after deps.io.close is redundant because Socket.IO (new
Server(server,...)) closes the underlying HTTP server; modify shutdown logic in
shutdown.ts so deps.io.close(...) invokes deps.exit(0) directly unless the HTTP
server still needs closing—i.e., inside the deps.io.close callback check if
deps.server?.listening (or similar runtime flag) and only call
deps.server.close(...) when true, otherwise call deps.exit(0) immediately;
update uses of deps.io.close, deps.server.close, and deps.exit accordingly.
… 繁中)
CodeRabbit + Codex review:
- [CodeRabbit major] shutdown.ts dispose 未 guard: try/catch,drain 失敗仍繼續 io/server close + exit(0),不 hang 到 SIGKILL
- [Codex P2] compose IFC_DOWNLOAD_STRICT hard-code false 覆蓋 .env: 改 ${IFC_DOWNLOAD_STRICT:-false} 讓 operator env 可覆蓋(否則 #9 production 指引失效)
- [CodeRabbit major] design/spec 章節改繁中(OpenSpec rules,parser headers 保留)
- defer follow-up(document 在 design): (Codex P2)non-http ref strict 未生效(超出 #9 http scope)、(Codex P2)in-flight intake drain race(既有 dispose 時機,#7 未惡化)
驗證: npm run verify 264 passed; openspec validate --strict 通過。
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
CH-2 review round 2:已修 shutdown dispose guard(CodeRabbit major,try/catch 避免 drain 失敗 hang)、compose IFC_DOWNLOAD_STRICT 改用 env 預設語法讓 operator 可覆蓋(Codex P2)、design/spec 章節繁中(OpenSpec rules)。 2 個 Codex P2 defer 為 follow-up(已 document 在 design.md 已知 follow-up 段): 驗證:npm run verify 264 passed;openspec validate strict 通過。 |
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
OpenSpec sync/archive(PR #142 merged squash 648f495 後): - change → archive/2026-06-01-harden-coordinator-ifc-intake - spec delta 併入: local-coordinator-ifc-ready-intake-boundary +1、project-risks-mitigation +1(specs 維持 32) - tasks 全勾;roadmap §1.6 加 2026-06-01 CH-2 entry Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CH-2(risk-triage 第二個 change):coordinator IFC intake 防護 + 死碼清理,收 3 風險,全 LOW impact,無新 production dependency。
#9 IFC strict 接線:config 加 ifcDownloadStrict(讀 IFC_DOWNLOAD_STRICT,code default false 不破壞既有 demo);app download 加 fallbackOnFetchError=!strict;compose ×2 加 IFC_DOWNLOAD_STRICT(production 設 true → non-2xx IFC 回 502 + download_failed,不靜默 placeholder)。
#7 graceful dispose:index.ts 接 SIGTERM/SIGINT → shutdown(dispose drain queued→dropped_on_restart → server.close → io.close → exit 0),補實作 RISK-IN-MEMORY-QUEUE-PERSISTENCE 既有 graceful shutdown requirement。
#19 退役 _bim-control 死碼:刪 BimControlClient + safeArtifacts + config bimControlApiBase + compose BIM_CONTROL_API_BASE 死 env;保留 Artifact type 與 buildArtifactBindings/buildStreamConfig 簽章;清 11 個 test makeApp。
OpenSpec:local-coordinator-ifc-ready-intake-boundary ADD 1、project-risks-mitigation ADD 1;#19 tasks-only。validate --strict 通過。
驗證:coordinator npm run verify = 260 passed(baseline 257 + 3 新 strict test,tsc 0 error = #19 型別清乾淨 + Artifact 簽章保住);root pytest = 65 passed(零回歸);主 repo 未污染。
Review 留意:#9 code default false(使用者拍板,不破壞 demo placeholder fallback);#7 不加 shutdown 超時 timer(follow-up),in-flight 刻意跑完;#19 徹底刪 client(已逐路徑驗證:env 永遠空、測試從不 mock 成功 fetch、buildArtifactBindings 走 inputBindings)。
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
IFC_DOWNLOAD_STRICTenvironment variable. When enabled, failed HTTP downloads return a 502 error instead of silently continuing with placeholder content.Configuration
IFC_DOWNLOAD_STRICTenvironment variable (defaults tofalsefor backward compatibility).