Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
68 changes: 28 additions & 40 deletions api/server/services/Files/Code/crud.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ async function deleteCodeEnvFile(req, file) {
return;
}

const missingOrUnsupportedStatuses = new Set([404, 405]);
for (const [executionRouteKey, ref] of refs) {
const executionProfile = ref.executionProfile ?? 'default';
const environments =
Expand Down Expand Up @@ -127,48 +126,37 @@ async function deleteCodeEnvFile(req, file) {
const bridgeWorkerId =
configuredEnvironment?.workerId ?? configuredEnvironment?.pairing?.workerId;
const authHeaders = await getCodeApiAuthHeaders(req, bridgeWorkerId);
const baseRequest = {
method: 'delete',
headers: {
'User-Agent': 'LibreChat/1.0',
...authHeaders,
...codeExecutionHeaders({
executionProfile,
bridgeWorkerId,
}),
},
httpAgent: codeServerHttpAgent,
httpsAgent: codeServerHttpsAgent,
timeout: 15000,
};
const urls = [
`${baseURL}/sessions/${ref.storage_session_id}/objects/${ref.file_id}${query}`,
`${baseURL}/files/${ref.storage_session_id}/${ref.file_id}${query}`,
];

let lastError;
let deleted = false;
for (const url of urls) {
try {
await axios({ ...baseRequest, url });
deleted = true;
break;
} catch (error) {
lastError = error;
if (!missingOrUnsupportedStatuses.has(error.response?.status)) {
throw error;
}
/* codeapi has mounted DELETE at `/files/:session_id/:fileId` since its
* first release. The file-server's own `/sessions/:id/objects/:fileId`
* only gained DELETE in LibreChat-AI/code-interpreter#85, so trying it
* first cost a guaranteed 404 against every older deployment. */
try {
await axios({
method: 'delete',
url: `${baseURL}/files/${ref.storage_session_id}/${ref.file_id}${query}`,
headers: {
'User-Agent': 'LibreChat/1.0',
...authHeaders,
...codeExecutionHeaders({
executionProfile,
bridgeWorkerId,
}),
},
httpAgent: codeServerHttpAgent,
httpsAgent: codeServerHttpsAgent,
timeout: 15000,
});
} catch (error) {
if (error.response?.status !== 404) {
throw error;
}
}
if (!deleted && lastError) {
/* Already gone. Logged rather than swallowed: a 404 from a
* misconfigured base URL is indistinguishable from one for an absent
* object, and this branch drops the file's record either way. */
logAxiosError({
error: lastError,
message: `Error deleting code environment file: ${lastError.message}`,
error,
message: `Code environment object already absent: ${error.message}`,
});
if (lastError.response?.status === 404) {
continue;
}
throw new Error(lastError.message || 'An error occurred during file deletion.');
}
}
}
Expand Down
64 changes: 29 additions & 35 deletions api/server/services/Files/Code/crud.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ describe('Code CRUD', () => {
expect(mockAxios).toHaveBeenCalledWith(
expect.objectContaining({
method: 'delete',
url: 'https://code-api.example.com/sessions/session-1/objects/file-1?kind=agent&id=agent-abc',
url: 'https://code-api.example.com/files/session-1/file-1?kind=agent&id=agent-abc',
headers: expect.objectContaining({
Authorization: 'Bearer codeapi-token',
'User-Agent': 'LibreChat/1.0',
Expand All @@ -239,7 +239,7 @@ describe('Code CRUD', () => {

expect(mockAxios).toHaveBeenCalledWith(
expect.objectContaining({
url: 'https://code-stateful.example.com/sessions/session-1/objects/file-1?kind=agent&id=agent-abc',
url: 'https://code-stateful.example.com/files/session-1/file-1?kind=agent&id=agent-abc',
headers: expect.objectContaining({
'X-CodeAPI-Expected-Profile': 'stateful',
}),
Expand Down Expand Up @@ -270,14 +270,14 @@ describe('Code CRUD', () => {
expect(mockAxios).toHaveBeenNthCalledWith(
1,
expect.objectContaining({
url: expect.stringContaining('/sessions/session-1/objects/file-1'),
url: expect.stringContaining('/files/session-1/file-1'),
headers: expect.objectContaining({ 'X-CodeAPI-Expected-Profile': 'default' }),
}),
);
expect(mockAxios).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
url: expect.stringContaining('/sessions/stateful-session/objects/stateful-file'),
url: expect.stringContaining('/files/stateful-session/stateful-file'),
headers: expect.objectContaining({ 'X-CodeAPI-Expected-Profile': 'stateful' }),
}),
);
Expand Down Expand Up @@ -320,33 +320,27 @@ describe('Code CRUD', () => {
expect(mockAxios).not.toHaveBeenCalled();
});

it.each([404, 405])(
'falls back to the legacy code environment delete route after a %s',
async (status) => {
mockAxios
.mockRejectedValueOnce(
Object.assign(new Error('missing route'), { response: { status } }),
)
.mockResolvedValueOnce({ status: 204 });

await deleteCodeEnvFile(req, file);

expect(mockAxios).toHaveBeenNthCalledWith(
1,
expect.objectContaining({
method: 'delete',
url: 'https://code-api.example.com/sessions/session-1/objects/file-1?kind=agent&id=agent-abc',
}),
);
expect(mockAxios).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
method: 'delete',
url: 'https://code-api.example.com/files/session-1/file-1?kind=agent&id=agent-abc',
}),
);
},
);
it('never calls the file-server path that only newer codeapi mounts', async () => {
mockAxios.mockResolvedValue({ status: 204 });

await deleteCodeEnvFile(req, file);

expect(mockAxios).toHaveBeenCalledTimes(1);
expect(mockAxios).not.toHaveBeenCalledWith(
expect.objectContaining({
url: expect.stringContaining('/sessions/session-1/objects/file-1'),
}),
);
});

it('surfaces an unsupported delete method instead of retrying elsewhere', async () => {
mockAxios.mockRejectedValue(
Object.assign(new Error('method not allowed'), { response: { status: 405 } }),
);

await expect(deleteCodeEnvFile(req, file)).rejects.toThrow('method not allowed');
expect(mockAxios).toHaveBeenCalledTimes(1);
});

it('skips files without a code environment ref', async () => {
await deleteCodeEnvFile(req, {});
Expand All @@ -356,12 +350,12 @@ describe('Code CRUD', () => {
});

it('treats missing code environment objects as already deleted', async () => {
mockAxios
.mockRejectedValueOnce(Object.assign(new Error('missing'), { response: { status: 404 } }))
.mockRejectedValueOnce(Object.assign(new Error('missing'), { response: { status: 404 } }));
mockAxios.mockRejectedValue(
Object.assign(new Error('missing'), { response: { status: 404 } }),
);

await expect(deleteCodeEnvFile(req, file)).resolves.toBeUndefined();
expect(mockAxios).toHaveBeenCalledTimes(2);
expect(mockAxios).toHaveBeenCalledTimes(1);
});

it('throws when code environment deletion fails', async () => {
Expand Down
2 changes: 2 additions & 0 deletions api/server/services/Files/process.js
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,8 @@ async function sweepExpiredFiles(options = {}) {
return sweepExpiredFilesWithDeps(options, {
getExpiredFiles: db.getExpiredFiles,
processDeleteRequest,
incrementFileDeletionAttempts: db.incrementFileDeletionAttempts,
deferExpiredFile: db.deferExpiredFile,
logger,
});
}
Expand Down
4 changes: 4 additions & 0 deletions api/server/services/Files/process.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ jest.mock('~/models', () => ({
findFileById: jest.fn(),
getConvo: jest.fn(),
getExpiredFiles: jest.fn(),
incrementFileDeletionAttempts: jest.fn(),
deferExpiredFile: jest.fn(),
addAgentResourceFile: jest.fn().mockResolvedValue({}),
removeAgentResourceFiles: jest.fn(),
removeAgentResourceFilesFromAllAgents: jest.fn(),
Expand Down Expand Up @@ -1840,6 +1842,8 @@ describe('sweepExpiredFiles', () => {
expect.objectContaining({
getExpiredFiles: db.getExpiredFiles,
processDeleteRequest: expect.any(Function),
incrementFileDeletionAttempts: db.incrementFileDeletionAttempts,
deferExpiredFile: db.deferExpiredFile,
logger: expect.objectContaining({
error: expect.any(Function),
info: expect.any(Function),
Expand Down
Loading
Loading