Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line numberDiff line numberDiff 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
Original file line numberDiff line numberDiff 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 DownExpand 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

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()
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.

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)
Comment on lines +165 to +174

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/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' go

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

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

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

Repository: 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,})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
202 changes: 202 additions & 0 deletions go/logic/snapshot_data_test.go
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
/*
Copyright 2026 ProxySQL Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logic

import (
"bytes"
"compress/gzip"
"encoding/json"
"io"
"os"
"path/filepath"
"testing"
"time"

test "github.com/proxysql/golib/tests"
"github.com/proxysql/orchestrator/go/config"
"github.com/proxysql/orchestrator/go/db"
"github.com/proxysql/orchestrator/go/inst"
)

func TestMain(m *testing.M) {
config.MarkConfigurationLoaded()
// keep hostname resolution local-only for the whole test binary, so that
// ResolveHostname never spawns asynchronous backend writes
config.Config.HostnameResolveMethod = "none"
os.Exit(m.Run())
}

// waitForInstanceDaoInit blocks until the background initializeInstanceDao()
// goroutine has created the package-level caches, which are required by
// WriteInstance/ForgetInstance.
func waitForInstanceDaoInit(t *testing.T) {
t.Helper()
deadline := time.Now().Add(10 * time.Second)
for {
ready := false
func() {
defer func() {
_ = recover()
}()
inst.InstanceIsForgotten(&inst.InstanceKey{})
ready = true
}()
if ready {
return
}
if time.Now().After(deadline) {
t.Fatal("instance DAO caches were not initialized in time")
}
time.Sleep(10 * time.Millisecond)
}
}

func setupSQLiteBackend(t *testing.T) {
t.Helper()
waitForInstanceDaoInit(t)
origBackendDB := config.Config.BackendDB
origSQLiteDataFile := config.Config.SQLite3DataFile
origHostnameResolveMethod := config.Config.HostnameResolveMethod
config.Config.BackendDB = "sqlite"
config.Config.SQLite3DataFile = filepath.Join(t.TempDir(), "orchestrator.sqlite3")
// avoid asynchronous hostname-resolve writes racing with the backend switch
config.Config.HostnameResolveMethod = "none"
t.Cleanup(func() {
config.Config.BackendDB = origBackendDB
config.Config.SQLite3DataFile = origSQLiteDataFile
config.Config.HostnameResolveMethod = origHostnameResolveMethod
})
_, err := db.OpenOrchestrator()
test.S(t).ExpectNil(err)
}

func writeTestInstance(t *testing.T, hostname string, port int) {
t.Helper()
instance := &inst.Instance{Key: inst.InstanceKey{Hostname: hostname, Port: port}}
test.S(t).ExpectNil(inst.WriteInstance(instance, false, nil))
}

func setInstanceLastSeen(t *testing.T, hostname string, port int, hoursAgo int) {
t.Helper()
_, err := db.ExecOrchestrator(`
update database_instance
set last_seen = NOW() - interval ? hour
where hostname = ? and port = ?`,
hoursAgo, hostname, port,
)
test.S(t).ExpectNil(err)
}

func readInstanceKeyMap(t *testing.T) *inst.InstanceKeyMap {
t.Helper()
keys, err := inst.ReadAllInstanceKeys()
test.S(t).ExpectNil(err)
keyMap := inst.NewInstanceKeyMap()
keyMap.AddKeys(keys)
return keyMap
}

func mkSnapshotReader(t *testing.T, snapshotData *SnapshotData) io.ReadCloser {
t.Helper()
b, err := json.Marshal(snapshotData)
test.S(t).ExpectNil(err)
var buf bytes.Buffer
zw := gzip.NewWriter(&buf)
_, err = zw.Write(b)
test.S(t).ExpectNil(err)
test.S(t).ExpectNil(zw.Close())
return io.NopCloser(bytes.NewReader(buf.Bytes()))
}

func TestReadRecentlySeenInstanceKeyMap(t *testing.T) {
setupSQLiteBackend(t)

writeTestInstance(t, "fresh", 3306)
writeTestInstance(t, "borderline", 3306)
writeTestInstance(t, "stale", 3306)
writeTestInstance(t, "never-seen", 3306)
setInstanceLastSeen(t, "borderline", 3306, 239)
setInstanceLastSeen(t, "stale", 3306, 241)
_, err := db.ExecOrchestrator(`update database_instance set last_seen = NULL where hostname = 'never-seen'`)
test.S(t).ExpectNil(err)

keys, err := inst.ReadRecentlySeenInstanceKeyMap(240)
test.S(t).ExpectNil(err)

test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "fresh", Port: 3306}))
test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "borderline", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "never-seen", Port: 3306}))
}

// TestSnapshotRestoreRetainsRecentlySeenInstances verifies the fix for issue #123:
// instances that exist in the local backend but are absent from a (stale) raft
// snapshot must not be purged at restore time while they are still within the
// UnseenInstanceForgetHours recency window. Genuinely stale instances must
// still be forgotten, and instances carried by the snapshot must be added.
func TestSnapshotRestoreRetainsRecentlySeenInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 240
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "fresh-a", 3306)
writeTestInstance(t, "fresh-b", 3307)
writeTestInstance(t, "stale", 3306)
setInstanceLastSeen(t, "stale", 3306, 300)

snapshotData := NewSnapshotData()
snapshotData.MinimalInstances = []inst.MinimalInstance{
{Key: inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}},
}

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-a", Port: 3306}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-b", Port: 3307}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
}

// TestSnapshotRestoreForgetsUnseenStaleInstances ensures the recency guard does
// not prevent restore from forgetting instances that are both absent from the
// snapshot and already stale locally (the pre-existing behavior for genuine
// decommissions).
func TestSnapshotRestoreForgetsUnseenStaleInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 1
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "stale-a", 3306)
writeTestInstance(t, "stale-b", 3307)
setInstanceLastSeen(t, "stale-a", 3306, 2)
setInstanceLastSeen(t, "stale-b", 3307, 2)

snapshotData := NewSnapshotData()

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-a", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-b", Port: 3307}))
}
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
raft: guard snapshot restore from deleting recently-seen instances by ahmetsoguksu · Pull Request #124 · ProxySQL/orchestrator · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line numberDiff line numberDiff 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
Original file line numberDiff line numberDiff 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 DownExpand 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

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()
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.

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)
Comment on lines +165 to +174

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/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' go

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

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

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

Repository: 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,})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
202 changes: 202 additions & 0 deletions go/logic/snapshot_data_test.go
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
/*
Copyright 2026 ProxySQL Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logic

import (
"bytes"
"compress/gzip"
"encoding/json"
"io"
"os"
"path/filepath"
"testing"
"time"

test "github.com/proxysql/golib/tests"
"github.com/proxysql/orchestrator/go/config"
"github.com/proxysql/orchestrator/go/db"
"github.com/proxysql/orchestrator/go/inst"
)

func TestMain(m *testing.M) {
config.MarkConfigurationLoaded()
// keep hostname resolution local-only for the whole test binary, so that
// ResolveHostname never spawns asynchronous backend writes
config.Config.HostnameResolveMethod = "none"
os.Exit(m.Run())
}

// waitForInstanceDaoInit blocks until the background initializeInstanceDao()
// goroutine has created the package-level caches, which are required by
// WriteInstance/ForgetInstance.
func waitForInstanceDaoInit(t *testing.T) {
t.Helper()
deadline := time.Now().Add(10 * time.Second)
for {
ready := false
func() {
defer func() {
_ = recover()
}()
inst.InstanceIsForgotten(&inst.InstanceKey{})
ready = true
}()
if ready {
return
}
if time.Now().After(deadline) {
t.Fatal("instance DAO caches were not initialized in time")
}
time.Sleep(10 * time.Millisecond)
}
}

func setupSQLiteBackend(t *testing.T) {
t.Helper()
waitForInstanceDaoInit(t)
origBackendDB := config.Config.BackendDB
origSQLiteDataFile := config.Config.SQLite3DataFile
origHostnameResolveMethod := config.Config.HostnameResolveMethod
config.Config.BackendDB = "sqlite"
config.Config.SQLite3DataFile = filepath.Join(t.TempDir(), "orchestrator.sqlite3")
// avoid asynchronous hostname-resolve writes racing with the backend switch
config.Config.HostnameResolveMethod = "none"
t.Cleanup(func() {
config.Config.BackendDB = origBackendDB
config.Config.SQLite3DataFile = origSQLiteDataFile
config.Config.HostnameResolveMethod = origHostnameResolveMethod
})
_, err := db.OpenOrchestrator()
test.S(t).ExpectNil(err)
}

func writeTestInstance(t *testing.T, hostname string, port int) {
t.Helper()
instance := &inst.Instance{Key: inst.InstanceKey{Hostname: hostname, Port: port}}
test.S(t).ExpectNil(inst.WriteInstance(instance, false, nil))
}

func setInstanceLastSeen(t *testing.T, hostname string, port int, hoursAgo int) {
t.Helper()
_, err := db.ExecOrchestrator(`
update database_instance
set last_seen = NOW() - interval ? hour
where hostname = ? and port = ?`,
hoursAgo, hostname, port,
)
test.S(t).ExpectNil(err)
}

func readInstanceKeyMap(t *testing.T) *inst.InstanceKeyMap {
t.Helper()
keys, err := inst.ReadAllInstanceKeys()
test.S(t).ExpectNil(err)
keyMap := inst.NewInstanceKeyMap()
keyMap.AddKeys(keys)
return keyMap
}

func mkSnapshotReader(t *testing.T, snapshotData *SnapshotData) io.ReadCloser {
t.Helper()
b, err := json.Marshal(snapshotData)
test.S(t).ExpectNil(err)
var buf bytes.Buffer
zw := gzip.NewWriter(&buf)
_, err = zw.Write(b)
test.S(t).ExpectNil(err)
test.S(t).ExpectNil(zw.Close())
return io.NopCloser(bytes.NewReader(buf.Bytes()))
}

func TestReadRecentlySeenInstanceKeyMap(t *testing.T) {
setupSQLiteBackend(t)

writeTestInstance(t, "fresh", 3306)
writeTestInstance(t, "borderline", 3306)
writeTestInstance(t, "stale", 3306)
writeTestInstance(t, "never-seen", 3306)
setInstanceLastSeen(t, "borderline", 3306, 239)
setInstanceLastSeen(t, "stale", 3306, 241)
_, err := db.ExecOrchestrator(`update database_instance set last_seen = NULL where hostname = 'never-seen'`)
test.S(t).ExpectNil(err)

keys, err := inst.ReadRecentlySeenInstanceKeyMap(240)
test.S(t).ExpectNil(err)

test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "fresh", Port: 3306}))
test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "borderline", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "never-seen", Port: 3306}))
}

// TestSnapshotRestoreRetainsRecentlySeenInstances verifies the fix for issue #123:
// instances that exist in the local backend but are absent from a (stale) raft
// snapshot must not be purged at restore time while they are still within the
// UnseenInstanceForgetHours recency window. Genuinely stale instances must
// still be forgotten, and instances carried by the snapshot must be added.
func TestSnapshotRestoreRetainsRecentlySeenInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 240
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "fresh-a", 3306)
writeTestInstance(t, "fresh-b", 3307)
writeTestInstance(t, "stale", 3306)
setInstanceLastSeen(t, "stale", 3306, 300)

snapshotData := NewSnapshotData()
snapshotData.MinimalInstances = []inst.MinimalInstance{
{Key: inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}},
}

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-a", Port: 3306}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-b", Port: 3307}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
}

// TestSnapshotRestoreForgetsUnseenStaleInstances ensures the recency guard does
// not prevent restore from forgetting instances that are both absent from the
// snapshot and already stale locally (the pre-existing behavior for genuine
// decommissions).
func TestSnapshotRestoreForgetsUnseenStaleInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 1
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "stale-a", 3306)
writeTestInstance(t, "stale-b", 3307)
setInstanceLastSeen(t, "stale-a", 3306, 2)
setInstanceLastSeen(t, "stale-b", 3307, 2)

snapshotData := NewSnapshotData()

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-a", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-b", Port: 3307}))
}
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' raft: guard snapshot restore from deleting recently-seen instances by ahmetsoguksu · Pull Request #124 · ProxySQL/orchestrator · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line numberDiff line numberDiff 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
Original file line numberDiff line numberDiff 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 DownExpand 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

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()
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.

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)
Comment on lines +165 to +174

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/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' go

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

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

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

Repository: 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,})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
202 changes: 202 additions & 0 deletions go/logic/snapshot_data_test.go
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
/*
Copyright 2026 ProxySQL Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logic

import (
"bytes"
"compress/gzip"
"encoding/json"
"io"
"os"
"path/filepath"
"testing"
"time"

test "github.com/proxysql/golib/tests"
"github.com/proxysql/orchestrator/go/config"
"github.com/proxysql/orchestrator/go/db"
"github.com/proxysql/orchestrator/go/inst"
)

func TestMain(m *testing.M) {
config.MarkConfigurationLoaded()
// keep hostname resolution local-only for the whole test binary, so that
// ResolveHostname never spawns asynchronous backend writes
config.Config.HostnameResolveMethod = "none"
os.Exit(m.Run())
}

// waitForInstanceDaoInit blocks until the background initializeInstanceDao()
// goroutine has created the package-level caches, which are required by
// WriteInstance/ForgetInstance.
func waitForInstanceDaoInit(t *testing.T) {
t.Helper()
deadline := time.Now().Add(10 * time.Second)
for {
ready := false
func() {
defer func() {
_ = recover()
}()
inst.InstanceIsForgotten(&inst.InstanceKey{})
ready = true
}()
if ready {
return
}
if time.Now().After(deadline) {
t.Fatal("instance DAO caches were not initialized in time")
}
time.Sleep(10 * time.Millisecond)
}
}

func setupSQLiteBackend(t *testing.T) {
t.Helper()
waitForInstanceDaoInit(t)
origBackendDB := config.Config.BackendDB
origSQLiteDataFile := config.Config.SQLite3DataFile
origHostnameResolveMethod := config.Config.HostnameResolveMethod
config.Config.BackendDB = "sqlite"
config.Config.SQLite3DataFile = filepath.Join(t.TempDir(), "orchestrator.sqlite3")
// avoid asynchronous hostname-resolve writes racing with the backend switch
config.Config.HostnameResolveMethod = "none"
t.Cleanup(func() {
config.Config.BackendDB = origBackendDB
config.Config.SQLite3DataFile = origSQLiteDataFile
config.Config.HostnameResolveMethod = origHostnameResolveMethod
})
_, err := db.OpenOrchestrator()
test.S(t).ExpectNil(err)
}

func writeTestInstance(t *testing.T, hostname string, port int) {
t.Helper()
instance := &inst.Instance{Key: inst.InstanceKey{Hostname: hostname, Port: port}}
test.S(t).ExpectNil(inst.WriteInstance(instance, false, nil))
}

func setInstanceLastSeen(t *testing.T, hostname string, port int, hoursAgo int) {
t.Helper()
_, err := db.ExecOrchestrator(`
update database_instance
set last_seen = NOW() - interval ? hour
where hostname = ? and port = ?`,
hoursAgo, hostname, port,
)
test.S(t).ExpectNil(err)
}

func readInstanceKeyMap(t *testing.T) *inst.InstanceKeyMap {
t.Helper()
keys, err := inst.ReadAllInstanceKeys()
test.S(t).ExpectNil(err)
keyMap := inst.NewInstanceKeyMap()
keyMap.AddKeys(keys)
return keyMap
}

func mkSnapshotReader(t *testing.T, snapshotData *SnapshotData) io.ReadCloser {
t.Helper()
b, err := json.Marshal(snapshotData)
test.S(t).ExpectNil(err)
var buf bytes.Buffer
zw := gzip.NewWriter(&buf)
_, err = zw.Write(b)
test.S(t).ExpectNil(err)
test.S(t).ExpectNil(zw.Close())
return io.NopCloser(bytes.NewReader(buf.Bytes()))
}

func TestReadRecentlySeenInstanceKeyMap(t *testing.T) {
setupSQLiteBackend(t)

writeTestInstance(t, "fresh", 3306)
writeTestInstance(t, "borderline", 3306)
writeTestInstance(t, "stale", 3306)
writeTestInstance(t, "never-seen", 3306)
setInstanceLastSeen(t, "borderline", 3306, 239)
setInstanceLastSeen(t, "stale", 3306, 241)
_, err := db.ExecOrchestrator(`update database_instance set last_seen = NULL where hostname = 'never-seen'`)
test.S(t).ExpectNil(err)

keys, err := inst.ReadRecentlySeenInstanceKeyMap(240)
test.S(t).ExpectNil(err)

test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "fresh", Port: 3306}))
test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "borderline", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "never-seen", Port: 3306}))
}

// TestSnapshotRestoreRetainsRecentlySeenInstances verifies the fix for issue #123:
// instances that exist in the local backend but are absent from a (stale) raft
// snapshot must not be purged at restore time while they are still within the
// UnseenInstanceForgetHours recency window. Genuinely stale instances must
// still be forgotten, and instances carried by the snapshot must be added.
func TestSnapshotRestoreRetainsRecentlySeenInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 240
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "fresh-a", 3306)
writeTestInstance(t, "fresh-b", 3307)
writeTestInstance(t, "stale", 3306)
setInstanceLastSeen(t, "stale", 3306, 300)

snapshotData := NewSnapshotData()
snapshotData.MinimalInstances = []inst.MinimalInstance{
{Key: inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}},
}

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-a", Port: 3306}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-b", Port: 3307}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
}

// TestSnapshotRestoreForgetsUnseenStaleInstances ensures the recency guard does
// not prevent restore from forgetting instances that are both absent from the
// snapshot and already stale locally (the pre-existing behavior for genuine
// decommissions).
func TestSnapshotRestoreForgetsUnseenStaleInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 1
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "stale-a", 3306)
writeTestInstance(t, "stale-b", 3307)
setInstanceLastSeen(t, "stale-a", 3306, 2)
setInstanceLastSeen(t, "stale-b", 3307, 2)

snapshotData := NewSnapshotData()

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-a", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-b", Port: 3307}))
}
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' raft: guard snapshot restore from deleting recently-seen instances by ahmetsoguksu · Pull Request #124 · ProxySQL/orchestrator · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line numberDiff line numberDiff 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
Original file line numberDiff line numberDiff 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 DownExpand 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

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()
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.

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)
Comment on lines +165 to +174

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/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' go

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

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

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

Repository: 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,})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
202 changes: 202 additions & 0 deletions go/logic/snapshot_data_test.go
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
/*
Copyright 2026 ProxySQL Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logic

import (
"bytes"
"compress/gzip"
"encoding/json"
"io"
"os"
"path/filepath"
"testing"
"time"

test "github.com/proxysql/golib/tests"
"github.com/proxysql/orchestrator/go/config"
"github.com/proxysql/orchestrator/go/db"
"github.com/proxysql/orchestrator/go/inst"
)

func TestMain(m *testing.M) {
config.MarkConfigurationLoaded()
// keep hostname resolution local-only for the whole test binary, so that
// ResolveHostname never spawns asynchronous backend writes
config.Config.HostnameResolveMethod = "none"
os.Exit(m.Run())
}

// waitForInstanceDaoInit blocks until the background initializeInstanceDao()
// goroutine has created the package-level caches, which are required by
// WriteInstance/ForgetInstance.
func waitForInstanceDaoInit(t *testing.T) {
t.Helper()
deadline := time.Now().Add(10 * time.Second)
for {
ready := false
func() {
defer func() {
_ = recover()
}()
inst.InstanceIsForgotten(&inst.InstanceKey{})
ready = true
}()
if ready {
return
}
if time.Now().After(deadline) {
t.Fatal("instance DAO caches were not initialized in time")
}
time.Sleep(10 * time.Millisecond)
}
}

func setupSQLiteBackend(t *testing.T) {
t.Helper()
waitForInstanceDaoInit(t)
origBackendDB := config.Config.BackendDB
origSQLiteDataFile := config.Config.SQLite3DataFile
origHostnameResolveMethod := config.Config.HostnameResolveMethod
config.Config.BackendDB = "sqlite"
config.Config.SQLite3DataFile = filepath.Join(t.TempDir(), "orchestrator.sqlite3")
// avoid asynchronous hostname-resolve writes racing with the backend switch
config.Config.HostnameResolveMethod = "none"
t.Cleanup(func() {
config.Config.BackendDB = origBackendDB
config.Config.SQLite3DataFile = origSQLiteDataFile
config.Config.HostnameResolveMethod = origHostnameResolveMethod
})
_, err := db.OpenOrchestrator()
test.S(t).ExpectNil(err)
}

func writeTestInstance(t *testing.T, hostname string, port int) {
t.Helper()
instance := &inst.Instance{Key: inst.InstanceKey{Hostname: hostname, Port: port}}
test.S(t).ExpectNil(inst.WriteInstance(instance, false, nil))
}

func setInstanceLastSeen(t *testing.T, hostname string, port int, hoursAgo int) {
t.Helper()
_, err := db.ExecOrchestrator(`
update database_instance
set last_seen = NOW() - interval ? hour
where hostname = ? and port = ?`,
hoursAgo, hostname, port,
)
test.S(t).ExpectNil(err)
}

func readInstanceKeyMap(t *testing.T) *inst.InstanceKeyMap {
t.Helper()
keys, err := inst.ReadAllInstanceKeys()
test.S(t).ExpectNil(err)
keyMap := inst.NewInstanceKeyMap()
keyMap.AddKeys(keys)
return keyMap
}

func mkSnapshotReader(t *testing.T, snapshotData *SnapshotData) io.ReadCloser {
t.Helper()
b, err := json.Marshal(snapshotData)
test.S(t).ExpectNil(err)
var buf bytes.Buffer
zw := gzip.NewWriter(&buf)
_, err = zw.Write(b)
test.S(t).ExpectNil(err)
test.S(t).ExpectNil(zw.Close())
return io.NopCloser(bytes.NewReader(buf.Bytes()))
}

func TestReadRecentlySeenInstanceKeyMap(t *testing.T) {
setupSQLiteBackend(t)

writeTestInstance(t, "fresh", 3306)
writeTestInstance(t, "borderline", 3306)
writeTestInstance(t, "stale", 3306)
writeTestInstance(t, "never-seen", 3306)
setInstanceLastSeen(t, "borderline", 3306, 239)
setInstanceLastSeen(t, "stale", 3306, 241)
_, err := db.ExecOrchestrator(`update database_instance set last_seen = NULL where hostname = 'never-seen'`)
test.S(t).ExpectNil(err)

keys, err := inst.ReadRecentlySeenInstanceKeyMap(240)
test.S(t).ExpectNil(err)

test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "fresh", Port: 3306}))
test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "borderline", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "never-seen", Port: 3306}))
}

// TestSnapshotRestoreRetainsRecentlySeenInstances verifies the fix for issue #123:
// instances that exist in the local backend but are absent from a (stale) raft
// snapshot must not be purged at restore time while they are still within the
// UnseenInstanceForgetHours recency window. Genuinely stale instances must
// still be forgotten, and instances carried by the snapshot must be added.
func TestSnapshotRestoreRetainsRecentlySeenInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 240
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "fresh-a", 3306)
writeTestInstance(t, "fresh-b", 3307)
writeTestInstance(t, "stale", 3306)
setInstanceLastSeen(t, "stale", 3306, 300)

snapshotData := NewSnapshotData()
snapshotData.MinimalInstances = []inst.MinimalInstance{
{Key: inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}},
}

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-a", Port: 3306}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-b", Port: 3307}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
}

// TestSnapshotRestoreForgetsUnseenStaleInstances ensures the recency guard does
// not prevent restore from forgetting instances that are both absent from the
// snapshot and already stale locally (the pre-existing behavior for genuine
// decommissions).
func TestSnapshotRestoreForgetsUnseenStaleInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 1
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "stale-a", 3306)
writeTestInstance(t, "stale-b", 3307)
setInstanceLastSeen(t, "stale-a", 3306, 2)
setInstanceLastSeen(t, "stale-b", 3307, 2)

snapshotData := NewSnapshotData()

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-a", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-b", Port: 3307}))
}
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' raft: guard snapshot restore from deleting recently-seen instances by ahmetsoguksu · Pull Request #124 · ProxySQL/orchestrator · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line numberDiff line numberDiff 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
Original file line numberDiff line numberDiff 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 DownExpand 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

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()
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.

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)
Comment on lines +165 to +174

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/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' go

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

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

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

Repository: 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,})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
202 changes: 202 additions & 0 deletions go/logic/snapshot_data_test.go
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
/*
Copyright 2026 ProxySQL Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logic

import (
"bytes"
"compress/gzip"
"encoding/json"
"io"
"os"
"path/filepath"
"testing"
"time"

test "github.com/proxysql/golib/tests"
"github.com/proxysql/orchestrator/go/config"
"github.com/proxysql/orchestrator/go/db"
"github.com/proxysql/orchestrator/go/inst"
)

func TestMain(m *testing.M) {
config.MarkConfigurationLoaded()
// keep hostname resolution local-only for the whole test binary, so that
// ResolveHostname never spawns asynchronous backend writes
config.Config.HostnameResolveMethod = "none"
os.Exit(m.Run())
}

// waitForInstanceDaoInit blocks until the background initializeInstanceDao()
// goroutine has created the package-level caches, which are required by
// WriteInstance/ForgetInstance.
func waitForInstanceDaoInit(t *testing.T) {
t.Helper()
deadline := time.Now().Add(10 * time.Second)
for {
ready := false
func() {
defer func() {
_ = recover()
}()
inst.InstanceIsForgotten(&inst.InstanceKey{})
ready = true
}()
if ready {
return
}
if time.Now().After(deadline) {
t.Fatal("instance DAO caches were not initialized in time")
}
time.Sleep(10 * time.Millisecond)
}
}

func setupSQLiteBackend(t *testing.T) {
t.Helper()
waitForInstanceDaoInit(t)
origBackendDB := config.Config.BackendDB
origSQLiteDataFile := config.Config.SQLite3DataFile
origHostnameResolveMethod := config.Config.HostnameResolveMethod
config.Config.BackendDB = "sqlite"
config.Config.SQLite3DataFile = filepath.Join(t.TempDir(), "orchestrator.sqlite3")
// avoid asynchronous hostname-resolve writes racing with the backend switch
config.Config.HostnameResolveMethod = "none"
t.Cleanup(func() {
config.Config.BackendDB = origBackendDB
config.Config.SQLite3DataFile = origSQLiteDataFile
config.Config.HostnameResolveMethod = origHostnameResolveMethod
})
_, err := db.OpenOrchestrator()
test.S(t).ExpectNil(err)
}

func writeTestInstance(t *testing.T, hostname string, port int) {
t.Helper()
instance := &inst.Instance{Key: inst.InstanceKey{Hostname: hostname, Port: port}}
test.S(t).ExpectNil(inst.WriteInstance(instance, false, nil))
}

func setInstanceLastSeen(t *testing.T, hostname string, port int, hoursAgo int) {
t.Helper()
_, err := db.ExecOrchestrator(`
update database_instance
set last_seen = NOW() - interval ? hour
where hostname = ? and port = ?`,
hoursAgo, hostname, port,
)
test.S(t).ExpectNil(err)
}

func readInstanceKeyMap(t *testing.T) *inst.InstanceKeyMap {
t.Helper()
keys, err := inst.ReadAllInstanceKeys()
test.S(t).ExpectNil(err)
keyMap := inst.NewInstanceKeyMap()
keyMap.AddKeys(keys)
return keyMap
}

func mkSnapshotReader(t *testing.T, snapshotData *SnapshotData) io.ReadCloser {
t.Helper()
b, err := json.Marshal(snapshotData)
test.S(t).ExpectNil(err)
var buf bytes.Buffer
zw := gzip.NewWriter(&buf)
_, err = zw.Write(b)
test.S(t).ExpectNil(err)
test.S(t).ExpectNil(zw.Close())
return io.NopCloser(bytes.NewReader(buf.Bytes()))
}

func TestReadRecentlySeenInstanceKeyMap(t *testing.T) {
setupSQLiteBackend(t)

writeTestInstance(t, "fresh", 3306)
writeTestInstance(t, "borderline", 3306)
writeTestInstance(t, "stale", 3306)
writeTestInstance(t, "never-seen", 3306)
setInstanceLastSeen(t, "borderline", 3306, 239)
setInstanceLastSeen(t, "stale", 3306, 241)
_, err := db.ExecOrchestrator(`update database_instance set last_seen = NULL where hostname = 'never-seen'`)
test.S(t).ExpectNil(err)

keys, err := inst.ReadRecentlySeenInstanceKeyMap(240)
test.S(t).ExpectNil(err)

test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "fresh", Port: 3306}))
test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "borderline", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "never-seen", Port: 3306}))
}

// TestSnapshotRestoreRetainsRecentlySeenInstances verifies the fix for issue #123:
// instances that exist in the local backend but are absent from a (stale) raft
// snapshot must not be purged at restore time while they are still within the
// UnseenInstanceForgetHours recency window. Genuinely stale instances must
// still be forgotten, and instances carried by the snapshot must be added.
func TestSnapshotRestoreRetainsRecentlySeenInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 240
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "fresh-a", 3306)
writeTestInstance(t, "fresh-b", 3307)
writeTestInstance(t, "stale", 3306)
setInstanceLastSeen(t, "stale", 3306, 300)

snapshotData := NewSnapshotData()
snapshotData.MinimalInstances = []inst.MinimalInstance{
{Key: inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}},
}

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-a", Port: 3306}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-b", Port: 3307}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
}

// TestSnapshotRestoreForgetsUnseenStaleInstances ensures the recency guard does
// not prevent restore from forgetting instances that are both absent from the
// snapshot and already stale locally (the pre-existing behavior for genuine
// decommissions).
func TestSnapshotRestoreForgetsUnseenStaleInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 1
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "stale-a", 3306)
writeTestInstance(t, "stale-b", 3307)
setInstanceLastSeen(t, "stale-a", 3306, 2)
setInstanceLastSeen(t, "stale-b", 3307, 2)

snapshotData := NewSnapshotData()

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-a", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-b", Port: 3307}))
}
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' raft: guard snapshot restore from deleting recently-seen instances by ahmetsoguksu · Pull Request #124 · ProxySQL/orchestrator · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line numberDiff line numberDiff 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
Original file line numberDiff line numberDiff 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 DownExpand 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

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()
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.

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)
Comment on lines +165 to +174

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/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' go

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

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

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

Repository: 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,})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
202 changes: 202 additions & 0 deletions go/logic/snapshot_data_test.go
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
/*
Copyright 2026 ProxySQL Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logic

import (
"bytes"
"compress/gzip"
"encoding/json"
"io"
"os"
"path/filepath"
"testing"
"time"

test "github.com/proxysql/golib/tests"
"github.com/proxysql/orchestrator/go/config"
"github.com/proxysql/orchestrator/go/db"
"github.com/proxysql/orchestrator/go/inst"
)

func TestMain(m *testing.M) {
config.MarkConfigurationLoaded()
// keep hostname resolution local-only for the whole test binary, so that
// ResolveHostname never spawns asynchronous backend writes
config.Config.HostnameResolveMethod = "none"
os.Exit(m.Run())
}

// waitForInstanceDaoInit blocks until the background initializeInstanceDao()
// goroutine has created the package-level caches, which are required by
// WriteInstance/ForgetInstance.
func waitForInstanceDaoInit(t *testing.T) {
t.Helper()
deadline := time.Now().Add(10 * time.Second)
for {
ready := false
func() {
defer func() {
_ = recover()
}()
inst.InstanceIsForgotten(&inst.InstanceKey{})
ready = true
}()
if ready {
return
}
if time.Now().After(deadline) {
t.Fatal("instance DAO caches were not initialized in time")
}
time.Sleep(10 * time.Millisecond)
}
}

func setupSQLiteBackend(t *testing.T) {
t.Helper()
waitForInstanceDaoInit(t)
origBackendDB := config.Config.BackendDB
origSQLiteDataFile := config.Config.SQLite3DataFile
origHostnameResolveMethod := config.Config.HostnameResolveMethod
config.Config.BackendDB = "sqlite"
config.Config.SQLite3DataFile = filepath.Join(t.TempDir(), "orchestrator.sqlite3")
// avoid asynchronous hostname-resolve writes racing with the backend switch
config.Config.HostnameResolveMethod = "none"
t.Cleanup(func() {
config.Config.BackendDB = origBackendDB
config.Config.SQLite3DataFile = origSQLiteDataFile
config.Config.HostnameResolveMethod = origHostnameResolveMethod
})
_, err := db.OpenOrchestrator()
test.S(t).ExpectNil(err)
}

func writeTestInstance(t *testing.T, hostname string, port int) {
t.Helper()
instance := &inst.Instance{Key: inst.InstanceKey{Hostname: hostname, Port: port}}
test.S(t).ExpectNil(inst.WriteInstance(instance, false, nil))
}

func setInstanceLastSeen(t *testing.T, hostname string, port int, hoursAgo int) {
t.Helper()
_, err := db.ExecOrchestrator(`
update database_instance
set last_seen = NOW() - interval ? hour
where hostname = ? and port = ?`,
hoursAgo, hostname, port,
)
test.S(t).ExpectNil(err)
}

func readInstanceKeyMap(t *testing.T) *inst.InstanceKeyMap {
t.Helper()
keys, err := inst.ReadAllInstanceKeys()
test.S(t).ExpectNil(err)
keyMap := inst.NewInstanceKeyMap()
keyMap.AddKeys(keys)
return keyMap
}

func mkSnapshotReader(t *testing.T, snapshotData *SnapshotData) io.ReadCloser {
t.Helper()
b, err := json.Marshal(snapshotData)
test.S(t).ExpectNil(err)
var buf bytes.Buffer
zw := gzip.NewWriter(&buf)
_, err = zw.Write(b)
test.S(t).ExpectNil(err)
test.S(t).ExpectNil(zw.Close())
return io.NopCloser(bytes.NewReader(buf.Bytes()))
}

func TestReadRecentlySeenInstanceKeyMap(t *testing.T) {
setupSQLiteBackend(t)

writeTestInstance(t, "fresh", 3306)
writeTestInstance(t, "borderline", 3306)
writeTestInstance(t, "stale", 3306)
writeTestInstance(t, "never-seen", 3306)
setInstanceLastSeen(t, "borderline", 3306, 239)
setInstanceLastSeen(t, "stale", 3306, 241)
_, err := db.ExecOrchestrator(`update database_instance set last_seen = NULL where hostname = 'never-seen'`)
test.S(t).ExpectNil(err)

keys, err := inst.ReadRecentlySeenInstanceKeyMap(240)
test.S(t).ExpectNil(err)

test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "fresh", Port: 3306}))
test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "borderline", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "never-seen", Port: 3306}))
}

// TestSnapshotRestoreRetainsRecentlySeenInstances verifies the fix for issue #123:
// instances that exist in the local backend but are absent from a (stale) raft
// snapshot must not be purged at restore time while they are still within the
// UnseenInstanceForgetHours recency window. Genuinely stale instances must
// still be forgotten, and instances carried by the snapshot must be added.
func TestSnapshotRestoreRetainsRecentlySeenInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 240
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "fresh-a", 3306)
writeTestInstance(t, "fresh-b", 3307)
writeTestInstance(t, "stale", 3306)
setInstanceLastSeen(t, "stale", 3306, 300)

snapshotData := NewSnapshotData()
snapshotData.MinimalInstances = []inst.MinimalInstance{
{Key: inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}},
}

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-a", Port: 3306}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-b", Port: 3307}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
}

// TestSnapshotRestoreForgetsUnseenStaleInstances ensures the recency guard does
// not prevent restore from forgetting instances that are both absent from the
// snapshot and already stale locally (the pre-existing behavior for genuine
// decommissions).
func TestSnapshotRestoreForgetsUnseenStaleInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 1
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "stale-a", 3306)
writeTestInstance(t, "stale-b", 3307)
setInstanceLastSeen(t, "stale-a", 3306, 2)
setInstanceLastSeen(t, "stale-b", 3307, 2)

snapshotData := NewSnapshotData()

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-a", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-b", Port: 3307}))
}
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' raft: guard snapshot restore from deleting recently-seen instances by ahmetsoguksu · Pull Request #124 · ProxySQL/orchestrator · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line numberDiff line numberDiff 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
Original file line numberDiff line numberDiff 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 DownExpand 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

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()
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.

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)
Comment on lines +165 to +174

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/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' go

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

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

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

Repository: 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,})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
202 changes: 202 additions & 0 deletions go/logic/snapshot_data_test.go
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
/*
Copyright 2026 ProxySQL Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logic

import (
"bytes"
"compress/gzip"
"encoding/json"
"io"
"os"
"path/filepath"
"testing"
"time"

test "github.com/proxysql/golib/tests"
"github.com/proxysql/orchestrator/go/config"
"github.com/proxysql/orchestrator/go/db"
"github.com/proxysql/orchestrator/go/inst"
)

func TestMain(m *testing.M) {
config.MarkConfigurationLoaded()
// keep hostname resolution local-only for the whole test binary, so that
// ResolveHostname never spawns asynchronous backend writes
config.Config.HostnameResolveMethod = "none"
os.Exit(m.Run())
}

// waitForInstanceDaoInit blocks until the background initializeInstanceDao()
// goroutine has created the package-level caches, which are required by
// WriteInstance/ForgetInstance.
func waitForInstanceDaoInit(t *testing.T) {
t.Helper()
deadline := time.Now().Add(10 * time.Second)
for {
ready := false
func() {
defer func() {
_ = recover()
}()
inst.InstanceIsForgotten(&inst.InstanceKey{})
ready = true
}()
if ready {
return
}
if time.Now().After(deadline) {
t.Fatal("instance DAO caches were not initialized in time")
}
time.Sleep(10 * time.Millisecond)
}
}

func setupSQLiteBackend(t *testing.T) {
t.Helper()
waitForInstanceDaoInit(t)
origBackendDB := config.Config.BackendDB
origSQLiteDataFile := config.Config.SQLite3DataFile
origHostnameResolveMethod := config.Config.HostnameResolveMethod
config.Config.BackendDB = "sqlite"
config.Config.SQLite3DataFile = filepath.Join(t.TempDir(), "orchestrator.sqlite3")
// avoid asynchronous hostname-resolve writes racing with the backend switch
config.Config.HostnameResolveMethod = "none"
t.Cleanup(func() {
config.Config.BackendDB = origBackendDB
config.Config.SQLite3DataFile = origSQLiteDataFile
config.Config.HostnameResolveMethod = origHostnameResolveMethod
})
_, err := db.OpenOrchestrator()
test.S(t).ExpectNil(err)
}

func writeTestInstance(t *testing.T, hostname string, port int) {
t.Helper()
instance := &inst.Instance{Key: inst.InstanceKey{Hostname: hostname, Port: port}}
test.S(t).ExpectNil(inst.WriteInstance(instance, false, nil))
}

func setInstanceLastSeen(t *testing.T, hostname string, port int, hoursAgo int) {
t.Helper()
_, err := db.ExecOrchestrator(`
update database_instance
set last_seen = NOW() - interval ? hour
where hostname = ? and port = ?`,
hoursAgo, hostname, port,
)
test.S(t).ExpectNil(err)
}

func readInstanceKeyMap(t *testing.T) *inst.InstanceKeyMap {
t.Helper()
keys, err := inst.ReadAllInstanceKeys()
test.S(t).ExpectNil(err)
keyMap := inst.NewInstanceKeyMap()
keyMap.AddKeys(keys)
return keyMap
}

func mkSnapshotReader(t *testing.T, snapshotData *SnapshotData) io.ReadCloser {
t.Helper()
b, err := json.Marshal(snapshotData)
test.S(t).ExpectNil(err)
var buf bytes.Buffer
zw := gzip.NewWriter(&buf)
_, err = zw.Write(b)
test.S(t).ExpectNil(err)
test.S(t).ExpectNil(zw.Close())
return io.NopCloser(bytes.NewReader(buf.Bytes()))
}

func TestReadRecentlySeenInstanceKeyMap(t *testing.T) {
setupSQLiteBackend(t)

writeTestInstance(t, "fresh", 3306)
writeTestInstance(t, "borderline", 3306)
writeTestInstance(t, "stale", 3306)
writeTestInstance(t, "never-seen", 3306)
setInstanceLastSeen(t, "borderline", 3306, 239)
setInstanceLastSeen(t, "stale", 3306, 241)
_, err := db.ExecOrchestrator(`update database_instance set last_seen = NULL where hostname = 'never-seen'`)
test.S(t).ExpectNil(err)

keys, err := inst.ReadRecentlySeenInstanceKeyMap(240)
test.S(t).ExpectNil(err)

test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "fresh", Port: 3306}))
test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "borderline", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "never-seen", Port: 3306}))
}

// TestSnapshotRestoreRetainsRecentlySeenInstances verifies the fix for issue #123:
// instances that exist in the local backend but are absent from a (stale) raft
// snapshot must not be purged at restore time while they are still within the
// UnseenInstanceForgetHours recency window. Genuinely stale instances must
// still be forgotten, and instances carried by the snapshot must be added.
func TestSnapshotRestoreRetainsRecentlySeenInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 240
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "fresh-a", 3306)
writeTestInstance(t, "fresh-b", 3307)
writeTestInstance(t, "stale", 3306)
setInstanceLastSeen(t, "stale", 3306, 300)

snapshotData := NewSnapshotData()
snapshotData.MinimalInstances = []inst.MinimalInstance{
{Key: inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}},
}

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-a", Port: 3306}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-b", Port: 3307}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
}

// TestSnapshotRestoreForgetsUnseenStaleInstances ensures the recency guard does
// not prevent restore from forgetting instances that are both absent from the
// snapshot and already stale locally (the pre-existing behavior for genuine
// decommissions).
func TestSnapshotRestoreForgetsUnseenStaleInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 1
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "stale-a", 3306)
writeTestInstance(t, "stale-b", 3307)
setInstanceLastSeen(t, "stale-a", 3306, 2)
setInstanceLastSeen(t, "stale-b", 3307, 2)

snapshotData := NewSnapshotData()

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-a", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-b", Port: 3307}))
}
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); raft: guard snapshot restore from deleting recently-seen instances by ahmetsoguksu · Pull Request #124 · ProxySQL/orchestrator · GitHub
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line numberDiff line numberDiff 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
Original file line numberDiff line numberDiff 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 DownExpand 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

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()
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.

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)
Comment on lines +165 to +174

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/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' go

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

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

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

Repository: 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,})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
202 changes: 202 additions & 0 deletions go/logic/snapshot_data_test.go
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
/*
Copyright 2026 ProxySQL Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logic

import (
"bytes"
"compress/gzip"
"encoding/json"
"io"
"os"
"path/filepath"
"testing"
"time"

test "github.com/proxysql/golib/tests"
"github.com/proxysql/orchestrator/go/config"
"github.com/proxysql/orchestrator/go/db"
"github.com/proxysql/orchestrator/go/inst"
)

func TestMain(m *testing.M) {
config.MarkConfigurationLoaded()
// keep hostname resolution local-only for the whole test binary, so that
// ResolveHostname never spawns asynchronous backend writes
config.Config.HostnameResolveMethod = "none"
os.Exit(m.Run())
}

// waitForInstanceDaoInit blocks until the background initializeInstanceDao()
// goroutine has created the package-level caches, which are required by
// WriteInstance/ForgetInstance.
func waitForInstanceDaoInit(t *testing.T) {
t.Helper()
deadline := time.Now().Add(10 * time.Second)
for {
ready := false
func() {
defer func() {
_ = recover()
}()
inst.InstanceIsForgotten(&inst.InstanceKey{})
ready = true
}()
if ready {
return
}
if time.Now().After(deadline) {
t.Fatal("instance DAO caches were not initialized in time")
}
time.Sleep(10 * time.Millisecond)
}
}

func setupSQLiteBackend(t *testing.T) {
t.Helper()
waitForInstanceDaoInit(t)
origBackendDB := config.Config.BackendDB
origSQLiteDataFile := config.Config.SQLite3DataFile
origHostnameResolveMethod := config.Config.HostnameResolveMethod
config.Config.BackendDB = "sqlite"
config.Config.SQLite3DataFile = filepath.Join(t.TempDir(), "orchestrator.sqlite3")
// avoid asynchronous hostname-resolve writes racing with the backend switch
config.Config.HostnameResolveMethod = "none"
t.Cleanup(func() {
config.Config.BackendDB = origBackendDB
config.Config.SQLite3DataFile = origSQLiteDataFile
config.Config.HostnameResolveMethod = origHostnameResolveMethod
})
_, err := db.OpenOrchestrator()
test.S(t).ExpectNil(err)
}

func writeTestInstance(t *testing.T, hostname string, port int) {
t.Helper()
instance := &inst.Instance{Key: inst.InstanceKey{Hostname: hostname, Port: port}}
test.S(t).ExpectNil(inst.WriteInstance(instance, false, nil))
}

func setInstanceLastSeen(t *testing.T, hostname string, port int, hoursAgo int) {
t.Helper()
_, err := db.ExecOrchestrator(`
update database_instance
set last_seen = NOW() - interval ? hour
where hostname = ? and port = ?`,
hoursAgo, hostname, port,
)
test.S(t).ExpectNil(err)
}

func readInstanceKeyMap(t *testing.T) *inst.InstanceKeyMap {
t.Helper()
keys, err := inst.ReadAllInstanceKeys()
test.S(t).ExpectNil(err)
keyMap := inst.NewInstanceKeyMap()
keyMap.AddKeys(keys)
return keyMap
}

func mkSnapshotReader(t *testing.T, snapshotData *SnapshotData) io.ReadCloser {
t.Helper()
b, err := json.Marshal(snapshotData)
test.S(t).ExpectNil(err)
var buf bytes.Buffer
zw := gzip.NewWriter(&buf)
_, err = zw.Write(b)
test.S(t).ExpectNil(err)
test.S(t).ExpectNil(zw.Close())
return io.NopCloser(bytes.NewReader(buf.Bytes()))
}

func TestReadRecentlySeenInstanceKeyMap(t *testing.T) {
setupSQLiteBackend(t)

writeTestInstance(t, "fresh", 3306)
writeTestInstance(t, "borderline", 3306)
writeTestInstance(t, "stale", 3306)
writeTestInstance(t, "never-seen", 3306)
setInstanceLastSeen(t, "borderline", 3306, 239)
setInstanceLastSeen(t, "stale", 3306, 241)
_, err := db.ExecOrchestrator(`update database_instance set last_seen = NULL where hostname = 'never-seen'`)
test.S(t).ExpectNil(err)

keys, err := inst.ReadRecentlySeenInstanceKeyMap(240)
test.S(t).ExpectNil(err)

test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "fresh", Port: 3306}))
test.S(t).ExpectTrue(keys.HasKey(inst.InstanceKey{Hostname: "borderline", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
test.S(t).ExpectFalse(keys.HasKey(inst.InstanceKey{Hostname: "never-seen", Port: 3306}))
}

// TestSnapshotRestoreRetainsRecentlySeenInstances verifies the fix for issue #123:
// instances that exist in the local backend but are absent from a (stale) raft
// snapshot must not be purged at restore time while they are still within the
// UnseenInstanceForgetHours recency window. Genuinely stale instances must
// still be forgotten, and instances carried by the snapshot must be added.
func TestSnapshotRestoreRetainsRecentlySeenInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 240
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "fresh-a", 3306)
writeTestInstance(t, "fresh-b", 3307)
writeTestInstance(t, "stale", 3306)
setInstanceLastSeen(t, "stale", 3306, 300)

snapshotData := NewSnapshotData()
snapshotData.MinimalInstances = []inst.MinimalInstance{
{Key: inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}},
}

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-a", Port: 3306}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "fresh-b", Port: 3307}))
test.S(t).ExpectTrue(keyMap.HasKey(inst.InstanceKey{Hostname: "in-snapshot", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale", Port: 3306}))
}

// TestSnapshotRestoreForgetsUnseenStaleInstances ensures the recency guard does
// not prevent restore from forgetting instances that are both absent from the
// snapshot and already stale locally (the pre-existing behavior for genuine
// decommissions).
func TestSnapshotRestoreForgetsUnseenStaleInstances(t *testing.T) {
setupSQLiteBackend(t)

origUnseenHours := config.Config.UnseenInstanceForgetHours
config.Config.UnseenInstanceForgetHours = 1
t.Cleanup(func() { config.Config.UnseenInstanceForgetHours = origUnseenHours })

writeTestInstance(t, "stale-a", 3306)
writeTestInstance(t, "stale-b", 3307)
setInstanceLastSeen(t, "stale-a", 3306, 2)
setInstanceLastSeen(t, "stale-b", 3307, 2)

snapshotData := NewSnapshotData()

applier := NewSnapshotDataCreatorApplier()
test.S(t).ExpectNil(applier.Restore(mkSnapshotReader(t, snapshotData)))

keyMap := readInstanceKeyMap(t)
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-a", Port: 3306}))
test.S(t).ExpectFalse(keyMap.HasKey(inst.InstanceKey{Hostname: "stale-b", Port: 3307}))
}
Loading
Loading