Skip to content

[Sim Theme B] Launch menu & simulator lifecycle#23

Draft
jmservera wants to merge 5 commits into
jmservera/sim-theme-e-ros-integrationfrom
jmservera/sim-theme-b-launch-menu
Draft

[Sim Theme B] Launch menu & simulator lifecycle#23
jmservera wants to merge 5 commits into
jmservera/sim-theme-e-ros-integrationfrom
jmservera/sim-theme-b-launch-menu

Conversation

@jmservera

Copy link
Copy Markdown
Owner

Implements Theme B of the Simulation & Visualization PRD (FR-B1..B8). Lane 1 (foundation) — depends on Theme A (registry + GET /simulators). Adds launch/stop endpoints, concurrent + reload-persistent lifecycle, allowlisted start/stop, configurable autostart. Plan: docs/plans/sim-theme-b-launch-menu.md. Draft.

@jmservera
jmservera force-pushed the jmservera/sim-theme-b-launch-menu branch from d2f0f14 to 44a7113 Compare July 21, 2026 11:18
@jmservera
jmservera changed the base branch from jmservera/gazebo-ros-integration to jmservera/sim-theme-e-ros-integration July 21, 2026 11:18
…ncurrency, reload reconnect, autostart (FR-B1..B8)
@jmservera jmservera mentioned this pull request Jul 21, 2026
@jmservera

Copy link
Copy Markdown
Owner Author

🤖 Automated PR review

Launch menu & lifecycle — POST /simulators/{id}/launch|stop, per-sim autostart reconcile, and the interactive menu with polling + namespaced sim:<id> Golden Layout panels that survive pop-out/reload. Base: #26.

Findings

Severity Location Finding
🟡 Medium services/control/server.js (reconcileAutostart) Runs on every control-plane boot and stops any running autostart:false sim. A control-plane restart (crash / restart: unless-stopped) therefore silently kills a simulator the user launched manually from the menu. Consider a one-shot-at-stack-start guard or a "user-touched" marker so reconcile doesn't revert manual launches.
🟢 Low services/control/server.js reconcileAutostart fires once after a fixed UBEROS_AUTOSTART_DELAY_MS (3s); a slow host that hasn't finished creating containers misses reconciliation. A short bounded retry would be more robust.

✅ Security hardening is exemplary: launch/stop uses a registry-derived allowlist distinct from the restart allowlist, the id is charset-constrained by regex and validated against the registry, and only start/stop-by-container-id are issued — no exec/create/shell. Idempotent 204/304 handling, clean polling teardown in onDestroy.

Recommendation: Approve; consider the reconcile-on-restart behavior before merge.

@jmservera jmservera left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🤖 Automated line-anchored review — see the summary comment for the full findings table.

// until launched from the menu, and autostart-on simulators are (re)started if
// the profile left them down. Runs once shortly after boot to let compose finish
// creating containers; failures are swallowed so the menu still works manually.
async function reconcileAutostart() {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 Medium — reconcile-on-every-boot reverts manual launches. reconcileAutostart() runs on every control-plane boot and stops any running autostart:false sim. A control-plane restart (crash / restart: unless-stopped) therefore silently kills a simulator the user launched manually from the menu. Consider a one-shot-at-stack-start guard or a "user-touched" marker so reconcile doesn't undo manual launches. Separately (🟢 Low): the single fixed UBEROS_AUTOSTART_DELAY_MS timer can miss slow container creation — a short bounded retry would be more robust. The launch/stop allowlist hardening in this file is otherwise exemplary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant