Add timeout to OpenAI Compatible Provider Client (#9898)

This commit is contained in:
Dennise Bartlett 2025-12-08 17:36:53 -08:00 committed by GitHub
parent de00ab10e2
commit 3356267aa0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 352 additions and 40 deletions

View file

@ -41,7 +41,7 @@ export const workspace = {
getWorkspaceFolder: () => null,
onDidChangeWorkspaceFolders: () => mockDisposable,
getConfiguration: () => ({
get: () => null,
get: (key, defaultValue) => defaultValue,
}),
createFileSystemWatcher: () => ({
onDidCreate: () => mockDisposable,

View file

@ -171,6 +171,44 @@ vi.mock("../i18n", () => ({
t: vi.fn((key) => key),
}))
// Mock ClineProvider - remoteControlEnabled must call BridgeOrchestrator.disconnect for the test
vi.mock("../core/webview/ClineProvider", async () => {
const { BridgeOrchestrator } = await import("@roo-code/cloud")
const mockInstance = {
resolveWebviewView: vi.fn(),
postMessageToWebview: vi.fn(),
postStateToWebview: vi.fn(),
getState: vi.fn().mockResolvedValue({}),
remoteControlEnabled: vi.fn().mockImplementation(async (enabled: boolean) => {
if (!enabled) {
await BridgeOrchestrator.disconnect()
}
}),
initializeCloudProfileSyncWhenReady: vi.fn().mockResolvedValue(undefined),
providerSettingsManager: {},
contextProxy: { getGlobalState: vi.fn() },
customModesManager: {},
upsertProviderProfile: vi.fn().mockResolvedValue(undefined),
}
return {
ClineProvider: Object.assign(
vi.fn().mockImplementation(() => mockInstance),
{
// Static method used by extension.ts
getVisibleInstance: vi.fn().mockReturnValue(mockInstance),
sideBarId: "roo-cline-sidebar",
},
),
}
})
// Mock modelCache to prevent network requests during module loading
vi.mock("../api/providers/fetchers/modelCache", () => ({
flushModels: vi.fn(),
getModels: vi.fn().mockResolvedValue([]),
initializeModelCacheRefresh: vi.fn(),
}))
describe("extension.ts", () => {
let mockContext: vscode.ExtensionContext
let authStateChangedHandler:

View file

@ -0,0 +1,119 @@
// npx vitest run api/providers/__tests__/base-openai-compatible-provider-timeout.spec.ts
import type { ModelInfo } from "@roo-code/types"
import { BaseOpenAiCompatibleProvider } from "../base-openai-compatible-provider"
// Mock the timeout config utility
vitest.mock("../utils/timeout-config", () => ({
getApiRequestTimeout: vitest.fn(),
}))
import { getApiRequestTimeout } from "../utils/timeout-config"
// Mock OpenAI and capture constructor calls
const mockOpenAIConstructor = vitest.fn()
vitest.mock("openai", () => {
return {
__esModule: true,
default: vitest.fn().mockImplementation((config) => {
mockOpenAIConstructor(config)
return {
chat: {
completions: {
create: vitest.fn(),
},
},
}
}),
}
})
// Create a concrete test implementation of the abstract base class
class TestOpenAiCompatibleProvider extends BaseOpenAiCompatibleProvider<"test-model"> {
constructor(apiKey: string) {
const testModels: Record<"test-model", ModelInfo> = {
"test-model": {
maxTokens: 4096,
contextWindow: 128000,
supportsImages: false,
supportsPromptCache: false,
inputPrice: 0.5,
outputPrice: 1.5,
},
}
super({
providerName: "TestProvider",
baseURL: "https://test.example.com/v1",
defaultProviderModelId: "test-model",
providerModels: testModels,
apiKey,
})
}
}
describe("BaseOpenAiCompatibleProvider Timeout Configuration", () => {
beforeEach(() => {
vitest.clearAllMocks()
})
it("should call getApiRequestTimeout when creating the provider", () => {
;(getApiRequestTimeout as any).mockReturnValue(600000)
new TestOpenAiCompatibleProvider("test-api-key")
expect(getApiRequestTimeout).toHaveBeenCalled()
})
it("should pass the default timeout to the OpenAI client constructor", () => {
;(getApiRequestTimeout as any).mockReturnValue(600000) // 600 seconds in ms
new TestOpenAiCompatibleProvider("test-api-key")
expect(mockOpenAIConstructor).toHaveBeenCalledWith(
expect.objectContaining({
baseURL: "https://test.example.com/v1",
apiKey: "test-api-key",
timeout: 600000,
}),
)
})
it("should use custom timeout value from getApiRequestTimeout", () => {
;(getApiRequestTimeout as any).mockReturnValue(1800000) // 30 minutes in ms
new TestOpenAiCompatibleProvider("test-api-key")
expect(mockOpenAIConstructor).toHaveBeenCalledWith(
expect.objectContaining({
timeout: 1800000,
}),
)
})
it("should handle zero timeout (no timeout)", () => {
;(getApiRequestTimeout as any).mockReturnValue(0)
new TestOpenAiCompatibleProvider("test-api-key")
expect(mockOpenAIConstructor).toHaveBeenCalledWith(
expect.objectContaining({
timeout: 0,
}),
)
})
it("should pass DEFAULT_HEADERS to the OpenAI client constructor", () => {
;(getApiRequestTimeout as any).mockReturnValue(600000)
new TestOpenAiCompatibleProvider("test-api-key")
expect(mockOpenAIConstructor).toHaveBeenCalledWith(
expect.objectContaining({
defaultHeaders: expect.any(Object),
}),
)
})
})

View file

@ -1,8 +1,5 @@
// npx vitest run src/api/providers/__tests__/groq.spec.ts
// Mock vscode first to avoid import errors
vitest.mock("vscode", () => ({}))
import OpenAI from "openai"
import { Anthropic } from "@anthropic-ai/sdk"

View file

@ -1,8 +1,5 @@
// npx vitest run src/api/providers/__tests__/sambanova.spec.ts
// Mock vscode first to avoid import errors
vitest.mock("vscode", () => ({}))
import OpenAI from "openai"
import { Anthropic } from "@anthropic-ai/sdk"

View file

@ -1,8 +1,5 @@
// npx vitest run src/api/providers/__tests__/zai.spec.ts
// Mock vscode first to avoid import errors
vitest.mock("vscode", () => ({}))
import OpenAI from "openai"
import { Anthropic } from "@anthropic-ai/sdk"

View file

@ -13,6 +13,7 @@ import { DEFAULT_HEADERS } from "./constants"
import { BaseProvider } from "./base-provider"
import { handleOpenAIError } from "./utils/openai-error-handler"
import { calculateApiCostOpenAI } from "../../shared/cost"
import { getApiRequestTimeout } from "./utils/timeout-config"
type BaseOpenAiCompatibleProviderOptions<ModelName extends string> = ApiHandlerOptions & {
providerName: string
@ -62,6 +63,7 @@ export abstract class BaseOpenAiCompatibleProvider<ModelName extends string>
baseURL,
apiKey: this.options.apiKey,
defaultHeaders: DEFAULT_HEADERS,
timeout: getApiRequestTimeout(),
})
}

View file

@ -1,7 +1,10 @@
import { describe, it, expect, vi, beforeEach, beforeAll } from "vitest"
import { describe, it, expect, vi, beforeEach } from "vitest"
import type { ClineProvider } from "../../webview/ClineProvider"
import type { ProviderSettings } from "@roo-code/types"
// All vi.mock() calls are hoisted to the top of the file by Vitest
// and are applied before any imports are resolved
// Mock vscode module before importing Task
vi.mock("vscode", () => ({
workspace: {
@ -72,16 +75,92 @@ vi.mock("@roo-code/telemetry", () => ({
},
}))
// Mock @roo-code/cloud to prevent socket.io-client initialization issues
vi.mock("@roo-code/cloud", () => ({
CloudService: {
isEnabled: () => false,
},
BridgeOrchestrator: {
subscribeToTask: vi.fn(),
},
}))
// Mock delay to prevent actual delays
vi.mock("delay", () => ({
__esModule: true,
default: vi.fn().mockResolvedValue(undefined),
}))
// Mock p-wait-for to prevent hanging on async conditions
vi.mock("p-wait-for", () => ({
default: vi.fn().mockResolvedValue(undefined),
}))
// Mock execa
vi.mock("execa", () => ({
execa: vi.fn(),
}))
// Mock fs/promises
vi.mock("fs/promises", () => ({
mkdir: vi.fn().mockResolvedValue(undefined),
writeFile: vi.fn().mockResolvedValue(undefined),
readFile: vi.fn().mockResolvedValue("[]"),
unlink: vi.fn().mockResolvedValue(undefined),
rmdir: vi.fn().mockResolvedValue(undefined),
}))
// Mock mentions
vi.mock("../../mentions", () => ({
parseMentions: vi.fn().mockImplementation((text) => Promise.resolve(text)),
openMention: vi.fn(),
getLatestTerminalOutput: vi.fn(),
}))
// Mock extract-text
vi.mock("../../../integrations/misc/extract-text", () => ({
extractTextFromFile: vi.fn().mockResolvedValue("Mock file content"),
}))
// Mock getEnvironmentDetails
vi.mock("../../environment/getEnvironmentDetails", () => ({
getEnvironmentDetails: vi.fn().mockResolvedValue(""),
}))
// Mock RooIgnoreController
vi.mock("../../ignore/RooIgnoreController")
// Mock condense
vi.mock("../../condense", () => ({
summarizeConversation: vi.fn().mockResolvedValue({
messages: [],
summary: "summary",
cost: 0,
newContextTokens: 1,
}),
}))
// Mock storage utilities
vi.mock("../../../utils/storage", () => ({
getTaskDirectoryPath: vi
.fn()
.mockImplementation((globalStoragePath, taskId) => Promise.resolve(`${globalStoragePath}/tasks/${taskId}`)),
getSettingsDirectoryPath: vi
.fn()
.mockImplementation((globalStoragePath) => Promise.resolve(`${globalStoragePath}/settings`)),
}))
// Mock fs utilities
vi.mock("../../../utils/fs", () => ({
fileExistsAtPath: vi.fn().mockReturnValue(false),
}))
// Import Task AFTER all vi.mock() calls - Vitest hoists mocks so this works
import { Task } from "../Task"
describe("Task grounding sources handling", () => {
let mockProvider: Partial<ClineProvider>
let mockApiConfiguration: ProviderSettings
let Task: any
beforeAll(async () => {
// Import Task after mocks are set up
const taskModule = await import("../Task")
Task = taskModule.Task
})
beforeEach(() => {
// Mock provider with necessary methods

View file

@ -1,7 +1,10 @@
import { describe, it, expect, vi, beforeEach, beforeAll } from "vitest"
import { describe, it, expect, vi, beforeEach } from "vitest"
import type { ClineProvider } from "../../webview/ClineProvider"
import type { ProviderSettings, ModelInfo } from "@roo-code/types"
// All vi.mock() calls are hoisted to the top of the file by Vitest
// and are applied before any imports are resolved
// Mock vscode module before importing Task
vi.mock("vscode", () => ({
workspace: {
@ -72,16 +75,92 @@ vi.mock("@roo-code/telemetry", () => ({
},
}))
// Mock @roo-code/cloud to prevent socket.io-client initialization issues
vi.mock("@roo-code/cloud", () => ({
CloudService: {
isEnabled: () => false,
},
BridgeOrchestrator: {
subscribeToTask: vi.fn(),
},
}))
// Mock delay to prevent actual delays
vi.mock("delay", () => ({
__esModule: true,
default: vi.fn().mockResolvedValue(undefined),
}))
// Mock p-wait-for to prevent hanging on async conditions
vi.mock("p-wait-for", () => ({
default: vi.fn().mockResolvedValue(undefined),
}))
// Mock execa
vi.mock("execa", () => ({
execa: vi.fn(),
}))
// Mock fs/promises
vi.mock("fs/promises", () => ({
mkdir: vi.fn().mockResolvedValue(undefined),
writeFile: vi.fn().mockResolvedValue(undefined),
readFile: vi.fn().mockResolvedValue("[]"),
unlink: vi.fn().mockResolvedValue(undefined),
rmdir: vi.fn().mockResolvedValue(undefined),
}))
// Mock mentions
vi.mock("../../mentions", () => ({
parseMentions: vi.fn().mockImplementation((text) => Promise.resolve(text)),
openMention: vi.fn(),
getLatestTerminalOutput: vi.fn(),
}))
// Mock extract-text
vi.mock("../../../integrations/misc/extract-text", () => ({
extractTextFromFile: vi.fn().mockResolvedValue("Mock file content"),
}))
// Mock getEnvironmentDetails
vi.mock("../../environment/getEnvironmentDetails", () => ({
getEnvironmentDetails: vi.fn().mockResolvedValue(""),
}))
// Mock RooIgnoreController
vi.mock("../../ignore/RooIgnoreController")
// Mock condense
vi.mock("../../condense", () => ({
summarizeConversation: vi.fn().mockResolvedValue({
messages: [],
summary: "summary",
cost: 0,
newContextTokens: 1,
}),
}))
// Mock storage utilities
vi.mock("../../../utils/storage", () => ({
getTaskDirectoryPath: vi
.fn()
.mockImplementation((globalStoragePath, taskId) => Promise.resolve(`${globalStoragePath}/tasks/${taskId}`)),
getSettingsDirectoryPath: vi
.fn()
.mockImplementation((globalStoragePath) => Promise.resolve(`${globalStoragePath}/settings`)),
}))
// Mock fs utilities
vi.mock("../../../utils/fs", () => ({
fileExistsAtPath: vi.fn().mockReturnValue(false),
}))
// Import Task AFTER all vi.mock() calls - Vitest hoists mocks so this works
import { Task } from "../Task"
describe("Task reasoning preservation", () => {
let mockProvider: Partial<ClineProvider>
let mockApiConfiguration: ProviderSettings
let Task: any
beforeAll(async () => {
// Import Task after mocks are set up
const taskModule = await import("../Task")
Task = taskModule.Task
})
beforeEach(() => {
// Mock provider with necessary methods
@ -127,7 +206,7 @@ describe("Task reasoning preservation", () => {
id: "test-model",
info: mockModelInfo,
}),
}
} as any
// Mock the API conversation history
task.apiConversationHistory = []
@ -163,10 +242,12 @@ describe("Task reasoning preservation", () => {
// Verify the API conversation history contains the message with reasoning
expect(task.apiConversationHistory).toHaveLength(1)
expect(task.apiConversationHistory[0].content[0].text).toContain("<think>")
expect(task.apiConversationHistory[0].content[0].text).toContain("</think>")
expect(task.apiConversationHistory[0].content[0].text).toContain("Here is my response to your question.")
expect(task.apiConversationHistory[0].content[0].text).toContain(
expect((task.apiConversationHistory[0].content[0] as { text: string }).text).toContain("<think>")
expect((task.apiConversationHistory[0].content[0] as { text: string }).text).toContain("</think>")
expect((task.apiConversationHistory[0].content[0] as { text: string }).text).toContain(
"Here is my response to your question.",
)
expect((task.apiConversationHistory[0].content[0] as { text: string }).text).toContain(
"Let me think about this step by step. First, I need to...",
)
})
@ -192,7 +273,7 @@ describe("Task reasoning preservation", () => {
id: "test-model",
info: mockModelInfo,
}),
}
} as any
// Mock the API conversation history
task.apiConversationHistory = []
@ -223,8 +304,10 @@ describe("Task reasoning preservation", () => {
// Verify the API conversation history does NOT contain reasoning
expect(task.apiConversationHistory).toHaveLength(1)
expect(task.apiConversationHistory[0].content[0].text).toBe("Here is my response to your question.")
expect(task.apiConversationHistory[0].content[0].text).not.toContain("<think>")
expect((task.apiConversationHistory[0].content[0] as { text: string }).text).toBe(
"Here is my response to your question.",
)
expect((task.apiConversationHistory[0].content[0] as { text: string }).text).not.toContain("<think>")
})
it("should handle empty reasoning message gracefully when preserveReasoning is true", async () => {
@ -248,7 +331,7 @@ describe("Task reasoning preservation", () => {
id: "test-model",
info: mockModelInfo,
}),
}
} as any
// Mock the API conversation history
task.apiConversationHistory = []
@ -277,8 +360,8 @@ describe("Task reasoning preservation", () => {
})
// Verify the message doesn't contain reasoning tags
expect(task.apiConversationHistory[0].content[0].text).toBe("Here is my response.")
expect(task.apiConversationHistory[0].content[0].text).not.toContain("<think>")
expect((task.apiConversationHistory[0].content[0] as { text: string }).text).toBe("Here is my response.")
expect((task.apiConversationHistory[0].content[0] as { text: string }).text).not.toContain("<think>")
})
it("should handle undefined preserveReasoning (defaults to false)", async () => {
@ -302,7 +385,7 @@ describe("Task reasoning preservation", () => {
id: "test-model",
info: mockModelInfo,
}),
}
} as any
// Mock the API conversation history
task.apiConversationHistory = []
@ -322,8 +405,8 @@ describe("Task reasoning preservation", () => {
})
// Verify reasoning was NOT prepended (undefined defaults to false)
expect(task.apiConversationHistory[0].content[0].text).toBe("Here is my response.")
expect(task.apiConversationHistory[0].content[0].text).not.toContain("<think>")
expect((task.apiConversationHistory[0].content[0] as { text: string }).text).toBe("Here is my response.")
expect((task.apiConversationHistory[0].content[0] as { text: string }).text).not.toContain("<think>")
})
it("should embed encrypted reasoning as first assistant content block", async () => {