feat: per-node config revision system for causal consistency (Phase 2)#186
Conversation
Implements the (_sender, _rev) tuple mechanism to prevent stale server- computed values from overwriting client state during high-frequency interactions. Rust changes: - Sanitize _-prefixed params before pipeline storage (websocket_handlers.rs) - Compositor extracts _sender/_rev from params, stamps view data JSON (compositor/mod.rs) TypeScript changes: - Add clientNonce (UUID) to WebSocketService, regenerated on connect - Add useConfigRev hook with per-node monotonic rev counters - Inject _sender/_rev in every commit adapter path (compositorCommit.ts) - Gate stale self-echoes in handleNodeParamsChanged (websocket.ts) - Gate stale view data in useServerLayoutSync (compositorServerSync.ts) - Add activeInteractionRef for live-mode interaction masking - Strip _-prefixed fields from YAML export (MonitorView.tsx) Tests: - useConfigRev unit tests (singleton counter behavior) - compositorCommit unit tests (stamping in all commit paths) - compositorServerSync unit tests (mapServerLayers pure helpers) - Integration tests for stale view-data gating and activeInteractionRef Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| if let Ok(mut json) = serde_json::to_value(&scene.layout) { | ||
| // Stamp view data with the sender/rev from the last | ||
| // UpdateParams so clients can detect stale self-echoes. | ||
| if !self.config_sender.is_empty() { | ||
| json["_sender"] = serde_json::Value::from(self.config_sender.as_str()); | ||
| json["_rev"] = serde_json::Value::from(self.config_rev); | ||
| } | ||
| view_data_helpers::emit_view_data(&view_data_tx, &node_name, || json); | ||
| } | ||
| last_layout = Some(scene.layout.clone()); |
There was a problem hiding this comment.
🚩 Multi-client concurrent edit: last-writer-wins stamp can cause missed geometry updates
The compositor node stores only the most recent config_sender/config_rev from UpdateParams. When two clients send concurrent updates, the second writer's metadata overwrites the first's. The resulting view data is stamped with the second writer's identity. The second writer's client may gate this view data (if rev <= localRev), missing geometry effects contributed by the first writer's change.
Example: Client A sends _sender=A, _rev=5, then Client B sends _sender=B, _rev=3. Both arrive before a tick. View data is emitted with _sender=B, _rev=3. Client B gates it (if localRev ≥ 3) and misses A's geometry contribution. Client A applies it (different sender). This is an inherent limitation of single-sender stamping and would require vector clocks or similar for a complete solution. For the target use case (preventing stale self-echoes during high-frequency slider drags by a single user), the approach is sufficient.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
…ends
- Rust: Always overwrite config_sender/config_rev on UpdateParams instead
of conditionally setting them. Non-stamped UpdateParams now clears to
defaults, preventing stale sender/rev from being emitted in view data.
- TS: Remove standalone _sender/_rev messages from the onParamChange path.
Each tuneNode call replaces the server's full node.params, so sending
_sender/_rev as separate messages would wipe durable params to {} after
stripping. Only the onConfigChange path (tuneNodeConfig) carries stamped
metadata since it sends the full config in a single message.
- Remove unused useConfigRev hook export (fixes Knip CI failure). The
standalone functions (getLocalConfigRev, bumpConfigRev, etc.) remain.
Signed-off-by: StreamKit Devin <devin@streamkit.dev>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
There was a problem hiding this comment.
Devin Review found 3 new potential issues.
⚠️ 1 issue in files not directly in the diff
⚠️ tuneNodeConfig/tuneNodeConfigSilent persist transient _sender/_rev into local Jotai param store (ui/src/hooks/useSession.ts:102)
In useSession.ts:102 and useSession.ts:129, writeNodeParams(nodeId, config, sessionId) writes the full stamped config — including _sender and _rev — into the Jotai nodeParamsAtom. Because writeNodeParams (ui/src/stores/sessionAtoms.ts:72) performs a shallow merge ({ ...current, ...params }), these transient keys persist indefinitely in the atom and are never cleaned up. Any consumer of the atom (e.g. InspectorPane.tsx:275, pipelineBuilder.ts:132) sees them. While critical downstream paths handle this (YAML export strips _-prefixed keys, stamp() overwrites them in outgoing configs), the local store is polluted with metadata that was intended to be purely in-flight.
View 7 additional findings in Devin Review.
Debug
There was a problem hiding this comment.
🔴 Stale _sender/_rev on view data after pin-management layout changes causes client to suppress legitimate updates
When a pin management event (input added/removed) triggers a layout change in the compositor, the emitted view data inherits the stale config_sender/config_rev from the last UpdateParams. The client that last sent a config update will suppress this view data via the stale echo gate in ui/src/hooks/compositorServerSync.ts:153-157, missing the layout change.
Scenario walkthrough
- Client A sends
UpdateParamswith_sender: "A", _rev: 5→ compositor storesconfig_sender="A", config_rev=5, marks dirty, emits view data stampedA/5. - Client A's gate:
sender==A && rev(5) <= localRev(5)→ suppressed (correct — client already has this layout). - A new input connects →
handle_pin_managementruns at line 664, setslayer_configs_dirty = truebut does NOT clearconfig_sender/config_rev. - On next tick,
resolve_scenerebuilds layout (new auto-PiP layer), layout differs fromlast_layout→ view data emitted stamped with staleA/5. - Client A's gate:
sender==A && rev(5) <= localRev(5)→ suppressed (incorrect — this is a new layout from the pin event that the client needs).
Client A misses the layout update until its next config change bumps the rev.
(Refers to lines 664-671)
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
🚩 Synchronous handle_tune_node lacks _-prefix stripping (inconsistency with fire-and-forget handler)
The synchronous handle_tune_node at apps/skit/src/websocket_handlers.rs:850 stores params.clone() directly into the pipeline model without stripping _-prefixed keys, unlike its fire-and-forget counterpart at line 983-986 which does map.retain(|k, _| !k.starts_with('_')). While the current client never sends _sender/_rev through the synchronous TuneNode action (the stamped configs use tunenodeasync and tunenodesilent), this inconsistency means that if any API client sends _sender/_rev via the synchronous path, those keys would leak into the durable pipeline model and appear in GetPipeline responses. Consider applying the same sanitization for defensive consistency.
(Refers to lines 847-857)
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
2 participants
Summary
Phase 2 of the compositor view-data causal consistency design. Implements the
(_sender, _rev)tuple mechanism to prevent stale server-computed values from overwriting client state during high-frequency interactions (slider drags, live resizes).Rust changes:
websocket_handlers.rs: Sanitize_-prefixed params (transient sync metadata) before storing in the durable pipeline model. Unsanitized params are still broadcast/forwarded to the engine so the compositor receives_sender/_rev.compositor/mod.rs: Extract_sender/_revfromUpdateParams, store onCompositorNode, and stamp outgoing view data JSON with the same values. Always overwrite (not conditionally set) so that non-stampedUpdateParamsclears stale values to defaults.TypeScript changes:
websocket.ts: AddclientNonce(UUID v4) toWebSocketService, regenerated on each WS connect. Exposed viagetClientNonce(). Stale echo-back gate inhandleNodeParamsChanged— if incoming_sendermatches our nonce and_rev<= local counter, skip the update. Also strips_-prefixed fields before writing to local state.useConfigRev.ts(new): Per-node monotonic revision counter (Map<nodeId, number>). ExportsgetLocalConfigRev,bumpConfigRev,resetAllConfigRevs,getClientNonce.compositorCommit.ts: EveryonConfigChangecommit path (commitLayers,commitOverlays,commitAll) now bumps the rev and injects_sender/_revinto outgoing config. TheonParamChangefallback path intentionally does NOT send standalone_sender/_revmessages because eachtuneNodecall replaces the server's fullnode.params— standalone metadata messages would wipe durable params to{}after stripping.compositorServerSync.ts: Stale view-data gate — if view data's_sendermatches our nonce and_rev<= local counter, skip applying. Also accepts optionalactiveInteractionRefto suppress view data during live interactions.useCompositorLayers.ts: AddedactiveInteractionRef(per-node boolean ref), threaded touseServerLayoutSync, and exposed in the hook result for consumer components to set during slider drags.MonitorView.tsx: Strip_-prefixed fields from YAML export to avoid leaking transient metadata.Benchmark results (compositor_pipeline, 300 frames, 1280×720):
Review & Testing Checklist for Human
_senderor_revfields in any node's params.Notes
TuneNodeSilent) will follow as a separate PR once this is validated, since the rev-gating mechanism here replaces the need for silent echo suppression.activeInteractionRefis exposed but not yet wired to slideronPointerDown/onPointerUpin the UI components — that wiring will be done in Phase 3 or can be added incrementally.config_sender/config_revpreserved whenUpdateParamslacks metadata, (2) standalone_sender/_revmessages inonParamChangepath would wipe server durable params.Link to Devin session: https://staging.itsdev.in/sessions/c3bbffbb30594d16845774b5bca4e32f
Requested by: @streamer45