Skip to content

raft: guard snapshot restore from deleting recently-seen instances #124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ahmetsoguksu wants to merge 1 commit into
base: master
Choose a base branch
from
Open

raft: guard snapshot restore from deleting recently-seen instances #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions go/inst/instance_dao.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2694,6 +2694,29 @@ func ReadAllInstanceKeys() ([]InstanceKey, error) {
return res, log.Errore(err)
}

// 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()
Comment on lines +2697 to +2701
query := `
select
hostname, port
from
database_instance
where
last_seen > NOW() - interval ? hour`
err := db.QueryOrchestrator(query, sqlutils.Args(recencyHours), func(m sqlutils.RowMap) error {
instanceKey, merr := NewResolveInstanceKey(m.GetString("hostname"), m.GetInt("port"))
if merr != nil {
return log.Errore(merr)
}
keys.AddKey(*instanceKey)
return nil
})
return keys, log.Errore(err)
}

// ReadAllInstanceKeysMasterKeys
func ReadAllMinimalInstances() ([]MinimalInstance, error) {
res := []MinimalInstance{}
Expand Down
22 changes: 18 additions & 4 deletions go/logic/snapshot_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"encoding/json"
"io"

"github.com/proxysql/orchestrator/go/config"
"github.com/proxysql/orchestrator/go/db"
"github.com/proxysql/orchestrator/go/inst"

Expand Down Expand Up @@ -152,13 +153,26 @@ func (s *SnapshotDataCreatorApplier) Restore(rc io.ReadCloser) error {
}

discardedKeys := 0
// Forget instances that were not in snapshot
// Forget instances that were not in snapshot.
// Guard: only forget an instance absent from the snapshot if it is ALSO stale
// locally (not seen within UnseenInstanceForgetHours). A freshly discovered
// instance may legitimately exist in our local backend but not yet be captured
// in the (older) snapshot we are restoring; deleting it here races with discovery
// and can wipe recent discoveries cluster-wide on restart/leader-change. Genuine
// decommissions age out and are removed both here and by ForgetLongUnseenInstances(),
// and explicit forgets arrive via the replicated "forget" command.
existingKeys, _ := inst.ReadAllInstanceKeys()
recentlySeenKeys, _ := inst.ReadRecentlySeenInstanceKeyMap(config.Config.UnseenInstanceForgetHours)
Comment on lines 164 to +165

@coderabbitai coderabbitai Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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.

Suggested change
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.

for _, existingKey := range existingKeys {
Comment on lines 164 to 166
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)
Comment on lines +165 to +174

@coderabbitai coderabbitai Bot Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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' go

Repository: 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/discovery

Repository: 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/discovery

Repository: 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 -180

Repository: 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,
})
PY

Repository: 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.

discardedKeys++
}
log.Debugf("raft snapshot restore: discarded %+v keys", discardedKeys)
existingKeysMap := inst.NewInstanceKeyMap()
Expand Down
Loading