feat: remove archipelago-core (clustering moves to Pulse) by mikhail-dcl · Pull Request #128 · decentraland/archipelago-workers

mikhail-dcl · 2026-07-30T14:39:43Z

Island clustering is Pulse's responsibility, so the core workspace is deleted rather than kept behind a flag. This repo is now WS Connector + Stats.

Important

Do not merge before comms-gatekeeper's peer.{addr}.cluster_change subscriber is live. That hop is the one piece of core's job that is not Pulse's, and nothing here can substitute for it — WS Connector forwards island_changed, it does not produce it. Without gatekeeper, clients get no LiveKit connection string.

What moved

core did	now
clustering (64/80 single-linkage, 100-peer cap)	Pulse — union-find over 100 u cells, uncapped
`engine.islands`, `engine.discovery`	Pulse
`island_changed` + LiveKit token minting + ban check at mint	comms-gatekeeper

Checked against Pulse's source before deleting anything, not just its design doc.

Breaking

This repo no longer publishes engine.islands, engine.discovery or engine.peer.{addr}.island_changed, and no longer mints LiveKit tokens.
GET /islands now reports maxPeers: 0 (was 100) and C{n} ids — clusters are uncapped. Both pass through untouched; nothing in stats parses ids or compares maxPeers.
dcl_archipelago_peers_count / _islands_count are gone. Pulse exposes dcl_pulse_*, so dashboards on the old names go blind rather than red.
Rollback is a revert + image rebuild, not a config flip, and the ordering matters — see the runbook.
archipelago-ea-core deploy jobs removed: the image no longer contains core/dist, so deploying it there would crash-loop the service.

Also in here

@dcl/protocol moved off a CDN branch build whose source commit no longer exists, onto the npm release of protocol#453 — current_time as uint64, since a truncated timestamp makes /core-status permanently unhealthy. Pinned by tests against frozen wire bytes.
Stats' NATS handlers extracted and guarded. engine.discovery had no try/catch, and a throw in one of those callbacks stops delivery on every subject — that publisher is now out of repo.
/core-status freshness is an absolute delta: the timestamp is stamped on Pulse's host, so forward clock skew must not read as fresh.

Cutover, verification and rollback: docs/core-decommission-runbook.md.

Verification

yarn install --frozen-lockfile, yarn build, yarn test (83 tests, 14 suites) green.

Not verified — needs a live deployment: stats serving Pulse topology with core gone, and /core-status healthy from Pulse's heartbeat. No shadow comparison was run, so the topology difference between the two algorithms is unmeasured by choice.

🤖 Generated with Claude Code

The previous pin was a CDN branch build whose source commit no longer exists upstream. The npm release 1.0.0-30361475985.commit-df8a4ff is the build of protocol#453, which restores ServiceStatus/ServiceDiscoveryMessage to archipelago.proto with current_time as uint64 — required by stats' /core-status health window, and by Pulse as the new publisher of engine.discovery. archipelago.gen is this repo's only @dcl/protocol import and its .d.ts is byte-identical between the two builds, so this is behaviour-neutral. The newer release commit-2726089 was passed over: it carries unrelated comms/avatar_shape proto drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Extracts the engine.islands decode out of its inline NATS callback into a pure module so the migration contract is assertable, then pins it against real protobuf bytes: C{n} cluster ids and maxPeers 0 reach GET /islands intact, and an epoch-millisecond current_time survives the round trip — a regression guard for protocol#453, since a uint32 truncation makes /core-status read permanently unhealthy. Verified while writing these: nothing in stats compares maxPeers or parses island ids, and /core-status exposes only {healthy, userCount}, so Pulse's server_name and commit hash are invisible to realm-provider. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Island clustering is Pulse's responsibility: it publishes engine.islands and engine.discovery, and comms-gatekeeper mints the LiveKit connection strings and publishes engine.peer.{addr}.island_changed. Nothing was left for core to do, so the workspace is deleted rather than kept behind a kill switch. Verified against Pulse's source before deleting: ClusterTracker covers the clustering (union-find over 100 u cells, uncapped, dwell debounce), NatsPublisher publishes engine.islands with MaxPeers 0 and the 10 s engine.discovery heartbeat carrying an epoch-millisecond current_time, and peer.{addr}.cluster_change carries per-peer assignments. The one hop that is not Pulse's — cluster_change -> mint -> island_changed — belongs to comms-gatekeeper, so this must not merge before that subscriber is live. Removes the archipelago-ea-core deploy jobs: the image no longer contains core/dist, so deploying it onto that service would crash-loop it. Their removal also means the running task keeps its last good image until infra retires the service. BREAKING CHANGE: archipelago-workers no longer publishes engine.islands, engine.discovery or engine.peer.{addr}.island_changed, and no longer mints LiveKit tokens. Rollback is a revert plus an image rebuild, not a config change. dcl_archipelago_peers_count and dcl_archipelago_islands_count are gone; Pulse exposes dcl_pulse_clusters and friends instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

README and ai-agent-context describe two services, attribute engine.islands/engine.discovery to Pulse and island_changed to comms-gatekeeper, and list the removed clustering configuration against its Pulse equivalents. The published OpenAPI contracts no longer claim an archipelago core service performs the clustering, and GET /islands documents maxPeers 0 with C{n} ids. The clustering algorithm document is archived rather than deleted: it is the only written record of behaviour consumers still ask about, and it now points at git history for the implementation. The runbook covers the preconditions (gatekeeper's subscriber must be live), verification after deploy, the revert-based rollback with Pulse's feed stopped first so the two never publish island_changed at once, and the follow-ups that fall outside this repo — the dcl_archipelago_* to dcl_pulse_* metric rename, core's retired scrape targets, and the per-process cluster-id collision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The runbook listed COMMS_GATEKEEPER_URL among the variables removed with core. Core did read it, but so does WS Connector — it gates the handshake ban check and the ban sweep, and it fails OPEN: unset means every handshake is allowed, signalled only by one boot-time warning. Following the runbook while tidying core's variables would have silently disabled ban enforcement at the WS entry point. Deleting core also deleted the only place the variable was documented, so it is now described in ws-connector/.env.default (empty default, identical behaviour to it being absent). Also corrects two claims in ai-agent-context: WS Connector forwards island assignments rather than driving them and does not mint connection strings, and its .env.default does not carry auth settings. The configuration reference now lists what each surviving service actually reads. ws-connector changes are comments and .env.default documentation only — no logic, no behaviour. Two comments pointed at core/src/components.ts as a duplicate implementation to keep in sync; that file no longer exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The engine.discovery handler was the only one of the four without a try/catch, and the NATS component invokes these callbacks unguarded: a decode throw unwinds into the client's reader loop, which stops delivery on *every* subject. One bad frame would freeze /islands, /peers, /parcels and /hot-scenes at their last values while /status kept answering 200 and nothing reached the logger. That subject's publisher is now Pulse, outside this repo, so a malformed or renumbered frame is no longer hypothetical. Moves all four subscriptions into logic/subscriptions.ts so they can be tested without the HTTP scaffolding, and guards each one. Tests assert that a frame protobufjs rejects does not escape any handler. Also makes the /core-status freshness check an absolute delta. currentTime is stamped on Pulse's host now, and an unsigned comparison read a clock running ahead of ours as fresh — a total outage would have looked healthy for as long as the skew. Adds frozen wire bytes to the contract tests. Encoding and decoding with the same generated module only proves the build round-trips; the literals pin the field numbers Pulse writes, so a regeneration that renumbers current_time fails the test instead of silently zeroing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Runbook, all found by review against Pulse's source and this repo's code: - Pulse gates /metrics on a bearer token, so the verification curl returned a bodiless 401 that `-s` hid — indistinguishable from "not publishing". Adds the header, the port, and publish_failed_total, which is the counter that moves when the path to the broker breaks (dropped_total stays 0 and published_total simply stops). - Rollback said "clear Nats:Url / NATS_URL" as if either would do. The flat NATS_URL alias fills the key when Nats__Url is unset, so clearing one leaves the feed alive — the exact dual-publish flap the ordering exists to prevent. Both must be cleared, and disabling gatekeeper's subscriber moves first because it is the only half a CI redeploy cannot undo. - Rollback now states its cost: no new session gets an island assignment or a LiveKit token for the whole build-and-deploy window, and /core-status flips unhealthy after 90s. - Clusters:Enabled is true in appsettings.json but false in the C# property default, so the precondition now says to check it rather than trusting it. - ARCHIPELAGO_FLUSH_FREQUENCY was 2.0 *seconds*, documented as 2000ms in two tables. Recreating config from those would have set a 2000-second flush. LIVEKIT_API_KEY/_SECRET/_HOST expanded to LIVEKIT_API_HOST, which never existed; all three are required at startup. - Verification checked island count only, which passes even when every island reports peers: [] — the peer list is a join against heartbeat data. Adds the peer total and the subject-prefix trap. - Percolation overstated: Pulse's benchmark measures 2 clusters with the larger holding 4091 of 4095 peers, not one cluster. - Notes where Pulse's §5 is now stale (flag-based rollback, shadow comparison) so cross-reading does not mislead. Contracts and context: the CoreStatus schema declared {status, timestamp} while the handler returns {healthy, userCount}; GET /islands and the ws-connector spec still named core as the clustering author; stats was described as having a 60s peer expiry it has never had; Node.js 16+/18+ against a repo pinned to 24. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Review found the branch above this repo's norm on commentary and doc redundancy. Code: subscriptions.ts had a 15-line prose header on an 89-line file where the baseline is one-liners, so the four handlers now share a `guarded` wrapper — the fail-safe is structural instead of a paragraph asking the reader to trust it, and it removes 24 lines of repeated skeleton. decode.ts and core-status.ts lose comments that restated the code beside them. The two ws-connector comments left pointing at core as a "keep in sync" duplicate are gone: the file they named no longer exists, and archaeology is not actionable. Tests: drops the epoch-millisecond round-trip case, which the two frozen-byte tests provably subsume — encode(X) == WIRE plus decode(WIRE) == X imply it — and folds the protocol#453 rationale onto the constant it guards. Docs: the removed-variables table existed three times and had already drifted (one copy was missing a row); the runbook keeps it, since a rollback recreates config from it, and the other two now link. Three standing caveats duplicated between the runbook and ai-agent-context survive only in Known Architectural Issues, which is where a reader looks for them. Also drops a struck-through tombstone section, promotes the one live fact buried in it (stats has no time-based peer expiry), and removes Pulse's internal algorithm and this repo's own history from a published REST contract. No behaviour change; 83 tests still pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Counterpart to the same pass on comms-gatekeeper, on the code that becomes the sole consumer of engine.peer.*.island_changed once core is removed. - Extract the deny list out of the WebSocket handler into src/adapters/deny-list. It was a full adapter inlined into a 301-line controller: hardcoded URL, hand-rolled TTL cache, global fetch. The URL is now DENYLIST_JSON_URL, and the 5 minute TTL plus the deliberate advance-the-timestamp-even-on-failure behaviour are preserved. - Inject the fetch component into ban-checker and deny-list instead of reaching for global fetch. It was already wired in BaseComponents and simply not passed. - Take dependencies as Pick<AppComponents, ...> rather than inline structural types, matching stats/src/logic/subscriptions.ts. - Move ban-checker, ban-sweep, peers-registry and deny-list to the component/types/index folder layout, with interfaces out of the implementation files. banSweep is now a named IBanSweepComponent. - Upgrade @well-known-components/interfaces to ^1.5.2 in both workspaces and move ban-sweep to [START_COMPONENT]/[STOP_COMPONENT]. 1.4.3 neither exported the symbols nor deprecated the plain form. ban-sweep was the only component with a lifecycle to convert. The nats and server warnings that remain come from upstream packages. - Add getErrorMessage, replacing four competing idioms and the catch (e: any) that went with them. - Reuse the guarded() NATS callback wrapper from the stats service: a throw escaping one of these callbacks stops delivery on every subject, and comms-gatekeeper is the only publisher after this branch lands. - JSDoc on the adapter factories and interfaces. Tests: - ws-handler.spec.ts tested no production code. It imported only Stage and WsUserData, never registerWsHandler, and every block re-implemented the logic it claimed to verify ("Replicates the FIXED fetchDenyList logic", "Replicates the post-auth platform-ban branch"). Three blocks carried comments claiming to document security fixes while passing regardless of the source. Rewritten to capture the handlers registered on server.app.ws and drive them directly: 18 real tests replacing ~545 lines. - Add a real deny-list component spec, and e2e cases for deny-list and platform-ban rejection that drive actual handshakes. - Add test/mocks factories so specs stop hand-rolling doubles and casting. - Rename it() blocks that did not state a behaviour. ws-handler.ts coverage 70.19% -> 81.67% statements, 51.21% -> 71.42% branches, while the spec shrank — the deleted tests contributed nothing.

LautaroPetaccio · 2026-07-31T20:19:03Z

Standards pass over `ws-connector` (`e256452`, `4a1cbc3`, `6fd846a`)

Counterpart to the same pass on comms-gatekeeper#283. Scoped to ws-connector — stats is in better shape and this branch barely touches it.

Worth stating up front: I checked that the two services already interoperate correctly before changing anything. Subject, peer-id extraction, address casing and the IslandChangedMessage wire format all match. This PR is not fixing the cutover — it's cleaning up the code that becomes load-bearing for all island traffic once core is gone.

Why

The deny list was a complete adapter inlined into a 301-line controller — hardcoded URL, hand-rolled TTL cache, global fetch. Controllers should call components, not fetch and cache remote JSON.
ban-checker also used global fetch, while fetch: IFetchComponent was already wired in BaseComponents and simply never passed. That makes both untestable without stubbing a global.
Dependencies were declared as inline structural types instead of Pick<AppComponents, …>, which stats/src/logic/subscriptions.ts already does correctly.
Four competing error idioms (?? 'unknown', ?? 'Unknown error', (error as Error).message, err.message), most behind catch (e: any).
service.ts hand-rolled the NATS callback guard that stats already had — and a throw escaping one of those callbacks stops delivery on every subject, which matters more now that comms-gatekeeper is the only publisher.

How

Change	Detail
`adapters/deny-list/`	extracted from `ws-handler`; `DENYLIST_JSON_URL` config; 5-min TTL and advance-timestamp-on-failure preserved exactly
`fetch` injected	`ban-checker` and `deny-list` take the component
`Pick<AppComponents, …>`	replaces inline structural types
Folder layout	`ban-checker`, `ban-sweep`, `peers-registry`, `deny-list` → `component`/`types`/`index`; named `IBanSweepComponent`
`logic/errors.ts`	`getErrorMessage`, and `catch (e: any)` gone from touched files
`logic/nats.ts`	`guarded()` wrapper, mirroring the stats service
JSDoc	on adapter factories and interfaces; every existing rationale comment kept verbatim

Dependency upgrade. @well-known-components/interfaces ^1.4.3 → ^1.5.2 in both workspaces (they share a hoisted copy), and ban-sweep now uses [START_COMPONENT]/[STOP_COMPONENT]. On 1.4.3 the symbols didn't exist and the plain form wasn't deprecated, so this wasn't possible before. ban-sweep was the only component with a lifecycle to convert — stats components extend IBaseComponent but none implement the hooks. Both workspaces build and test clean on 1.5.2.

Two deprecation warnings remain at boot and are upstream, not ours: nats.* from @well-known-components/nats-component and server.* from @dcl/uws-http-server.

The part worth reviewing carefully: three specs tested nothing

ws-handler.spec.ts and service.spec.ts both tested no production code. Neither imported the thing it named — ws-handler.spec.ts imported only Stage and WsUserData, service.spec.ts never imported main — and each re-implemented in the spec the logic it claimed to verify. Their own comments say so:

// Replicates the FIXED fetchDenyList logic
// Replicates the post-auth platform-ban branch from registerWsHandler.

Three of those blocks carried comments claiming to document security fixes (deny-list bypass via claimed address, platform-ban handshake rejection) while passing regardless of what the source did.

Both now drive the real code:

ws-handler.spec.ts captures the handler object registerWsHandler registers on server.app.ws and invokes open/message/close against a stubbed socket — real code, no µWS server needed.
service.spec.ts calls the real main() against a local NATS broker, and moved to test/integration/ — it stands up a broker and exercises the whole subscribe → decode → forward path, which is not a unit test.

Plus a real deny-list component spec, a ban-checker spec (the fetch injection is what made it testable), and specs for peers-registry, getErrorMessage and the guarded wrapper.

Two independent checks that these actually bite:

Mutation. Moving the address/stage assignment to after the welcome send — the exact ghost-entry bug the old comment claimed to document — kills precisely the two tests guarding it. Same for the deny-list timestamp: regressing it to advance-only-on-success kills exactly the retry-storm test. Same for the reconnect re-subscription loop.
Coverage went up while the specs shrank. Deleting 215 lines of the fake ws-handler tests raised its coverage. They contributed nothing.

Coverage

Every file these commits touch:

File	Before	After
`ban-checker/component.ts`	52.38%	100%
`service.ts`	52.38%	100%
`logic/nats.ts`	33.33%	100%
`logic/errors.ts`	75%	100%
`peers-registry/component.ts`	70%	100%
`ban-sweep/component.ts`	91.3%	100%
`deny-list/component.ts`	— (new)	100%
`ws-handler.ts`	70.19%	100% stmts / 96.96% branch

Workspace: 77.72% → 92.91% statements, 63.52% → 82.14% branches. Tests 43 → 109. stats untouched at 40.

Dead code removed

Parcel and WsApp types — zero references anywhere. WsApp had been carried through the folder move unchecked.
safeEndWebSocket had an unreachable branch: no call site passes a code without a message. Rather than delete it and leave a trap where a later caller passes one and has it silently dropped, the parameters now travel as a pair so the invalid combination can't be expressed.

Flagged, not removed: google-protobuf and @types/google-protobuf are declared in both workspaces and referenced in no TypeScript anywhere — the generated protocol code uses protobufjs/minimal. Almost certainly core leftovers, but dropping a runtime dependency deserves a deliberate call and touches out-of-scope stats.

Not changed

stats is out of scope by choice — and worth noting it has no integration tests at all.
Note testMatch is test/**/*.spec.ts under one config, so unit/ vs integration/ here is directory convention rather than a separate runner.
The deny-list and platform-ban rejection paths are covered both in ws-handler.spec.ts (cheap, stubbed socket) and e2e.spec.ts (real handshake). Deliberate overlap — the e2e versions prove the real server path rejects, which stubs can't — but it is overlap.
The workspace-level ws-connector lint:check fails on test/** because its tsconfig excludes those files — pre-existing, and not what CI runs (the root lint:check passes).
The pre-existing floating promise in src/index.ts.

coveralls · 2026-07-31T20:20:35Z

coverage: 92.5% (+13.9%) from 78.61% — chore/decommission-archipelago-core into main

Coverage over the code the previous commit touched, plus the dead code that review turned up. The main find is that service.spec.ts was a third fake test, alongside the two already replaced: it never imported main() and re-implemented the island_changed forwarding inside the spec, so src/service.ts sat at 52% and would have passed with the real handler broken. It now drives the real main() against a local NATS broker, and moved to test/integration — it stands up a broker and exercises the whole subscribe/decode/forward path, which is not a unit test. New specs for ban-checker (the fetch injection is what made it testable), peers-registry, getErrorMessage and the guarded NATS wrapper, plus the protocol-violation branches of ws-handler and the error paths of ban-sweep. Coverage of the changed files: ban-checker 52.38% -> 100% service.ts 52.38% -> 100% logic/nats.ts 33.33% -> 100% logic/errors.ts 75% -> 100% peers-registry 70% -> 100% ban-sweep 91.3% -> 100% ws-handler 81.67% -> 100% statements, 96.96% branches Workspace total 77.72% -> 92.72% statements, 63.52% -> 81.92% branches. Dead code: - Remove the Parcel type and the WsApp type, both with zero references. WsApp was carried through the folder move without being checked. - safeEndWebSocket had an unreachable branch: no call site passes a code without a message. Rather than delete it and leave a trap where a later caller passes one and has it silently dropped, the parameters now travel as a pair so the invalid combination cannot be expressed. - Apply getErrorMessage to the three idioms missed in ws-handler last time ((err as Error).message and two catch (err: any)). Not removed: google-protobuf and @types/google-protobuf are declared in both workspaces and referenced in no TypeScript anywhere — the generated protocol code uses protobufjs/minimal. Left for a deliberate decision, since dropping a runtime dependency also touches the out-of-scope stats workspace.

decentraland-bot

I found one blocking resource-leak issue in the new fetch paths. CI is green, and the PR title/branch follow ADR-6. I also reviewed the public/API impact: the breaking semantics for /islands and the core removal are explicitly documented in the PR body/runbook, so I am not raising a separate blocker there.

Findings

P1 Unconsumed fetch response bodies on non-OK responses can leak response streams in the WS handshake path.
P2 Ban sweep intervals can overlap when checks are slow, multiplying load beyond the intended concurrency cap.

Security review: no secrets or auth bypasses found in the diff; the fetch body leak is the only security/reliability-sensitive issue I found.

Tests: I attempted to run yarn test --runInBand, but this checkout has no installed dependencies (jest: not found). GitHub checks show build/test/validations passing.

Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack

decentraland-bot · 2026-07-31T20:47:39Z

+        signal: AbortSignal.timeout(BAN_CHECK_TIMEOUT_MS)
+      })
+
+      if (!response.ok) {


[P1] This non-OK branch returns without consuming or cancelling the fetch response body. In the native fetch implementation used by @dcl/fetch-component, response streams must be consumed or explicitly cancelled when unused; otherwise repeated failed ban checks in the handshake path can leak resources. Please cancel before returning, e.g. response.body?.cancel() (or consume the body) before the return false.

decentraland-bot · 2026-07-31T20:47:39Z

+
+    try {
+      const response = await fetch.fetch(url)
+      if (!response.ok) {


[P1] Same fetch-body issue here: when the deny-list endpoint returns a non-OK status, this throws before consuming/cancelling the response body. Since this runs from the WebSocket handshake path, repeated failures can leave response streams open. Please cancel or consume the body before throwing, e.g. response.body?.cancel() before throw new Error(...).

decentraland-bot · 2026-07-31T20:47:39Z

-      }
+  async function start(): Promise<void> {
+    logger.info(`Ban sweep running every ${intervalMs}ms`)
+    handle = setInterval(sweep, intervalMs)


[P2] setInterval(sweep, intervalMs) can start another async sweep before the previous one finishes. If comms-gatekeeper is slow or peer count is high, the effective request concurrency becomes 20 * overlapping sweeps. Consider a simple running guard or scheduling the next setTimeout only after the current sweep completes.

Both raised in review. Unconsumed response bodies. @dcl/fetch-component cancels the bodies it discards internally and says why in its own source: an unconsumed undici body pins its socket and buffers the received bytes until GC. It also hands ownership of the returned response to the caller, and both adapters dropped it on the non-OK path without reading or cancelling it. The ban check is the one that matters: it runs on every WS handshake, so a gatekeeper returning 5xx — the outage the fail-open exists to survive — would pin a socket and buffer its bytes per connecting player, leaking worst exactly when the system is already degraded. The deny list has the same bug bounded by its 5 minute TTL, so at most one body per window. Both now cancel. Overlapping ban sweeps. setInterval does not await an async callback, so a sweep slower than the interval had the next one start on top of it. Each sweep carries its own budget of BAN_SWEEP_CONCURRENCY in-flight checks, so overlapping them multiplies load on comms-gatekeeper — and a slow gatekeeper is what makes a sweep outrun its interval in the first place, so the pile-up compounds the condition that caused it. Reachable rather than theoretical: a sweep takes up to ceil(peers / 20) * 1s against the 1s per-check timeout, so it outruns the 30s default past roughly 600 peers. Guarded against re-entry, and the skip is logged: it means either the interval is too short for the peer count or the ban check is degraded. A guard rather than a self-scheduling setTimeout keeps the fixed cadence and the unref(). Both fixes are mutation-tested: removing either one kills exactly its own test.

decentraland-bot

Re-reviewed the latest head (6fd846a). The previous P1 fetch-body leak is fixed in both non-OK paths, and the ban sweep now has a re-entry guard, so I don't see any remaining blockers.

Findings

P2 island_changed subject addresses are still looked up verbatim. The code already documents that the registry is lower-case/exact-match, so normalizing the subject token before getPeerWs would make the cutover robust to a mixed-case publisher.
P2 Runbook link to the ban-checker implementation is stale after the adapter folder split.

Public/API impact: the intended breaking changes (core subjects removed, /islands maxPeers: 0/C{n} ids, old metrics removed) are documented in the PR/runbook. I did not find an additional undocumented consumer break in this repo. The external ordering precondition remains: don't merge/deploy until comms-gatekeeper's cluster-change subscriber is live and verified.

Security review: no secrets, auth bypasses, sensitive logging, or new injection issues found.

Tests: NODE_ENV=development yarn build passes locally. Local yarn test cannot complete in this container because uWebSockets.js requires glibc (ld-linux-x86-64.so.2 missing here); GitHub CI shows build/test/validations passing.

Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack

decentraland-bot · 2026-07-31T21:14:56Z

+    guarded('island_changed', logger, (message) => {
+      // Peers register under their lower-cased address and this lookup is an exact string
+      // match, so a checksummed address on the wire would silently drop the message here.
      const id = message.subject.split('.')[2]


[P2] The registry stores lower-cased addresses from the handshake, but the address token from the NATS subject is used verbatim. If comms-gatekeeper ever publishes a checksummed/mixed-case address, this exact lookup drops the islandChanged message silently. Consider normalizing here before lookup, e.g. const id = normalizeAddress(message.subject.split('.')[2]), and add a mixed-case subject test.

decentraland-bot · 2026-07-31T21:14:56Z

+
+> **Do not remove `COMMS_GATEKEEPER_URL`.** Core read it too, but **WS Connector still does** —
+> it gates the per-handshake ban check and the periodic ban sweep
+> ([ban-checker.ts](../ws-connector/src/adapters/ban-checker.ts)). The check **fails open**: with


[P2] This link is stale after the adapter split; the implementation now lives at ws-connector/src/adapters/ban-checker/component.ts.

decentraland-bot

Re-reviewed latest head (f6a971f) after applying the two follow-up fixes.

Resolved

island_changed subject addresses are now normalized before peersRegistry.getPeerWs, with integration coverage for mixed-case subjects.
The runbook ban-checker link now points at the moved component.ts implementation.

Findings

P2 Stats still joins Pulse island peer IDs against heartbeat-derived peer keys verbatim. If Pulse ever emits checksummed/mixed-case wallet IDs in engine.islands, /islands can silently omit those peers. This is not a blocker for the documented cutover if Pulse emits lower-case IDs, but normalizing at decode/join time would make the stats path as robust as WS Connector.
P2 The rollback warning says “step 1 stops cluster_change, engine.islands and engine.discovery together,” but step 1 only disables gatekeeper’s island_changed output; Pulse’s NATS feed is stopped in step 2. Worth clarifying to avoid operator confusion.

Security review: no new security issues found in the follow-up normalization path; it does not expose LiveKit tokens in logs or widen peer access beyond the existing NATS publisher trust boundary.

Tests: NODE_ENV=development yarn build and NODE_ENV=development yarn lint:check pass locally. Local Jest still cannot run in this container because uWebSockets.js requires glibc (ld-linux-x86-64.so.2 missing here). GitHub CI build/test/validations are passing.

Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack

LautaroPetaccio

Great work!

LautaroPetaccio · 2026-07-31T20:51:02Z

+
+
+# Platform deny list JSON. Defaults to the public Decentraland list when unset.
+DENYLIST_JSON_URL=


Would you mind setting this in the definitions of the ws-connector?

mikhail-dcl and others added 8 commits July 29, 2026 16:37

mikhail-dcl requested a review from LautaroPetaccio July 30, 2026 14:45

decentraland-bot requested changes Jul 31, 2026

View reviewed changes

decentraland-bot approved these changes Jul 31, 2026

View reviewed changes

fix: normalize island change subject addresses

f6a971f

decentraland-bot approved these changes Jul 31, 2026

View reviewed changes

LautaroPetaccio approved these changes Jul 31, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: remove archipelago-core (clustering moves to Pulse) - #128

feat: remove archipelago-core (clustering moves to Pulse)#128
mikhail-dcl wants to merge 12 commits into
mainfrom
chore/decommission-archipelago-core

mikhail-dcl commented Jul 30, 2026

Uh oh!

LautaroPetaccio commented Jul 31, 2026 •

edited

Loading

Uh oh!

coveralls commented Jul 31, 2026 •

edited

Loading

Uh oh!

decentraland-bot left a comment

Uh oh!

decentraland-bot Jul 31, 2026

Uh oh!

decentraland-bot Jul 31, 2026

Uh oh!

decentraland-bot Jul 31, 2026

Uh oh!

decentraland-bot left a comment

Uh oh!

decentraland-bot Jul 31, 2026

Uh oh!

decentraland-bot Jul 31, 2026

Uh oh!

decentraland-bot left a comment

Uh oh!

LautaroPetaccio left a comment

Uh oh!

LautaroPetaccio Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants



		# Platform deny list JSON. Defaults to the public Decentraland list when unset.
		DENYLIST_JSON_URL=

Uh oh!

Conversation

mikhail-dcl commented Jul 30, 2026

What moved

Breaking

Also in here

Verification

Uh oh!

LautaroPetaccio commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Standards pass over ws-connector (e256452, 4a1cbc3, 6fd846a)

Why

How

The part worth reviewing carefully: three specs tested nothing

Coverage

Dead code removed

Not changed

Uh oh!

coveralls commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decentraland-bot left a comment

Choose a reason for hiding this comment

Uh oh!

decentraland-bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

decentraland-bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

decentraland-bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

decentraland-bot left a comment

Choose a reason for hiding this comment

Uh oh!

decentraland-bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

decentraland-bot Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

decentraland-bot left a comment

Choose a reason for hiding this comment

Uh oh!

LautaroPetaccio left a comment

Choose a reason for hiding this comment

Uh oh!

LautaroPetaccio Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LautaroPetaccio commented Jul 31, 2026 •

edited

Loading

Standards pass over `ws-connector` (`e256452`, `4a1cbc3`, `6fd846a`)

coveralls commented Jul 31, 2026 •

edited

Loading