🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🟡 Health monitor restart suppression for stuck busy channels via fresh inbound events (availability/DoS risk)

Description

evaluateChannelHealth() was changed to treat a busy channel with stale run activity as healthy if it is still receiving fresh events (lastEventAt). The channel health monitor only restarts channels when health.healthy === false, so this change can prevent automatic recovery from stuck runs.

Impact:

• If an attacker (or any external user) can trigger inbound events (e.g., messages/reactions) that update lastEventAt, they can keep lastEventAt within the staleEventThresholdMs window.
• When a run is actually stuck (run activity stale >= 25 min), the monitor will now skip the restart as long as events keep arriving.
• This can prolong a degraded or hung channel indefinitely and may also keep readiness probes green, reducing detection and recovery.

Vulnerable logic:

if (runActivityAge < BUSY_ACTIVITY_STALE_THRESHOLD_MS) {
  return { healthy: true, reason: "busy" };
}
if (hasFreshCurrentLifecycleEvent(snapshot, policy, lastStartAt)) {
  return { healthy: true, reason: "healthy" };
}
return { healthy: false, reason: "stuck" };

Note: lastEventAt is commonly updated from network-sourced inbound events (e.g., Slack/Discord monitors call setStatus({ lastEventAt: Date.now() }) on every inbound event), so an external actor may influence this liveness signal.

Recommendation

Avoid allowing fresh inbound events to override stuck-run recovery, or at minimum require additional signals.

Options:

1. Keep the stuck-run restart behavior regardless of lastEventAt:

if (runActivityAge < BUSY_ACTIVITY_STALE_THRESHOLD_MS) {
  return { healthy: true, reason: "busy" };
}
return { healthy: false, reason: "stuck" };

2. If you need to reduce false positives, introduce a distinct reason and handle it explicitly (e.g., alert-only, or restart after a longer hard timeout):

if (hasFreshCurrentLifecycleEvent(snapshot, policy, lastStartAt)) {
  return { healthy: false, reason: "stuck" }; // or "stuck-with-events" handled separately
}

3. Gate event-based suppression so it cannot mask stuck runs, e.g. require activeRuns === 0 (or another run-progress indicator) before using lastEventAt to mark healthy.

Also consider emitting metrics/logging when runActivityAge is stale but events are fresh, so operators can detect a potentially stuck worker even if you choose not to restart immediately.

Analyzed PR: #41030 at commit b55127e

_{Last updated on: 2026-03-09T13:40:19Z}

Greptile Summary

This PR fixes a false-positive restart bug in the gateway channel health monitor. Connected busy channels were being restarted as "stuck" as soon as lastRunActivityAt went stale, even when the gateway WebSocket was actively delivering fresh events. The root cause was that the stale-busy branch had no way to distinguish a truly hung channel from a long-running one that is still healthy.

Changes:

• Adds hasFreshCurrentLifecycleEvent helper in channel-health-policy.ts that returns true only when: the channel is connected (not telegram/webhook), lastEventAt is a finite number, the event post-dates the current lifecycle start, and the event age is within staleEventThresholdMs.
• Calls this guard after the BUSY_ACTIVITY_STALE_THRESHOLD_MS check — if a fresh event is present, the channel returns { healthy: true, reason: "healthy" } instead of { healthy: false, reason: "stuck" }.
• Rejects future lastEventAt values (eventAge >= 0 guard) to prevent clock skew on the reporting side from silently suppressing recovery.
• Adds two new policy unit tests and one monitor integration test covering the false-positive and clock-skew regression paths.

Confidence Score: 5/5

• This PR is safe to merge — it narrows a restart condition without introducing new failure modes.
• The change is well-scoped: it adds a single guard function with clear invariants (finite timestamp, lifecycle ownership, non-future value, within threshold) and inserts it at exactly one decision point. All existing tests continue to pass conceptually, and two new policy unit tests plus one monitor integration test cover both the happy path and the clock-skew edge case. The fix cannot cause missed restarts for truly stuck channels because it only skips recovery when the gateway is provably receiving events from the current lifecycle.
• No files require special attention.

_{Last reviewed commit: b55127e}

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

Closing this as duplicate or superseded after Codex automated review.

PR #41030 is superseded by the merged and shipped transport-liveness fix in #69833/v2026.4.22. Current main solves the linked #39096 quiet-channel false-positive problem by separating app-level inbound events from provider-proven transport activity, while still treating stale busy run activity as a true stuck-run recovery signal. The PR's lastEventAt-based suppression would now conflict with that design and with the availability concern raised in the PR discussion.

Best possible solution:

Close PR #41030 as superseded by #69833/v2026.4.22. Keep the current transport-liveness plus run-heartbeat design; any future tuning of stale active-run restarts should be a new focused issue using a non-user-controlled progress signal or an explicit hard-timeout policy.

What I checked:

• Current health policy keeps stale busy runs restartable: On current main, busy channels with initialized lifecycle state return healthy only while lastRunActivityAt is younger than the 25-minute threshold; once stale, evaluation returns { healthy: false, reason: "stuck" }. There is no fresh-lastEventAt override in this branch. (src/gateway/channel-health-policy.ts:96, 46b9044c3f9d)
• Current health policy uses transport liveness, not app-event freshness: The stale-socket path explicitly says app-level events are not socket liveness and only checks lastTransportActivityAt when a connected channel reports provider-proven transport activity. (src/gateway/channel-health-policy.ts:115, 46b9044c3f9d)
• Regression tests cover the shipped behavior: Tests assert stale app events without transport activity remain healthy, and connected channels without transport tracking are not restarted as stale sockets. This covers the quiet Discord/Slack-style false-positive class without trusting inbound lastEventAt as liveness. (src/gateway/channel-health-policy.test.ts:139, 46b9044c3f9d)
• Run-state heartbeat is the progress signal for active runs: createRunStateMachine publishes lastRunActivityAt on a heartbeat while active runs exist, and Discord inbound work uses that helper. That makes stale run activity meaningful without relying on externally-triggered inbound events. (src/channels/run-state-machine.ts:48, 46b9044c3f9d)
• Superseding PR merged and shipped: PR Fix Slack socket health stale detection #69833 merged on 2026-04-22 and includes commit 38c35c48cb12e7b5ffa83e253617490c553f203d (fix: use transport activity for stale health). The v2026.4.22 changelog says channel health now uses provider-proven transport activity instead of inbound app-event freshness to prevent quiet Slack/Discord/Telegram/Matrix/local-style channels from being restarted solely because no user traffic arrived. (CHANGELOG.md:860, 38c35c48cb12)
• Related issue already closed as implemented: The linked issue Discord: health-monitor 'stuck' detection triggers excessive false-positive reconnects #39096 was closed on 2026-04-24 with a maintainer/Codex review pointing to the same transport-liveness implementation and v2026.4.22 release evidence. The PR discussion also contains an Aisle availability warning that fresh inbound events should not suppress true stuck-run recovery.

So I’m closing this here and keeping the remaining discussion on the canonical linked item.

Codex Review notes: model gpt-5.5, reasoning high; reviewed against 46b9044c3f9d; fix evidence: release v2026.4.22, commit 38c35c48cb12.