Uh oh!
There was an error while loading. Please reload this page.
feat(runtime): add event_log_dir to configure event log output directory - #474
feat(runtime): add event_log_dir to configure event log output directory#474nskun (nskun) wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #474 +/- ##
=======================================
Coverage ? 91.73% =======================================
Files ? 146 Lines ? 23779 Branches ? 0 =======================================
Hits ? 21814 Misses ? 1965 Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nskun (nskun)
commented
Aug 24, 2026
The Windows CI failure is in an unrelated, timing-sensitive TUI timer test: tests/test_fleet/test_tui_runs.py::TestRunsScreenPausesWhileNotOnTop::test_the_animation_timer_is_paused_and_resumed This PR does not modify the TUI code, and the same Windows check passes on the base commit. Could a maintainer please rerun the failed job? |
nskun (nskun)
commented
Aug 24, 2026
I pushed an empty commit to retrigger CI. |
nskun (nskun)
commented
Aug 24, 2026
I also fixed relative path resolution so runtime.event_log_dir is now resolved against the workflow file's directory, as documented, for both run and resume. Added regression coverage for cases where the process CWD differs from the workflow directory. |
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Reviewed commit 13cba6c in a worktree with a set of focused agents. Lint and format pass, ty adds no new diagnostics, and the four new tests pass.
The problem this solves is real. The event log is the primary post-mortem artifact for a crashed run and $TMPDIR gets swept, so making the location configurable is worth doing. My comments are about how the configured value is resolved, and about the parts of the system that still assume the old location.
One finding has no line to attach to, because the file is not in this diff:
tests/test_fleet/test_run_record_wiring.py builds its config with _mock_config(), which returns a MagicMock. MagicMock implements __fspath__, so _eld is truthy and Path(_eld).resolve() produces a path inside the repository, which event_log.py:160 then creates. From a clean tree:
$ pytest tests/test_fleet/test_run_record_wiring.py
10 passed
$ git status --porcelain
?? MagicMock/
$ find MagicMock -type f | head -1
MagicMock/mock.workflow.runtime.event_log_dir/137076342320704/conductor-wiring-test-...events.jsonl
Twelve test files drive run_workflow_async or resume_workflow_async, so it is not confined to one suite. It gets past both autouse guards in tests/conftest.py because they patch $TMPDIR and CONDUCTOR_HOME, while this path anchors on the process working directory. It also weakens an existing assertion: test_run_record_wiring.py:202 used to prove the default $TMPDIR path worked, and now only proves that a MagicMock/ path exists. Setting event_log_dir = None in _mock_config() clears it.
Two behaviours I verified by running a workflow from a directory other than the one holding the YAML: ./logs resolved against the shell's working directory rather than the workflow file's directory, and ~/conductor-logs created a directory literally named ~. On the fleet side, a completed run appears in build_history_entries() with the default location and gives zero entries with event_log_dir set.
Nothing here is hard to fix, and most of it collapses into one shared resolution helper.
| _eld = config.workflow.runtime.event_log_dir | ||
| _event_log_dir = Path(_eld).resolve() if _eld else None |
There was a problem hiding this comment.
Path(...).resolve() anchors a relative path to the process working directory, but the docstring for this field at schema.py:3459 says relative paths resolve against the workflow file's directory. I checked: a workflow at /tmp/eld-test/wf.yaml with event_log_dir: ./logs, invoked from /tmp/eld-cwd, wrote to /tmp/eld-cwd/logs. The same workflow file puts its logs somewhere different depending on which shell you start it from.
Two related problems on the same line. There is no expanduser(), so event_log_dir: ~/logs creates a directory named ~ under the current directory (verified). And resolve() follows symlinks, which _resolve_agent_working_dir at engine/workflow.py:620 deliberately avoids so symlink aliases stay distinct.
workflow_path is already a parameter of both functions, so the workflow directory is available right here.
| _eld=config.workflow.runtime.event_log_dir | |
| _event_log_dir=Path(_eld).resolve() if_eldelseNone | |
| configured_dir=config.workflow.runtime.event_log_dir | |
| _event_log_dir=None | |
| ifconfigured_dir: | |
| candidate=Path(configured_dir).expanduser() | |
| ifnotcandidate.is_absolute(): | |
| candidate=Path(workflow_path).resolve().parent/candidate | |
| _event_log_dir=Path(os.path.normpath(candidate)) |
Better still, put this in one helper instead of applying it twice. The same two lines appear again at 2774, and the Run/Resume parity rule in AGENTS.md names the event log subscriber specifically. Two copies of a resolution policy is how those two paths drift apart.
| """Directory for event log output. | ||
| When set, event logs are written to this directory instead of | ||
| ``$TMPDIR/conductor/``. Relative paths are resolved against the | ||
| workflow file's directory. | ||
| When omitted, behavior is unchanged (writes to ``$TMPDIR/conductor/``). | ||
| """ |
There was a problem hiding this comment.
The sentence about relative paths does not match cli/run.py:2058, which resolves against the process working directory. See my note there. Whichever way that goes, this text has to agree with it. Worth noting spill_dir a few hundred lines up has the same mechanics and documents the opposite rule, so right now two adjacent fields contradict each other.
There is also no validator. event_log_dir: " " is truthy, so it creates a directory named with three spaces. "" happens to behave, but only because run.py tests truthiness while event_log.py:155 tests is not None. That split is what _normalize_spill_dir at line 3123 exists to prevent, and its docstring says so directly.
@field_validator("event_log_dir")@classmethoddef_normalize_event_log_dir(cls, v: str|None) ->str|None:
"""Normalize an empty or whitespace-only value to None. cli/run.py tests this field for truthiness while EventLogSubscriber tests it for `is not None`, so without one normalization point an empty string means "default directory" to one consumer and "process cwd" to the other. """ifvisNone:
returnNonereturnv.strip() orNoneTwo things the docstring should also say: a resumed run keeps writing to the log recorded in its checkpoint and ignores this setting, and a log written outside $TMPDIR/conductor/ is skipped by the retention sweep and by conductor fleet History.
grep event_log_dir docs/ currently comes back empty, and docs/cli-reference.md:333 still states the log lives under $TMPDIR/conductor/.
| *, | ||
| existing_path: Path | None = None, | ||
| existing_run_id: str | None = None, | ||
| event_log_dir: Path | None = None, |
There was a problem hiding this comment.
The Args: block below documents the other three parameters but not this one. The precedence rule is missing too: event_log_dir is ignored when the subscriber appends to existing_path. That behaviour is correct, since a resumed run should keep one continuous log, but a test is currently the only place it is recorded.
Something like:
event_log_dir: Base directory for a freshly created log, replacing
the default ``$TMPDIR/conductor/``. Ignored when the
``existing_path`` append branch is taken, so a resumed run keeps
writing to the log its checkpoint points at. Callers resolve
relative paths and ``~`` before passing a value here.
The case this leaves confusing: set event_log_dir after an initial run, then resume. The new directory stays empty, the log keeps growing in $TMPDIR/conductor/ where the retention sweep will eventually remove it, and nothing says why. A logger.info on that branch would cover it. The sibling branch already logs when it cannot append, so the asymmetry stands out.
The module docstring at lines 3-4 also still says the log goes to $TMPDIR/conductor/ without qualification.
| base_dir = ( | ||
| event_log_dir | ||
| if event_log_dir is not None | ||
| else Path(tempfile.gettempdir()) / "conductor" | ||
| ) |
There was a problem hiding this comment.
Minor: this collapses to one line. PurePath defines neither __bool__ nor __len__, so every Path is truthy and None is the only falsy input, which makes or equivalent to the is not None test here.
| base_dir= ( | |
| event_log_dir | |
| ifevent_log_dirisnotNone | |
| elsePath(tempfile.gettempdir()) /"conductor" | |
| ) | |
| base_dir=event_log_dirorPath(tempfile.gettempdir()) /"conductor" |
That equivalence depends on the parameter staying Path | None. If it is ever widened to accept str, an empty string becomes falsy and or would quietly swallow it.
| if event_log_dir is not None | ||
| else Path(tempfile.gettempdir()) / "conductor" | ||
| ) | ||
| self._path = base_dir / f"conductor-{workflow_name}-{ts}-{self._run_id}.events.jsonl" |
There was a problem hiding this comment.
Worth deciding explicitly what should happen to a run whose log lands outside $TMPDIR/conductor/. Five places derive that directory independently and none of them consult the new setting:
fleet/retention.py:76(event_log_root), so the log is never pruned and the directory grows without boundfleet/history.py:476, so the run never appears inconductor fleetHistoryfleet/resume.py, which joins checkpoints against History entries, so the Resume action is never offered for itfleet/records.py:889, the event-log recovery lookupcli/bg_runner.py:1058, which still writes.bg.stderr.logand.bg.stdout.logto$TMPDIR/conductor/while the events log moves elsewhere
I confirmed the History case: one entry from build_history_entries() with the default location, zero with event_log_dir set.
That last item has a user-visible consequence of its own. cli/app.py:219 locates the capture logs with Path(record.event_log_path).parent, so conductor status --json reports stderr_log: null for a background run whose capture logs are sitting one directory over. It also breaks the --web-bg debugging procedure in AGENTS.md, which depends on the three artifacts sharing a directory, and retention's guarantee that they are kept or removed together.
Either route these readers through a shared accessor, or say plainly in the field docstring that a custom directory opts the run out of History, retention, and background log correlation. The empty History screen is the part I would not leave silent. AGENTS.md is explicit that showing an empty state for a directory that was never read is a claim of absence that reads like success.
| ) | ||
| self._path = base_dir / f"conductor-{workflow_name}-{ts}-{self._run_id}.events.jsonl" | ||
| self._path.parent.mkdir(parents=True, exist_ok=True) | ||
| self._handle = open(self._path, "w", encoding="utf-8") # noqa: SIM115 |
There was a problem hiding this comment.
Now that this path can come from user config, these two lines can raise. I checked both cases: pointing event_log_dir at an existing file gives FileExistsError, and an unwritable directory gives PermissionError. Neither run_workflow_async nor resume_workflow_async wraps the call in an except, so it reaches print_error and the user gets a panel titled PermissionError containing [Errno 13] Permission denied: '/sys/conductor-logs'. The field name never appears, and the workflow exits 1 before running a single step.
So a typo in a diagnostics setting now stops work that would otherwise have succeeded, which is the opposite of what an always-on flight recorder should do.
Everything comparable in this codebase degrades instead. The existing_path branch at lines 112-125 catches OSError and creates a fresh log with a warning. --log-file warns and continues (run.py:1962). The dashboard warns and continues (run.py:2016). spill_dir in mcp/manager.py:450 is documented as best effort that must never raise.
Catching OSError here and falling back to $TMPDIR/conductor/ with a warning naming the field would match the rest, and it keeps the log alive rather than trading it for a dead run. If strict failure is preferred instead, it should be a config error raised by conductor validate, which currently passes clean on all of these values.
| from conductor.events import WorkflowEvent | ||
| def test_runtime_config_event_log_dir(): |
There was a problem hiding this comment.
Every test in this file hands the subscriber a Path that is already absolute, so the piece this PR actually adds, the conversion from the YAML string in cli/run.py, is never exercised. That is why the relative-path and ~ behaviour went unnoticed.
Cases worth adding: a relative value where the process runs somewhere other than the workflow file's directory, ~, whitespace, a path that already exists as a file, and the resume wiring. tests/test_fleet/test_run_record_wiring.py is a natural home for the last one, since it already drives both async helpers and can assert where record.event_log_path lands.
Two smaller notes. This test exercises RuntimeConfig rather than the engine, so tests/test_config/ fits better. And test_default_writes_to_tmpdir largely repeats test_event_log.py:16, whose TestEventLogSubscriber class already owns this surface, so these might be more discoverable as methods there.
Line 17 also has a stray pair of parens around the string literal.
| finally: | ||
| if not sub._handle.closed: | ||
| sub.close() |
There was a problem hiding this comment.
close() is already idempotent. event_log.py:194 guards with self._handle is not None and not self._handle.closed, and test_event_log.py:73 already pins that with a double close. This guard re-implements the check and reaches into a private attribute to do it.
| finally: | |
| ifnotsub._handle.closed: | |
| sub.close() | |
| finally: | |
| sub.close() |
nskun (nskun)
commented
Aug 25, 2026
Thank you so much for the incredibly thorough review — I really appreciate the time and care you put into it. |
Summary
Add a new event_log_dir field to RuntimeConfig so users can configure where event logs are written, instead of the hardcoded $TMPDIR/conductor/ .
Motivation
Event log are always written to $TMPDIR/conductor/ (typically /tmp/conductor/ ).
On Linux, tmp is cleared on reboot or by tmpwatch, so diagnostic data from long-running workflows can be lost before it is reviewed.
Users currently need awrapper script to copy event logs to a persistent directory after each run.
Configurable log output directories are standard in self-hosted orchestration tools (Airflow base_log_folder, Argo Workflows artifactRepository, Docker log-driver, Kubernetes --log-dir).
Usage
Relative paths are resolved against the workflow file's directory.
When omitted, writes to $TMPDIR/conductor as before.
Filename pattern (conductor-{name}-{ts}-{run_id}.events.jsonl) is unchanged -- only the parent directory changes.
Closes#475