Repository navigation
fix(ci): improve e2e worker failure diagnostics and cleanup - #1015
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates configuration for E2E testing infrastructure including model specifications (worker arguments, startup timeouts), environment setup (unbuffered I/O), workflow timeouts, and logging diagnostics. Shifts failure logging from per-test hooks to end-of-session diagnostics. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a diagnostic utility to dump worker logs upon backend startup failure, increases the startup timeout for the gpt-oss-120b model, and enables line buffering for worker logs. Feedback was provided to improve the log dumping function by handling default log locations, optimizing memory usage for large files using a deque, and ensuring output is flushed correctly.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 485baf44dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return | ||
| first_log = logs[0] | ||
| try: | ||
| lines = first_log.read_text(encoding="utf-8", errors="replace").splitlines() |
There was a problem hiding this comment.
_dump_first_worker_log reads the entire log into memory via read_text(...).splitlines() and only then slices the last 200 lines. When startup failures produce very large logs (common with repeated model boot retries), this can consume excessive memory or raise MemoryError right before pytest.exit, which undermines the new fail-fast diagnostic path. Please switch to a bounded tail implementation (e.g., seek-from-end or a fixed-size deque) so memory usage stays constant.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4f46ca5 — switched to deque(f, maxlen=_WORKER_LOG_TAIL_LINES) so only the last N lines are retained in memory.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/fixtures/setup_backend.py`:
- Around line 55-63: The helper currently returns early when log_dir is falsy,
missing worker logs written to the system temp dir as "smg-worker-*.log"; update
the logic around log_dir/log_path/logs so that when log_dir is unset it falls
back to searching the system temp directory (use tempfile.gettempdir()) for both
"worker-*.log" and "smg-worker-*.log" patterns, collect and sort matching Path
objects (as with the existing sorted(..., key=lambda p: p.stat().st_mtime)), and
proceed to pick the chosen log (first_log) instead of returning early.
- Around line 65-66: The code currently reads the entire file into memory via
first_log.read_text(...).splitlines() to compute tail using
_WORKER_LOG_TAIL_LINES; change this to stream the file and only retain the last
N lines (e.g., use collections.deque with maxlen=_WORKER_LOG_TAIL_LINES or an
equivalent reverse-chunk reader) so you don't load the whole file; replace the
lines/tail assignment so tail is built from the deque/streamed reader while
continuing to use the same first_log and _WORKER_LOG_TAIL_LINES identifiers.
In `@e2e_test/infra/worker.py`:
- Line 328: The parent-side open call using self._log_file = open(log_path, "w",
encoding="utf-8", buffering=1) only affects parent buffering; make the child
process emit unbuffered stdout by setting PYTHONUNBUFFERED='1' in the
environment when spawning the worker (update the env passed to the subprocess
launcher that creates the child), and remove reliance on buffering=1 for
delivering immediate child logs; optionally switch to buffering=0 for
self._log_file if truly unbuffered parent writes are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bc330d6-b483-4ab1-b82c-68f976de6c11
📒 Files selected for processing (3)
e2e_test/fixtures/setup_backend.pye2e_test/infra/model_specs.pye2e_test/infra/worker.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f46ca55cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
e2e_test/fixtures/setup_backend.py (1)
64-69:⚠️ Potential issue | 🟡 MinorMinor race condition in log file sorting.
If a log file is deleted between
glob()andstat(), thesorted()call will raiseFileNotFoundError. While unlikely and non-critical for debugging code, consider wrapping with a fallback:🛡️ Proposed defensive fix
+ def _mtime_safe(p: Path) -> float: + try: + return p.stat().st_mtime + except OSError: + return float("inf") # Push missing files to end + if not search_path.is_dir(): return - logs = sorted(search_path.glob(pattern), key=lambda p: p.stat().st_mtime) + logs = sorted(search_path.glob(pattern), key=_mtime_safe)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/fixtures/setup_backend.py` around lines 64 - 69, When computing and sorting `logs` (the list produced by `search_path.glob(pattern)`), defend against a race where files may be removed between `glob()` and `p.stat()` by filtering out non-existent paths or retrying the glob on error: replace the current `logs = sorted(search_path.glob(pattern), key=lambda p: p.stat().st_mtime)` with logic that first keeps only paths where `p.exists()` (or wraps the sort in a try/except catching `FileNotFoundError` and re-globbing) so `first_log` selection is safe; update the code around the `logs` calculation (the spots referencing `search_path`, `pattern`, `logs`, and `first_log`) to use this defensive approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e_test/fixtures/setup_backend.py`:
- Around line 64-69: When computing and sorting `logs` (the list produced by
`search_path.glob(pattern)`), defend against a race where files may be removed
between `glob()` and `p.stat()` by filtering out non-existent paths or retrying
the glob on error: replace the current `logs = sorted(search_path.glob(pattern),
key=lambda p: p.stat().st_mtime)` with logic that first keeps only paths where
`p.exists()` (or wraps the sort in a try/except catching `FileNotFoundError` and
re-globbing) so `first_log` selection is safe; update the code around the `logs`
calculation (the spots referencing `search_path`, `pattern`, `logs`, and
`first_log`) to use this defensive approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87253045-59c4-4850-a39c-f43a5ff7c1a6
📒 Files selected for processing (3)
.github/workflows/e2e-gpu-job.ymle2e_test/fixtures/setup_backend.pye2e_test/infra/worker.py
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d2e48ae18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/infra/model_specs.py`:
- Around line 35-36: The E2E model specs currently force a startup OOM via the
"worker_args" entry set to ["--mem-fraction-static", "1.1"] (and the
accompanying TODO comment), which will break normal test runs; remove or restore
this line so the default worker_args are used (delete the "worker_args":
["--mem-fraction-static", "1.1"] entry or replace it with the standard/empty
worker_args for the primary model in model_specs.py) and delete the TODO(keyang)
comment if no longer needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 794e68b1-83c1-458f-b9ad-f618f6f2f6d1
📒 Files selected for processing (1)
e2e_test/infra/model_specs.py
|
|
||
| outcome = yield | ||
| report = outcome.get_result() | ||
| def pytest_sessionfinish(session, exitstatus): |
There was a problem hiding this comment.
🟡 Nit: pytest_sessionfinish runs unconditionally — on a green run this dumps 200 lines of worker output that nobody needs. Guard with if exitstatus != 0: (or at least exitstatus not in the success range) to keep CI logs clean on passing runs.
Additionally, on the fail-fast path (_MAX_WORKER_START_FAILURES exceeded in setup_backend.py), _dump_first_worker_log() is called explicitly and then pytest.exit() triggers this hook, resulting in the first worker log being dumped twice. Either guard here with exitstatus != 0 + deduplicate with a module-level flag, or remove the explicit call in setup_backend.py and rely solely on this hook.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64fd0a19ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| log_dir = os.environ.get("E2E_LOG_DIR") | ||
| if not log_dir or not Path(log_dir).is_dir(): | ||
| return |
There was a problem hiding this comment.
pytest_sessionfinish currently emits the worker log inventory and a 200-line log tail even when the test session passes (exitstatus == 0). In large E2E shards this adds substantial noise to every green run and can push GitHub Actions logs toward truncation, which makes real failures harder to debug when they do occur; this diagnostic path should be gated to non-success exit statuses.
Useful? React with 👍 / 👎.
| # Dump first worker log snapshot | ||
| worker_logs = [f for f in log_files if f.name.startswith("worker-")] | ||
| if worker_logs: | ||
| first_log = worker_logs[0] |
There was a problem hiding this comment.
The log list is sorted by modification time ascending and then worker_logs[0] is reported as the “first failed worker log”, which actually selects the oldest worker log in the directory. When multiple workers have started across the session, this often prints an unrelated early-success log instead of the startup attempt that just failed, so the new inline diagnostics can mislead triage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e_test/conftest.py (1)
192-204:⚠️ Potential issue | 🟡 Minor
__all__exportspytest_runtest_makereportwhich does not exist.Per the AI summary,
pytest_runtest_makereportwas removed in this PR, but it remains in__all__. This hook is neither defined in this file nor imported from the fixtures package (lines 107-113). This will cause issues if anyone attempts to import it and should be removed.Proposed fix
__all__ = [ # Hooks "pytest_runtest_logstart", - "pytest_runtest_makereport", + "pytest_sessionfinish", "pytest_runtest_setup", "pytest_collection_modifyitems", "pytest_configure", # Fixtures "setup_backend", "backend_router", "model", "api_client", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/conftest.py` around lines 192 - 204, Remove the stale symbol pytest_runtest_makereport from the module export list: update the __all__ definition to no longer include "pytest_runtest_makereport" so that exported symbols reflect the actual hooks and fixtures defined (e.g., keep "pytest_runtest_logstart", "pytest_runtest_setup", "pytest_collection_modifyitems", "pytest_configure" and the fixture names like "setup_backend", "backend_router", "model", "api_client").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e_test/conftest.py`:
- Around line 121-156: pytest_sessionfinish currently always prints "First
failed worker log" and dumps the oldest worker log regardless of exit status;
wrap the block that selects and prints the worker log (the code that computes
worker_logs, first_log, tail_lines and the try/except printing using
LOG_SEPARATOR_WIDTH) in a guard that checks if exitstatus != 0 so the dump only
runs on test failure, and update the header text to remove "failed" (or
alternatively change the guard to only adjust the header when exitstatus == 0);
refer to pytest_sessionfinish, exitstatus, LOG_SEPARATOR_WIDTH and note that
setup_backend.py::_dump_first_worker_log may also emit the same log.
---
Outside diff comments:
In `@e2e_test/conftest.py`:
- Around line 192-204: Remove the stale symbol pytest_runtest_makereport from
the module export list: update the __all__ definition to no longer include
"pytest_runtest_makereport" so that exported symbols reflect the actual hooks
and fixtures defined (e.g., keep "pytest_runtest_logstart",
"pytest_runtest_setup", "pytest_collection_modifyitems", "pytest_configure" and
the fixture names like "setup_backend", "backend_router", "model",
"api_client").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 816980ed-b845-4e3a-95d9-7dae3a6a3124
📒 Files selected for processing (1)
e2e_test/conftest.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 300e96be5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # TODO(keyang): REVERT THIS — temporary OOM trigger to validate crash log diagnostics | ||
| "worker_args": ["--mem-fraction-static", "1.1"], |
There was a problem hiding this comment.
This adds --mem-fraction-static 1.1 to the primary default model spec, which requests more than 100% static GPU memory and can make SGLang worker startup fail by design. Because many tests rely on the default model path when no @pytest.mark.model is set, this turns broad E2E coverage into systematic startup failures rather than signal.
Useful? React with 👍 / 👎.
| def pytest_unconfigure(config): | ||
| """Print worker log dump and file listing after all pytest output.""" | ||
| from collections import deque |
There was a problem hiding this comment.
Moving diagnostics to pytest_unconfigure without any exit-status check makes every run print a 200-line worker log tail plus a full log inventory whenever E2E_LOG_DIR exists. On successful shards this adds substantial noise and increases the chance of CI log truncation, which reduces the usefulness of logs when an actual failure occurs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
e2e_test/conftest.py (1)
121-167:⚠️ Potential issue | 🟠 MajorFailure diagnostics are unconditional and can surface the wrong worker log.
At Line 121,
pytest_unconfigure(config)cannot access sessionexitstatus, so Lines 138-152 run even on success. Also, selectingworker_logs[-1]can miss the actual startup-failure log and reduce diagnostic value.Proposed fix
-def pytest_unconfigure(config): - """Print worker log dump and file listing after all pytest output.""" +def pytest_sessionfinish(session, exitstatus): + """Print worker log dump and file listing at end of session.""" from collections import deque from infra import LOG_SEPARATOR_WIDTH @@ - # Dump last worker log (most recent = most likely the one that failed) - worker_logs = [f for f in log_files if f.name.startswith("worker-")] - if worker_logs: - last_log = worker_logs[-1] + # Dump worker log tail only when session failed + if exitstatus != 0: + worker_logs = [f for f in log_files if f.name.startswith("worker-")] + if not worker_logs: + return + first_log = worker_logs[0] try: - with last_log.open("r", encoding="utf-8", errors="replace") as fh: + with first_log.open("r", encoding="utf-8", errors="replace") as fh: tail = deque(fh, maxlen=200) print(f"\n{sep}") - print(f"Last worker log: {last_log.name} (last {len(tail)} lines)") + print(f"Worker log: {first_log.name} (last {len(tail)} lines)") print(dash) for line in tail: print(line.rstrip("\n")) print(sep) except OSError: passIn current pytest, what are the signatures and timing differences between `pytest_unconfigure` and `pytest_sessionfinish`, and which hook should be used when logic must run only on failed sessions via `exitstatus`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e_test/conftest.py` around lines 121 - 167, Move the unconditional diagnostic logic out of pytest_unconfigure and into pytest_sessionfinish(session, exitstatus) so you can check exitstatus and only run diagnostics on failure (exitstatus != 0). In that hook, reuse the existing listing logic but choose the worker log more robustly (e.g., pick last_log = max(worker_logs, key=lambda p: (p.stat().st_size, p.stat().st_mtime)) or at minimum max by st_mtime) instead of worker_logs[-1] so you surface the most relevant/failing file; keep the deque tail dump and size-printing code otherwise unchanged (referencing pytest_sessionfinish, pytest_unconfigure, exitstatus, worker_logs, last_log).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e_test/conftest.py`:
- Around line 121-167: Move the unconditional diagnostic logic out of
pytest_unconfigure and into pytest_sessionfinish(session, exitstatus) so you can
check exitstatus and only run diagnostics on failure (exitstatus != 0). In that
hook, reuse the existing listing logic but choose the worker log more robustly
(e.g., pick last_log = max(worker_logs, key=lambda p: (p.stat().st_size,
p.stat().st_mtime)) or at minimum max by st_mtime) instead of worker_logs[-1] so
you surface the most relevant/failing file; keep the deque tail dump and
size-printing code otherwise unchanged (referencing pytest_sessionfinish,
pytest_unconfigure, exitstatus, worker_logs, last_log).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3064acfb-bc4c-42b3-9e23-c39a4261a825
📒 Files selected for processing (1)
e2e_test/conftest.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4ed9cf2dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "tp": 1, | ||
| "features": ["chat", "streaming", "function_calling"], | ||
| # TODO(keyang): REVERT THIS — temporary OOM trigger to validate crash log diagnostics | ||
| "worker_args": ["--mem-fraction-static", "1.1"], |
There was a problem hiding this comment.
The default meta-llama/Llama-3.1-8B-Instruct spec now injects --mem-fraction-static 1.1, which requests more than 100% static GPU memory and can make worker startup fail by design. Because this is the fallback/default model path for many E2E runs, it turns broad test coverage into systematic startup failures and masks real regressions behind intentional OOM behavior.
Useful? React with 👍 / 👎.
| DASH="------------------------------------------------------------" | ||
|
|
||
| # Dump last worker log (most recent = last retry of the failed model) | ||
| LAST_LOG=$(ls -t "$LOG_DIR"/worker-*.log 2>/dev/null | head -1) |
There was a problem hiding this comment.
🔴 Important: With set -euo pipefail, this line crashes the script when the log directory exists but contains no worker-*.log files. ls -t returns non-zero on no matches, and pipefail propagates that through the pipeline, causing set -e to exit the script immediately.
This can happen when the directory is created (e.g. by mkdir -p in a prior step or by router logs) but the worker never wrote a log file. The CI step would then fail, and in e2e-gpu-job.yml the subsequent echo "label=${LABEL}" >> "$GITHUB_OUTPUT" would never execute, breaking the artifact upload name.
Line 38 already handles this pattern correctly with || echo " (none)". Apply the same fix here:
| LAST_LOG=$(ls -t "$LOG_DIR"/worker-*.log 2>/dev/null | head -1) | |
| LAST_LOG=$(ls -t "$LOG_DIR"/worker-*.log 2>/dev/null | head -1 || true) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b985ab3f26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| DASH="------------------------------------------------------------" | ||
|
|
||
| # Dump last worker log (most recent = last retry of the failed model) | ||
| LAST_LOG=$(ls -t "$LOG_DIR"/worker-*.log 2>/dev/null | head -1) |
There was a problem hiding this comment.
With set -euo pipefail, the command substitution on this line exits non-zero when worker-*.log matches nothing, so ci_dump_worker_logs.sh aborts on empty log directories instead of printing the (none) summary. Both .github/workflows/e2e-gpu-job.yml and .github/workflows/pr-test-rust.yml call this script on any failure/cancellation, so failures that happen before any worker starts (or after only directory creation) make the diagnostic step itself fail and can leave downstream artifact naming/diagnostics incomplete.
Useful? React with 👍 / 👎.
…meout Line-buffer worker log files so output is not lost on crash, dump the first failed worker log (last 200 lines) to CI output after all retries are exhausted, and extend gpt-oss-120b startup timeout to 10 minutes. Signed-off-by: key4ng <rukeyang@gmail.com>
The hardcoded 20-minute step timeout was too tight for jobs like e2e-2gpu-responses where model startup alone can take 10 minutes. Signed-off-by: key4ng <rukeyang@gmail.com>
- Fall back to tempdir when log_dir is unset so crash logs are found - Use deque for memory-efficient log tail instead of reading entire file - Set PYTHONUNBUFFERED=1 on child process instead of parent-side buffering - Increase e2e test step timeout from 20 to 25 minutes Signed-off-by: key4ng <rukeyang@gmail.com>
…ostics REVERT THIS COMMIT after verifying the worker log dump works in CI. Sets --mem-fraction-static 1.1 to trigger a guaranteed startup crash. Signed-off-by: key4ng <rukeyang@gmail.com>
Replace per-test-failure log listing with a single pytest_unconfigure hook that runs after all pytest output. Dumps the last worker log (last 200 lines) with file sizes, then lists all log files. Removes duplicate log dump from setup_backend fail-fast path. Signed-off-by: key4ng <rukeyang@gmail.com>
Replace pytest hook-based log diagnostics with a dedicated CI step that runs on failure. This ensures logs are dumped even if pytest is killed by the step timeout, and keeps test code clean. Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
Append worker to the list before calling start() so that if startup fails (timeout or crash), stop_workers() still kills the process and releases GPU memory. Previously the leaked process held GPU memory, causing retries to fail with the same OOM. Also keep the CI log dump step using the last worker log, which now correctly reflects the final retry's genuine error rather than a cascading OOM from leaked prior attempts. Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
- Inline label computation into the snapshot step (remove separate step) - Rename steps to "Worker log snapshot" for clarity - Add worker log snapshot to PR benchmark job Signed-off-by: key4ng <rukeyang@gmail.com>
Move inline log dump logic to scripts/ci_dump_worker_logs.sh and call it from both e2e-gpu-job.yml and pr-test-rust.yml benchmarks. Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
Signed-off-by: key4ng <rukeyang@gmail.com>
key4ng
changed the title
|
worker error will directly show in ci output |
…ect#1015) Signed-off-by: key4ng <rukeyang@gmail.com>
2 participants
Description
Problem
When a worker crashes during e2e test startup (e.g. OOM during model loading):
Solution
PYTHONUNBUFFERED=1to worker child processes so log output is flushed immediatelystart()sostop_workers()kills leaked processesscripts/ci_dump_worker_logs.shscriptopenai/gpt-oss-120bstartup timeout to 10 minutesChanges
scripts/ci_dump_worker_logs.sh: New shared script for dumping the last worker log and listing all log files with sizes.github/workflows/e2e-gpu-job.yml: Add "Worker failure diagnostics" step, bump test step timeout to 25 min.github/workflows/pr-test-rust.yml: Add "Worker failure diagnostics" step to PR benchmarkse2e_test/infra/worker.py: SetPYTHONUNBUFFERED=1on child env; fix worker cleanup orderinge2e_test/infra/model_specs.py: Addstartup_timeout: 600foropenai/gpt-oss-120be2e_test/conftest.py: Remove old per-test-failure log listing hook (replaced by CI step)Test Plan
--mem-fraction-static 1.1)Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassespre-commit run --all-filespasses