Skip to content

add timeout to env for sampling messages #8

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

Merged
Royal-lobster merged 1 commit into from
Dec 11, 2025
Merged

add timeout to env for sampling messages #8

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
5 changes: 5 additions & 0 deletions .changeset/happy-suits-say.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@iqai/mcp-discord": patch
---

Adds timeout setting in env for sampling messages
8 changes: 8 additions & 0 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,14 @@ const envSchema = z.object({
.optional()
.default(true)
.describe('Only respond to messages that mention the bot'),
SAMPLING_DEFAULT_TIMEOUT: z
.number()
.optional()
.describe('A timeout (in milliseconds) for this request'),
SAMPLING_REACTION_TIMEOUT: z
.number()
.optional()
.describe('A timeout for sending reactions to discord'),
Comment on lines +35 to +42

@gemini-code-assist gemini-code-assist Bot Dec 11, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

These new timeout configurations could be improved for robustness and clarity.

  1. Missing Defaults: SAMPLING_DEFAULT_TIMEOUT and SAMPLING_REACTION_TIMEOUT are optional but lack default values. If not set, they will be undefined, which could lead to requests hanging if the underlying SDK has no default timeout. This is particularly risky for SAMPLING_DEFAULT_TIMEOUT as there's no other timeout mechanism for the main message sampling.
  2. Misleading Description: The description for SAMPLING_REACTION_TIMEOUT is inaccurate. It's a timeout for the sampling request, not for sending the reaction to Discord.

I suggest adding default values and correcting the description to address these points.

  SAMPLING_DEFAULT_TIMEOUT: z
    .number()
    .optional()
    .default(30000) // 30 seconds
    .describe('A timeout (in milliseconds) for this request'),
  SAMPLING_REACTION_TIMEOUT: z
    .number()
    .optional()
    .default(3000) // Align with REACTION_TIMEOUT_MS
    .describe('A timeout (in milliseconds) for the reaction sampling request'),

Comment on lines +35 to +42

@gemini-code-assist gemini-code-assist Bot Dec 11, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For consistency with other configuration variables in this file (like DEFAULT_RATE_LIMIT_SECONDS which has a --rate-limit flag), consider adding command-line argument support for SAMPLING_DEFAULT_TIMEOUT and SAMPLING_REACTION_TIMEOUT. This would allow overriding them via CLI flags (e.g., --sampling-default-timeout and --sampling-reaction-timeout) in addition to environment variables. This change would need to be implemented in the getConfigFromArgs function.

BLOCK_DMS: z
.stringbool()
.optional()
Expand Down
8 changes: 6 additions & 2 deletions src/sampling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,10 @@ export class SamplingHandler {
maxTokens: 10,
},
},
z.any()
z.any(),
{
timeout: env.SAMPLING_REACTION_TIMEOUT,
}
);
Comment on lines +190 to 194

@gemini-code-assist gemini-code-assist Bot Dec 11, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

With the introduction of the timeout option in server.request using SAMPLING_REACTION_TIMEOUT, the existing client-side timeout using Promise.race and REACTION_TIMEOUT_MS in addDynamicReaction appears to be redundant. Having two timeout mechanisms for the same operation can be confusing and hard to maintain.

Consider refactoring to use only the SAMPLING_REACTION_TIMEOUT. This would simplify the code in addDynamicReaction by removing the Promise.race, and would also allow for removing the REACTION_TIMEOUT_MS configuration variable.


const response =
Expand Down Expand Up @@ -225,7 +228,8 @@ export class SamplingHandler {
maxTokens: 200,
},
},
z.any()
z.any(),
{ timeout: env.SAMPLING_DEFAULT_TIMEOUT }
);
}

Expand Down