Skip to content

viewer: add worker and cold process timeouts and stream large assets - #305

Merged
earthtojake merged 5 commits into
earthtojake:developfrom
warun7:fix/viewer-worker-deadlock-and-timeouts
Aug 26, 2026
Merged

viewer: add worker and cold process timeouts and stream large assets#305
earthtojake merged 5 commits into
earthtojake:developfrom
warun7:fix/viewer-worker-deadlock-and-timeouts

Conversation

@warun7

@warun7 warun7 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Prevents permanent server lockups on hung worker/subprocess calls and prevents high memory usage on large file transfers:

  1. Warm Worker Request Timeout & Deadlock Prevention:

    • Implemented _read_line with bounded timeout (VIEWER_CADGEN_TIMEOUT, default 300s; VIEWER_CAD_WORKER_PING_TIMEOUT, default 10s) in worker_client.py.
    • When a worker request times out or faults, _WorkerTransportError is raised, triggering process termination (_reap()) and a single transparent respawn. Subsequent requests no longer block forever on self._lock.
  2. Cold Subprocess Execution Timeout:

    • Added timeout handling (subprocess.TimeoutExpired) to cadgen_bridge.py:run_cadgen_cold, returning {ok: false, error: "cadgen <module> timed out after <N>s"} rather than hanging indefinitely.
  3. Chunked Asset & Static File Streaming:

    • Updated _serve_static_file and _serve_asset in server.py to stream files in 64 KiB chunks using shutil.copyfileobj instead of buffering whole multi-GB files in memory via handle.read().

Testing

  • Added viewer/server_py/tests/test_worker_timeouts.py:
    • test_worker_read_line_timeout_raises_transport_error
    • test_worker_request_timeout_reaps_and_recovers
    • test_cadgen_bridge_cold_subprocess_timeout
    • test_stream_file_serves_chunks
  • All 180 Python tests in viewer/server_py/tests/ passed.
  • All 364 JavaScript tests in viewer/ passed.
  • Verified viewer self-containment check.

- Add bounded timeout to worker JSON-RPC readline calls so hung workers raise transport errors, get reaped, and cleanly respawn rather than blocking all builds under the worker lock.
- Add timeout support and TimeoutExpired error handling to cold cadgen subprocess execution in cadgen_bridge.
- Stream static files and assets in 64 KiB chunks via copyfileobj instead of buffering entire files in memory.
- Add unit tests for worker timeout recovery, cold subprocess timeouts, and chunked streaming.
@earthtojake

Copy link
Copy Markdown
Owner

Sorry for the delayed response, and thanks for the contributions so far.

The streaming work is good and stands alone. 64 KiB chunks with an explicit
content-length beats loading whole GLBs into memory, and nothing about it depends on
the timeout question. Split it into its own PR and it can land right away.

The timeouts have a problem no constant can fix. Real builds run long: a mid-size
robot regenerates for around 30 seconds, and the generation runner's own comments
call out multi-minute gen_step() runs. The first open in the viewer is exactly
when that full cost lands. There is also no cross-run cache, so a kill at 300s does
worse than delay the build: the retry restarts from zero and dies at the same wall,
and a slow model becomes one the viewer can never build. Killing a warm worker also
throws away its paid OCP import. Anyone who raises VIEWER_CADGEN_TIMEOUT to cover
their largest model has switched the guard off, which says wall-clock time is the
wrong signal.

The signal that separates hung from slow is progress. Builds already write it
(.generation.progress.json feeds the viewer's progress UI, and the cold path
streams progress lines), so an idle watchdog fits here: kill after N seconds of
silence rather than N seconds of work. A hung process goes quiet and gets caught in
seconds, while a slow build keeps reporting and lives. The 10s ping timeout is fine
as-is, since a ping is bounded work.

One architecture note: worker_client.py is deleted on release/0.5.0 when the
viewer joins the shared warm daemon pool, so timeout work built there gets discarded
at the rebase. The cold-path bridge survives the move. That's another reason to land
streaming separately and keep the timeout redesign small.

If you want to chat further, message me on Discord: https://discord.gg/5FGB9DwJYU

… clock

Review rework of the timeout half. A wall-clock cap is the wrong signal for
cadgen: real builds run tens of seconds to minutes, their first open pays the
full cost, and with no cross-run cache a killed build restarts from zero and
dies at the same wall -- so anyone who raises the cap to fit their largest
model has switched the guard off.

The signal that separates hung from slow is output. worker.py routes all build
narration (phase lines, progress chatter, C-level prints) to stderr; the client
now always drains that pipe -- which it must anyway, since an undrained pipe
fills and blocks the worker mid-write, a self-inflicted hang -- and every line
stamps a liveness time. The response wait slides its deadline while narration
arrives and declares the worker dead only after a full budget of total silence
(VIEWER_CADGEN_IDLE_TIMEOUT, default 300s, <= 0 disables). The cold subprocess
gets the same rule by swapping capture_output for pumped pipes.

The ping keeps its short bounded budget; a transport fault still reaps and
respawns once before failing the request, so the module-global lock can no
longer wedge forever on one hung read.
@warun7

warun7 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Reworked per review (dbeea4f), and the streaming half is split out into #333 so it can land on its own.

Timeouts are now an idle watchdog, not a wall clock:

  • worker.py already routes all build narration to stderr; the client now always drains that pipe (an undrained stderr pipe was also a latent hang: a chatty build fills the 64 KiB buffer and blocks the worker mid-write) and every line stamps a liveness time.
  • The response wait slides its deadline while narration arrives and declares the worker dead only after VIEWER_CADGEN_IDLE_TIMEOUT (default 300s, <=0 disables) of total silence. A hung readline is caught at one quiet budget; a build narrating phases waits as long as it needs. Ping keeps its short bounded budget.
  • The cold subprocess got the same rule by swapping capture_output for pumped pipes, killing on the same silence budget (run_cadgen_cold survives the 0.5.0 daemon move, as you noted).
  • Transport faults still reap + respawn once before failing the request, so the module-global lock can't wedge forever.

Tests: silent worker → dead at the budget; narrating worker that outlives a wall-clock cap → survives; drainer freshness pinned; same pair for the cold path. Mutation-checked both directions.

The os.pipe/fdopen harness never delivered the line to the drainer on
Windows; a subprocess producer is the shape production uses anyway.
@earthtojake

Copy link
Copy Markdown
Owner

The idle-watchdog redesign is right, and I verified the hard part against a real
build: a 9.8s OCP artifact build (planetary gear, forced) ran under the watchdog,
narrated, and returned ok=True. The sliding deadline, the pipe drain, and the
kill-on-silence all behave as the tests describe.

One bug blocks the merge: the rewrite of run_cadgen_cold dropped the function's
final return. The last line computes message and then the function ends, so every
subprocess that exits without printing a JSON line returns None instead of an
error dict. develop's version ends with:

return {"ok": False, "exitCode": proc.returncode, "error": message}

That path is routine, and the caller crashes on it. Reproduction:

from server_py import cadgen_bridge
result = cadgen_bridge.run_cadgen_cold(
    "cadgen.step_artifact_cli",
    ["--repo-root", root, "--source-path", target],   # missing required --step
    root,
)
# result is None; develop returns {"ok": False, "error": "...required: --step..."}

On develop the argparse message reaches the client as a build error. On this branch
result.get(...) raises AttributeError in backend._run_artifact_build and the
route 500s.

The tests miss it because each one either times out (early return) or prints valid
JSON. Two asks:

  1. Restore the terminal return.
  2. Add a test where the subprocess exits without a JSON line and assert the error
    dict comes back, so the path stays covered.

Everything else is ready; with those two this merges.

The idle-watchdog rewrite dropped the function's final return, so every
subprocess that exited without printing a JSON line -- argparse usage errors
are the routine case -- returned None and crashed the caller with
AttributeError instead of reporting the build error. The terminal return is
back, pinned by a test where the subprocess exits with a usage message and no
JSON.
@warun7

warun7 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Both asks done in 24e9d28:

  1. Terminal return {"ok": False, "exitCode": ..., "error": message} restored — good catch, that was a straight-up drop in the rewrite.
  2. New test: a subprocess that exits with a usage message and no JSON line must return the error dict (asserts not-None, ok false, exit code, and the message content). Mutation-verified: removing the return fails the test.

@earthtojake
earthtojake merged commit c7e2a7c into earthtojake:develop Aug 26, 2026
3 checks passed
earthtojake pushed a commit that referenced this pull request Aug 26, 2026
Source ref: develop
Source commit: cce04de
Target branch: main
Previous target: 8f9a7d7
Release base: 8f9a7d7
Previous source: 96675ba

Included commits since previous source:
cce04de Merge pull request #337 from earthtojake/release/0.4.28
c3f3856 Release 0.4.28
c7e2a7c Merge pull request #305 from warun7/fix/viewer-worker-deadlock-and-timeouts
2b65d4f Merge branch 'develop' into fix/viewer-worker-deadlock-and-timeouts
6f0265d Merge pull request #335 from warun7/fix/skill-remediations-and-coverage
1e4aea1 Merge branch 'develop' into fix/skill-remediations-and-coverage
1f75ced Merge pull request #336 from earthtojake/claude/port-probe-bind
3236a5c viewer: probe port availability by binding, not connecting
99a806f tests: pick viewer-smoke ports outside the ephemeral range
5633b65 tests: call the module-level drain helper directly
788bb5d tests: retire a busy candidate port instead of failing the viewer smoke
7306fbe tests: skip the cadgen probe in the viewer start smoke, surface its output
603e812 tests: resolve npm through PATH for the viewer start smoke on Windows
0b64fa3 skills: point gcode at the real cad export CLI; cover cad-viewer; fix skill deps
24e9d28 viewer: restore run_cadgen_cold's terminal error return
3150457 tests: drive the stderr drainer from a real subprocess pipe
dbeea4f viewer: kill the CAD worker and cold subprocess on idleness, not wall clock
06bf1b3 viewer: add worker and cold process timeouts and stream large assets
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