Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #709 +/- ##
=======================================
Coverage ? 91.91%
=======================================
Files ? 207
Lines ? 29422
Branches ? 0
=======================================
Hits ? 27043
Misses ? 2379
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
Reviewed exact head 75bc2fbe833a122574fef96c670ea8c001e3e752. The atomic UPDATE prevents two callers from claiming the same row, but it does not serialize delivery to one terminal. A deterministic two-thread reproduction with two queued rows reached two overlapping sends (paste:first, paste:second, then the two submits), so this still violates the one-message-per-IDLE-cycle contract and can merge or reorder tasks. Changes requested for the inline P1.
| # Claim atomically (#164, #406): a concurrent deliver_pending call for this | ||
| # terminal can reach this point before this one commits, so only an atomic | ||
| # UPDATE, not a prior read, decides who delivers each message. | ||
| messages = claim_pending_messages(terminal_id, limit=limit) |
There was a problem hiding this comment.
[P1] Serialize the whole per-terminal delivery sequence\n\nThe atomic claim only arbitrates individual rows. If two callers both pass the preceding pending/status checks while the terminal is IDLE and at least two rows are queued, caller A claims the oldest row and caller B claims the next one; both then call send_input() concurrently. The real tmux sink performs separate paste, delay, and Enter operations with no delivery lock, so these calls can interleave. I reproduced paste:first, paste:second, enter:second, enter:first, with both rows left DELIVERED. That can concatenate/reorder tasks and consumes two messages in one IDLE cycle despite this service’s contract. Please hold a per-terminal lock across the pending read, status check, claim, send, and failure reset (or otherwise serialize that complete sequence), rather than only making the row claim atomic.
There was a problem hiding this comment.
Good catch, thanks. Pushed a fix: deliver_pending now holds a per-terminal lock across the whole read-check-claim-send-reset sequence, so two callers claiming different rows for the same terminal can no longer both call send_input concurrently. Other terminals are unaffected, each gets its own lock. Added a real two-thread regression test that fails without the lock and passes with it. Also rebased onto main to pick up the session-ownership PR that landed since this was opened.
deliver_pending() read the PENDING rows for a terminal, then marked them DELIVERED right before send_input(). That read-then-write gap is not atomic: the status-event path, the immediate-delivery path, and the retry sweep can all reach the same receiver_id concurrently, so two callers can both read a row while it is still PENDING and both deliver it. claim_pending_messages() replaces the read with a single UPDATE ... WHERE status = 'pending' ... RETURNING that both selects and marks DELIVERED in one statement. Only one caller's UPDATE matches each row; the other finds it already DELIVERED and gets nothing back for it. The PENDING/FAILED reset paths on a send failure are unchanged. Fixes awslabs#406
75bc2fb to
7f13798
Compare
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
The new per-terminal lock fixes the byte-level overlap from the prior review, but a queued contender still sends the next message in the same cached-IDLE cycle. One blocking delivery issue therefore remains.
| provider, "accepts_input_while_processing", False | ||
| ) | ||
| if not eager_eligible: | ||
| with _terminal_delivery_lock(terminal_id): |
There was a problem hiding this comment.
[P1] Coalesce concurrent delivery attempts instead of queuing them
This blocking lock prevents the two send_input() calls from overlapping, but every contender still runs the full delivery after it acquires the lock. send_input() returns immediately after submitting Enter; notify_input_sent() only arms a future status detection, while assume_processing_on_dispatch defaults to false and no provider overrides it. A caller that waited behind the first therefore sees the same cached IDLE, claims the next row, and pastes it before the first task has begun processing. The added regression test codifies this by requiring len(intervals) == 2. I reproduced two IDLE checks and paste:first, enter:first, paste:second, enter:second from two simultaneous callers. This removes byte interleaving but still consumes two messages in one IDLE cycle, so the second task can be buffered or interpreted during the first turn while both rows remain DELIVERED. Coalesce/return when delivery is already active (or track a dispatch generation/busy state under the lock), and leave the second row pending until a later ready event.
There was a problem hiding this comment.
You're right, and I confirmed it by reading notify_input_sent/get_status directly. Without assume_processing_on_dispatch, notify_input_sent only arms the next PROCESSING detection, it does not flip the cached status itself, and status_monitor.get_status() keeps returning the pre-send IDLE/COMPLETED until real terminal output runs through StatusMonitor. So the lock only decided who goes first, both contenders still passed the same stale ready check.
Pushed a follow-up commit that adds a per-terminal busy marker alongside the lock. It is set right after a successful send_input and checked at the top of deliver_pending before touching the DB or status_monitor, so a second call for the same terminal coalesces and leaves its row PENDING instead of dispatching into the same cycle. The marker is cleared the moment InboxService.run() sees the next status event for that terminal (that only fires once the real pipeline has processed a genuine transition, so it cannot be fooled by the same stale value). It also expires after a short window on its own, so a terminal that stops producing any status event after the dispatch cannot get wedged shut against the orphaned-message reconcile sweep.
Extended the regression test: two simultaneous callers against a constantly-IDLE mocked status now assert exactly one send_input call and one row left PENDING, plus a follow-up test showing that row delivers once a later status event clears the marker, and a TestRun case confirming a PROCESSING event clears the marker even though it does not itself trigger delivery. Ran the targeted suite (test/services/test_inbox_service.py test/clients/test_database.py test/services/test_plugin_event_emission.py) in a clean container, 126/126 pass, and confirmed the new tests fail against the pre-fix code (they error on the missing busy-marker attribute, and reverting just the source changes reproduces the original two-send behavior). black and isort are clean on the diff.
The per-terminal lock added for awslabs#709 stops two callers from interleaving their tmux paste/delay/Enter sequences, but a queued caller still ran the full delivery the instant it got the lock. status_monitor.get_status() keeps returning the cached IDLE/COMPLETED from before the first send: notify_input_sent() only arms the next PROCESSING detection, it does not flip the cached status itself, and the real detection needs actual terminal output to run. A second caller that only checks status sees the same stale ready value and dispatches its own message into a terminal that has not started on the first one yet. Add a per-terminal busy marker alongside the lock. It is set right after a successful send_input and checked at the top of deliver_pending before touching the DB or status_monitor, so a concurrent or immediately following call coalesces and leaves its row PENDING for a later ready event instead of consuming it into the same cycle. The marker is cleared the instant InboxService.run() observes the next status event for that terminal (any value: a published event proves the real pipeline ran a genuine transition), and it also expires on its own after a short window so a terminal that stops producing status events after the dispatch cannot get wedged shut against the orphaned-message reconcile sweep. Extends the concurrent-delivery regression test to assert exactly one send_input call and one row left PENDING, adds a test proving that row delivers once a later status event clears the marker, and a TestRun case confirming a PROCESSING event clears the marker even though it does not itself trigger delivery.
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
Re-reviewed at exact head 49caa558d367349e99cd33783e3133d3add8b091. The busy marker fixes the simultaneous-contender reproduction, but it is not causally tied to a post-dispatch status transition: an older queued status event or the fixed timeout can remove it while the first turn is still active. The one-message-per-ready-cycle P1 therefore remains.
| # now stale. Clear the busy marker before deciding whether to | ||
| # deliver, so a real ready event right behind a dispatch is never | ||
| # starved by its own dispatch (#709). | ||
| _clear_dispatch_active(terminal_id) |
There was a problem hiding this comment.
[P1] Only clear this guard for a post-dispatch transition
A status event in this queue is not necessarily newer than the dispatch that set _dispatch_active: the immediate API, OpenCode poller, and reconcile paths call deliver_pending() outside this consumer. If an IDLE event is already queued, one of those paths can send the first row and set the marker before run() consumes the older event; this line then clears the marker and the cached IDLE lets the event path claim and send the second row in the same cycle. I reproduced that exact ordering by queueing IDLE before the first deliver_pending() call, delaying run(), and observed first, second. Independently, the five-second expiry permits the same two sends with no status event at all when startup/output latency exceeds the window. Tie the guard to a status generation/sequence captured at dispatch (or move the source-of-truth status to busy atomically); neither an unsequenced event nor elapsed wall time proves the first turn has advanced.
There was a problem hiding this comment.
Good catch, and confirmed by reading the ordering directly: the busy marker had no way to tell a status event that predates the dispatch from one that follows it.
Pushed a fix that gives StatusMonitor a monotonic per-terminal transition counter (get_status_generation), published alongside each status event. deliver_pending now snapshots that counter when it marks the terminal busy, and run() only clears the marker when the incoming event's generation is strictly newer than the one recorded at dispatch time. A stale, already-queued event can no longer clear a marker set after it.
Added two focused regression tests: one that reproduces your exact ordering (a generation-1 event queued before a dispatch that also observes generation 1, marker must survive) and one confirming a genuinely later event (generation 2) still clears it, so the fix cannot regress into never clearing. Full targeted suite 159/159 green.
…transition The busy marker set by deliver_pending was cleared by run() on ANY status event for the terminal, including one already sitting in the queue before the dispatch happened (the immediate API, OpenCode poller and reconcile paths all call deliver_pending() outside this consumer). A stale queued event could clear the marker before the real post-dispatch transition arrives, reopening the window it exists to close. status_monitor now exposes a monotonic per-terminal transition generation (get_status_generation), published alongside each status event. The busy marker records the generation at dispatch time, and run() only clears it when the event's own generation is strictly newer.
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
Re-reviewed at exact head 2d44ecdc2aabef516b6b4d6961f3959efeb972e2. The status-generation check fixes the pre-dispatch queued-event race from the previous review. The independent elapsed-time bypass remains: the guard still expires without evidence that the first turn advanced, so one blocking delivery issue remains.
| if entry is None: | ||
| return False | ||
| dispatched_at, _generation_at_dispatch = entry | ||
| if time.monotonic() - dispatched_at < INBOX_DISPATCH_COALESCE_WINDOW_S: |
There was a problem hiding this comment.
[P1] Keep the guard until readiness is re-established
The generation check now prevents an older queued event from clearing the marker, but this branch still removes it solely because five seconds elapsed. There is no contract that a provider must emit output or a PROCESSING transition inside that window; after a slow/silent start, the cached status can still be the same pre-send IDLE. The next immediate delivery (and especially the five-second OpenCode poller) then claims and sends a second row during the first turn. I reproduced this on the new head with no status event: after advancing past the configured window, two deliver_pending() calls produced first, second against the unchanged IDLE/generation. A timeout may trigger recovery/revalidation, but elapsed time alone must not authorize another send; retain the guard until a newer transition proves the cycle advanced, or atomically move the source-of-truth status out of ready at dispatch.
There was a problem hiding this comment.
You're right, and I reproduced it the same way: patching the window small and sleeping past it with the mocked status and generation held constant, deliver_pending sent twice.
Pushed a fix that removes the elapsed-time expiry entirely. The busy marker now only ever clears through the existing generation check, so a terminal that never produces another status event holds its remaining PENDING messages instead of risking a second send; the message that set the marker was already delivered. INBOX_DISPATCH_COALESCE_WINDOW_S is gone along with the time-based path.
Replaced the test that asserted the old expiry with one asserting the marker survives elapsed time, and added a regression test driving two deliver_pending calls with a real sleep past the old window and no status event, confirming only one send. Full targeted suite 129/129 pass; also confirmed the same scenario against the pre-fix code in a clean container produces two sends.
The busy marker set by deliver_pending expired on its own after INBOX_DISPATCH_COALESCE_WINDOW_S (5s), even with no status event and an unchanged generation. A provider is not contractually bound to emit output or a PROCESSING transition inside that window, so a slow or silent start left the cached status at the pre-send IDLE the whole time, and the next caller (in particular the five-second OpenCode poller) dispatched a second message into the same unconfirmed cycle. _dispatch_active now stores only the generation recorded at dispatch and is cleared exclusively by _clear_dispatch_active on a genuinely newer transition; _is_dispatch_active no longer tracks or checks elapsed time. INBOX_DISPATCH_COALESCE_WINDOW_S is removed along with it. A terminal whose provider truly never produces another status event holds its remaining PENDING messages rather than risk another interleaved send; the first message that set the marker was already delivered. Replaced the test asserting the old time-based expiry with one asserting the marker survives elapsed time, and added a regression test driving two deliver_pending calls with no status event across a real sleep past the old window, confirming only one send happens. Verified against the pre-fix code in a clean container that the same scenario (constant patched small, real sleep past it) produces two sends. Full targeted suite (test/services/test_inbox_service.py test/clients/test_database.py test/services/test_plugin_event_emission.py) 129/129 pass; black/isort clean. Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
| def claim_pending_messages(receiver_id: str, limit: int = 1) -> List[InboxMessage]: | ||
| """Atomically move up to `limit` PENDING messages for receiver_id to DELIVERED. | ||
|
|
||
| Picks the oldest PENDING rows and flips them to DELIVERED in a single | ||
| ``UPDATE ... WHERE status = 'pending' ... RETURNING``, so concurrent callers |
| event = await queue.get() | ||
| status_value = event["data"]["status"] | ||
| event_generation = event["data"]["generation"] | ||
| terminal_id = terminal_id_from_topic(event["topic"]) |
| def get_status_generation(self, terminal_id: str) -> int: | ||
| """Current value of the per-terminal transition counter. | ||
|
|
||
| Lets a caller record "no genuine transition has happened yet" (by | ||
| snapshotting this value) and later tell a status event that predates |
claim_pending_messages used UPDATE ... WHERE status = 'pending' ... RETURNING, which needs SQLite 3.35+ and is explicitly avoided elsewhere in this codebase for exactly that reason (workflow_journal.py's _connect docstring, TD-2). Replaced with a BEGIN IMMEDIATE transaction: the write lock is taken before the SELECT runs, so no other writer's claim can interleave between the pick and the flip, without depending on RETURNING to report which rows changed. InboxService.run() also indexed event["data"]["generation"] directly, but ApprovalBridge publishes to the same terminal.*.status topic without a generation field, so that event shape is already live in this codebase, not hypothetical. Defaults to 0 instead of raising; a missing generation just can't confirm a dispatch marker, it doesn't drop the status event. StatusMonitor.clear_terminal()/reset_buffer() popped every other per-terminal dict but left _status_generations behind, leaking one entry per deleted terminal and letting a stale count carry into a relaunched CLI mode on the same terminal_id. Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
…c-claim Signed-off-by: Amir Fathi <amirfathi.me@gmail.com> # Conflicts: # src/cli_agent_orchestrator/services/inbox_service.py # src/cli_agent_orchestrator/services/status_monitor.py
|
Copilot flagged three real gaps, all fixed here (head a90a05b):
Also rebased onto main to pick up #712, which touched the same files and Full targeted suite (test/services/test_inbox_service.py |
| with _delivery_registry_guard: | ||
| lock, count = _delivery_locks.get(terminal_id, (threading.Lock(), 0)) | ||
| _delivery_locks[terminal_id] = (lock, count + 1) |
There was a problem hiding this comment.
Confirmed, fixed at b14d9a5: only construct the Lock when the terminal key is absent instead of on every call.
| # Other publishers share this topic (e.g. ApprovalBridge) and don't | ||
| # carry a generation, so default to 0 rather than raise: that never | ||
| # clears a real dispatch marker early, it just leaves this one | ||
| # event unable to confirm one (see _clear_dispatch_active). | ||
| event_generation = event["data"].get("generation", 0) | ||
| terminal_id = terminal_id_from_topic(event["topic"]) |
There was a problem hiding this comment.
Confirmed, fixed at b14d9a5. ApprovalBridge only subscribes to this topic; StatusMonitor is the sole publisher and always includes a generation. Reworded the comment to say that.
| with SessionLocal() as db: | ||
| raw_conn = db.connection().connection.dbapi_connection | ||
| raw_conn.execute("BEGIN IMMEDIATE") | ||
| try: |
There was a problem hiding this comment.
Tested this directly: db.connection() marks the SQLAlchemy Core transaction as begun logically, but with the pysqlite dialect it does not issue a real BEGIN to sqlite3 until the first statement actually executes (raw_conn.in_transaction reads False right after db.connection()). The manual BEGIN IMMEDIATE right after is therefore the first real transaction start, and the existing concurrent-claim tests (test_claim_pending_messages_concurrent_threads_never_double_claim) already exercise this path with real threads and pass. Not changing this; happy to revisit if you have a repro where it raises.
_terminal_delivery_lock's default-arg .get() constructed a fresh threading.Lock() on every call regardless of whether one already existed for the terminal. Only create one when the key is absent. The generation-default comment named ApprovalBridge as a publisher without a generation field; it only subscribes to this topic and never publishes to it. StatusMonitor is the sole production publisher and always includes a generation.
|
Fixed at 59490cd: clear_terminal and reset_buffer now run a teardown hook that drops inbox_service's dispatch-active marker, so a reset inside the unconfirmed window no longer strands the terminal. The same hook reaps the marker on terminal deletion, closing the leak you flagged alongside it. Left the eager-delivery throttling question, the doc update, and the remaining test-hygiene nits for a follow-up. None of them are correctness issues on their own, and I'd rather keep this round scoped to the blocking one. 183 baseline tests plus 4 new ones (clear_terminal, reset_buffer, the leak, and the reset-mid-dispatch end to end case) pass in a clean container. The new tests fail against the prior head, confirming the reproduction. |
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 59490cdd. The new status check fixes the previously reported direct-completion case, and the reset hook fixes generation-counter teardown. One adjacent check-then-mark race still strands the inbox: a post-dispatch transition can be published and consumed after the final generation/status snapshot but before _dispatch_active is installed. I reproduced that ordering with two real pending rows; only the first send occurred and the second remained permanently coalesced. The focused existing set passes (51 tests), but it does not schedule this boundary.
haofeif re-reviewed exact head 59490cd and found one more check-then-mark window: the marker was armed from a post-dispatch generation read, so a completion event landing between that read and _mark_dispatch_active could be consumed by InboxService.run() while no marker existed yet, then this call would install one for a generation the event had already confirmed. Nothing left to arrive would ever clear it, coalescing every later message to that terminal. Arm the marker before send_input is called instead, using the pre-dispatch generation. No status event caused by this dispatch can be published before the marker exists, so the event-driven clear always has something to work against. This also drops the post-dispatch generation comparison entirely: the real transition, whenever it lands, clears the marker on its own. A failed or unresolved send now aborts the marker explicitly in both except branches, since nothing will confirm a dispatch that never reached the terminal. Reproduced the exact race from a fresh clone at 59490cd and added a regression test for it plus a companion showing the old ordering strands the marker. Docker-verified (python:3.12-slim): 189 passed across test/services/test_inbox_service.py, test/clients/test_database.py, test/services/test_plugin_event_emission.py and test/services/test_status_monitor.py; black --check clean on both changed files. Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
|
Fixed at 027cd13: the marker now arms before send_input is called (with the pre-dispatch generation), not after. That closes the window you found, since no status event caused by this dispatch can be published before the marker exists, so InboxService.run() always has something to clear against. Dropped the post-dispatch generation comparison entirely since it is no longer needed, and added an explicit abort on a failed or unresolved send so a dispatch that never reached the terminal cannot leave a marker behind either. Added a regression test that reproduces your exact ordering (event consumed while still inside send_input) plus one that replays the old ordering directly to show it strands the marker. |
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 027cd13b. Pre-arming fixes the reported post-send check-then-mark race, but two independently reproduced marker-ownership gaps remain: a pre-dispatch status event can clear the marker before input reaches the terminal (P1), and failure of a later sender group can remove protection for an earlier successful send (P2). The focused inbox suite passes (40 tests) but does not cover either ordering.
|
Both are real, and the shared cause is the same one that produced the last few rounds: P1 needs two phases, not one snapshot: an early "busy" marker (armed before prep, so P2 needs the slot to hold more than one outstanding attempt: within one I don't want to ship a ninth version of this dict shape as a quick patch. This is a redesign (a per-terminal set of outstanding attempts, each with its own confirm state), and I'd rather do it as its own pass and validate it the way the last several rounds were, with concurrent tests pinning each ordering, than patch around the current struct again. |
…ake abort attempt-specific haofeif reviewed the exact head 027cd13 and found two more races. First (P1): the marker's confirming generation was read BEFORE send_input was even called. An event about output that predates this dispatch, queued while send_input is still preparing metadata/status/provider work, could already exceed that pre-dispatch snapshot and clear the marker before the input actually crossed the backend dispatch boundary. A second caller then found the terminal not busy and sent into it while the first message was still being typed. Second (P2): with num_messages > 1, sequential sender groups under one terminal lock shared a single marker slot. If group 1's send succeeded and group 2's then failed, group 2's abort unconditionally cleared the slot, wiping the protection still owed to group 1's successful-but-unconfirmed dispatch. Both come from the same underlying gap: one shared (terminal_id -> generation) slot can't distinguish "not yet confirmable" from "confirmed", or one attempt from another. Replaced it with a list of (token, boundary) entries per terminal. Arm now records no boundary at all; only once send_input returns does a confirm set the boundary to the post-dispatch generation, so an event that landed before that point can never satisfy it. Abort and confirm both take the token _arm_dispatch_active returned, so a failed sender group only ever touches its own entry. Reproduced both races directly against the prior single-slot code (a mock send_input clearing mid-call, and a two-sender-group batch where the second group's send raises) and added regression tests for each, plus a companion confirming a real post-send transition still clears the marker normally. _mark_dispatch_active keeps its existing single-step behavior for the callers (mainly tests) that already know the confirming generation up front. Docker-verified (python:3.12-slim): 42 passed in test/services/test_inbox_service.py; black and isort clean on both changed files. Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
…c-claim # Conflicts: # src/cli_agent_orchestrator/clients/database.py # test/clients/test_database.py
|
Both fixed at e155e2b (rebased onto main at 6108d39). Replaced the single generation slot with a list of (token, boundary) attempts. Boundary stays None from arm until send_input returns, so an event consumed during dispatch preparation can no longer confirm a send that hasn't happened yet. Abort and confirm both take the token from arm, so a failed sender group only ever touches its own entry, never a sibling group's still-unconfirmed one. Reproduced both races against the prior code and added a regression test for each, plus one confirming a real post-send transition still clears normally. 205 passed (inbox + database + related suites) in a clean container, black and isort clean on the whole tree. |
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 6108d39066e2fa56dc1cf1da924e41a295d5396f against base ce18db239553407c01a817aba52129fc54657818, including the follow-up to the previously reviewed 027cd13b1aaae06bbb9b15f2740edc2cf93e6959.
Both latest findings are fixed: pre-dispatch status activity no longer removes the newly armed protection, and a later sender-group failure no longer removes ownership belonging to an earlier successful send. Permanent and one-shot transient failure controls preserve the earlier attempt.
P1 - Dispatch confirmation loses genuine post-write completion
At inbox_service.py:394-396, confirmation samples the generation only after send_input() returns. That is later than the actual backend write: terminal_service.send_input() still updates last-active metadata after send_keys() (2346-2356), before returning. A genuine completion can arrive in that interval.
If the event is consumed before confirmation, its attempt still has a None boundary and the clearing path retains it without remembering the event. If consumed afterward, confirmation has recorded the already-completed generation, so the event fails the strictly-newer comparison. There is a second manifestation of the same handshake defect when completion is consumed between the generation snapshot and _confirm_dispatch_active() storing it.
Deterministic reproduction with real SQLite claims, terminal_service.send_input, StatusMonitor, EventBus, and the real inbox consumer leaves two rows as DELIVERED, PENDING, with the terminal COMPLETED and its marker still active. Explicit delivery retries and orphan reconciliation do not release the later row; it remains suppressed until unrelated status activity or reset. This also reproduces with a genuine PROCESSING-to-COMPLETED sequence during the post-write interval, not just direct completion. Completion after confirmation works normally, and the no-transition control remains conservatively guarded. Synthetic processing on dispatch was disabled.
Preserve the early busy guard and attempt-specific ownership, but tie the handshake to the actual dispatch boundary and atomically retain/reconcile genuine post-write transitions observed during confirmation. Simply discarding preparation-time events until the entire send_input() call returns loses legitimate completions. Cover both sides of the write boundary and the snapshot-to-confirmation gap.
This is one consolidated P1 regression introduced by the follow-up, continuing the existing direct-completion finding, not two new independent blockers. The previous inbox implementation delivers both rows in the post-write comparison cases; that differential isolates the previous inbox module with current dependencies, rather than claiming a full previous-checkout baseline.
All 104 focused cases pass, but they do not catch this ordering. The corrected diagnostic harness covers 17 scenarios. Historical feedback was reviewed; no additional supported blocker was established. No current-head check-run/status results were available, so this is not a claim of passing GitHub CI.
…ration A genuine completion can land and be consumed by InboxService.run() while send_input is still executing (it keeps working after send_keys, e.g. updating last-active metadata), i.e. while the marker's boundary is still None and the event cannot confirm it. Reading a fresh post-call generation and arming it as the boundary then needs something strictly newer than an event that already happened and will not repeat, stranding the marker forever. Compare the post-call generation against the one read at arm time instead: if it already moved, resolve the attempt immediately rather than arming an unreachable boundary. Closes the eleventh round of awslabs#709 (haofeif). Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
Re-reviewed 8f17ec5, base ce18db2, against the previously reviewed 6108d39.
Changes requested: one P1, a regression of the existing pre-dispatch finding—not a new independent issue.
[P1] Do not use preparation-time activity to confirm the dispatch
At inbox_service.py:196-198, current_generation > pre_dispatch_generation also includes transitions that happen BEFORE the backend write. With cached IDLE at generation 1, let the real monitor observe COMPLETED at generation 2 during get_terminal_metadata() or inject_memory_context(), and let InboxService.run() consume it while the attempt still has a None boundary. The first backend send then succeeds, with no subsequent activity. Confirmation nevertheless removes its token because 2 > 1; the queued consumer or next drain can send the second row using that same stale COMPLETED status.
The same diagnostic against the complete previous and current checkouts gives:
| Ordering | 6108d390 |
8f17ec53 |
|---|---|---|
| Completion before the first backend write, during metadata lookup or memory preparation | One write; DELIVERED, PENDING |
Two writes; DELIVERED, DELIVERED — regression |
Genuine completion after the backend write but before send_input() returns, with early or late event consumption |
Next row stranded | Next row delivered — fixed |
| Genuine completion in the old snapshot-to-confirmation window | Next row stranded | Next row delivered — fixed |
The existing pre-dispatch regression calls _clear_dispatch_active(..., 1) but leaves the mocked monitor generation at 0. It therefore does not exercise the counter advance that makes the new confirmation branch remove the marker. Cover this with the real generation update and a second inbox row, alongside the genuine post-write case.
Neither entry to nor return from send_input() is the actual dispatch boundary: it contains both preparation and post-write bookkeeping. Keep preparation-time events from confirming the attempt while retaining genuine post-write events for atomic confirmation. Moving a blind snapshot back to either outer boundary trades one of these races for the other.
This reopens the existing pre-dispatch P1. The previous direct-completion finding is fixed in the reproduced orderings. Permanent and one-shot transient later sender-group failures still preserve the earlier successful attempt; no-transition coalescing, subsequent completion, and reset recovery also remain intact.
Evidence: 105 focused repository tests passed; the ten-scenario diagnostic ran against both complete heads using real SQLite claims, StatusMonitor, EventBus, InboxService.run(), and terminal_service.send_input(), with external I/O and timing controlled and synthetic PROCESSING disabled. GitHub CI, Secret Scan, and cargo-deny currently report action_required, not successful checks.
…ion, not the pre-call one The eleventh-round fix read the boundary generation before send_input was even called, then compared it against a post-call read to detect a transition landing during the call. That window includes send_input's own prep (get_terminal_metadata, inject_memory_context), which runs before the actual backend write. A transition observed during prep predates this dispatch and is not evidence of a response to it, so the comparison could mistake leftover prep-window activity for confirmation and drop the marker before the terminal had actually started on the message. Have status_monitor.notify_input_sent snapshot the generation itself, right where send_input calls it: after prep, immediately before the write. InboxService now confirms against that snapshot instead of one taken at arm time. A transition during prep is already folded into it and cannot register as newer; only a transition at or after the write can. Closes the twelfth round of awslabs#709 (haofeif). Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
Re-reviewed 0d36424 against base ce18db2, comparing the complete previous 8f17ec5 checkout.
Changes requested: the same existing pre-dispatch P1 is only partially addressed. No new independent finding.
The metadata-lookup and memory-injection cases are now fixed. However, notify_input_sent() is still before, not atomic with, the actual backend write. After the snapshot at status_monitor.py:611, the real send_input path still clears the rolling buffer and marks provider input. The backend also does work before application delivery: TmuxClient.send_keys loads a buffer before pasting it.
Let the real monitor observe COMPLETED at generation N+1 after notify_input_sent() captured N, but while load-buffer is running, before any paste-buffer or Enter call. The inbox consumer consumes that event while the attempt boundary is None. The first paste and submission then finish with no subsequent status transition. Confirmation still removes the token because N+1 > N, allowing the queued delivery to claim and send a second row against stale COMPLETED.
| Transition ordering | 8f17ec53 |
0d364240 |
|---|---|---|
| During metadata lookup or memory injection, before notify | Two pastes; both rows DELIVERED | One paste; second row PENDING — fixed |
| After notify but before rolling-buffer clearing, during provider marking, or in load-buffer before paste | Two pastes; both rows DELIVERED | Same incorrect early release — P1 remains |
| Genuine completion after Enter while the backend call is still running, or during post-write bookkeeping | Next row delivered | Still correct |
This uses real SQLite claims, StatusMonitor, EventBus, InboxService.run(), terminal_service.send_input(), TmuxBackend, and TmuxClient.send_keys(); only external I/O and scheduling were controlled. All three remaining pre-write placements observed zero paste and zero Enter calls at the status transition. Synthetic PROCESSING was explicitly disabled. No real tmux session was created.
The new fake send_input test reduces the send path to preparation -> notify -> success, so it omits precisely this remaining pre-paste interval. The fix needs to coordinate the dispatch boundary with status observation, rather than moving a blind snapshot to another earlier call. Preserve genuine post-write completion as well as pre-write protection; moving the snapshot after the whole send would reintroduce the previously fixed loss of completion.
Keeping the existing pre-dispatch P1 open. The prior completion-loss/snapshot-gap fixes, no-transition conservatism, reset recovery, and permanent/one-shot transient sender-group ownership controls remain intact. This is a standalone review against the stated base, not a dependency on another unmerged PR.
…c-claim # Conflicts: # src/cli_agent_orchestrator/services/status_monitor.py
…he pane write notify_input_sent's snapshot runs after send_input's prep but before clear_rolling_buffer, provider.mark_input_received, and the backend's own pre-write steps, so a transition landing in that window was still treated as prep-window noise instead of a real response. Threads a pre_write_hook through send_keys down to TmuxClient (fires between load-buffer and paste-buffer) and HerdrBackend (fires immediately before send-text); terminal_service.send_input passes status_monitor.mark_pre_write as the hook, overwriting the earlier snapshot at the last point before the pane is actually touched. Closes the thirteenth round of awslabs#709 (haofeif). Signed-off-by: Amir Fathi <amirfathi.me@gmail.com>
|
Found a further gap in the same window while re-reading the twelfth-round fix: notify_input_sent's snapshot lands after send_input's prep but before clear_rolling_buffer, provider.mark_input_received, and the backend's own pre-write steps, so a transition in that stretch still read as prep noise instead of a real response. Threaded a pre_write_hook through send_keys (TmuxClient fires it between load-buffer and paste-buffer, HerdrBackend immediately before send-text) so status_monitor can re-snapshot right before the pane is actually touched. Pushed at 06b1442. |
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
Re-reviewed 06b1442 against the exact ancestral main base aacd835, including comparison with the complete previously reviewed 0d36424 tree.
The previous pre-dispatch P1 is fixed and accepted. The transport hook now folds completion during buffer clearing, provider marking, and tmux load-buffer into the final pre-write snapshot. Those placements retain the first attempt and leave the second row PENDING, rather than prematurely delivering both. Genuine post-submit completion still releases the attempt with early or late event consumption, including completion while the backend call is finishing and during last-active bookkeeping. The metadata/memory, stale-ready-event, reset, and sender-group ownership repairs remain intact. I am resolving the existing pre-dispatch thread rather than carrying it forward.
One newly identified P2 remains: the shared guard is not connected to Herdr's native status producer. This is an integration gap introduced by the PR's guard, not a regression in the latest pre-write hook. The actual HerdrInboxService event loop calls deliver_pending on native ready events without advancing or publishing the generation that the guard requires. Native get_status() also returns the provider's result without recording that transition.
The composed native path uses the real socket-event JSON decoder, Codex provider/native-status resolver, Herdr backend, SQLite inbox, status monitor, and delivery service. After native working followed by done or idle, and after honoring the provider's documented completion flush wait, get_status() is COMPLETED but the head still has generation 0, one paste, and DELIVERED/PENDING rows. Repeated ready events, immediate delivery, and reconciliation cannot release it. The identical current-broadcast and supported legacy event cases deliver both rows against the exact base. CLI subprocess responses and time were controlled; this is not live authenticated provider acceptance.
The inline finding includes the correction direction. P2 reflects loss of automatic inbox progression on the optional Herdr backend; the queued messages are retained, and the tmux preparation-window repair is accepted.
| with self._delivery_lock(terminal_id): | ||
| self._deliver_pending_locked(terminal_id, num_messages, registry) | ||
| with _terminal_delivery_lock(terminal_id): | ||
| if _is_dispatch_active(terminal_id): |
There was a problem hiding this comment.
[P2] Connect Herdr's native progress to the dispatch guard
This gate stays closed after the first successful Herdr delivery. api/main.py:1308-1314 wires this service into HerdrInboxService, whose native-event loop calls _deliver() on idle/done (herdr_inbox_service.py:536-540) but never advances StatusMonitor's generation or publishes its status topic. The native branch of StatusMonitor.get_status() merely returns provider.get_status(). Consequently the first send leaves an active boundary of 0, and later native working/ready events cannot clear it; this early return also blocks the immediate and reconciliation paths.
With the real Herdr event decoder and Codex native-status path, after the documented 10-second flush wait the provider reports COMPLETED, yet the head retains DELIVERED/PENDING rows, one paste/two Enters, and generation 0. Current pane.updated working-to-done and working-to-idle sequences both behave this way. The identical sequences deliver both rows on base aacd835a.
Have genuine native busy/ready transitions participate in the shared sequenced confirmation mechanism before the delivery callback, retaining per-terminal serialization, stale-event rejection, and no-progress coalescing. Add coverage through HerdrInboxService._event_loop() and the real inbox service: manually injecting _apply_detection() events into a transport-only case hides this missing producer integration. Simply expiring or bypassing the guard would reintroduce the earlier stale-ready problem.
|
Since your last review, I found and fixed two more gaps in the same dispatch-slot logic while re-reading the code (e155e2b, then a pre_write_hook fix). It's been quiet for about two weeks; flagging in case another look is due. |
Fixes #406.
deliver_pending()read a terminal's PENDING inbox rows withget_pending_messages(), then marked them DELIVERED right beforesend_input(). That is a read-then-write gap, not an atomic claim: the status-event path, the immediate-delivery path, and the retry sweep can all calldeliver_pending()for the same terminal, so two of them can read the same PENDING row before either one marks it, and both deliver it.claim_pending_messages()(indatabase.py) replaces the read with a singleUPDATE inbox SET status = 'delivered' WHERE id IN (SELECT ... WHERE status = 'pending' ORDER BY created_at LIMIT n) RETURNING .... That statement both picks and marks the rows atomically, so only one caller's UPDATE matches each row; every other caller finds it already DELIVERED and gets nothing back for it.deliver_pending()now calls this instead of the old read-plus-loop, and the existing PENDING/FAILED reset paths on a send failure are untouched.I confirmed every entry point into inbox delivery goes through
deliver_pending()(the status-event consumer,poll_opencode_pending_messages(), and both call sites inapi/main.py), so the atomic claim covers the whole surface described in the issue, not just one path into it.Verification
test_claim_pending_messages_only_one_caller_winsandtest_claim_pending_messages_concurrent_threads_never_double_claim(two real OS threads racing the same rows over an on-disk SQLite file), both fail against the oldget_pending_messages()read-based approach and pass againstclaim_pending_messages().deliver_pending()unit tests (test_inbox_service.py,test_plugin_event_emission.py) that asserted the old DELIVERED-then-maybe-reset sequence.main. I did not verify the threeTestSessionOwnershipIntegrationcases or thetest_manifest_freeze/test_git_baseline/test_path_validationcases either way, since they fail identically with and without this diff in a plainpython:3.1x-slimcontainer (missingtmux, running as root, and so on), unrelated to this change.black,isortclean on the changed files;mypyreports no new issues.