Repository navigation
feat: retain pending takeovers in assignment recovery - #498
Conversation
Test this pull request
|
mikhail-dcl
changed the title
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Summary
This PR adds retained pending takeovers (field 7) and an optional confirmed token floor (field 8) to PeerClusterChange, plus a new TakeoverCompleted message. It reserves draft field 6 by number and by name. The schema change only adds fields and is wire-compatible in both directions:
- Old readers skip fields 7 and 8. The tests cover this, including a relay through an old reader that drops the floor.
- Fields 1–5 keep their golden bytes.
- The presence semantics of
optional uint64hold on the wire and in JSON.
CI is green: check_and_build and the compatibility check against main both pass. A sub-agent reviewer also ran npm i && bash scripts/test.sh on Node 24 against the diff, and all 9 wire tests passed.
I found no P0 or P1 issues, so I'm approving. The P2 notes below are worth addressing before or soon after merge.
Findings
P2: the trust model for takeover_completed is only a recommendation (docs/pulse-room-recovery.md:36, pulse_clusters.proto:56-61). TakeoverCompleted carries only an ID and a cutoff. A forged completion with a known ID clears the pending entry, and Gatekeeper then issues credentials without the displaced session having been removed. Existing peer.* subjects already rely on NATS trust in the same way, so this PR doesn't make things worse. Still, the doc should state the publish ACL as a requirement (MUST) rather than advice. It should also say takeover_id must be unguessable (for example 128-bit random), not just globally unique.
P2: revoke_before has no upper bound. Pulse applies the maximum confirmed floor, and that floor never goes down. One faulty completion carrying a far-future value (the tests accept MAX_SAFE_INTEGER) would block that wallet's credentials permanently. Consider having Pulse reject revoke_before > now + skew.
P2: revoke_before has no unit, and the timestamp naming doesn't follow repo conventions. token_not_before documents whole seconds, but revoke_before has no comment. Elsewhere in the repo, timestamps are mostly int64 *_at in milliseconds. State the unit on field 2, or put it in the field names.
P2: fields 4/5 now overlap with pending_takeovers. Add a comment on displaced_session / displaced_cluster_id saying that pending_takeovers is authoritative for cleanup and that fields 4/5 remain a one-shot legacy hint. Also, the header comment (lines 5-6) still says the message is published only on cluster_change. The doc says it now also travels on snapshot hints and assignment replies.
P2: rollout order. A new Gatekeeper running against an old Pulse sees no floor, so it blocks all credentials. An old Gatekeeper running against a new Pulse ignores pending takeovers and issues credentials anyway. The description says the two need coordinated activation. A short deploy-order or feature-flag note in the Pulse and Gatekeeper PRs would make that actionable.
P2: scripts/generate-room-recovery.ps1 is tied to one machine (see inline). It isn't shipped in the npm package, so this is a repo-hygiene issue only.
P2: doc link to a feature branch (see inline).
Minor: the safe-integer tests match on ts-proto's internal longToNumber error text. That is stable while @dcl/ts-proto is pinned to 1.154.0, but it will break on an upgrade.
Consumer impact
The change only adds fields and messages. pulse_clusters.proto is re-exported through public/comms.proto, so the new types become part of the published @dcl/protocol TS and C# surface. Known consumers of PeerClusterChange are Pulse (generated PulseClusters.cs) and comms-gatekeeper (src/logic/cluster-subscriber). Both have companion PRs, so no existing consumer breaks.
The edited header comment (engine.peer.{address}.island_changed.{session}) matches Gatekeeper's current publisher, which uses the session-suffixed subject when a session is present.
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
|
|
||
| $ErrorActionPreference = 'Stop' | ||
| $protocolRoot = [System.IO.Path]::GetFullPath((Join-Path $PSScriptRoot '..')) | ||
| $verificationMutex = [System.Threading.Mutex]::new($false, 'Local\DCL_It2_HeavyVerification') |
There was a problem hiding this comment.
[P2] This script looks tied to one machine or agent environment rather than being general repo tooling:
- the named mutex
Local\DCL_It2_HeavyVerification - the 5 GB free-RAM gate (line 15)
- the
NODE_OPTIONSoverride - the hardcoded
protoc-22.2-win64path (duplicatesconfig.protocVersion) - the hardcoded
dcl-protocol-1.0.0.tgz(line 57), which only works because the local version is the 1.0.0 placeholder
It also copies scripts/test.sh but skips the new wire test and buf lint, so the doc's claim that it is an "equivalent" isn't accurate. Consider dropping it from this repo, or keeping it in Pulse or local tooling.
| The previous draft artifact used field 6 and different completion subjects. Reserving that | ||
| field makes it fail closed for the new consumer; the two drafts are not interoperable. | ||
| Explorer's wire format and metadata stay unchanged. Connector must forward refreshed credentials | ||
| for the same room. See the [implementation plan](https://github.com/decentraland/Pulse/blob/fix/pulse-owned-room-recovery/docs/demand-driven-recovery-protocol-plan.md) |
There was a problem hiding this comment.
[P2] This links to the fix/pulse-owned-room-recovery feature branch in Pulse, so the link will break once that branch is merged or deleted. Use a commit permalink or the main path once it lands.
| still carries the authenticated session selector. Hints trigger a fresh lookup before action. | ||
|
|
||
| Gatekeeper publishes the only new NATS event, `TakeoverCompleted`, on | ||
| `peer.{wallet}.takeover_completed`. Restrict publishing to Gatekeeper through broker ACLs. |
There was a problem hiding this comment.
[P2] The safety of the issuance barrier depends on only Gatekeeper being able to publish here: a forged completion with a known ID clears the pending entry. Please make this a normative requirement ("MUST be restricted by broker ACL"). Also require takeover_id to be unguessable (for example 128-bit random), not just globally unique.
| // ID has no effect. The effective cutoff must exceed the takeover's prior token floor. | ||
| message TakeoverCompleted { | ||
| string takeover_id = 1; | ||
| uint64 revoke_before = 2; |
There was a problem hiding this comment.
[P2] Please document the unit here (whole Unix seconds, as for token_not_before). Consider also stating an upper bound: Pulse keeps the maximum floor and it never goes down, so a far-future revoke_before would permanently block the wallet's credentials. Rejecting values above now + skew would guard against that.
|
|
||
| // Unfinished takeovers survive notification loss and later ownership changes. | ||
| // Gatekeeper must complete every entry before issuing current-session credentials. | ||
| repeated PendingTakeover pending_takeovers = 7; |
There was a problem hiding this comment.
[P2] Consider noting on fields 4/5 (or here) that pending_takeovers is authoritative for cleanup and that displaced_session / displaced_cluster_id remain the legacy one-shot hint. Otherwise new consumers have two overlapping sources.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Summary
Re-review at 8d67359. The only change since my previous (now dismissed) approval at 5ce0df9 is in docs/pulse-room-recovery.md:
- The departed-record lifecycle is now documented.
- The broker ACL is now a requirement ("must").
takeover_idis now specified as unguessable (128-bit random).
These address two of the earlier P2 notes. The .proto, the tests and the scripts are byte-identical to the version I approved before.
The schema change is still additive and wire-safe:
- Fields 7 and 8 are new, and field 6 is reserved by number and by name. That passes
buf breaking(WIRE_JSON). optional uint64keeps presence on the wire and in JSON.optionalis already used elsewhere in the pulse protos.
CI is green on this head: check_and_build and the compatibility check against main both pass. make test runs scripts/test.sh, which now includes the 9 room-recovery.test.cjs wire tests.
I found no P0 or P1 issues, so I'm approving.
Still open from the previous round (P2)
- P2:
revoke_beforeunit and bound are missing from the proto (pulse_clusters.proto:60, existing inline thread). The unit is documented only in the markdown. Pulse keeps the maximum floor, so one far-future value blocks the wallet permanently. Consider stating that consumers reject values above roughly now plus clock skew. - P2: the header comment is stale (
pulse_clusters.proto:5-6, see inline on line 13). It still says the message is published only oncluster_change. The doc says it also travels oncluster_snapshothints andcluster_assignmentreplies. - P2: fields 4/5 vs
pending_takeovers(existing inline thread). Say thatpending_takeoversis authoritative for cleanup and that fields 4/5 remain a one-shot legacy hint. - P2:
scripts/generate-room-recovery.ps1is tied to one machine (existing inline thread). It has a named mutex, a 5 GB free-RAM check, a hard-codedprotoc-22.2-win64path and a hard-codeddcl-protocol-1.0.0.tgzfilename. It also already differs fromtest.sh: it doesn't run the new node test. I suggest dropping it, or moving consumer-side generation into Pulse. - P2: doc link to the Pulse feature branch (
docs/pulse-room-recovery.md:59, existing inline thread). The link will break once that branch is merged or deleted.
New minor notes (P2)
- Forged completions. The doc now makes the ACL mandatory, which is the right control. Unguessable IDs add little defence in depth, though, because the IDs are broadcast on
peer.{wallet}.cluster_snapshotandcluster_assignment. Anyone who can read those subjects can forge a completion. This doesn't make anything worse than the existingpeer.*trust model, which is why I'm keeping it at P2. Read ACLs on those subjects, or Pulse checking the sender identity, would close the gap. - Running the tests standalone. The doc's
node --test scripts/room-recovery.test.cjsneedsout-jsto exist first. Mention that it has to run afterscripts/test.sh/make test. - Brittle test matching. The safe-integer tests match ts-proto's internal
longToNumbererror text. This is fine while@dcl/ts-protostays pinned to1.154.0, but the tests will break on an upgrade.
Consumer impact
The change is purely additive. Known consumers of PeerClusterChange are Pulse and comms-gatekeeper, and both have companion PRs. Old readers skip the new fields, and the tests cover that. As the description notes, the Gatekeeper/Pulse pair still needs coordinated activation.
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
| // Comms Gatekeeper subscribes here, mints the LiveKit connection string (the ban check is | ||
| // authority-local to gatekeeper), and re-emits the legacy | ||
| // decentraland.kernel.comms.v3.IslandChangedMessage on `engine.peer.{address}.island_changed`, | ||
| // decentraland.kernel.comms.v3.IslandChangedMessage on `engine.peer.{address}.island_changed.{session}`, |
There was a problem hiding this comment.
[P2] While this header is being edited, lines 5-6 above are now stale. Per docs/pulse-room-recovery.md, PeerClusterChange also travels on periodic peer.{address}.cluster_snapshot hints and positive peer.{address}.cluster_assignment replies, not only on cluster_change. Consider updating them, for example: "Published by Pulse on peer.{address}.cluster_change when the assignment changes, and reused for cluster_snapshot hints and cluster_assignment replies."
# Pull Request Description ## What does this PR change? Reports Nearby voice activity to scenes through `PBAvatarBase.talking` (`optional bool`, field 6). It is the same signal Bevy already ships, so scenes get one cross-engine field: - decentraland/protocol#493 (merged into `experimental-bevy`) - decentraland/protocol#501 (cherry-pick to `main`, merged) How it flows: - **Remote players:** `NearbyLivekitBridgeSystem` (global world) writes every `IsActivelySpeakingTag` flip (LiveKit active speakers) into the new scene-world component `SDKTalking` of the scene the player stands in; `NearbyTalkingSDKPropagationSystem` seeds the current value when the player's scene assignment changes. On purpose it is **not** gated on mute, block, scene ban or listening state: the field says the avatar talks, not that the local player hears it. - **Local player:** `NearbyVoiceChatStateModel.IsLocalSpeaking` → `TalkingComponent` (global world) → `LocalPlayerCRDTEntityHandlerSystem` mirrors it into `SDKTalking` of the persistent player entity of every loaded scene, portable experiences included. - **Change-only writes:** `SDKTalking` carries its own dirty flag (reset by `CleanUpAvatarPropagationComponentsSystem`), so a speaking flip rewrites only `AvatarBase`, never `AvatarEquippedData`. `WriteSDKAvatarBaseSystem` writes `talking` only when the value is set. - **Unset when Nearby is off:** the system is registered inside the Nearby feature-flag block only. Without it `SDKTalking.Value` stays null and the field is never written, so scenes can tell "no data" from "not talking". `scripts/package.json` pins the published `@dcl/protocol@1.0.0-39771882714.commit-70f2654` (`main` with decentraland/protocol#501 merged). The regenerated bindings also pick up the additive `PendingTakeover` / `TakeoverCompleted` messages from decentraland/protocol#498. Related PRs: - decentraland/js-sdk-toolchain#1654 - decentraland/sdk7-test-scenes#112 Supersedes Gabriel Díaz's `PlayerVoiceState` / `VoiceChatModifierArea` PRs (area-based voice components are dropped in favour of the address-targeted `PBAudioSourceEffect` design, backlog). Thanks to Gabriel for the groundwork on the active-speaker source and change-only writes: - #10417 - #10418 - #10419 - #10420 ## Test Instructions **Steps (standard run)**: ```bash metaforge explorer run 10462 ``` **Expected result**: The QA scene shows a label above every avatar: **no data** (grey, field unset), **silent** (white, `false`), **talking** (green, `true`). A player's label turns **talking** while they speak in Nearby voice chat and goes back to **silent** when they stop, on every client in the scene. With the Nearby feature flag off every label stays **no data**. ### Test Steps Setup: - Scene `4,26-avatarbase-talking` is deployed to the zone world `sdk7testscenes.dcl.eth`. MetaMask on **SEPOLIA**; open with the deep link `decentraland://?dclenv=zone&realm=sdk7testscenes.dcl.eth` or launch with `--dclenv zone --realm sdk7testscenes.dcl.eth`. - **Two clients on this build**, both standing inside parcel 4,26, Nearby voice chat enabled. A client on an older build shows **no data** above itself on every client: expected, its engine does not write the field. 1. Idle: both labels read **silent**. 2. Client A speaks: A's label turns **talking** on both clients, back to **silent** when A stops. 3. On client B mute or block A, then A speaks: A's label still turns **talking** on B (talking is not hearing). 4. Client A speaks while a portable experience is loaded: the PE reads the same `talking` on the local player entity. 5. Walk out of the parcel and back: the label reappears with the right state. 6. Nearby feature flag off: launch the client with `--nearby-voice-chat false` (overrides the remote `alfa-nearby-voice-chat` flag, works in any build without `--debug`). Every label stays **no data**, never **silent**. 7. Nearby disabled in the settings of client B: B's own label reads **silent**, A's label on B still follows A's speech (the engine keeps tracking the room). ## Quality Checklist - [x] Changes have been tested locally - [ ] Documentation has been updated (if required) - [x] Performance impact has been considered — change-only writes, no per-frame allocations - [x] For SDK features: Test scene is included — decentraland/sdk7-test-scenes#112
A missed takeover notification currently loses the displaced session/room from assignment recovery. This adds retained pending takeovers and a confirmed token timestamp to the existing assignment message, plus one backend
TakeoverCompletedevent.This replaces the earlier draft room-plan protocol: no epoch/revision/admission enum, bootstrap message, or retirement observation. Fields 1–5 remain unchanged; draft field 6 is reserved. A present zero timestamp is distinct from an unsupported response.
Validation: schema lint/build and released-main compatibility passed; generation/TypeScript compilation and all nine wire regressions passed.
Consumers: Pulse #54, Gatekeeper #308. Connector #137 forwards refreshed same-room credentials and is independently compatible. The backend pair requires coordinated activation. Crash-safe uncertain-operation recovery and Pulse state-loss recovery are outside this narrower contract.