Task: Make ImageCache batching byte-size aware
Table of Contents
This page documents a task in the Fix image-batch NATS payload overflow, add SVG compression story. It captures the goal, current status, acceptance, and any notes or results.
Goal
Make ImageCache's batch-fetch logic track cumulative estimated
payload size while building each batch, not just image count, so a
batch is cut short before adding an image that would risk exceeding
NATS's max payload – rather than relying solely on
MAX_IMAGES_PER_REQUEST as a count-based proxy for size. This is the
remaining backstop after the compression task lands: compression
removes most of the overflow risk, but a byte-size-aware cap makes the
guarantee hold regardless of how images happen to cluster in a given
request. Depends on the decision task confirming NATS (not HTTP
transport) is the chosen path, and should land after (or alongside)
the compression task, since the safe-size threshold is computed
against post-compression payload size.
Revisit MAX_IMAGES_PER_REQUEST once this lands: raise it back up,
remove it, or reinterpret it as a soft cap now that byte size is the
real constraint being enforced.
Status
| Field | Value |
|---|---|
| State | DONE |
| Parent story | Fix image-batch NATS payload overflow, add SVG compression |
| Now | Nothing. |
| Waiting on | Nothing. |
| Next | Nothing. |
| Last touched | 2026-07-26 |
Acceptance
[X]ImageCache's batch-fetch logic tracks cumulative estimated payload size, not just image count –image_infogainedsize_bytes(free server-side; the list handler already loaded each image's full data into memory), andfetchImagesInBatchescuts a batch before adding an image that would push the running estimate past a 900KB safe threshold.[X]Verified by hand-tracing (own review + four independent automated review passes) against the decision task's worst-case scenarios: unknown-size images, mixed sizes, a single oversized image, and the count-cap boundary. No unit test harness exists forores.qt/apito automate this (declined as a separate, non-blocking finding – see* Review).[X]MAX_IMAGES_PER_REQUESTrevisited: raised from 15 to 200, repurposed as a generous sanity bound rather than the primary defence, per the story's acceptance criteria.
Plan
(Implementation strategy. Written when work starts; key decisions
are distilled into the parent story's * Decisions at close, but the
plan itself stays — it is the historical record of what we did.)
Notes
Test Scenarios
Manual QA scenarios (scaffolded via compass add test_scenario, run
through the QA Validation Runner panel) that verify this task. Link
new ones here as they're created; the scenario doc itself links back
via its "Verifies task" field.
| Scenario | State | Notes |
|---|---|---|
PRs
| PR | Title |
|---|---|
| #1712 | [assets,qt] Make ImageCache batching byte-size aware |
Review
| # | Comment summary | File | Decision | Notes |
|---|---|---|---|---|
| 1 | Optimization may not engage on common login path due to unguaranteed loadAll()/loadImageList() async race | ImageCache.cpp/.hpp | Fixed | Added finishLoadAllChain()/resumeDeferredLoadAllChain(): loadAll()'s chain now defers its final fetch until available_images_ is populated instead of proceeding with unknown-size fallback. |
| 2 | O(n*m) linear scan merging incremental-change sizes into available_images_ | ImageCache.cpp | Declined | Non-blocking per review; list sizes are low hundreds, not worth the complexity now. |
| 3 | No unit tests for fetchImagesInBatches | ImageCache.cpp | Declined | No existing test scaffolding for ores.qt/api (private static method, live ClientManager* dependency); consistent with existing coverage, not a regression. |
| 4 | Round-1 fix (#1) introduces a deadlock: loadImageList() early-returns without setting its QFuture when disconnected, so onImageListLoaded() never fires to clear the deferred wait, permanently stalling load_all_in_progress_ | ImageCache.cpp | Fixed | finishLoadAllChain() now checks connectivity itself; when disconnected it fetches immediately (matching pre-fix behaviour) instead of deferring, since a real fetch can't happen either way. |
Result
Shipped in PR #1712. image_info gained size_bytes (free
server-side); ImageCache caches it in available_images_ across
both full and incremental loads; fetchImagesInBatches cuts batches
by cumulative estimated wire size (900KB threshold, unknown-size
images always get their own batch) instead of a count cap alone.
Deliberately estimates against uncompressed size rather than assuming
the compression task's transparent gzip is in effect, so this backstop
holds independent of compression's actual ratio.
Two review rounds: round 1 fixed a real gap where the optimization
might not engage on the common login path due to an unguaranteed
loadAll()=/=loadImageList() async race (added
finishLoadAllChain()=/=resumeDeferredLoadAllChain()); round 2 fixed
a deadlock that fix itself introduced (disconnected-client path never
resumed the deferred wait). Both confirmed fixed by follow-up
automated review passes with no new findings. Two minor findings
(O(n·m) merge scan, no unit tests for the batching cutover logic)
declined as non-blocking.
Two follow-up tasks split off separately: raw-bytes-not-base64 (not
load-bearing, deferred pre-existing task) and decompressing
jetstream_admin's peek path (unrelated to this task, filed from the
compression PR's review).
Full local verification: build clean, ctest 74/74 passing.