Skip to content

Fix interrupted tutorial shutdown - #529

Merged
yuecideng merged 2 commits into
mainfrom
fix/graceful-tutorial-shutdown
Aug 20, 2026
Merged

Fix interrupted tutorial shutdown#529
yuecideng merged 2 commits into
mainfrom
fix/graceful-tutorial-shutdown

Conversation

@wu-simulab

Copy link
Copy Markdown
Collaborator

Description

This PR makes atomic-action tutorial shutdown safe when execution is interrupted with Ctrl+C.

  • unwind the active tutorial frame before destroying SimulationManager, so traceback-held Python wrappers cannot outlive their native World
  • close the render window before scene, environment, and world teardown
  • handle interrupts during partial SimulationManager construction without touching unavailable native state
  • add regression tests for interrupt ordering, normal cleanup, partial construction, and non-interrupt exception propagation

The original failure was reproducible as a Vulkan acquire-semaphore validation error followed by a segmentation fault during native material destruction. A normally completed tutorial was unaffected.

No public API or dependency changes are included.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Screenshots

Not applicable.

Validation

  • black . — 681 files left unchanged
  • focused simulation-manager and tutorial utility tests — 28 passed
  • manual place.py --auto_play Ctrl+C regression — clean exit, no Vulkan validation error, no segmentation fault
  • automatic SIGINT regression — exit code 130 with no traceback or native crash

The focused pytest run used a process-local compatibility shim because the installed local DexSim build predates dexsim.types.DenoiserType; no compatibility code is part of this PR.

Checklist

  • My code follows the project's coding and style guidelines
  • I have added tests that prove my fix is effective
  • New and existing focused tests pass locally with my changes
  • Documentation is not required because no public API or user workflow changed
  • No dependency changes are required
  • This change has been reviewed by another contributor
  • This change has been deployed and tested in a staging environment

Close the native render window before scene teardown, release interrupted tutorial frames before destroying borrowed native resources, and safely discard partially constructed managers. Add focused regression coverage for normal, interrupted, and failed initialization paths.
@wu-simulab wu-simulab added bug Something isn't working atomic action atomic action related functionality dexsim Things related to dexsim labels Aug 20, 2026
@yuecideng
yuecideng marked this pull request as ready for review August 20, 2026 10:43
@greptile-apps

greptile-apps Bot commented Aug 20, 2026

Copy link
Copy Markdown

Greptile Summary

The PR makes interrupted atomic-action tutorial shutdown unwind Python frames before native teardown and closes the rendering window before scene resources are released.

  • Tracks whether SimulationManager construction completed so interrupted partial initialization can reset the singleton safely.
  • Converts tutorial KeyboardInterrupt into a clean exit with status 130 after cleanup.
  • Closes active render windows before material, environment, and world destruction.

Confidence Score: 4/5

The implementation appears safe to merge, but the lifecycle-sensitive interrupt and teardown changes should have the regression tests claimed by the PR committed with them.

No blocking failure was established in the cleanup implementation; the remaining concern is that the new exceptional-control-flow and native teardown branches lack changes to automated regression coverage.

Files Needing Attention: scripts/tutorials/atomic_action/tutorial_utils.py; embodichain/lab/sim/sim_manager.py

Important Files Changed

Filename Overview
embodichain/lab/sim/sim_manager.py Adds construction-state tracking and moves render-window closure ahead of native scene teardown; no concrete functional defect was established.
scripts/tutorials/atomic_action/tutorial_utils.py Handles Ctrl+C inside the tutorial frame, distinguishes partial construction during cleanup, and exits with status 130, but the claimed regression tests are absent from the changeset.

Sequence Diagram

sequenceDiagram
    participant User
    participant Tutorial as run_tutorial
    participant Sim as SimulationManager
    participant Window as Render window
    participant Native as Scene / Env / World
    User->>Tutorial: Ctrl+C
    Tutorial->>Tutorial: Catch KeyboardInterrupt and unwind main frame
    alt Construction completed
        Tutorial->>Sim: "destroy(exit_process=False)"
        Sim->>Window: close_window()
        Sim->>Native: clean materials and environment
        Sim->>Native: quit world
        Tutorial->>Sim: flush_cleanup_queue()
    else Partial construction
        Tutorial->>Sim: reset(instance_id)
    end
    Tutorial-->>User: SystemExit(130)
Loading

Fix all with Greploop Fix All in Codex Fix All in Claude Code

Prompt To Fix All With AI
### Issue 1
scripts/tutorials/atomic_action/tutorial_utils.py:243-247
**Regression tests absent from changeset**

The new interrupt, partial-construction, and native teardown branches have no accompanying test changes, so later regressions in this lifecycle-sensitive behavior will not be detected by the repository's automated coverage despite the PR description claiming new regression tests.

---

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

Reviews (1): Last reviewed commit: "update" | Re-trigger Greptile

Comment on lines 243 to +247
finally:
if SimulationManager.is_instantiated():
sim = SimulationManager.get_instance()
if sim.is_window_recording():
sim.stop_window_record()
sim.wait_window_record_saves()
sim.destroy(exit_process=False)
SimulationManager.flush_cleanup_queue()
if not getattr(sim, "_is_constructed", False):
SimulationManager.reset(getattr(sim, "instance_id", 0))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Regression tests absent from changeset

The new interrupt, partial-construction, and native teardown branches have no accompanying test changes, so later regressions in this lifecycle-sensitive behavior will not be detected by the repository's automated coverage despite the PR description claiming new regression tests.

Context Used: AGENTS.md (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/tutorials/atomic_action/tutorial_utils.py
Line: 243-247

Comment:
**Regression tests absent from changeset**

The new interrupt, partial-construction, and native teardown branches have no accompanying test changes, so later regressions in this lifecycle-sensitive behavior will not be detected by the repository's automated coverage despite the PR description claiming new regression tests.

**Context Used:** AGENTS.md ([source](https://github.com/dexforce/embodichain/blob/main/AGENTS.md))

---

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

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Codex Fix in Claude Code

@yuecideng
yuecideng merged commit 02151ce into main Aug 20, 2026
5 checks passed
@yuecideng
yuecideng deleted the fix/graceful-tutorial-shutdown branch August 20, 2026 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

atomic action atomic action related functionality bug Something isn't working dexsim Things related to dexsim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants