Skip to content

feat(bridgeservice): sync status for all syncers with is_halted (#1861) - #1877

Merged
arnaubennassar merged 11 commits into
developfrom
feat/sync-status-all-syncers
Oct 2, 2026
Merged

arnaubennassar merged 11 commits into
developfrom
feat/sync-status-all-syncers

Conversation

@arnaubennassar

@arnaubennassar arnaubennassar commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

🔄 Changes Summary

  • Add three new /bridge/v1/sync-status entries: l1_info_tree_info, claim_l1_info and claim_l2_info, so all six syncers used by a bridge-service instance (bridgesync L1/L2, l2gersync, l1infotreesync, claimsync L1/L2) are now reported, not just the first three.
  • Add is_halted to every sync-status entry (l1_info, l2_info, l2_ger_info and the three new ones). It is false for syncers with no halt state, and false (alongside is_active:false) for a syncer that isn't configured on this instance — the two cases are distinguished by which entries are present, per the docs.
  • Add a per-entry error (redacted via the existing aggkitcommon.RedactError helper from fix(bridgetracker): redact error URLs + gate bridgeservicefinder auto-registration #1862: URLs and hosts are stripped) instead of failing the whole request on the first component error.
  • /health gains matching details keys l1_info_tree, claim_l1 and claim_l2, plus is_halted on every details.* entry. /health is derived from the exact same sync-status computation as before (pure function over the result), so the two endpoints can never drift.
  • computeSyncStatus now computes all six components concurrently instead of sequentially, and a panic in a single component's goroutine is logged with its original stack before being re-raised.
  • Add L1InfoTreeSync.IsActive(ctx) bool to l1infotreesync, mirroring bridgesync.BridgeSync.IsActive.
  • Fix a regression introduced (and caught) inside this branch: cmd/run_bridgeservice.go was passing nil concrete syncer pointers into interface parameters, which produces a non-nil interface holding a typed nil. On any instance without a claim or l1infotree syncer configured, both /bridge/v1/sync-status and /health panicked and returned 500. Nil concrete pointers are now converted to untyped nil interfaces before being wired in.
  • Fix bridgesync.GetContractDepositCount, l1infotreesync.GetLastProcessedBlock and l2gersync.GetLastProcessedBlock to honour the caller's ctx: the bridgesync getter now passes ctx via bind.CallOpts, and the two SQLite getters use QueryRowContext instead of QueryRow/meddler.QueryRow. The /bridge/v1/sync-status read timeout and /health compute timeout now actually bound these calls, which previously ignored ctx. Also removes the unused l2gersync.BlockNum helper type.

⚠️ Breaking Changes

  • 🛠️ Config: N/A
  • 🔌 API/CLI: The JSON response shape is additive only — no existing field is renamed, removed, or repurposed. The behavioural change: GET /bridge/v1/sync-status now always answers 200, with a per-entry error string, instead of returning 500 on the first failing component. GET /health details.*.error text is now redacted (hosts/URLs stripped) instead of a raw error. Clients that previously treated a 500 from /bridge/v1/sync-status as their failure signal must switch to inspecting each entry's error/is_halted field; a 500 is no longer produced by this endpoint for component failures.
  • 🗑️ Deprecated Features: None.

📋 Config Updates

  • None.

🔌 API Updates

🔌 Bridge service API

  • GET /bridge/v1/sync-status: added l1_info_tree_info, claim_l1_info, claim_l2_info; added is_halted to every entry; added error to every entry; the endpoint now always responds 200 instead of 500 on a component failure. Not a breaking interface change apart from the 200-vs-500 status behaviour described above — no field was removed or renamed.
  • GET /health: added details.l1_info_tree, details.claim_l1, details.claim_l2; added is_halted to every details.* entry; error text in details.*.error is now redacted. Still always returns HTTP 200. Not a breaking interface change.

🔌 Proxy API

  • None.

🔌 Others API

  • None.

✅ Testing

  • 🤖 Automatic: New unit tests in bridgeservice/bridge_test.go (TestSyncStatusAllSyncers, TestHealthFromSyncStatus, TestSyncStatusRedactsErrors, TestComputeSyncStatus_ComponentsRunConcurrently, TestComputeSyncStatusPanicPropagates), a real halted-processor test in l1infotreesync/bridgeservice_halt_test.go (TestBridgeServiceReportsL1InfoTreeHalt), the cmd/run_bridgeservice_test.go nil-wiring regression tests, and a bridgetracker/sources/activity_test.go regression test (TestIsNetworkSynced) confirming an erroring network still reports not-synced. New e2e test TestBridgeServiceSyncStatusAllSyncers in test/e2e/sync_status_all_syncers_test.go, plus an extended TestBridgeServiceHealthSyncStatus.

  • Regression tests for the ctx fix, verified to fail on the pre-fix code: TestGetContractDepositCount_HonoursContextDeadline in bridgesync (a blocked eth_call), and TestGetLastProcessedBlock_HonoursContext in both l1infotreesync and l2gersync (an already-cancelled ctx).

  • 🖱️ Manual: golangci-lint v2.4.0 reported 0 issues. make test-unit passed. E2E run (AGGKIT_E2E_ENV=anvil-2chains make test-e2e TEST_RUN='^(TestBridgeServiceSyncStatusAllSyncers|TestBridgeServiceHealthSyncStatus)$') passed for both L2A and L2B: --- PASS: TestBridgeServiceHealthSyncStatus (3.51s), --- PASS: TestBridgeServiceSyncStatusAllSyncers (9.03s).

    Before (develop today): /bridge/v1/sync-status returns only l1_info, l2_info and l2_ger_info — no l1_info_tree_info, claim_l1_info or claim_l2_info, and no is_halted anywhere. A single failing component (e.g. an RPC blip on l1infotreesync) causes the whole endpoint to answer HTTP 500 instead of returning what it could compute.

    After (this branch), real payload from the e2e env:

    GET /bridge/v1/sync-status

    {
      "l1_info": {"contract_deposit_count":2,"synchronized_deposit_count":2,"is_synced":true,"is_active":true,"is_halted":false},
      "l2_info": {"contract_deposit_count":0,"synchronized_deposit_count":0,"is_synced":true,"is_active":true,"is_halted":false},
      "l2_ger_info": {"is_active":true,"last_processed_block":100,"is_halted":false},
      "l1_info_tree_info": {"is_active":true,"is_halted":false,"last_processed_block":329},
      "claim_l1_info": {"is_active":true,"is_halted":false,"last_processed_block":329},
      "claim_l2_info": {"is_active":true,"is_halted":false,"last_processed_block":234}
    }

    GET /health

    {
      "status": "ok",
      "time": "2026-09-29T07:33:26.425307489Z",
      "version": "v0.11.0-rc10-13-gaf2f650d",
      "sync_status": "done",
      "details": {
        "l1": {"is_active":true,"is_synced":true,"is_halted":false},
        "l2": {"is_active":true,"is_synced":true,"is_halted":false},
        "l2_ger": {"is_active":true,"is_halted":false},
        "l1_info_tree": {"is_active":true,"is_halted":false},
        "claim_l1": {"is_active":true,"is_halted":false},
        "claim_l2": {"is_active":true,"is_halted":false}
      }
    }

🐞 Issues

🔗 Related PRs

📝 Notes

  • Halt info is exposed as a boolean only (is_halted), with no reason or halted block, because those strings can carry DB/RPC internals — this follows the same redaction policy as fix(bridgetracker): redact error URLs + gate bridgeservicefinder auto-registration #1862.
  • Claimsync, l1infotreesync and l2gersync entries never produce pending in /health's aggregation: they have no in-service "caught up" signal the way the L1/L2 bridge syncers do (claimsync syncs on demand via SetNextRequiredBlock). They only ever feed the error bucket or done.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GRC3mRecjKCrgAHRcDnWuo

arnaubennassar and others added 8 commits September 29, 2026 08:21
Add a public IsActive(ctx) method to L1InfoTreeSync mirroring
bridgesync.BridgeSync.IsActive, so callers can tell whether the
syncer's processor is currently halted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GRC3mRecjKCrgAHRcDnWuo
…erfaces

Extend L1InfoTreeSyncer with IsActive and GetLastProcessedBlock, and
Claimer with GetLastProcessedBlock, so the sync-status and health
handlers can report halt state and progress for these syncers.
Regenerate mocks accordingly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GRC3mRecjKCrgAHRcDnWuo
…ypes

Add IsHalted and Error to NetworkSyncInfo and L2GERSyncInfo, and a new
SyncerSyncInfo type used by three new SyncStatus entries
(l1_info_tree_info, claim_l1_info, claim_l2_info). Add IsHalted to
ComponentHealth and matching new keys (l1_info_tree, claim_l1,
claim_l2) to HealthCheckDetails. All additive; no existing JSON tag is
renamed or removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GRC3mRecjKCrgAHRcDnWuo
Compute every configured syncer independently in computeSyncStatus, so
one component's failure no longer blanks the others or turns the whole
request into a 500. GetSyncStatusHandler now always answers 200 with a
per-entry, redacted error instead. Populate the three new entries
(l1_info_tree_info, claim_l1_info, claim_l2_info) and is_halted on
every entry. Derive /health details from the same result, adding
l1_info_tree, claim_l1 and claim_l2 alongside is_halted, and aggregate
sync_status from halt/error state across all six components.

Also fix cmd/run_bridgeservice.go so that a nil concrete syncer is
converted to an untyped nil interface before being wired into
BridgeService: passing a nil concrete pointer through directly created
a non-nil interface holding a typed nil, which made both endpoints
panic (and answer 500) on any instance without a claim or l1infotree
syncer configured.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GRC3mRecjKCrgAHRcDnWuo
…dling

Add table-driven tests proving the new contract: all six syncers
reporting when healthy, each halt-capable syncer reporting is_halted
independently, per-component errors staying isolated with a 200
response, unconfigured syncers omitted or defaulted, concurrent
computation, panic propagation, and error redaction across every
component. Add a real-processor halt test for l1infotreesync using new
export_test.go helpers. Add a bridgetracker regression test confirming
isNetworkSynced still treats a component error as not synced.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GRC3mRecjKCrgAHRcDnWuo
…erate swagger

Update the sync-status and health-check sections with the new entries,
is_halted semantics, the not-configured-vs-halted rule, per-entry
error and the always-200 behaviour. Regenerate swagger from the
updated handler annotations, including dropping the now-unused
@failure 500 on GetSyncStatusHandler.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GRC3mRecjKCrgAHRcDnWuo
Add TestBridgeServiceSyncStatusAllSyncers, which polls
/bridge/v1/sync-status and /health against a live env, asserts every
running syncer's entry (including the new l1_info_tree/claim_l1/claim_l2
ones), checks that last_processed_block advances, and asserts
consistency between the two endpoints. Extend
TestBridgeServiceHealthSyncStatus for the new details keys. Tighten
checks.go's connectivity pre-check to inspect per-entry errors now
that sync-status always answers 200.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GRC3mRecjKCrgAHRcDnWuo
GetContractDepositCount, l1infotreesync's GetLastProcessedBlock, and
l2gersync's GetLastProcessedBlock all ignored the caller's context,
so a hung L1 RPC call or a stuck DB read could outlive the
sync-status/health endpoint timeouts even though those timeouts were
already threaded down to these getters.

- bridgesync: pass ctx via bind.CallOpts to DepositCount instead of nil.
- l1infotreesync: use db.QueryRowContext instead of db.QueryRow.
- l2gersync: replace meddler.QueryRow (no ctx-aware variant) with
  database.QueryRowContext + Scan, dropping the now-unused BlockNum
  helper type.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GRC3mRecjKCrgAHRcDnWuo
@arnaubennassar arnaubennassar self-assigned this Sep 29, 2026
@arnaubennassar

Copy link
Copy Markdown
Collaborator Author

@claude

@claude

claude Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Claude finished @arnaubennassar's task in 5m 54s —— View job


Review: feat(bridgeservice): sync status for all syncers with is_halted

  • Gather context (read PR diff vs origin/develop)
  • Review bridgeservice/bridge.go sync-status computation + concurrency/panic handling
  • Review cmd/run_bridgeservice.go typed-nil interface fix
  • Review ctx fixes (bridgesync, l1infotreesync, l2gersync)
  • Review types/docs/swagger and tests (unit + e2e)
  • Post review + PR summary template

🚀 What's New

  • GET /bridge/v1/sync-status now reports all six syncers an instance runs: the three existing entries (l1_info, l2_info, l2_ger_info) plus l1_info_tree_info, claim_l1_info and claim_l2_info (new types.SyncerSyncInfo shape, omitted when the syncer isn't configured).
  • is_halted added to every sync-status entry and every /health details.* entry, disambiguating "not configured" from "configured but halted" for the three always-present legacy entries.
  • Per-entry error (redacted through aggkitcommon.RedactError) instead of failing the whole request: /bridge/v1/sync-status now always answers 200 and reports what it could compute.
  • /health gains matching details.l1_info_tree, details.claim_l1, details.claim_l2, still derived from the same computeSyncStatus result via the pure healthFromSyncStatus, so the two endpoints can't drift.
  • computeSyncStatus evaluates the six components concurrently; a panic in one component's goroutine is logged with its original stack, then re-raised on the caller goroutine after wg.Wait().
  • New L1InfoTreeSync.IsActive(ctx), mirroring BridgeSync.IsActive; bridgeservice.Claimer/L1InfoTreeSyncer gain GetLastProcessedBlock.
  • Docs (docs/bridge_service.md), swagger and e2e coverage updated; removes the unused l2gersync.BlockNum type.

🐛 Bug Fixes

  • cmd/run_bridgeservice.go: nil concrete syncer pointers were assigned straight into interface-typed params, producing non-nil interfaces holding typed nils. On instances without a claim/l1infotree syncer both /bridge/v1/sync-status and /health panicked → 500. Now converted via bridgerOrNil/claimerOrNil/l1InfoTreeSyncerOrNil/l2GERSyncerOrNil while the concrete type is still known. Regression-tested in cmd/run_bridgeservice_test.go.
  • bridgesync.GetContractDepositCount ignored the caller's ctx (DepositCount(nil)) → now &bind.CallOpts{Context: ctx}.
  • l1infotreesync.processor.GetLastProcessedBlock and l2gersync.processor.GetLastProcessedBlock ignored ctx (QueryRow / meddler.QueryRow) → now QueryRowContext. The sync-status read timeout and /health compute timeout actually bound these calls now.

📋 Config Updates

None.

⚠️ Breaking Changes

  • GET /bridge/v1/sync-status no longer returns 500 on a component failure — it answers 200 with a per-entry error. Clients keying on the status code must switch to inspecting each entry's error/is_halted.
  • GET /health details.*.error text is now redacted (URLs/hosts stripped) rather than raw.
  • JSON shape itself is additive only (no field renamed/removed/repurposed).
  • Not listed in the PR body but worth calling out: /health's error bucket is now wider — see 🟡 Dev Docs #4.

Review feedback

Overall this is careful, well-documented work. The concurrency is correct (distinct struct fields / distinct []any elements per goroutine, wg.Wait() supplying the happens-before, and the recover deferred after wg.Done() so panics[i] is set before Wait returns), the IsActive == !isHalted mapping matches both bridgesync.go:530 and l1infotreesync.go:354, the typed-nil fix is applied at exactly the right layer, and TestComputeSyncStatus_ComponentsRunConcurrently is a genuinely deterministic concurrency proof (channel handshake, no sleeps) rather than a timing test. docs/bridge_service.md's "Not configured vs. halted" section is in fact more accurate than the PR body, which says the two cases are distinguished by entry presence — true only for the three new entries.

No blockers found. Notes below.

🟡 Suggested

1. /bridge/v1/sync-status is uncached and now fans out 6× per request — bridgeservice/bridge.go:1662-1675

/health is protected by healthCheckCache (TTL + single-flight), but the sync-status handler calls computeSyncStatus directly on every request. Where develop did 3 sequential component reads, this now does 6 concurrent ones, so N in-flight requests on a public, unauthenticated, frequently-polled endpoint means ~6N concurrent SQLite reads plus 2N eth_calls. Consider reusing the same TTL cache (or a short one) for this handler.

Fix this →

2. sync-status is bounded by readTimeout, not the compute timeout — bridgeservice/bridge.go:1670

ctx, cancel := context.WithTimeout(c, b.readTimeout) — PublicREST.ReadTimeout defaults as high as 5 minutes (sized for large paginated bodies). Now that GetContractDepositCount actually honours ctx, a black-holed RPC endpoint holds the request and its six goroutines for up to that budget, which is the exact scenario DefaultHealthCheckComputeTimeout's doc comment argues against for /health. Bounding this handler with healthCheckComputeTimeout (or min(readTimeout, computeTimeout)) would make the two paths consistent.

3. Claim/l1infotree handlers guard the wrong field — same typed-nil class this PR fixes

GetClaimsHandler checks b.bridgeL1 == nil at bridge.go:510 then calls b.claimL1.GetClaimsPaged at :517; same pattern at :526/:533, GetUnsetClaimsHandler :584/:606, GetSetClaimsHandler :658/:680. Those fields are not nil-equivalent: runClaimSyncL2IfNeeded (cmd/run.go:1049) gates on {aggsender, aggsender-validator, aggchain-proof-gen, bridge, l2-claim-sync} while runBridgeSyncL2IfNeeded also matches l2-bridge-sync. So components = ["l2-bridge-sync"] gives bridgeL2 != nil, claimL2 == nil → nil-interface method call → panic → 500 instead of the intended 503. b.l1InfoTree is likewise used unguarded (:999, :1099, :1174, :2941, …). Pre-existing, but this PR is precisely what makes those fields genuine untyped nils, so it's a natural follow-up.

Fix this →

4. /health's error bucket silently widened — bridgeservice/bridge.go:1819-1832

l1infotreesync halting to resolve a reorg is routine, and it now flips the aggregate sync_status to error where it previously stayed pending/done. Nothing in-repo reads sync_status programmatically (I checked: bridgeservicefinder keys only on HTTP 200), so no in-repo behaviour changes — but external dashboards/alerting almost certainly do. The Breaking Changes section covers redaction and 200-vs-500 but not this; worth an explicit line in the release notes.

5. e2e pre-check is now stricter and has no retry — test/e2e/envs/checks.go:216

checkSyncStatusEntryErrors fails the one-shot env check if any of six entries carries an error. Six entries × a transient RPC/DB blip is six times the chance of a red suite at setup time, where before only connectivity mattered. Consider retrying, or restricting the check to the entries the suite actually depends on.

6. Minor style in checks.go — test/e2e/envs/checks.go:218-262

The anonymous struct field is named error, shadowing the builtin type name (errMsg reads better). And errorOf/errorOfL2GER/errorOfSyncer are three copies of the same four-line nil guard — one small generic, or inlining, would cut ~18 lines.

7. export_test.go comment overstates visibility — l1infotreesync/export_test.go:3

"letting tests outside this package" — _test.go files aren't compiled into the importable package, so HaltForTest is only reachable from package l1infotreesync and the in-directory package l1infotreesync_test (which is what bridgeservice_halt_test.go uses, so the mechanism is correct). Worth rewording to "the external test package in this directory".

🟢 Optional

8. componentNames/panics parallel-array bookkeeping — bridgeservice/bridge.go:1588-1637

The iota block, the [numComponents]string names array and the six run(...) call sites have to be kept aligned by hand (the doc comment says as much). A single []struct{name string; run func()} built once would make the name↔component pairing mechanical.

9. Waiter path in getOrCompute on a panicking compute (pre-existing) — bridgeservice/bridge.go:1888-1896

The h.inflight != nil branch returns h.value unconditionally, without re-checking h.expires. If compute() panics on the first-ever computation, waiters get the zero-valued entry → HTTP 200 with "sync_status": "" (not a valid HealthSyncStatus) and empty details. Untouched by this PR, but it's the same panic story the PR's new comments reason about, so worth a follow-up.

10. Claimsync presence isn't reported anywhere — test/e2e/sync_status_all_syncers_test.go:51-57

runningBridgeComponents / GET /bridge/v1/config don't carry claimsync flags, which is why the new e2e test has to hardcode both claim syncers as configured. Adding the two flags would let the test derive presence the same way it does for the other four — and would also help operators, given #3 above shows the two can genuinely diverge.


Verification notes

I could not execute go build, go test or golangci-lint here (those commands required approval in this environment), so I verified the mechanical correctness by reading instead:

  • bind is already imported at bridgesync/bridgesync.go:20; meddler is still used elsewhere in l2gersync/processor.go (lines 133, 151, 178, …), so neither edit orphans an import.
  • p.db / p.database are both *sql.DB, which has QueryRowContext.
  • claimsync.ClaimSync.GetLastProcessedBlock(ctx) (uint64, bool, error) (claimsync/claimsync.go:213) matches the new Claimer method exactly; L1InfoTreeSync has both new methods.
  • l2gersync.BlockNum has no remaining references repo-wide — safe removal.
  • aggkitcommon.RedactError exists at common/redact.go:95.

The PR body reports make test-unit green and golangci-lint v2.4.0 clean; I'm taking that at face value rather than re-running.
· branch feat/sync-status-all-syncers

@arnaubennassar
arnaubennassar enabled auto-merge (squash) October 2, 2026 08:17
@arnaubennassar
arnaubennassar merged commit 5c04f54 into develop Oct 2, 2026
33 checks passed
@arnaubennassar
arnaubennassar deleted the feat/sync-status-all-syncers branch October 2, 2026 09:17
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.

Add sync status for all syncers on the bridge api

2 participants