Skip to content

test: make stdio redirection scoped and exception-safe - #243

Merged
Emin017 merged 1 commit into
openecos-projects:mainfrom
Ayoubbelguellaoui:main
Sep 9, 2026
Merged

Emin017 merged 1 commit into
openecos-projects:mainfrom
Ayoubbelguellaoui:main

Conversation

@Ayoubbelguellaoui

Copy link
Copy Markdown
Contributor

What Changed

  • Replaced process-global stdout/stderr redirection in chipcompiler/utility/log.py with a thread-safe, context-managed _StdioRedirect implementation that safely saves and restores file descriptors even upon exceptions.

Updated chipcompiler/engine/flow.py, chipcompiler/engine/rerun.py, and agent/engine.py to ensure step-level log redirects close cleanly.

Updated chipcompiler/runtime/operations.py to treat event publisher exceptions as non-fatal and wrapped active operation tracking in finally blocks to guarantee cleanup.

Added and updated tests in test/cli/rendering/test_progress.py, test/runtime/test_events.py, and test/runtime/test_operations.py to cover stdio restoration, thread isolation under concurrency, and resilient operation tracking.

Scope

Select the areas touched by this PR:

  • CLI - command behavior, Typer command surface, output formats, or workspace commands.
  • Flow/runtime - workspace lifecycle, EngineFlow, step execution, logs, metrics, or artifacts.
  • EDA integration - Yosys, ECC-Tools, DreamPlace, KLayout, PDKs, or native/runtime wrappers.
  • Build/package - Nix, PyInstaller, wheels, uv.lock, or release artifacts.
  • CI/release - GitHub Actions, version checks, changelog, or release automation.
  • Tests/docs only

Runtime And Packaging Impact

  • No runtime or packaging impact
  • CLI output or machine-readable contract changed
  • Workspace layout, flow state, or artifact paths changed
  • Native toolchain or wrapper behavior changed
  • ecc-tools or ecc-dreamplace dependency changed
  • PyInstaller, Nix, or release artifact changed

Notes:

  • Fixed critical bugs where process-global stdio redirection corrupts concurrent flow logs, and where event publisher failures leave active operations permanently uncleaned.

Validation

List the commands you ran. Mark checks that are not applicable as N/A.

  • uv run pytest test/
  • uv run ruff check chipcompiler test
  • uv run ruff format --check chipcompiler test
  • PyInstaller smoke: ecc --help, ecc --version, ecc version --json
  • Nix smoke: nix run .#cli -- --help
  • Manual flow smoke:
  • Other: - uv run pytest test/ — 1460 passed, 8 skipped, 4 xfailed, 2 environment-related failures.

Skipped checks and reason:

Checklist

  • I kept the change scoped to ECC.
  • I updated docs or user-facing CLI text where behavior changed.
  • I included lockfile or version metadata updates when dependencies changed.
  • I documented any submodule updates and why they are needed.
  • I did not include local caches, virtual environments, or generated build outputs.
  • I explained skipped validation and remaining risk.

@Ayoubbelguellaoui Ayoubbelguellaoui changed the title stdio redirection + operation cleanup fix: make stdio redirection scoped and exception-safe Aug 25, 2026
@Emin017

Emin017 commented Aug 25, 2026

Copy link
Copy Markdown
Member

Thank you for your contribution! However, does this PR have any overlapping parts with #236?

@Ayoubbelguellaoui

Copy link
Copy Markdown
Contributor Author

There is some overlap at the step-execution/runtime level, particularly around EngineFlow and failure cleanup, but the two PRs address different issues.

PR #236 focuses on containing tool failures, persisting Incomplete, interrupted-operation recovery, and terminal failure state propagation.

This PR focuses specifically on making process-global stdout/stderr redirection scoped and exception-safe, isolating concurrent redirects, and guaranteeing runtime operation cleanup when event publishing fails.

The overlap is therefore limited to the surrounding execution/cleanup paths; the implementations address separate failure modes.

@Emin017

Emin017 commented Aug 25, 2026

Copy link
Copy Markdown
Member

This PR focuses specifically on making process-global stdout/stderr redirection scoped and exception-safe, isolating concurrent redirects, and guaranteeing runtime operation cleanup when event publishing fails.

The overlap is therefore limited to the surrounding execution/cleanup paths; the implementations address separate failure modes.

Got it, I just merged #236. Could you rebase and resolve the conflicts?

@Emin017

Emin017 commented Aug 26, 2026

Copy link
Copy Markdown
Member

Hi @Ayoubbelguellaoui , I’d like to ask why this PR was closed. Is there anything we can help with?

@Ayoubbelguellaoui

Copy link
Copy Markdown
Contributor Author

just for making sur that is work good , thanks i will reopen it

@Ayoubbelguellaoui
Ayoubbelguellaoui force-pushed the main branch 3 times, most recently from d4c3cf2 to 6ed53e8 Compare August 26, 2026 05:50
@Emin017

Emin017 commented Aug 26, 2026

Copy link
Copy Markdown
Member

@Ayoubbelguellaoui Is this PR ready to be merged?

@Ayoubbelguellaoui

Copy link
Copy Markdown
Contributor Author

yes it's ready

@Emin017 Emin017 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 - Close the process-level redirect created by init_api_runtime_log

At this line, _StdioRedirect.__enter__() saves dup(1)/dup(2), but the instance is immediately discarded. Since _restore() is only reached through close()/__exit__(), every call leaks two file descriptors and there is no handle to restore or replace the previous redirect. Repeated initialization on the same process grows the fd count (8 -> 10 -> 12 in a local check). Please retain a process-owned handle and close the previous redirect before reinitializing, and add a repeated-init cleanup test.

Comment thread chipcompiler/utility/log.py Outdated
Comment thread chipcompiler/utility/log.py Outdated
Comment thread agent/engine.py Outdated
Comment thread test/test_engine_flow.py Outdated

@Emin017 Emin017 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous step-log failure-summary issue is fixed, but the new capture error path introduces a blocking execution regression. Please address the inline finding before merging.

Comment thread chipcompiler/utility/log.py Outdated
@Ayoubbelguellaoui

Copy link
Copy Markdown
Contributor Author

fixed !

@Emin017 Emin017 added the bug Something isn't working label Sep 3, 2026
@Emin017 Emin017 added this to the 0.1.0-alpha.12 milestone Sep 3, 2026

@Emin017 Emin017 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous exception-escape regression is addressed, but the latest fixes still leave two blocking execution/ownership regressions. Please address the inline findings before merging.

Comment thread chipcompiler/engine/step_execution.py Outdated
Comment thread chipcompiler/runtime/operations.py Outdated
@Ayoubbelguellaoui

Copy link
Copy Markdown
Contributor Author

Fixed the P1 race condition by making terminal state publication and workspace cleanup atomic under a single lock, with a finally block for abnormal exits, and added regression tests. All 924 tests passed cleanly, and the lint checks out.

@Emin017 Emin017 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM. Could you rebase this?

Verify that AgentEngineFlow.run_step() executes the tool even when the
step log path is unusable (e.g. a directory), and that a failing tool
still becomes Incomplete without leaving any step stuck in Ongoing.
@Ayoubbelguellaoui

Copy link
Copy Markdown
Contributor Author

Done. The other changes are already covered by the merged commits.

@Emin017 Emin017 modified the milestones: 0.1.0-alpha.12, 0.1.0-alpha.13 Sep 9, 2026
@Emin017 Emin017 changed the title fix: make stdio redirection scoped and exception-safe test: make stdio redirection scoped and exception-safe Sep 9, 2026

@Emin017 Emin017 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Emin017
Emin017 merged commit 0623a3f into openecos-projects:main Sep 9, 2026
6 checks passed
@Emin017

Emin017 commented Sep 9, 2026

Copy link
Copy Markdown
Member

Done. The other changes are already covered by the merged commits.

Sorry about that! I didn’t realize the other changes were already covered by the merged commits.
If you’d like, I can still add you as a co-author on the relevant commit(s)/PR. Just let me know.

@Ayoubbelguellaoui

Copy link
Copy Markdown
Contributor Author

No, there’s no need. What matters most is improving the tool
thank you !

KoEkko added a commit that referenced this pull request Sep 10, 2026
Bring in #243 so the branch sits on current ECC main.
Yell-walkalone pushed a commit that referenced this pull request Sep 10, 2026
- Replaced process-global stdout/stderr redirection in
chipcompiler/utility/log.py with a thread-safe, context-managed
_StdioRedirect implementation that safely saves and restores file
descriptors even upon exceptions.

Updated chipcompiler/engine/flow.py, chipcompiler/engine/rerun.py, and
agent/engine.py to ensure step-level log redirects close cleanly.

Updated chipcompiler/runtime/operations.py to treat event publisher
exceptions as non-fatal and wrapped active operation tracking in finally
blocks to guarantee cleanup.

Added and updated tests in test/cli/rendering/test_progress.py,
test/runtime/test_events.py, and test/runtime/test_operations.py to
cover stdio restoration, thread isolation under concurrency, and
resilient operation tracking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants