Docker: 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

Fixes #3093

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

🐞 Bug fix

Walkthroughs

Description

• Add upload timeout configuration to prevent indefinite hanging
• Track and cancel delayed cleanup tasks on shutdown
• Re-check shutdown event before processing events
• Fix Makefile LOG_LEVEL default and BASIC_AUTH variables scope

Diagram

flowchart LR
  A["Upload Process"] -->|timeout config| B["Prevent Hanging"]
  C["Session Cleanup"] -->|track tasks| D["Cancel on Shutdown"]
  E["Event Loop"] -->|re-check shutdown| F["Faster Termination"]
  G["Makefile Config"] -->|fix defaults| H["Correct Test Setup"]

Loading

File Changes

1. Video/video_service.py 🐞 Bug fix +28/-3

Fix recording shutdown and upload timeout handling

• Added upload_timeout configuration parameter (default 300s) to prevent indefinite upload hangs
• Introduced _cleanup_tasks list to track delayed cleanup tasks for proper cancellation
• Wrapped proc.communicate() with asyncio.wait_for() timeout and proper error handling
• Added re-check of shutdown event in subscribe_events() before processing received frames
• Implemented task cancellation in cleanup() method to immediately cancel delayed cleanup tasks
• Added done callback to remove completed tasks from tracking list

Video/video_service.py

---

2. Makefile ⚙️ Configuration changes +3/-3

Fix Makefile test configuration and variable scope

• Changed default LOG_LEVEL from "FINE" to "INFO" for test_node_docker target
• Moved BASIC_AUTH_USER and BASIC_AUTH_PASSWORD variable declarations outside platform-specific
 conditional block
• Ensures authentication variables are always defined regardless of platform

Makefile

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Undocumented upload timeout env 🐞 Bug ⚙ Maintainability

Description

VideoService now reads SE_VIDEO_UPLOAD_TIMEOUT to enforce an rclone upload timeout, but this new env
var is not documented in either the in-file environment variable list, README upload env table, or
ENV_VARIABLES.md. This makes the new timeout behavior difficult to discover/configure and can lead
to unexpected upload termination at the default 300s.

Code

Video/video_service.py[166]

+        self.upload_timeout = int(os.environ.get("SE_VIDEO_UPLOAD_TIMEOUT", "300"))

Evidence

The code introduces a new env var (SE_VIDEO_UPLOAD_TIMEOUT) and uses it to bound rclone uploads via
asyncio.wait_for, but the repo’s env var documentation and the module’s own docstring omit it, so
users won’t know how to adjust the timeout when uploads are slow.

Video/video_service.py[156-170]
Video/video_service.py[16-32]
README.md[839-848]
ENV_VARIABLES.md[34-39]

Agent prompt

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

### Issue description
`SE_VIDEO_UPLOAD_TIMEOUT` is now used to time-bound video uploads, but it is undocumented in the project’s env-var references.

### Issue Context
Users relying on documentation (README upload env table / ENV_VARIABLES.md / module docstring) will not discover this knob and may hit unexpected upload timeouts.

### Fix Focus Areas
- Video/video_service.py[1-32]
- README.md[839-848]
- ENV_VARIABLES.md[34-39]

### Suggested fix
- Add `SE_VIDEO_UPLOAD_TIMEOUT` (units: seconds, default: 300) to:
 - the top-of-file env var list in `Video/video_service.py`
 - `README.md` upload environment variables table
 - `ENV_VARIABLES.md` table
- (Optional) mention it near the upload timeout logic in code comments so future readers understand the expected units and behavior.

ⓘ 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