Skip to content

Commit d6d90b5

Browse files
committed
Address cliffhall's review: update docs and add combined timeout test
- Update README example to show --max-total-timeout used with --request-timeout and --reset-timeout-on-progress - Add Test 32: combined_timeout_options that demonstrates all three flags working together - Update test numbers (34-36) to account for new test - Add explanatory comment about progress notification limitation in CLI mode - Fix Windows npx spawn issue by adding shell: true - Adjust test timeouts to work within 10-second test timeout limit - Document that progress-based timeout resets don't work in CLI mode yet
1 parent c509065 commit d6d90b5

File tree

2 files changed

+37
-8
lines changed

2 files changed

+37
-8
lines changed

README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -419,8 +419,8 @@ npx modelcontextprotocol/inspector --cli node build/index.js --method tools/call
419419
# With request timeout configs (in milliseconds) - reset timeout on progress
420420
npx modelcontextprotocol/inspector --cli node build/index.js --method tools/call --tool-name longRunningOperation --tool-arg duration=15 steps=5 --reset-timeout-on-progress true
421421

422-
# With request timeout configs (in milliseconds) - max total timeout
423-
npx modelcontextprotocol/inspector --cli node build/index.js --method tools/call --tool-name longRunningOperation --tool-arg duration=15 steps=5 --max-total-timeout 30000
422+
# With request timeout configs (in milliseconds) - max total timeout (used with request timeout and reset timeout on progress)
423+
npx modelcontextprotocol/inspector --cli node build/index.js --method tools/call --tool-name longRunningOperation --tool-arg duration=35 steps=5 --reset-timeout-on-progress true --request-timeout 7000 --max-total-timeout 35000
424424

425425
# List available tools
426426
npx @modelcontextprotocol/inspector --cli node build/index.js --method tools/list

cli/scripts/cli-tests.js

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1075,6 +1075,7 @@ async function runTests() {
10751075
{
10761076
detached: true,
10771077
stdio: "ignore",
1078+
shell: true,
10781079
},
10791080
);
10801081
runningServers.push(httpServer);
@@ -1222,11 +1223,39 @@ async function runTests() {
12221223
"100",
12231224
);
12241225

1226+
// Test 32: Combined timeout options for long-running operations
1227+
// This tests request-timeout + reset-timeout-on-progress + max-total-timeout working together
1228+
// Note: We cannot fully validate progress-based timeout resets in CLI mode since progress
1229+
// notifications are not yet captured/handled, so the timeout won't actually reset on progress.
1230+
// We use a request-timeout long enough for the operation to complete without relying on resets.
1231+
await runBasicTest(
1232+
"combined_timeout_options",
1233+
null,
1234+
TEST_CMD,
1235+
...TEST_ARGS,
1236+
"--cli",
1237+
"--method",
1238+
"tools/call",
1239+
"--tool-name",
1240+
"longRunningOperation",
1241+
"--tool-arg",
1242+
"duration=2",
1243+
"steps=2",
1244+
"--request-timeout",
1245+
"5000",
1246+
"--reset-timeout-on-progress",
1247+
"true",
1248+
"--max-total-timeout",
1249+
"10000",
1250+
);
1251+
12251252
// console.log(
12261253
// `\n${colors.YELLOW}=== Running Progress-Related Timeout Tests ===${colors.NC}`,
12271254
// );
12281255

1229-
// // Test 32: Reset timeout on progress disabled - should fail with timeout
1256+
// // Test 33: Reset timeout on progress disabled - should fail with timeout
1257+
// // This test is commented out because we cannot yet capture progress notifications
1258+
// // in CLI mode to validate that timeouts are NOT being reset on progress
12301259
// await runErrorTest(
12311260
// "reset_timeout_disabled",
12321261
// "should_timeout_quickly",
@@ -1238,12 +1267,12 @@ async function runTests() {
12381267
// "--tool-name",
12391268
// "longRunningOperation",
12401269
// "--tool-arg",
1241-
// "duration=15", // Same configuration as above
1270+
// "duration=15",
12421271
// "steps=5",
12431272
// "--request-timeout",
12441273
// "2000",
12451274
// "--reset-timeout-on-progress",
1246-
// "false", // Only difference is here
1275+
// "false",
12471276
// "--max-total-timeout",
12481277
// "30000",
12491278
// );
@@ -1252,7 +1281,7 @@ async function runTests() {
12521281
`\n${colors.YELLOW}=== Running Input Validation Tests ===${colors.NC}`,
12531282
);
12541283

1255-
// Test 33: Invalid request timeout value
1284+
// Test 34: Invalid request timeout value
12561285
await runErrorTest(
12571286
"invalid_request_timeout",
12581287
null,
@@ -1265,7 +1294,7 @@ async function runTests() {
12651294
"invalid",
12661295
);
12671296

1268-
// Test 34: Invalid reset-timeout-on-progress value
1297+
// Test 35: Invalid reset-timeout-on-progress value
12691298
await runErrorTest(
12701299
"invalid_reset_timeout",
12711300
null,
@@ -1278,7 +1307,7 @@ async function runTests() {
12781307
"not-a-boolean",
12791308
);
12801309

1281-
// Test 35: Invalid max total timeout value
1310+
// Test 36: Invalid max total timeout value
12821311
await runErrorTest(
12831312
"invalid_max_timeout",
12841313
null,

0 commit comments

Comments
 (0)