refactor: migrate LogDisplayer to @heroku/sdk streamLogs#3722
Merged
eablack merged 7 commits intoMay 22, 2026
Conversation
Replaces the EventSource-based polling + manual session-recreate loop
with a single 'for await' over the SDK's logSession.streamLogs async
iterable. The SDK now owns:
- generation detection (Cedar vs Fir) and the per-generation log
session option shape
- forcing tail=true on Fir, since the platform doesn't support a
bounded session there
- opening the logplex_url and parsing the chunked text/plain stream
(the platform never spoke text/event-stream — EventSource was
the wrong abstraction for this endpoint)
- transparent session recreate after the platform's idle timeout
when tailing
The CLI side is now just transport plumbing (User-Agent + proxy via
a custom fetch override), an AbortController hooked to SIGINT/SIGTERM,
the EPIPE handler for piping into 'head'/'grep', and an
onSessionCreated callback that prints the legacy 'Fetching logs...'
notice for Fir on the first session.
…fault The SDK's streamLogs now defaults to a Node-aware fetch that adds a User-Agent and routes through undici.EnvHttpProxyAgent (which honors HTTP_PROXY / HTTPS_PROXY / NO_PROXY env vars). The CLI's local buildFetch wrapper is redundant and is removed. This also fixes a likely-broken proxy path: the previous CLI code passed an https-proxy-agent via the 'agent' option, which Node's native fetch ignores entirely. Proxy support has probably not worked since the CLI moved off node-fetch. The undici dispatcher in the SDK is the modern correct way to wire proxy support into native fetch.
The SDK now routes the logplex fetch through HerokuApiClient with service: 'custom', which inherits proxy + UA support from heroku-fetch. The CLI's wrapper was already removed; this just picks up the new SHA where the SDK's internals match.
The LogDisplayer class wrapped a single public method (display) and required an unused APIClient constructor parameter (kept for back- compat after the SDK migration). With the SDK now owning generation detection, transport, and session recreate, the class adds nothing — fold it into a plain async function. Both call sites (logs and run/detached) keep a static reference (Cmd.displayLogs = displayLogs) so existing sinon stubs can swap it out for tests, mirroring the pattern used by pipelines:promote's Promote.promotePipeline. (Drop the explanatory comment on those static references — the pattern's now repeated enough to be obvious.) Replaces the 412-line log-displayer.unit.test.ts (which was tightly coupled to the old EventSource + getGenerationByAppId architecture) with a small unit test that asserts on the streamLogs call arguments. Real network behavior is covered by the existing integration test at test/integration/logs.integration.test.ts.
The legacy LogDisplayer used EventSource to consume the logplex stream. With the SDK migration, that's gone — the stream is read directly as chunked text/plain, which is what the platform actually serves. No remaining imports of 'eventsource' anywhere in src/.
heroku/heroku-sdk#28 (logSession.streamLogs) has merged. Picks up @heroku/heroku-fetchbf6be07 transitively (the merged dispatcher fix plus the proxy/Accept/ky2 work).
setupProcessHandlers ran on every displayLogs() call and added a fresh process.stdout 'error' listener with no removal. Single-shot CLI invocations don't notice, but repeated calls in-process would accumulate listeners and trip Node's MaxListeners warning. Move the listener to module load (it's already process-global state either way) so each call wires only the per-invocation SIGINT/SIGTERM abort handlers. Also surface err.message ?? String(err) instead of err.stack — err.stack can be undefined on non-Error throwables. Adds a unit test for the abort-swallow path: when streamLogs rejects with an AbortError after we abort the controller, displayLogs should resolve cleanly.
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
2 participants
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.
Summary
Migrates
heroku logs(andheroku run:detached's--tailpath) from the legacyEventSource-based polling + manual session-recreate loop to the new SDK'slogSession.streamLogsasync iterable.What the SDK now owns (heroku/heroku-sdk#28)
dyno/typeseparate on Fir, collapsed on Cedar;linesCedar-only;tailalways-true on Fir).logplex_urland parses the chunkedtext/plainstream throughHerokuApiClient.stream(). The platform never spoketext/event-stream, soEventSourcewas the wrong abstraction for this endpoint and there was risk of losing lines —EventSourceparses lines prefixed withdata:/event:/id:/retry:and silently drops anything else, while logplex sends raw log entries terminated by\n. Reading the chunked stream directly delivers every byte.for awaitloop never sees the disconnect.What stays in the CLI (
displayLogs)for awaitloop drivingstreamLogs.colorize(line)per line.AbortController.abort().head/grepexits cleanly.Fetching logs...notice, surfaced via the SDK'sonSessionCreatedcallback.Drive-by cleanup: class → function
The old
LogDisplayerwas a class with a single public method (display) and an unusedAPIClientconstructor parameter. With the SDK now owning generation detection, transport, and recreate, the class added nothing — fold it into a plaindisplayLogsfunction. Both call sites (logsandrun/detached) keep a static reference (Cmd.displayLogs = displayLogs) so existing sinon stubs still work, mirroring the pattern used byPromote.promotePipeline.The 412-line
log-displayer.unit.test.ts(tightly coupled to the oldEventSource+getGenerationByAppIdarchitecture) is replaced with a small unit test that asserts on thestreamLogscall arguments and the abort-swallow path. Real network behavior is covered bytest/integration/logs.integration.test.ts.Drive-by fix: probably-broken proxy support
The legacy CLI passed
agent: HttpsProxyAgentvia fetch options, which Node's nativefetchignores entirely. Proxy support has likely been broken since the CLI moved offnode-fetch. heroku-fetch (heroku/heroku-fetch#14) now routes requests throughundici.EnvHttpProxyAgentwhenHTTP_PROXY/HTTPS_PROXY/NO_PROXYare set — those env vars now do what they say.Drive-by cleanup: drop
eventsourcedependencyThe
eventsourcepackage was only used by the oldLogDisplayer. With that gone, drop the dep frompackage.json(and 13 lines from the lockfile).Type of Change
Breaking Changes (major semver update)
!after your change type to denote a change that breaks current behaviorFeature Additions (minor semver update)
Patch Updates (patch semver update)
Testing
Notes:
<cedar-app>and<fir-app>with apps you have access to.--tailsession running for >15 min; otherwise just confirm the stream stays connected.Steps:
npm run build./bin/run logs --app <cedar-app> --num 5— bounded, prints last 5 lines and exits../bin/run logs --app <cedar-app> --tail— tails until Ctrl-C; lines arrive in real time, colorization preserved, Ctrl-C exits cleanly../bin/run logs --app <fir-app>— exercises Fir auto-tail + theFetching logs...notice../bin/run logs --app <cedar-app> --tail | head -3— exercises EPIPE handler; pipeline exits 0../bin/run run:detached -a <cedar-app> --tail echo hello— exercisesrun:detached's tail path.HTTPS_PROXY=http://your-proxy:8080 ./bin/run logs --app <cedar-app> --num 5— fetch routes through the proxy.npx mocha 'test/unit/commands/logs.unit.test.ts' 'test/unit/commands/run/detached.unit.test.ts' 'test/unit/lib/run/log-displayer.unit.test.ts' --timeout 30000— expect 19 passing.--tailsession open for >15 minutes and confirm it survives the platform's idle timeout (the SDK's auto-recreate should handle the disconnect transparently).Screenshots (if applicable)
Related Issues
GUS work item: W-22265373