rob
a287e74bd7
Reload no longer crashes on Chain.UNSPECIFIED, removes gRPC upstreams by prefixed id with lifecycle cleanup, applies methods.disabled changes synchronously, pushes status via existing SubscribeChainStatus streams, and optionally closes client RPCs so edges reconnect. NativeCall returns -32601 (CODE_METHOD_NOT_EXIST) for disabled/unknown methods. Co-authored-by: Cursor <cursoragent@cursor.com>
151 lines
9.5 KiB
Markdown
151 lines
9.5 KiB
Markdown
# dshackle v0.79.10 — SIGHUP reload removal investigation
|
|
|
|
**Base tag:** `v0.79.10` (`f2c1bd06`, "Update deps (#877)")
|
|
**Branch:** `reload-removal`
|
|
|
|
---
|
|
|
|
## (a) Hot-reload codepath and the `Key UNSPECIFIED missing` failure
|
|
|
|
### SIGHUP entry
|
|
|
|
| Step | Location |
|
|
|------|----------|
|
|
| OS signal registration | `ReloadConfigSetup.kt:21-22` — `Signal.handle(signalHup, this)` |
|
|
| Handler / lock | `ReloadConfigSetup.kt:25-58` — catches exceptions, logs `Config is not reloaded, cause - ${e.message}` |
|
|
| Upstream processor | `UpstreamConfigReloadConfigProcessor` in `ReloadConfigProcessor.kt:20-46` |
|
|
|
|
### Diff logic
|
|
|
|
1. **Full-config equality short-circuit** — `ReloadConfigProcessor.kt:24-26` compares deserialized YAML to `mainConfig.initialConfig` (`ReloadConfigService.kt:27`, `MainConfig.kt:28-38`). Any change (including `methods.disabled`) marks the upstream entry as modified.
|
|
|
|
2. **Per-upstream diff** — `ReloadConfigProcessor.kt:53-79`:
|
|
- Missing in new config → `removed`
|
|
- Present but not equal (data class) → `reloaded` (treated as remove **and** re-add)
|
|
- New keys → `added`
|
|
|
|
3. **Default-options diff** — `ReloadConfigProcessor.kt:82-108`: changed/removed chain names in `defaultOptions` populate `chainsToReload`, which triggers **bulk removal of all upstreams on that chain** before re-adding (`ReloadConfigUpstreamService.kt:50-56` pre-patch).
|
|
|
|
4. **Removal execution (v0.79.10 bugs)** — `ReloadConfigUpstreamService.kt:44-68` (original):
|
|
- Calls `multistreamHolder.getUpstream(chain)` for every chain in `chainsToReload` and every `upstreamsToRemove` pair.
|
|
- `CurrentMultistreamHolder.getUpstream()` uses `chainMapping.getValue(chain)` (`CurrentMultistreamHolder.kt:33-34`).
|
|
- Multistreams are created only for `Chain.entries.filterNot { UNSPECIFIED }` (`MultistreamsConfig.kt:30-31`).
|
|
- **`Chain.UNSPECIFIED` is not in the map** → Kotlin throws `NoSuchElementException: Key UNSPECIFIED missing`.
|
|
|
|
**When `UNSPECIFIED` enters the reload set:**
|
|
- `Global.chainById(unknownChainName)` returns `UNSPECIFIED` (`Global.kt:56-62`).
|
|
- `analyzeDefaultOptions` / `analyzeUpstreams` used `chainById` without filtering (`ReloadConfigProcessor.kt:62-63, 100, 106` pre-patch).
|
|
- `buildDefaultOptions` in `ConfiguredUpstreams.kt:97-105` (pre-patch) could store options under `UNSPECIFIED` when a default-options chain name was unknown.
|
|
|
|
5. **Additional v0.79.10 removal gaps (fixed in patch):**
|
|
- **Async removal:** `Multistream.processUpstreamsEvents()` emits to a Reactor sink processed on a scheduler (`Multistream.kt:153-157, 441-444`). Reload called `addUpstreams()` immediately after firing REMOVED events, so add/remove could race.
|
|
- **gRPC upstream IDs:** Config `id: foo` registers per-chain upstreams as `foo_ethereum`, `foo_polygon`, etc. (`GenericGrpcUpstream.kt:69`). Removal looked up exact config id (`ReloadConfigUpstreamService.kt:61-64` pre-patch) → **no match, upstream never removed**.
|
|
- **gRPC client lifecycle:** `ConfiguredUpstreams` started `GrpcUpstreams.start()` with no registry/stop on config removal (`ConfiguredUpstreams.kt:77-88` pre-patch) — outbound describe loop kept running after YAML entry deleted.
|
|
|
|
### `methods.disabled` on reload
|
|
|
|
- Parsed into `ManagedCallMethods` at upstream creation (`UpstreamCreator.kt:97-112`).
|
|
- A `methods.disabled` change makes the upstream data class unequal → `reloaded` set → remove + re-add path (`ReloadConfigProcessor.kt:69-70`).
|
|
- **Does apply on reload** (not startup-only), provided the YAML change is detected.
|
|
- Aggregated method list on the multistream is recalculated in `MultistreamState.updateMethods()` (`MultistreamState.kt:81-97`) via `onUpstreamsUpdated()` after REMOVED/ADDED events.
|
|
|
|
---
|
|
|
|
## (b) How method availability is announced to gRPC clients
|
|
|
|
| Mechanism | RPC | Method list? | Live updates? |
|
|
|-----------|-----|--------------|---------------|
|
|
| **Describe** | `BlockchainRpc.describe` → `Describe.kt:33-79` | Yes — `DescribeChain.supportedMethods` from `multistream.getMethods().getSupportedMethods()` (`Describe.kt:42-46`) | **No** — unary snapshot at call time |
|
|
| **SubscribeChainStatus** | `BlockchainRpc.subscribeChainStatus` → `SubscribeChainStatus.kt:27-47` | Yes — `SupportedMethodsEvent` in `ChainEvent` (`SubscribeChainStatus.kt:71`, `ChainEventMapper.kt:124-125`) | **Yes** — `multistream.stateEvents()` pushes `MethodsEvent` when methods change (`MultistreamStateHandler.kt:11-12`, `MultistreamState.kt:74-76`) |
|
|
| **SubscribeStatus** | `SubscribeStatus.kt:34-48` | **No** — availability + quorum only | Yes for status |
|
|
| **NativeCall** | — | Enforces at call time via `upstream.getMethods().isAvailable(method)` (`NativeCall.kt:313-327`) | N/A |
|
|
|
|
**Implication for dRPC edge gateways**
|
|
|
|
- Edges on an active **`SubscribeChainStatus`** stream receive updated method lists without reconnect (comment at `SubscribeChainStatus.kt:28-29` explicitly mentions hot reload).
|
|
- Edges that only call **`Describe` at connect** will **not** see removals until reconnect.
|
|
- **`Describe` is not streamed.**
|
|
|
|
---
|
|
|
|
## (c) NativeCall typed error for disabled / unknown methods
|
|
|
|
Proto field: `NativeCallReplyItem.item_error_code` (int), set in `NativeCall.buildResponse()` (`NativeCall.kt:193-196`).
|
|
|
|
| Condition | Behaviour (v0.79.10) |
|
|
|-----------|------------------------|
|
|
| Method not in aggregated list | `InvalidCallContext` with `ChainCallError(RpcResponseError.CODE_METHOD_NOT_EXIST, …)` (`NativeCall.kt:314-327`) |
|
|
| Method not callable on remote path | `RpcException(CODE_METHOD_NOT_EXIST, …)` (`NativeCall.kt:445-446`) |
|
|
| JSON-RPC standard code | `RpcResponseError.CODE_METHOD_NOT_EXIST = -32601` (`RpcResponseError.java:28`) — "method does not exist / is not available" |
|
|
|
|
**Bug (pre-patch):** `InvalidCallContext` stored the **request item id** in `CallError.id`, and `buildResponse` emitted that as `item_error_code` instead of `-32601`. Upstream failures via `ChainException` already used the RPC code as `CallError.id` (`NativeCall.kt:768`).
|
|
|
|
**Patch:** `CallError.itemErrorCode()` returns `upstreamError?.code ?: id` (`NativeCall.kt`).
|
|
|
|
Distinct codes exist: `-32601` (method missing) vs `-32603` (internal) vs `-32000..` (upstream) per `RpcResponseError.java`.
|
|
|
|
---
|
|
|
|
## (d) Graceful client disconnect mechanism (v0.79.10)
|
|
|
|
| Mechanism | Present? | Location |
|
|
|-----------|----------|----------|
|
|
| Server `@PreDestroy shutdown()` | Yes — full server stop only | `GrpcServer.kt:124-129` |
|
|
| `maxConnectionIdle(3600s)` | Yes — idle timeout, not reload-triggered | `GrpcServer.kt:81`, `Defaults.kt:33` |
|
|
| Per-connection GOAWAY / drain API | **No** | — |
|
|
| HTTP/2 GOAWAY on demand | **No** public API in grpc-java server used here | — |
|
|
|
|
**Conclusion:** v0.79.10 had **no** reload-triggered client refresh. Edges holding stale `Describe` data need either live `SubscribeChainStatus` or a forced reconnect.
|
|
|
|
### Patch approach (`reload-removal` branch)
|
|
|
|
1. **`GrpcClientRefreshService`** — server interceptor tracking active `ServerCall`s; `disconnectClients()` closes them with `Status.UNAVAILABLE` (`GrpcClientRefreshService.kt`).
|
|
2. **Config flag** — `reload.disconnect-clients-on-removal` (default `true`) in `ReloadConfig.kt` / `MainConfigReader.kt`.
|
|
3. **Triggered after** upstream/method removal in `UpstreamConfigReloadConfigProcessor.reload()` (`ReloadConfigProcessor.kt`).
|
|
|
|
This closes active RPCs (including long-lived `SubscribeChainStatus`), prompting client reconnect. It is not a raw Netty GOAWAY on the transport, but achieves the same operational outcome for grpc-java clients.
|
|
|
|
### Recommendation
|
|
|
|
| Edge behaviour | Approach |
|
|
|----------------|----------|
|
|
| Uses **SubscribeChainStatus** | Live `MethodsEvent` propagation is sufficient for method list; optional disconnect still helps for `Describe`-cached state |
|
|
| Uses **Describe-at-connect only** | **Requires reconnect** — use `disconnectClientsOnRemoval` (default on) or accept stale method lists until natural reconnect |
|
|
| Calls removed methods | **NativeCall `-32601`** — immediate typed failure even without reconnect |
|
|
|
|
**GOAWAY vs error-code:** Use **both**: error-code for in-flight calls to removed methods; disconnect (or SubscribeChainStatus stream) for updating the edge's cached capability set. Error-code alone leaves the edge believing the method still exists.
|
|
|
|
---
|
|
|
|
## Patch summary (`reload-removal`)
|
|
|
|
1. Filter `Chain.UNSPECIFIED` from reload diffs and default-options processing.
|
|
2. Synchronous removal via `Multistream.processUpstreamsEventsSync()`.
|
|
3. Match gRPC upstreams by `{configId}_` prefix; `GrpcUpstreamsRegistry` stops outbound clients on removal.
|
|
4. Propagate removals through existing `SubscribeChainStatus` / `MultistreamState` path.
|
|
5. Optional client disconnect on removal (configurable, default on).
|
|
6. Fix `item_error_code` for disabled methods (`-32601`).
|
|
7. Tests extended in `ReloadConfigTest.kt`.
|
|
|
|
---
|
|
|
|
## Build / test verification
|
|
|
|
```text
|
|
# Prerequisites (this workspace)
|
|
# - foundation published to mavenLocal (./foundation/gradlew publishToMavenLocal)
|
|
# - foundation/src/main/resources/public/chains.yaml (drpcorg/public submodule or raw fetch)
|
|
# - emerald-grpc/ proto submodule cloned
|
|
|
|
export JAVA_HOME=/usr/lib/jvm/java-21-openjdk-amd64
|
|
./gradlew test --tests "io.emeraldpay.dshackle.config.reload.*"
|
|
# BUILD SUCCESSFUL — 8 tests (ReloadConfigTest + HealthReloadConfigProcessorTest)
|
|
|
|
./gradlew build
|
|
# compiles + ktlint pass; full suite: 1635 tests, 2 failed (pre-existing on v0.79.10):
|
|
# - GenericWsHeadSpec (WS disconnect liveness)
|
|
# - IntegrationTest (Spring context / environment)
|
|
```
|
|
|
|
Reload-specific tests all pass. Full `./gradlew build` reports two failures that also occur on unmodified `v0.79.10` in this environment.
|