Switch from assert false to get_parent as a temporary fix

[edwardbr]

It has been worked out that this happens when a reference to an zone is passed to a zone that does not know of its existence.
SOLUTION:
Create a temporary "snail trail" of service proxies if not present from the caller to the called this would result in the i_marshaller::send method having an additional list of zones that are referred to in any parameter that is passing an interface. This list needs to be not encrypted along the chain of services in a function call so that they can maintain that snail trail. All service proxies whether already present or ephemeral will need to be protected with a shared pointer for the lifetime of the call. This will require a change to the code generator to populate the list of zones in the send method for the receiving service to process. TEMPORARY FIX:
This fix below assumes that the bottom most parent knows about the zone in question. However one branch may have a zone with a child that the bottom most node does not know about so this will break. With the proposed snail trail fix this logic branch should assert false as it should then be impossible to get to this position.

[gemini-code-assist]

Hello @edwardbr, 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!

Summary of Changes

Hello team, Gemini here with a summary of this pull request. This PR, authored by edwardbr, addresses an issue where a reference to a zone is passed to another zone that is not aware of its existence. The long-term solution proposed involves building a 'snail trail' of service proxies during calls, which would require changes to the code generator. This pull request implements a temporary fix for the immediate problem. It replaces an RPC_ASSERT(false) in a specific code path within rpc/src/service.cpp with a call to get_parent(). The author notes that this temporary fix assumes the bottom-most parent knows about the zone, which might not always be true and could still break in certain scenarios. The intention is that once the 'snail trail' solution is implemented, this code path should become unreachable, allowing the assert(false) to be reinstated.

Highlights

• Temporary Fix for Zone Reference Issue: Implements a temporary workaround for a known issue where zone references are passed to zones that don't know about them. A more robust solution involving a 'snail trail' is planned.
• Replaced Assertion with Parent Lookup: Replaces an RPC_ASSERT(false) statement with a call to caller = get_parent() in a specific else block within the service handling logic.
• Added Contextual Comments: Includes detailed comments in the code explaining the underlying problem, the proposed long-term solution, and the limitations of this temporary fix, directly mirroring the PR description.

Changelog

• rpc/src/service.cpp
  • Replaced RPC_ASSERT(false) with caller = get_parent() in the else block starting around line 803 (now line 804 in the new version).
  • Added extensive comments (lines 805-821) detailing the issue, the planned 'snail trail' solution, and the temporary nature of this fix.

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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

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 issue 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 is currently in preview and 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 to provide feedback.

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.

---

An assert removed,
A parent sought instead,
Fix for now, not long.

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 introduces a temporary fix for an issue where a zone reference is passed to a zone that is unaware of it. The change replaces an RPC_ASSERT(false) with a call to get_parent(), aiming to find a known zone. The PR description and the extensive inline comments clearly articulate the problem, the intended long-term "snail trail" solution, and the risks associated with this temporary workaround. This transparency is commendable.

My main concerns revolve around the verbosity of the inline comment and the potential for the temporary fix to introduce more subtle failure modes than an immediate assertion. I've provided specific feedback on these points.

Summary of Findings

• Verbose Inline Comment: The detailed explanation within the code (lines 805-821) is very verbose and largely duplicates the PR description. It's recommended to condense this and link to an external issue/ticket for full details to improve code conciseness.
• Potential Risks of Temporary Fix: The temporary fix caller = get_parent(); (line 822) might introduce risks. While RPC_ASSERT(caller) (line 825) handles nullptr from get_parent(), there's a concern that a valid but incorrect parent could lead to silent failures or misrouting, which might be harder to debug than the original RPC_ASSERT(false).

Merge Readiness

The pull request addresses a known issue with a temporary workaround, and the author has been commendably transparent about its nature and associated risks.

However, due to the high severity concern regarding the potential for the temporary fix (caller = get_parent();) to introduce subtle failure modes (e.g., silent misrouting if a valid but incorrect parent is returned), I recommend that these potential implications be thoroughly discussed and understood before merging. The long-term "snail trail" solution should be prioritized to mitigate these risks permanently.

As a reviewer, I am not authorized to approve pull requests. This feedback is intended to help ensure code quality and stability. Please ensure the concerns, particularly the high-severity one, are addressed or the risks explicitly accepted by the team before proceeding with the merge.