fix: address latest coderabbit review findings
This commit is contained in:
@@ -475,6 +475,9 @@ export async function handleChatSurfaceRequest(
|
||||
recordDownstreamCost: (estimatedCost) => {
|
||||
recordDownstreamCostUsage(request, estimatedCost);
|
||||
},
|
||||
bestEffortMetrics: {
|
||||
errorLabel: '[proxy/chat] failed to record success metrics',
|
||||
},
|
||||
});
|
||||
return;
|
||||
}
|
||||
@@ -541,6 +544,9 @@ export async function handleChatSurfaceRequest(
|
||||
recordDownstreamCost: (estimatedCost) => {
|
||||
recordDownstreamCostUsage(request, estimatedCost);
|
||||
},
|
||||
bestEffortMetrics: {
|
||||
errorLabel: '[proxy/chat] failed to record success metrics',
|
||||
},
|
||||
});
|
||||
|
||||
return reply.send(downstreamResponse);
|
||||
|
||||
@@ -21,6 +21,7 @@ const resolveProxyUsageWithSelfLogFallbackMock = vi.fn();
|
||||
const resolveProxyLogBillingMock = vi.fn();
|
||||
const refreshOauthAccessTokenSingleflightMock = vi.fn();
|
||||
const consoleWarnMock = vi.spyOn(console, 'warn').mockImplementation(() => {});
|
||||
const consoleErrorMock = vi.spyOn(console, 'error').mockImplementation(() => {});
|
||||
|
||||
vi.mock('../../services/tokenRouter.js', () => ({
|
||||
tokenRouter: {
|
||||
@@ -88,6 +89,7 @@ vi.mock('../../services/oauth/refreshSingleflight.js', () => ({
|
||||
describe('selectSurfaceChannelForAttempt', () => {
|
||||
afterAll(() => {
|
||||
consoleWarnMock.mockRestore();
|
||||
consoleErrorMock.mockRestore();
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
@@ -111,6 +113,7 @@ describe('selectSurfaceChannelForAttempt', () => {
|
||||
resolveProxyLogBillingMock.mockReset();
|
||||
refreshOauthAccessTokenSingleflightMock.mockReset();
|
||||
consoleWarnMock.mockClear();
|
||||
consoleErrorMock.mockClear();
|
||||
});
|
||||
|
||||
it('refreshes models and retries selectChannel on the first attempt when no channel is available', async () => {
|
||||
@@ -155,6 +158,29 @@ describe('selectSurfaceChannelForAttempt', () => {
|
||||
expect(refreshModelsAndRebuildRoutesMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('logs refresh failures and still retries selection once on the first attempt', async () => {
|
||||
const selected = { channel: { id: 33 } };
|
||||
selectChannelMock
|
||||
.mockResolvedValueOnce(null)
|
||||
.mockResolvedValueOnce(selected);
|
||||
refreshModelsAndRebuildRoutesMock.mockRejectedValueOnce(new Error('refresh failed'));
|
||||
|
||||
const { selectSurfaceChannelForAttempt } = await import('./sharedSurface.js');
|
||||
const result = await selectSurfaceChannelForAttempt({
|
||||
requestedModel: 'gpt-5.2',
|
||||
downstreamPolicy: EMPTY_DOWNSTREAM_ROUTING_POLICY,
|
||||
excludeChannelIds: [],
|
||||
retryCount: 0,
|
||||
});
|
||||
|
||||
expect(result).toBe(selected);
|
||||
expect(selectChannelMock).toHaveBeenCalledTimes(2);
|
||||
expect(consoleWarnMock).toHaveBeenCalledWith(
|
||||
'[proxy/surface] failed to refresh routes after empty selection',
|
||||
expect.any(Error),
|
||||
);
|
||||
});
|
||||
|
||||
it('writes proxy logs through the shared log formatter and store', async () => {
|
||||
composeProxyLogMessageMock.mockReturnValue('normalized error');
|
||||
formatUtcSqlDateTimeMock.mockReturnValue('2026-03-21 22:00:00');
|
||||
@@ -824,4 +850,64 @@ describe('selectSurfaceChannelForAttempt', () => {
|
||||
billingDetails: { source: 'pricing-test' },
|
||||
});
|
||||
});
|
||||
|
||||
it('treats success metrics as best-effort when requested', async () => {
|
||||
resolveProxyUsageWithSelfLogFallbackMock.mockRejectedValueOnce(new Error('billing failed'));
|
||||
const logSuccess = vi.fn().mockResolvedValue(undefined);
|
||||
const recordDownstreamCost = vi.fn();
|
||||
|
||||
const { recordSurfaceSuccess } = await import('./sharedSurface.js');
|
||||
const result = await recordSurfaceSuccess({
|
||||
selected: {
|
||||
channel: { id: 11, routeId: 22 },
|
||||
account: { id: 33, username: 'oauth-user' },
|
||||
site: { id: 44, url: 'https://upstream.example.com', name: 'Codex OAuth' },
|
||||
tokenValue: 'live-token',
|
||||
tokenName: 'default',
|
||||
actualModel: 'upstream-model',
|
||||
},
|
||||
requestedModel: 'gpt-5.2',
|
||||
modelName: 'upstream-model',
|
||||
parsedUsage: {
|
||||
promptTokens: 10,
|
||||
completionTokens: 5,
|
||||
totalTokens: 15,
|
||||
},
|
||||
requestStartedAtMs: 1000,
|
||||
latencyMs: 250,
|
||||
retryCount: 1,
|
||||
upstreamPath: '/v1/responses',
|
||||
logSuccess,
|
||||
recordDownstreamCost,
|
||||
bestEffortMetrics: {
|
||||
errorLabel: '[proxy/chat] failed to record success metrics',
|
||||
},
|
||||
});
|
||||
|
||||
expect(consoleErrorMock).toHaveBeenCalledWith(
|
||||
'[proxy/chat] failed to record success metrics',
|
||||
expect.any(Error),
|
||||
);
|
||||
expect(recordSuccessMock).toHaveBeenCalledWith(11, 250, 0, 'upstream-model');
|
||||
expect(recordDownstreamCost).toHaveBeenCalledWith(0);
|
||||
expect(logSuccess).toHaveBeenCalledWith(expect.objectContaining({
|
||||
promptTokens: 10,
|
||||
completionTokens: 5,
|
||||
totalTokens: 15,
|
||||
estimatedCost: 0,
|
||||
billingDetails: null,
|
||||
}));
|
||||
expect(result).toEqual({
|
||||
resolvedUsage: {
|
||||
promptTokens: 10,
|
||||
completionTokens: 5,
|
||||
totalTokens: 15,
|
||||
recoveredFromSelfLog: false,
|
||||
estimatedCostFromQuota: 0,
|
||||
selfLogBillingMeta: null,
|
||||
},
|
||||
estimatedCost: 0,
|
||||
billingDetails: null,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -113,7 +113,11 @@ export async function selectSurfaceChannelForAttempt(input: {
|
||||
);
|
||||
|
||||
if (!selected && input.retryCount === 0) {
|
||||
await routeRefreshWorkflow.refreshModelsAndRebuildRoutes();
|
||||
try {
|
||||
await routeRefreshWorkflow.refreshModelsAndRebuildRoutes();
|
||||
} catch (error) {
|
||||
console.warn('[proxy/surface] failed to refresh routes after empty selection', error);
|
||||
}
|
||||
selected = await tokenRouter.selectChannel(input.requestedModel, input.downstreamPolicy);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user