Skip to content

[ANCHOR-1251]: Permanent DoS of Stellar-RPC Payment Observer via Broken Fault-Recovery - #1983

Merged
amandagonsalves merged 2 commits into
developfrom
fix/anchor-1251
Jul 27, 2026
Merged

amandagonsalves merged 2 commits into
developfrom
fix/anchor-1251

Conversation

@amandagonsalves

Copy link
Copy Markdown
Collaborator

Description

StellarRpcPaymentObserver reuses a single-shot ScheduledExecutorService across restarts. shutdownInternal() calls executorService.shutdownNow(), which terminates it permanently, but the field is never reassigned. The first time any transient fault (RPC disconnect, a silence timeout, a DB hiccup, an event-publisher error) triggers AbstractPaymentObserver.restartInternal(), startInternal() tries to reschedule onto that already-terminated executor and throws RejectedExecutionException. That exception was only ever caught as TransactionException in restartInternal(), and checkStatus() — the body of the statusWatcher's scheduleWithFixedDelay task — had no guard around it at all, so the exception escaped both. Per ScheduledExecutorService's contract, an uncaught exception in a fixed-delay task permanently suppresses every future execution of that task. The result: one ordinary transient fault permanently and silently halts all on-chain payment detection until a manual process restart.

Fixing only the executor reuse isn't sufficient on its own. restartInternal() and checkStatus() are shared by both StellarRpcPaymentObserver and HorizonPaymentObserver — Horizon isn't vulnerable to this specific trigger only because it happens to build a fresh SSEStream on every restart, not because the supervisor itself is hardened. A future change to Horizon's own lifecycle could reintroduce this same class of failure there. The supervisor loop itself needed to become crash-proof, independent of which subclass or which fault caused the failure.

Changes

  • StellarRpcPaymentObserver.startInternal: recreates executorService when it's null or already shut down, instead of always scheduling onto the same single-shot instance created at construction. This is the actual root-cause fix — restarts no longer hit a terminated executor.
  • AbstractPaymentObserver.restartInternal: broadened the catch from TransactionException only to also catch RuntimeException, setting STREAM_ERROR. This status choice is deliberate: restartInternal() is only ever called from within checkStatusInternal()'s own STREAM_ERROR/SILENCE_ERROR/PUBLISHER_ERROR/DATABASE_ERROR branches, each of which has its own bounded backoff-then-shutdown logic (streamBackoffTimer.isTimerMaxed(), silenceTimeoutCount, etc). STREAM_ERROR is a no-op when set from the STREAM_ERROR branch itself and a rejected, harmless transition from the other three (per ObserverStatus.stateTransition, which only allows STREAM_ERROR from RUNNING) — either way, control returns with the original error status intact, so each branch's own retry count/timer still advances normally on the next tick instead of being short-circuited.
  • AbstractPaymentObserver.checkStatus: split into a thin wrapper around the renamed checkStatusInternal(), catching any RuntimeException as a last-resort backstop so the supervisor task itself can never die. Falls back to NEEDS_SHUTDOWN rather than STREAM_ERROR here, since NEEDS_SHUTDOWN is the only status reachable from every error state in the transition table — STREAM_ERROR would be silently rejected in most of them, leaving the failure invisible instead of converging to a clean, observable stop.
  • StellarRpcPaymentObserverTest.kt: added regression tests — executor recreation after shutdown, restartInternal genuinely resuming polling (verified via sorobanServer.getEvents actually being invoked again), restartInternal swallowing the exact RejectedExecutionException the defect throws, and checkStatus's backstop converging to NEEDS_SHUTDOWN when something unexpected throws.
  • StellarRpcPaymentObserverRecoveryE2ETest.kt (new): drives the observer's real start() lifecycle — actual executorService/silenceWatcher/statusWatcher threads on real wall-clock timing, only the SorobanServer network boundary mocked — through an induced transient outage past silence_timeout and back, asserting polling genuinely resumes and health returns to GREEN.

Acceptance Criteria

  • After a transient RPC/DB/publisher fault triggers restartInternal(), the observer resumes polling and its health check returns to GREEN, with no process restart required.
  • StellarRpcPaymentObserver.startInternal() never throws RejectedExecutionException after shutdownInternal() has run.
  • An unexpected RuntimeException anywhere in checkStatus()'s switch body never permanently stops the status watcher's scheduled task.
  • Existing bounded backoff-then-shutdown behavior (streamBackoffTimer.isTimerMaxed(), silenceTimeoutCount vs silenceTimeoutRetries, etc.) is unchanged for restarts that succeed normally.

Context

HackerOne #3857031

Testing

  • Unit: ./gradlew :platform:test --tests "org.stellar.anchor.platform.observer.stellar.StellarRpcPaymentObserverTest"
  • Integration: ./gradlew :platform:test --tests "org.stellar.anchor.platform.observer.stellar.StellarRpcPaymentObserverRecoveryE2ETest"

Documentation

N/A

Known limitations

N/A

* fix: rpc observer restart executor service after shutdown

* add: generic runtime exception catch in restart logic

* refactor: checkstatus to handle unexpected internal exceptions

* add: e2e test for rpc observer recovery after outages

* add: unit tests for observer executor and error handling
@amandagonsalves amandagonsalves self-assigned this Jul 23, 2026
Copilot AI review requested due to automatic review settings July 23, 2026 19:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens payment observer fault recovery to prevent polling from permanently stopping after transient failures.

Changes:

  • Recreates terminated Stellar RPC polling executors during restart.
  • Guards restart and supervisor paths against runtime exceptions.
  • Adds unit and lifecycle recovery tests.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
AbstractPaymentObserver.java Adds supervisor exception handling.
StellarRpcPaymentObserver.java Recreates the polling executor after shutdown.
StellarRpcPaymentObserverTest.kt Adds restart regression tests.
StellarRpcPaymentObserverRecoveryE2ETest.kt Adds transient-outage recovery coverage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

*   refactor stellar rpc observer recovery e2e test wait logic

*   add assertions for executor replacement and shutdown in recovery test

*   update stellar rpc payment observer restart internal test mocks and verification
@amandagonsalves
amandagonsalves merged commit 4981646 into develop Jul 27, 2026
11 checks passed
@amandagonsalves
amandagonsalves deleted the fix/anchor-1251 branch July 27, 2026 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants