Skip to content

fix: prevent race condition in camera buffer management#4649

Closed
thesohamdatta wants to merge 2 commits intoBasedHardware:mainfrom
thesohamdatta:main
Closed

fix: prevent race condition in camera buffer management#4649
thesohamdatta wants to merge 2 commits intoBasedHardware:mainfrom
thesohamdatta:main

Conversation

@thesohamdatta
Copy link

Add safety check in take_photo() to prevent capturing while upload
is in progress. This fixes potential memory corruption and crashes
during continuous photo capture operations.

fix: add upload guard to prevent camera buffer race condition
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The pull request introduces a safety check in the take_photo() function to prevent capturing while a photo upload is in progress, which is a good step towards preventing race conditions and memory corruption. However, the effectiveness of this fix relies heavily on the thread-safety of the photoDataUploading flag. If this flag is a shared global variable and modified by multiple tasks, its access needs proper synchronization to truly prevent race conditions in a multi-tasking environment, aligning with the rule that global variables are only safe without thread-safe accessors if modified by a single thread.

bool take_photo()
{
// Safety check: Don't capture if currently uploading
if (photoDataUploading) {
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The photoDataUploading flag is used here to prevent a race condition. However, if photoDataUploading is a global boolean variable modified by another task (e.g., the photo upload task) and read by this function, its access needs to be synchronized (e.g., using a mutex or std::atomic_bool) to ensure thread-safety. Without proper synchronization, a data race could still occur, leading to inconsistent state and potentially memory corruption, which this PR aims to prevent. This is a critical issue for the reliability of the system.

References
  1. This rule implies that for shared variables that are modified and read by multiple tasks, a critical section (or other synchronization mechanism) is necessary to prevent race conditions and ensure data consistency.

@beastoin
Copy link
Collaborator

@thesohamdatta Quick nudge: tests/demo evidence is required before review. Please add test output or a short screenshot/video, then flip this back to ready. Thanks.


by AI for @beastoin

@beastoin beastoin marked this pull request as draft February 11, 2026 01:19
@beastoin
Copy link
Collaborator

Closing this stale draft — no activity in 3+ days. Feel free to reopen when it's ready for review.

@beastoin beastoin closed this Feb 17, 2026
@github-actions
Copy link
Contributor

Hey @thesohamdatta 👋

Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request.

After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:

  • Project standards — Ensuring consistency across the codebase
  • User needs — Making sure changes align with what our users need
  • Code best practices — Maintaining code quality and maintainability
  • Project direction — Keeping aligned with our roadmap and vision

Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out.

Thank you for being part of the Omi community! 💜

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.

2 participants

Comments