feat: report synchronized property updates to RPC invocation observers

[totally-not-ai]

What

Synchronized property updates (mSync) were the one client-to-server RPC type that never reached the RPC invocation observers on the service event bus. Every other invocation — DOM events, @ClientCallable/template handlers, navigation, return channel messages — was already bracketed by RpcInvocationStartedEvent, RpcInvocationFailedEvent and RpcInvocationEndedEvent, so a tracing integration saw a gap exactly where property syncs spend session-lock time.

This PR reports property syncs like any other invocation, with the property name as the invocation name, and fixes the error handling on that path.

Why

A property sync is handled in two steps: the value of every synchronized property in the request is applied to the state tree first, and only then are the corresponding change events fired, so application code sees a fully updated tree.

The phase events surround the second step, since that is where application code runs. But the first step — applying the value — sat outside every try/catch. A property that is not synchronized at all, or a signal bound to it that rejects the write, threw straight out of handleRpc: no phase event was fired and the node-aware error handler was never reached.

Now such a failure is reported where it happens, with the same started/failed/ended events, and then rethrown — because refusing a value the client should not have sent deliberately aborts the request and reports an internal error to the client, which is the behaviour ElementPropertySignalBindingIT asserts for a binding without a write callback. The events of one property sync are still fired exactly once: either around the change event, or around the failure that replaced it.

Contract, spelled out

The javadoc of AbstractRpcInvocationEvent now states what reporting does and does not cover, because the code and the previous wording disagreed in both directions:

• Being reported does not mean the invocation had an effect. An RPC targeting a detached, disabled or inert node is reported and only then discarded by its handler — except a property sync, whose events are tied to the change event and are absent entirely when the update is discarded. A test pins that asymmetry with a request carrying both an event and a property sync for the same missing node.
• An update rejected by a model filter, or one whose value is already the one the client sent, yields a change event that does nothing, so it is reported even though nothing was handled.
• A property bound to a signal through Element.bindProperty has its write callback run in the first step, so its span brackets no work.
• Because the first step is completed for the whole request up front, all property updates in a request are reported before any other invocation it carries.
• Notifications do not overlap within one request, but a listener is registered on the service, so invocations of different sessions reach it from several threads at once.

Refactor

The property sync path would have repeated the whole of the phase-event plumbing that routing to a handler already did — the same listener check, the same extraction of node id and name, the same started/failed/ended firing around a body. Only the body and the invocation type differ, so those became parameters of a shared observeInvocation, and the details are still extracted once and only when the event bus actually has a listener, so an application observing nothing allocates nothing per invocation.

Tests

ServerRpcHandlerTest gains coverage for the events and the failure ordering on the new path, a signal-bound property whose span brackets no change event, a read-only signal binding (asserting the IllegalStateException that deferredUpdateFromClient documents, rather than any RuntimeException), a non-synchronized property, an unobserved failure still reaching the error handler, the single-phase-listener short-circuit, and the previously untested unsupported invocation type.

Use case

A team runs Vaadin behind OpenTelemetry and wants to know which client interaction is holding the session lock when a view feels slow. They already get spans for DOM events and @ClientCallable calls, but a form where every keystroke syncs a property showed up as unexplained time inside the request span. With property syncs reported, each sync becomes its own span labelled with the property name — and a signal binding that rejects a write surfaces as a failed span instead of a silent abort.

public class RpcTracingInitListener implements VaadinServiceInitListener {

    private static final ThreadLocal<Span> CURRENT_SPAN = new ThreadLocal<>();

    private final Tracer tracer = GlobalOpenTelemetry.getTracer("vaadin-rpc");

    @Override
    public void serviceInit(ServiceInitEvent event) {
        VaadinServiceEventBus eventBus = event.getSource().getEventBus();

        eventBus.addListener(RpcInvocationStartedEvent.class, e -> {
            // e.getType() is "mSync" for a synchronized property update,
            // e.getName() the property name (never the value it carries)
            CURRENT_SPAN.set(tracer.spanBuilder(e.getType() + " " + e.getName())
                    .setAttribute("vaadin.rpc.type", e.getType())
                    .setAttribute("vaadin.rpc.name", e.getName())
                    .setAttribute("vaadin.rpc.nodeId", e.getNodeId())
                    .startSpan());
        });

        eventBus.addListener(RpcInvocationFailedEvent.class,
                e -> CURRENT_SPAN.get().recordException(e.getError()));

        eventBus.addListener(RpcInvocationEndedEvent.class, e -> {
            CURRENT_SPAN.get().end();
            CURRENT_SPAN.remove();
        });
    }
}

Registered through META-INF/services/com.vaadin.flow.server.VaadinServiceInitListener, this now produces one span per property sync in the request, each named after the property that changed.

[github-actions]

Test Results

 1 384 files  ±0   1 385 suites  ±0   1h 23m 10s ⏱️ - 3m 42s
10 564 tests +9  10 497 ✅ +9  67 💤 ±0  0 ❌ ±0 
10 883 runs  +9  10 815 ✅ +9  68 💤 ±0  0 ❌ ±0 

Results for commit 7015685. ± Comparison against base commit 65c7ff3.

♻️ This comment has been updated with latest results.

[Artur-]

@claude merge origin/main

[claude]

Claude finished @Artur-'s task in 2m 22s —— View job

---

Merging origin/main

• Fetch latest origin/main
• Merge origin/main — blocked by tool permissions
• Push the result

I fetched origin/main, but I'm unable to run the merge. git merge, git pull, and even the read-only git merge-tree are all rejected with "This command requires approval" in my current environment — my --allowedTools only permits git add, git commit, git rm, and the git-push.sh helper, not git merge/git pull. There's no interactive approval available in this run, so these calls are auto-denied.

Good news: the branch is only one commit behind origin/main, and it's test-tooling only, so the merge should be trivial (almost certainly conflict-free):

e39aaf8e77 test: add generator script for the flow-tests migration list (#24926)

To complete this yourself:

git checkout 25236-msync-rpc-invocation-name
git fetch origin
git merge origin/main
git push origin HEAD

Alternatively, re-run me with git merge added to --allowedTools (e.g. Bash(git merge:*)) and I'll do it.

I'm sorry I couldn't complete the merge directly — see the FAQ for more on adjusting allowed tools.
· Branch

[Artur-]

Most of this seems to be duplicated from below? Can we extract an observer instead of extracting this, and e.g. pass in a lambda?

[totally-not-ai]

Good catch — it was a full copy of the plumbing, not just a few lines: the same three-way hasListener check, the same node id / name extraction, and the same started/failed/ended firing around a body.

Extracted observeInvocation(ui, type, invocationJson, Runnable), which now holds the only copy. handleInvocationData passes the routing to the handler as the lambda, and the property sync path passes the deferred change event, so runPropertyChangeEvent and the isInvocationObserved helper are both gone. Net −19 lines.

Two things that fell out of doing it this way: the failure path also collapses, since both sites already routed the throwable to callErrorHandler identically, and the observed short-circuit is now guaranteed to behave the same for both rather than by inspection.

[sonarqubecloud]

Quality Gate passed

Issues
14 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud