Skip to content

Conversation

@kuhe
Copy link
Contributor

@kuhe kuhe commented Dec 4, 2025

Issue

#7538

Description

Enable the new throwOnRequestTimeout compatibility flag on node-http-handler.
Consume the response body stream even if the status code is not 200.

Testing

Newly added unit tests:

 ✓ src/MetadataService.spec.ts (12 tests) 15ms
   ✓ MetadataService Socket Leak Checks > fetchMetadataToken - body consumption checks > should consume response body on 200 status and return token 5ms
   ✓ MetadataService Socket Leak Checks > fetchMetadataToken - body consumption checks > should consume response body on 400 status before throwing 1ms
   ✓ MetadataService Socket Leak Checks > fetchMetadataToken - body consumption checks > should consume response body on 500 status before throwing 1ms
   ✓ MetadataService Socket Leak Checks > fetchMetadataToken - body consumption checks > should include statusCode in error object for non-200 responses 1ms
   ✓ MetadataService Socket Leak Checks > fetchMetadataToken - error handling > should set disableFetchToken on 403 error 1ms
   ✓ MetadataService Socket Leak Checks > fetchMetadataToken - error handling > should set disableFetchToken on 404 error 1ms
   ✓ MetadataService Socket Leak Checks > fetchMetadataToken - error handling > should set disableFetchToken on 405 error 1ms
   ✓ MetadataService Socket Leak Checks > fetchMetadataToken - error handling > should set disableFetchToken on TimeoutError 0ms
   ✓ MetadataService Socket Leak Checks > fetchMetadataToken - error handling > should NOT set disableFetchToken on 400 error 1ms
   ✓ MetadataService Socket Leak Checks > socket cleanup verification > should not leave open handles after 400 response 1ms
   ✓ MetadataService Socket Leak Checks > socket cleanup verification > should not leave open handles after successful 200 response 1ms
   ✓ MetadataService Socket Leak Checks > throwOnRequestTimeout flag > should pass throwOnRequestTimeout: true to NodeHttpHandler 2ms

 Test Files  1 passed (1)
      Tests  12 passed (12)

Checklist

  • If the PR is a feature, add integration tests (*.integ.spec.ts).
  • If you wrote E2E tests, are they resilient to concurrent I/O?
  • If adding new public functions, did you add the @public tag and enable doc generation on the package?

@kuhe kuhe requested a review from a team as a code owner December 4, 2025 16:26

describe("socket cleanup verification", () => {
it("should not leave open handles after 400 response", async () => {
const initialResources = process.getActiveResourcesInfo();
Copy link
Contributor

Choose a reason for hiding this comment

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

process._getActiveHandles() might be a better method here but it isn't stable and could be removed in the future without warning.

await new Promise((resolve) => setImmediate(resolve));

const finalResources = process.getActiveResourcesInfo();
const socketCount = (resources: string[]) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

might be a hacky way to count sockets, open to suggestions, but i could not find any robust ways to measure socket counts before and after. The most relevant Node internals APIs might not be stable.

@kuhe kuhe merged commit 2dc10c6 into main Dec 5, 2025
7 checks passed
@kuhe kuhe deleted the fix/imds branch December 5, 2025 16:47
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