mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-10-22 01:53:33 +00:00
🧭 fix: Delete Code Environment Files Through the Route That Exists
`deleteCodeEnvFile` tried `/sessions/:sid/objects/:fid` before falling back to `/files/:sid/:fid`. Both have been there since #13424, but only the second is mounted by any released codeapi — the first gained DELETE in LibreChat-AI/code-interpreter#85 — so every deletion paid a guaranteed 404 and a wasted round trip, and #15511 read that 404 as the whole bug. Call `/files/:sid/:fid` directly. It is the safe direction to collapse toward: codeapi has mounted it since its first release, so this works against older deployments as well as post-#85 ones, whereas keeping the other path would not. Collapsing the loop tightens two behaviours that only existed to serve it. A 405 now surfaces instead of being swallowed on the way to a second attempt; there is no second route to try, and a service that refuses the method should say so. A 404 is still treated as "already gone" — that is the only thing it can now mean — but it is logged rather than passed over in silence, because a 404 caused by a misconfigured base URL looks identical and this branch drops the file's metadata record either way.
This commit is contained in:
Danny Avila
parent
bd60b7ead1
commit
0e529249f6
2 changed files with 56 additions and 74 deletions
|
|
@ -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 =
|
||||
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
}
|
||||
if (!deleted && lastError) {
|
||||
logAxiosError({
|
||||
error: lastError,
|
||||
message: `Error deleting code environment file: ${lastError.message}`,
|
||||
/* 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,
|
||||
});
|
||||
if (lastError.response?.status === 404) {
|
||||
continue;
|
||||
} catch (error) {
|
||||
if (error.response?.status !== 404) {
|
||||
throw error;
|
||||
}
|
||||
throw new Error(lastError.message || 'An error occurred during file deletion.');
|
||||
/* 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,
|
||||
message: `Code environment object already absent: ${error.message}`,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
|
|
@ -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',
|
||||
}),
|
||||
|
|
@ -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' }),
|
||||
}),
|
||||
);
|
||||
|
|
@ -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 });
|
||||
it('never calls the file-server path that only newer codeapi mounts', async () => {
|
||||
mockAxios.mockResolvedValue({ status: 204 });
|
||||
|
||||
await deleteCodeEnvFile(req, file);
|
||||
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',
|
||||
}),
|
||||
);
|
||||
},
|
||||
);
|
||||
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, {});
|
||||
|
|
@ -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 () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue