Conversation
Model provisioning (via `install-model` and ad-hoc at startup) downloaded every artifact serially and silently, then enforced a hardcoded 5-minute absolute per-file cap that wasn't even wired to installModel's options. A large model on a slow-but-working link aborted mid-download with an opaque error, and startup went quiet until the model was ready or boot stalled. - Add lib/vector_embedding/progress.js: rendering-agnostic formatters plus a TTY-aware CLI reporter (in-place %/bytes/rate/ETA line, plain milestone lines when piped) and a throttled cds.log reporter for startup. - Emit plan/file:skip/file:start/progress/file:done/done events from downloadModelIfNeeded and thread onProgress through the download chain. - Replace the single absolute timeout with an idle/stall timeout as the primary guard plus a generous absolute ceiling; errors name bytes-of-total and point at install-model. - Log download progress at startup and fail with actionable guidance to pre-provision when a download stalls. - Note the new behavior in README and .docs/vector-embeddings.md.
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one compatibility issue around the renamed timeout option. Overall the implementation is well-structured, with progress reporting cleanly separated and covered by focused tests.
PR Bot Information
Version: 1.31.58
- File Content Strategy: Full file content
- LLM:
gpt-5.5 - Correlation ID:
744b2540-bb22-11f1-8a65-e498ce616a01 - Event Trigger:
pull_request.opened
| const { | ||
| fetchImpl = globalThis.fetch, | ||
| idleTimeoutMs = DOWNLOAD_IDLE_TIMEOUT_MS, | ||
| downloadTimeoutMs = DOWNLOAD_TIMEOUT_MS, |
There was a problem hiding this comment.
Bug: Existing timeoutMs callers are silently ignored
downloadFile previously accepted timeoutMs, and provisionModel/downloadModelIfNeeded forwarded arbitrary options to it; after this change those callers get the new 60-minute default instead of their configured ceiling. Consider keeping timeoutMs as a backwards-compatible alias while introducing downloadTimeoutMs.
| downloadTimeoutMs = DOWNLOAD_TIMEOUT_MS, | |
| downloadTimeoutMs = options.timeoutMs ?? DOWNLOAD_TIMEOUT_MS, |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
ERROROur service encountered an error while processing your request. Please find additional information below: Correlation ID: |
This branch has not been deployed
1 participant
Why
Model provisioning — both
npx @cap-js/ai install-model <model>and the ad-hoc download on first server start — downloaded every artifact serially and silently, then enforced a hardcoded 5-minute absolute per-file cap (DOWNLOAD_TIMEOUT_MS) that wasn't even wired toinstallModel's options. Consequences:Timed out after 300000 ms while downloading <url>.install-modelprinted nothing until a singleInstalled …line at the end.The expected
file.sizefor every artifact is already known before download, so percentage/ETA were computable all along — just never surfaced.What
lib/vector_embedding/progress.js— rendering-agnostic formatters (formatBytes/formatDuration/formatRate/estimate/overallPercent) plus two reporter factories:createCliProgressReporter— TTY: single in-place line (\r) with %, downloaded/total, rate, ETA; non-TTY (piped/CI): plain milestone lines, no\rspam.createLogProgressReporter— throttled plain lines viacds.logfor startup provisioning.downloadModelIfNeededpre-scans files (skipped vs pending), emitsplan/file:skip/file:start/progress/file:done/done;onProgressis threaded throughcli → installModel → provisionModel → downloadModelIfNeeded → downloadFile(closing the gap where timeout/progress options weren't forwarded).downloadFilenow uses an idle/stall timeout (reset per chunk, 60s default) as the primary guard plus a generous absolute ceiling (60m). Errors name bytes-downloaded-of-total and point atinstall-model..docs/vector-embeddings.mdnote the progress output, the stall-not-deadline behavior, and the recommendation to pre-install large models.All options are optional; absence preserves the prior behavior.
Testing
tests/progress.test.js(formatters, CLI TTY vs non-TTY, log reporter).tests/model-provisioning.test.js(idle-timeout abort, absolute ceiling, progress/skip event accounting).