fix(images): add a connect deadline to provider artifact downloads

[luvs01]

Summary

• pinnedHttpsGet now forwards a connect deadline (DOWNLOAD_CONNECT_TIMEOUT_MS, 10s default) to the pinned transport for provider-returned artifact URLs.
• Previously only idleTimeoutMs was passed, so a URL whose TCP or TLS setup never completed could wait without a bound; the connect phase now fails with connect_timeout.
• Existing idle-timeout behavior and the maxBytes cap are unchanged; callers can still override via connectTimeoutMs.

Verification

• bun test tests/images/pinned-https-get.test.ts — 7 pass, including a new case asserting the 10s connect deadline is scheduled by default and a connecting socket that never completes rejects with connect timed out.
• bun x tsc --noEmit — clean.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed.
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

• Bug Fixes
  • Image downloads now have a default 10-second connection timeout, preventing stalled connection attempts from hanging indefinitely.
  • Existing idle-timeout behavior remains unchanged.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

• All CI tests are green on my local testing.

• I pushed my PR to the latest dev commit.

• I resolved all correct Codex and CodeRabbit findings.

• My PR is ready for review.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: f645b68f-e7cd-4b7a-9324-9bd834bbad9a

📥 Commits

Reviewing files that changed from the base of the PR and between 53725b0 and 866a49b.

📒 Files selected for processing (2)

• src/images/artifacts.ts
• tests/images/pinned-https-get.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.

---

📝 Walkthrough

Walkthrough

The image download helper now exposes a 10-second default connection timeout. pinnedHttpsGet forwards this timeout to pinnedHttpGet. Tests verify rejection when a connection remains unresolved.

Changes

Image download connection timeout

Layer / File(s)	Summary
Timeout contract, forwarding, and validation src/images/artifacts.ts, tests/images/pinned-https-get.test.ts	src/images/artifacts.ts:15-16 exports DOWNLOAD_CONNECT_TIMEOUT_MS as 10_000. pinnedHttpsGet accepts connectTimeoutMs and forwards the caller value or the default to pinnedHttpGet at src/images/artifacts.ts:275, 285-289. tests/images/pinned-https-get.test.ts:57-89 verifies rejection with a connect-timeout error after an unresolved connection reaches the 10-second deadline.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant pinnedHttpsGet
  participant pinnedHttpGet
  participant ConnectingRequest
  pinnedHttpsGet->>pinnedHttpGet: Forward connectTimeoutMs or 10000 ms
  pinnedHttpGet->>ConnectingRequest: Start connection
  pinnedHttpGet->>ConnectingRequest: Reject after connection deadline
  ConnectingRequest-->>pinnedHttpsGet: Return connect-timeout error

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding a connection deadline to provider artifact downloads. It matches the changes in src/images/artifacts.ts and the stated PR objective.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

✅ Deterministic PR hygiene checks passed.

[github-actions]

⏳ DRAFT

• review readiness checklist open (3/4 boxes ticked).

What to do

• Tick all four boxes in the PR description once you're done (currently 3/4).
• The PR is more than 10 commits behind dev; the latest dev box has been unticked.
• The checklist has been reset: re-test against the latest code and tick the boxes again.

Review readiness checklist

• ✅ All CI tests are green on my local testing.
• ⬜ I pushed my PR to the latest dev commit.
• ✅ I resolved all correct Codex and CodeRabbit findings.
• ✅ My PR is ready for review.

3/4 boxes ticked.

The PR is more than 10 commits behind dev; the latest dev box has been unticked.
The checklist has been reset: re-test against the latest code and tick the boxes again.
This PR stays in draft until every box above is ticked.

[lidge-jun]

리뷰 · 우선순위 71 / 80

이 PR은 제공자가 돌려준 그림 주소(HTTPS)를 받을 때, TCP·TLS가 안 끝나도 오래 기다리지 않게 하려고 합니다. pinnedHttpsGet에 기본 연결 시한 DOWNLOAD_CONNECT_TIMEOUT_MS(10초)를 넣고, 그 값을 아래 전송층 pinnedHttpGet의 connectTimeoutMs로 넘깁니다. 이미 있던 유휴 시한(60초)과 크기 상한은 그대로입니다. 테스트는 소켓이 connecting: true로만 남는 가짜 HTTPS 요청을 만들고, setTimeout(10000)이 잡히면 connect timed out으로 실패하는 한 케이스를 추가했습니다. 베이스는 dev이고, 같은 주제로 열린 다른 PR은 없습니다. types.ts/config.ts 분할과 무관합니다. PR은 아직 draft이고 체크리스트 네 칸은 비어 있습니다.

그런데 실제로 제공자 그림·영상 URL을 받는 길은 downloadImageToArtifact → fetchPublicHttpsImage → connectPublicHttps → pinnedHttpGet입니다. connectPublicHttps의 기본 구현은 maxBytes와 context만 넘기고 connectTimeoutMs를 안 넣습니다. pinnedHttpsGet은 이 파일 안에서 정의·주석으로만 나오고, 저장소 src/ 생산 코드에서는 호출처가 테스트뿐입니다. 그래서 제목·본문이 말하는 “아티팩트 다운로드에 연결 시한”은 이 커밋만으로는 운영 경로에 안 닿습니다. 영상(downloadVideoToArtifact)도 같은 connectPublicHttps를 씁니다.

또, connectTimeoutMs가 없어도 pinned-http 레거시 유휴 모드에서는 첫 바이트·요청 시한이 기본 60초로 잡힙니다. “한도가 없다”기보다 “연결만 짧게(10초) 끊고 싶다”에 가깝습니다. 그 짧은 시한을 쓰려면 운영 경로에 넘겨야 합니다.

라인 - src/images/artifacts.ts pinnedHttpsGet — 10초 기본값을 pinnedHttpGet에 넘기는 배선 자체는 맞습니다. 다만 운영 다운로드는 여기를 거치지 않습니다.
라인 - src/images/artifacts.ts connectPublicHttps 기본 pinnedHttpGet — maxBytes·context만 있고 connectTimeoutMs(및 이미지용 idleTimeoutMs)가 없습니다. PR이 고치려는 구멍이 여기입니다. 영상도 동일합니다.
라인 - tests/images/pinned-https-get.test.ts “applies a connect deadline by default” — pinnedHttpsGet만 검사합니다. fetchPublicHttpsImage / downloadImageToArtifact / connectPublicHttps 기본 경로에서 10초가 스케줄되는지는 안 봅니다.
라인 - 같은 테스트의 spyOn(globalThis, "setTimeout") — 모든 setTimeout을 가로채고 ms === 10_000일 때만 콜백을 돌립니다. 의도는 보이지만, mock.module("node:https")는 finally에서 되돌리지 않습니다(파일 안 다른 테스트와 같은 습관).
라인 - PR 본문 “wait without a bound” — 운영 경로에도 레거시 60초 유휴/첫바이트 시한이 있어, 완전 무한 대기는 아닙니다. 문구를 “연결 단계만 10초로 줄인다”로 고치는 편이 맞습니다.
라인 - 체크리스트 — 0/4. draft 유지와 맞습니다. 최신 쪽 enforce-target·hygiene·label은 통과로 보입니다.

메인테이너의 판단이 필요한 지점

연결 10초를 (1) 이미지·영상 공개 URL 다운로드 전부(connectPublicHttps)에 넣을지, (2) pinnedHttpsGet만의 계약으로 두고 운영 경로를 pinnedHttpsGet으로 바꿀지. 구조 문서(CL-03)는 Lab만 연결 시한을 명시하고 다른 pinned-HTTP 호출자는 예전 동작을 유지한다고 적혀 있어, 이미지 경로만 예외로 열지 정해야 합니다. 10초가 CDN·느린 TLS에 짧은지도 운영 감각이 필요합니다.

너의 추천

지금 머지하지 마세요. connectPublicHttps 기본 pinnedHttpGet 호출에 connectTimeoutMs: DOWNLOAD_CONNECT_TIMEOUT_MS(필요하면 idleTimeoutMs: DOWNLOAD_IDLE_TIMEOUT_MS)를 넣거나, 기본 다운로드를 pinnedHttpsGet으로 바꾸세요. 회귀 테스트는 그 운영 경로에서 10초가 잡히고 연결이 안 끝나면 connect timed out이 나는지 보세요. pinnedHttpsGet 단위 테스트는 보조로 남겨도 됩니다. 고친 뒤 체크리스트를 채우고 draft를 해제하세요.

이 댓글은 grok-bot이 작성했습니다

[luvs01]

Consolidated into #5349 as the existing, more complete artifact-download fix.

Source head: 866a49bebf732105d0eaf83d063e196a123b8570. Replacement head: 7078e0a9233cc4af21832a8d98fb5cfcfb242e97.

Direct final-source comparison confirms that both implementations give pinnedHttpsGet a 10,000 ms default connect deadline, preserve its per-call connectTimeoutMs override, and forward the resolved value to pinnedHttpGet without altering the idle timeout or size cap. The source differences in that wrapper are comments and property ordering. The replacement additionally applies the same deadline to connectPublicHttps, the actual image/video download path, and adds a production-path regression. Its wrapper tests cover both the default deadline and a 250 ms override with connection-timeout rejection, covering the original default-deadline test's behavior. The replacement description already identifies #5295 as a sibling where only one should land. Closing this narrower duplicate preserves one complete review target; this read-only equivalence check does not independently assert that the replacement's CI, build or merge gates pass.

Closing this duplicate standalone review entry as part of the requested consolidation after verifying coverage. This is not a merge or release claim; remaining integration checks and reviews are tracked on the replacement. Original branches are retained.