feat(mongodb-log-writer): add support for recursively cleaning up old log files MCP-103 #565
Conversation
| try { | ||
| dirHandle = await fs.opendir(dir); | ||
| dirHandle = await fs.opendir(dir, { recursive }); |
There was a problem hiding this comment.
I'm not sure we are aware that this can remove folders that we might not own. With the current logic, if someone creates a folder for retention (f.e, a daily folder in the same folder and copies all the logs) the logging system won't delete them, but with this changes, they will.
I'm concerned this might inadvertently delete files that we shouldn't.
I think it's fine to keep recursive as an optional value that defaults to the current behaviour, but I would add a comment in the function mentioning this change of behaviour and dangers.
There was a problem hiding this comment.
Yeah, that's also one of my concerns. I'll try and explore a different approach here - might be a better way to handle things, so will put that back in draft.
There was a problem hiding this comment.
Yeah, that's also one of my concerns. I'll try and explore a different approach here - might be a better way to handle things, so will put that back in draft.
| } | ||
| } | ||
|
|
||
| let basePath = path.dirname(filePath); | ||
| while (basePath !== this._options.directory) { |
There was a problem hiding this comment.
What if this._options.directory was specified as /foo/bar/? Repeatedly calling path.dirname() on /foo/bar/baz will never reach that filename.
I'd imagine that path.normalize() together with some sort of prefix check would work here, but I'm not sure if that's the most reliable way to do this either
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| } catch (err: any) { | ||
| // If the directory is not empty, we stop deleting. | ||
| if (err?.code === 'ENOTEMPTY' || err?.code === 'ENOENT') { |
There was a problem hiding this comment.
These would be the most common errors that rmdir() could encounter, but I don't think it makes sense to continue in any error situation here, right?
|
|
||
| if (!dirent.isFile()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This leaves us in a situation where, if removing a directory fails for any reason, we're never going to clean that directory up. I think that may be something we just need to accept, unless we want to start e.g. imposing naming restrictions on the nested subdirectories – that's something we should probably decide on now, though.
addaleax
changed the title
|
Moving that back into draft - I feel I may have jumped the gun on that approach and I'm not super happy with some of the implications from this change. I'll think a bit more about it and see if I can come up with something better. |
|
Closing this as I've ultimately decided to pursue the other approach, where we log from all sessions in the same log file, but we'll be attributing log messages from different sessions with their ids. |
Description
This allows the log manager to optionally consider log files inside child directories. This is related to MCP-103 where we will be creating child log managers writing files inside subdirectories for specific sessions, while the parent log manager will be responsible for cleaning up all logs.
Open Questions
Checklist