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_info gained size_bytes (free server-side; the list handler already loaded each image's full data into memory), and fetchImagesInBatches cuts 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 for ores.qt/api to automate this (declined as a separate, non-blocking finding – see * Review).
  • [X] MAX_IMAGES_PER_REQUEST revisited: 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.

Emacs 29.3 (Org mode 9.6.15)