Instrument pending external-event queue health - #2257
Conversation
Add an in-memory metrics collector for the pending external-event delivery queue, plus a delivery-terminal hook on ExternalEventStore so every delivered and terminal-failed transition is counted once from a single observation point. SpaceRuntime records enqueues (by source + target run/node state), flush attempts, claim conflicts, stale-session skips, and structured logs at the previously-silent skip seams, and exposes getQueueHealthSnapshot() merging cumulative counters with live gauges (depth, event-age p95, in-flight, digest backlog, persisted pending). Surface it via the space.externalEvents.queueHealth RPC. Service wires the shared metrics to the store hook.
Add a QueueHealthSummary panel to the external-events settings that fetches the daemon-wide queue-health snapshot via the new space.externalEvents.queueHealth RPC and renders cumulative counters (enqueue by source/target state, flush, delivered, failures by reason/category, skips) and live gauges (depth, age p95, in-flight, digest backlog). Add typed store helper + QueueHealthSnapshot types.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 608e9dd745
ℹ️ 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
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM/Zhipu)
Model: glm-5.1 | Client: NeoKai | Provider: GLM (Zhipu)
Recommendation: REQUEST_CHANGES
Overall this is a well-built, well-tested change. The delivery-terminal hook on ExternalEventStore (counting terminal outcomes from a single changes > 0 point), the shared-metrics wiring between service/runtime/store, and the snapshot → RPC → UI plumbing all hang together correctly, and the unit + integration tests exercise real transitions (including the no-double-count and cap-eviction paths). The findings below are refinements to metric correctness — which is exactly the thing this task exists to make trustworthy. Detailed suggestions are in the anchored line comments.
P2 — Paused-space digest requeue isn't counted as a skip (metric-contract violation). staleSessionSkips' own docstring promises it counts deferrals where the target session is "no longer live (or its space was paused)," but space-runtime.ts only records the skip for sessionLoss. A digest firing while the space is paused requeues every item silently — so the panel reports zero stale-session skips in precisely the pause-induced backlog scenario the metric should surface.
P2 — Snapshot materializes every pending row + full-age sort on each refresh. listPendingDeliveries().length constructs every delivery record solely for .length, and getPendingDeliveryAges() pulls every age into JS for an in-memory p95 sort. There's no global pending cap, so under the pathological backlog this view exists to diagnose, the synchronous RPC can block the event loop and delay real delivery. Use SQL COUNT/MIN/MAX/AVG and a bounded/approximate p95.
P2 — Enqueue counter undercounts via preservePendingDigestItem. queueForPendingNode is instrumented, but preservePendingDigestItem (called from stop() to rehouse rate-limited digest items into the pending queue) pushes genuinely-new items without recordEnqueue. Those items were never counted (they bypassed queueForPendingNode into the digest buffer), so on shutdown with a pending digest the cumulative enqueue counter ends up lower than reality and disagrees with the live queueDepth gauge. Add a recordEnqueue after the dedup guard.
P3 — Failure-category mapping has gaps + one unreachable mapping. Several real terminal reasons reach the hook but land in other: auto_pr_subscription_cleared, node_execution_cancelled, run_interests_rebuilt, run_terminal_cleanup. Separately, blocked_run_gate_not_opened is categorized but is persisted via markEventFailed (event-level), which never fires the delivery-terminal hook — so that mapping is unreachable, and the unit test asserting it exercises a path that can't occur in production. Consider routing these to deliverability and dropping the dead mapping.
What's solid: single-source-of-truth hook with changes > 0 guard prevents double counting across retry/exhaustion/cleanup paths; the listPendingDeliveries(workflowRunId?) overload is used correctly for the daemon-wide snapshot; RPC registration follows the existing space.externalEvents.* pattern (no new auth surface); the integration test mirrors the service wiring and covers zero-state, enqueue attribution, cap eviction, and delivered paths.
Address PR #2257 review feedback: - Compute persisted-pending health via SQL aggregates (summarizePendingDeliveries: COUNT/MIN/MAX/AVG + LIMIT/OFFSET p95) instead of materializing every pending row + a full in-memory sort on each snapshot read. - Count paused-space deferrals as a separate pausedSpaceSkips counter (recorded at the digest-requeue, fresh-delivery, and retry-injection pause sites), so staleSessionSkips stays a true session-loss signal and the pause-induced backlog scenario is no longer reported as zero skips. - Instrument preservePendingDigestItem with recordEnqueue so rehoused digest items keep the cumulative enqueue counter consistent with queueDepth. - Fix failure-category mapping: drop unreachable blocked_run_gate_not_opened (event-level markEventFailed never fires the delivery hook) and route auto_pr_subscription_cleared / node_execution_cancelled / run_interests_rebuilt / run_terminal_cleanup to deliverability.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM/Zhipu)
Model: glm-5.1 | Client: NeoKai | Provider: GLM (Zhipu)
Recommendation: APPROVE
Re-reviewed commit 521ae4a06 from scratch. All four REQUEST_CHANGES findings are correctly resolved, with no regressions and no new issues:
- Materialization (P2 ✓):
summarizePendingDeliveriescomputes COUNT/MIN/MAX/AVG via SQL aggregates and p95 via a singleLIMIT 1 OFFSET klookup — no per-row JS allocation or in-memory sort on refresh. The two queries are synchronous (no event-loop yield between them), so count and p95 stay consistent. - Paused-space skip (P2 ✓): New
pausedSpaceSkipscounter recorded at all three pause-deferral sites (digest-requeue, fresh-delivery, retry-injection);staleSessionSkipsis now a pure session-loss signal with a fixed docstring. Wired into the web types and the operator panel. preservePendingDigestItemundercount (P2 ✓):recordEnqueueadded after the dedup guard, so rehoused digest items keep the cumulative counter consistent with the livequeueDepthgauge.- Failure-category mapping (P3 ✓): Unreachable
blocked_run_gate_not_openeddropped (annotated why);auto_pr_subscription_cleared/node_execution_cancelled/run_interests_rebuilt/run_terminal_cleanuprouted todeliverability.
Verified empirically: typecheck, knip, lint all exit 0; external-event-store (70), queue-health-metrics (22), space-runtime-external-events (176), and web QueueHealthSummary + Settings (48) tests pass. All 6 review threads resolved; PR open and mergeable.
Nice work — the metric semantics are now internally consistent (counters vs gauges agree, the skip signals match their contracts, and the failure taxonomy covers every real terminal reason).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 521ae4a068
ℹ️ 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".
Address PR #2257 round-2 review: - Count claim conflicts on the long-horizon dispatch loop + LH retry reschedule + workflow retry reschedule, so the daemon-wide claimConflicts counter no longer omits long-horizon/retry delivery races. - Bound failure-reason cardinality: track failuresByCategory directly (stays exact) and cap finalFailuresByReason at 64 distinct reasons, folding the rest into an __other__ bucket so per-session dynamic reasons can't grow daemon memory / the snapshot payload without bound. - Record staleSessionSkips on the direct stale-session deferral path (target retains a sessionId that is no longer live) so single-event stale-worker deferrals are visible, not only digest session-loss requeues.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM/Zhipu)
Model: glm-5.1 | Client: NeoKai | Provider: GLM (Zhipu)
Recommendation: APPROVE
Re-reviewed commit 2db1edf04 (round 2) from scratch, including the dev rebase. All three round-2 findings are correctly resolved with no regressions, and the PR scope stayed clean post-rebase (13 in-scope files, no provider/core drift):
- Long-horizon / retry claim conflicts (P2 ✓):
recordClaimConflictnow fires on the LH dispatch loop (with theisDeliveryTerminal || inFlightguard correctly split so terminal skips don't count as conflicts), the LH retry reschedule, and the workflow retry reschedule.claimConflictsno longer omits LH/retry delivery races. - Failure-reason cardinality (P2 ✓):
failuresByCategoryis tracked directly (stays exact regardless of the cap), andfinalFailuresByReasonis bounded at 64 distinct reasons with an__other__overflow bucket. Existing reasons keep incrementing past the cap; only the long tail of distinct reasons folds. The sum invariant holds. Verified by a dedicated test covering cap, overflow, post-cap increment, total, and exact category. - Direct stale-session deferral (P2 ✓): the dead-session branch (
resolved.sessionIdcaptured at spawn but no longer live) now recordsstaleSessionSkips, so single-event stale-worker deferrals are visible.
Verified empirically: typecheck / knip / lint all exit 0; queue-health-metrics (23), space-runtime-external-events (176), external-event-store (70), and web (48) tests pass. All 9 review threads resolved; PR open and mergeable. CI: the affected unit shards (1-core — previously flaky and now green after the dev merge — 4-space-storage, 5-space-runtime-b, 5-space-agent-other) and the lint/knip/typecheck job all pass; the remaining Daemon Online shards were still pending at review time but are unaffected by this observability-only change.
The metric coverage is now complete and internally consistent across both delivery paths (workflow + long-horizon), retries, and all skip/deferral seams.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2db1edf044
ℹ️ 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".
Address PR #2257 round-3 review: - Record staleSessionSkips on the post-activation deferral branch (activation returned a session that is already non-live), matching the pre-activation stale-session path. - Classify long-horizon retry-exhaustion failures ('long-horizon agent unavailable', 'long-horizon event delivery unavailable') as retry_exhausted instead of landing in 'other'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02de563de0
ℹ️ 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".
|
|
||
| function recordToObject(map: Map<string, number>): Record<string, number> { | ||
| const obj: Record<string, number> = {}; | ||
| for (const [key, value] of map) obj[key] = value; |
There was a problem hiding this comment.
__proto__ failure reasons in snapshots
When a terminal delivery's free-form failure reason is exactly __proto__—which is possible because runtime exceptions are persisted verbatim—assigning it to this ordinary object invokes the inherited prototype setter instead of creating an own property. The serialized snapshot then omits that failure, causing the UI's terminal-failure total and success rate to undercount and disagree with the separately tracked category total; construct a null-prototype object or use Object.fromEntries.
Useful? React with 👍 / 👎.
1 participant
Adds metrics, structured logs, and an operator/debug view for the pending external-event delivery queue.
ExternalEventQueueMetricsin-memory collector (enqueue by source + target run/node state, flush attempts, claim conflicts, stale-session skips, delivered, terminal failures by reason) + failure-category breakdown.ExternalEventStoregets a delivery-terminal hook (fires once per real terminal transition viachanges > 0) so delivered/failed outcomes are counted from a single point, plusgetPendingDeliveryAgesfor the age gauge.SpaceRuntimerecords enqueues/flushes/skips at the queue seams, emitslog.debugat the previously-silent skip points (claim conflict, stale session, deliverability guards), and exposesgetQueueHealthSnapshot()merging counters with live gauges (depth, event-age p95, in-flight, digest backlog, persisted pending).space.externalEvents.queueHealthRPC;SpaceRuntimeServicewires the shared metrics to the store hook.QueueHealthSummarypanel (under Space → Configure → Settings → Tools → external events) renders the daemon-wide snapshot.Counters are process-lifetime (reset on restart); gauges are computed at read time. Covered by unit tests for the metrics class, the store hook/age query, and a
SpaceRuntimesnapshot integration test, plus a web component test.