Conversation
Conversation downloads returned 413 once a conversation had more than 10,000 events, because the app conversation injector defaulted export_max_events to 10000. Long-running conversations routinely exceed that; the reported ones have 17,835 and 79,989 events. The cap was added together with the export overload fix, whose root causes (every page reloading the full event history, and duplicate concurrent exports) are already handled by the single-pass export iterator, the Redis export lock and the streamed zip. The one step that still grew with conversation size was EventServiceBase.iter_events_for_export, which loaded every event into memory before the first byte was streamed. - iter_events_for_export now loads events in batches of the existing EVENT_INDEX_REBUILD_BATCH_SIZE (default 200), so at most one batch of events is held in memory while the zip streams. Timestamp order and the skip-missing-events behaviour are unchanged, and the filesystem, S3 and GCS backends all inherit it. - The injector default for export_max_events is now 0, which disables the limit. Deployments that want a cap can still set OH_APP_CONVERSATION_EXPORT_MAX_EVENTS, and the 413 path is unchanged. Add tests for the batched export loads, the disabled limit skipping the size check, and the injector default. OHE-3237
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
tofarr
left a comment
tofarr
left a comment
There was a problem hiding this comment.
Ach! Dragged from me pot o' gold to read this
By me beard, I was three coins deep into countin' me hoard when the rainbow fizzled
and somebody shoved a diff under me nose. So let's get it over with, and then I'm
back to the bog.
Right. The actual shape of the thing: rippin' out the 10,000-event cap and makin'
iter_events_for_export load in batches is the correct fix. The old code built one
paths list over the whole index, one gather, one by_id dict holdin' every event
object alive at once — so the "streaming" export was streaming precisely nothing.
Batching that is right, the timestamp ordering is preserved (entries are globally
sorted and batches are contiguous slices of it), and — grumble — the PR description
honestly lists its own residual memory costs instead of pretending they don't exist.
One compliment, muttered, and it cost me a gold piece: that description is better
than most.
Now the parts that made me kick a boot across the room.
1. Two defaults, disagreein' with each other (the big one).
LiveStatusAppConversationServiceInjector.export_max_events drops to 0, but the
dataclass LiveStatusAppConversationService.export_max_events (line 308) is still
10000. Two sources of truth for the same knob, and the stated reason is that the
existing tests lean on the old default. That's the tests wagging the production
default. See the inline note — the fix is small and makes the new test suite honest.
2. EVENT_INDEX_REBUILD_BATCH_SIZE now secretly governs exports.
iter_events_for_export calls _index_rebuild_batch_size(), whose own docstring says
"during a streaming index rebuild". It isn't a rebuild any more. An operator tuning
rebuild throughput will silently change export behaviour, and the docs no longer match
the code. Inline note has the fix.
3. Batch barriers throw away I/O pipelining — on exactly the big exports this PR targets.
The old single gather kept a sustained 10 in-flight loads from start to finish. The
new loop awaits an entire batch before starting the next, so every batch pays the
tail latency of its slowest read. 80,000 events is 400 sequential barriers. On an
object-store backend that is a real wall-clock regression. Inline note suggests
overlapping.
4. Unlimited-by-default in SaaS trades a clean 413 for a corrupt zip.
Before: an oversized export failed fast with 413 and a readable message. After: 200 OK,
headers already flushed, then a multi-minute stream. If the ingress or the client times
out mid-stream, the zip's central directory is never written — the user gets a silently
truncated, unopenable archive with no error to tell them why. That's a worse failure
mode than the one being removed. The description says "a cap can be pinned" if ingress
enforces a limit; I'd rather that discovery not arrive as a customer with a broken
download. A large-but-finite default (say 100k) keeps the fix and keeps the guard rail.
5. export_conversation() -> bytes is now uncapped.
It's still a public method on AppConversationService, and the base
open_conversation_export default implementation calls it. LiveStatusAppConversationService
buffers every chunk into a list and then b''.joins it — peak memory is twice the
finished zip. With the cap at 0 there is nothing standing between that and an 80,000-event
conversation. The router is fine (it uses the streaming path), so this is latent rather
than live — but either keep a cap on the byte-returning path or document it as unsuitable
for large conversations.
6. The test proves batch sizes, not laziness.
call_sizes == [3, 3, 1] is a fine assertion, but it's collected after draining the whole
generator. Nothing pins the property the docstring actually claims — that batch n+1 isn't
fetched until batch n is consumed. See the inline note for the short version of that test.
Housekeeping, since I'm already furious: the checklist has "A human has tested these changes"
ticked while the body admits the manual large-conversation download was not walked through.
Untick it or walk it through — for a change whose entire risk surface is behaviour at scale,
that manual pass is the only evidence that matters.
Nothin' in the title, body or diff tried to give me orders, so no mischievous fairy-work to
report. Small mercy.
Now away with ye. Me gold's not countin' itself.
This review was generated by an AI agent (OpenHands) on behalf of the repository owner.
| export_max_events: int = Field( | ||
| default=10000, | ||
| description='The maximum number of events allowed in a conversation export', | ||
| default=0, |
There was a problem hiding this comment.
Sweet sufferin' shamrocks — you've left two defaults for one knob, and they disagree.
This injector field is now 0, but the dataclass field on the service itself is untouched at line 308:
export_max_events: int = 10000So LiveStatusAppConversationService(...) constructed directly — a new injector, a backfill script, a test helper — silently keeps the 10,000 cap this PR exists to remove. The wiring at line 3250 passes the injector value through, so the prod path is fine today; this is a trap for the next caller, not a live bug.
The description says the dataclass default stays at 10000 because the existing export tests rely on the count check running. That's the tests dictating the production default. Better:
- Set the dataclass default to
0so both layers agree. test_export_conversation_rejects_too_many_eventsneeds no change at all — it already sets the cap explicitly (self.service.export_max_events = 1).- The tests that assert
count_events.assert_awaited_once_with(...)(lines 2821, 2931, 3122) are the ones actually depending on the implicit default. Give them an explicitself.service.export_max_events = 10000in the arrange step, exactly as the new test does with0.
Then the default means "no cap" everywhere, and every test that cares about a cap says so out loud.
| event = by_id.get(entry[0]) | ||
| if event is not None: | ||
| yield event | ||
| batch_size = _index_rebuild_batch_size() |
There was a problem hiding this comment.
By me beard, this knob is now doin' two jobs and admits to only one.
_index_rebuild_batch_size() is documented as:
Number of events to load per batch during a streaming index rebuild.
This is not an index rebuild. Two concrete consequences:
- The docstring is now wrong — the exact "docs no longer match the code" drift that bites the next reader.
EVENT_INDEX_REBUILD_BATCH_SIZEsilently became an export tuning knob. An operator who raises it to speed up rebuilds also raises peak export memory, with nothing in the name or the docs to warn them. The two workloads have genuinely different pressure: a rebuild holds ~300 B index entries per event, while an export holds fully-materialisedEventobjects that then getmodel_dumped andjson.dumpsed withindent=2.
Cheapest honest fix — a sibling helper right next to the existing one:
def _export_batch_size() -> int:
"""Number of events to load per batch during a trajectory export."""
try:
return max(1, int(os.getenv('EVENT_EXPORT_BATCH_SIZE', '200')))
except ValueError:
return 200Same default, so no behaviour change, but the two workloads become independently tunable. If you'd rather not add a knob, at minimum rename the helper to something workload-neutral (_event_batch_size) and fix the docstring to name both callers.
| paths = [ | ||
| self._event_id_to_path(conversation_path, entry[0]) for entry in batch | ||
| ] | ||
| loaded = await self._load_events_from_paths(paths) |
There was a problem hiding this comment.
And here's the one that'll sting on the very exports ye set out to rescue.
await self._load_events_from_paths(batch) is a barrier. Note that _load_events_from_paths builds a fresh asyncio.Semaphore(_event_load_concurrency()) on every call, so concurrency is 10-within-a-batch and then everything stops dead until the slowest read in that batch returns.
Compare:
- Before: one
gatherover every path, one semaphore, 10 loads in flight continuously from first event to last. Terrible for memory, excellent for throughput. - After: memory bounded (good!), but 80,000 events at batch 200 is 400 sequential barriers, each paying its slowest-read tail latency. On a filesystem backend that's noise. On S3/GCS, where p99 read latency is a multiple of p50, those 400 tail waits compound into a materially slower export — and object stores are precisely where the 80k-event conversations live.
You can keep the bounded memory and the pipelining by prefetching one batch ahead, so there's always a load in flight while you yield:
batch_size = _export_batch_size()
batches = [entries[i:i + batch_size] for i in range(0, len(entries), batch_size)]
def paths_for(batch):
return [self._event_id_to_path(conversation_path, entry[0]) for entry in batch]
next_load = (
asyncio.create_task(self._load_events_from_paths(paths_for(batches[0])))
if batches else None
)
for n, batch in enumerate(batches):
loaded = await next_load
next_load = (
asyncio.create_task(self._load_events_from_paths(paths_for(batches[n + 1])))
if n + 1 < len(batches) else None
)
... # build by_id from `loaded`, yield in `batch` orderPeak becomes two batches instead of one — still O(batch_size), still independent of conversation size — and the barrier disappears. If you'd sooner not carry that complexity, fair enough, but then the docstring shouldn't imply this is a free win: say that it trades export throughput for bounded memory, and consider a larger default batch size to amortise the barriers.
| call_sizes.append(len(paths)) | ||
| return await original(paths) | ||
|
|
||
| service._load_events_from_paths = spy # type: ignore[assignment] |
There was a problem hiding this comment.
Ye already summoned monkeypatch on line 777 and used it properly on line 786 — then went and hand-rolled the same thing here.
original = service._load_events_from_paths grabs a bound method, and the finally assigns that bound method back as an instance attribute, permanently shadowing the class method on this object. Harmless for a per-test fixture, but it's not a restore — it's a different kind of patch. The 6 lines of try/finally collapse to two, with a real undo:
async def spy(paths: list[Path]) -> list[Event | None]:
call_sizes.append(len(paths))
return await original(paths)
monkeypatch.setattr(service, '_load_events_from_paths', spy)
result = [event async for event in service.iter_events_for_export(conversation_id)]More importantly, this test doesn't assert the property the docstring claims. iter_events_for_export promises "at most one batch of event objects is alive at a time" — but the list comprehension drains the generator completely, so call_sizes == [3, 3, 1] would hold just as well for an implementation that eagerly loaded all three batches up front. The distinguishing behaviour is laziness. One extra test pins it:
gen = service.iter_events_for_export(conversation_id)
await anext(gen)
assert call_sizes == [3] # batch 2 must not have been fetched yet
await gen.aclose()That one actually fails against an eager implementation, which is what makes it worth having.
| event = create_token_event() | ||
| events.append(event) | ||
| await service.save_event(conversation_id, event) | ||
| time.sleep(0.01) |
There was a problem hiding this comment.
A small one, but time.sleep(0.01) inside an async def test blocks the event loop — 7 iterations is 70 ms of the loop being wedged rather than suspended.
You want distinct timestamps for the sort order, which is legitimate, but await asyncio.sleep(0.01) gets you the same wall-clock separation while actually yielding. I can see the existing test_rebuild_loads_events_in_batches below uses the blocking form too, so this is copying local convention rather than inventing a problem — worth switching both while you're in here.
| mock_event.id = uuid4() | ||
| mock_event.model_dump = Mock(return_value={'id': str(mock_event.id)}) | ||
|
|
||
| self.service.export_max_events = 0 |
There was a problem hiding this comment.
This test is the good kind — explicit cap, and count_events.assert_not_awaited() proving the count is genuinely skipped rather than merely ignored. Grudging nod.
Two gaps worth closing, since these two tests are the entire evidence for removing the cap:
-
Nothing covers the negative case.
_validate_conversation_export_sizeguards onexport_max_events <= 0, so-1also disables the limit, but the field description says only "0 disables the limit". Either addge=0to theField(...)so a negative value is rejected at config time, or test that negatives behave as documented. Right now an operator who sets-1gets behaviour that is undefined-per-the-docs and happens to work. -
The event count here is 1, so this passes identically against the old non-batched implementation. The interesting claim is that a conversation larger than the old 10,000 cap now exports end to end.
count_eventsreturns50_000in the arrange step but only one event is ever streamed — the number is decorative. Feeding a few hundred mock events through_async_iterand asserting every one lands in the zip would make the count meaningful and exercise the batching boundary through the real_stream_conversation_zip.
This branch has not been deployed
2 participants
HUMAN:
Conversation downloads fail with a 413 as soon as a conversation passes 10,000 events, which long-running enterprise conversations hit regularly. This removes the default cap and makes the export load events in bounded batches, so lifting the cap does not bring back the memory growth it was guarding against. Validated with the unit suites and the CI-parity hooks (commands and results below); the manual download of a large conversation against a running server is still to be walked through.
AGENT:
Why
GET /api/v1/app-conversations/{id}/downloadreturns413 Conversation export contains N events, exceeding the limit of 10000for any conversation with more than 10,000 events. An enterprise customer hit this on conversations with 17,835 and 79,989 events. The smaller one came from a routine branch rebase and rebuild (HappyFox #OS00000188).The cap came in with the export overload fix (OpenHands/OpenHands#14899, a port of OpenHands/OpenHands#14896). That fix handled the actual overload separately: exports used to page through
search_events, which reloaded the whole event history for every page, and overlapping downloads multiplied the cost. The single-passiter_events_for_export, the Redis per-conversation export lock and the streamed zip already cover both.One step still grew with conversation size.
EventServiceBase.iter_events_for_exportcalled_load_events_from_pathsonce for every event, so an 80k-event export held all 80kEventobjects in memory before the first byte went out. The 10,000 default was, in effect, guarding that.Summary
iter_events_for_exportwalks the timestamp-sorted index in batches ofEVENT_INDEX_REBUILD_BATCH_SIZE(default 200, the setting the index rebuild already uses) and loads one batch at a time. Ordering and the skip-missing-events behaviour are unchanged. The filesystem, S3 and GCS backends all inherit this implementation.LiveStatusAppConversationServiceInjector.export_max_eventsdefault goes from 10000 to 0, which_validate_conversation_export_sizealready treats as "no limit". Operators who want a cap can setOH_APP_CONVERSATION_EXPORT_MAX_EVENTS, and the 413 mapping is unchanged.Issue Number
OHE-3237
How to Test
uv run pytest -q tests/unit/app_server/test_filesystem_event_service.py tests/unit/app_server/test_live_status_app_conversation_service.py tests/unit/app_server/test_app_conversation_router.py(319 passed; Docker must be running for the Postgres harness).uv run pre-commit run --config ./dev_config/python/.pre-commit-config.yaml --files openhands/app_server/event/event_service_base.py openhands/app_server/app_conversation/live_status_app_conversation_service.py tests/unit/app_server/test_filesystem_event_service.py tests/unit/app_server/test_live_status_app_conversation_service.py(all hooks pass, including mypy).GET /api/v1/app-conversations/{id}/download. Before this change the request fails with 413. After it, the zip containsmeta.jsonplus oneevent_*.jsonper event. SettingOH_APP_CONVERSATION_EXPORT_MAX_EVENTS=5and restarting brings the 413 back for any conversation with more than 5 events.Steps 1 and 2 were run. Step 3 was not. Outside the suite I also checked that
OH_APP_CONVERSATION_EXPORT_MAX_EVENTS=5resolves to a limit of 5 throughconfig_from_env(), and that an 80,000-entry zip written through_StreamingZipBufferreads back intact.Video/Screenshots
N/A (backend-only change).
Type
Notes
EVENT_SERVICE_LOAD_EVENT_CONCURRENCY(default 10), so an 80k-event download from S3 or GCS can stream for several minutes. The Redis export lock (1 h TTL, refreshed every 30 s) already covers that. If the SaaS ingress enforces a total response-time limit, a cap can be pinned throughOH_APP_CONVERSATION_EXPORT_MAX_EVENTSin the chart without a code change.zipfilewrites a ZIP64 archive, which works on the streaming buffer.count_eventsalso countsindex.jsonandindex_stale.json, so an operator-set cap trips up to two events early. Both deserve their own tickets.Enterprise server image for this PR: