feat(jobs): add controller diagnostics and per-module E2E config pooling - #440
Merged
Conversation
Contributor
|
🌿 Preview your docs: https://nvidia-preview-auth-tests-4-rsadler.docs.buildwithfern.com/nemo-platform |
Contributor
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (27)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (20)
📝 WalkthroughWalkthroughAdds job diagnostics, workspace-scoped job/task handling, and a pooled local E2E harness with auth-enabled job tests and CLI URL injection. ChangesJobs Service Changes
E2E Harness Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)services/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.19][ERROR]: unable to find a config; path services/core/jobs/src/nmp/core/jobs/app/dispatcher.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.22][ERROR]: unable to find a config; path services/core/jobs/src/nmp/core/jobs/config.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.17][ERROR]: unable to find a config; path
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/conftest.py (1)
245-253: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDon't inherit ambient auth env for pooled local services.
This still forwards
NMP_ACCESS_TOKEN/NMP_CONTEXT_NAMEwhen_services_instanceis the local pooled platform. If those env vars are set, the auth E2Es can run under the wrong principal or combine bearer auth with the injected admin headers. Only pass env credentials on the external-cluster path.Suggested fix
- access_token = os.environ.get("NMP_ACCESS_TOKEN") - context_name = os.environ.get("NMP_CONTEXT_NAME") + use_external_auth = _services_instance.proc is None + access_token = os.environ.get("NMP_ACCESS_TOKEN") if use_external_auth else None + context_name = os.environ.get("NMP_CONTEXT_NAME") if use_external_auth else None headers = admin_headers() if _services_instance.auth_enabled else {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/conftest.py` around lines 245 - 253, The NeMoPlatform construction in the local service setup is still picking up ambient NMP_ACCESS_TOKEN and NMP_CONTEXT_NAME even for the pooled local services path. Update the logic around the NeMoPlatform call to only read and pass those env credentials on the external-cluster branch, and keep the pooled local path using only the injected admin_headers from admin_headers() when _services_instance.auth_enabled is true. Use the existing _services_instance, admin_headers, and NeMoPlatform symbols to separate the auth sources cleanly.
🧹 Nitpick comments (2)
services/core/jobs/tests/controllers/test_diagnostics.py (1)
37-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression for stack truncation.
These tests only cover the job-log flag. Add a case with an oversized traceback in
error_details["error"]and assert the snapshot is capped, otherwise the serializer bug above stays invisible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/jobs/tests/controllers/test_diagnostics.py` around lines 37 - 66, Add a regression test for oversized traceback handling in collect_job_diagnostics, since the current cases only verify the include_job_logs_in_diagnostics flag. Extend test_diagnostics.py with a scenario that sets a very large value in error_details["error"] and assert the serialized diagnostics snapshot is truncated/capped appropriately. Use collect_job_diagnostics and the existing _make_sdk_with_logs helper so the serializer behavior stays covered even when raw job logs are disabled.services/core/jobs/tests/controllers/test_reconciler.py (1)
65-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the unexpected-exception branch too.
The reconciler now logs diagnostics from two new paths, but this test only exercises the error-transition one. Add a case where
sync()raises and assert the"unexpected reconciliation error"context so that handler cannot regress silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/jobs/tests/controllers/test_reconciler.py` around lines 65 - 89, The reconciler test only covers the error-status transition path in JobReconciler.step; add coverage for the unexpected-exception branch as well. Extend the test around JobReconciler and log_job_diagnostics_if_debug so that a patched test_backend.sync raises an exception, then assert log_job_diagnostics_if_debug is called with the same mock_client and test_step_active but with context set to "unexpected reconciliation error" and the JobReconciler logger.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/services_pool.py`:
- Around line 405-415: The HTTP-only wait loops in _wait_for_healthy and the
other startup polling helpers keep sleeping even after the child process has
already exited, so update these checks to inspect the spawned process state
during the wait and abort early when it terminates. In the functions that poll
readiness for services in services_pool.py, add a process-exit check alongside
the existing httpx status polling, and return failure immediately if the child
is no longer running instead of consuming the full timeout.
- Around line 101-104: The acquire_for_module path currently forces external_url
runs to use RunningServices with auth_enabled=False, which ignores the module’s
marker-derived auth mode. Update acquire_for_module to preserve the auth setting
inferred earlier for the module instead of hard-coding unauthenticated behavior,
using the existing RunningServices constructor and any auth-related state
already determined by the harness.
In `@e2e/test_jobs_auth.py`:
- Around line 227-229: The jobs listing assertion is only checking the first
page returned by jobs.list(workspace=ALL_WORKSPACES), so it can miss the target
job when results are paginated. Update the test to either filter by the specific
job/workspace using the jobs.list call or the related jobs API helper, or
iterate through all pages before asserting; use the existing jobs.list and
jobs.pagination/data handling in the test to locate the job reliably.
- Around line 78-79: These tests create workspaces directly via
admin_sdk.workspaces.create and grant_workspace_role, bypassing the workspace
fixture cleanup path and risking leaked resources. Update the affected tests in
test_jobs_auth.py to ensure every created workspace is always torn down, either
by wrapping the test body in try/finally cleanup that removes the workspace and
associated grants/filesets/jobs, or by introducing and using an admin workspace
fixture with teardown. Apply the same cleanup pattern to the other direct
workspace-creation spots in this file so the lifecycle matches the existing
workspace fixture contract.
In `@services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py`:
- Around line 374-379: Normalize the fallback timestamp used in the subprocess
recovery path before selecting the latest task: in the logic that computes
latest_task with max() in the subprocess backend, make sure the default value
matches the timezone-aware timestamp contract used by task.updated_at and
task.created_at. Replace the naive datetime.datetime.min fallback with a
timezone-aware minimum and keep the comparison within the existing task
selection flow so max() can safely compare tasks even when some metadata
timestamps are missing.
In `@services/core/jobs/src/nmp/core/jobs/controllers/diagnostics.py`:
- Around line 36-61: The debug serializers in _task_dict, _step_dict, and
_job_dict pass through error_details without limiting traceback size, so apply
the same trimming used by _trim_error_stack to any traceback stored under
error_details (especially the "error" field) before returning the snapshot. Add
or reuse a small helper near _trim_error_stack to cap long strings inside
error_details, and make sure all three dict builders call it so task, step, and
job payloads stay bounded.
In `@services/core/jobs/tests/controllers/test_subprocess_backend.py`:
- Around line 320-323: The test currently removes the subprocess registry entry
before stopping the spawned process, which can leave the real sleep process
orphaned. In test_subprocess_backend.py, update the cleanup around
_schedule_without_otel_export and SubprocessProcessKey so the process is
explicitly terminated or waited on first, and only then pop the entry from
backend._process_registry. Make sure the test uses the tracked handle from the
registry to stop the child before discarding it.
---
Outside diff comments:
In `@e2e/conftest.py`:
- Around line 245-253: The NeMoPlatform construction in the local service setup
is still picking up ambient NMP_ACCESS_TOKEN and NMP_CONTEXT_NAME even for the
pooled local services path. Update the logic around the NeMoPlatform call to
only read and pass those env credentials on the external-cluster branch, and
keep the pooled local path using only the injected admin_headers from
admin_headers() when _services_instance.auth_enabled is true. Use the existing
_services_instance, admin_headers, and NeMoPlatform symbols to separate the auth
sources cleanly.
---
Nitpick comments:
In `@services/core/jobs/tests/controllers/test_diagnostics.py`:
- Around line 37-66: Add a regression test for oversized traceback handling in
collect_job_diagnostics, since the current cases only verify the
include_job_logs_in_diagnostics flag. Extend test_diagnostics.py with a scenario
that sets a very large value in error_details["error"] and assert the serialized
diagnostics snapshot is truncated/capped appropriately. Use
collect_job_diagnostics and the existing _make_sdk_with_logs helper so the
serializer behavior stays covered even when raw job logs are disabled.
In `@services/core/jobs/tests/controllers/test_reconciler.py`:
- Around line 65-89: The reconciler test only covers the error-status transition
path in JobReconciler.step; add coverage for the unexpected-exception branch as
well. Extend the test around JobReconciler and log_job_diagnostics_if_debug so
that a patched test_backend.sync raises an exception, then assert
log_job_diagnostics_if_debug is called with the same mock_client and
test_step_active but with context set to "unexpected reconciliation error" and
the JobReconciler logger.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 61f1d69c-0792-4e63-b660-c4bf29600712
📒 Files selected for processing (25)
docs/set-up/config-reference.mdxe2e/configs/local-subprocess.yamle2e/conftest.pye2e/services_pool.pye2e/test_data_designer.pye2e/test_jobs.pye2e/test_jobs_auth.pypackages/nmp_testing/src/nmp/testing/__init__.pypackages/nmp_testing/src/nmp/testing/utils.pypackages/nmp_testing/tests/unit/test_e2e_harness.pypytest.iniservices/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.pyservices/core/jobs/src/nmp/core/jobs/app/dispatcher.pyservices/core/jobs/src/nmp/core/jobs/config.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/src/nmp/core/jobs/controllers/diagnostics.pyservices/core/jobs/src/nmp/core/jobs/controllers/reconciler.pyservices/core/jobs/src/nmp/core/jobs/controllers/scheduler.pyservices/core/jobs/tests/controllers/test_diagnostics.pyservices/core/jobs/tests/controllers/test_reconciler.pyservices/core/jobs/tests/controllers/test_scheduler.pyservices/core/jobs/tests/controllers/test_subprocess_backend.pyservices/core/jobs/tests/integration/test_task_auth_runtime.pyservices/core/jobs/tests/test_dispatcher_cross_workspace.pyservices/core/jobs/tests/test_timestamp_contracts.py
matthewgrossman
left a comment
matthewgrossman
left a comment
Contributor
There was a problem hiding this comment.
like the e2e config hashing mechanism!
matthewgrossman
approved these changes
Jun 24, 2026
ironcommit
force-pushed
the
auth-tests-4/rsadler
branch
2 times, most recently
from
June 24, 2026 19:00
ab87b4e to
c3b9646
Compare
ironcommit
enabled auto-merge
June 24, 2026 19:02
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
services/core/jobs/tests/controllers/test_subprocess_backend.py (1)
324-327: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse backend cleanup helpers before dropping registry entry.
process.terminate()can miss child processes for/bin/sh -c ..., and popping without_finish_logs()skips log-thread/logger cleanup. Use backend-managed termination/finish, then pop.Suggested fix
- metadata.process.terminate() - assert metadata.process.wait(timeout=5) is not None - backend._process_registry.pop(key) + backend._terminate_process(metadata, force=True) + backend._finish_logs(metadata) + backend._process_registry.pop(key)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/core/jobs/tests/controllers/test_subprocess_backend.py` around lines 324 - 327, The subprocess test is bypassing backend-managed shutdown by calling metadata.process.terminate() and removing the registry entry directly, which skips child-process handling and log cleanup. Update the test to use the backend cleanup path exposed by SubprocessBackend, making sure the process is terminated through the backend helper and _finish_logs() runs before backend._process_registry.pop(key) is called. Use the existing metadata, key, and backend references in test_subprocess_backend.py to locate the cleanup sequence.
🧹 Nitpick comments (1)
tests/test_e2e_jobs_auth.py (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a concrete type hint instead of the string forward reference.
list["_StubPage"]is string-based. Addfrom __future__ import annotationsat the top so this can be written aslist[_StubPage].♻️ Proposed change
+from __future__ import annotations + import pytest- def __init__(self, pages: list["_StubPage"], job_names: list[str]) -> None: + def __init__(self, pages: list[_StubPage], job_names: list[str]) -> None:As per coding guidelines: "Always prefer concrete type hints over string-based ones in Python code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_e2e_jobs_auth.py` at line 29, Add future annotations support at the top of the test module so the constructor in the stub class can use a concrete type hint instead of the string forward reference. Update the __init__ signature that currently uses list["_StubPage"] to use list[_StubPage], and keep the rest of the stub/test code unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/services_pool.py`:
- Line 6: Remove the unnecessary postponed-annotations import from this module.
In e2e/services_pool.py, delete the from __future__ import annotations statement
and keep the existing type hints concrete; there is no forward-reference
requirement here, so the file should follow the project’s rule against
string-based annotations and use regular imports where needed.
---
Duplicate comments:
In `@services/core/jobs/tests/controllers/test_subprocess_backend.py`:
- Around line 324-327: The subprocess test is bypassing backend-managed shutdown
by calling metadata.process.terminate() and removing the registry entry
directly, which skips child-process handling and log cleanup. Update the test to
use the backend cleanup path exposed by SubprocessBackend, making sure the
process is terminated through the backend helper and _finish_logs() runs before
backend._process_registry.pop(key) is called. Use the existing metadata, key,
and backend references in test_subprocess_backend.py to locate the cleanup
sequence.
---
Nitpick comments:
In `@tests/test_e2e_jobs_auth.py`:
- Line 29: Add future annotations support at the top of the test module so the
constructor in the stub class can use a concrete type hint instead of the string
forward reference. Update the __init__ signature that currently uses
list["_StubPage"] to use list[_StubPage], and keep the rest of the stub/test
code unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0bc81dc7-0512-4196-b3ee-8ab33ea3c454
📒 Files selected for processing (27)
docs/set-up/config-reference.mdxe2e/configs/local-subprocess.yamle2e/conftest.pye2e/services_pool.pye2e/test_data_designer.pye2e/test_jobs.pye2e/test_jobs_auth.pypackages/nmp_testing/src/nmp/testing/__init__.pypackages/nmp_testing/src/nmp/testing/utils.pypackages/nmp_testing/tests/unit/test_e2e_harness.pypytest.iniservices/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.pyservices/core/jobs/src/nmp/core/jobs/app/dispatcher.pyservices/core/jobs/src/nmp/core/jobs/config.pyservices/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.pyservices/core/jobs/src/nmp/core/jobs/controllers/diagnostics.pyservices/core/jobs/src/nmp/core/jobs/controllers/reconciler.pyservices/core/jobs/src/nmp/core/jobs/controllers/scheduler.pyservices/core/jobs/tests/controllers/test_diagnostics.pyservices/core/jobs/tests/controllers/test_reconciler.pyservices/core/jobs/tests/controllers/test_scheduler.pyservices/core/jobs/tests/controllers/test_subprocess_backend.pyservices/core/jobs/tests/integration/test_task_auth_runtime.pyservices/core/jobs/tests/test_dispatcher_cross_workspace.pyservices/core/jobs/tests/test_timestamp_contracts.pytests/test_e2e_jobs_auth.pytests/test_e2e_services_pool.py
✅ Files skipped from review due to trivial changes (3)
- e2e/test_jobs.py
- docs/set-up/config-reference.mdx
- packages/nmp_testing/src/nmp/testing/init.py
🚧 Files skipped from review as they are similar to previous changes (19)
- services/core/jobs/src/nmp/core/jobs/config.py
- services/core/jobs/src/nmp/core/jobs/api/v2/jobs/endpoints.py
- services/core/jobs/tests/test_timestamp_contracts.py
- services/core/jobs/tests/test_dispatcher_cross_workspace.py
- pytest.ini
- packages/nmp_testing/src/nmp/testing/utils.py
- e2e/test_data_designer.py
- services/core/jobs/tests/controllers/test_reconciler.py
- services/core/jobs/src/nmp/core/jobs/app/dispatcher.py
- packages/nmp_testing/tests/unit/test_e2e_harness.py
- services/core/jobs/tests/controllers/test_scheduler.py
- services/core/jobs/tests/integration/test_task_auth_runtime.py
- e2e/configs/local-subprocess.yaml
- services/core/jobs/src/nmp/core/jobs/controllers/backends/subprocess.py
- services/core/jobs/src/nmp/core/jobs/controllers/scheduler.py
- e2e/test_jobs_auth.py
- services/core/jobs/src/nmp/core/jobs/controllers/diagnostics.py
- e2e/conftest.py
- services/core/jobs/src/nmp/core/jobs/controllers/reconciler.py
Add a `diagnostics` module to the jobs controller that collects a structured snapshot of job/step/task state for debugging stuck or failing jobs. Log snapshots at DEBUG level in the scheduler and reconciler, with a `include_job_logs_in_diagnostics` config flag (default off) to gate raw log inclusion since logs may contain secrets or PII. Refactor the E2E harness to support per-module config selection via an `e2e_config()` pytest mark. Config layers (YAML files + inline overlays) are resolved to a canonical hash and pooled, so modules sharing the same effective config reuse one running services instance and it is torn down as soon as the last consumer finishes. Extract all service lifecycle logic from conftest.py into a new `services_pool.py` module. Add `e2e/configs/local-subprocess.yaml` and a new `test_jobs_auth.py` suite that enables auth and exercises job principal propagation end-to-end using unsigned JWTs. Add unit tests for diagnostics, scheduler priority ordering, subprocess backend, and timestamp contracts. Signed-off-by: Ryan S <267728323+ironcommit@users.noreply.github.com>
ironcommit
force-pushed
the
auth-tests-4/rsadler
branch
from
June 24, 2026 19:45
c3b9646 to
f9749fa
Compare
This was referenced Jul 6, 2026
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
2 participants
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.
Add a
diagnosticsmodule to the jobs controller that collects a structured snapshot of job/step/task state for debugging stuck or failing jobs. Log snapshots at DEBUG level in the scheduler and reconciler, with ainclude_job_logs_in_diagnosticsconfig flag (default off) to gate raw log inclusion since logs may contain secrets or PII.Refactor the E2E harness to support per-module config selection via an
e2e_config()pytest mark. Config layers (YAML files + inline overlays) are resolved to a canonical hash and pooled, so modules sharing the same effective config reuse one running services instance and it is torn down as soon as the last consumer finishes. Extract all service lifecycle logic from conftest.py into a newservices_pool.pymodule.Add
e2e/configs/local-subprocess.yamland a newtest_jobs_auth.pysuite that enables auth and exercises job principal propagation end-to-end using unsigned JWTs. Add unit tests for diagnostics, scheduler priority ordering, subprocess backend, and timestamp contracts.Summary by CodeRabbit
New Features
include_job_logs_in_diagnostics(off by default) to optionally include raw job logs in diagnostics snapshots.e2e_config(...)pytest marker support for selecting layered E2E configurations.Bug Fixes
Tests