[MdTanwer]

HUMAN:

I reviewed and tested the bounded dispatcher KV state changes locally, and verified that all test suites pass cleanly.

AGENT:

---

Why

The agent conversation dispatcher records each subject's conversation under agent-conversation-<uuid5> in the automation's shared KV document. The KV store has a 64 KiB document cap. Over time, completed conversation records accumulate unboundedly, causing subsequent PUT requests to fail with HTTP 413 Payload Too Large. Furthermore, running naive cleanup on every webhook dispatch causes N+1 KV round-trips once over the limit.

Summary

• Bound total dispatcher conversation KV records using high (50) and low (35) watermarks to avoid N+1 round-trips on every dispatch.
• Batch-prune oldest completed conversation records down to the low watermark when exceeding the cap.
• Protect active and running conversations from eviction during pruning.
• Add self-healing recovery on HTTP 413 Payload Too Large by aggressively removing completed records and retrying.
• Replace silent exception swallowing in KV listing and delete calls with diagnostic warnings to stderr.
• Document deduplication tradeoff and expected KV listing response shape contracts.

Issue Number

Fixes #687

How to Test

Run the dispatcher unit tests and consumer test suites:

uv run pytest -q tests/test_agent_conversation_dispatch.py
uv run pytest -q tests/test_agent_conversation_dispatch.py tests/test_github_reviewer_delivery.py tests/test_github_triage_delivery.py tests/test_github_issue_to_pr.py
python3 scripts/sync_extensions.py --check

All 168 tests pass and sync checks exit cleanly.

Video/Screenshots

N/A (backend dispatcher state bounding and test suite coverage)

Notes

• Bounding state within the 64 KiB limit trades long-term deduplication history for oldest completed subjects to prevent service outage.
• Record schema (subject, conversation_id, delivery, head, started_at) remains 100% backward compatible for local and cloud consumers.

[all-hands-bot]

This review was posted by an AI agent (OpenHands).

Review summary

I re-fetched the PR head (4425be47...), labels, review threads, CI results, and the current body/labels of linked issue #687. The focused suites pass locally (168 tests) and sync_extensions.py --check exits 0, but the change's central premise and its KV listing contract no longer match the automation service, which owns that API (OpenHands/automation, per AGENTS.md cross-repo boundaries).

Blocking: the aggregate 64 KiB cap this change guards against was removed

Issue #687 and this PR rest on the claim that the automation KV store combines all keys into one 64 KiB document, so per-subject records accumulate until every PUT returns HTTP 413. That was true when the issue was filed, but OpenHands/automation PR #524 ("fix(kv): remove the aggregate 64 KiB automation-state limit", merged 2026-10-02T13:31Z, one day before this head) changed storage to one encrypted row per (automation_id, key). Its client guide (docs/kv-store-client-guide.md) now states: "Max value size 64 KB - Per individual value, not a total across keys; Max keys per automation: none", and "hundreds of small values do not add up to a failure". The 413 now fires only when a single value exceeds 64 KiB.

Consequences for this head:

• The watermarks (50/35) and the 413 recovery path address a failure mode the service no longer has for these small records; per-key rows cannot overflow from accumulation.
• Evicting completed subjects is then pure loss: each eviction forfeits delivery dedup and resume continuity for that subject (the _prune_records note acknowledges this) while buying nothing per-key storage does not already provide. The smallest correct fix is to stop retaining a record per subject, not to cap the count.
• If some deployment still caps the aggregate document, that gate must be named and dated in the PR. On current main, this change deletes working dedup state to solve an already-fixed outage.

Blocking: the key-listing contract is guessed wrong

_list_dispatcher_keys issues an unpaginated GET /v1/kv and reads res.get("keys", list(res.keys())). The real endpoint (OpenHands/automation, openhands/automation/kv_router.py) returns KVListKeysResponse = {keys, count, total, limit, offset}, with limit defaulting to 100 and offset pagination. For an automation with more than 100 keys the listing is silently truncated, so _prune_records sees a partial set and never evicts below the high watermark - defeating the stated goal. The docstring's "or a dict mapping keys to values" fallback is likewise speculative. Please page with limit/offset (or limit=1000) using total to detect truncation, and drop the guessed shapes.

Non-blocking observations

• Pruning runs on every deliver(); while over the watermark it is N+1 GETs plus one cloud get/attach per record (_is_running), and a cloud get per record can also appear inside the 413 retry. Consider only listing/pruning once the count is known to be high, and batching.
• The new tests stub _kv_request with the guessed shapes ({"keys": [...]} and per-key state.get(key) returning the raw record), so they never exercise the real paginated envelope: they pass while the production path misreads the API. They also cannot catch that the aggregate cap is gone.

Because the primary premise is obsolete and the new listing call is wrong against the service that owns it, this should get a maintainer decision before merging.

🔄 CHANGES REQUESTED

[all-hands-bot]

This pruning step (and the 413 recovery below) targets an aggregate 64 KiB KV document. OpenHands/automation PR #524, merged 2026-10-02, changed KV storage to one row per (automation_id, key); the aggregate cap no longer exists and 413 now fires only for a single value over 64 KiB. These small conversation records cannot overflow from accumulation, so this watermark pruning only discards dedup/resume state. Please re-validate the premise against current main (or name the deployment that still enforces an aggregate cap) before merging.

[all-hands-bot]

The real endpoint returns KVListKeysResponse = {keys, count, total, limit, offset} with limit defaulting to 100 and offset pagination (OpenHands/automation, openhands/automation/kv_router.py). This unpaginated GET /v1/kv is silently truncated past 100 keys, so _prune_records can never evict below the high watermark, and the res.get("keys", list(res.keys())) / dict-of-values fallbacks are speculative. Page with limit/offset (or limit=1000) and use total to detect truncation.

[enyst]

I'm an AI agent (Claude Code, based on Opus 5.5) helping Engel Nyst (@enyst) with project work.

Requesting changes. all-hands-bot's 2026-10-03 blocking findings (obsolete aggregate-cap premise, unpaginated GET /v1/kv) stand. The eviction is also unsafe on its own, so I'd close this and ask for #687 to be closed or re-scoped against the per-key store, rather than iterate on it.

1. Evicting a record permanently breaks that subject on Cloud. _deliver_cloud assumes "no record means the stable id was never started". With an empty record, a conversation whose sandbox is ERROR/MISSING is restarted under the stable uuid5 id (L380), and the API never starts an id twice (main's test_cloud_run_replaces_a_conversation_whose_sandbox_is_gone relies on that). Old completed conversations are exactly the ones _prune_records evicts and the ones most likely to have lost their sandbox. After eviction, every trigger for that subject fails, and the record that would fix it is never written. A record that pointed at a replacement uuid4 conversation is lost the same way.
2. Housekeeping sits between the side effect and the record write. _prune_records runs after the conversation was started or sent a turn, but before the PUT (L243). Its per-key GET (L334) and _cloud.get (L291) are unguarded, so a transient error aborts deliver with work started and no record. The next trigger can then start a duplicate or send a second turn.
3. "Oldest" is not oldest. started_at is only written for a Cloud created delivery (L236-L239). Local records never have it, so the sort at L337 falls back to key (uuid) order. On Cloud it is the creation time, not the last activity, so a long-lived active PR is evicted before a stale one.

Overlap with #690: both rewrite _kv_request and the record-write tail of deliver, and both touch the dispatcher tests and bundle-index.js, so whichever lands second needs a manual rebase (#690 already conflicts with main). There is also a semantic conflict: #690 makes _kv_request return the whole response body. _prune_records' bare _kv_request(key, "GET") would then read the envelope, _is_running would be false for every record, and running subjects would be deleted. #690's registry key agent-conversations-in-flight does not match the agent-conversation- prefix, so it is not pruned. The two PRs would also add two separate liveness checks to one dispatcher.

Checked: _deliver_cloud is unchanged from main. The required test/sync-extensions/validate-claude-code checks have not run on this fork head.

[MdTanwer]

Closing this PR based on the review feedback. The aggregate 64 KiB cap was resolved in OpenHands/automation PR OpenHands/automation#524 (storage is now per-key rather than an aggregate document), so bounding and evicting records is no longer needed and avoids breaking Cloud resume continuity.