Fix/pty command execution - #175
Open
kartikloops wants to merge 11 commits into
Open
Conversation
OpenROAD runs a readline-style line editor when driven through a PTY. At the hardcoded 80 columns that editor horizontally scrolls any command longer than the window, redrawing the whole visible line once per character: a single 75-character read_liberty came back as a cascade of near-identical lines that buried the real output. Widen the terminal well past any realistic command and ask for a plain TERM so the editor skips the cursor-addressing layer we cannot interpret anyway. The terminal variables were also applied after the caller's env, silently overriding it, so create_interactive_session's env parameter could never set TERM, COLUMNS or LINES. Apply the defaults first and let the caller win. Also adds PTY_LINE_TERMINATOR, used by the session layer in the next commit.
…e window readOutput decided a command had finished once output went quiet for 100ms. The PTY echoes the command back instantly, so that window closed while OpenROAD was still working: read_db on a 15MB checkpoint and a 56s repair_timing both returned the bare echo with error:null, indistinguishable from success. timeout_ms was inert too, because the inner break abandoned the whole loop on the first quiet moment rather than running to the deadline. Write a nonce marker after each command and read until it appears. The Tcl assembles the marker at runtime via join, so the literal never occurs in the echoed input line and a match can only come from real stdout. Consequences: timeout_ms now bounds the wait and reports CommandTimeout instead of empty success, a process that dies mid-command reports SessionTerminated, and execution_time is the command's real duration rather than a flat 0.101s for everything. Commands are also terminated with CR instead of LF. A line editor in raw mode binds CR to accept-line; sending LF can leave the command sitting unsubmitted in the edit buffer, which is how ten commands piled up without one executing. readOutput is kept for buffer sampling and documented as unable to detect completion, with a characterisation test pinning that behaviour so nobody routes commands back through it. Output accumulation is capped at the buffer size, keeping the tail, so a verbose repair_timing cannot grow the heap.
An OpenROAD process can come up looking perfectly healthy -- alive, prompt printed, responding to inspect -- while its line editor never accepts the lines we write. Every command then silently does nothing. In one 23-minute run both sessions reported ten successful commands with 00:00:00 of CPU and RSS pinned at the startup footprint; nothing had been loaded or computed. createSession now runs a probe command through the sentinel path and fails if it does not come back, so this turns into an obvious error within seconds of session creation instead of a run's worth of results that were never real. The probe is stripped from the audit trail afterwards. The failure path also terminates the session. start() may already have spawned a process, and previously a rejected creation dropped it from the registry while leaving the OpenROAD process orphaned.
list_report_images returned total_images:0 for a directory holding five real images. save_images does not name them consistently: this build wrote final_all.webp.png and final_routing.webp.png, PNGs carrying a doubled extension, and the filter only matched a trailing ".webp". Accept .webp.png, .webp and .png, and strip the doubled form before classification so final_all.webp.png still resolves to complete_design rather than falling through to unknown. read_report_image accepts the same set. An empty listing now says which case it is -- no images generated, or images present under names that did not match -- because a bare zero with error:null leaves the caller unable to tell the run from the tool. That ambiguity is what sent the last run off to read the files straight off disk instead.
The startup probe could be satisfied by the PTY's echo of its own source line, so a session that echoes every line but executes nothing -- the exact failure the handshake exists to catch -- was accepted as healthy. Assemble the probe token at runtime, as the completion sentinel already does, so only real stdout can match it. Also normalise embedded newlines, not just the trailing one, to CR: an inner LF strands everything after it in the line editor's buffer for the same reason a trailing LF did. And let a timeout outrank an error matched in the partial output, so a command that printed "Error:" and then kept running is not reported as finished.
…atch metadata.format was hardcoded to webp even for plain PNG/webp.png files, and the unrecognized-file hint regex matched extension substrings anywhere in the filename instead of at the end.
…hake createSession ran session.start() and verifyResponsive() -- which can take up to a 15s (now 60s) timeout on a slow cold start -- entirely inside cleanupLock, so one slow or stuck session stalled every other create/list/ terminate call sharing the lock. Reserve the session id under the lock, release it, spawn and verify outside the lock, then take the lock again to publish the result. Reservations (sessions still mapped to null) now count towards maxSessions alongside active ones, since concurrent creates would otherwise all see the same pre-spawn total and admit past the cap together.
…rding them cols/rows used Number(x) || default, so an explicit caller override of "0" (or any falsy Number() result) fell through to the hardcoded default instead of being honored, even though the surrounding code exists specifically to let caller-supplied env win. Validate for a positive integer instead of relying on JS falsiness.
Sessions failed for anyone launching an MCP client whose PATH does not already include openroad (common with GUI-launched clients), since the server only ever spawned by name and had no fallback. applyInheritedEnv now runs before settings/logging init: if openroad is not already reachable, it merges in the login-shell PATH and common install locations (/opt/homebrew/bin, conda, local OpenROAD builds), without overriding an explicit PATH that already resolves it. Updates README, CROSS_PLATFORM, and SECURITY docs to match, and drops the old "find your paths and edit .env" setup section since the common case no longer needs it.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #175 +/- ##
===========================================
+ Coverage 67.53% 89.39% +21.86%
===========================================
Files 22 22
Lines 1611 1660 +49
Branches 0 381 +381
===========================================
+ Hits 1088 1484 +396
+ Misses 523 150 -373
- Partials 0 26 +26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
The sentinel-based session rewrite replaced manager.ts's sendCommand + readOutput call pair with a single session.runCommand(), but the benchmarks/memory_monitoring mock sessions still only implemented the old methods, so every executeCommand call in those suites threw "runCommand is not a function" in CI's Docker test stage.
repair_timing returned [ERROR RSZ-0089] with error: null because the detector only recognised "Error:" / "ERROR:" with a colon. OpenROAD's native log format has no colon, so a hard tool failure was reported as success. Match [ERROR …] and [FATAL …] before the generic rules, and leave [WARNING …] as a non-error.
luarss
reviewed
Aug 18, 2026
| // spawns, so OpenROAD still has to load and reach its prompt inside this | ||
| // budget. A container on a cold filesystem is comfortably slower than the | ||
| // handshake's own default. | ||
| private readonly handshakeTimeoutMs: number = SESSION_HANDSHAKE_TIMEOUT_MS; |
Collaborator
There was a problem hiding this comment.
why do we need this again? is this needed only at the start or needed throughout the whole session lifecycle?
also - it sounds like a perf issue if it doesn't start up in time? normal openroad starts should be quite instantaneous because it is written in C++, so it shouldn't be the bottleneck. Might be worth investigating where this regression comes from
Collaborator
Author
There was a problem hiding this comment.
Only at create time verifyResponsive() runs once before we return a session id. Normal commands use OPENROAD_COMMAND_TIMEOUT (30s). The 60s budget is a fail-safe for cold Docker/NFS starts and for detecting sessions that spawn but never accept input (we hit this in prod: alive process, zero CPU, commands echoed but not executed). Happy path is still sub-second locally; we’re not expecting 60s startups. The recent manager change moved handshake outside the lock so a slow create no longer blocks other sessions.
Collaborator
There was a problem hiding this comment.
Can you list some steps to reproduce it? This seems like quite a big concern for developer experience, if the first startup even requires 60s.
i know heavier mcp servers like playwright/chrome don't even require 60s to start on smaller VMs, so this should not be the case
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
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.
This branch fixes a set of real reliability bugs surfaced by dogfooding OpenROAD sessions: PTY command execution was broken by a fixed 80-column terminal that caused readline to redraw/scroll instead of returning clean output, and by terminal env defaults silently overriding caller-supplied TERM/COLUMNS/LINES (including a residual bug where an explicit COLUMNS=0 override was discarded); command completion detection was rewritten from a silence-window heuristic to an explicit runtime-generated sentinel marker, closing a false-positive where the PTY's own echo of the probe command could satisfy the startup handshake and a slow-but-healthy start could get misreported as unresponsive; createSession no longer holds the manager's global lock through the multi-second spawn-and-verify handshake, using a reserve-then-publish pattern so one stuck session can't stall every other session-manager call; report-image handling now accepts the doubled .webp.png extension some ORFS builds emit, reports the true image format instead of hardcoding webp, and anchors its "unrecognized file" hint regex to real extensions; and the server now auto-detects PATH/ORFS_FLOW_PATH at startup (merging login-shell PATH and common install locations when openroad isn't already reachable) so GUI-launched MCP clients no longer require manual .env setup, with docs updated to match.