Skip to content

fix(agent): keep idle event streams alive#2032

Merged
tatoalo merged 2 commits intomainfrom
fix/cloud-agent/idle-events-socket-sse-ka
May 5, 2026
Merged

fix(agent): keep idle event streams alive#2032
tatoalo merged 2 commits intomainfrom
fix/cloud-agent/idle-events-socket-sse-ka

Conversation

@tatoalo
Copy link
Copy Markdown
Contributor

@tatoalo tatoalo commented May 5, 2026

Problem

cloud task event streams can sit idle while the sandbox is still healthy. The backend reader has an inactivity timeout, so idle /events sockets can disconnect and surface as Cloud stream disconnected even though retry immediately reconnects

Changes

  • emitting SSE ka-frames from the agent /events endpoint every 25 seconds, keeping the socket active at the transport layer and are ignored by SSE event parsers, so no task event or workflow heartbeat is created

@tatoalo tatoalo self-assigned this May 5, 2026
@tatoalo tatoalo marked this pull request as ready for review May 5, 2026 13:12
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 5, 2026

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
packages/agent/src/server/agent-server.ts:344-366
`sseController` is created before `enqueueSseFrame` is declared, yet `sseController.send` directly calls `enqueueSseFrame`. This works at runtime because the closure captures the binding rather than the value and `send` is never invoked before the `const` initialises, but it creates a fragile forward-reference: any future refactor that causes `sseController.send` to be called synchronously before `enqueueSseFrame` is declared (e.g. an eager callback inside `initializeSession`) would produce a TDZ ReferenceError. Declaring `enqueueSseFrame` first removes the dependency entirely.

```suggestion
          const encoder = new TextEncoder();
          const enqueueSseFrame = (frame: string): void => {
            try {
              controller.enqueue(encoder.encode(frame));
            } catch {
              clearKeepalive();
              this.detachSseController(sseController);
            }
          };

          const sseController: SseController = {
            send: (data: unknown) => {
              enqueueSseFrame(`data: ${JSON.stringify(data)}\n\n`);
            },
            close: () => {
              try {
                clearKeepalive();
                controller.close();
              } catch {
                this.detachSseController(sseController);
              }
            },
          };
```

### Issue 2 of 2
packages/agent/src/server/agent-server.test.ts:284-291
The test hard-codes `25_000` to identify the keepalive interval, duplicating the value of `SSE_KEEPALIVE_INTERVAL_MS` from production code (OnceAndOnlyOnce). If the constant is ever changed, this test will silently stop exercising the real keepalive logic without a compile-time error. Exporting the constant and importing it in the test removes the duplication.

```suggestion
        .mockImplementation(
          (callback: (_: undefined) => void, timeout?: number) => {
            if (timeout === SSE_KEEPALIVE_INTERVAL_MS) {
              keepaliveCallback.current = () => callback(undefined);
            }
            return setTimeout(() => undefined, 60_000);
          },
        );
```

Reviews (1): Last reviewed commit: "fix(agent): keep idle event streams aliv..." | Re-trigger Greptile

Comment thread packages/agent/src/server/agent-server.ts Outdated
Comment thread packages/agent/src/server/agent-server.test.ts
@tatoalo tatoalo requested a review from a team May 5, 2026 14:00
Copy link
Copy Markdown
Contributor

@skoob13 skoob13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

this.replayPendingEvents();
}
keepaliveInterval = setInterval(() => {
enqueueSseFrame(": keepalive\n\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know that it's conventional to omit the message prefix🤯

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahaha yeah, SSE have specific rules there in the parser, here we basically want to send bytes over the idle socket to avoid read timeouts, if we were to use the other data: convention, that would create a real SSE data message instead

@tatoalo tatoalo merged commit 6462dc9 into main May 5, 2026
15 checks passed
@tatoalo tatoalo deleted the fix/cloud-agent/idle-events-socket-sse-ka branch May 5, 2026 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants