mirror of
https://github.com/diodeinc/pcb.git
synced 2026-09-14 20:17:19 +08:00
Adam M Momen
ee4e7e2b90
* Add local FreeRouting auto-routing engine (pcb route --engine freerouting)
Routes boards locally via a FreeRouting Java JAR, as an alternative to
DeepPCB cloud routing. Squashed from the branch's original 29-commit
history (which included an earlier --remote-flag design, a REST-API
rewrite, and a revert back to a refined REST-API design) into one
commit for a clean rebase onto upstream/main.
- pcb route --engine freerouting|deeppcb (deeppcb remains default)
- Auto-downloads and SHA-256-verifies a pinned FreeRouting JAR
(also verifies a $PATH-discovered JAR; --fr-jar/FREEROUTING_JAR are
explicit overrides and skip verification)
- Drives FreeRouting's local REST API (not one-shot CLI mode) so
Ctrl+C and timeouts can recover partial routing progress, working
around a confirmed upstream bug where CLI mode's TIMED_OUT state
never reaches COMPLETED
- All work happens on a temp copy of the board + project file; the
original is only overwritten once a validated result is ready
- Requires Java 21+ on $PATH; does not auto-manage a JRE/JDK
- Test sandbox now exempts loopback from its dead-proxy egress block,
needed for tests that spawn a local FreeRouting API server
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Keep pcb route hidden from top-level command list
Re-adds #[command(hide = true)] on Commands::Route, which was dropped
at some point. Avoids needing to regenerate the pcbc help snapshot,
and keeps route --engine freerouting unlisted while local FreeRouting
routing is still new.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Preserve cached partial output when losing contact with FreeRouting
poll_job hard-bailed after 20 consecutive get_job failures, discarding
last_known_output even though it held real progress cached from an
earlier RUNNING poll. A crashed or unreachable server lost that
progress entirely, unlike the cancel/timeout paths which already
return it. Return it here too (still treated as a Cancelled outcome
for execute() to publish) instead of only when we still have a live
connection to ask for it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Warn (non-fatally) when --fr-jar/FREEROUTING_JAR don't match the pinned hash
Both are explicit user overrides, so a mismatch isn't rejected outright
(may be an intentional dev/patched build), but running an unverified
JAR with zero visibility wasn't right either. Surface a warning instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Validate --fr-timeout range before dispatching to FreeRouting
fr_timeout had no bounds checking, unlike DeepPCB's timeout (rejected
above 60 min). Out-of-range values could panic (Instant + Duration
overflow for huge values) or produce a malformed job_timeout
(format_hms hour field >= 24). Reject anything outside 1..=3540
seconds (the documented 59-minute cap) up front with a clear error.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Align --fr-timeout cap with DeepPCB's 60-minute cap (3600s)
Was 3540s (59 min); use 3600s (60 min) to match DeepPCB's timeout cap.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Honor Ctrl+C outside the FreeRouting poll loop
CANCEL was only ever read inside poll_job, so pressing Ctrl+C during
export_dsn or import_ses (zone fill) printed "Stopping FreeRouting and
fetching the best result so far..." but the step ran to completion
anyway with no way to abort short of SIGKILL.
Check CANCEL after export_dsn (bail before starting FreeRouting at
all) and after import_ses (publish whatever was imported and stop
before opening KiCad), so the flag is honored at both phase
boundaries outside the poll loop too.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Shorten freerouting CHANGELOG entry to match other one-liners
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Trim unnecessary comments
Removed step-numbering and code-restating comments in
freerouting/mod.rs and tests/route.rs; shortened the FREEROUTING_VERSION
doc comment to its essential rationale. Kept comments explaining
non-obvious invariants, races, and upstream quirks.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Use reqwest instead of shelling out to curl in freerouting tests
resolve_freerouting_jar's download loop and wait_for_http_ok both
shelled out to curl, reinventing HTTP calls already made correctly
elsewhere in this same PR (download_to_file in freerouting/mod.rs,
FreeroutingApiClient::wait_ready in freerouting/api.rs) using reqwest,
already a dependency of this crate. The curl version was also
strictly worse for the JAR download: no retry/backoff and no hash
verification.
Left the curl_json helper in test_freerouting_cancel_returns_output
as-is — it deliberately drives the raw API via curl to test the wire
protocol independent of our own reqwest-based client, not an
oversight.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Replace raw \r pass-counter with an animated Spinner
The poll loop printed " pass N\r" via a bare print!/flush with no
line-clearing, which could leave stray/garbled characters behind
depending on terminal state (e.g. overlapping with a previous longer
line) and gave no visible feedback between pass updates otherwise.
Reuse the Spinner component already used elsewhere in this file
(Exporting DSN..., Starting FreeRouting...) instead of hand-rolling
terminal control: it animates continuously via indicatif, updates its
message with the current pass number, and clears/finishes cleanly
with a success or warning icon depending on whether the run completed
or was cancelled/timed out.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Remove redundant doc comment on JobOutput
"Result of fetching a job's output" just restates the enum's name;
its variants already carry their own explanatory docs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Scope the sandbox's loopback proxy bypass to the client that needs it
Replaces the blanket NO_PROXY=127.0.0.1,localhost exemption (which
widened every sandboxed command's "no accidental egress" guarantee to
"any loopback service is reachable") with the client-side .no_proxy()
opt-out FreeroutingApiClient already uses in production.
wait_for_http_ok now builds its own no_proxy() client instead of
relying on the ambient NO_PROXY var, matching that pattern, so the
sandbox's NO_PROXY can go back to "" (nothing depended on the
exemption: the sandbox env only applies to `sandbox.run("pcbc", ...)`
calls, and the only such call that reaches loopback already opts out
explicitly).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Simplify FreeRouting spinner message; hide pass count behind --debug
"Running FreeRouting (timeout: 300s)... pass 0" surfaces two pieces
of internal detail with no clear meaning to a user watching the CLI:
the configured timeout doesn't change moment-to-moment, and "pass N"
is FreeRouting's internal routing-iteration count, not something most
users can act on or interpret.
Drop the timeout from the spinner message entirely, and only append
the pass count when --debug is set (checked via the log crate's
active level, which env_logger already raises to debug when --debug
is passed) rather than showing it unconditionally.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix misleading "FreeRouting finished in Xs" message on timeout/cancel
Reported: running against a real board, the CLI printed "! FreeRouting
finished in 300.1s" (with a warning icon) immediately followed by "No
routing progress to save. Board left untouched." — "finished" reads as
success, contradicting the very next line saying nothing was produced.
The message didn't distinguish a genuine completion from a timeout/
cancel with zero output.
Split the message by outcome and whether any output was captured:
"finished in Xs" only for a real RunOutcome::Completed, "stopped after
Xs (partial result)" when cancelled/timed out but something was
salvaged, and "stopped after Xs with no routing progress" when
nothing was ever produced — so the message matches what actually
happened instead of always sounding like success.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Show elapsed time in the FreeRouting spinner while running
The spinner sat on a static "Running FreeRouting..." for the whole
run with no visible feedback beyond the animation itself. Update the
message every ~1s poll tick with elapsed seconds ("Running
FreeRouting... 45s elapsed"), regardless of whether the pass count
changed, so long stretches on one pass still show visible progress.
Under --debug, also appends the current pass number.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Show pass N/max as a rough progress signal in the spinner
FreeRouting's job settings already fix max_passes at 200 via
update_settings; extracted to a shared FREEROUTING_MAX_PASSES constant
so poll_job can show "pass N/200" alongside elapsed time instead of
just the bare pass number, giving a rough (FreeRouting can finish
earlier via its own improvement_threshold, so not exact) sense of how
close a run is to done. Shown by default now rather than gated behind
--debug, since it's cheap and more informative than a bare elapsed
timer.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Only show pass N/max once past 0; drop "Local" from the routing message
FreeRouting's documented API (docs/API/API_v1.md, confirmed against
context7's indexed copy) has no current_pass field at all in the
GET /v1/jobs/{jobId} schema — only max_passes as a config input under
router_settings, not a live counter. In practice a static "pass
0/200" was observed sitting unchanged for the whole run on some
boards (likely the undifferentiated initial routing stage, which
runs before any numbered optimization pass starts), which reads as
stuck/broken rather than as progress. Only display the pass fraction
once it's actually > 0 — elapsed time alone still updates every tick
either way, so the spinner never goes fully static.
Also drop "Local" from "Local routing X via FreeRouting": redundant
now that --engine freerouting is the only local engine and DeepPCB
already reads as the cloud option by contrast.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Bump Java version gate to 25 to match FreeRouting v2.2.4's requirement
FreeRouting v2.2.4's build.gradle targets JavaVersion.VERSION_25, so
the pinned jar fails to load (UnsupportedClassVersionError) on
anything older. resolve_java/is_java_21_plus only gated on Java 21+
and the error message told users to install OpenJDK 21, so a user
with Java 21-24 passed our prerequisite check and then hit a confusing
crash when FreeRouting's API server died at launch instead of a clear
"install Java 25+" error upfront.
Extracted REQUIRED_JAVA_VERSION as a named constant (kept in sync with
FREEROUTING_VERSION) instead of hardcoding 21 in three places, and
renamed is_java_21_plus to is_java_version_sufficient since it no
longer names a fixed version. Updated the duplicated check/messages in
tests/route.rs to match.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Show download percentage progress for the FreeRouting JAR
try_download_to_file previously used a single io::copy with no
visibility into a ~50-70MB download's progress beyond a static
"Downloading..." line. Stream the response in chunks against
Content-Length using the existing pcb_ui::ProgressBar component
(already used elsewhere in this file via Spinner) instead, falling
back to the old plain io::copy when the server doesn't report a
Content-Length (e.g. chunked encoding).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Human-readable byte sizes and a versioned message on the download bar
Use indicatif's {bytes}/{total_bytes} template placeholders (auto-
formats sizes like "2.3 MiB") instead of the default template's raw
{pos}/{len} byte counts. Also include the pinned version in the
message ("Downloading FreeRouting (2.2.4)"), matching how Homebrew
Cask labels its downloads.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix rustfmt violations and convert cancel-test from curl to reqwest
cargo fmt was failing CI on freerouting/api.rs, freerouting/mod.rs, and
route.rs. Also switches the cancel-job integration test off shelling out
to curl, matching the reqwest::blocking already used elsewhere in the
test file.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Trim explanatory comments in sandbox, route, and route tests
Cuts down verbose justification comments to the essentials, keeping
only the ones that carry genuinely non-obvious information (e.g. the
FreeRouting UUID header requirement, java -version writing to stderr).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Check Java before auto-downloading the FreeRouting JAR; make --fr-timeout minutes
Splits JAR resolution so the explicit --fr-jar/FREEROUTING_JAR path is
still validated immediately, but the PATH search and network
auto-download fall back only after Java is confirmed installed —
previously a user without Java would pay for the full JAR download
before being told the run can't proceed anyway, and it made
test_freerouting_java_not_found vacuous.
Also switches --fr-timeout to minutes (default 5, max 60), matching
--timeout's format and validation style instead of a seconds range.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Don't mislabel a completed route as partial on late Ctrl+C
The post-import CANCEL check tested the sticky global Ctrl+C flag
instead of the authoritative outcome from run_freerouting. Pressing
Ctrl+C during the "Importing SES..." step after FreeRouting had
already fully completed would report "Partial result saved," skip the
timing summary, and skip opening KiCad even though the board was fully
routed. Removing the early return lets the common path below use
outcome to report correctly in all cases.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Avoid concurrent JAR download races; clarify --fr-timeout comments
Suffix the FreeRouting JAR temp download path with the process ID so
two concurrent `pcb route` runs don't interleave writes into the same
.tmp file and spuriously fail the SHA-256 check. Also reword comments
that described format_hms's sub-minute precision as reachable via
--fr-timeout, which is always minute-aligned in practice.
* Stage sibling .kicad_dru when routing so zone fill honors custom rules
Custom design rules live in <project>.kicad_dru, not the .kicad_pro
file. The local FreeRouting path staged only the board and project
copies, so the DRC engine used by import_ses's zone fill fell back to
default clearances and published fills that ignored the board's
custom rules.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Don't report a completed route as success when final output fetch fails
poll_job's Completed branch silently fell back to last_known_output
(a stale snapshot from an earlier RUNNING poll) when the terminal
get_output call failed, while still reporting RunOutcome::Completed.
That published a partially-routed board over the user's original
file while printing "Result saved" instead of "Partial result
saved". Now a failed final fetch downgrades the outcome to
Cancelled so the messaging matches what's actually written to disk.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Harden FreeRouting cache dir against local-user tampering on /tmp fallback
When dirs::cache_dir() is unavailable, the JAR cache fell back to a
fixed, predictable /tmp/pcb/freerouting path that another local user
could pre-create with hostile permissions/ownership. Restrict newly
created cache dirs to owner-only (0700) and refuse to trust one owned
by a different uid.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Throttle poll-loop output refresh to reduce overhead on large boards
get_output makes the FreeRouting server serialize the whole
in-progress board to SES and base64-encode it. poll_job was calling
it on every ~1s poll tick while the job was in progress, forcing a
full serialize/encode/decode cycle every second for the entire
(up to 60 minute) run. Throttle refreshes to once every 5s instead;
the cache only needs to be recent enough to survive a cancel/output
race, not perfectly up to date.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Don't report a fully-routed board's clean completion as a fetch failure
poll_job's JobState::Completed branch used best_effort_output(), which
collapsed the API's JobOutput::NothingToRoute (a legitimate "nothing
left to route" result) into the same None as a genuine output-fetch
error. A board where every net was already connected therefore saw
"finished but its final output could not be fetched" and had its
outcome downgraded from Completed to Cancelled, reporting "stopped ...
with no routing progress" for a run that actually succeeded.
Match on JobOutput directly so NothingToRoute stays a clean Completed
result with no SES to write, and reserve the warning + downgrade for
real fetch failures.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Actually refuse the FreeRouting cache dir when owned by another user
restrict_dir_to_owner only warned on an ownership mismatch and let
find_or_download_freerouting_jar proceed anyway, so the /tmp fallback
path's doc comment ("refuse to reuse it if it's owned by someone
else") didn't match behavior — an attacker-owned cache dir was still
downloaded/cached into. The dir's contents were still hash-verified,
but the directory itself stayed attacker-controlled (pre-placed
files, .tmp tampering, rename races).
restrict_dir_to_owner now returns whether the dir is safely owned by
us; a mismatch bails out with a clear error instead of silently
downloading into it. The ownership check also now runs before the
cached-JAR reuse path, not just before a fresh download, since reuse
had skipped the check entirely.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Scope the FreeRouting temp-dir cache fallback by uid
The dirs::cache_dir() fallback used a fixed pcb/freerouting path
under the shared system temp dir, so any two local users landed on
the same cache location and had to fight over ownership (handled by
the ownership-mismatch bailout added previously). Scope the fallback
by effective uid (pcb-<uid>/freerouting) so different users never
share the path in the first place; restrict_dir_to_owner remains as
defense in depth rather than the primary mechanism.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Give get_output a longer timeout than status/control endpoints
The API client used one flat 20s timeout for every request, but
get_output's cost is proportional to board size (the server
serializes and base64-encodes the whole in-progress or final board),
unlike the cheap status/control calls. On a large board, a genuinely
completed job's final output fetch could exceed 20s, which poll_job's
Completed branch treated the same as "nothing to save" and fell back
to a possibly-empty last_known_output — silently discarding a
successful route. get_output now overrides the client default with a
120s per-request timeout.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Temporarily pin FreeRouting JAR to AdamMomen/freerouting v2.2.5
Upstream freerouting/freerouting has been slow to cut releases off
main, so point the auto-download/PATH-search integrity check at a
fork's v2.2.5 release (built off upstream's release head) as a
stopgap. FREEROUTING_REPO is a single constant, so reverting to
freerouting/freerouting (with a re-pinned SHA-256 for the matching
upstream release) once upstream catches up is a one-line change.
See PR discussion for the full rationale.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Extract execute_freerouting to match execute_deeppcb's pattern
route::execute inlined the freerouting engine's timeout validation
and board resolution directly in the match arm, unlike the deeppcb
arm which delegates to its own execute_deeppcb function. Extract the
same shape for consistency.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Restore explanatory comment on sandbox network-blocking env vars
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Drop DeepPCB comparison from FreeRouting changelog entry
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Trim comments in freerouting/api.rs
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Don't let get_output's 120s timeout block Ctrl+C responsiveness
get_output's timeout was raised to 120s for the terminal
post-completion fetch, but that same long budget also applied to the
opportunistic in-progress refresh and cancel-path fetches inside
poll_job's loop, so a slow response there could delay noticing
CANCEL for up to 120s. get_output now takes an explicit per-call
timeout: GET_OUTPUT_TIMEOUT (120s) only for the one-shot completed
fetch, DEFAULT_TIMEOUT (20s) for the in-loop refresh and
best_effort_output.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Update integration tests to use the pinned FreeRouting fork/version
Tests were downloading freerouting-2.2.4.jar from upstream
freerouting/freerouting, while production now pins AdamMomen/freerouting
v2.2.5 with a different hash/URL — so boot, cancel, and end-to-end
coverage exercised a different artifact than what users actually get.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Trim comments in freerouting/mod.rs
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Pin FreeRouting to official upstream v2.3.0 release
Upstream now ships the stack overflow fixes previously only available
on a temporary fork, so switch back to freerouting/freerouting.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Simplify FreeRouting output polling per review feedback
Fetch /output once at our own deadline (or Ctrl+C), then cancel,
instead of maintaining a 5s-refreshed output cache throughout the
run. FreeRouting's own job_timeout now trails our deadline by a
safety margin instead of matching it exactly, since it's just a
backstop for when our own cancel request fails to land.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Run FreeRouting in its own process group so Ctrl+C doesn't kill it early
Java previously inherited pcb's foreground process group, so the terminal's
SIGINT went straight to it and it could exit before the poll loop fetched
partial output. A second Ctrl+C now force-kills it immediately.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Add unit tests for FreeRouting process-group detachment
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Handle FreeRouting's TERMINATED job state
Add the missing TERMINATED variant and treat it like CANCELLED/TIMED_OUT:
fetch whatever partial route exists instead of hard-failing the run.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix incorrect FreeRouting v2.3.0 jar SHA-256 pin
The pinned checksum didn't match the actual published release
artifact, so integrity verification would fail on every download.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Revert "Fix incorrect FreeRouting v2.3.0 jar SHA-256 pin"
This reverts commit 4b5e58d880.
* Pipe FreeRouting stdout/stderr to a log file instead of memory
Java's output is now redirected straight to an on-disk log rather than
buffered in memory. Startup/lost-contact errors report the log path
(and exit status if known) instead of inlining captured output.
Extracted the stdout/stderr-to-log-file redirect into
pcb_command_runner::log_file_stdio, reusing the pattern already used
by CommandRunner's non-capturing path.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Use tempfile_in + persist for atomic board publish
The cross-filesystem fallback in publish_board wrote to a fixed
sibling .tmp path, which could collide across concurrent routes on
the same board and was left behind on failure. Switch to the same
tempfile_in(...).persist(...) pattern used elsewhere in pcbc for
atomic file publishing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Simplify FreeRouting jar acquisition to fr-jar/env/cache with bounded download
Drop the PATH scan, UID-scoped temp-cache fallback, and multi-attempt
retry logic — sources are now --fr-jar, FREEROUTING_JAR, or the
verified ~/.cache/pcb/freerouting cache. reqwest download is now a
single bounded attempt (60s timeout, 200MB cap).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Merge --fr-timeout into --timeout for pcb route
Per review: --timeout should apply to whichever engine is selected,
no need for a separate --fr-timeout.
* Remove --fr-jar flag, keep FREEROUTING_JAR env var; reject --project-id with --engine freerouting
Per review: only one way to pin an explicit JAR is simpler than a flag
and an env var doing the same thing. Also reject engine-mismatched
options instead of silently ignoring them.
* Ignore Java/KiCad FreeRouting integration tests by default, require FREEROUTING_TEST_JAR
Cheap error-path tests now run on all platforms including Windows. The
Java/KiCad-dependent tests are #[ignore]d so plain `cargo test -p pcbc`
never downloads/runs an external JAR, and fail loudly instead of
silently skipping when run explicitly with prerequisites missing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Require FREEROUTING_TEST_JAR only, drop FREEROUTING_JAR fallback in test jar resolution
Matches the reviewer's exact wording ("require FREEROUTING_TEST_JAR"),
keeping the test-only override separate from pcbc's production
FREEROUTING_JAR env var.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix job_timeout safety margin race with GET_OUTPUT_TIMEOUT
JOB_TIMEOUT_SAFETY_MARGIN_SECS (30s) was shorter than GET_OUTPUT_TIMEOUT
(120s), so FreeRouting's own job_timeout could fire while our deadline
handler's output fetch was still in flight, causing it to refuse output
the same way it does post-cancellation. Bump the margin to 150s so our
fetch always has time to complete first.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix three Bugbot-flagged FreeRouting issues: RUNNING output cache, Windows force-kill, publish permissions
- poll_job now caches SES output opportunistically while the job is
RUNNING, and falls back to that cache on terminal Cancelled/TimedOut/
Terminated states — FreeRouting's /output can refuse to return partial
data once a job leaves RUNNING, same as it does post-explicit-cancel.
- kill_child_now had no Windows implementation, so a second Ctrl+C
during a blocking output fetch couldn't unblock it; now shells out to
taskkill /F on Windows, matching the Unix SIGKILL path.
- publish_board's cross-filesystem fallback staged through a tempfile
(owner-only 0o600) then persisted over the original board, silently
tightening permissions; now copies the source board's mode onto the
staged file before publishing.
- FreeroutingServer::spawn's log file used a predictable PID-based path
in the shared temp dir (File::create, vulnerable to pre-staked
symlinks); now uses a uniquely-named tempfile, consistent with the
JAR cache's existing hardening.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Decouple output-cache refresh from pass-change detection
The salvage cache in poll_job only refreshed when current_pass changed,
so a long-running pass (e.g. an extended pass 0) left cached_output
empty/stale until the job reached a terminal state where /output can
refuse to return partial data. Refresh on a fixed 5s interval instead.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Gate output cache on RUNNING state, fall back to it on Completed fetch errors
The interval cache fired regardless of job state, so a Completed status
could trigger a redundant DEFAULT_TIMEOUT fetch right before the real
GET_OUTPUT_TIMEOUT fetch, and any bytes it cached were discarded on the
Completed error path (output: None). Gate the cache on RUNNING and use
it as a fallback when the terminal fetch on Completed errors.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Apply cargo fmt and fix zombie process in test
cargo fmt --all reformats three lines; detach_process_group_puts_child_in_its_own_group
was leaving a zombie by not calling child.wait() after killing it.
* Surface FreeRouting TERMINATED as an error instead of a normal cancel
Previously TERMINATED (a server-side crash, per FreeRouting's own
RoutingJobState docs) was folded into RunOutcome::Cancelled: execute()
reported "Partial result saved" with a zero exit code, identical to a
deliberate Ctrl+C, and the crash log was unconditionally deleted afterward.
Give it its own RunOutcome::Terminated so the log is preserved and execute()
returns an error after saving the partial board.
* Fail SES import on ImportSpecctraSES/Fill failure instead of ignoring it
Both return bool but the results were discarded, so a malformed SES or a
failed zone fill would still exit 0 and get published.
* Drop the 5s opportunistic output cache in poll_job
stop_and_capture already fetches output before calling cancel_job, so
the cache's justification (a race right after cancel_job) never
applied to that path. Remove the redundant periodic full-board
serialize/encode.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Kill FreeRouting via the shared Child handle instead of raw PID/FFI
ctrlc runs the Ctrl+C callback on a dedicated thread, not inside a
signal handler, so there's no async-signal-safety requirement forcing
a raw PID + kill(2)/taskkill. Share the Child directly and call
Child::kill() on it, removing the PID-reuse race, unsafe FFI, and the
Windows taskkill subprocess.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Report unrouted connections instead of always claiming success
FreeRouting's COMPLETED state only means the router stopped, not that
every net got routed. Fetch its own DRC report (GET /jobs/{id}/drc,
which reflects the live routed board) and surface the unrouted count
instead of unconditionally printing "Result saved".
* Download the FreeRouting JAR into a single NamedTempFile
Collapses the PID-suffixed .tmp path plus a separately-derived .part
path (two temp files, two renames, manual cleanup on each error path)
into one tempfile::NamedTempFile in the cache dir, persisted atomically
on success. Mirrors the pattern already used by publish_board.
* Trim the REST-vs-CLI rationale duplicated across freerouting doc comments
Now that FreeRouting's REST API mode is settled, the CLI-mode-vs-REST
design justification doesn't need restating in both mod.rs and api.rs.
* Strip explanatory comments from freerouting module
Removes doc comments and inline comments that restated what the code
already shows, plus organizational section banners. Keeps only the
module docs, one genuinely load-bearing ordering constraint, and the
SAFETY justification on the unsafe geteuid call.
* Add a send() helper and verb wrappers to remove repeated request/status/error handling
Each mutating endpoint repeated the same self.client.<verb>(self.url(...))
plus with_headers -> send -> check status -> api_error boilerplate.
Collapses both into a shared send() helper and thin get/post/put wrappers,
used by 8 of the 9 endpoints; get_output keeps its own status handling
since it treats some non-2xx responses as non-errors.
* Prune freerouting test module to logic-testing tests only
Drop tests that exercised std/OS behavior (nonexistent path, hex
round-trip, missing file, process groups, raw kill) rather than our
own logic, keeping only the known-digest and format_hms regression
tests.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Drop redundant JobStatus Option-field serde tests
Both only verified serde's normal Option handling; the all-states
test already covers the meaningful API contract.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix vacuous pass in test_freerouting_java_not_found
Force PATH empty so the test always exercises the missing-Java path
instead of silently passing when the host has Java 25+ on PATH.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Drop redundant DrcReport default-empty test
Same pattern as the JobStatus Option tests removed earlier: this only
verified serde's #[serde(default)] Vec handling, not app logic.
drc_report_counts_unconnected_items already covers the meaningful case.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix crash-with-no-output exiting successfully
The no-SES early return exited Ok(()) unconditionally, before the
Terminated hard-failure check below it ever ran. A FreeRouting crash
that leaves no salvageable SES now still bails with a non-zero exit.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Tighten visibility and dedupe Terminated bail message
DEFAULT_TIMEOUT is only used within api.rs, no need for pub. Also
collapse the duplicated "terminated unexpectedly" bail into a single
bail_if_terminated helper shared by both call sites.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix lock-order inversion in kill_child_now
CHILD was locked, then child locked while still holding CHILD -- the
opposite order from FreeroutingServer::drop (child then CHILD), which
can deadlock if force-kill races with the server being dropped. Clone
the Arc out of CHILD and release that lock before locking child.
* Propagate get_output errors instead of masking them as cancellation
When a job reached COMPLETED but fetching its output failed, the error
was swallowed and reported as RunOutcome::Cancelled with no output --
turning a real fetch failure into a false success/cancellation. Just
propagate the error.
* Don't eagerly keep the FreeRouting log file, persist it only on failure
.keep() bypassed cleanup for the entire run, so any early error path
(create_session, enqueue_job, update_settings, upload_input, start_job,
poll_job bails) leaked the log file with no path ever printed. Store
log_file as Option<NamedTempFile>: as long as .keep() is never called,
it auto-deletes on drop for free; keep_log() takes it out of the
Option and persists it only on a genuine failure path (start failure,
lost contact, TERMINATED, INVALID), so the user has something to
inspect.
* Remove unnecessary JAVA_HOME forwarding in sandbox
resolve_java() only ever looks for java on PATH (already forwarded),
so JAVA_HOME has no effect on the tested binary.
* Persist FreeRouting log before SES write on Terminated
keep_log() ran after the fallible SES write, so a write failure would
return early, drop FreeroutingServer, and delete the log the user was
just told to consult.
* Report log path even when keep() persist fails
keep_log() discarded the PersistError via .ok(), silently dropping the
still-owned NamedTempFile and returning an empty path to callers.
* Distinguish unknown unrouted count from zero unrouted
get_unrouted_count failures were swallowed via .ok(), and the None
result was treated identically to Some(0) in both the spinner and
final summary, reporting a failed DRC check as a clean route.
* Disable cleanup on log file before returning path on persist failure
keep_log() returned e.file.path() but then dropped the still-cleanup-
enabled NamedTempFile at the end of the match arm, deleting the log
and leaving the returned path pointing at nothing.
* Check pcbnew.SaveBoard/ExportSpecctraDSN return values in route scripts
Both calls returned bool but were unchecked, letting a failed board save
or DSN export exit 0 and silently produce a stale/incomplete result.
* Fail closed on missing unconnected_items in FreeRouting DRC report
serde(default) turned a missing field into an empty vec, reporting a
falsely-clean route instead of the intended "could not verify" state.
* Use routable-nets fixture in test_freerouting_cli integration test
BOARD_WITH_LAYOUT_ZEN has no nets needing an actual trace, so the E2E
test never exercised FreeRouting's real routing/output path.
* Remove FreeRouting cancel-then-fetch API tests
FreeRouting rejects output once a job settles into CANCELLED, so
test_freerouting_cancel_returns_output asserted a premise that's false
in practice (reproduced: 204 No Content after cancel). Removed it and
test_freerouting_api_server_boots along with their exclusive helpers.
* Drop shared-cache-dir ownership check for FreeRouting JAR cache
Per review: the SHA-256 verification on the cached/downloaded JAR
already guards against a corrupted or tampered file, making the
geteuid/chmod ownership check redundant defense-in-depth.
* Move FreeRouting changelog entry to Unreleased
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Akhil Velagapudi <akhilvelagapudi@gmail.com>