fix: stream conversation exports in bounded batches and drop the 10,000-event cap

[ak684]

HUMAN: Review the change and its validation evidence.

• A human has tested these changes.

AGENT:

---

Why

GET /api/v1/app-conversations/{id}/download returns 413 Conversation export contains N events, exceeding the limit of 10000 for any conversation past 10,000 events. Long-running enterprise conversations reach that routinely (the reported cases were roughly 1.8x and 8x the cap).

The cap came in with #14899. It was guarding a real problem: EventServiceBase.iter_events_for_export loaded every event of the conversation with one gather and kept a second full copy in a dict before yielding the first one, so the "streaming" zip only started after the whole conversation was in memory. Measured on main with the cap disabled (synthetic 2 KB events, filesystem backend): 68 MB traced peak and 7.8 s before the first event chunk at 10,001 events; 137 MB and 15.7 s at 20,000. Both grow linearly.

This builds on #438 (same approach: batched export, cap disabled by default) and additionally addresses the review findings left open there plus a count bug that made the cap trip early.

Summary

• iter_events_for_export walks the timestamp-sorted index in batches of EVENT_EXPORT_BATCH_SIZE (default 200, a dedicated knob rather than reusing the index-rebuild size), loading one batch at a time so memory is bounded by the batch. Order and skip-missing behaviour are unchanged; filesystem, S3 and GCS backends all inherit it. Prefetching the next batch was tried and measured (below) and left out.
• count_events without filters no longer counts index.json / index_stale.json as events (the guard and /events/count were up to two events high).
• export_max_events defaults to 0 (no cap) on both the service dataclass and the injector, so they agree; the injector rejects negative values (ge=0). The 413 mapping, the OH_APP_CONVERSATION_EXPORT_MAX_EVENTS override and the per-conversation Redis export lock are unchanged, so operators keep a ceiling if their ingress enforces a total response time.
• export_conversation() (bytes) documents that it buffers the whole zip; the router uses the streaming path.

Issue Number

OHE-3237 (same issue as #438)

How to Test

Docker is not available in the environment used here, so the root conftest.py Postgres container was skipped with --confcutdir=tests/unit; none of these tests need the database.

1. uv run pytest --confcutdir=tests/unit 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 tests/unit/app_server/test_aws_event_service.py tests/unit/app_server/test_event_router.py -q — passes on this branch. With only the product files reverted, the 8 new tests fail (fail-before check): batching, lazy batch loading, missing-event skip, index files excluded from the count, 500-event stream with no cap, defaults agree, negative rejected, and an end-to-end export over a real filesystem backend where a cap equal to the event count admits the export.
2. uv run pre-commit run --config ./dev_config/python/.pre-commit-config.yaml --files <the four changed files> — all hooks pass including mypy.
3. Synthetic large export (no product test data): a private script writes N synthetic events to a temp filesystem backend, runs the real open_conversation_export and validates the zip (testzip, contiguous event_NNNNNN, unique ids, ids in timestamp order, sentinel last).

Events	main, cap disabled	this branch (default)
10,001	68.2 MB peak, first event chunk 7.8 s, 11.3 s total	14.3 MB peak, 0.19 s, 11.4 s total
20,000	137.0 MB peak, 15.7 s, 23.0 s total	26.5 MB peak, 0.22 s, 23.2 s total

On main with the default cap the 10,001-event case is rejected with 413 ("contains 10002 events" because the index file was counted).

Batch barriers versus prefetch, measured with a simulated per-read latency (2,000 events, 20 ms each, concurrency 10): plain batching 6.1 s, prefetch with a cooperative yield per event 5.6 s; at 5 ms latency the prefetch variant was slower (7.2 s vs 6.1 s) because of the extra loop turns, and time to first event is identical. Under the GIL the parse work in executor threads does not overlap the zip serialisation, so the plain loop is kept and the tradeoff is stated in the docstring.

4. Against a running server: open a conversation with more than 10,000 events and use the download option in the conversation menu; before, 413; after, a zip with meta.json plus one event_*.json per event in order. Setting OH_APP_CONVERSATION_EXPORT_MAX_EVENTS=5 and restarting brings the 413 back above 5 events.

Step 4 was run on a self-hosted test install (Replicated, bundled MinIO for events, OH_APP_MODE=saas, Redis export lock required) with a run-owned conversation holding 10,010 events (9 real + 10,001 synthetic, last one a sentinel), driven through the real UI by a Playwright scenario that saves and inspects the browser download:

Image	Conversation	Result
1.61.0 (revision dd789851)	9 events	200, zip with meta.json + 9 ordered entries
1.61.0	10,010 events	413 "contains 10011 events, exceeding the limit of 10000" (index file counted), error toast
this PR (sha-37086dc, OCI revision 37086dc6, in-pod defaults 0)	10,010 events	200, 5,515,652-byte zip, 10,011 entries, contiguous event_000000..010009, unique ids, timestamp order, sentinel last; headers after 0.85 s, download complete after 24.2 s
this PR	9 events	200, same structure as 1.61.0

Unauthenticated download requests returned 401 on both images. Only the app's main container was switched; init containers, migrations, replicas and the other workloads stayed at 1.61.0, and the original image was restored and verified afterwards. Because the candidate is based on current main while the instance schema is 1.61.0, /api/v1/settings failed there (custom_secrets.is_org_shared missing) — unrelated to export and expected for an unmigrated database.

Video/Screenshots

Backend change. Browser screenshots, downloaded zips, receipts and the synthetic reproduction script are retained in the private validation record for this task.

Type

• Bug fix
• Feature
• Refactor
• Breaking change
• Docs / chore

Notes

• Residual memory that still scales with size: the index entries (already loaded by every search) and the zip central directory (one record per entry; ZIP64 above 65,535 entries works on the streaming buffer). Measured peaks above include both.
• Long exports stream one object read per event at EVENT_SERVICE_LOAD_EVENT_CONCURRENCY (default 10); an 80k-event S3 export can take minutes. Bytes now flow from the first batch onward (a batch is at most 200 reads), which is what idle-timeout ingresses key on; a total-response-time limit is the case for pinning OH_APP_CONVERSATION_EXPORT_MAX_EVENTS. A larger EVENT_EXPORT_BATCH_SIZE trades memory for fewer batch barriers.
• Not changed: both web UIs show a generic download-error toast for any failure (413/409/503); surfacing the server detail is a separate frontend change.

[ak684]

@OpenHands /codereview-roasted

Review exactly commit 5df50a8. If the PR head changes, report that and do not approve another commit. Review only: do not push code, merge, deploy, or change attribution. Check correctness, regression coverage, and whether the PR validation claims match the evidence. Keep code comments short. Post the substantive review and link it in your completion message. End the substantive review with these plain lines (no code fence), choosing one verdict:

REVIEWED_HEAD: 5df50a8
REVIEW_REQUEST: a40be4fcebf64f308785168ec495b24f
VERDICT: APPROVED

Use VERDICT: CHANGES_REQUESTED instead when findings require changes. If unable to review, explain the blocker without an approval verdict.

[openhands-ai]

I'm on it! ak684 can track my progress at all-hands.dev

[github-actions]

Coverage report

Click to see where and how coverage changed

File	Statements	Missing	Coverage	Coverage (new stmts)	Lines missing
openhands/app_server/app_conversation
live_status_app_conversation_service.py
openhands/app_server/event
event_service_base.py					55, 67-68
Project Total

This report was generated by python-coverage-comment-action

[ak684]

@OpenHands /codereview-roasted

Review exactly commit 37086dc. If the PR head changes, report that and do not approve another commit. Review only: do not push code, merge, deploy, or change attribution. Check correctness, regression coverage, and whether the PR validation claims match the evidence. Keep code comments short. Post the substantive review and link it in your completion message. End the substantive review with these plain lines (no code fence), choosing one verdict:

REVIEWED_HEAD: 37086dc
REVIEW_REQUEST: 0fd87c70321b4b7fad561e200941a56d
VERDICT: APPROVED

Use VERDICT: CHANGES_REQUESTED instead when findings require changes. If unable to review, explain the blocker without an approval verdict.

[openhands-ai]

I'm on it! ak684 can track my progress at all-hands.dev

[ak684]

Taste: good. No material findings.

Verified on head 5df50a8 (matches PR head and local checkout):

• Tests: uv run pytest --confcutdir=tests/unit 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 tests/unit/app_server/test_aws_event_service.py tests/unit/app_server/test_event_router.py -q → 368 passed (run twice). The new batching/prefetch/cancel/missing-skip tests are stable across repeats..
• Lint: pre-commit on the four changed files passes, including mypy and ruff..
• Count fix: _event_paths drops index.json/index_stale.json; e2e filesystem test confirms a cap equal to event count admits the export and count-1 rejects (guard exact.; the +2 double-count claim from the PR is consistent with both index files being counted in the old path..
• Env override: probed config_from_env() end-to-end: default export_max_events == 0, OH_APP_CONVERSATION_EXPORT_MAX_EVENTS=7 yields 7, negative → ValidationError. Service dataclass and injector defaults agree..
• Edge cases: empty conversation → 0 events, count 0, no prefetch task left behind; early aclose() cancels the prefetch (verified by test and by inspection of the finally path.); batching preserves timestamp order and skip-missing behavior unchanged..
• No prior reviews or unresolved review threads; CI green except Python tests (in_progress at review time).

Non-blocking notes (not findings): the PR's step-4 live validation (>10k events on a running self-hosted server) is still pending per the PR body and the HUMAN checkbox is unchecked — run it before merge. The synthetic performance numbers (68→14 MB, 7.8→0.18 s peak/first-chunk) were produced by a private script not in the repo and were not independently reproduced here; the code+tests do substantiate bounded-memory, flowing-first-batch behavior..

[RISK ASSESSMENT] This changes shared EventServiceBase used by filesystem/S3/GCS backends (medium blast radius) and flips the default export cap 10000→0 with the streaming zip writer holding the central directory in memory. That is a deliberate, documented tradeoff;the residual memory scaling (index entries + zip central directory) is disclosed in the PR notes and bounded (no full event-object copies).. Overall 🟡 MEDIUM: merge with caution — no code findings block merge, but the author's own pending live >10k-event validation (step 4)and a human run of it are recommended before merge..

Key architectural insight:the double-buffer async prefetch turns the previously all-or-nothing gather into a pipelined reader that is bounded by two batches,yielding bytes from the first batch onward — exactly what idle-timeout ingress proxies need to keep a long export alive..

This review was generated by an AI agent (OpenHands) on behalf of the repository's PR review workflow.

REVIEWED_HEAD: 5df50a8
REVIEW_REQUEST: a40be4fcebf64f308785168ec495b24f
VERDICT: APPROVED

[openhands-ai]

The review is posted. Summary and link:

Review posted for PR #455

I reviewed exactly commit 5df50a880c50f0471c48754020a6a00ac07fc195 (verified it matches both the PR head and the local checkout; the head has not changed).

Verdict: APPROVED — no material findings. Review link: #455 (review)

What I verified

• Tests — ran the exact five-file suite from "How to Test": 368 passed (twice; the new batching/prefetch/cancel/missing-skip tests are stable on repeat. The claimed end-to-end filesystem export (batch size 3, cap==count admits, cap==count−1 rejects) passes.
• Lint — pre-commit on the four changed files passes, including mypy and ruff (as claimed).)
• Env override — probed config_from_env() end-to-end: default export_max_events == 0 leaks through OH_APP_CONVERSATION_EXPORT_MAX_EVENTS=7 → 7, negative → ValidationError; service dataclass and injector defaults agree..
• Count fix — _event_paths excludes both index files; consistent with the PR's "10002 events" 413 report (both index.json and index_stale.json were being counted.;the guard now matches reality.
• Edge cases — empty conversation → 0 events, 0 count, and no prefetch task leaked; early generator close cancels the prefetch via the finally block;order and skip-missing are unchanged..
• CI — 12/13 checks green; Python Tests was still in_progress at review time.

Note (non-blocking)

• The PR's own step-4 live validation (>10k events on a running self-hosted server) is still pending per the PR body (HUMAN checkbox unchecked), and the synthetic performance numbers came from a private script not in the repo — so I flagged both in the review and recommended a human run of step 4 before merge. No code defect was found;the code and tests substantiate the bounded-memory/flowing-first-batch claims..
• GitHub blocks the reviewing token (ak684) from approving its own PR, so the review was submitted as a COMMENT with the VERDICT: APPROVED line, matching the custom repo guidance (submit the review state that matches your verdict — the only option available here was the state that doesn't create a stale block;there was no prior CHANGES_REQUESTED to clear）.

[ak684]

Taste: good. No material findings.

Reviewed exactly commit 37086dc6190caf30a33add5654c7dea7ebb65061 (matches PR head and local checkout; head has not changed since this request).

Correctness

• iter_events_for_export now loads EVENT_EXPORT_BATCH_SIZE-sized batches sequentially; each batch is loaded before the next, so at most one batch of Event objects is alive (no stray task to cancel on close — the prefetch variant removed cleanly). Timestamp order and skip-missing behavior preserved (batch order + by_id[entry[0]] per batch; verified by loads_in_batches, loads_lazily, skips_missing_events).
• Env knob is dedicated (EVENT_EXPORT_BATCH_SIZE, default ̈200, max(1, ..)` guards non-positive, ValueError falls back to 200).
• Count fix: _event_paths excludes index.json/index_stale.json in both the rebuild scan (behavior-neutral refactor)and _count_events_no_filter (fixes the guard and /events/count overcount). End-to-end test proves cap == event count admits exportand cap == event count − 1 rejects.

Defaults & wiring (verified end-to-end)

• Service dataclass export_max_events: int = 0 and injector Field(default=0, ge=0) agree; new tests assert both.
• config_from_env() honors OH_APP_CONVERSATION_EXPORT_MAX_EVENTS: unset → 0, =7 → 7, =-3 → ValidationError (ge=0 message). _validate_conversation_export_size short-circuits when <=0, so unlimited export skips the count (test asserts count_events not awaited). The 413 mapping is unchanged (router parametrized test covers it).
• export_conversation() docstring now discloses it buffers the whole zip; router uses streaming path.

Tests & CI

• Ran the PR's five-file suite: test_filesystem_event_service (36), test_live_status_app_conversation_service (200), test_app_conversation_router (88), test_aws_event_service +test_event_router (43) → all pass (367 passed; PR claims ̈368 — one-count discrepancy in the PR body only, no test fails).
• Pre-commit on the four changed files passes, including mypy and ruff.
• Current-head CI: python lint, frontend lint, builds (amd64/arm64), version/package checks all success;Python Tests on Linux (3.12) was in_progress at review time. No failed current-head run..

Non-blocking notes (not findings): hte PR's step-4 live >10k-event validation is still pending per the PR body and the HUMAN test checkbox is unchecked — run it before merge.

[RISK ASSESSMENT] Shared EventServiceBase change (filesystem/S3/GCS backends) with the default export cap flipped 10000→0 is 🟡 MEDIUM: no code findings block merge, but the author's own pending live >10k-event validation anda human run of it are recommended before merge. Residual memory scaling (index entries + zip central directory) is disclosed in the PR notesand bounded (no full event-object copies). Key architectural insight: batching turns the previous all-or-nothing gather into a byte-flowing reader that reaches the zip writer from the first batch onward — exactly what idle-timeout ingress proxies need for long exports.

This review was generated by an AI agent (OpenHands) on behalf of the repository's PR review workflow.

REVIEWED_HEAD: 37086dc
REVIEW_REQUEST: 0fd87c70321b4b7fad561e200941a56d
VERDICT: APPROVED

[openhands-ai]

Review posted for PR #455 (OpenHands/enterprise): pullrequestreview-5273816678

Review of commit 37086dc6190caf30a33add5654c7dea7ebb65061 — VERDICT: APPROVED

Taste: good. No material findings.

What I verified

• Correctness — iter_events_for_export loads one EVENT_EXPORT_BATCH_SIZE batch at a time (dedicated EVENT_EXPORT_BATCH_SIZE knob, default 200, max(1,…) guard, ValueError fallback), so memory is bounded by the batch; timestamp order and skip-missing behavior are preserved (covered by loads_in_batches, loads_lazily, skips_missing_events tests). The prefetch variant was removed cleanly (no lingering task on aclose()), per the GIL measurement documented in the docstring.

• Count fix — _event_paths excludes index.json/index_stale.json from both the rebuild scan (behavior-neutral refactor) and _count_events_no_filter (the actual overcount fix for the guard and /events/count). The end-to-end filesystem test proves a cap equal to event count admits the export and cap = count − 1 rejects.

• Defaults/wiring — dataclass and injector defaults agree (export_max_events: int = 0 / Field(default=0, ge=0)); verified end-to-end via config_from_env(): unset → 0, OH_APP_CONVERSATION_EXPORT_MAX_EVENTS=7 → 7, =-3 → ValidationError. Unlimited export short-circuits the count (count_events not awaited in the limit-disabled test); the 413 mapping is unchanged (router parametrized test covers it)..

• Tests & CI — Ran the PR's five-file suite locally: 367 passed (all files green;; PR body says ̈368 — a one-count discrepancy in the PR body only, no failing tests). Pre-commit on the four changed files passes, including mypy and ruff. Current-head CI: all completed jobs succeed; Python Tests on Linux (3.12) was in_progress at review time (no failed run).

• Head check — 37086dc62 matches both the current PR head and the local checkout; not changed since this request..

Non-blocking note — the PR's step-4 live >10k-event validation is still pending per the PR body, and the HUMAN test checkbox is unchecked — run it before merge (🟡 MEDIUM risk assessment: shared EventServiceBase + default cap flip 10000→0; disclosed residual memoryis bounded)..