[binaryaaron]

📋 Summary

A child forked after the parent has run DataDesigner hangs on its first preview() or create(), because it inherits background event loops whose threads did not survive the fork. This PR adds os.register_at_fork(after_in_child=...) hooks that drop the inherited engine and MCP loops, so the child starts fresh ones. The OpenTelemetry metrics server has the same problem and is fixed separately in #1070.

🔗 Related Issue

Fixes #1068

🔄 Changes

• async_concurrency.py: the hook clears _loop and _thread and replaces _lock, which also covers a fork while _lock is held during loop startup. (The test holds it from the forking thread; the child's copy is locked either way.)
• mcp/io.py: MCPIOService._reset_after_fork() does the same for the MCP loop, then clears sessions, in-flight tasks and the tools cache via _reset_state(), since they are bound to the dead loop.
• Registrations are guarded by hasattr(os, "register_at_fork") (absent on Windows).
• architecture/mcp.md, architecture/dataset-builders.md: one line each on fork behavior.

The hooks drop the inherited loop rather than close() it: parent and child share one epoll instance, and closing the child's copy unregisters the parent loop's wakeup pipe. In a probe, the parent's next run_coroutine_threadsafe(...).result(3) timed out after a child closed its copy.

spawn and forkserver children are unaffected. No public API changes. A side effect: a forked child that exits through sys.exit no longer spends up to 10 s in the MCP atexit shutdown waiting on the dead loop.

Residual risk: the child does not close inherited MCP stdio pipes, because they belong to transports bound to the dead loop. A long-lived child can delay end-of-file to an MCP server the parent shuts down.

🧪 Testing

• make test passes: one pre-existing failure, see below
• Unit tests added/updated
• E2E tests added/updated (if applicable): N/A, no model endpoint needed

Fork tests run each scenario in a fresh interpreter (tests/engine/fork_probe.py) instead of forking the pytest worker. Forking the worker copied whatever earlier tests left behind, including DuckDB thread pools and memory reservations sized from the machine's CPU and memory limits. That is the likely cause of the first CI run's ENOMEM in fork().

• test_fork_safety.py: the Forked child process hangs on preview()/create() after the parent has run DataDesigner #1068 reproduction end to end, preview() in a child forked after a parent preview().
• test_async_concurrency.py: engine loop after parent use, and with _lock held at fork.
• test_mcp_io.py: MCP loop after parent use.

All four fail on main and pass with this change. make test (CPython 3.14.0): one failure, test_create_dataset_e2e_with_file_contents_seed_source_unreadable_file_raises_generation_error, which also fails on unmodified main when run as root (chmod(0) does not block root's reads). Ruff check and format pass.

✅ Checklist

• Follows commit message conventions
• Commits are signed off (DCO)
• If I am an external contributor, the linked issue is open and labeled triaged: N/A
• Architecture docs updated (if applicable)

🤖 Generated with Claude Code

[github-actions]

All contributors have signed the DCO ✍️ ✅
_{Posted by the DCO Assistant Lite bot.}

[github-actions]

Fern preview: https://nvidia-preview-pr-1069.docs.buildwithfern.com/nemo/datadesigner

Fern previews include the docs-website version archive with PR changes synced into latest. Notebook tutorials are rendered without execution outputs in previews.

[github-actions]

Thanks for the careful fix and the detailed write-up, @binaryaaron. The epoll note about why the hooks drop the loop instead of closing it is especially useful.

Summary

This PR registers os.register_at_fork(after_in_child=...) hooks for the three process-wide background resources: the AsyncEngine loop, the MCP I/O loop and the OpenTelemetry metrics server. A forked child then starts fresh ones instead of blocking on threads that did not survive fork(). The code does what the PR description says. I checked locally on CPython 3.14.3:

• The 98 tests in the three touched test modules pass.
• Ruff check and format pass.
• A probe matching the PR's reasoning confirmed that dropping the inherited loop in the child leaves the parent loop working, including a subprocess pipe it had registered.

Findings

Warnings — Worth addressing

packages/data-designer/src/data_designer/integrations/opentelemetry.py:283 — Forked child keeps the parent's metrics listening socket open

• What: _reset_after_fork() sets self._server = None, but the child process still has its inherited copy of the ThreadingHTTPServer listening socket's file descriptor. The comment says the socket "belongs to the parent", but after fork() both processes hold it. Nothing in the child ever closes it, and the child also binds its own server on the next run.

• Why: I reproduced this on this branch. The parent ran one observe_create(port) and forked a child that stays alive. The parent then called runtime.shutdown() and then observe_create(port) again:

  • A scrape of the old port timed out instead of getting connection refused. The child's socket copy kept the port in LISTEN, so the kernel queued the connection and nobody accepted it.
  • The second observe_create(port) could not rebind the same port, so metrics_url was None. Metrics are silently unavailable for the parent's later runs.

  This matters for the use case the PR targets: long-lived forked workers (a multiprocessing pool, gunicorn-style pre-fork) would hold the parent's metrics port for as long as they live.

• Suggestion: Close only the child's copy of the socket. server_close() just closes the socket. It does not call shutdown(), so it does not wait on serve_forever(). Closing a descriptor copy in the child does not affect the parent's socket. With this change my probe got connection refused after the parent shut down, and the rebind to the same port worked:

  server = self._server
  self._server = None
  self._server_thread = None
  self._port = None
  if server is not None:
      with contextlib.suppress(Exception):
          server.server_close()

  A regression assertion could be added to test_forked_child_does_not_wait_on_parent_metrics_server. Keep the child alive briefly, call runtime.shutdown() in the parent, then assert that observe_create(<same port>) binds again (metrics_url is not None).

Suggestions — Take it or leave it

packages/data-designer-engine/tests/engine/dataset_builders/utils/test_async_concurrency.py:30, tests/engine/mcp/test_mcp_io.py:1307, tests/integrations/test_opentelemetry.py:1017 — Same fork-and-join harness in three places

• What: Each test repeats the same steps: get the fork context, start the child, join(timeout=20), kill it if still alive and pytest.fail, then assert the exit code. The MCP and OTel tests hard-code 20, while the engine test uses _CHILD_JOIN_TIMEOUT_S.
• Why: There are three copies across two packages, so the timeout and failure messages will drift over time.
• Suggestion: Optional. At minimum, use one named timeout constant in each file. If more fork-safety tests are planned, a small shared helper (or fixture) that runs a target in a forked child and returns its exit code would remove the repetition. Note that it would have to live where both packages' tests can import it.

What Looks Good

• Drop, don't close. The PR verified that closing the child's copy of the loop unregisters the parent loop's wakeup pipe from the epoll instance they share, and it explains this in the code comments and in architecture/mcp.md. That is easy to get wrong, and the comments make sure the next person doesn't "fix" it.
• The lock is replaced, not just the loop. Creating a new _lock / _loop_lock / _lock (RLock) also handles a fork that happens while another thread holds the lock during loop startup. test_forked_child_does_not_inherit_held_loop_lock pins this down directly.
• The tests show real behavior. Each one runs a real forked child with a bounded join, checks that the child's work actually ran, and then checks that the parent's loop or server still works afterwards. They are skipped cleanly where fork is unavailable, and the hasattr(os, "register_at_fork") guards keep Windows working.

Residual Risk

• Other state still tied to the parent's loop. Only the loops themselves are reset. Any other object the parent tied to its old engine loop and that the child reuses would still point at the dead loop. Examples are an httpx.AsyncClient held by an HttpModelClient in a reused model registry, and asyncio locks or semaphores. The usual flow builds these per create(), and the regression tests cover that flow, so I'm not flagging it. But it is a limit of the fix if users fork after building a DataDesigner and then reuse it in the child.
• Child metrics start from the parent's totals. The OTel meter provider and registry are kept in the child, so a child's metrics endpoint starts from the parent's accumulated counter and histogram values. This is probably fine (the PR says the provider and registry are kept), but it may surprise someone scraping per-worker metrics.

Structural Impact

The one reported import-direction violation (request_event_sink_accepts() → .accepts_request_event()) is an inferred duck-typed call that already exists on main. It is not introduced by this PR, and the engine does not import the interface package. The added hooks only touch module-private state in their own modules, so I don't see any blast-radius or backward-compatibility concern for the high-connectivity nodes listed below.

Structural Impact (graphify 0.9.90, 8.2s)

Risk: HIGH (1 import direction violation(s))

• 6 Python files, 122 AST entities, 5/187 clusters

Import Direction Violations (1)

Legal direction: interface -> engine -> config

• request_event_sink_accepts() (engine) --indirect_call--> .accepts_request_event() (interface)

High-Connectivity Changes

• io.py (51 deps) in packages/data-designer-engine/src/data_designer/engine/mcp/io.py
• MCPIOService (35 deps) in packages/data-designer-engine/src/data_designer/engine/mcp/io.py
• OpenTelemetryRuntime (29 deps) in packages/data-designer/src/data_designer/integrations/opentelemetry.py
• opentelemetry.py (22 deps) in packages/data-designer/src/data_designer/integrations/opentelemetry.py
• _coerce_tool_result_content() (13 deps) in packages/data-designer-engine/src/data_designer/engine/mcp/io.py
• _coerce_tool_result_content_item() (13 deps) in packages/data-designer-engine/src/data_designer/engine/mcp/io.py
• ensure_async_engine_loop() (11 deps) in packages/data-designer-engine/src/data_designer/engine/dataset_builders/utils/async_concurrency.py
• async_concurrency.py (10 deps) in packages/data-designer-engine/src/data_designer/engine/dataset_builders/utils/async_concurrency.py
• +35 more

Cross-Package Dependencies

• OpenTelemetryRuntime (interface) --uses--> RequestAdmissionEvent (engine)
• OpenTelemetryRuntime (interface) --uses--> RuntimeCorrelation (engine)
• OpenTelemetryRuntime (interface) --uses--> SchedulerAdmissionEvent (engine)
• .observe_create() (interface) --calls--> RuntimeCorrelation (engine)
• .emit_scheduler_event() (interface) --references--> SchedulerAdmissionEvent (engine)
• .emit_request_event() (interface) --references--> RequestAdmissionEvent (engine)
• +8 more

Verdict

Needs changes. There is one item before merge:

• Close the child's inherited copy of the metrics server's listening socket in OpenTelemetryRuntime._reset_after_fork(), so a long-lived forked child doesn't keep the parent's metrics port bound. A one-line regression check in the existing fork test would cover it.

Everything else looks solid, and the loop-reset approach is right.

---

This review was generated by an AI assistant.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issues were found.

Summary

This PR lets forked children start fresh engine and MCP background loops.

• Forked children start fresh engine and MCP event loops.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Parent uses background loops] --> B[Parent forks]
    B --> C[Parent keeps its loops]
    B --> D[Child hooks run]
    D --> E[Forget inherited loops and replace locks]
    E --> F[Clear copied MCP sessions and caches]
    F --> G[First child call starts fresh loops]

Loading

_{Reviews (2) · Last reviewed commit: "test: preview() in a child forked after ..." · Reviewed by Greptile}

[greptile-apps]

Child counts vanished requests

If the process forks while another thread has an active model request, the child clears _active_request_attempts but keeps _active_model_requests and its meter provider. The copied counter still includes the parent's request, but completion and run cleanup can no longer find the entry needed to subtract it. The child's endpoint can therefore report active requests after all child work finishes.

Reset the child's active-request measurements together with their bookkeeping, and test a fork during an active request.

Knowledge Base Used: Observability instrumentation

Prompt To Fix With AI

This is a comment left during a code review.
Path: packages/data-designer/src/data_designer/integrations/opentelemetry.py
Line: 291

Comment:
**Child counts vanished requests**

If the process forks while another thread has an active model request, the child clears `_active_request_attempts` but keeps `_active_model_requests` and its meter provider. The copied counter still includes the parent's request, but completion and run cleanup can no longer find the entry needed to subtract it. The child's endpoint can therefore report active requests after all child work finishes.

Reset the child's active-request measurements together with their bookkeeping, and test a fork during an active request.

**Knowledge Base Used:** [Observability instrumentation](https://app.greptile.com/nvidia-public-github/-/custom-context/knowledge-base/nvidia-nemo/datadesigner/-/docs/observability-instrumentation.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[binaryaaron]

Fixed in #1070: the child now discards the inherited meter provider and instruments, so data_designer.model.request.active starts from zero in the child.

[github-actions]

Issue #1068 received the triaged label. The linked issue check is being re-evaluated.

[nabinchha]

Thanks for the focused fork-safety fix and regression tests, @binaryaaron.

Summary

The engine and MCP hooks restart background loops after a fork, matching the PR's intent. The metrics hook still retains SDK state that can deadlock a child. Reviewed commit 26ca25ef81548c0d04f4490bf1fdf0b1875491e2.

Validation: all 98 tests in the three affected test modules pass on CPython 3.13.3; Ruff lint and formatting checks pass for all six changed Python files.

Findings

Critical — Let's fix these before merge

packages/data-designer/src/data_designer/integrations/opentelemetry.py:287 — [P2] Recreate the metrics SDK state in forked children

• What: The hook replaces the runtime's lock and server but retains _initialized, _meter_provider, the instruments and registry. Those objects contain additional locks used by the parent's metrics collection and recording threads. If another thread holds one when the process forks, its child copy stays locked permanently.
• Why: I reproduced this with the singleton runtime initialized in the parent and its create-duration histogram aggregation lock held by another thread at fork. The child entered observe_create() and bound a fresh server, but scraping timed out and exiting the context hung in _record_create_duration(). Releasing the lock in the parent did not unblock the child, which needed termination. The same probe without the held lock exited successfully, and the parent's endpoint remained healthy. Holding the SDK collection lock instead caused child scrapes to time out even though the create context completed. The existing fork test only forks while these SDK locks are idle.
• Suggestion: Discard inherited SDK/provider/registry/instrument state as well, and initialize fresh metrics state lazily in the child. Avoid shutting down the inherited provider through potentially locked SDK methods, and remove the inherited metric log handler before registering its replacement. Add a bounded fork regression with a metrics lock held by another thread that verifies both child context completion and a successful child scrape.

What Looks Good

• Replacing loop locks handles forks during loop startup, with a direct engine regression test.
• Real fork tests verify child execution and continued parent operation with bounded waits.
• The architecture notes explain why inherited event loops must not be closed.

Residual Risk

The inherited listening-socket issue and stale active-request measurements already have feedback in the existing review and inline thread. They are not duplicated as new findings here. Validation covered the affected modules and targeted probes, rather than the full suite.

Verdict

Needs changes. Reset the retained metrics SDK state and cover a fork while a metrics lock is held.

---

This review was generated by an AI assistant.

[binaryaaron]

@nabinchha Agreed, and reproduced: holding the create-duration histogram's aggregation lock at fork hangs the child on its first record. I've moved the OpenTelemetry fix out of this PR into #1070. There the child drops all inherited SDK state without calling into it and initializes fresh state on its next run, and a fresh-interpreter regression test holds those locks at fork. This PR is now the engine and MCP loops only.

[binaryaaron]

The socket finding is fixed in #1070: the child closes its copy of the listening socket, and a test checks that the parent can rebind its port while a forked child is still alive. The duplicated harness is gone here: the engine fork tests share tests/engine/fork_probe.py and run in a fresh interpreter.

The ENOMEM on Ubuntu/3.12 most likely came from forking the pytest worker after earlier tests had grown it: DuckDB sizes its thread pool and memory reservation from the available CPUs and memory, and the worker accumulates both across the suite. On my machine it had grown to well over a thousand threads and several reservations of 80% of the memory limit by the time the fork tests ran. I couldn't inspect the runner itself. The fork tests now run in a fresh interpreter. The macOS failures were all in the OpenTelemetry test, which moved to #1070.

[binaryaaron]

I have read the DCO document and I hereby sign the DCO.

[nabinchha]

Thanks for separating the OpenTelemetry work and strengthening the fork regressions, @binaryaaron.

Summary

Reviewed commit 27296f4554044b2fa1114704401037ca2fb3f612 against base 9a09fce628be7c0af5de91e68a6cd402c3d3df72. PR #1069 now resets only the engine and MCP loops, matching its revised description. The OpenTelemetry change that triggered my earlier request for changes is no longer in this PR; it is tracked separately in #1070.

Findings

No new findings. My previous blocking finding no longer applies to the current diff. Registering _MCP_IO_SERVICE._reset_after_fork is appropriate internal use: the callback is wired up inside the same module that defines and owns the singleton, so the private method does not become a public API dependency.

What Looks Good

• Fresh-interpreter fork probes avoid inheriting unrelated pytest-worker state while preserving bounded failure handling and parent-loop checks.
• The new interface regression exercises the reported failure through actual parent and child DataDesigner.preview() calls.
• The production hooks still replace inherited loop locks and clear MCP session/cache state without closing the parent's shared event-loop selector registrations.

Residual Risk

All 67 tests in the three affected test modules pass locally on CPython 3.13.3, and Ruff lint and formatting checks pass for all six changed Python files. The full suite was not rerun. Inherited MCP stdio descriptors remain a documented limitation of the approach. This follow-up does not review or approve the separate OpenTelemetry implementation in #1070.

Verdict

Ship it. No outstanding findings for the revised scope of PR #1069.

---

This review was generated by an AI assistant.