Repository navigation
fix: keep thread-analysis log dirs available to resumed runs - #700
Merged
Merged
Conversation
) A resumed thread analysis failed before launching the provider when agent-logs/thread-analysis/ no longer existed. The pipeline creates that directory early, while it is still empty. LaunchProcess then opened the log with a bare os.Create and treated the missing parent as fatal. Create the parent directory where the log is opened, so every subprocess task (fresh or resumed) works no matter what happened to the directory after the run set it up.
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: d3d22eba7833
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (broad); scope: 2 files; examples:internal/llm/subprocess.go,internal/llmadapters/subprocess_test.go; body-inspected: 2 files; examples:internal/llm/subprocess.go,internal/llmadapters/subprocess_test.go; skipped: none; constraints: none
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 24s | gpt-6.1-terra | cr 0.10.357
| Field | Value |
|---|---|
| Model | gpt-6.1-terra |
| Reviewers | go:implementation-tests |
| Engine | codex_cli · gpt-6.1-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 24s wall · 15s compute |
| Cost | unavailable |
| Tokens | 128.7k in / 546 out |
Per-workstream usage
orchestrator-selection— gpt-6.1-terra- In: 18.1k
- Out: 110
- Cache read: 15.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 5s
go:implementation-tests— gpt-6.1-terra- In: 73.5k
- Out: 282
- Cache read: 53.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 7s
orchestrator-rollup— gpt-6.1-terra- In: 37.1k
- Out: 154
- Cache read: 33.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 3s
…#699) removeEmptyParents walked the whole runs root and removed every empty directory, including empty subdirs inside a live run's artifact tree. The pipeline creates agent-logs/thread-analysis before writing to it, so a prune could delete it out from under an in-flight run. Stop the sweep at attempt dirs. It now removes only empty layout parents left behind by deleted runs; unreferenced attempt dirs are still handled by orphan removal.
rianjs
changed the title
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 93f66ff33628
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (constrained); scope: 4 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/datalifecycle_test.go,internal/llm/subprocess.go,internal/llmadapters/subprocess_test.go; body-inspected: 4 files; examples:internal/datalifecycle/datalifecycle.go,internal/datalifecycle/datalifecycle_test.go,internal/llm/subprocess.go,internal/llmadapters/subprocess_test.go; skipped: none; constraints: Review was limited to static inspection; tests were not run in the read-only environment.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 24s | gpt-6.1-terra | cr 0.10.357
| Field | Value |
|---|---|
| Model | gpt-6.1-terra |
| Reviewers | go:implementation-tests |
| Engine | codex_cli · gpt-6.1-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 24s wall · 14s compute |
| Cost | unavailable |
| Tokens | 261.3k in / 889 out |
Per-workstream usage
go:implementation-tests— gpt-6.1-terra- In: 203.7k
- Out: 686
- Cache read: 167.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 11s
orchestrator-rollup— gpt-6.1-terra- In: 57.6k
- Out: 203
- Cache read: 51.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 3s
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.
Closes #699
Problem
A
cr reviewrerun on #698 failed inthread_analysisbefore the provider started (resume_llm, 0 ms). It couldn't openagent-logs/thread-analysis/<thread>.jsonl:no such file or directory.The pipeline does create that directory (
prepareSelectionContext), but only once, early in the run, while it is still empty.llm.LaunchProcessthen opened the log with a bareos.Create, so a missing parent directory was fatal.The run's artifact timestamps show the directory was removed between being created and the thread analysis running. In the failing run,
agent-logs/anddossier/were created at 15:54:4x–50, and the thread-analysis task started at 15:55:03. Thethread-analysis/log dir on disk now was created later, at 15:55:48, by the--fresh-sessionrerun. The only code that deletes directories under the runs root is retention prune'sremoveEmptyParents, which removes every empty directory. Anothercr reviewstarted on a different PR at the same time (15:55:09). I couldn't prove a prune got past the active-runs lock, so that part is a likely explanation, not a confirmed one.Fix
Two changes, one per side of the failure:
openSubprocessLognow runsMkdirAllon the log's parent (0700, the same mode the pipeline uses) before it creates the file. Every subprocess task, fresh or resumed, then works no matter what happened to the directory after setup.removeEmptyParentsused to walk the whole runs root and remove every empty directory, including empty subdirs inside a live run's artifact tree. It now stops at attempt (run-*) dirs, so it removes only empty layout parents left behind by deleted runs. Unreferenced attempt dirs are still cleaned up by orphan removal, after its grace window.Tests
TestSubprocessCodexSafetyModes/resume_creates_a_missing_log_directory: resumes a codex session withLogPathunder a missingagent-logs/thread-analysis/. Without the fix it fails with the sameopen …: no such file or directoryerror as the issue. With the fix it passes.TestPruneKeepsEmptyArtifactSubdirsOfLiveRuns: a live run with an emptyagent-logs/thread-analysis/keeps it throughPrune, and an empty layout parent is still removed. Without the fix the live run's dir is deleted.go test ./...(3825 passed) andmake lint(0 issues).