fix(space): bound external-event delivery retries — cap defeated by flush-reset, no backoff, no failure classification [#950] by lsm · Pull Request #2500 · lsm/HyperNeo

lsm · 2026-08-15T23:33:56Z

Fixes #950. The 5-attempt external-event retry cap was defeated because both periodic-flush paths cleared the in-memory attempt counter right before re-dispatching the same delivery — a stuck target was re-injected ~once per tick for its whole TTL window (10 injections observed vs a cap of 5).

The flush now cancels only the retry timer (attempt count survives), retries back off exponentially (1s→2s→4s… capped at 30s), a terminal task/run inject failure terminalizes immediately instead of burning the budget, and retry_exhausted/ttl_expired drops emit an externalEvent.dropped event that reaches the web UI as a toast so a lost PR review comment is visible instead of a silent dead letter.

…lush-reset, no backoff, no failure classification [#950] The EXTERNAL_EVENT_RETRY_MAX_ATTEMPTS cap never bounded total injections: both periodic-flush paths cleared the in-memory attempt counter right before re-dispatching the same deliveryKey, so every failing cycle restarted the count at 0 and only the 5-minute TTL stopped the loop (10 injections observed live against a cap of 5, task #947). - flushPendingNodeQueue/Async now cancel only the retry TIMER (attempt count preserved across periodic re-dispatch); the count clears only on genuine terminal outcomes, so a stuck target exhausts its budget in ~6 injections - scheduled retries back off exponentially (min(base*2^(n-1), 30s cap)), applied to the workflow, activation, and long-horizon retry paths - a non-retryable inject failure (target task/run terminal) terminalizes the delivery immediately with a target_task_terminal reason instead of burning the retry budget on guaranteed dead letters - retry exhaustion on the workflow path now carries the retry_exhausted; marker (mirroring long-horizon) so queue-health categorizes it correctly - terminal drops (retry_exhausted / ttl_expired) surface as an externalEvent.dropped internal event (emitted from the store's delivery-terminal hook), bridged space-scoped to clients, and toasted in the web SpaceIsland so a lost PR review comment is visible instead of silently dead-lettering

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 563b227698

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

lsm

🤖 Review by glm-5.1 (Z.ai)

Model: glm-5.1 | Client: HyperNeo | Provider: Z.ai

Recommendation: REQUEST_CHANGES (own-PR — GitHub won't accept REQUEST_CHANGES from the author; marker line below is authoritative)

Recommendation: REQUEST_CHANGES

The core retry-layer fix is correct and well-verified: the flush now preserves the attempt counter so the 5-attempt cap bounds total injections (6 dispatches max vs the 10 observed live), exponential backoff (1s→30s) is applied consistently across the workflow, activation, and long-horizon retry paths, the tick is spawn-gated (no periodic bypass), all remaining clearExternalEventRetry sites are terminal-paired, and the retry_exhausted;/ttl_expired categorization drives the new drop signal through to the UI toast with exactly-once hook semantics. Daemon tests cover all four task-required scenarios. One P1 must be fixed before merge: the new SpaceIsland effect breaks Web Tests CI.

P1 — Web Tests CI is red: unhandled rejection from the new `getHub()` effect (blocking)

packages/web/src/islands/SpaceIsland.tsx:236 — void (async () => { const hub = await connectionManager.getHub(); … })() has no .catch. The Web Tests job failed: all 262 files / 6891 tests pass, but Vitest caught 35 unhandled rejections (WebSocket to ws://localhost:9283/ws closed before open (code=1006)), all attributed to SpaceIsland.test.tsx, and the JUnit guard blocks CI on it. Cause: this PR introduces the first-ever connectionManager import in SpaceIsland (0 refs on dev), and SpaceIsland.test.tsx mocks 19 modules but not connection-manager — every mount triggers a real WebSocket connect under happy-dom, and the rejection escapes the un-caught IIFE. Merge-base 6c634f1fc CI was green, so this is new in this PR. Fix: add .catch(() => {}) to the IIFE (matching the selectSpace(...).catch(() => {}) idiom on line 221 and PendingHookBanner's try/catch), and mock connection-manager in the test file.

P3 — `Sub-session not found` misclassified as retryable for terminal targets

packages/daemon/src/lib/space/runtime/space-runtime.ts:776 — includes('task/run is terminal') correctly matches the guard throw at task-agent-manager.ts:2030. But when the execution row is already cleaned up, the terminal guard is skipped and the same cancelled/archived task surfaces as Sub-session not found: <id> (TAM:2083 via rehydrate refusals at 4174/4199), which misses the match and burns the full ~31s retry budget before landing as retry_exhausted instead of terminalizing immediately as target_task_terminal. The next attempt usually hits the runtime's own lifecycle check, so impact is bounded (wasted attempts + wrong category) — but worth a follow-up classification.

P3 — Per-topic toast dedupe lasts for the whole space mount

packages/web/src/islands/SpaceIsland.tsx:247 — toastedTopics suppresses every later drop on the same topic until the user leaves and re-enters the space. A genuinely distinct drop an hour later (second PR review comment) is silently swallowed — the exact silent-loss problem this PR set out to fix. Consider keying by topic + eventId or time-bounding the set.

P3 — No web test for the new subscription; toast mock lacks `warning`

The daemon side is well tested (all four scenarios, honest assertion ranges), but the web toast path has no test. SpaceIsland.test.tsx:343 is the natural home (it already asserts toast behavior via mockToastError), and note its toast mock only exposes { toast: { error } } — toast.warning is undefined there, so the new handler would TypeError if a test delivered an event. Add the mock + one subscription/toast test alongside the P1 fix.

Verified in depth (context for the next reviewer):

Cap semantics: flush → enqueueDeliverableExternalEvent → deliverToSession catch → queueForRetry → scheduleExternalEventRetry increments on every dispatch failure; exhaustion at attempts > 5 terminalizes with the retry_exhausted; marker. Tick flushes are spawn-gated (pending executions only), not 5s-periodic — backoff is not bypassed.
In-flight claim: enqueueDeliverableExternalEvent claims synchronously at entry (space-runtime.ts:2878-2884), deliverExternalEventToWorkflowTarget re-checks at :2449, and a fired timer callback self-deletes before dispatch — no double-inject introduced (one narrow pre-existing suspension window at :2469/:2519/:2618).
Drop-signal gating: publishExternalEventDropped fires for exactly retry_exhausted;* and ttl_expired (categorizer prefix rules verified against every new reason string; target_task_terminal;*/subscription_no_longer_active correctly stay quiet). Hook fires exactly once per terminal transition, after the synchronous SQLite write.
One deliberate behavior change worth explicit sign-off: the recoverable-failure retry window is now ≈31s (1+2+4+8+16) vs effectively the full 5-min TTL before; targets recovering after ~31s now become surfaced drops. This matches the task's acceptance criteria, but it is a real reduction in recovery window.

Also noted (not PR findings): the worktree carries an uncommitted main.yml edit (exit 0; before the web vitest command) — looks like a local workaround for this exact CI failure. It isn't in the PR, but please don't push it.

…d; add tests The new externalEvent.dropped subscription was the first connectionManager import in SpaceIsland: under happy-dom (tests) every mount opened a real WebSocket that rejected, surfacing as 35 unhandled rejections that the Web Tests JUnit guard blocks on. The IIFE now .catches (same idiom as the selectSpace effect), and the test file mocks connection-manager so no socket is ever opened. The toast dedupe also keyed on topic alone, which would swallow a genuinely distinct later drop on the same topic (second PR review comment an hour later). Keyed by eventId instead — one event fanned out to multiple deliveries still collapses to one toast, but a distinct event always toasts. Adds four tests for the subscription: retry-exhausted toast, ttl-expired detail, cross-space filtering, and the dedupe semantics; the toast mock now exposes warning (the handler calls toast.warning, previously undefined in the mock).

…t dedupe window Two review findings on the drop subscription: (1) a daemon-offline mount left the Space view without an externalEvent.dropped handler for the remainder of the mount — the effect now re-subscribes whenever connectionState returns to 'connected', so drops on the new hub still surface; (2) the per-eventId dedupe never expired, which would suppress recurring drops on the same event indefinitely — entries now expire after a 60s coalescing window (with a bounded-map sweep). Tests: the fake-timer window-expiry case subscribes under real timers before freezing the clock (waitFor needs a live clock), and the reconnect case drives the real connectionState signal from 'disconnected' through 'connected'.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cedfa99363

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

…le drop listener [#950] Two review findings: - P1: a burst of sibling events could bypass the retry backoff. Each new event's delivery path flushes the target's queued older deliveries and force-dispatched them, canceling their armed timers — N events could burn the whole 5-attempt budget in milliseconds instead of the 1s/2s/4s/… schedule. The event-delivery flush now leaves a queued delivery on its timer while its backoff deadline (externalEventRetryDueAt) is unelapsed and requeues it for the timer's own dispatch. Only the delivery-retry path records a deadline: activation-success flushes (worker spawn / session recovery) remain immediate, since a just-activated target is a recovery boundary, and long-horizon deliveries never traverse this flush. - P2 (web): the drop listener registered twice on a connected mount — signal subscriptions invoke their callback immediately, so the unconditional subscribe() plus the immediate callback registered two listeners and overwrote the cleanup handle. The signal callback alone now drives both the initial subscription (connected mount) and reconnection. Tests: a burst regression asserting the first delivery's attempt gaps stay ≥ the scheduled backoff; a single-registration assertion on connected mount.

lsm

🤖 Review by glm-5.1 (Z.ai)

Model: glm-5.1 | Client: HyperNeo | Provider: Z.ai

Re-review of head e430a885a (inspected OID matches). CI green 38/38, all four threads resolved.

Verdict on the two prior findings — both correctly fixed ✅

P1 (backoff bypass) — fixed and sound. scheduleExternalEventRetry records externalEventRetryDueAt = now + delayMs when arming; flushPendingNodeQueueAsync leaves a still-cooling delivery on its timer and requeues it via queueForPendingNode (deliveryKey-deduped). I traced the interleavings that matter: the requeue races are safe because enqueueDeliverableExternalEvent claims externalEventDeliveriesInFlight synchronously before its first await; every dueAt write has a matching delete (timer fire / both clear variants / stop()), so no orphan deadline can requeue forever; the claim-conflict reschedule genuinely re-arms (timer entry deleted before the recursive call); and restart recovery re-arms fresh deadlines via preserveAttemptCount: true scheduling. The activation carve-out holds as described: sync flushPendingNodeQueue (spawn tick :8595, queued-handoff repair :8991) and scheduleActivationRetry deliberately record no deadline, and long-horizon never traverses this flush. The burst regression test's lower-bound gap assertions cannot false-fail under scheduler slack.

P2 (double drop listener) — fixed and sound. The connectionState signal callback alone drives registration (fires immediately on connected mount, again on reconnect); subscribe() tears down the previous handler after await getHub(); cancelled gates late resolution after unmount; the handlers.length === 1 assertion pins it. Per-effect toastedAt closure prevents cross-space carryover.

New findings

Everything below is P2/P3 — nothing blocking. Since the own-PR fallback requires a marker for post-approval purposes, see the recommendation line at the end.

P2 — test gap: nothing pins the scheduleActivationRetry carve-out (space-runtime.ts:3504). If someone adds a dueAt write there, the async activation-success flush at :2656 would strand queued siblings behind backoff — the exact regression your own comment warns about — and no test in the repo fails. Every activation test either uses the sync flush or a single event (which is includeCurrent and exempt). Suggest: two events queued sessionless, activation succeeds via sibling B's path, assert A dispatches immediately through the async flush.

P2 — test gap: requeue-driven queue overflow is untested (space-runtime.ts:2128). The new cooling requeue routes through queueForPendingNode, whose 50-cap terminalizes the oldest evictee (pending_node_queue_overflow) — a delivery can now die with zero injection attempts via a path this PR introduced, and preparePendingNodeQueueDispatchable also merges uncapped persisted rows. The existing overflow test is publish-driven only.

P2 — factually wrong comment (space-runtime.ts:3459): "a prior non-terminal mark would already have consumed the row update" — false. markDeliveryFailed's guard is state NOT IN ('delivered','failed'); a pending row after a non-terminal mark still matches and updates. The reorder is fine (it avoids a redundant intermediate write carrying the un-prefixed reason); reword the justification so it doesn't mislead future edits.

P2 — terminal classification is string-coupled across modules (space-runtime.ts:777 ↔ task-agent-manager.ts:2027). isNonRetryableInjectFailure matches the English substring 'task/run is terminal' thrown by another module. A reword of that throw — or a new terminal-status guard with different phrasing — silently reverts terminal classification to retryable and re-burns 5–6 injections per delivery. Extract a shared typed error or exported marker constant.

P2 — burst of >3 dropped events is mostly invisible (SpaceIsland.tsx:268 → ToastContainer.tsx:9). toasts.slice(-3) renders only the last 3; earlier toasts auto-dismiss at 8s unseen. Retry exhaustion is precisely a burst-shaped failure mode (offline backlog draining), so the PR's "visible instead of silent" goal is defeated for exactly that scenario. Suggest collapsing to a count after the first 2–3 in a window (e.g. "5 more external events dropped — view delivery log").

Minor (P3, no action required this PR): retry budget is per-process — every stop()/start() or daemon restart grants a fresh 5-attempt budget for the same persisted delivery (:5422); pre-existing and TTL-bounded but worth a comment. The cooling requeue inflates recordEnqueue counters (flush churn counted as enqueues, plus a getRun read per requeued item). Post-activation async flush at :2656 contradicts the stated "activation flushes stay immediate" rationale at one of its two boundaries (cooling siblings wait out their timers). category is string with an untyped union while the web handler branches on exactly two literals. reason crosses to clients carrying unbounded internal error text (not rendered — hygiene only). Cap-eviction (cap_eviction) drops stay un-surfaced while the longer queue residence this PR introduces slightly raises overflow odds. Web externalEvent.dropped payload is inline-typed ad hoc (matches existing idiom; shared-module payload would have been the moment). Reconnect test's final assertion is toBeGreaterThan(0) where toBe(1) would catch double registration on the recovery transition; the resubscribe-cleanup path (reconnect after successful subscribe) is never driven. Toast copy nit: "dropped for coder" can read as the agent doing the dropping.

Summary

The two review findings are genuinely fixed and the surrounding engineering is careful — the dueAt lifecycle, race-safety of the requeue, and the flush carve-outs all check out under adversarial tracing. The remainder is test coverage for invariants this PR itself documents, two comment/robustness corrections, and burst UX. None block merge on correctness grounds.

Recommendation: REQUEST_CHANGES

…urst-collapsed drop toasts [#950] Review round 4: - Export TASK_RUN_TERMINAL_MARKER from task-agent-manager and build both the inject-guard error and SpaceRuntime's non-retryable classifier from it — the throw site and classifier can no longer drift apart (the error crosses the command bus as a string, ruling out instanceof). - The cooling-down flush skip no longer requeues into the in-memory pending queue: every cooling sibling is a persisted pending delivery whose retry timer is still armed, so the timer re-attempts it and the next flush rediscovers it via collectPersistedPendingDeliveries (the persisted-only design the activation-hold branch already uses). Requeueing routed through queueForPendingNode's 50-entry guard, whose eviction could terminalize deliveries with zero injection attempts. Skipping also leaves the event record's createdAt TTL anchor untouched. - Correct the scheduleActivationRetry exhaustion comment: the store does not "consume" the row update on a non-terminal mark (its guard only excludes already-terminal rows); the reorder avoids a redundant intermediate write carrying the un-prefixed reason. - Web: a burst of drops beyond the first three collapses into one counted summary toast when the 10s burst window closes — the toast container only renders the last three toasts, so per-event toasts were invisible for exactly the burst-shaped exhaustion scenario this surfaces. Tests: activation carve-out regression (queued sibling dispatches in the activation-success flush rather than behind its own backoff), 60-event cooling burst survives flushes with zero terminalizations and delivers after recovery, web burst-collapse summary.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f42f078757

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-16T13:12:22Z

+          summary: string;
+          category: string;
+          agentName: string;
+        }>('externalEvent.dropped', (event) => {


Replay drops missed while disconnected

When a delivery reaches retry exhaustion or TTL expiry while the browser WebSocket is disconnected, this listener cannot observe it, and reconnecting only registers a new handler. The source InternalEventBus explicitly has no persistence or replay, while this effect never reconciles the durable failed-delivery log, so the drop receives no toast even after connectivity returns. Re-establishing the listener fixes future events but not those emitted during the outage; query recent terminal drops on reconnect or persist an acknowledgement cursor.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-16T13:12:22Z

+          if (burstWindowStart === null || now - burstWindowStart >= BURST_WINDOW_MS) {
+            burstWindowStart = now;
+            burstShown = 0;
+            burstSuppressed = 0;


Flush the previous burst before resetting its counters

When the summary timer is throttled past the 10-second window, such as in a background tab, a new drop can enter this branch before the delayed callback runs. Resetting burstSuppressed here discards the previous burst's hidden-drop count while leaving burstSummaryTimer armed; subsequent suppressed drops then reuse that stale timer and can be summarized early or attributed to the wrong window. Emit/cancel the pending summary before starting the new burst, then arm a timer for the new window independently.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-16T13:12:22Z

+      // The flush takes over dispatch from any scheduled retry — cancel the
+      // timer only (see flushPendingNodeQueue for the cap-preservation rationale).
+      this.cancelExternalEventRetryTimer(item.deliveryKey);


Reset activation attempts before delivery retries

When an inactive target accumulates five activation retries and then becomes live, this flush preserves that activation count in the same map used by scheduleExternalEventRetry. If the first actual message injection then fails transiently, the delivery count advances from 5 to 6 and terminalizes immediately as retry_exhausted, so activation failures consume all of the backoff budget despite the comment describing a cap on total injections. Separate activation and injection counters, or reset the activation count at the successful-activation boundary before attempting delivery.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-16T13:12:22Z

+          category,
+          reason: event.reason,
+          agentName: delivery.agentName,


Resolve long-horizon agent names before publishing drops

For a long-horizon subscription, the delivery row stores match.agentId in agentName, and this publisher forwards that field unchanged. Consequently a retry-exhausted or TTL-expired long-horizon delivery produces a warning such as “External event dropped for ” rather than naming the configured agent, even though the repository has its handle and display name. Detect long_horizon: deliveries and resolve the agent record, or expose an agentId field and let the client resolve a display label.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-08-16T13:12:22Z

+          if (toastedAt.size > 64) {
+            for (const [key, at] of toastedAt) {
+              if (now - at >= TOAST_DEDUPE_WINDOW_MS) toastedAt.delete(key);
+            }


Enforce the toast deduplication map bound

During a large retry-exhaustion burst, every unique event ID is inserted into toastedAt, but this size check removes only entries already older than 60 seconds. Fresh evidence beyond the earlier deduplication fix is that when hundreds or thousands of drops arrive within that window, none qualify for deletion, so the supposedly bounded map grows with the entire burst and remains allocated until another later drop triggers a sweep or the Space unmounts. After sweeping expired entries, evict the oldest remaining keys until the map is actually at its intended cap.

Useful? React with 👍 / 👎.

chatgpt-codex-connector Bot reviewed Aug 15, 2026

View reviewed changes

Comment thread packages/web/src/islands/SpaceIsland.tsx Outdated

Comment thread packages/web/src/islands/SpaceIsland.tsx Outdated

lsm commented Aug 16, 2026

View reviewed changes

lsm added 2 commits August 15, 2026 20:42

chatgpt-codex-connector Bot reviewed Aug 16, 2026

View reviewed changes

Comment thread packages/web/src/islands/SpaceIsland.tsx Outdated

Comment thread packages/daemon/src/lib/space/runtime/space-runtime.ts

lsm commented Aug 16, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Aug 16, 2026

View reviewed changes

Conversation

lsm commented Aug 15, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

lsm left a comment

Choose a reason for hiding this comment

🤖 Review by glm-5.1 (Z.ai)

P1 — Web Tests CI is red: unhandled rejection from the new getHub() effect (blocking)

P3 — Sub-session not found misclassified as retryable for terminal targets

P3 — Per-topic toast dedupe lasts for the whole space mount

P3 — No web test for the new subscription; toast mock lacks warning

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

lsm left a comment

Choose a reason for hiding this comment

🤖 Review by glm-5.1 (Z.ai)

Verdict on the two prior findings — both correctly fixed ✅

New findings

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

P1 — Web Tests CI is red: unhandled rejection from the new `getHub()` effect (blocking)

P3 — `Sub-session not found` misclassified as retryable for terminal targets

P3 — No web test for the new subscription; toast mock lacks `warning`