Repository navigation
fix(cli): catalog loads registry items a few at a time so a slow link no longer times them out - #5829
Conversation
… no longer times them out
Edit accuracy: accurate 271 (base branch 271), smooth 187 of thoseThe gate passes. Quarantined, measured but not gated (0) |
eb3a2d4 to
b0c1399
Compare
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
Reviewed exact HEAD b0c1399252ae4c8fe39999d4c7b091e9d30a08be against effective BASE b74686ef8e2680ee1f4d623cad92d1a8218f8746 (API BASE 2b9009ab520a31fa7f23cbad44583733a6602abc).
No must-fix blocker found. The extracted helper preserves the existing algorithm and positional results, and the registry callback isolates item failures rather than allowing them to reject the shared pool.
Important, nonblocking — stale-cache timeouts bypass the new fuse
packages/cli/src/registry/resolver.ts:59–63, with packages/cli/src/registry/remote.ts:162–172: the fuse only observes a rejected fetchItemManifest. A successful prior catalog run can populate valid item caches; after 24 hours, an offline/blackholed connection causes revalidation to time out, but fetchItemManifest returns each stale cached item from its catch. Consequently timedOut stays false and every queued row still attempts the network.
The bounded pool therefore introduces ceil(N / 32) timeout waves on that ordinary stale-cache path, versus the effective BASE's single concurrent wave. For 96 stale items, that is three configured 10-second windows rather than one, before catalog.ts:238 can continue to filtering/search/output. This is source-derived scheduling/arithmetic, not measured wall time or an executed reproduction. Both versions eventually return the same valid cached items; I am treating the regression as important/nonblocking, not a wrong-data or permanent-liveness blocker.
Follow-up: preserve stale-data success while propagating timeout observation separately to the per-load fuse. Add a case that primes more than 32 item caches through the normal fetch/write path, expires them, then rejects revalidation with TimeoutError; assert all cached results survive without additional network cohorts. The new resolver tests use empty caches, while the existing remote stale-fallback test does not exercise the pool. The PR body's one-timeout-window claim does not currently cover stale caches.
Scope: full changed effective-BASE source read before full changed HEAD source, applicable standards/current review rubric, current public history, actual catalog/cache/transport paths, helper/Lambda integration, and every pair of bundled behavioral groups. Two independent source-only passes considered the same stale-cache issue. The final successor changes only the helper JSDoc. Untouched source interiors and platform/dependency implementation remain outside this bounded audit. No repository/dependency code, tests, builds, browser/native work, benchmarks, or CI were executed or queried; author test/performance claims remain unverified.
Verdict: APPROVE
Reasoning: No demonstrated must-fix correctness, security, persistence, or permanent-liveness defect remains in the reviewed scope. The stale-cache hot-path latency regression above is an important follow-up and does not certify the claimed offline timing.
— Review by tai (pr-review)
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at 6c56be4d. The previous approval was tai's at b0c13992, so I reviewed the whole PR rather than only the delta.
- Delta.
b0c13992..6c56be4dchanges only the doc comment onrunWithConcurrencyLimitinutils/concurrency.ts, which now wraps onto two lines. No code changed. - Helper move. The body of
runWithConcurrencyLimitis identical to the copy removed fromlambda/render-batch.ts.render-batch.tsand its test now import it fromutils/concurrency.js. - Bounded fetching.
loadAllItemsruns up to 32 fetches at a time, and each request starts its own 10 sAbortSignal.timeoutinsidefetchJson. So a timeout now covers only that request's transfer, not time spent waiting in a queue. - Fuse. The first
TimeoutErrorsetstimedOut. After that,fetchItemManifest(..., { cacheOnly: true })returns any cached copy, stale or not, or throws without touching the network. The fetch-failed-with-cache path is unchanged. - Warning. One summary line, or the old single-item line when only one item fails.
- Tests. The new resolver tests pin the in-flight cap at 32, and check that only the first wave of 32 reaches the network after a timeout.
- Not blocking. I agree with tai's follow-up: when the cache is stale,
fetchItemManifestreturns the cached copy instead of throwing the timeout, so the fuse never trips. An offline run with a stale cache can wait through several 10 s waves before it ends.
All three commits are signed and verified. CI: 53 checks pass, none pending or failing. No live run on my side.
— Rames
What changes
hyperframes catalog --query <q> --jsonno longer floods stderr withTimeoutError: The operation was aborted due to timeoutlines, and no longer quietly searches only part of the catalog, when the registry link is slow. Item manifests now load 32 at a time instead of all ~450 at once. If one does time out, the rest are served from cache instead of waiting on the network. Failures print one summary line.Why it happened
The catalog search needs each item's
registry-item.json;registry.jsoncarries only names and types.loadAllItemsstarted every fetch together throughPromise.allSettled. Each fetch has its own 10 sAbortSignal.timeout, and all those clocks started at the same moment while ~450 responses shared one link.On a link that can't move them all within 10 s, every response still queued at that point timed out together. Each one printed its own
skipped item "<name>": TimeoutError ...line, and the search ran over whatever had arrived. Item manifests are cached for 24 hours, so this hits the first run of the day, a fresh machine, or a sandbox with an empty home.The fix
loadAllItemsruns at most 32 fetches at a time through the existingrunWithConcurrencyLimit, moved fromcommands/lambda/render-batch.tstoutils/concurrency.tsso the registry code doesn't import the Lambda module. Each request's timeout now covers only its own transfer.TimeoutError, the remaining items usefetchItemManifest(..., { cacheOnly: true }), which serves a cached copy (even a stale one) or skips the item. A blackholed network still costs one 10 s wait, not one per batch of 32.skipped 199 items ("a", "b", "c", ...): <first reason>, or the old single-item line when only one item fails.Evidence
A local HTTPS registry built from this repo's
registry/that releases one response every 40 ms (a slow shared link), with an empty cache:TimeoutErrorlines on stderrtotal)The real registry over a healthy connection with an empty cache: 0.8-1.3 s before, 0.94-0.99 s after (3 runs each), 447 items either way.
Tests
Two new cases in
registry/resolver.test.ts:TimeoutError: only the first 32 reach the network, and exactly one summary warning is printed. On main there are 100 fetches.With only the cache-only fallback reverted, the second case fails (100 calls). Both passed 3 runs in a row. The registry, catalog and moved-helper suites pass (335 tests).