chore: run settlement/cache poll reads without stalling on DDL#1400
Merged
Conversation
Time Submission Status
Submit or update total time with: Add time on top of previous submission with: See available commands to help comply with our Guidelines. |
📝 WalkthroughWalkthroughAdds a bounded Postgres Changestn_cache lock-timeout wiring
tn_settlement independent read pool
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant EngineOperations
participant ReadPool
participant Postgres
Scheduler->>EngineOperations: LoadSettlementConfig / FindUnsettledMarkets / AttestationExists
EngineOperations->>EngineOperations: check readDB != nil
alt readDB is nil
EngineOperations-->>Scheduler: error "settlement read handle not initialized"
else readDB set
EngineOperations->>ReadPool: Execute(SQL statement)
ReadPool->>Postgres: acquire connection, run query (lock_timeout applied)
Postgres-->>ReadPool: rows or lock timeout error
ReadPool-->>EngineOperations: sql.ResultSet or error
EngineOperations-->>Scheduler: parsed result or error
end
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
extensions/tn_cache/tn_cache.go (1)
460-463: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaming: constant says "Read" but comment/usage covers writes too.
cacheReadLockTimeoutMillisis applied to the whole pool (reads and writes per the comment at Lines 487-489). Consider renaming to something likecacheLockTimeoutMillisfor accuracy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/tn_cache/tn_cache.go` around lines 460 - 463, The constant name in tn_cache.go is misleading because its comment and pool usage cover both cache reads and writes, not just reads. Rename cacheReadLockTimeoutMillis to a more accurate symbol like cacheLockTimeoutMillis, and update any related references/comments in the cache lock timeout setup so the name matches the behavior in createIndependentConnectionPool and the pool configuration code.extensions/tn_settlement/tn_settlement.go (1)
61-79: 🩺 Stability & Availability | 🔵 TrivialNo rebuild path if
NewReadPoolfails at startup.If pool construction fails transiently at boot (e.g., DB not yet reachable),
readDBstays nil for the rest of the process's lifetime — settlement reads are disabled "until restart," as the comment states. Since there's already a background retry worker (ext.startRetryWorker()) for config-reload resilience, consider whether it should also periodically attempt to (re)build the read pool, so a transient startup blip doesn't require a manual restart to recover.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/tn_settlement/tn_settlement.go` around lines 61 - 79, `NewReadPool` only runs once during startup, so a transient failure leaves `readDB` nil for the rest of the process and disables settlement reads until restart. Update the startup flow in `tn_settlement.go` around `internal.NewReadPool` and `internal.NewEngineOperations` to add a retry/rebuild path, likely by extending the existing `ext.startRetryWorker()` logic or a similar background worker, so it periodically re-attempts building the read pool and rebinds `readDB` when the DB becomes available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/tn_settlement/internal/read_pool.go`:
- Around line 45-55: NewReadPool is still building the PostgreSQL DSN by
concatenating dbConfig.Pass into the conn string, which breaks passwords
containing spaces or special characters. Update NewReadPool to stop appending
the password into connStr and instead let newReadPoolFromConnString or its
parsing path set the password on the parsed pgx ConnConfig after ParseConfig.
Keep the fix localized around NewReadPool and the connection-string parsing flow
so the password is handled via ConnConfig.Password rather than raw DSN
interpolation.
---
Nitpick comments:
In `@extensions/tn_cache/tn_cache.go`:
- Around line 460-463: The constant name in tn_cache.go is misleading because
its comment and pool usage cover both cache reads and writes, not just reads.
Rename cacheReadLockTimeoutMillis to a more accurate symbol like
cacheLockTimeoutMillis, and update any related references/comments in the cache
lock timeout setup so the name matches the behavior in
createIndependentConnectionPool and the pool configuration code.
In `@extensions/tn_settlement/tn_settlement.go`:
- Around line 61-79: `NewReadPool` only runs once during startup, so a transient
failure leaves `readDB` nil for the rest of the process and disables settlement
reads until restart. Update the startup flow in `tn_settlement.go` around
`internal.NewReadPool` and `internal.NewEngineOperations` to add a retry/rebuild
path, likely by extending the existing `ext.startRetryWorker()` logic or a
similar background worker, so it periodically re-attempts building the read pool
and rebinds `readDB` when the DB becomes available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d53b5df9-91a4-472f-8fb5-ed153640dff1
📒 Files selected for processing (10)
extensions/tn_cache/pool_config_test.goextensions/tn_cache/tn_cache.goextensions/tn_settlement/extension.goextensions/tn_settlement/internal/engine_ops.goextensions/tn_settlement/internal/engine_ops_test.goextensions/tn_settlement/internal/read_pool.goextensions/tn_settlement/internal/read_pool_pglive_test.goextensions/tn_settlement/internal/read_pool_test.goextensions/tn_settlement/settlement_integration_test.goextensions/tn_settlement/tn_settlement.go
Contributor
Author
|
@holdex pr submit-time 4h |
MicBun
force-pushed
the
refactor/settlement-cache-deadlock-safe-reads
branch
from
53fdbcd to
6c39173
Compare
MicBun
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves: #1401
Re-running a schema migration — or any DDL
exec-sql— against a live node could deadlock the whole network. A background poller (the settlement scheduler on the leader, the cache refresher on every node) held the engine interpreter's read lock while itsSELECTwaited on a Postgres table lock. If a block was applyingALTER/DROP/CREATEon a table that poller reads, the block held that table'sAccessExclusiveLockand needed the interpreter's write lock, while the poller held the interpreter read lock and needed the table lock. Neither could proceed and the chain stopped producing blocks.There is a second, related variant: the settlement config reload runs inside
EndBlock; if the same block alteredsettlement_config, that read self-blocks at the Postgres layer and the block can never commit.This makes those poll reads unable to deadlock block production:
lock_timeout(3s). A read blocked by an in-block DDL lock now fails fast and retries on the next tick instead of hanging. Ordinary DML never conflicts with a plainSELECT, so this only ever trips during real DDL contention.The change is node-only and consensus-neutral — these reads are not part of any block's write set.
Verified
ACCESS EXCLUSIVElock fails fast in ~3s instead of hanging (it would hang → fail the test on the old path).tn_cachesuite, and settlement/cache unit tests all pass; full build clean.lock_timeout, and that settlement reads fail closed when the pool is unavailable.Follow-up (not in this PR)
tn_lp_rewardsandtn_digestshare the sameEndBlockconfig-reload pattern. It is not reachable today (no migration alters their config tables), so it is tracked as a hardening item rather than changed here.Summary by CodeRabbit
New Features
Bug Fixes