raft: guard snapshot restore from deleting recently-seen instances - #124
raft: guard snapshot restore from deleting recently-seen instances#124ahmetsoguksu wants to merge 1 commit into
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. Fixes ProxySQL#123 Signed-off-by: Ahmet Soğuksu <ahmet.soguksu@mono.tr>
📝 WalkthroughWalkthroughThe change adds a recent-instance lookup and uses it during snapshot restoration. Recently seen local instances are retained when absent from the snapshot. Absent stale instances continue to be forgotten. ChangesRecent instance retention
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Restore
participant Config
participant InstanceDAO
participant Database
Config-->>Restore: UnseenInstanceForgetHours
Restore->>InstanceDAO: ReadRecentlySeenInstanceKeyMap(hours)
InstanceDAO->>Database: query recent last_seen instances
Database-->>InstanceDAO: hostname and port records
InstanceDAO-->>Restore: resolved InstanceKeyMap
Restore->>Restore: retain matching or recently seen instances
Restore->>Restore: forget absent stale instances
Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
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() |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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() | |
| if err != nil { | |
| return log.Errore(err) | |
| } | |
| recentlySeenKeys, err := inst.ReadRecentlySeenInstanceKeyMap(config.Config.UnseenInstanceForgetHours) | |
| if err != nil { | |
| return log.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/bash
set -eu
printf '%s\n' '--- target file outline ---'
ast-grep outline go/logic/snapshot_data.go 2>/dev/null || true
printf '%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/bash
set -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/bash
set -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/bash
set -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/bash
set -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/bash
set -eu
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class Row:
last_seen_recent: bool
present: bool = True
row = Row(last_seen_recent=False)
recent_snapshot = row.last_seen_recent
row.last_seen_recent = True # discovery updates last_seen after the read
if not recent_snapshot and row.present: # current ForgetInstance deletes by key only
row.present = False
print({
"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.
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