Skip to content

feat(merkle): register with multiple merkle-services and fan out reprocess - #362

Open
galt-tr wants to merge 16 commits into
mainfrom
feat/multi-merkle-service
Open

galt-tr wants to merge 16 commits into
mainfrom
feat/multi-merkle-service

Conversation

@galt-tr

@galt-tr galt-tr commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Lets arcade register every transaction with several merkle-services so one going down neither blocks registration nor stops SEEN/STUMP/BLOCK_PROCESSED delivery. Duplicate callbacks from the extra services are no-ops.

This is PR 1 of 2. It covers the steady state where every service holds the same registered set (so STUMPs are byte-identical). PR 2 (stacked on this branch) handles the outage case where a service missed registrations and therefore emits different STUMPs.

Config

merkle_service:
  url: "https://merkle-service-us-1.bsvb.tech"   # still works alone
  urls:
    - "https://merkle-service-us-2.bsvb.tech"
  auth_token: ""                                  # shared by every endpoint

Env form: ARCADE_MERKLE_SERVICE_URLS=a,b. merkle_service.* and callback_url/callback_token now have viper defaults registered, without which the ARCADE_* overrides for them were silently ignored.

Behaviour

  • merkleservice.Pool (new) fans /watch and /reprocess out to every endpoint. A tx is registered once any endpoint accepts it; only a tx refused everywhere is requeued (F-024 still gates broadcast). When every endpoint failed, the most retryable error is surfaced (context > network/5xx > 401/403 > other 4xx) so the existing propagation and watchdog classification keeps its meaning. auth_error now means every endpoint rejected the token.
  • Per-endpoint catch-up queue: a tx the pool reported registered must still reach every endpoint, or the watch sets diverge and the STUMPs disagree. Every endpoint-specific failure behind a pool-level success (refused or timed-out /watch, or skipped because the breaker was open) is kept in that endpoint's bounded in-memory queue and re-sent from the pool's background loop once the endpoint answers (500 per 5s tick, backoff, failures re-queued). Exposed as arcade_merkle_endpoint_catchup_pending{endpoint} and in /health.
  • Breaker + probe: 3 consecutive transport/5xx failures open an endpoint's breaker and it is skipped on the hot path (a black-holed service no longer adds its timeout to every batch). 4xx never trips it. A background GET /health probe (5s) closes it on any non-5xx answer; probe outcomes are classified (ok / err_5xx / err_network / canceled). An endpoint skipped for an open breaker counts as ErrNoHealthyEndpoints in the error merge, so a 4xx/401 from the others is not the pool's verdict while it may just be down.
  • Single-url deployments get the breaker too (behaviour change). Three in-flight 5xx/transport failures trip it, and registrations are then refused locally (ErrNoHealthyEndpoints, requeued by propagation and charged to the merkle retry budget) for up to one probe tick (5s). Before, every batch waited out the 30s HTTP timeout instead. Documented in docs/getting-started.md and config.example.yaml.
  • Straggler grace: once every tx of a /watch batch has an acceptance, the remaining endpoints get 5s and are then canceled. The cancellations count against an endpoint's breaker only if it acknowledged nothing in that batch (black-holed), not when it is merely slower.
  • Resync on overflow: the queue holds 200k entries per endpoint (about 30 min at 100 TPS). Past that the oldest entries are dropped and, once the queue drains, propagation runs a full lookback replay against that endpoint alone (same window as the startup replay, ignoring merkle_registered_at). A /watch that fails during the resync goes back to the queue. Coalesced per endpoint; waited on by Stop.
  • Interface: every consumer now takes merkleservice.Service; app.Bootstrap builds one Pool whenever the integration is enabled. The startup replay now marks only the txids that actually registered instead of dropping the whole batch's stamp on one failure.
  • Duplicate callbacks: SEEN_* were already idempotent via the status lattice (tests now pin the B/C/D scenario: 3× SEEN_ON_NETWORK then 3× SEEN_MULTIPLE_NODES ⇒ two publishes). BLOCK_PROCESSED redeliveries still re-publish MINED for the stored BUMP's rows, as on main: suppressing them is only safe when a failed re-mine or publish can take the block's processed_at stamp off again, which needs ClearBlockProcessed from feat(bump-builder): idempotent block processing across several merkle-services #363. feat(bump-builder): idempotent block processing across several merkle-services #363 re-enables the dedup together with that un-stamp.
  • Observability: /health lists merkle_endpoints; new arcade_merkle_endpoint_requests_total{endpoint,op,outcome} and arcade_merkle_endpoint_healthy{endpoint}.

Docs

docs/merkle-lifecycle-durability.md (new "Multiple merkle-services" section), docs/getting-started.md, config.example.yaml, metrics/README.md, openspec/specs/propagation/spec.md; stale "best-effort registration" wording in docs/overview.md and doc.md corrected.

Test plan

  • golangci-lint run ./... clean
  • go test -race on merkleservice, config, propagation, watchdog, bump_builder, api_server, app
  • New: pool fan-out / error merge / breaker / probe classification / catch-up queue (isolated failure, skipped-while-open, overflow→resync, Endpoint() queueing) tests, config list+env tests, resync-replay tests, duplicate BLOCK_PROCESSED publishes-only-newly-mined, multi-source SEEN scenario, /health merkle endpoints
  • Manual: two merkle-service containers, stop one mid-submission, confirm arcade_merkle_endpoint_healthy flips, arcade_merkle_endpoint_catchup_pending rises then drains after restart, and a single SEEN/MINED publish per transition

🤖 Generated with Claude Code

@galt-tr
galt-tr requested a review from mrz1836 as a code owner October 9, 2026 01:23
Copilot AI balanced review requested due to automatic review settings October 9, 2026 01:23
@github-actions github-actions Bot added the feature Any new significant addition label Oct 9, 2026
@galt-tr

galt-tr commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up PR 2 (divergent-STUMP handling, stacked on this branch): #363. Merge this one first; #363 then retargets to main automatically.

@github-actions github-actions Bot added the size/XL Very large change (>500 lines) label Oct 9, 2026

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.

🟡 Changes recommended

Endpoint-specific failures and recovery gaps can leave services permanently missing transaction registrations.

5 open findings
What changed in this PR

Adds multi-endpoint Merkle Service fan-out, circuit breaking, recovery replay, duplicate-callback handling, configuration, and observability.

Changes:

  • Introduces merkleservice.Service and multi-endpoint Pool.
  • Adds endpoint recovery replay and idempotent callback processing.
  • Expands configuration, metrics, health reporting, tests, and documentation.
File Description
services/​watchdog/​watchdog.go Uses the Merkle service interface.
services/​watchdog/​watchdog_test.go Updates test helper interface.
services/​watchdog/​service.go Supports pooled Merkle endpoints.
services/​propagation/​replay.go Adds targeted recovery replay.
services/​propagation/​replay_multi_test.go Tests pool registration and recovery.
services/​propagation/​propagator.go Wires recovery hooks and lifecycle state.
services/​propagation/​propagator_test.go Updates Merkle dependency typing.
services/​bump_builder/​reconciler.go Accepts pooled Merkle services.
services/​bump_builder/​multi_source_test.go Tests duplicate block callbacks.
services/​bump_builder/​builder.go Suppresses duplicate MINED events.
services/​bump_builder/​builder_test.go Models previously mined rows.
services/​api_server/​server.go Accepts the Merkle service interface.
services/​api_server/​handlers.go Reports endpoint health.
services/​api_server/​callback_multi_source_test.go Tests callback idempotency and health.
README.md Documents additional endpoints.
openspec/​specs/​propagation/​spec.md Specifies registration fan-out behavior.
metrics/​README.md Documents endpoint metrics.
metrics/​metrics.go Defines endpoint request and health metrics.
merkleservice/​service.go Defines shared service interfaces.
merkleservice/​pool.go Implements fan-out and circuit breakers.
merkleservice/​pool_test.go Tests pool behavior.
docs/​overview.md Updates registration lifecycle semantics.
docs/​merkle-lifecycle-durability.md Documents multi-service durability.
docs/​getting-started.md Adds multi-endpoint setup guidance.
doc.md Clarifies callback processing.
config/​config.go Adds endpoint lists and validation.
config/​config_test.go Tests endpoint and environment configuration.
config.example.yaml Adds multi-endpoint examples.
config.example.standalone.yaml Adds the empty endpoint list.
compose/​arcade-config.yaml Shows redundant endpoint configuration.
app/​app.go Constructs and starts the Merkle pool.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread merkleservice/pool.go
Comment thread services/propagation/replay.go
Comment thread merkleservice/pool.go Outdated
Comment thread services/propagation/replay.go
Comment thread metrics/README.md Outdated
Copilot AI balanced review requested due to automatic review settings October 9, 2026 01:54
…ocess

Add merkle_service.urls (alongside the existing url) so arcade registers
every transaction with every configured merkle-service and each of them
delivers callbacks. A tx counts as registered once any endpoint accepts
it, so a single merkle-service outage no longer blocks broadcast; only a
tx refused everywhere is requeued (F-024 still holds).

- merkleservice.Pool fans /watch and /reprocess out to all endpoints and
  surfaces the most retryable error when every endpoint failed, so the
  existing propagation/watchdog error classification keeps its meaning.
  Per-endpoint breaker (3 consecutive transport/5xx failures) with a
  background GET /health probe; 4xx never trips it.
- merkleservice.Service interface replaces *Client in every consumer;
  app.Bootstrap builds one Pool whenever the integration is enabled.
- Propagation re-registers the window an endpoint missed when its
  breaker closes (per-endpoint recovery replay, coalesced); the startup
  replay now marks only the txids that actually registered.
- Duplicate BLOCK_PROCESSED no longer re-publishes MINED for rows already
  mined against the block (short-circuit mines with onlyChanged=true).
- /health lists merkle_endpoints; new per-endpoint metrics
  arcade_merkle_endpoint_requests_total and arcade_merkle_endpoint_healthy.
- Register merkle_service.* and callback_* viper defaults so the
  ARCADE_* env overrides bind.

Duplicate SEEN_* callbacks were already idempotent via the status lattice;
tests now pin the multi-source scenario. Divergent STUMPs from a service
that missed registrations are handled in the follow-up change.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

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.

Comment thread config/config.go
Comment thread services/propagation/replay.go
Comment thread services/propagation/replay.go Outdated
…-window replay

Review findings on the recovery design: a per-tx pool success discarded
which endpoints failed, so an endpoint with isolated 5xx/transport
failures (below the breaker threshold) never got those txids and no
replay ever ran; the recovery replay's status-timestamp window missed
rows whose registration was old; and a /watch failing during that replay
was dropped.

The pool now keeps a bounded per-endpoint catch-up queue: every
endpoint-specific failure behind a pool-level success (refused, timed
out, or skipped while the breaker was open) is queued and re-sent from
the background loop once the endpoint answers, with backoff; failures go
back to the queue. On overflow the oldest entries are dropped and, once
the queue drains, propagation runs a full lookback replay against that
endpoint alone (Recoverable.OnResyncNeeded replaces OnRecovered).
Endpoint() now returns a Service that honors the breaker and queues its
failures, so the resync replay cannot lose a registration either.

Also: probe outcomes are classified (err_5xx for a 5xx /health, canceled
for the caller's context) instead of all counting as err_network, a new
arcade_merkle_endpoint_catchup_pending gauge and /health field expose the
queue, and the README's PromQL example is valid.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@galt-tr
galt-tr force-pushed the feat/multi-merkle-service branch from ea591f4 to 413da31 Compare October 9, 2026 02:04
Copilot AI balanced review requested due to automatic review settings October 9, 2026 02:04

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.

Comment thread services/propagation/replay.go Outdated
Comment thread docs/merkle-lifecycle-durability.md Outdated
Comment thread merkleservice/pool.go Outdated
Comment thread merkleservice/pool.go Outdated
Comment thread metrics/metrics.go Outdated
@mrz1836 mrz1836 assigned galt-tr and unassigned mrz1836 Oct 9, 2026

@mrz1836 mrz1836 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TLDR LGTM

@oskarszoon oskarszoon 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.

Request changes. Queuing per-endpoint misses fixes the silent partial failures from the first round: a 500/429/401/400 on one endpoint now lands in that endpoint's queue and retries. The headline availability claim still doesn't hold at the moment an endpoint goes down, though.

P1

  • merkleservice/pool.go:426-440 (RegisterBatchWithResults): each endpoint goroutine runs its whole client batch before record, and wg.Wait gates the merge on the slowest endpoint. So when an endpoint black-holes (hangs rather than refuses), the in-flight batch waits ceil(N/merkle_concurrency) × client timeout before any tx is released, even though the healthy endpoint accepted them in milliseconds. That's about 30 s × N/10 per propagation batch at defaults, and much longer for a 1000-tx replay batch. The breaker only trips after the batch, so the second batch is fast. Measured: live + hanging endpoint, 12 txs, concurrency 2, 200 ms timeout gives 1.21 s on the first batch and 4 ms on the second. Fix: record per request and check the breaker before each tx (or cancel a per-endpoint ctx when it trips), or return once any endpoint has accepted and finish the rest in the background.

Also before merge

  • sendCatchup has the same shape. It records only after the whole 500-tx catch-up batch, so a hanging endpoint with a non-empty queue blocks the pool's single background loop. The P1 fix needs to cover this path.
  • The miss queue is in-memory. The startup replay skips rows stamped merkle_registered_at within 30 min, and that stamp is set as soon as any endpoint accepts, so a registration still owed to the lagging endpoint is lost across a restart or deploy. The docs' "a restart loses nothing durable" doesn't hold at default config.
  • Permanent 4xx/401/403 entries are re-queued forever with only the gauge as a signal, and nothing tests the re-queue of a failed catch-up entry (removing it at pool.go:714 passes the suite).

…arts

Addresses the second review round on #362.

- Pool: send each endpoint's batch request by request and record every
  result as it lands, so the breaker can open mid-batch; once open, the
  rest of the batch is skipped (ErrNoHealthyEndpoints) and owed to the
  endpoint. Shared by the fan-out, catch-up and Endpoint() paths. A hanging
  endpoint now costs the batch that trips it only the requests in flight.
- Catch-up drains run on a goroutine per endpoint (one at a time), so a
  hanging endpoint no longer pins the probe loop.
- Catch-up drops registrations the endpoint rejected outright (4xx other
  than 401/403/408/429) with a warning; 401/403 stay queued and are logged.
  In-flight catch-up entries count as pending until settled.
- catchup_pending gauge is refreshed on every queue change, including
  refusals while the breaker is open.
- merkle_replay_skip_recent_minutes defaults to -1 (auto): no skip with
  several merkle-services, since merkle_registered_at is stamped once any
  endpoint accepted and the catch-up queue is lost on restart; 30 with one.
- Replay bounds the lookback window itself (Aerospike ignores since).
- A resync pass whose store scan failed is retried with backoff instead of
  being consumed.
- Reject merkle_service URLs with a query or fragment.
- Docs: auth_error semantics with mixed failures, op="catchup", restart
  behaviour, breaker timing.
…n parallel

/health can answer while /watch hangs, so closing on /health alone reopened
the endpoint every tick and stalled the next batch for a full timeout. The
probe now re-sends one owed registration first. Open endpoints are probed
concurrently. Alert guidance uses min by (endpoint), since every process
keeps its own pool view.
Copilot AI balanced review requested due to automatic review settings October 14, 2026 16:02
@galt-tr

galt-tr commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

@oskarszoon thanks, all four points are addressed in 25be7ec and 3a36cd3:

  • P1 hanging endpoint: each endpoint's batch now records results one request at a time and checks the breaker before every send. Once it trips, the rest is skipped and owed to the endpoint. Your measurement is now a test (TestPool_HangingEndpointDoesNotHoldTheBatch: 12 txs, concurrency 2, 200 ms timeout). The first batch waits two timeout rounds instead of six, and the second doesn't wait at all. Remaining cost: the batch that trips the breaker still waits for the requests already in flight (up to one client timeout). I went with this instead of returning early and finishing in the background, because that would need detached contexts and rules for late results, with feat(bump-builder): idempotent block processing across several merkle-services #363 stacked on top. In addition, a breaker now only closes after a trial /watch of an owed registration answers, since /health can be up while /watch hangs.
  • sendCatchup: uses the same per-request runner, and each endpoint drains on its own goroutine, so a hanging endpoint no longer blocks the loop. Probes run in parallel too.
  • Restart: merkle_replay_skip_recent_minutes now defaults to -1 (auto): no skip with several merkle-services, 30 minutes with one. The docs no longer claim that a restart loses nothing.
  • Permanent 4xx: catch-up drops a registration the endpoint rejected outright (a 4xx other than 401/403/408/429) and logs a warning. 401/403 stay queued and log a warning about the auth token. TestPool_FailedCatchupIsRequeued covers re-queueing: removing the re-add makes it fail. So does TestPool_CatchupDropsPermanentRejectionsOnly.

I also merged main into the branch, without a rebase, so there was no force-push. #363 will need this branch merged into it.

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.

Comment thread merkleservice/pool.go Outdated
return nil
}
out := make([]error, len(registrations))
targets := p.closedEndpoints(opWatch)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c6111ff: an endpoint skipped for an already-open breaker now counts len(registrations) skipped_open samples for /watch (1 for /reprocess). Test: TestPool_SkippedOpenCountsEveryRegistration.

Comment thread services/propagation/replay.go Outdated
Comment on lines +207 to +210
// next boot re-registers them all again. A failed entry is not
// retried here: the pool keeps it in the endpoint's catch-up queue
// (startup: for every endpoint that did not accept it; resync: for
// the single target endpoint).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c6111ff: new Recoverable.Retain queues every tx no endpoint accepted (excluding permanent 4xx) into every endpoint's catch-up queue; the startup replay hands its failed subset back through it. Not stamped merkle_registered_at. Test: TestRunMerkleReplay_RetainsTxsEveryEndpointRefused.

Comment thread metrics/README.md Outdated
Comment on lines +89 to +92
Every arcade process runs its own pool, and processes that never send
`/watch` (api-server, watchdog, bump-builder) keep reporting 1, so a
`sum()` across pods never drops below the endpoint count. The propagation
pods' series are the authoritative view.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 0d61935: the README now says any process that calls /watch or /reprocess can report 0, only one that has made neither stays at 1, and the alert is scoped to the propagation pods.

…istered

RegisterBatchWithResults waited for every endpoint before returning, so a
single hung merkle-service (black-holed, or a stopped container whose IP
vanished) held each /watch batch - and with it broadcast - for the full
30s HTTP timeout although the other endpoints had accepted every tx in
milliseconds. Found by chaos testing 3 merkle-services: with one stopped,
submit->SEEN p95 went from 9.8s to 34s until the breaker tripped 33s in.

Once every tx of a batch is accepted somewhere, the remaining endpoints get
stragglerGrace (5s, the trial-/watch bar) and are then canceled. Results are
still recorded against the caller's ctx, so the canceled in-flight calls
count against the straggler's breaker (it trips on its first batch) and the
txs it did not acknowledge go to its catch-up queue as before. With no
endpoint accepting, nothing is cut short.

@galt-tr galt-tr left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Chaos test: 3 merkle-services, one/two/all down

Stack: 3 teranodes (regtest), arcade built from this PR + #363 stacked, 3 merkle-services (merkle_service.urls), wallet-toolbox blaster at 2 tx/s for 4 min per scenario, a block every 30s. Each fault starts 60s into the send and is healed 90s later. Per-tx status transitions were polled from arcade at 1s resolution; arcade /health merkle_endpoints[] was sampled every second. Harness: chaos-test feat/multi-merkle-chaos.

The redundancy contract holds. No tx was lost in any scenario: every tx reached MINED with 1, 2 or all 3 endpoints down (≈480–620 txs per run). With all three down, 0 of 181 txs submitted during the outage were broadcast before an endpoint came back (F-024 intact); 135 parked at PENDING_RETRY and all were mined after the restore. Catch-up queues (up to ~260 owed) drained within ~10–20s of an endpoint returning.

Latency was the problem, and 0366c51 fixes most of it. Submit→SEEN / submit→MINED p95 for txs submitted during the fault:

scenario before (3a36cd3) SEEN / MINED after (0366c51) SEEN / MINED breaker opened after
baseline, no fault 9.8s / 34.3s — —
1 down (stop) 34.1s / 60.1s 11.7s / 36.2s 33s → 13s
1 down (pause, black hole) 64.8s / 92.3s 14.4s / 38.9s 40s → 10s
2 down 71.1s / 98.1s 19.5s / 37.0s 34s, 45s → 16s, 18s
1 flapping (stop/start every 12s ×4) 23.3s / 48.8s (breaker never tripped) 21.1s / 41.2s (trips and recovers each cycle) — → 11s
3 down 144.9s / 157.4s (parked until restore, by design) 130.9s / 143.9s —

Under podman a stopped container's IP disappears, so connections to it hang rather than being refused: "stop" here measures the hung/black-holed endpoint class, like "pause". In k8s a deleted pod usually answers connection-refused quickly and would not stall like this; a partitioned or wedged one would.

Inline: the fix (0366c51) and three follow-ups the runs point at. Not inline because the code isn't in this diff:

  • Recovery after an all-down outage takes ~45–55s after the first endpoint is back (breaker closes in ~7s). The rest is parked txs waiting out the PENDING_RETRY backoff (5s doubling to 300s, ±20%) and the 30s reaper tick: parkExhaustedMerkleRegistrations → schedulePendingRetry. All 181 were re-broadcast in one sweep 45s (after) / 54s (before) after the restore. Suggest a follow-up: when a breaker closes while no other endpoint was healthy, pull merkle-parked rows' next_retry_at forward (or trigger one drainParkedRetries). Not done here because it needs a store query that bypasses the schedule.
  • The pinned merkle-service image in the harness sends BLOCK_PROCESSED without the #195 enrichment, so every build fetches the block from a datahub. The first fetch usually 404s and the retry waits the 5s grace window again, adding ~10s to every MINED. Unrelated to this PR.

Comment thread merkleservice/pool.go
// so a black-holed endpoint trips on its first batch instead of holding every
// batch for a full HTTP timeout. What the stragglers did not acknowledge
// lands in their catch-up queue through the caller's usual path.
func (p *Pool) collectEndpointBatches(ctx context.Context, targets []*endpoint, registrations []Registration, maxConcurrency int) map[*endpoint][]error {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Chaos finding → fixed in 0366c51. Before this, RegisterBatchWithResults did a plain wg.Wait() on every endpoint. With one of three merkle-services black-holed, every /watch batch (and the broadcast behind it) waited the full 30s client timeout, even though the other two had accepted every tx in ~100ms. The breaker only tripped once 3 of those timeouts landed (~33–40s in), so the whole window was stalled: SEEN p95 went from 9.8s to 34s (stop) and 65s (pause), and to 71s with two endpoints down.

Now, once every tx has a success, the remaining endpoints get stragglerGrace (5s, the same bar as trialWatch) and are then canceled through sendCtx. record still classifies against the caller's ctx, so those cancellations count as err_network, and the straggler trips on its first batch (10–16s instead of 33–45s). Txs it never acknowledged reach its catch-up queue through the existing out[j]==nil path. With no endpoint accepting, nothing is cut short. After the fix: SEEN p95 11.7s / 14.4s / 19.5s for stop / pause / 2-down. Tests: TestPool_StragglerDoesNotHoldTheBatchOnceEveryTxIsAccepted, …SingleHangingEndpointStillWaitsForItsTimeout, …SlowEndpointWithinGraceIsNotPenalized.

Comment thread merkleservice/pool.go
}
out := make([]error, len(registrations))
targets := p.closedEndpoints(opWatch)
if len(targets) == 0 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correlated 5xx turns into a pool-wide outage. First pass of the harness had the merkle-services on sqlite; under 2 tx/s all three returned 500 (SQLITE_BUSY) in the same ~ms whenever a block landed, because they process the same block at the same moment. Each endpoint opened its breaker 50+ times in 10 minutes with no fault injected, often all three at once. This branch then failed every tx fast with ErrNoHealthyEndpoints until a probe tick closed one: 34 txs parked at PENDING_RETRY, and the watchdog logged /reprocess request failed … no healthy endpoints for blocks it was trying to recover.

The sqlite part was the harness (now Postgres), but correlated failure across endpoints is the normal shape here (same blocks, same datahubs). Suggestion: when every breaker is open, send to the open endpoints anyway, bounded by defaultProbeTimeout, rather than refusing locally. A success closes the breaker through record; a black hole costs 5s per batch, and that only while everything is down anyway. Same for Reprocess (line 420).

Comment thread merkleservice/pool.go
ep.mu.Lock()
ep.consecutiveFailures++
ep.lastFailureAt = p.now()
trip := !ep.open && ep.consecutiveFailures >= p.threshold

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Related: consecutiveFailures counts in-flight requests, so with merkle_concurrency 10 a single sub-millisecond blip (the SQLITE_BUSY burst above) produces 3+ failures from one instant and opens the breaker. Consider counting all failures of one batch as one strike, or requiring the streak to span some minimum time, so the threshold measures sustained failure. The black-hole case still trips on its first batch with 0366c51, because the canceled stragglers come from separate requests over ≥5s.

Comment thread app/app.go
// any endpoint accepts it, and a per-endpoint breaker keeps a dead
// merkle-service from adding its timeout to every batch. Start runs
// the /health probe that closes breakers again.
pool := merkleservice.NewPool(endpoints, cfg.MerkleService.AuthToken, 0)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

0 → Client's 30s defaultTimeout. That number sets the cost of every hung-endpoint path: before 0366c51 it was the stall per batch, and it is still the stall for a single-endpoint deployment and for the all-down case until breakers trip (~31–38s here). Worth exposing as merkle_service.timeout_ms (same default) so operators can tighten it; /watch answers in ~100–300ms on a healthy service. Follow-up, not blocking.

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.

🟡 Changes recommended

Publication recovery can lose MINED notifications, and endpoint aggregation has unresolved timeout and retry-classification issues.

5 open findings
Previously missed (1)

In code that hasn't changed since last review

Medium severity Skipped open endpoints incorrectly produce terminal reprocessing errors

merkleservice/​pool.go:435

Open endpoints are omitted from errs. If A is temporarily open and B returns 404, this surfaces a terminal ReprocessError even though A may accept the block after recovery. The watchdog applies terminal backoff instead of transient retry (watchdog.go:404–411), and the API returns 422. Include a transient failure for skipped endpoints when merging; an actual success should still win.

🧠 Review effort: Balanced

Comment thread services/bump_builder/builder.go Outdated
// status is not MINED.
func (b *Builder) markMinedAndPublish(ctx context.Context, logger *zap.Logger, blockHash string, blockHeight uint64, txids []string) bool {
_, err := setMinedAndPublish(ctx, logger, b.store, b.publisher, blockHash, blockHeight, txids, "", false)
_, err := setMinedAndPublish(ctx, logger, b.store, b.publisher, blockHash, blockHeight, txids, "", true)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in bea148e + 21c4906 without an outbox: processed_at is stamped only after the publish, so dedup (onlyChanged) now applies only when the block was already stamped before this delivery touched it; otherwise the stored set is republished in full. A failed PublishBulk now also withholds the stamp, so the watchdog's re-drive republishes. Tests: TestBuilder_HandleMessage_RedeliveryOfUnstampedBlock_RepublishesMinedRows, …PublishFailure_LeavesBlockUnstamped.

Comment thread merkleservice/pool.go Outdated
results := make(chan endpointResult, len(targets))
for _, ep := range targets {
go func() {
results <- endpointResult{ep, p.runEndpointBatch(ctx, sendCtx, ep, opWatch, registrations, maxConcurrency)}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c6111ff: runEndpointBatch reports each acceptance as it lands and the collector starts the grace once per-tx acceptances cover the batch. Test: TestPool_ComplementaryAcceptanceStartsTheGrace.

Copilot AI balanced review requested due to automatic review settings October 15, 2026 20:52

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.

🟡 Changes recommended

Credential exposure, incorrect recovery error classification, and unsynchronized health metrics remain unresolved.

6 open findings
Previously missed (2)

In code that hasn't changed since last review

Medium severity Include skipped endpoints in error merging

merkleservice/​pool.go:614

Endpoints skipped because their breakers are open are omitted from both error merges. If A is unavailable and B returns 404 for /reprocess, the pool reports a terminal 404, so the watchdog applies terminal backoff and consumes its attempt budget instead of retrying transiently. /watch can likewise report an authentication failure despite another endpoint being unavailable. Include ErrNoHealthyEndpoints for skipped endpoints in both merges, while preserving success when any endpoint accepts.

Medium severity Update healthy gauge while holding breaker lock

merkleservice/​pool.go:720

The healthy gauge is written after releasing ep.mu in both breaker transitions. An opening request can unlock here, be overtaken by closeBreaker writing 1, and then write 0. The breaker is closed but the metric remains unhealthy until another transition; the reverse ordering can hide an open breaker. Move both gauge writes inside the locked sections that update ep.open.

🧠 Review effort: Balanced

Comment thread config/config.go
Comment on lines +1478 to +1480
if (parsed.Scheme != "http" && parsed.Scheme != "https") || parsed.Host == "" {
return fmt.Errorf("merkle_service url %q must be an absolute http(s) URL", u)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 71dd3c3: URLs with user info are rejected at config validation (the error redacts the password); auth_token is the supported mechanism. Test: TestValidate_MerkleServiceRejectsUserinfo.

Copilot AI balanced review requested due to automatic review settings October 16, 2026 13:20

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

… refused startup replays

- collectEndpointBatches starts the straggler grace from per-tx acceptances,
  so complementary coverage (A takes tx1, B takes tx2, each hangs on the
  other) no longer waits for the HTTP timeout.
- An endpoint skipped for an already-open breaker counts one skipped_open
  per registration, matching mid-batch skips; /reprocess still counts one.
- Pool.Retain (Recoverable) queues txs no endpoint accepted for every
  endpoint; the startup replay hands its refused entries back through it
  instead of dropping them until the next restart.
The endpoint URL is shown in /health, metric labels and startup logs, so
credentials embedded in it would leak there. auth_token is the supported
mechanism; the validation error redacts the password.
…very finalized

onlyChanged=true dropped the MINED event of a row whose SetMinedByTxIDs
landed but whose PublishBulk never ran (crash in between): the redelivery
saw it already MINED and stamped processed_at without publishing. Stamping
happens after publishing, so dedup now applies only when processed_at was
set before this delivery touched the block; otherwise the stored set is
republished in full.
…lish; don't queue permanent rejections

- markMinedAndPublish reports a PublishBulk failure as incomplete, so the
  block stays un-stamped and the watchdog's re-drive republishes it. Without
  this a stamped block could carry unpublished MINED rows that the next
  delivery's onlyChanged filter then suppresses for good.
- endpointService no longer queues a 4xx rejection for catch-up, matching
  sendCatchup and Retain.
Copilot AI balanced review requested due to automatic review settings October 16, 2026 14:15

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@oskarszoon oskarszoon 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.

The four points from my last review are fixed at 21c4906:

  • Black-holed endpoint: runEndpointBatch checks the breaker before each send and records per request, and the straggler grace cancels a hung endpoint once every tx has an acceptance. With one live and one hanging endpoint, the first batch now costs one grace (5s) and later batches skip the hung endpoint. With the isOpen check mutated out, TestPool_HangingEndpointDoesNotHoldTheBatch fails.
  • Catch-up drains per endpoint, so a hung /watch no longer holds the loop.
  • merkle_replay_skip_recent_minutes: -1 closes the restart gap.
  • Catch-up drops permanent 4xx, and tests pin the re-queue.

Holding approval: bea148e (onlyChanged = finalized) is this PR's commit, and it loses the MINED event when a publish fails on an already-stamped block. Details are in my #363 review (services/bump_builder/builder.go:1571-1583, shortCircuit). An approve here would likely auto-merge that into main before the fix lands. I'll approve once it's fixed on either PR.

Follow-ups, none blocking:

  • merkleservice/pool.go:431 Reprocess: an endpoint skipped for an open breaker is left out of mergeErrors. With A open and B answering 404, B's terminal *ReprocessError wins and the watchdog applies the 4h terminal backoff, even though A may simply be down. The same applies to RegisterBatchWithResults: a 401 from B reads as auth_error while A is down. Count each skipped endpoint as ErrNoHealthyEndpoints in the merge. Copilot raised this too.
  • pool.go:563 collectEndpointBatches: the grace also trips healthy endpoints that are only slower. Example: A at 10ms per request, B at 80ms, 1000 txs at concurrency 10. B is canceled at 5s, marked unhealthy, and owes 270 entries. Consider not counting grace cancellations against an endpoint that acknowledged at least one request in this batch. A hung endpoint acknowledges none, so it still trips on its first batch.
  • config/config.go:1476,1485,1490: only the userinfo branch redacts. A parse error, a scheme-less svc:s3cret@host:8080, or a ?token=… URL is echoed raw in the startup error.
  • app/app.go:198: single-url deployments now get the breaker as well. Three in-flight 5xx trip it, and registrations are then refused locally for up to one probe tick. That's a fair trade against the old 30s-per-batch stall, but state it in the PR body and docs.
  • config.example.yaml:141: urls: [] followed by list items is still invalid YAML once uncommented. Use the # urls: form from compose/arcade-config.yaml.

bea148e suppressed MINED events for rows already MINED against a stamped
block. On this branch a short-circuit whose mine or publish fails on an
already-stamped block cannot take the stamp off (no ClearBlockProcessed
here), so the next delivery would filter out MINED rows whose event never
went out. The short-circuit now passes onlyChanged=false, as main does: a
duplicate MINED per extra delivery instead of a lost one. The multi-source
STUMP work (#363) re-enables the dedup together with the un-stamp.
…points from grace cancels

- An endpoint skipped up front for an open breaker now takes part in the
  per-tx and /reprocess merge as ErrNoHealthyEndpoints, so a 404 or 401
  from the endpoints that answered is not the pool's verdict while another
  endpoint may simply be down.
- A request the straggler grace cancels counts against the endpoint's
  breaker only if it acknowledged nothing in the batch: a black-holed
  endpoint still trips on its first batch, a merely slower one keeps its
  health and owes the rest through catch-up.
…s; document the single-url breaker

- Parse errors, scheme-less user:pass@host (opaque), query and fragment
  URLs no longer echo raw in the startup error.
- config.example.yaml uses the commented '# urls:' form so uncommenting
  yields valid YAML.
- getting-started and the example config state that a single url is also
  behind the breaker, that /health shows the answering process's own pool,
  and that a duplicate BLOCK_PROCESSED can repeat MINED.
Copilot AI balanced review requested due to automatic review settings October 16, 2026 19:07

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

galt-tr added a commit that referenced this pull request Oct 16, 2026
Resolution: #362 turned the short-circuit's MINED dedup off (11cf909)
because it cannot un-stamp a block there. This branch keeps the dedup
(onlyChanged = blockFinalized) since shortCircuit un-stamps on a failed mine
or publish (06d27b2), and keeps its PublishesOnlyNewlyMined /
AllMined_PublishesNothing tests and getting-started wording.
@galt-tr

galt-tr commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Re: #362 (review) (head 68135b1)

Blocking, bea148e (11cf909): this PR can't take the stamp off a block, since ClearBlockProcessed is only added in #363. So on this branch tryShortCircuit passes onlyChanged=false again and republishes the stored set, as main does. The cost is a duplicate MINED per extra delivery instead of a lost one. #363 keeps the dedup together with the un-stamp from 06d27b2. I resolved the merge (9e9b571) by hand so #363 keeps finalized, because git would otherwise have carried this branch's false into #363 silently. The fresh-build path still passes blockFinalized. That's safe here: rows it mines aren't yet MINED against the block, and any later delivery takes the short-circuit, which republishes everything. Test: TestBuilder_HandleMessage_DuplicateBlockProcessed_RepublishesStoredSet. The PR body is updated to match.

Follow-ups:

  • Skipped endpoints in the merge (6826a2b): an endpoint skipped up front for an open breaker now joins both the /reprocess merge and the per-tx /watch merge as ErrNoHealthyEndpoints. With A open, B's 404 or 401 no longer becomes the verdict. Test: TestPool_SkippedOpenEndpointOutranksTerminalAnswers.
  • Grace cancellations against slower endpoints (6826a2b): I went with your suggestion. A request the grace cancels is recorded as canceled when the endpoint acknowledged at least one request in that batch. A black-holed endpoint acknowledges none, so it still trips on its first batch (TestPool_StragglerDoesNotHoldTheBatchOnceEveryTxIsAccepted). New test: TestPool_SlowerEndpointCutByGraceKeepsItsHealth.
  • Config redaction (68135b1): every merkle_service URL error now goes through redactURL. It masks user info, opaque parts (scheme-less svc:s3cret@host:8080), query and fragment, and a URL that doesn't parse isn't echoed at all. Test: TestValidate_MerkleServiceErrorsRedactSecrets.
  • Breaker on single-url deployments (68135b1): this is now stated in the PR body, docs/getting-started.md and config.example.yaml. The docs also say that /health shows the answering process's own pool.
  • config.example.yaml urls (68135b1): now uses the commented # urls: form.

@oskarszoon oskarszoon 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.

Approving at 68135b1. 11cf909 resolves my hold: tryShortCircuit passes onlyChanged=false again, the same as main, so this no longer carries the lossy dedup into main. The other follow-ups also check out:

  • Skipped-open endpoints now take part in both merges.
  • A grace cut no longer trips an endpoint that acknowledged requests in the batch.
  • Every config error goes through redactURL.
  • The single-URL breaker is documented.
  • The example YAML parses once uncommented.

Mutating each fix makes its new test fail.

Two follow-ups, neither blocking:

  • services/bump_builder/builder.go:737,865: the fresh build still passes blockFinalized, so it is now the only dedup on this branch. Suppose a stamped block's short-circuit fails to publish a late tx, and the next delivery falls through to the rebuild because GetBUMP errored or the stored BUMP is corrupt, with the STUMP set complete. That rebuild then filters the tx out, and its MINED event is lost. It takes three faults at once, but passing false here costs nothing, because finalized is already false on an unstamped block. Let #363 re-enable it together with ClearBlockProcessed.
  • docs/merkle-lifecycle-durability.md:143 (and :48) still say the short-circuit runs with onlyChanged=true and doesn't republish rows that are already MINED.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature Any new significant addition size/XL Very large change (>500 lines)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants