fix: create logs dir before writing llm request log#8522
Open
J-Palomino wants to merge 1 commit intoaaif-goose:mainfrom
Open
fix: create logs dir before writing llm request log#8522J-Palomino wants to merge 1 commit intoaaif-goose:mainfrom
J-Palomino wants to merge 1 commit intoaaif-goose:mainfrom
Conversation
RequestLog::start opened <state>/logs/llm_request.<uuid>.jsonl with File::create, which creates the file but not its parent directory. On fresh installs (or when the logs dir was cleared), every provider streaming request failed with ENOENT, and the test_stream_ollama_timeout_on_stall test panicked. Call fs_err::create_dir_all on the logs dir at the top of RequestLog::start so callers no longer need to pre-create it. Signed-off-by: Juan Palomino <jpalomino09@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RequestLog::startopened<state>/logs/llm_request.<uuid>.jsonlwithFile::create, which creates the file but not its parent directory. On fresh installs (or when thelogs/directory was cleared), every provider streaming request failed withENOENT, and theproviders::ollama::tests::test_stream_ollama_timeout_on_stalltest panicked at theRequestLog::start(...).unwrap()call.This change calls
fs_err::create_dir_allon the logs directory at the top ofRequestLog::startso the rename infinish()and the file write here both have a guaranteed parent. The existing workaround incrates/goose-acp/tests/fixtures/mod.rs(which pre-creates the dir before tests run) is no longer load-bearing.Testing
providers::utils::tests::test_request_log_start_creates_logs_dir— pointsGOOSE_PATH_ROOTat a fresh tempdir, assertslogs/does not exist, callsRequestLog::start, asserts it now exists. Uses the sameenv_lockpattern asproviders::init::tests.cargo test -p goose --lib providers::utils::tests::test_request_log_start_creates_logs_dir -- --exact— passes.cargo fmt --check -p goose— clean.cargo clippy -p goose --lib --tests -- -D warnings— clean.Related Issues
Fixes #8485