fix: remove mutexes around ownerinfo/rvinfo

[runcom]

Fix #11

[gemini-code-assist]

Summary of Changes

Hello @runcom, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request streamlines the ownerinfo and rvinfo API handlers by removing unnecessary sync.Mutex implementations. The change addresses a specific issue, likely indicating that the previous synchronization mechanism was either redundant or incorrectly applied, and that the underlying operations are inherently thread-safe or managed by other means. This simplification reduces code complexity and potentially improves performance by eliminating contention on these mutexes.

Highlights

• Mutex Removal in OwnerInfo Handlers: The sync.Mutex and its associated locking/unlocking calls have been removed from the OwnerInfoHandler and its helper functions (createOwnerInfo, updateOwnerInfo).
• Mutex Removal in RVInfo Handlers: Similarly, the sync.Mutex and its associated locking/unlocking calls have been removed from the RvInfoHandler and its helper functions (createRvInfo, updateRvInfo).
• Code Simplification: The removal of these mutexes simplifies the handler functions by eliminating the mu *sync.Mutex parameter and the explicit Lock() and Unlock() calls.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request removes mutexes from the ownerinfo and rvinfo handlers. The removal in api/handlers/ownerinfo.go is correct, as the mutex was scoped per-request and thus ineffective. However, removing the shared mutex in api/handlers/rvinfo.go re-introduces a critical race condition that can lead to unhandled errors and 500 responses to clients. I've added a detailed comment explaining the issue and suggesting solutions. While the mutex in ownerinfo was ineffective, it's worth noting that both handlers share a similar check-then-act pattern vulnerable to race conditions, which would be best addressed at the database level for a scalable solution.

[runcom]

/gemini review

[gemini-code-assist]

Code Review

This pull request removes mutexes from the ownerinfo and rvinfo handlers, with the goal of relying on database constraints for ensuring atomicity. This is a positive change in direction. However, the implementation is incomplete as it leaves behind check-then-act patterns that were previously protected by the mutexes. Without the mutexes, these patterns introduce race conditions. My review focuses on identifying these race conditions and suggests removing the redundant pre-checks to make the create and update operations fully atomic.

[runcom]

/gemini review

[gemini-code-assist]

Code Review

This pull request correctly removes the ineffective local mutexes in the ownerinfo and rvinfo handlers and replaces the racy check-then-act logic with atomic database operations. This significantly improves the correctness and robustness of these endpoints under concurrent requests. The addition of tests for the new conflict and not-found behaviors is also a great improvement.

I've added a few comments regarding error handling for data validation. Currently, user data parsing errors are resulting in a 500 Internal Server Error, when a 400 Bad Request would be more appropriate. Addressing this would make the API more robust and user-friendly. Overall, this is a solid improvement.

[runcom]

/gemini review

[gemini-code-assist]

Code Review

This pull request is a great improvement. It removes the application-level mutexes around ownerinfo and rvinfo handling and instead relies on the database for concurrency control. This is a more robust solution that avoids race conditions, especially in multi-instance deployments. The changes correctly use atomic database operations and handle the resulting errors in the HTTP handlers. The addition of integration tests for this new behavior is also excellent. I have one suggestion to refactor some duplicated code in the database layer for better maintainability.

[kgiusti]

LGTM - one nit: could you add another testcase to trigger the new error types ErrInvalidOwnerInfo = errors.New("invalid ownerinfo data")
ErrInvalidRvInfo = errors.New("invalid rvinfo data")

[runcom]

one nit: could you add another testcase to trigger the new error types ErrInvalidOwnerInfo = errors.New("invalid ownerinfo data")
ErrInvalidRvInfo = errors.New("invalid rvinfo data")

addressed with the latest revision

[djach7]

LGTM

[mmartinv]

LGTM overall, just some nit.

As a side comment: I really don't like the OwnerInfo name/term as it's misleading and not present at the FIDO Specification. I would rename it to RVTO2Addr or similar

[runcom]

I really don't like the OwnerInfo name/term as it's misleading and not present at the FIDO Specification. I would rename it to RVTO2Addr or similar

Created #118 to start the discussion there (AI made all the changes, so, let's discuss if it's what we want and decide what to do)

thanks for the reviews folks!