Skip to content

Handle ProxySQL cleanup during sandbox deletion #48

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
renecannao merged 1 commit into from
Mar 24, 2026
Merged

Handle ProxySQL cleanup during sandbox deletion #48

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions sandbox/sandbox.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1245,6 +1245,13 @@ func RemoveSandbox(sandboxDir, sandbox string, runConcurrently bool) (execList [
}
}

// If a proxysql sub-sandbox exists, stop it before removing the directory
proxysqlStop := path.Join(fullPath, "proxysql", "stop")
if _, statErr := os.Stat(proxysqlStop); statErr == nil {
common.CondPrintf("Stopping ProxySQL in %s\n", path.Join(fullPath, "proxysql"))
_, _ = common.RunCmd(proxysqlStop)
Comment on lines +1249 to +1252
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

When runConcurrently is true, this block executes proxysql/stop immediately instead of returning it as part of execList like the main sandbox stop/removal steps. That breaks the function’s concurrency contract (side effects during planning) and can also run ProxySQL stop before the scheduled DB stop. Consider using common.ExecExists and, if runConcurrently, append a concurrent.ExecCommand for proxysqlStop with the same priority as the main stop; if not concurrent, run it and handle/propagate errors (instead of ignoring them) so a failed stop doesn’t silently proceed to deletion.

Suggested change
proxysqlStop := path.Join(fullPath, "proxysql", "stop")
if _, statErr := os.Stat(proxysqlStop); statErr == nil {
common.CondPrintf("Stopping ProxySQL in %s\n", path.Join(fullPath, "proxysql"))
_, _ = common.RunCmd(proxysqlStop)
proxysqlSandbox := path.Join(fullPath, "proxysql")
proxysqlStop := path.Join(proxysqlSandbox, "stop")
if common.ExecExists(proxysqlStop) {
if runConcurrently {
var proxysqlCommand = concurrent.ExecCommand{
Cmd: proxysqlStop,
Args: []string{},
}
// Use the same priority as the main stop command
execList = append(execList, concurrent.ExecutionList{Logger: nil, Priority: 0, Command: proxysqlCommand})
} else {
common.CondPrintf("Stopping ProxySQL in %s\n", proxysqlSandbox)
_, err = common.RunCmd(proxysqlStop)
if err != nil {
return emptyExecutionList, fmt.Errorf("error while stopping ProxySQL in %s: %s", proxysqlSandbox, err)
}
}

Copilot uses AI. Check for mistakes.
}
Comment on lines +1248 to +1253
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

New behavior (detecting and stopping a proxysql/stop script) isn’t covered by tests. Since sandbox/sandbox_test.go already exercises sandbox removal paths, please add a unit/integration-style test that creates a proxysql/stop script (e.g., one that writes a sentinel file) and asserts it is invoked during deletion, both for the concurrent and non-concurrent paths (as applicable).

Copilot uses AI. Check for mistakes.

rmTargets := []string{fullPath, logDirectory}

for _, target := range rmTargets {
Expand Down Expand Up @@ -1359,6 +1366,13 @@ func RemoveCustomSandbox(sandboxDir, sandbox string, runConcurrently, useStop bo
}
}

// If a proxysql sub-sandbox exists, stop it before removing the directory
proxysqlStop := path.Join(fullPath, "proxysql", "stop")
if _, statErr := os.Stat(proxysqlStop); statErr == nil {
common.CondPrintf("Stopping ProxySQL in %s\n", path.Join(fullPath, "proxysql"))
_, _ = common.RunCmd(proxysqlStop)
Comment on lines +1371 to +1373
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

Same issue here as in RemoveSandbox: this runs proxysql/stop synchronously even when runConcurrently is true, and it ignores execution errors. Please make ProxySQL stop follow the same execution model as the rest of the deletion workflow (enqueue as a priority-0 task when concurrent; otherwise run and handle/propagate errors). Also prefer common.ExecExists over os.Stat so we only attempt to execute an actually-executable stop script.

Suggested change
if _, statErr := os.Stat(proxysqlStop); statErr == nil {
common.CondPrintf("Stopping ProxySQL in %s\n", path.Join(fullPath, "proxysql"))
_, _ = common.RunCmd(proxysqlStop)
if common.ExecExists(proxysqlStop) {
common.CondPrintf("Stopping ProxySQL in %s\n", path.Join(fullPath, "proxysql"))
if runConcurrently {
var proxysqlCommand = concurrent.ExecCommand{
Cmd: proxysqlStop,
}
execList = append(execList, concurrent.ExecutionList{Logger: nil, Priority: 0, Command: proxysqlCommand})
} else {
if _, err = common.RunCmd(proxysqlStop); err != nil {
return emptyExecutionList, fmt.Errorf(globals.ErrWhileStoppingSandbox, path.Join(fullPath, "proxysql"))
}
}

Copilot uses AI. Check for mistakes.
}

rmTargets := []string{fullPath, logDirectory}

for _, target := range rmTargets {
Expand Down
Loading