awslabs / awslabs/cli-agent-orchestrator
Herdr inbox subscribes after its snapshot, dropping delivery events on herdr 0.9.0
- Dominant language
- Python
- Stars
- 1.3k
- Forks
- 267
- Avg merge
- 1d 23h
- Merged PRs (30d)
- 70
Description
## Summary
`HerdrInboxService` takes its state snapshot *before* it subscribes to herdr events. herdr 0.9.0 removes retained-history replay on subscribe, so every event that occurs between the snapshot and the subscribe is now lost permanently. A dropped `pane.updated` carrying `idle` or `done` means `_deliver()` never fires, so a worker finishes and the supervisor is never notified.
This is latent on herdr 0.8.x and becomes a live bug the moment a user upgrades to 0.9.0.
## Why it breaks on herdr 0.9.0
From the herdr 0.9.0 release notes, under Changed:
> New lifecycle event subscriptions now start with live events rather than replaying retained history. API clients should subscribe before taking their initial snapshot to avoid missing changes.
Replay is what currently closes the gap. Without it, the window between snapshot and subscribe is unobserved.
## Where
All references are against `upstream/main` at `ce18db23`.
`src/cli_agent_orchestrator/services/herdr_inbox_service.py`, in `_socket_loop`:
```python
:219 await self._connect()
:221 # Reconcile map against live herdr state before subscribing
:222 await self._reconcile() # snapshot: herdr api snapshot (:302)
:228 await self._subscribe_all_events() # subscribe happens last
```
Nothing else covers the gap. The event path and the status path are separate:
- Inbox delivery is event-driven only: `:529` reads `agent_status`, `:536` delivers on `idle`/`done`. A lost event is a lost delivery.
- The 1s poll (`backends/herdr_backend.py:682`, `herdr pane get`) feeds `status_monitor`, not inbox delivery, so it does not compensate.
Note also that `blocked` is not handled in the event loop, so a blocked agent already gets no event-driven delivery.
## Impact
Silent, non-deterministic missed handoffs. The supervisor waits indefinitely for a worker that has already finished. Because the reconnect path re-runs the same ordering, the gap reopens on every socket reconnect, not just at startup.
## The current ordering is not actually required
The spec scenario "Re-subscription on reconnect uses fresh pane_ids" (`openspec/specs/herdr-inbox/spec.md`) justifies reconcile-before-subscribe as a way to re-resolve `pane_id`s after compaction during a disconnect.
That rationale no longer matches the code. `_subscribe_all_events` sends no `pane_id` at all (`:477-481`):
```python
subscriptions = [
{"type": "pane.updated"},
{"type": "pane.closed"},
{"type": "workspace.closed"},
]
```
and its own docstring says so (`:470-475`):
> The subscription is a broadcast pane.updated (sent with NO pane_id) ... This is independent of `_pane_to_terminal` - no per-pane enumeration is needed.
Since the subscription does not depend on `pane_id`s, reconciling first buys nothing, and the ordering can be swapped without affecting subscription correctness.
## A wider gap: the deferred connect
`_socket_loop` holds the connection until the first terminal registers (`:215`):
```python
while not self._pane_to_terminal:
await asyncio.sleep(0.5)
```
Registration happens at `services/terminal_service.py:1476`, after the terminal is fully created and the agent is launched. Replay previously covered that entire create-and-launch window. With replay gone, an agent that reaches `idle` before the first subscribe drops its delivery event permanently.
Swapping the order inside the loop does not fix this part. It needs a decision on whether to connect and subscribe at service start rather than waiting for the first registration. The deferral exists to avoid churn from herdr closing idle connections with no active subscriptions, so a fix should keep that in mind. Filing it here because it has the same root cause.
## Proposed fix
Swap the order so the subscription is live before the snapshot is taken:
```python
await self._connect()
await self._subscribe_all_events()
await self._reconcile()
```
This keeps exactly one `events.subscribe` per connection, so the constraint documented at `:465-468` (herdr resets the connection on a second `events.subscribe`) still holds.
Any event arriving between subscribe and reconcile is then either reflected in the snapshot or delivered as an event. Both paths are idempotent with respect to delivery, but that is worth confirming as part of the fix.
## Follow-on cleanup
Once replay is gone, the machinery built to filter replayed events becomes dead weight:
- `_label_still_live()` (`:548-578`) exists only to suppress stale replayed closes, and shells out to `herdr tab list` on every event it guards. Called at `:385` and `:650`.
- The comment at `:638-639` describes the replay behavior as the reason for the guard.
Docs and tests that pin the old ordering:
- `openspec/specs/herdr-inbox/spec.md`, scenario "Re-subscription on reconnect uses fresh pane_ids" - mandates the ordering this issue proposes reversing.
- `test/backends/test_herdr_inbox_service.py:434` `test_reconcile_is_called_before_subscribe` - asserts the old ordering and should fail on the fix.
- `test/backends/test_herdr_inbox_service.py:788` `test_pane_closed_skips_delete_when_label_still_live` - covers the replay-filtering behavior.
A regression test asserting subscribe-before-snapshot would be the useful replacement for the first of those.
## Unverified
The magnitude of the dropped-event window is inferred from the herdr changelog, not from herdr's source. An empirical check on 0.9.0 (subscribe on a fresh connection, confirm an earlier `pane.closed` is not replayed) would confirm it.
Contributor guide
Assessment
This issue has not been assessed yet.