Skip to content

fix: kill nix-eval-jobs subprocess on evaluation failure - #908

Draft
WHOIM1205 wants to merge 4 commits into
NixOS:mainfrom
WHOIM1205:fix/kill-orphan-eval-subprocess
Draft

fix: kill nix-eval-jobs subprocess on evaluation failure#908
WHOIM1205 wants to merge 4 commits into
NixOS:mainfrom
WHOIM1205:fix/kill-orphan-eval-subprocess

Conversation

@WHOIM1205

Copy link
Copy Markdown
Contributor

Description

This PR fixes a critical subprocess lifecycle bug where nix-eval-jobs processes were left running as orphans whenever an exception occurred during evaluation ingestion.

Previously, failures inside the ingestion loop (e.g. DB errors, integrity issues, timeouts) would mark the evaluation as CRASHED but never terminate the spawned subprocess, leading to severe resource leaks and inconsistent system state.

Changes

  • Updated evaluation_entrypoint() in nix_evaluation.py to properly manage subprocess lifecycle
    • Wrapped subprocess execution in a try/finally block
    • Ensures eval_process.kill() and await eval_process.wait() are always called on failure paths
  • Added safety check to only kill the process if it is still running (returncode is None)

Impact

  • Prevents orphaned nix-eval-jobs processes consuming CPU and memory
  • Eliminates cascading failures caused by resource exhaustion under concurrent workloads
  • Ensures evaluation slot tracking remains accurate (DB state matches actual running processes)
  • Guarantees safe cleanup of worktrees and temporary directories
  • No behavior change on successful execution path

Fixes a critical ingestion reliability issue and improves overall system stability under failure conditions.

Signed-off-by: WHOIM1205 <rathourprateek8@gmail.com>
@fricklerhandwerk

Copy link
Copy Markdown
Collaborator

I think all this does is terminate nix-eval-jobs early if our ingestion code has a crash; "critical" and "severe" are big words. Please stop spamming with your LLM, and instead put it to good use and make it help you produce a test that demonstrates that the change does what it promises. Without automated tests I have no reliable way of validating this does what it promises.

@fricklerhandwerk fricklerhandwerk 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.

Needs a test

@WHOIM1205

Copy link
Copy Markdown
Contributor Author

HEY @fricklerhandwerk
added tests for this case

covers failure during ingestion and ensures the subprocess is cleaned up properly

let me know if anything else should be added

Comment thread src/shared/tests/test_nix_evaluation.py Outdated
return proc


def _setup_mocks():

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.

All these helper procedures should be proper fixtures. This is both more readable (idiomatic, shows up in test arguments as dependencies) and easier to work with (naturally composes). For instance, returning a tuple is cumbersome to extend in the future. With fixtures we'd just add another fixture.

@fricklerhandwerk

Copy link
Copy Markdown
Collaborator

Now this is an interesting build failure to fix: https://github.com/NixOS/nix-security-tracker/actions/runs/23434730709/job/68187732968?pr=908#step:5:4175

It may amount to adding a package to the Nix expression, but please make sure this builds before pushing more commits.

Use AsyncMock for perform_evaluation and realtime_batch_process_attributes
so await resolves correctly. Convert helpers to pytest fixtures and use
arefresh_from_db in async context.

Signed-off-by: WHOIM1205 <rathourprateek8@gmail.com>
@WHOIM1205

Copy link
Copy Markdown
Contributor Author

hey @fricklerhandwerk
pushed an update

fixed the async mocks and moved the helpers into fixtures

should be passing now

@fricklerhandwerk

fricklerhandwerk commented Mar 24, 2026

Copy link
Copy Markdown
Collaborator

please make sure this builds before pushing more commits.

CI does nothing more but nix-build -A tests, please verify this locally before pushing commits.

@fricklerhandwerk
fricklerhandwerk marked this pull request as draft March 24, 2026 11:10
@fricklerhandwerk

Copy link
Copy Markdown
Collaborator

With #909 merged we can reuse quite a bit of the test setup, but you'd need to rebase. This sync/async stuff is a bit messy to get right, but we'll get there. It's going in the right direction already.

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