Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(W-17812967): Heroku-cli-command: Header overflow #159

Merged
merged 2 commits into from
Feb 17, 2025

Conversation

justinwilaby
Copy link
Contributor

@justinwilaby justinwilaby commented Feb 12, 2025

This PR fixes the accumulation of requestIds in the header causing a header overflow error in long running poll operations. These have been observed to be larger than 8k (Node default max) in commands such as pg:wait

This PR limits the Request-Id header size to 7k which allows 1k for other header data. If the 7k limit is exceeded, the Request-Id is stripped of all previous values and is started over.

Note that a side effect of this is that long running poll operations now have the potential of fracturing the API call 'story' in tracking data.

test

  1. build & pack (npm run build && npm pack)
  2. install the packed tarball in the Heroku CLI project
  3. comment out the break statement in packages/cli/src/commands/ps/wait.ts
  4. run the command and let it go a while
  5. OBSERVE - no header overflow is seen

@justinwilaby justinwilaby requested a review from a team as a code owner February 12, 2025 22:27
Copy link
Contributor

@k80bowman k80bowman left a comment

Choose a reason for hiding this comment

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

Could you please add a unit test or two to cover this logic? Also, since this logic touches a lot of what the CLI does, are there other commands we should be testing to make sure this change doesn't have unintended consequences?

@justinwilaby justinwilaby force-pushed the jw/fix/header-overflow branch 2 times, most recently from 15d81ea to aec06f0 Compare February 13, 2025 14:33
@justinwilaby justinwilaby force-pushed the jw/fix/header-overflow branch from aec06f0 to 3a8161a Compare February 13, 2025 14:35
Copy link
Contributor

@eablack eablack left a comment

Choose a reason for hiding this comment

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

Nice job including comments to explain why this logic is in place.

Copy link
Contributor

@k80bowman k80bowman left a comment

Choose a reason for hiding this comment

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

Thank you for adding the tests

@eablack eablack merged commit 5379d04 into main Feb 17, 2025
3 checks passed
@eablack eablack deleted the jw/fix/header-overflow branch February 17, 2025 20:15
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.

3 participants