Fix recording shutdown for node docker

[VietND96]

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

• 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 change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[qodo-code-review]

Review Summary by Qodo

Centralize supervisord shutdown signaling to fix recording shutdown

🐞 Bug fix

Walkthroughs

Description

• Centralize supervisord shutdown signaling in Python controller
• Prevent duplicate SIGTERM signals causing extended shutdown delays
• Handle external vs self-initiated shutdown scenarios correctly
• Add safeguards against re-entrant signal handling in shell mode

Diagram

flowchart LR
  A["video_recorder.py"] -->|"Captures external shutdown"| B["Signal handlers"]
  B -->|"SIGTERM/SIGINT"| C["Mark external shutdown"]
  A -->|"Self-initiated exit"| D["_signal_supervisord"]
  D -->|"SIGTERM"| E["supervisord"]
  F["video.sh graceful_exit_force"] -->|"Delegates to controller"| A
  G["Shell recorder"] -->|"Forward signal once"| H["video.sh"]
  H -->|"Exit normally"| D

Loading

File Changes

1. Video/video.sh 🐞 Bug fix +7/-1

Remove supervisord signaling from shell script

• Add guard flag _graceful_exit_done to prevent re-entrant calls to graceful_exit_force
• Remove direct supervisord SIGTERM signaling from shell script
• Add comment explaining supervisord signaling is delegated to Python controller
• Simplify shutdown logic by removing duplicate signal handling

Video/video.sh

---

2. Video/video_recorder.py 🐞 Bug fix +61/-4

Centralize supervisord signaling and fix shutdown logic

• Add _signal_supervisord() function to centralize supervisord shutdown logic
• Capture external shutdown state before asyncio replaces signal handlers in event-driven mode
• Only signal supervisord for self-initiated exits, not external shutdowns
• Prevent re-entrant signal forwarding in shell mode to avoid SIGTERM ping-pong
• Add comprehensive documentation explaining shutdown behavior and container lifecycle

Video/video_recorder.py

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

1. SIGTERM swallowed pre-async ☑ 🐞 ☼

Description

In event-driven mode, main() installs SIGTERM/SIGINT handlers that only set a flag and return, so a
stop request received before asyncio.run() installs loop signal handlers will not terminate or
initiate shutdown and can hang until supervisord SIGKILLs the process.

Code

Video/video_recorder.py[R48-57]

+        # Capture whether shutdown was externally initiated (SIGTERM/SIGINT)
+        # before asyncio.run() replaces the signal handlers via add_signal_handler.
+        _external_shutdown = [False]
+
+        def _mark_external_shutdown(signum, frame):
+            _external_shutdown[0] = True
+
+        signal.signal(signal.SIGTERM, _mark_external_shutdown)
+        signal.signal(signal.SIGINT, _mark_external_shutdown)
+

Evidence

video_recorder.py replaces SIGTERM/SIGINT with _mark_external_shutdown that does not exit or trigger
shutdown, and video_service.py only installs the real shutdown handlers later via
loop.add_signal_handler. Because the supervisord program uses stopsignal=TERM with a 60s
stopwaitsecs, a TERM during this window can prevent graceful stop and force a kill.

Video/video_recorder.py[44-57]
Video/video_service.py[997-1004]
Video/recorder.conf[1-10]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
In `SE_VIDEO_EVENT_DRIVEN=true` mode, `video_recorder.py` installs temporary SIGTERM/SIGINT handlers that only set a flag and return. If supervisord (or the user) sends SIGTERM during startup (before the asyncio loop handlers in `video_service.py` are installed), the process will ignore the stop request and keep running until supervisord hits `stopwaitsecs` and SIGKILLs.

### Issue Context
The event-driven service later installs its own handlers using `loop.add_signal_handler(...)`, but those are not active during the early startup window.

### Fix Focus Areas
- Video/video_recorder.py[48-57]

### Suggested fix
Update `_mark_external_shutdown` to *also* preserve termination semantics during the pre-async window. For example:
- For SIGTERM: set the flag then `raise SystemExit(0)` (or re-raise the signal after restoring `SIG_DFL`).
- For SIGINT: set the flag then call `signal.default_int_handler(signum, frame)`.
This keeps the flag behavior while ensuring SIGTERM/SIGINT still stop the process when received before asyncio installs its handlers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Event-driven shutdown misclassified ☑ 🐞 ≡

Description

In event-driven mode, SIGTERM/SIGINT delivered after the asyncio loop starts are handled by
video_service.py (loop.add_signal_handler), leaving _external_shutdown false and causing
video_recorder.py to treat an external stop as self-initiated and signal supervisord redundantly.

Code

Video/video_recorder.py[R71-73]

+        # Only trigger container shutdown for self-initiated exits (drain).
+        if not _external_shutdown[0]:
+            _signal_supervisord()

Evidence

video_recorder.py decides whether to call _signal_supervisord() based on _external_shutdown, but
video_service.py replaces SIGTERM/SIGINT handling with loop.add_signal_handler that does not set
that flag. As a result, an externally-triggered stop during normal event-driven operation can still
follow the "self-initiated" path in video_recorder.py.

Video/video_recorder.py[48-57]
Video/video_recorder.py[71-73]
Video/video_service.py[997-1004]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
In event-driven mode, `_external_shutdown` is only set by the temporary `signal.signal(...)` handlers in `video_recorder.py`. Once `video_service.py` installs `loop.add_signal_handler(...)`, SIGTERM/SIGINT no longer update `_external_shutdown`, so `video_recorder.py` may call `_signal_supervisord()` even when shutdown was initiated by supervisord.

### Issue Context
This is redundant (and possibly confusing) signaling: supervisord already initiated shutdown.

### Fix Focus Areas
- Video/video_recorder.py[48-57]
- Video/video_recorder.py[71-73]

### Suggested fix
Make event-driven signal handling set the external-shutdown flag in the *active* handlers:
- Option A: Modify `video_service.main()` to accept a callback/flag-setter, and have the `loop.add_signal_handler` lambda set both `external_shutdown=True` and `service.shutdown_event.set()`.
- Option B: In `video_recorder.py`, avoid the temporary `signal.signal` approach and instead have `service_main()` return an exit reason (e.g., `"external_signal"` vs `"drain"`) so the controller can decide whether to signal supervisord.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more