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-simulabwu-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

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

FilenameOverview
embodichain/lab/sim/sim_manager.pyAdds 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.pyHandles 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 GreploopFix All in CodexFix 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.

P2Regression 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 CodexFix in Claude Code

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

Labels

atomic actionatomic action related functionalitybugSomething isn't workingdexsimThings related to dexsim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@wu-simulab@yuecideng