feat(bridgetracker): add SQLite-backed cache for tracker/activity state (#1822) - #1852
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff9615bf3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
ff9615b to
3575935
Compare
…ched bridges - sqlite_activity_store: saveBridgeRow was dropping ScannedBridge.Source on save, so a reload decoded it back as "" — losing the bridge-service/RPC origin fetchNewBridgesFrom and invalidatedBridges key off, letting a reorged-out RPC-sourced bridge stay cached indefinitely. - sqlite_registry: a persisted tracked_bridge row was reused on reload based solely on its schema version, never checking whether its origin block was since reorged out — so a resolved bridge accepted under LatestBlock finality could be served as terminal forever after a restart. selectRow's callers now go through selectFreshRow, which discards (and re-resolves) a row whose origin_block_hash no longer matches the chain's current hash via the optional new BlockHashVerifier port; create()'s "lost the race" fallback shares the same staleness check so it doesn't just hand back the reorged-out row it was trying to replace. proxy/cmd/run.go wires this to the tracker's existing per-network RPC clients. Addresses the two P1 findings from PR #1852's Codex review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
…te (#1822) Adds a persisted alternative to the bridge tracker's in-memory registry and activity cache, so the tracker survives a restart instead of re-resolving every bridge (and re-issuing every bridge-service/agglayer call behind it) from scratch, and can serve activity/tracking queries faster on a warm cache. - bridgetracker/db (new package, package db, mirrors aggsender/db): sqliteRegistry implements domain.SupervisedRegistry over a new tracked_bridge table; sqliteActivityStore implements domain.ActivityQuerier over new activity_address/activity_bridge tables. Both share one migration set (bridgetracker/db/migrations) and can point at the same SQLite file. - Tracking data survives via a schema_version column: a row written under a different version is treated as a cache miss and discarded rather than risking a misinterpreted decode. - Pruning/retention is deliberately left in-memory-only for now: the SQLite adapters' PruneTerminal/PruneIdle (and activity's idle sweep) are no-ops, so rows are never deleted yet. This is a conscious, temporary trade-off while the DB-side retention policy is still undecided. - bridgetracker/domain: BridgeInfo gains BlockHash (populated in sources.BridgeEventSource), and TrackingData gains IsTerminal()/ TerminallyFailed(), hoisted out of registry.go so both the in-memory and SQLite adapters share the same derivation. - bridgetracker.Config gains DBPath (empty keeps the in-memory adapters) and an Activity override field; proxy/cmd/run.go wires the SQLite-backed stores when DBPath is set, and proxy/config/default.go enables it by default (/tmp/bridgetracker.sqlite). Closes #1822.
…ched bridges - sqlite_activity_store: saveBridgeRow was dropping ScannedBridge.Source on save, so a reload decoded it back as "" — losing the bridge-service/RPC origin fetchNewBridgesFrom and invalidatedBridges key off, letting a reorged-out RPC-sourced bridge stay cached indefinitely. - sqlite_registry: a persisted tracked_bridge row was reused on reload based solely on its schema version, never checking whether its origin block was since reorged out — so a resolved bridge accepted under LatestBlock finality could be served as terminal forever after a restart. selectRow's callers now go through selectFreshRow, which discards (and re-resolves) a row whose origin_block_hash no longer matches the chain's current hash via the optional new BlockHashVerifier port; create()'s "lost the race" fallback shares the same staleness check so it doesn't just hand back the reorged-out row it was trying to replace. proxy/cmd/run.go wires this to the tracker's existing per-network RPC clients. Addresses the two P1 findings from PR #1852's Codex review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
97f2e3e to
38cbaaa
Compare
arnaubennassar
left a comment
arnaubennassar
left a comment
There was a problem hiding this comment.
Code review: SQLite-backed tracker/activity cache
Reviewed the full diff (16 files, +2594/-48). 11 findings below, posted inline: 1-3 blocking, 4-7 correctness, 8-11 lower.
The headline concern is that findings 1-3 all come from the same decision — proxy/config/default.go turns the SQLite path on by default while retention/pruning is still a no-op and the reorg check runs on every read. Shipping DBPath as opt-in would defuse all three without changing any of the storage code.
Things I checked and cleared
- JSON round-trip of the persisted shapes:
types.Duration,ErrorStep,CertificateData,agglayertypes.CertificateStatusall decode back correctly; thejson:"-"fields live only onGERData, which is never persisted inBridgeStepPath. ON DELETE CASCADEinFlushActivityworks —db.NewSQLiteDBsets_foreign_keys=on(plus WAL and_busy_timeout=30000, so the two*sql.DBhandles on one file don't deadlock).BridgeInfo.BlockHashis populated on the only production construction site (sources/bridge_event.go:146) andBridgeInfois not serialized into any API response, so no wire-format change.- Concurrent read-modify-write on
UpdateTrackingStep/UpdateTrackingBridgeTxis safe in practice:Engine.tickandresolveTriggeredrun on the same goroutine and parallelise only across distinct ids. - Index planner behaviour:
idx_tracked_bridge_terminal_sincedoes get picked forGetTrackerActives(verified withEXPLAIN QUERY PLAN), butterminal_since = 0is the selective side here, so it is not a repeat of the PR-1784 planner hijack. go build ./...on the branch is clean.
…/activity store hardening
- proxy/config/default.go: DBPath now defaults to empty (opt-in), not an enabled-by-default
/tmp path — a shared, world-writable, fixed filename any local user could pre-create or
symlink, and one that would run with pruning still a no-op and a per-read reorg check.
Doc comments for RetentionPeriod/IdleTimeout/MaxTrackedBridges now note which apply only to
the in-memory adapter.
- sqlite_registry.go:
- create() now checks for an existing (stale) row before the maxEntries capacity gate, so
re-registering an id whose own row is stale is never wrongly rejected as ErrRegistryFull
just because the registry happens to be full of that very row.
- rowIsStale now also treats an undecodable row as stale, so a corrupted row self-heals on
the next registration instead of permanently erroring every reader.
- GetTrackerActives/GetNetworks skip (and log) a row that fails to decode instead of failing
the whole call — previously one bad row wedged the engine's poll tick forever.
- UpdateTrackingBridgeTx no longer bumps last_access: it is the idle-eviction anchor and must
only move on an actual read (Get/GetAndAwait/Subscribe), not the engine's own writes.
- UpdateTrackingBridgeTx/UpdateTrackingStep only map a genuine not-found to
ErrTrackingNotFound; a real DB error now propagates instead of being misreported the same
way.
- sqlite_activity_store.go: GetActivity now returns persisted scan_state as warnings instead of
only the current call's scan result, so a network's last known failure actually surfaces to
later callers instead of being written and never read back.
- proxy/cmd/run.go: fixed CanonicalBlockHash returning (zero hash, nil error) when a block is
absent from both Headers and Errors, which would have looked like a reorg and silently
discarded a good cache row; both SQLite-backed stores are now Close()d on shutdown.
Closes review comments on #1852 (findings 1-11 from the arnaubennassar review).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Addressed all 11 findings from the review above in 6192f3d. Blocking 1-3 (unsafe 4 ( 5 (capacity gate precedes stale-row overwrite): fixed — 6 (one undecodable row wedges the engine): fixed — 7 (engine writes bump 8 (real DB errors laundered into 9 ( 10 (stores never 11 (stale retention comments): covered by the DBPath fix above — comments now reflect that those knobs are in-memory-adapter-only.
|
…racker (#1856) Based on #1852 ## 🔄 Changes Summary - `GET /activity/from/{address}` used to scan every configured bridge service inline inside the HTTP request, with no background cadence. It now follows the tracker's own register + background-engine + bounded-wait pattern: - Registering a `from_address` (`RegisterAndAwait`) adds it to a supervised list instead of scanning it inline. - A new `ActivityEngine` refreshes every supervised address on its own `ActivityPollInterval` ticker, plus immediately on first registration via a trigger channel. - The first request for a new address waits up to `ActivityRegisterResolveTimeout` for that first refresh before answering; an already-registered address never waits. - Idle addresses are forgotten via `PruneIdle` — now a real sweep on both the in-memory and SQLite-backed stores (the SQLite side, `sweepIdle`, was a no-op before, see #1822). - `GetActivity` becomes a cache-only read; the scan/claim-check/tracker-registration logic moves to `RefreshAddress`, called only by the engine. - New domain ports: `ActivitySupervisedStore`, `ActivityTriggerable`, `ActivityRegistry` (mirroring `SupervisedStore`/`Triggerable`/`SupervisedRegistry`). - New migration `bridgetracker0003.sql`: `activity_address` gains `include_tracking` (sticky flag for tracker enrichment across background refreshes) and `last_warnings` (persists the last scan's per-network warnings for the now-decoupled read path). - `bridgetracker/db.sqlite_activity_store.go`'s `saveBridgeRow`/`entry` now also persist/restore `ActivityEntry.Tracking` (previously never serialized, since it used to be sourced live within the same call that computed it). ##⚠️ Breaking Changes - 🛠️ **Config**: adds `[Tracker]` fields `ActivityPollInterval` (default `30s`), `ActivityRegisterResolveTimeout` (default `5s`), `ActivityMaxConcurrentRefreshes` (default `10`). No existing field changes meaning. - 🔌 **API/CLI**: `GET /activity/from/{address}` can now return `503` (`ErrorData`) if the supervised-address registry is at capacity, in addition to the existing `400`/`500`. ## 📋 Config Updates - 🧾 **Diff/Config snippet**: ```toml [Tracker] ActivityPollInterval = "30s" ActivityRegisterResolveTimeout = "5s" ActivityMaxConcurrentRefreshes = 10 ``` ## ✅ Testing - 🤖 **Automatic**: `go build ./...`, `go vet ./...`, `golangci-lint run ./bridgetracker/... ./proxy/...` (0 issues), `go test -race ./bridgetracker/... ./proxy/...` (all green). Added `bridgetracker/activity_engine_test.go` (new `ActivityEngine` coverage: trigger fast-path, tick concurrency bound, idle pruning), extended `activity_test.go`/`sqlite_activity_store_test.go` for the register/refresh/read split, and `bridgetracker/api/activity_command_test.go` for the handler's register-before-read ordering and the new 503 path. - 🖱️ **Manual**: start the binary with `DBPath` set (SQLite) and unset (in-memory); `GET /tracker/v1/activity/from/{addr}` twice in a row (second is served from cache); wait `ActivityPollInterval` and confirm data refreshes without a new client request; confirm an address idle past `ActivityIdleTimeout` disappears from the supervised list. ## 🐞 Issues - No associated issue. ## 🔗 Related PRs - Stacked on #1852 (feat/proxy-db-cache-1822) — targets that branch as base, not `develop`. ## 📝 Notes - The background refresh can no longer know a future request's `filterBridges` value, so it now always fetches a claimed bridge's claim record (previously skipped for `pending`/`readyToClaim`/`error` filters); the filter still only gates what `GetActivity` returns from the cache. - `includeTracking` is now a sticky per-address flag: the very first response after it's first requested may still show `tracking: null` until the *next* background refresh populates it — the same "poll again" precedent the tracker itself established for a freshly registered bridge. - SQLite retention for the *activity* tables is now real (`PruneIdle`); the tracker's own `sqliteRegistry.PruneTerminal/PruneIdle` no-ops are intentionally untouched here (separate, broader piece of #1822). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Resolves the two conflicts caused by #1852 (SQLite-backed bridgetracker cache), keeping both sides' intent: - bridgetracker/bridgetracker.go: api.NewAPI now receives upstream's ActivityRegisterResolveTimeout and activityPollInterval arguments *and* our trailing cfg.PendingNetworksLister. - bridgetracker/api/activity_command.go: keeps upstream's rewritten Execute (validate-before-flush, RegisterAndAwait, 503 + Retry-After) while retaining our aggkitcommon.RedactError on the two error sites that existed before the rewrite (ParseActivityFilter 400, GetActivity 500), plus our ActivityItem/ActivityWarningItem MarshalJSON redaction. Our redaction in bridgetracker/activity.go followed upstream's rewrite to its new sites (entry.Errors "claim" and "readiness"). Redacting the error sites the upstream commit newly introduced is deliberately left out of this merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
🔄 Changes Summary
bridgetracker/dbpackage (mirrorsaggsender/db):sqliteRegistryimplementsdomain.SupervisedRegistryover a newtracked_bridgetable;sqliteActivityStoreimplementsdomain.ActivityQuerierover newactivity_address/activity_bridgetables. Both share one migration set and can point at the same SQLite file.schema_versioncolumn: a row written under a different version is treated as a cache miss and discarded rather than risking a misinterpreted decode.bridgetracker/domain:BridgeInfogainsBlockHash(populated insources.BridgeEventSource);TrackingDatagainsIsTerminal()/TerminallyFailed(), hoisted out ofregistry.goso both the in-memory and SQLite adapters share the same derivation.bridgetracker.ConfiggainsDBPath(empty keeps the in-memory adapters) and anActivityoverride field;proxy/cmd/run.gowires the SQLite-backed stores whenDBPathis set, andproxy/config/default.goenables it by default (/tmp/bridgetracker.sqlite).DBPathis additive and defaults to empty inConfigitself; the proxy's shippeddefault.godoes set it to/tmp/bridgetracker.sqlite, switching the proxy binary's default registry/activity store from in-memory to SQLite-backed.📋 Config Updates
[Tracker].DBPathkey (proxy/config/default.go):✅ Testing
bridgetracker/db/sqlite_registry_test.goandsqlite_activity_store_test.go; full suite verified withgo build ./...,go vet ./...,golangci-lint run ./bridgetracker/... ./proxy/..., andgo test ./bridgetracker/... ./proxy/...(all green).🐞 Issues
🔗 Related PRs
📝 Notes
🤖 Generated with Claude Code