Prevent Copilot SDK event-log failures from terminating agent runs#55500
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #55500 does not have the implementation label and has 0 new lines of code in business logic directories (threshold: 100).
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #55500 and found no actionable changed-line issues to comment on; no GitHub write other than completion signaling needed.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Pull request overview
Makes Copilot SDK event-log failures non-fatal while preserving stderr diagnostics.
Changes:
- Handles directory and asynchronous stream errors.
- Adds regression coverage for unavailable directories.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/copilot_sdk_session.cjs |
Adds resilient event logging. |
actions/setup/js/copilot_sdk_driver.test.cjs |
Tests synchronous directory failure. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| it("continues when the SDK event log directory is unavailable", async () => { | ||
| const unavailableBase = path.join(testSessionStateDir, "not-a-directory"); | ||
| fs.writeFileSync(unavailableBase, "file"); |
There was a problem hiding this comment.
Added a new test "continues when the SDK event log stream fails asynchronously" that pre-creates events.jsonl as a directory so createWriteStream succeeds synchronously but fails asynchronously with EISDIR. It asserts exit code 0, the SDK event log write failed warning, and the stderr event.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
L271-280: shrink: split event-log setup into a tiny helper that returns a stream or null. That would collapse the duplicate warning branches and remove the extra mutable state around eventsStream.
net: -8 lines possible.
Generated by ✂️ Ponytail Reviewer for #55500 · codex · mai10 · 4.97 AIC · ⌖ 1.97 AIC · ⊞ 16.7K
Comment /ponytail to run again
| // Snapshot to a non-null local for closure-safe writes (JSDoc nullability narrowing). | ||
| const stream = eventsStream; | ||
| log(`serialising SDK events to ${eventsPath}`); | ||
| try { |
There was a problem hiding this comment.
L271-280: shrink: split event-log setup into a tiny helper that returns a stream or null. That would collapse the duplicate warning branches and remove the extra mutable state around eventsStream.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues in the session file.
📋 Key Themes & Highlights
Key Themes
- Resource leak (
copilot_sdk_session.cjs:275):eventsStreamis nulled on error but never.destroy()-ed, leaking the underlying file descriptor for the process lifetime. - Error handler ordering (
copilot_sdk_session.cjs:281): If any code betweencreateWriteStreamand.on('error', ...)throws, an unhandled'error'event could still crash the process — defeating the resilience goal. - Test assertion ordering (
copilot_sdk_driver.test.cjs:126): Minor improvement — ordering assertions by importance improves failure message clarity.
Positive Highlights
- ✅ Root cause correctly identified and addressed (non-fatal telemetry path).
- ✅ Regression test added that covers the exact failure scenario end-to-end.
- ✅
eventsStream?.write(jsonl)optional-chain is clean and safe. - ✅ stderr fallback ensures event observability is preserved even when the file path is unavailable.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 26.5 AIC · ⌖ 10.1 AIC · ⊞ 7.6K
Comment /matt to run again
| fs.mkdirSync(sessionDir, { recursive: true }); | ||
| eventsStream = fs.createWriteStream(eventsPath, { flags: "a" }); | ||
| eventsStream.on("error", err => { | ||
| log(`warning: SDK event log write failed at ${eventsPath}: ${getErrorMessage(err)}; continuing with stderr event stream`); |
There was a problem hiding this comment.
[/diagnosing-bugs] Stream resource leak: when the "error" event fires, eventsStream is set to null but .destroy() is never called — the underlying file descriptor is abandoned for the lifetime of the process.
💡 Suggested fix
eventsStream.on("error", err => {
log(`warning: SDK event log write failed at ${eventsPath}: ${getErrorMessage(err)}; continuing with stderr event stream`);
const s = eventsStream;
eventsStream = null;
s?.destroy(); // release the underlying fd
});Node.js WriteStream holds an open file descriptor until explicitly destroyed. Long-running agent sessions may accumulate leaked fds.
@copilot please address this.
There was a problem hiding this comment.
Fixed in latest commit — the error handler now snapshots the stale stream and calls .destroy() before nulling eventsStream.
| log(`serialising SDK events to ${eventsPath}`); | ||
| } catch (err) { | ||
| log(`warning: SDK event log unavailable at ${eventsPath}: ${getErrorMessage(err)}; continuing with stderr event stream`); | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The eventsStream variable is set inside a try block but still referenced in the writeEvent closure declared after it. If mkdirSync succeeds but createWriteStream throws, eventsStream remains null — this is fine. However if createWriteStream succeeds and the subsequent on("error", ...) call is what throws, eventsStream would be non-null but without an error handler, causing an unhandled 'error' event that terminates the process. Consider moving the on('error') registration directly after the createWriteStream call with no intervening code that could throw.
@copilot please address this.
There was a problem hiding this comment.
The on("error") handler is already registered immediately after createWriteStream, with no intervening code that could throw, so this ordering concern doesn't apply as written.
| sessionStateBaseDir: unavailableBase, | ||
| sdkModule: { | ||
| CopilotClient: FakeCopilotClient, | ||
| RuntimeConnection: { forUri: vi.fn(() => ({})) }, |
There was a problem hiding this comment.
[/tdd] The test asserts logs contains a string matching "SDK event log unavailable" but never verifies that the session output reached the caller via the normal path — i.e., that result.output was populated from the in-memory event (not only from a file). The test already fires an assistant.message event, so adding expect(result.output).toContain("completed without event file") (which is present) is good. Consider also asserting result.exitCode === 0 before the stderrWriteSpy assertion so a non-zero exit surfaces a clearer failure message.
💡 Suggested assert order
expect(result.exitCode).toBe(0); // 1. session completed
expect(result.output).toContain('...'); // 2. output propagated
expect(logs).toContainEqual(...); // 3. warning logged
expect(stderrWriteSpy).toHaveBeenCalled...// 4. stderr fallbackThis ordering matches Arrange→Act→Assert importance and gives better failure messages.
@copilot please address this.
There was a problem hiding this comment.
The existing test already asserts exitCode first, then output, then the log warning, then the stderr event — matching the suggested order.
There was a problem hiding this comment.
Review: Prevent Copilot SDK event-log failures from terminating agent runs
The fix is clean and well-scoped. Wrapping mkdirSync/createWriteStream in a try-catch and gracefully degrading to stderr-only logging is the right approach. The new test covers the primary failure path.
One non-blocking suggestion: call .destroy() on the stale stream reference before nulling it in the error handler to avoid an open file-handle leak (see inline comment).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 20.8 AIC · ⌖ 9.08 AIC · ⊞ 6.2K
| log(`warning: SDK event log write failed at ${eventsPath}: ${getErrorMessage(err)}; continuing with stderr event stream`); | ||
| eventsStream = null; | ||
| }); | ||
| log(`serialising SDK events to ${eventsPath}`); |
There was a problem hiding this comment.
Non-blocking (harden): When the stream emits an error, eventsStream is set to null but the underlying stream object is not explicitly destroyed. This leaves an open file handle until GC collects it. Consider calling .destroy() on the stale reference before nulling it:
eventsStream.on("error", err => {
log(`warning: SDK event log write failed at ${eventsPath}: ${getErrorMessage(err)}; continuing with stderr event stream`);
const stale = eventsStream;
eventsStream = null;
stale?.destroy();
});@copilot please address this.
There was a problem hiding this comment.
Fixed in latest commit — the error handler now snapshots the stale stream and calls .destroy() before nulling eventsStream.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
|
@copilot This PR looks ready for forward progress. Please review the latest feedback, refresh the branch if needed, and run the
|
…-error test coverage Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Copilot workflows could exit with code 1 when the SDK failed to create its auxiliary JSONL event-log directory. This telemetry failure was retried repeatedly and terminated otherwise valid agent sessions.
Resilient event logging
Regression coverage