Skip to content

fix(manager): stamp event ids with the run identity readers validate - #72

Open
Huang-404-Q wants to merge 1 commit into
AMAP-ML:mainfrom
Huang-404-Q:fix-runid-event-id-inference
Open

Huang-404-Q wants to merge 1 commit into
AMAP-ML:mainfrom
Huang-404-Q:fix-runid-event-id-inference

Conversation

@Huang-404-Q

Copy link
Copy Markdown
Contributor

What this fixes

With a custom --log-dir (anything outside <runs-root>/<run-id>/lh_harness), the dashboard's event feed stays empty for a healthy run: every record is silently dropped with event_id does not belong to the requested run.

Why: the worker stamps event ids from the event log's grandparent path component (manager.py _append_event):

run_id = path.parents[2].name if len(path.parents) > 2 else "local"

but the dashboard/API validates each id against the run id it derives — the reserved id under --runs-root, or log_dir.parent.name for a pinned --log-dir. Those only coincide for the default layout. Concretely, lh-harness run --log-dir /x/myrun --dashboard stamps ids myrun:000001… while the embedded dashboard reads with the reserved id 20260828T000000Z_abcd1234 — the strict identity guard (webapi/events.py) then rejects every record, so REST replay, WebSocket deltas, and the snapshot's last-200 events all come back empty. The standalone web --log-dir path hits the same wall through its "local" fallback.

The change

  • HarnessConfig gains an optional run_id — the reader-facing run identity.
  • _append_event accepts run_event_id and prefers it over the path-derived stamp; every manager call site passes config.run_id.
  • The CLI threads the reserved run id into the config. Without a configured id (direct/test callers), the legacy path-derived stamp is kept, so default-layout behavior is unchanged.
  • The cross-run identity guard itself is untouched: foreign prefixes are still rejected.

Tests

tests/test_event_run_id.py (3 tests):

  • a configured run_id overrides the path-derived stamp;
  • without one, the stamp still falls back to the log directory's parent (default layout unchanged);
  • the reader accepts the configured stamp and keeps rejecting foreign ones.

Red/green verified: the first and third fail on main (empty feed for a pinned --log-dir), all pass with the fix. Full suite: 408 passed, 1 skipped; ruff clean on the touched files.

prax211 referenced this pull request in cogniziocompany/LongHorizon-Harness Sep 6, 2026
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
prax211 referenced this pull request in cogniziocompany/LongHorizon-Harness Sep 6, 2026
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
prax211 referenced this pull request in cogniziocompany/LongHorizon-Harness Sep 6, 2026
… open question

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@Upper9527 Upper9527 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explicit run identity is useful for the embedded dashboard, but it introduces a regression in the standalone reader. I tested this on current main with this PR applied, including the new tests (which pass).

lh-harness run --log-dir /.../custom/logs now stamps the reserved run ID. However, run_web_server(log_dir=...) still sets its run ID to Path(log_dir).parent.name (custom), so opening that same directory with lh-harness web --log-dir ... rejects every new event. In an HTTP reproduction through the actual standalone server setup, the legacy path-derived record yields 1 event, while the new configured run ID yields 0.

Please persist and resolve one canonical run identity across both embedded and standalone readers, with a backward-compatible fallback for existing logs. Add integration coverage that writes through the CLI/config path and reopens through web --log-dir, including snapshot, replay, and reconnect behavior. The current tests give the EventTailer the desired ID directly and therefore miss the reader/writer disagreement. Keeping this unmerged until that regression is addressed.

…aders

The CLI stamped event ids with the reserved run id, but the standalone
reader derived its id from the log directory's parent, so
`lh-harness web --log-dir` rejected every event of a run started with a
custom --log-dir.

Thread the reserved run id through HarnessConfig, stamp event ids with it,
and persist it as run_id.txt beside the events. Both run_web_server and
DashboardState.current_run_id resolve that file first and keep the
path-derived fallback for logs written before it existed.

Adds tests for the persisted identity and the legacy fallback.
@Huang-404-Q
Huang-404-Q force-pushed the fix-runid-event-id-inference branch from de85ae7 to 972fe2e Compare October 21, 2026 04:11
@Huang-404-Q

Copy link
Copy Markdown
Contributor Author

@Upper9527 Thanks — rebuilt from current main so the diff is only the run-identity change, and addressed the reader/writer disagreement:

  • The reserved run id is threaded through HarnessConfig and used for event ids.
  • It is persisted as run_id.txt beside the events.
  • run_web_server and DashboardState.current_run_id resolve that file first, with the path-derived fallback for logs written before it existed.

Added tests for the persisted identity and the legacy fallback. The event-append path needs POSIX (O_NOFOLLOW), so CI should run the full integration coverage.

@Upper9527 Upper9527 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed 972fe2e. Persisting the identity and using it in standalone readers addresses the earlier writer/reader disagreement, but the new resolver introduces an unsafe read in the dashboard/API path.

resolve_run_id() directly calls Path.read_text() on a worker-writable file. I reproduced both: a run_id.txt symlink to an outside file is accepted as the run identity; replacing it with a FIFO blocks the call indefinitely (the reproduction subprocess was killed after a 1-second timeout). The read is also unbounded, unlike the surrounding hardened metadata readers.

Use the existing anchored no-follow opening primitive, require an unaliased regular file, cap the byte length, and validate the run-ID syntax. Reject malformed/unsafe identities and retain the legacy fallback. Add real standalone HTTP snapshot/event replay coverage plus symlink, hard-link, FIFO, and oversized-file rejection tests. Also scope _RUN_EVENT_ID with a ContextVar token and reset it in a finally in the public run boundary: setting it without reset leaks the prior identity to subsequent direct event writes in the same async context. Keep this unmerged until those boundaries are covered.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants