fix(collector): bound emit failure memory and word network failures accurately#6
Merged
Merged
Conversation
…ccurately Follow-up to the emit-feedback diagnostics (#5), addressing two Copilot review notes: - Cli.ts kept every failed EmitResult in an array but only ever read the count and the last one. On a run that emits per onProgress, repeated failures grew that array unbounded. Replace it with a counter plus the most recent failure. - The emit diagnostic said the collector "rejected" the emits even when the POST never reached it (connection refused/timeout/DNS — no HTTP status). Word an HTTP rejection and a delivery failure distinctly. - Collector.emit returned the full rejection body, which a caller retains; a large error page could bloat memory. Cap the stored body at 10k chars (the log line still truncates to 500 independently). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens collector emit failure handling in the CLI by (a) preventing unbounded memory growth when emits fail repeatedly and (b) improving the accuracy of the emitted diagnostic message by distinguishing HTTP rejections from delivery/network failures. It also attempts to bound memory retained from large HTTP rejection bodies.
Changes:
- Replace the unbounded
emitFailures: EmitResult[]accumulation withemitFailureCount+lastEmitFailurein the CLI. - Improve emit failure diagnostics to differentiate HTTP status failures (“rejected”) vs no-status delivery failures (“failed”).
- Cap the rejection response body stored on
EmitResultto a maximum length.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/collector/Collector.ts | Adds a cap on stored rejection bodies to reduce retained memory from large HTTP responses. |
| src/cli/Cli.ts | Bounds emit-failure tracking memory and refines end-of-run diagnostics for emit failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…running" (#7) A chrome run that captured nothing and recorded nothing was reported as success (ok:true, exit 0) and could sit on the dashboard's "running" badge forever — the agent had no signal it was broken. Root cause: the failure signals lived in stderr logs or dashboard-only state, never in the JSON envelope the agent reads. This closes those gaps and the matching violations of the logger's own two-channel contract (codes.ts: the stderr log and the envelope diagnostic for one event should carry the SAME code). - Terminal envelope on abort. If a run throws (attach failed, engine crashed, recording threw), DynamicCommand now emits a terminal envelope via onProgress — no `running` flag, ok:false, an ENGINE_FATAL diagnostic — so the collector resolves the session instead of leaving its initial "running" partial orphaned. Cli flushes that emit before exiting non-zero. - Recording outcomes are now envelope diagnostics, not just stderr. RECORD_EMPTY (no frames → empty video) and RECORD (render/upload threw) were log-only, so "no video" was invisible to a --json reader. Both now push a warn diagnostic carrying the same code as the log. - Upload failure no longer masquerades as a clean local save. S3ArtifactStore swallows failures and returns null, which #record could not tell apart from "no S3 configured". It now distinguishes the two and emits an UPLOAD diagnostic when a configured upload fails (link missing, local copy kept). - ENGINE_FATAL is now logged as well as diagnosed, so it appears in both channels per the contract. Adds test/dynamic-diagnostics.test.js (fake-tracer unit tests for the abort terminal envelope, captured-fatal, and clean-empty cases). typecheck + build clean; 44/45 tests pass (1 DB round-trip skipped). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nostic tests Addresses the two Copilot review notes on the PR: - Collector.emit buffered the whole rejection body via response.text() before slicing, so an oversized error page still caused a transient memory spike and download latency even though the stored body was capped. Read only up to MAX_BODY_CHARS off the stream, then cancel it — bounding memory and latency at the source. - Extract the emit-failure diagnostic wording into an exported emitFailureMessage() helper and cover it with focused tests: HTTP status → "rejected", no status (network) → "failed" (the regression guard so a POST that never landed isn't reported as a rejection), plus the no-body / missing-error / oversized-body edge cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t dropped
readCappedText decoded chunks with { stream: true } but never did a final
decode() flush. A response ending on a multi-byte UTF-8 sequence split
across the last chunk boundary would drop/garble that trailing character in
the stored body and logged reason. Flush after the read loop.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
burrows99
added a commit
that referenced
this pull request
Jun 19, 2026
The DX-gaps analysis enumerated six ways the CLI misled an agent; all six have since shipped. Keep the analysis verbatim as the rationale, but add a status banner + table and a per-gap resolution note pointing to where each was closed (and flagging the few sub-items still open: dashboard age-out of stale "running" (#6b), done/upload decoupling (#6c), and lockfile-based port auto-discovery (#5)). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #5, addressing the two Copilot review comments left on that PR.
What & why
1.
emitFailuresgrew unbounded (src/cli/Cli.ts)The array stored every failed
EmitResult, but only the count and the last failure were ever read. A run that emits peronProgressagainst a failing collector would accumulate one result per emit with no upper bound. Replaced with aemitFailureCountcounter +lastEmitFailurereference — same diagnostic, constant memory.2. "rejected" was inaccurate for network failures (
src/cli/Cli.ts)The diagnostic claimed the collector
rejectedthe emits even when the POST never reached it (connection refused / timeout / DNS — no HTTP status). Now an HTTP status reads as a rejection (collector <url> rejected N emit(s): HTTP 400 — …) and a delivery failure reads as such (N emit(s) to collector <url> failed: <error>).3. Unbounded rejection body retained (
src/collector/Collector.ts)Collector.emitreturned the full response body, which callers keep on the result. A collector answering a rejection with a large HTML error page could bloat memory. Capped the stored body at 10k chars; the log line still truncates to 500 independently.Testing
npm run typecheck— cleannode --test test/output.test.js— 4/4 pass (includes theCollector.emitfailed-POST contract test)🤖 Generated with Claude Code