Uh oh!
There was an error while loading. Please reload this page.
raft: guard snapshot restore from deleting recently-seen instances - #124
Conversation
Restore() previously did an unconditional set-difference delete: any instance present in the local backend but absent from the (best-effort, possibly stale) raft snapshot was forgotten via ForgetInstance, with no recency check. On a rolling restart with a leader change, this could wipe recently-discovered instances cluster-wide if the on-disk snapshot predated their discovery. Add ReadRecentlySeenInstanceKeyMap() and use it to skip forgetting any key whose last_seen is within UnseenInstanceForgetHours (reuses the existing config, no new setting). Restore's deletes become a strict subset of what ForgetLongUnseenInstances would already remove. FixesProxySQL#123 Signed-off-by: Ahmet Soğuksu <ahmet.soguksu@mono.tr>
Warning Review limit reachedNext included review available in 51 minutes. View limit detailsLimit details: You’ve used the included review currently available. This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds a recent-instance lookup and uses it during snapshot restoration. Recently seen local instances remain when absent from the snapshot. Absent stale instances continue to be forgotten. ChangesRecent instance retention
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk:⚪ Minimal · up to The change prevents snapshot restore from forgetting recently seen instances; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant RaftTest
participant LeaderAPI
participant ClusterNodes
participant MySQL3
RaftTest->>LeaderAPI: forget mysql3
RaftTest->>ClusterNodes: create snapshots without mysql3
ClusterNodes->>MySQL3: rediscover mysql3 locally
RaftTest->>MySQL3: stop mysql3
RaftTest->>ClusterNodes: perform rolling restarts
ClusterNodes-->>RaftTest: retain mysql3 in local backends
RaftTest->>MySQL3: restart mysql3
ClusterNodes-->>RaftTest: restore three-instance topology
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a raft snapshot restore data-loss path by preventing Restore() from forgetting locally-known instances that are absent from a potentially stale snapshot but have been seen recently, using the existing UnseenInstanceForgetHours recency window. This fits into orchestrator’s raft-backed HA behavior by making snapshot reconciliation safer for clusters using per-node local backends (notably SQLite).
Changes:
- Add a recency guard in raft snapshot restore to avoid deleting recently-seen instances missing from the snapshot.
- Introduce
inst.ReadRecentlySeenInstanceKeyMap()to fetch instance keys seen within a configurable window, reused by restore logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| go/logic/snapshot_data.go | Adds recency-aware guard before forgetting instances absent from a restored snapshot. |
| go/inst/instance_dao.go | Adds DAO helper to fetch “recently seen” instance keys based on last_seen. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| existingKeys, _ := inst.ReadAllInstanceKeys() | ||
| recentlySeenKeys, _ := inst.ReadRecentlySeenInstanceKeyMap(config.Config.UnseenInstanceForgetHours) | ||
| for _, existingKey := range existingKeys { |
| from | ||
| database_instance | ||
| where | ||
| last_seen > NOW() - interval ? hour` |
| // ReadRecentlySeenInstanceKeyMap returns the set of instance keys whose last_seen is | ||
| // within the given recency window (hours). Used to protect freshly-discovered instances | ||
| // from being purged during raft snapshot restore before they've propagated into a snapshot. | ||
| func ReadRecentlySeenInstanceKeyMap(recencyHours uint) (*InstanceKeyMap, error) { | ||
| keys := NewInstanceKeyMap() |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@go/logic/snapshot_data.go`:
- Around line 164-165: Update the snapshot key-loading flow around
ReadAllInstanceKeys and ReadRecentlySeenInstanceKeyMap to capture and check both
read errors before entering the deletion loop. If either read fails, return the
restore error or skip the purge, and do not call ForgetInstance with incomplete
key data; preserve normal cleanup when both reads succeed.
- Around line 165-174: Make snapshot cleanup atomic by adding a DAO operation in
instance_dao.go that deletes an instance only when its key matches and last_seen
still satisfies the stale predicate, preserving NULL last_seen behavior. Update
forgetInstanceKeys and discardedKeys only when the conditional delete affects a
row, and use this operation from forgetInstanceKeys instead of the separate
read-then-ForgetInstance flow. Serialize or flush buffered discovery writes
during restore, and add a test covering a discovery write interleaved with
snapshot cleanup.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 15306ba3-2f10-4fb8-a2d3-074ee60fed79
📒 Files selected for processing (2)
go/inst/instance_dao.gogo/logic/snapshot_data.go
| existingKeys, _ := inst.ReadAllInstanceKeys() | ||
| recentlySeenKeys, _ := inst.ReadRecentlySeenInstanceKeyMap(config.Config.UnseenInstanceForgetHours) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Fail closed when the instance reads fail.
Line 165 discards the error from ReadRecentlySeenInstanceKeyMap. A query or resolution failure can leave recentlySeenKeys empty or partial. The loop then treats every snapshot-absent key as stale and calls ForgetInstance. Check both read errors before deleting any key. Return the restore error or skip the purge when either read fails.
Suggested error handling
- existingKeys, _ := inst.ReadAllInstanceKeys()- recentlySeenKeys, _ := inst.ReadRecentlySeenInstanceKeyMap(config.Config.UnseenInstanceForgetHours)+ existingKeys, err := inst.ReadAllInstanceKeys()+ if err != nil {+ return log.Errore(err)+ }+ recentlySeenKeys, err := inst.ReadRecentlySeenInstanceKeyMap(config.Config.UnseenInstanceForgetHours)+ if err != nil {+ return log.Errore(err)+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| existingKeys, _:=inst.ReadAllInstanceKeys() | |
| recentlySeenKeys, _:=inst.ReadRecentlySeenInstanceKeyMap(config.Config.UnseenInstanceForgetHours) | |
| existingKeys, err:=inst.ReadAllInstanceKeys() | |
| iferr!=nil { | |
| returnlog.Errore(err) | |
| } | |
| recentlySeenKeys, err:=inst.ReadRecentlySeenInstanceKeyMap(config.Config.UnseenInstanceForgetHours) | |
| iferr!=nil { | |
| returnlog.Errore(err) | |
| } |
🤖 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 `@go/logic/snapshot_data.go` around lines 164 - 165, Update the snapshot
key-loading flow around ReadAllInstanceKeys and ReadRecentlySeenInstanceKeyMap
to capture and check both read errors before entering the deletion loop. If
either read fails, return the restore error or skip the purge, and do not call
ForgetInstance with incomplete key data; preserve normal cleanup when both reads
succeed.
| recentlySeenKeys, _ := inst.ReadRecentlySeenInstanceKeyMap(config.Config.UnseenInstanceForgetHours) | ||
| for _, existingKey := range existingKeys { | ||
| if !snapshotInstanceKeyMap.HasKey(existingKey) { | ||
| _ = inst.ForgetInstance(&existingKey) | ||
| discardedKeys++ | ||
| if snapshotInstanceKeyMap.HasKey(existingKey) { | ||
| continue | ||
| } | ||
| if recentlySeenKeys.HasKey(existingKey) { | ||
| log.Debugf("raft snapshot restore: retaining recently-seen instance %+v absent from snapshot", existingKey) | ||
| continue | ||
| } | ||
| _ = inst.ForgetInstance(&existingKey) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -eu
printf'%s\n''--- target file outline ---'
ast-grep outline go/logic/snapshot_data.go 2>/dev/null ||trueprintf'%s\n''--- target lines ---'
cat -n go/logic/snapshot_data.go | sed -n '120,205p'printf'%s\n''--- related symbols ---'
rg -n --glob '*.go''ReadRecentlySeenInstanceKeyMap|ForgetInstance|last_seen|discardedKeys|recentlySeenKeys|snapshotInstanceKeyMap' goRepository: ProxySQL/orchestrator
Length of output: 15414
🏁 Script executed:
#!/bin/bashset -eu
printf'%s\n''--- database and instance definitions ---'
rg -n --glob '*.go''type .*Instance|database_instance|RecentlySeen|ForgetInstance|ReadRecentlySeen'.printf'%s\n''--- candidate DAO files ---'
git ls-files 'go/**/*.go'| rg '(_dao\.go$|instance|snapshot|discover)'Repository: ProxySQL/orchestrator
Length of output: 27970
🏁 Script executed:
#!/bin/bashset -eu
printf'%s\n''--- recency query ---'
cat -n go/inst/instance_dao.go | sed -n '2688,2720p'printf'%s\n''--- write/last_seen path ---'
cat -n go/inst/instance_dao.go | sed -n '2920,3188p'printf'%s\n''--- forget path ---'
cat -n go/inst/instance_dao.go | sed -n '3218,3318p'printf'%s\n''--- forget-cache references ---'
rg -n --glob '*.go''forget.*cache|Forget.*Cache|forgotten|discardedKeys|InstanceKeyMap' go/inst go/logic go/discoveryRepository: ProxySQL/orchestrator
Length of output: 24071
🏁 Script executed:
#!/bin/bashset -eu
printf'%s\n''--- discovery call graph ---'
rg -n --glob '*.go''WriteInstance\(|writeInstance\(|Flush|flushInstanceWriteBuffer|last_seen' go/discovery go/logic go/inst | head -240
printf'%s\n''--- database execution helpers and transaction patterns ---'
rg -n --glob '*.go''Begin\(|BeginTx|Commit\(|Rollback\(|Exec.*delete from database_instance|delete from database_instance' go/inst go/db | head -240
printf'%s\n''--- relevant tests and test setup ---'
rg -n --glob '*_test.go''ForgetInstance|ReadRecentlySeenInstanceKeyMap|snapshot|Restore|WriteInstance' go/logic go/inst go/discoveryRepository: ProxySQL/orchestrator
Length of output: 10508
🏁 Script executed:
#!/bin/bashset -eu
printf'%s\n''--- schema and read-all behavior ---'
cat -n go/db/generate_base.go | sed -n '18,48p'
cat -n go/inst/instance_dao.go | sed -n '2720,2790p'printf'%s\n''--- database write wrapper ---'
cat -n go/db/db.go | sed -n '250,345p'printf'%s\n''--- discovery write callers ---'
cat -n go/inst/instance_dao.go | sed -n '1215,1250p'
cat -n go/inst/instance_dao.go | sed -n '2295,2380p'
rg -n --glob '*.go''DiscoverInstance|discoverInstance|enqueueInstanceWrite|WriteInstance\(' go/logic go/discovery go/inst | head -220
printf'%s\n''--- SQL driver and backend support ---'
rg -n --glob 'go.mod' --glob '*.go''go-sql-driver/mysql|mattn/go-sqlite3|lib/pq|sqlite|mysql' go go.mod | head -180Repository: ProxySQL/orchestrator
Length of output: 36906
🏁 Script executed:
#!/bin/bashset -eu
python3 - <<'PY'from dataclasses import dataclass`@dataclass`class Row: last_seen_recent: bool present: bool = Truerow = Row(last_seen_recent=False)recent_snapshot = row.last_seen_recentrow.last_seen_recent = True # discovery updates last_seen after the readif not recent_snapshot and row.present: # current ForgetInstance deletes by key only row.present = Falseprint({ "read_then_delete_result": "deleted" if not row.present else "retained", "recent_at_delete_time": row.last_seen_recent,})PYRepository: ProxySQL/orchestrator
Length of output: 229
Make snapshot cleanup atomic with last_seen updates.
ReadRecentlySeenInstanceKeyMap reads last_seen, then ForgetInstance deletes by key. A discovery write between these operations can be deleted. Add a DAO operation in go/inst/instance_dao.go that conditionally deletes by key and stale predicate in one SQL DELETE. Update forgetInstanceKeys and discardedKeys only when the delete affects a row. Preserve current NULL last_seen behavior. Serialize or flush buffered discovery writes during restore. Add an interleaving test.
🤖 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 `@go/logic/snapshot_data.go` around lines 165 - 174, Make snapshot cleanup
atomic by adding a DAO operation in instance_dao.go that deletes an instance
only when its key matches and last_seen still satisfies the stale predicate,
preserving NULL last_seen behavior. Update forgetInstanceKeys and discardedKeys
only when the conditional delete affects a row, and use this operation from
forgetInstanceKeys instead of the separate read-then-ForgetInstance flow.
Serialize or flush buffered discovery writes during restore, and add a test
covering a discovery write interleaved with snapshot cleanup.
Unit tests (go/logic/snapshot_data_test.go) run Restore() against a real SQLite backend: - ReadRecentlySeenInstanceKeyMap returns only instances seen within the recency window (fresh/borderline included, stale/never-seen excluded). - Restore retains recently-seen instances that are absent from the snapshot and still forgets genuinely stale ones (fails without the fix). Functional test (tests/functional/test-raft.sh, Phase 5) reproduces the issue ProxySQL#123 scenario deterministically: forget mysql3, snapshot all nodes, let continuous discovery re-discover mysql3 locally (no raft command), stop mysql3 to block re-discovery, then rolling-restart all nodes with a leader change per round and assert every node's local backend still contains the recently-seen instance. Signed-off-by: René Cannaò <rene.cannao@gmail.com>
…estart Until the election completes, remaining nodes keep reporting the old (stopped) leader via /api/raft-leader, so the re-election wait must require the agreed leader to differ from the stopped node's address (as phase 3 already does). Also compare the rejoined node's leader against a live node's current leader instead of a fixed value. Signed-off-by: René Cannaò <rene.cannao@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.
Description
Fixes a data-loss bug in raft snapshot restore.
Restore()did an unconditionalset-difference delete: any instance present in the local backend but absent
from the (best-effort, possibly stale) raft snapshot was forgotten via
ForgetInstance, with no recency check. On a rolling restart with a leaderchange, this could wipe recently-discovered instances cluster-wide if the
on-disk snapshot predated their discovery — reproduced on a 3-node
raft+SQLite lab cluster (see issue #123 for full repro).
Fix: add
ReadRecentlySeenInstanceKeyMap()and skip forgetting any key whoselast_seenis withinUnseenInstanceForgetHours(reuses the existingconfig, no new setting). Restore's deletes become a strict subset of what
ForgetLongUnseenInstanceswould already remove — genuine decommissions areunaffected, only the race window is closed.
Verified: unit tests pass (
go/inst,go/logic), and live-tested end-to-endon a 3-node raft+SQLite cluster — reproduced the original bug on the
unpatched binary (instances vanish on rolling restart w/ leader change),
then confirmed the patched binary retains a freshly-discovered instance
through an identical restart + leader change.
Checklist
gofmtgit commit -s)Summary by CodeRabbit