Skip to content

Add optional timeout parameter to HTTP client #111

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.

Sign up for GitHub

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

Open
akar1ngo wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add optional timeout parameter to HTTP client #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions dist/index.d.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 18 additions & 22 deletions dist/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/index.js.map
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion src/ids-host.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,12 @@ export class IdsHost {
prevUrl: URL,
nextUrl: URL,
) => void,
timeout?: number,
): Promise<Got> {
if (this.client === undefined) {
this.client = got.extend({
timeout: {
request: DEFAULT_TIMEOUT,
request: timeout ?? DEFAULT_TIMEOUT,
Comment on lines +49 to +54
Copy link

@coderabbitai coderabbitai bot Oct 8, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Client caching breaks per-call timeout customization.

The client is cached in this.client (line 51) and reused for all subsequent calls. Once created with a specific timeout, that timeout persists for all future requests, ignoring any different timeout values passed to later getGot calls.

For example:

  • First call: getGot(undefined, 5000) creates client with 5s timeout
  • Second call: getGot(undefined, 30000) reuses the same client (still 5s timeout)

This defeats the purpose of the timeout parameter.

Solution: Remove the client caching or make the cache timeout-aware:

Option 1 (Recommended): Remove caching

   async getGot(
     recordFailoverCallback?: (
       incitingError: unknown,
       prevUrl: URL,
       nextUrl: URL,
     ) => void,
     timeout?: number,
   ): Promise<Got> {
-    if (this.client === undefined) {
-      this.client = got.extend({
+    return got.extend({
         timeout: {
           request: timeout ?? DEFAULT_TIMEOUT,
         },
 
         retry: {
           limit: Math.max((await this.getUrlsByPreference()).length, 3),
           methods: ["GET", "HEAD"],
         },
 
         hooks: {
           beforeRetry: [
             async (error, retryCount) => {
               const prevUrl = await this.getRootUrl();
               this.markCurrentHostBroken();
               const nextUrl = await this.getRootUrl();
 
               if (recordFailoverCallback !== undefined) {
                 recordFailoverCallback(error, prevUrl, nextUrl);
               }
 
               actionsCore.info(
                 `Retrying after error ${error.code}, retry #: ${retryCount}`,
               );
             },
           ],
 
           beforeRequest: [
             async (options) => {
               // The getter always returns a URL, even though the setter accepts a string
               const currentUrl: URL = options.url as URL;
 
               if (this.isUrlSubjectToDynamicUrls(currentUrl)) {
                 const newUrl: URL = new URL(currentUrl);
 
                 const url: URL = await this.getRootUrl();
                 newUrl.host = url.host;
 
                 options.url = newUrl;
                 actionsCore.debug(`Transmuted ${currentUrl} into ${newUrl}`);
               } else {
                 actionsCore.debug(`No transmutations on ${currentUrl}`);
               }
             },
           ],
         },
       });
-    }
-
-    return this.client;
   }

Remove the client field from the constructor as well:

   constructor(
     idsProjectName: string,
     diagnosticsSuffix: string | undefined,
     runtimeDiagnosticsUrl: string | undefined,
   ) {
     this.idsProjectName = idsProjectName;
     this.diagnosticsSuffix = diagnosticsSuffix;
     this.runtimeDiagnosticsUrl = runtimeDiagnosticsUrl;
-    this.client = undefined;
   }

And remove the field declaration:

 export class IdsHost {
   private idsProjectName: string;
   private diagnosticsSuffix?: string;
   private runtimeDiagnosticsUrl?: string;
   private prioritizedURLs?: URL[];
-  private client?: Got;

Option 2: Make cache timeout-aware (more complex)

Use a Map to cache clients by timeout value, but this adds complexity and may not be worth it if clients are lightweight to create.

🤖 Prompt for AI Agents 
In src/ids-host.ts around lines 49 to 54, the method caches a single got client
on this.client and reuses it for all calls so a client created with one timeout
will incorrectly apply that timeout to subsequent calls; remove the persistent
client cache and instantiate a new got client per getGot call (or, if you prefer
caching, implement a timeout-aware cache keyed by timeout values) so that the
passed timeout parameter is respected for each request; also remove the client
field from the class (or change its type to a Map keyed by timeout) and update
any constructor references accordingly.

},

retry: {
Expand Down
17 changes: 11 additions & 6 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,8 +486,8 @@ export abstract class DetSysAction {
* Fetches the executable at the URL determined by the `source-*` inputs and
* other facts, `chmod`s it, and returns the path to the executable on disk.
*/
async fetchExecutable(): Promise<string> {
const binaryPath = await this.fetchArtifact();
async fetchExecutable(timeout?: number): Promise<string> {
const binaryPath = await this.fetchArtifact(timeout);
await chmod(binaryPath, fsConstants.S_IXUSR | fsConstants.S_IXGRP);
return binaryPath;
}
Expand Down Expand Up @@ -572,7 +572,7 @@ export abstract class DetSysAction {
}
}

async getClient(): Promise<Got> {
async getClient(timeout?: number): Promise<Got> {
return await this.idsHost.getGot(
(incitingError: unknown, prevUrl: URL, nextUrl: URL) => {
this.recordPlausibleTimeout(incitingError);
Expand All @@ -582,6 +582,7 @@ export abstract class DetSysAction {
nextUrl: nextUrl.toString(),
});
},
timeout,
);
}

Expand Down Expand Up @@ -733,7 +734,7 @@ export abstract class DetSysAction {
* URL determined by the other `source-*` inputs (`source-url`, `source-pr`,
* etc.).
*/
private async fetchArtifact(): Promise<string> {
private async fetchArtifact(timeout?: number): Promise<string> {
const sourceBinary = getStringOrNull("source-binary");

// If source-binary is set, use that. Otherwise fall back to the source-* parameters.
Expand All @@ -756,7 +757,9 @@ export abstract class DetSysAction {
JSON.stringify(this.identity),
);

const versionCheckup = await (await this.getClient()).head(correlatedUrl);
const versionCheckup = await (
await this.getClient(timeout)
).head(correlatedUrl);
if (versionCheckup.headers.etag) {
const v = versionCheckup.headers.etag;
this.addFact(FACT_SOURCE_URL_ETAG, v);
Expand All @@ -783,6 +786,7 @@ export abstract class DetSysAction {
const fetchStream = await this.downloadFile(
new URL(versionCheckup.url),
destFile,
timeout,
);

if (fetchStream.response?.headers.etag) {
Expand Down Expand Up @@ -817,8 +821,9 @@ export abstract class DetSysAction {
private async downloadFile(
url: URL,
destination: PathLike,
timeout?: number,
): Promise<Request> {
const client = await this.getClient();
const client = await this.getClient(timeout);

return new Promise((resolve, reject) => {
// Current stream handle
Expand Down