amnezia-vpn/amnezia-client

Fork 0

mirror of https://github.com/amnezia-vpn/amnezia-client.git synced 2026-10-14 21:34:26 +00:00

[PR #2454] fix(openvpn): break Disconnect → infinite reconnect loop in stop() #2814

New issue

Open

opened 2026-10-14 21:05:28 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-14 21:05:28 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/amnezia-vpn/amnezia-client/pull/2454
Author: @alexhmt
Created: 4/8/2026
Status: 🔄 Open

Base: dev ← Head: fix/openvpn-disconnect-reconnect-loop

📝 Commits (1)

5f45509 fix(openvpn): break Disconnect → infinite reconnect loop in stop()

📊 Changes

1 file changed (+25 additions, -9 deletions)

View changed files

📝 client/protocols/openvpnprotocol.cpp (+25 -9)

📄 Description

Summary

When the user clicks Disconnect, the desktop client gets stuck alternating between Disconnected and Reconnecting… forever; the only way out is to manually kill openvpn.exe. Reproduced on AmneziaVPN 4.8.14.5 + amnezia-openvpn-cloak container on Windows 10 22H2.

Two compounding bugs in `OpenVpnProtocol::stop()`:

```cpp
void OpenVpnProtocol::stop()
{
qDebug() << "OpenVpnProtocol::stop()";
setConnectionState(Vpn::ConnectionState::Disconnecting);

// TODO: need refactoring
// sendTermSignal() will even return true while server connected ???
if ((m_connectionState == Connected) || ...) {
    if (!sendTermSignal()) { killOpenVpnProcess(); }
    QThread::msleep(10);
    m_managementServer.stop();
}
...

}
```

Bug 1 — the shutdown if-block is unreachable

`VpnProtocol::setConnectionState()` synchronously assigns `m_connectionState`, so by the time the `if` on the next line checks `m_connectionState` it is already `Disconnecting` and never matches `Connected` / `Connecting` / `Reconnecting` / `Preparing`. Neither `sendTermSignal()` nor `killOpenVpnProcess()` is ever called from `stop()`. The author already left a `// TODO: need refactoring` next to it.

Bug 2 — the 10 ms sleep is too short and `sendTermSignal()`'s return value is misleading

`sendTermSignal()` is just `m_managementServer.writeCommand("signal SIGTERM")` — it returns `true` as soon as bytes are written to the management socket, NOT when openvpn actually processes the command. 10 ms isn't enough for openvpn to read and act on it before `m_managementServer.stop()` tears the socket down. openvpn then sees the closed management socket as a connection-reset and emits:

```
SIGUSR1[soft,connection-reset] received, process restarting
MANAGEMENT: >STATE: RECONNECTING,connection-reset
Restart pause, 5 second(s)
```

The UI sees the `RECONNECTING` state and gets stuck. (In the unpatched code this happens via `OpenVpnOverCloakProtocol::stop()` / the `~OpenVpnProtocol` destructor path that does close the management server, since the `if`-block in `OpenVpnProtocol::stop()` is unreachable.)

Fix

Capture `prevState` BEFORE `setConnectionState()` mutates `m_connectionState`, so the kill path actually runs when `stop()` is called from a live state.
Always send SIGTERM via the management interface (drop the misleading `if (!sendTermSignal())` short-circuit).
Sleep 200 ms so openvpn can actually receive and process SIGTERM before the management socket is closed.
Always call `killOpenVpnProcess()` afterwards as a hard fallback. If SIGTERM was processed in time the process is already gone and this is a no-op; if it wasn't, this breaks the soft-restart loop.

Verification

Tested on Windows 10 22H2 with the `amnezia-openvpn-cloak` server container, by building this branch via a stripped-down windows-only GitHub Actions workflow on `windows-latest` and replacing `AmneziaVPN.exe` + `AmneziaVPN-service.exe` in the install directory. Several connect → disconnect → connect cycles with timestamps from `AmneziaVPN.log`:

stop()	→ Disconnected	gap
09:14:39.585	09:14:39.824	239 ms
09:14:43.307	09:14:43.511	204 ms
09:15:00.791	09:15:01.013	222 ms

The ~200 ms gap proves the new shutdown block is actually executed (the original block was effectively a no-op and the same sequence completed in 4 ms).

After the patch the log contains no occurrences of any of:

`SIGUSR1[soft,connection-reset] received`
`Connection reset, restarting [-1]`
`Restart pause, 5 second(s)`
`STATE: RECONNECTING,connection-reset`
`Connection state: 'Переподключение...'`

Connect → Connected → Disconnect → Disconnected works cleanly, repeatedly, without leaving `openvpn.exe` orphaned.

Test plan

Reproduce the bug on a clean install of 4.8.14.5
Build patched binaries from this branch
Replace `AmneziaVPN.exe` + `AmneziaVPN-service.exe` and re-test
Verify multiple connect/disconnect cycles in a row
Verify no orphaned `openvpn.exe` processes after Disconnect
Verify log no longer contains `SIGUSR1[soft,connection-reset]` after stop()

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/amnezia-vpn/amnezia-client/pull/2454 **Author:** [@alexhmt](https://github.com/alexhmt) **Created:** 4/8/2026 **Status:** 🔄 Open **Base:** `dev` ← **Head:** `fix/openvpn-disconnect-reconnect-loop` --- ### 📝 Commits (1) - [`5f45509`](https://github.com/amnezia-vpn/amnezia-client/commit/5f4550988fb59c030998a1e0767f4ae04e9a5c71) fix(openvpn): break Disconnect → infinite reconnect loop in stop() ### 📊 Changes **1 file changed** (+25 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `client/protocols/openvpnprotocol.cpp` (+25 -9) </details> ### 📄 Description ## Summary When the user clicks **Disconnect**, the desktop client gets stuck alternating between *Disconnected* and *Reconnecting…* forever; the only way out is to manually kill `openvpn.exe`. Reproduced on AmneziaVPN **4.8.14.5** + `amnezia-openvpn-cloak` container on Windows 10 22H2. Two compounding bugs in [\`OpenVpnProtocol::stop()\`](https://github.com/amnezia-vpn/amnezia-client/blob/dev/client/protocols/openvpnprotocol.cpp#L41): \`\`\`cpp void OpenVpnProtocol::stop() { qDebug() << \"OpenVpnProtocol::stop()\"; setConnectionState(Vpn::ConnectionState::Disconnecting); // TODO: need refactoring // sendTermSignal() will even return true while server connected ??? if ((m_connectionState == Connected) || ...) { if (!sendTermSignal()) { killOpenVpnProcess(); } QThread::msleep(10); m_managementServer.stop(); } ... } \`\`\` ### Bug 1 — the shutdown if-block is unreachable [\`VpnProtocol::setConnectionState()\`](https://github.com/amnezia-vpn/amnezia-client/blob/dev/client/protocols/vpnprotocol.cpp#L90) synchronously assigns \`m_connectionState\`, so by the time the \`if\` on the next line checks \`m_connectionState\` it is already \`Disconnecting\` and never matches \`Connected\` / \`Connecting\` / \`Reconnecting\` / \`Preparing\`. Neither \`sendTermSignal()\` nor \`killOpenVpnProcess()\` is ever called from \`stop()\`. The author already left a \`// TODO: need refactoring\` next to it. ### Bug 2 — the 10 ms sleep is too short and \`sendTermSignal()\`'s return value is misleading \`sendTermSignal()\` is just \`m_managementServer.writeCommand(\"signal SIGTERM\")\` — it returns \`true\` as soon as bytes are written to the management socket, NOT when openvpn actually processes the command. 10 ms isn't enough for openvpn to read and act on it before \`m_managementServer.stop()\` tears the socket down. openvpn then sees the closed management socket as a connection-reset and emits: \`\`\` SIGUSR1[soft,connection-reset] received, process restarting MANAGEMENT: >STATE: RECONNECTING,connection-reset Restart pause, 5 second(s) \`\`\` The UI sees the \`RECONNECTING\` state and gets stuck. (In the unpatched code this happens via \`OpenVpnOverCloakProtocol::stop()\` / the \`~OpenVpnProtocol\` destructor path that does close the management server, since the \`if\`-block in \`OpenVpnProtocol::stop()\` is unreachable.) ## Fix * Capture \`prevState\` BEFORE \`setConnectionState()\` mutates \`m_connectionState\`, so the kill path actually runs when \`stop()\` is called from a live state. * Always send SIGTERM via the management interface (drop the misleading \`if (!sendTermSignal())\` short-circuit). * Sleep **200 ms** so openvpn can actually receive and process SIGTERM before the management socket is closed. * Always call \`killOpenVpnProcess()\` afterwards as a hard fallback. If SIGTERM was processed in time the process is already gone and this is a no-op; if it wasn't, this breaks the soft-restart loop. ## Verification Tested on Windows 10 22H2 with the \`amnezia-openvpn-cloak\` server container, by building this branch via a stripped-down windows-only GitHub Actions workflow on \`windows-latest\` and replacing \`AmneziaVPN.exe\` + \`AmneziaVPN-service.exe\` in the install directory. Several connect → disconnect → connect cycles with timestamps from \`AmneziaVPN.log\`: | stop() | → Disconnected | gap | |---|---|---| | 09:14:39.585 | 09:14:39.824 | **239 ms** | | 09:14:43.307 | 09:14:43.511 | **204 ms** | | 09:15:00.791 | 09:15:01.013 | **222 ms** | The ~200 ms gap proves the new shutdown block is actually executed (the original block was effectively a no-op and the same sequence completed in 4 ms). After the patch the log contains **no occurrences** of any of: - \`SIGUSR1[soft,connection-reset] received\` - \`Connection reset, restarting [-1]\` - \`Restart pause, 5 second(s)\` - \`STATE: RECONNECTING,connection-reset\` - \`Connection state: 'Переподключение...'\` Connect → Connected → Disconnect → Disconnected works cleanly, repeatedly, without leaving \`openvpn.exe\` orphaned. ## Test plan - [x] Reproduce the bug on a clean install of 4.8.14.5 - [x] Build patched binaries from this branch - [x] Replace \`AmneziaVPN.exe\` + \`AmneziaVPN-service.exe\` and re-test - [x] Verify multiple connect/disconnect cycles in a row - [x] Verify no orphaned \`openvpn.exe\` processes after Disconnect - [x] Verify log no longer contains \`SIGUSR1[soft,connection-reset]\` after stop() --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-14 21:05:28 +00:00

No labels

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

amnezia-vpn/amnezia-client#2814

No description provided.

Rows
Columns