Murzaby/s-ui
1
0
Fork 0
mirror of https://github.com/alireza0/s-ui.git synced 2026-10-20 19:02:09 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

unified pipelines
Some checks are pending
Release S-UI / test (push) Waiting to run
Details
Release S-UI / build-frontend (push) Blocked by required conditions
Details
Release S-UI / build-386 (push) Blocked by required conditions
Details
Release S-UI / build-amd64 (push) Blocked by required conditions
Details
Release S-UI / build-armv5 (push) Blocked by required conditions
Details
Release S-UI / build-armv6 (push) Blocked by required conditions
Details
Release S-UI / build-armv7 (push) Blocked by required conditions
Details
Release S-UI / build-arm64 (push) Blocked by required conditions
Details
Release S-UI / build-s390x (push) Blocked by required conditions
Details
Release S-UI / build-windows-amd64 (push) Blocked by required conditions
Details
Release S-UI / build-windows-arm64 (push) Blocked by required conditions
Details
Release S-UI / publish (push) Blocked by required conditions
Details

Browse source
This commit is contained in:
Alireza Ahmadi 2026-09-12 17:25:04 +02:00
parent 9ec56e93b1
commit dcbe9a8728
22 changed files with 300 additions and 532 deletions
Download patch file Download diff file Expand all files Collapse all files

125
.github/workflows/release.yml vendored
View file

@ -9,22 +9,27 @@ on:
- main
tags:
- "*"
paths:
- '.github/workflows/release.yml'
- 'frontend/**'
- '**.sh'
- '**.go'
- 'go.mod'
- 'go.sum'
- 's-ui.service'
permissions:
contents: read
env:
NODE_VERSION: "26"
CRONET_GO_REPO: https://github.com/sagernet/cronet-go.git
BOOTLIN_BASE_URL: https://toolchains.bootlin.com/downloads/releases/toolchains
LIBCRONET_BASE_URL: "https://github.com/SagerNet/cronet-go/releases/latest/download"
jobs:
test:
# Skipped on a tag and on a published release: both ship a commit that
# already went through this gate on main.
if: ${{ github.event_name != 'release' && !startsWith(github.ref, 'refs/tags/') }}
uses: ./.github/workflows/test.yml
build-frontend:
needs: test
# A skipped gate is not a failed one, so the build still runs on a tag.
if: ${{ always() && needs.test.result != 'failure' && needs.test.result != 'cancelled' }}
runs-on: ubuntu-latest
steps:
- name: Checkout repository (frontend only)
@ -214,15 +219,109 @@ jobs:
path: ./s-ui-linux-${{ matrix.platform }}.tar.gz
retention-days: 30
build-windows:
needs: build-frontend
name: build-windows-${{ matrix.arch }}
strategy:
fail-fast: false
matrix:
include:
- { arch: amd64, runner: windows-latest, cgo: "1" }
- { arch: arm64, runner: ubuntu-latest, cgo: "0" }
runs-on: ${{ matrix.runner }}
steps:
- name: Checkout repository
uses: actions/checkout@v6.0.2
- name: Download frontend artifact
uses: actions/download-artifact@v8
with:
name: frontend-dist
path: web/html
- name: Setup Go
uses: actions/setup-go@v6
with:
cache: false
go-version-file: go.mod
- name: Install zip for Windows
if: matrix.arch == 'amd64'
shell: powershell
run: |
# Install Chocolatey if not available
if (!(Get-Command choco -ErrorAction SilentlyContinue)) {
Set-ExecutionPolicy Bypass -Scope Process -Force
[System.Net.ServicePointManager]::SecurityProtocol = [System.Net.ServicePointManager]::SecurityProtocol -bor 3072
iex ((New-Object System.Net.WebClient).DownloadString('https://community.chocolatey.org/install.ps1'))
}
# Install zip
choco install zip -y
- name: Build s-ui
shell: bash
run: |
export CGO_ENABLED=${{ matrix.cgo }}
export GOOS=windows
export GOARCH=${{ matrix.arch }}
echo "Building for Windows ${{ matrix.arch }}"
go version
go env GOOS GOARCH
. ./build-tags.sh
TAGS=$(tags_for windows)
LDFLAGS=$(ldflags_for windows)
go build -ldflags="$LDFLAGS" -tags "$TAGS" -o sui.exe main.go
file sui.exe
mkdir s-ui-windows
cp sui.exe s-ui-windows/
cp -r windows/* s-ui-windows/
- name: Download libcronet-go
shell: bash
run: |
curl -qsL -o s-ui-windows/libcronet.dll ${{ env.LIBCRONET_BASE_URL }}/libcronet-windows-${{ matrix.arch }}.dll
- name: Package
shell: bash
run: |
zip -r "s-ui-windows-${{ matrix.arch }}.zip" s-ui-windows
- name: Upload files to Artifacts
uses: actions/upload-artifact@v7
with:
name: s-ui-windows-${{ matrix.arch }}
path: ./s-ui-windows-${{ matrix.arch }}.zip
retention-days: 30
# One place that touches the release, reached only once every build has
# succeeded. Publishing from each matrix leg left a half-populated release
# behind whenever one platform failed, and the two copies of this step had
# drifted to different tag expressions.
publish:
needs: [build-linux, build-windows]
if: |
(github.event_name == 'release' && github.event.action == 'published') ||
(github.event_name == 'push' && startsWith(github.ref, 'refs/tags/'))
runs-on: ubuntu-latest
permissions:
contents: write
steps:
- name: Download build artifacts
uses: actions/download-artifact@v8
with:
pattern: s-ui-*
merge-multiple: true
path: dist
- name: Upload to Release
uses: svenstaro/upload-release-action@v2
if: |
(github.event_name == 'release' && github.event.action == 'published') ||
(github.event_name == 'push' && startsWith(github.ref, 'refs/tags/'))
with:
repo_token: ${{ secrets.GITHUB_TOKEN }}
tag: ${{ github.event_name == 'release' && github.event.release.tag_name || github.ref_name }}
file: s-ui-linux-${{ matrix.platform }}.tar.gz
asset_name: s-ui-linux-${{ matrix.platform }}.tar.gz
file: dist/*
file_glob: true
prerelease: true
overwrite: true

12
.github/workflows/test.yml vendored
View file

@ -1,17 +1,11 @@
name: Test
# No push trigger: release.yml calls this as its gate, so a push would otherwise
# run the suite twice.
on:
workflow_dispatch:
pull_request:
push:
branches:
- main
paths:
- '.github/workflows/test.yml'
- 'build-tags.sh'
- '**.go'
- 'go.mod'
- 'go.sum'
workflow_call:
permissions:
contents: read

142
.github/workflows/windows.yml vendored
View file

@ -1,142 +0,0 @@
name: Build S-UI for Windows
on:
workflow_dispatch:
release:
types: [published]
push:
branches:
- main
tags:
- "*"
paths:
- '.github/workflows/windows.yml'
- 'frontend/**'
- '**.go'
- 'go.mod'
- 'go.sum'
- 'windows/**'
env:
NODE_VERSION: "25"
LIBCRONET_BASE_URL: "https://github.com/SagerNet/cronet-go/releases/latest/download"
jobs:
build-frontend:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v6.0.2
with:
submodules: recursive
fetch-depth: 1
- name: Setup Node.js
uses: actions/setup-node@v6
with:
node-version: ${{ env.NODE_VERSION }}
registry-url: 'https://registry.npmjs.org'
- name: Build frontend
run: |
cd frontend
npm install
npm run build
cd ..
- name: Upload frontend artifact
uses: actions/upload-artifact@v7
with:
name: frontend-dist
path: frontend/dist
retention-days: 1
build-windows:
needs: build-frontend
name: build-windows-${{ matrix.arch }}
strategy:
fail-fast: false
matrix:
include:
- { arch: amd64, runner: windows-latest, cgo: "1" }
- { arch: arm64, runner: ubuntu-latest, cgo: "0" }
runs-on: ${{ matrix.runner }}
steps:
- name: Checkout repository
uses: actions/checkout@v6.0.2
- name: Download frontend artifact
uses: actions/download-artifact@v8
with:
name: frontend-dist
path: web/html
- name: Setup Go
uses: actions/setup-go@v6
with:
cache: false
go-version-file: go.mod
- name: Install zip for Windows
if: matrix.arch == 'amd64'
shell: powershell
run: |
# Install Chocolatey if not available
if (!(Get-Command choco -ErrorAction SilentlyContinue)) {
Set-ExecutionPolicy Bypass -Scope Process -Force
[System.Net.ServicePointManager]::SecurityProtocol = [System.Net.ServicePointManager]::SecurityProtocol -bor 3072
iex ((New-Object System.Net.WebClient).DownloadString('https://community.chocolatey.org/install.ps1'))
}
# Install zip
choco install zip -y
- name: Build s-ui
shell: bash
run: |
export CGO_ENABLED=${{ matrix.cgo }}
export GOOS=windows
export GOARCH=${{ matrix.arch }}
echo "Building for Windows ${{ matrix.arch }}"
go version
go env GOOS GOARCH
. ./build-tags.sh
TAGS=$(tags_for windows)
LDFLAGS=$(ldflags_for windows)
go build -ldflags="$LDFLAGS" -tags "$TAGS" -o sui.exe main.go
file sui.exe
mkdir s-ui-windows
cp sui.exe s-ui-windows/
cp -r windows/* s-ui-windows/
- name: Download libcronet-go
shell: bash
run: |
curl -qsL -o s-ui-windows/libcronet.dll ${{ env.LIBCRONET_BASE_URL }}/libcronet-windows-${{ matrix.arch }}.dll
- name: Package
shell: bash
run: |
zip -r "s-ui-windows-${{ matrix.arch }}.zip" s-ui-windows
- name: Upload files to Artifacts
uses: actions/upload-artifact@v7
with:
name: s-ui-windows-${{ matrix.arch }}
path: ./s-ui-windows-${{ matrix.arch }}.zip
retention-days: 30
- name: Upload to Release
uses: svenstaro/upload-release-action@v2
if: |
(github.event_name == 'release' && github.event.action == 'published') ||
(github.event_name == 'push' && startsWith(github.ref, 'refs/tags/'))
with:
repo_token: ${{ secrets.GITHUB_TOKEN }}
tag: ${{ github.ref }}
file: s-ui-windows-${{ matrix.arch }}.zip
asset_name: s-ui-windows-${{ matrix.arch }}.zip
prerelease: true
overwrite: true

2
build-tags.sh
View file

@ -11,7 +11,7 @@
# test yes - go test; release.yml's non-naive targets
# dev yes musl build.sh
# release yes musl release.yml, naive targets only
# windows yes purego windows.yml
# windows yes purego release.yml, build-windows
# docker - purego Dockerfile, Dockerfile.frontend-artifact
#
# with_musl links a prebuilt musl libcronet.a and needs the Chromium toolchain

42
cmd/migration/main.go
View file

@ -12,13 +12,9 @@ import (
"gorm.io/gorm"
)
// MigrateDb brings an older database up to the current version.
//
// It returns an error rather than calling log.Fatal, which it used to do from
// six places. That mattered because ImportDB calls this in the middle of a
// restore: a migration failure killed the whole panel process with the live
// database already renamed away, and systemd restarted it straight into the
// same failure.
// MigrateDb brings an older database up to the current version. It returns an
// error rather than calling log.Fatal: ImportDB calls this mid-restore, with
// the live database already renamed away.
func MigrateDb() error {
// void running on first install
path := config.GetDBPath()
@ -47,9 +43,8 @@ func MigrateDb() error {
currentVersion := config.GetVersion()
dbVersion := ""
// The error was discarded here, so a missing or unreadable settings table
// was indistinguishable from an unset version -- and the empty string sends
// the whole legacy chain through again.
// An unreadable settings table must not look like an unset version, which
// would send the whole legacy chain through again.
if err := tx.Raw("SELECT value FROM settings WHERE key = ?", "version").Find(&dbVersion).Error; err != nil {
return fmt.Errorf("reading database version: %w", err)
}
@ -73,8 +68,7 @@ func MigrateDb() error {
dbVersion = "1.2"
}
// Before 1.3. Was `dbVersion[0:3] == "1.2"`, which panics on any stored
// value shorter than three characters.
// Before 1.3. Was dbVersion[0:3], which panics on a short value.
if major, minor := majorMinor(dbVersion); major == 1 && minor == 2 {
if err := to1_3(tx); err != nil {
return fmt.Errorf("migration to 1.3 failed: %w", err)
@ -107,12 +101,8 @@ func MigrateDb() error {
}
// setVersion records the schema version, inserting the row when it is absent.
//
// A plain UPDATE affects zero rows and reports no error when there is no
// version row -- and ResetSettings deletes every settings row, so after
// `s-ui setting -reset` the next migrate replayed the entire legacy chain
// against a current database, failed to record anything, and did it again on
// every subsequent run.
// A plain UPDATE affects zero rows and reports no error, so after a settings
// reset every migrate replayed the whole legacy chain and recorded nothing.
func setVersion(tx *gorm.DB, version string) error {
res := tx.Exec("UPDATE settings SET value = ? WHERE key = ?", version, "version")
if res.Error != nil {
@ -125,13 +115,9 @@ func setVersion(tx *gorm.DB, version string) error {
}
// compareVersions orders two dotted versions numerically, returning -1, 0 or 1.
//
// These were compared as strings, which is correct only while every component
// is a single digit: "1.10.0" < "1.5.1" is true lexicographically, so the
// release after 1.9 would have replayed to1_5_1 against every database and
// stripped the explicit CA from every TLS client config.
//
// A missing component counts as zero, so "1.2" and "1.2.0" compare equal.
// Not as strings: "1.10.0" < "1.5.1" lexicographically, so the release after
// 1.9 would replay to1_5_1 against every database. A missing component counts
// as zero, so "1.2" and "1.2.0" compare equal.
func compareVersions(a, b string) int {
av, bv := parseVersion(a), parseVersion(b)
for i := range av {
@ -150,9 +136,9 @@ func majorMinor(v string) (int, int) {
return parsed[0], parsed[1]
}
// parseVersion reads up to three numeric components. Anything it cannot parse
// stops the scan and leaves the rest at zero, so a garbage value sorts as the
// oldest possible version rather than panicking or comparing as text.
// parseVersion reads up to three numeric components. Anything unparseable
// stops the scan and leaves the rest zero, so garbage sorts as the oldest
// version rather than panicking.
func parseVersion(v string) [3]int {
var parsed [3]int
v = strings.TrimPrefix(strings.TrimSpace(v), "v")

16
cmd/migration/main_test.go
View file

@ -2,11 +2,8 @@ package migration
import "testing"
// Versions were compared as strings, which is correct only while every
// component stays a single digit. "1.10.0" < "1.5.1" is true lexicographically,
// so the first release after 1.9 would have replayed to1_5_1 against every
// database -- stripping the explicit CA from every TLS client config that had
// one.
// Compared as strings, "1.10.0" < "1.5.1", so the first release after 1.9 would
// have replayed to1_5_1 and stripped the CA from every TLS client config.
func TestCompareVersions(t *testing.T) {
testCases := []struct {
a, b string
@ -28,8 +25,7 @@ func TestCompareVersions(t *testing.T) {
{"1.2", "1.2.0", 0},
{"1", "1.0.0", 0},
// An unset version is the oldest thing there is, so every migration
// still runs against a database that has never recorded one.
// Unset is the oldest version, so every migration still runs.
{"", "1.0.0", -1},
{"", "", 0},
@ -49,8 +45,7 @@ func TestCompareVersions(t *testing.T) {
}
}
// The 1.3 step was gated on dbVersion[0:3] == "1.2", which panics outright on
// any stored value shorter than three characters.
// The 1.3 gate was dbVersion[0:3], which panics on a short stored value.
func TestMajorMinorHandlesShortAndEmptyVersions(t *testing.T) {
testCases := []struct {
version string
@ -72,8 +67,7 @@ func TestMajorMinorHandlesShortAndEmptyVersions(t *testing.T) {
}
}
// The 1.2-series gate must match the whole series and nothing outside it. A
// prefix comparison on "1.2" also matched "1.20".
// A prefix comparison on "1.2" also matched "1.20".
func TestOneTwoSeriesGate(t *testing.T) {
inSeries := func(v string) bool {
major, minor := majorMinor(v)

50
core/lifecycle_test.go
View file

@ -6,19 +6,13 @@ import (
"testing"
)
// A config with no inbounds: enough for a real Box to build and start, cheap
// enough to cycle repeatedly, and it binds no ports so the test is safe to run
// anywhere.
// No inbounds: a real Box builds and starts, binds no ports, cycles cheaply.
const emptyCoreConfig = `{"log":{"level":"error"}}`
// TestCoreConcurrentLifecycle drives the real publish and detach paths while
// readers hammer the accessors, which is the shape of the running panel: the
// five-second watchdog and the config-save handler start and stop the core
// while the dashboard poll reads IsRunning and GetInstance.
//
// Core used to be a bare struct with no mutex at all, so this tripped the race
// detector on both fields. Run with -race; without it the test only proves the
// accessors do not panic.
// The shape of the running panel: the watchdog and the save handler start and
// stop the core while the dashboard poll reads IsRunning and GetInstance. Core
// used to have no mutex, so this tripped -race on both fields. Without -race it
// only proves the accessors do not panic.
func TestCoreConcurrentLifecycle(t *testing.T) {
if raceEnabled {
t.Skip("starting a Box trips sing-box's own race in route.NetworkManager; see race_on_test.go")
@ -39,8 +33,7 @@ func TestCoreConcurrentLifecycle(t *testing.T) {
return
default:
}
// Whatever interleaving happens, a caller must never see
// isRunning true alongside a nil instance.
// Never isRunning true alongside a nil instance.
if box, err := c.running(); err == nil && box == nil {
t.Error("running() returned a nil box with no error")
return
@ -68,9 +61,8 @@ func TestCoreConcurrentLifecycle(t *testing.T) {
wg.Wait()
}
// TestCoreStartStopState checks the accessors agree with each other across a
// full cycle, and that a second Stop is harmless -- StopCore is reached from
// the maintenance toggle, an explicit restart and shutdown.
// The accessors must agree across a full cycle, and a second Stop must be
// harmless: three separate paths reach it.
func TestCoreStartStopState(t *testing.T) {
if raceEnabled {
t.Skip("starting a Box trips sing-box's own race in route.NetworkManager; see race_on_test.go")
@ -115,10 +107,9 @@ func TestCoreStartStopState(t *testing.T) {
}
}
// TestStartRejectsMalformedConfig locks down a silent failure: the unmarshal
// error was logged and then ignored, so the box was built from a zero-value
// option set. It started, reported itself healthy with no inbounds at all, and
// the watchdog never retried because IsRunning was true.
// The unmarshal error was logged and ignored, so the box was built from a
// zero-value option set: healthy, no inbounds, and IsRunning true so the
// watchdog never retried.
func TestStartRejectsMalformedConfig(t *testing.T) {
c := NewCore()
t.Cleanup(func() { _ = c.Stop() })
@ -138,14 +129,12 @@ func TestStartRejectsMalformedConfig(t *testing.T) {
}
}
// TestFailedStartLeavesNoInstance covers the other half of the same path: a
// config that parses but cannot start must not publish a half-built box.
// A config that parses but cannot start must not publish a half-built box.
func TestFailedStartLeavesNoInstance(t *testing.T) {
c := NewCore()
t.Cleanup(func() { _ = c.Stop() })
// An inbound on a port that cannot be bound: port 0 is fine, but an
// unparseable listen address fails while the box is being built.
// An unparseable listen address fails while the box is being built.
err := c.Start([]byte(`{"inbounds":[{"type":"mixed","tag":"in","listen":"not-an-address","listen_port":1080}]}`))
if err == nil {
t.Skip("this config built successfully; the failure path needs another trigger")
@ -161,17 +150,14 @@ func TestFailedStartLeavesNoInstance(t *testing.T) {
}
}
// TestCoreLockConcurrency exercises Core's own mutex without starting a Box, so
// it runs under -race where the box-starting tests above cannot. Readers race
// the detach path in Stop and the rejection path in Start; the invariant is the
// one the old lock-free struct broke -- no caller may ever observe isRunning as
// true together with a nil instance.
// Core's own mutex without starting a Box, so this runs under -race where the
// tests above cannot. The invariant is the one the lock-free struct broke: no
// caller may observe isRunning true together with a nil instance.
func TestCoreLockConcurrency(t *testing.T) {
c := NewCore()
// Two groups: the writers finish on their own, and only once they have can
// the readers be told to stop. Waiting on a single group would deadlock,
// since the readers loop until the channel closes.
// Two groups: the readers loop until the channel closes, so they can only
// be told to stop once the writers have finished. One group deadlocks.
var readers, writers sync.WaitGroup
stop := make(chan struct{})

45
core/main.go
View file

@ -13,29 +13,21 @@ import (
_ "github.com/sagernet/sing-box/transport/v2rayquic"
)
// Core owns the running sing-box instance.
// Core owns the running sing-box instance. Everything mutable lives behind mu,
// and the managers are read off the Box rather than cached in package vars, so
// a caller cannot mix managers from one box with an instance from another.
//
// Everything mutable lives behind mu. The managers this used to keep in
// package-level vars (inbound_manager, router, factory, ...) were copies of
// fields the Box already holds, written by Start and read by the endpoint
// methods without synchronisation; they are now read off the Box itself, so a
// caller cannot mix managers from one box with an instance from another.
// RWMutex because the panel's status poll calls IsRunning on every request and
// must not queue behind a start.
//
// mu is an RWMutex because IsRunning is called by the panel's status poll on
// every request, and must not queue behind a start.
//
// Lock ordering: service.startCoreMu (outer) is always taken before Core.mu
// (inner). Nothing in this package calls into package service, so that order
// holds by construction.
// Lock ordering: service.lifecycleMu (outer) before Core.mu (inner).
type Core struct {
mu sync.RWMutex
isRunning bool
instance *Box
// ctx carries the protocol registries. It is built once in NewCore and
// never reassigned, so it needs no lock. Start used to wrap it on every
// run (globalCtx = service.ContextWith(globalCtx, c)) -- nothing ever read
// that value back, and it added a context layer per restart.
// Carries the protocol registries. Built once in NewCore and never
// reassigned, so it needs no lock.
ctx context.Context
}
@ -62,9 +54,7 @@ func (c *Core) IsRunning() bool {
}
// running returns the live box, or an error when the core is stopped. One
// RLock gives the caller a consistent view: it cannot observe isRunning as true
// and then find instance already nil, which is what made the old
// check-then-use pattern panic when a stop landed in between.
// RLock, so a caller cannot see isRunning true and then find instance nil.
func (c *Core) running() (*Box, error) {
c.mu.RLock()
defer c.mu.RUnlock()
@ -74,20 +64,13 @@ func (c *Core) running() (*Box, error) {
return c.instance, nil
}
// Start builds a box from the config and publishes it.
//
// The build and the start happen outside the lock on purpose: NewBox
// constructs every inbound, outbound and endpoint, and Start binds their
// listeners, which takes seconds. Holding mu across that would block every
// status poll for the duration. Only the publish is locked, so a half-built
// box is never visible -- a failed start is closed and discarded without ever
// being assigned.
// Start builds a box from the config and publishes it. The build and the start
// run outside the lock -- they bind listeners and take seconds -- and only the
// publish is locked, so a half-built box is never visible.
func (c *Core) Start(sbConfig []byte) error {
var opt option.Options
// A malformed config used to be logged and then used anyway, which left
// an empty option set: the box started with zero inbounds and reported
// itself healthy, so the watchdog never retried and no client could
// connect.
// Returned, not just logged: an empty option set starts a box with zero
// inbounds that reports itself healthy, so the watchdog never retries.
if err := opt.UnmarshalJSONContext(c.ctx, sbConfig); err != nil {
return common.NewErrorf("unmarshal config: %v", err)
}

12
cronjob/cronJob.go
View file

@ -24,11 +24,10 @@ func NewCronJob() *CronJob {
}
func (c *CronJob) Start(loc *time.Location, trafficAge int, statsBucketSeconds int64, globalReset string) error {
// Recover: robfig/cron does not recover panics by default, so a nil deref
// in any job took the whole panel process down -- gin only covers the HTTP
// side. SkipIfStillRunning: the stats job fires every 10s and can block
// that long on the SQLite write lock, and overlapping runs each drain the
// core's traffic counters.
// Recover: robfig/cron does not recover panics by default, and gin only
// covers the HTTP side. SkipIfStillRunning: the stats job fires every 10s,
// can block that long on the write lock, and overlapping runs each drain
// the core's traffic counters.
c.cron = cron.New(
cron.WithLocation(loc),
cron.WithParser(cronParser),
@ -38,8 +37,7 @@ func (c *CronJob) Start(loc *time.Location, trafficAge int, statsBucketSeconds i
),
)
// Registered before Start, not from a goroutine racing it, so a job cannot
// be missed on a panel that is stopped moments after boot.
// Registered before Start, not from a goroutine racing it.
addJob := func(spec string, job cron.Job, name string) {
if _, err := c.cron.AddJob(spec, job); err != nil {
logger.Warning("unable to schedule ", name, " <", spec, ">: ", err)

20
cronjob/resetTrafficJob.go
View file

@ -33,12 +33,9 @@ func (s *ResetTrafficJob) Run() {
logger.Warning("ResetTrafficJob: get last reset time failed: ", err)
return
}
// Configured start date / next boundary not reached yet.
//
// Logged, because this used to be a silent return: the setting holds the
// *next* boundary despite its name, and it is not cleared when the operator
// changes the schedule. Someone switching a monthly reset to a daily one
// would then see nothing happen for up to a month, with no clue why.
// Logged, not a silent return: the setting holds the *next* boundary
// despite its name and is not cleared when the schedule changes, so a
// monthly-to-daily switch can look dead for up to a month.
if last > now.Unix() {
logger.Debug("ResetTrafficJob: next reset is at ", time.Unix(last, 0).In(loc).Format(time.RFC3339), ", nothing to do")
return
@ -49,13 +46,10 @@ func (s *ResetTrafficJob) Run() {
return
}
// Restart before the bookkeeping write, not after.
//
// Clients were just re-enabled in the database, but the running core still
// holds the old user list. When the write below failed, the old code
// returned early and never restarted, so those clients stayed disconnected
// -- and the watchdog does not help, because it only starts a core that is
// not running.
// Restart before the bookkeeping write: clients are re-enabled in the
// database but the core still holds the old user list, and a failed write
// used to return early and leave them disconnected. The watchdog does not
// help -- it only starts a core that is not running.
if err = s.ConfigService.RestartCore(); err != nil {
logger.Error("ResetTrafficJob: unable to restart core: ", err)
}

59
database/backup.go
View file

@ -31,13 +31,8 @@ func GetDb(exclude string) ([]byte, error) {
}
}
// os.CreateTemp, not a hand-built name. The old path was
// `dir + config.GetName() + time.Now().Format("20060102-200203")`, which had
// two defects: no separator, so the file landed in the binary's *parent*
// directory; and "200203" is a typo for the reference clock "150405", so
// every backup taken in the same period rendered the same name. Two
// downloads on one day opened the same file and deleted it from under each
// other.
// CreateTemp, not a hand-built timestamp: two downloads in the same period
// used to render the same name and delete the file from under each other.
dir, err := filepath.Abs(filepath.Dir(os.Args[0]))
if err != nil {
return nil, err
@ -47,8 +42,7 @@ func GetDb(exclude string) ([]byte, error) {
return nil, err
}
dbPath := tmp.Name()
// SQLite opens the path itself; this handle is only here to reserve a
// unique name.
// SQLite opens the path itself; this handle only reserves the name.
tmp.Close()
defer os.Remove(dbPath)
@ -68,8 +62,7 @@ func GetDb(exclude string) ([]byte, error) {
}
defer closeBackup()
// Same list InitDB migrates, so a table can never be created in the live
// database but missing from the backup.
// Same list InitDB migrates, so no table can be live but unbacked-up.
if err = backupDb.AutoMigrate(schemaModels()...); err != nil {
return nil, err
}
@ -83,8 +76,7 @@ func GetDb(exclude string) ([]byte, error) {
}
}
// Fold the WAL back into the main file, otherwise the bytes read below are
// missing everything still sitting in the sidecar.
// Fold the WAL in, or the bytes read below miss whatever is still in it.
if err = backupDb.Exec("PRAGMA wal_checkpoint(TRUNCATE);").Error; err != nil {
return nil, err
}
@ -114,12 +106,9 @@ func ImportDB(file multipart.File) error {
return common.NewErrorf("Error removing existing temporary db file: %v", err)
}
// Everything below up to the rename works on the upload alone. The live
// connection pool stays open throughout: it used to be closed here, before
// the copy and the validation, so any failure after that point -- a
// truncated upload, a full disk -- left every later query in the process
// failing with "sql: database is closed" until someone restarted the
// service by hand.
// Everything up to the rename works on the upload alone, and the live pool
// stays open: closing it here means a failed import breaks every later
// query with "sql: database is closed" until someone restarts the service.
tempFile, err := os.Create(tempPath)
if err != nil {
return common.NewErrorf("Error creating temporary db file: %v", err)
@ -132,26 +121,22 @@ func ImportDB(file multipart.File) error {
}
defer os.Remove(tempPath)
// Open it and check it is actually an s-ui database, not merely some
// SQLite file. The header check above passes for any SQLite file at all --
// a browser profile, another panel's database -- and importing one of those
// used to replace the live database and then crash the process on the
// migration, leaving systemd to restart it straight into the same crash.
// The header check above passes for any SQLite file at all, including a
// browser profile. Importing one of those replaces the live database and
// then crashes the migration, into a systemd restart loop.
if err = validateImport(tempPath); err != nil {
return err
}
// Close the live pool and fold its WAL back in. Renaming the database out
// from under a -wal/-shm pair leaves those sidecars beside the imported
// file, and SQLite then runs recovery for a different database against it.
// Renaming out from under a -wal/-shm pair leaves those sidecars beside the
// imported file, and SQLite recovers the wrong database's pages into it.
if sqlDB, e := db.DB(); e == nil {
_ = db.Exec("PRAGMA wal_checkpoint(TRUNCATE);").Error
_ = sqlDB.Close()
}
removeSidecars(dbPath)
// Keep the pre-import database, timestamped, so a bad import is
// recoverable. The old code deleted it on the success path.
// Keep the pre-import database, timestamped, so a bad import is recoverable.
fallbackPath := fmt.Sprintf("%s.backup-%s", dbPath, time.Now().Format("20060102-150405"))
if err = os.Rename(dbPath, fallbackPath); err != nil {
reopen(dbPath)
@ -198,9 +183,8 @@ func validateImport(path string) error {
}
}()
// These three carry the panel's identity: settings holds the schema
// version and the session secret, and an import without clients and
// inbounds is not a panel backup whatever else it contains.
// settings carries the schema version and session secret; without clients
// and inbounds it is not a panel backup whatever else it holds.
for _, required := range []string{"settings", "clients", "inbounds"} {
if !candidate.Migrator().HasTable(required) {
return common.NewErrorf("Not an s-ui database: table %q is missing", required)
@ -209,9 +193,8 @@ func validateImport(path string) error {
return nil
}
// removeSidecars drops the -wal and -shm files belonging to path. They describe
// the database being replaced, and leaving them next to a different one invites
// SQLite to recover pages into it.
// removeSidecars drops the -wal and -shm files belonging to path; they describe
// the database being replaced, not the one taking its place.
func removeSidecars(path string) {
for _, suffix := range []string{"-wal", "-shm"} {
if err := os.Remove(path + suffix); err != nil && !os.IsNotExist(err) {
@ -220,8 +203,7 @@ func removeSidecars(path string) {
}
}
// reopen restores the connection pool after an import gave up, so the panel
// keeps serving instead of failing every query until it is restarted.
// reopen restores the connection pool after an import gave up.
func reopen(path string) {
if err := InitDB(path); err != nil {
logger.Error("unable to reopen the database after a failed import: ", err)
@ -240,8 +222,7 @@ func restoreFallback(dbPath, fallbackPath string) {
func IsSQLiteDB(file io.Reader) (bool, error) {
signature := []byte("SQLite format 3\x00")
buf := make([]byte, len(signature))
// ReadFull, because a single Read may return fewer bytes than asked for and
// would then compare a partly-filled buffer.
// ReadFull: a single Read may return short and compare a partial buffer.
if _, err := io.ReadFull(file, buf); err != nil {
return false, err
}

23
database/backup_test.go
View file

@ -14,9 +14,8 @@ import (
gormlogger "gorm.io/gorm/logger"
)
// seedEveryTable puts at least one row in every table the schema describes, so
// a backup that drops a table is visible as a missing row rather than as an
// empty database nobody notices until a restore.
// A row in every table the schema describes, so a dropped table shows up as a
// missing row rather than as an empty database nobody notices until a restore.
func seedEveryTable(t *testing.T) {
t.Helper()
@ -56,10 +55,8 @@ func openBackup(t *testing.T, contents []byte) *gorm.DB {
return restored
}
// TestBackupCarriesEveryTable is the test that would have caught the schema
// drift: InitDB migrated eleven models while GetDb backed up nine, so services
// and API tokens were silently absent from every backup. Restoring one then
// AutoMigrated an empty services table over the top.
// The schema drift: InitDB migrated eleven models while GetDb backed up nine,
// so services and API tokens were absent from every backup.
func TestBackupCarriesEveryTable(t *testing.T) {
if err := InitDB(filepath.Join(t.TempDir(), "test.db")); err != nil {
t.Fatal(err)
@ -90,8 +87,7 @@ func TestBackupCarriesEveryTable(t *testing.T) {
}
}
// The exclude parameter is what the UI offers for trimming a large backup. It
// must skip exactly the named tables and nothing else.
// exclude must skip exactly the named tables and nothing else.
func TestBackupExcludesOnlyWhatWasAsked(t *testing.T) {
if err := InitDB(filepath.Join(t.TempDir(), "test.db")); err != nil {
t.Fatal(err)
@ -128,10 +124,8 @@ func TestBackupExcludesOnlyWhatWasAsked(t *testing.T) {
}
}
// Two backups taken close together must not collide. The temp path used to be
// built with the layout "20060102-200203" -- a typo for "150405" -- which
// rendered the same string for every call in a period, so concurrent downloads
// shared one file and deleted it from under each other.
// The temp path used the layout "20060102-200203", a typo for "150405", so
// every call in a period rendered the same name and two downloads collided.
func TestBackupsDoNotShareATempFile(t *testing.T) {
if err := InitDB(filepath.Join(t.TempDir(), "test.db")); err != nil {
t.Fatal(err)
@ -150,8 +144,7 @@ func TestBackupsDoNotShareATempFile(t *testing.T) {
t.Fatalf("a backup came back empty: %d and %d bytes", len(first), len(second))
}
// Both must be openable databases carrying the seeded rows; a shared temp
// file would leave one of them truncated or gone.
// A shared temp file leaves one of them truncated or gone.
for i, contents := range [][]byte{first, second} {
restored := openBackup(t, contents)
var count int64

17
database/db.go
View file

@ -36,11 +36,9 @@ func initUser() error {
func OpenDB(dbPath string) error {
dir := path.Dir(dbPath)
// 0700, not the old 01740. That literal's 01000 bit is not Go's sticky bit
// (os.ModeSticky is 1<<24), so it was silently dropped and the directory
// came out 0740; the intent was clearly owner-only. MkdirAll also does
// nothing when the directory already exists, so an install that once had a
// looser mode kept it -- hence the explicit Chmod below.
// 0700, not the old 01740: that 01000 bit is not Go's sticky bit
// (os.ModeSticky is 1<<24) and was dropped, leaving 0740. MkdirAll is a
// no-op on an existing directory, hence the explicit Chmod.
if err := os.MkdirAll(dir, 0o700); err != nil {
return err
}
@ -86,12 +84,9 @@ func OpenDB(dbPath string) error {
sqlDB.SetConnMaxLifetime(time.Hour)
sqlDB.SetConnMaxIdleTime(5 * time.Minute)
// SQLite creates the database 0666 & ~umask, so it lands world-readable on
// a default umask. It holds every client's credentials, the API tokens in
// plaintext and the session secret. The owner-only directory above already
// blocks other users from reaching it, but the file should not depend on
// that alone -- a copied or moved database would carry the loose mode with
// it. The sidecars hold the same pages.
// SQLite creates the file 0666 & ~umask. The owner-only directory already
// shields it, but a copied or moved database carries its own mode. The
// sidecars hold the same pages.
for _, p := range []string{dbPath, dbPath + "-wal", dbPath + "-shm"} {
if err := os.Chmod(p, 0o600); err != nil && !os.IsNotExist(err) {
return err

16
database/model/model.go
View file

@ -25,12 +25,11 @@ type User struct {
type Client struct {
Id uint `json:"id" form:"id" gorm:"primaryKey;autoIncrement"`
Enable bool `json:"enable" form:"enable"`
// Indexed because Name is the join key on every hot path: the stats job
// looks up each online user by it every ten seconds, and every subscription
// fetch resolves a client by it. Uniqueness is still enforced in
// ClientService.validateClientName rather than by the index -- a unique
// index needs existing duplicates renamed first, and a client's name is its
// subscription ID, so renaming one silently breaks that user's link.
// Name is the join key on every hot path: the stats job and every
// subscription fetch resolve a client by it. Uniqueness stays in
// ClientService.validateClientName -- a unique index would need existing
// duplicates renamed, and a name is the subscription ID, so renaming one
// breaks that user's link.
Name string `json:"name" form:"name" gorm:"index"`
Config json.RawMessage `json:"config,omitempty" form:"config"`
Inbounds json.RawMessage `json:"inbounds" form:"inbounds"`
@ -58,9 +57,8 @@ type Client struct {
type Stats struct {
Id uint64 `json:"id" gorm:"primaryKey;autoIncrement"`
// Second index on date_time alone: it sits third in idx_stats_bucket, so
// that index cannot serve the retention purge, which filters on date_time
// by itself.
// date_time sits third in idx_stats_bucket, so that index cannot serve the
// retention purge, which filters on date_time alone.
DateTime int64 `json:"dateTime" gorm:"uniqueIndex:idx_stats_bucket,priority:3;index:idx_stats_date_time"`
Resource string `json:"resource" gorm:"uniqueIndex:idx_stats_bucket,priority:1"`
Tag string `json:"tag" gorm:"uniqueIndex:idx_stats_bucket,priority:2"`

14
database/schema.go
View file

@ -6,14 +6,9 @@ import (
"gorm.io/gorm"
)
// table ties a model to the code that copies its rows, so the schema is
// described in exactly one place.
//
// It exists because the list was previously written out twice -- once in
// InitDB's AutoMigrate and once in GetDb's -- and the two drifted: Service and
// Tokens were migrated but never backed up. A restore then AutoMigrated an
// empty services table over the top, and every configured service and every API
// token was gone with no warning.
// table ties a model to the code that copies its rows. The list used to be
// written out twice, in InitDB and in GetDb, and drifted: Service and Tokens
// were migrated but never backed up.
type table struct {
// name is what the backup endpoint's exclude parameter matches on.
name string
@ -22,8 +17,7 @@ type table struct {
copyRows func(src, dst *gorm.DB) error
}
// schema is the whole database. Adding a model here migrates it and backs it up;
// there is no second list to remember.
// schema is the whole database: adding a model here migrates it and backs it up.
func schema() []table {
return []table{
{"settings", &model.Setting{}, copyRows[model.Setting]},

18
logger/logger_test.go
View file

@ -8,11 +8,9 @@ import (
"github.com/op/go-logging"
)
// TestBufferConcurrentAccess reproduces the shape of the panel at runtime: cron
// jobs, HTTP handlers and sing-box connection goroutines all log while the
// /logs endpoint reads the buffer. Before addToBuffer and GetLogs shared a
// mutex this tripped the race detector on both the append and the re-slice.
// Run with -race; without it the test only proves nothing panics.
// Cron jobs, HTTP handlers and sing-box goroutines all log while /logs reads
// the buffer. Before addToBuffer and GetLogs shared a mutex this tripped -race
// on both the append and the re-slice; without -race it only proves no panic.
func TestBufferConcurrentAccess(t *testing.T) {
InitLogger(logging.ERROR)
resetBuffer()
@ -44,9 +42,7 @@ func TestBufferConcurrentAccess(t *testing.T) {
}
}
// TestGetLogsRespectsCount locks down an off-by-one: the loop condition was
// `len(output) <= c`, which was still true once c entries had been collected,
// so each call returned c+1 lines.
// The loop condition was `len(output) <= c`, so each call returned c+1 lines.
func TestGetLogsRespectsCount(t *testing.T) {
InitLogger(logging.ERROR)
resetBuffer()
@ -62,8 +58,7 @@ func TestGetLogsRespectsCount(t *testing.T) {
}
}
// TestGetLogsFiltersByLevel checks the level filter still excludes finer levels
// than the one requested, so the mutex change did not alter what is returned.
// The mutex change must not alter which levels are returned.
func TestGetLogsFiltersByLevel(t *testing.T) {
InitLogger(logging.ERROR)
resetBuffer()
@ -84,8 +79,7 @@ func TestGetLogsFiltersByLevel(t *testing.T) {
}
}
// TestBufferIsBounded covers the ring-buffer trim, which is the operation the
// concurrent append used to race against.
// The ring-buffer trim, which the concurrent append used to race against.
func TestBufferIsBounded(t *testing.T) {
if testing.Short() {
t.Skip("fills the 10240-entry buffer")

27
service/client.go
View file

@ -231,13 +231,9 @@ func (s *ClientService) Save(tx *gorm.DB, act string, data json.RawMessage, host
return inboundIds, nil
}
// preserveServerOwnedFields restores the columns the panel maintains itself,
// so an edit cannot write them back from whatever the form was rendered with.
//
// The traffic counters matter as much as the timestamps: the stats job adds to
// up/down every ten seconds, so an admin who opened the client editor and saved
// a minute later used to roll them back to the values the form was built from,
// silently discarding that minute of usage against the client's quota.
// preserveServerOwnedFields restores the columns the panel maintains itself.
// The traffic counters matter as much as the timestamps: the stats job writes
// up/down every ten seconds, so a stale form would roll them back.
func (s *ClientService) preserveServerOwnedFields(tx *gorm.DB, client *model.Client) {
var existing model.Client
if err := tx.Model(model.Client{}).
@ -253,12 +249,9 @@ func (s *ClientService) preserveServerOwnedFields(tx *gorm.DB, client *model.Cli
client.TotalDown = existing.TotalDown
}
// clientNameJSON encodes a client name for the changes log.
//
// These were built as "\"" + name + "\"", so a name containing a quote or a
// backslash produced invalid JSON that every reader of the log then failed on.
// cmd/migration/1_1.go already carries a repair migration for the previous
// generation of this same bug.
// clientNameJSON encodes a client name for the changes log. Built by string
// concatenation, a name with a quote or backslash produced unreadable JSON --
// cmd/migration/1_1.go already repairs the previous generation of this bug.
func clientNameJSON(name string) json.RawMessage {
encoded, err := json.Marshal(name)
if err != nil {
@ -583,12 +576,8 @@ func (s *ClientService) ResetClients(tx *gorm.DB, dt int64) ([]uint, error) {
}
allClients = append(allClients, resetClients...)
// Set periodic reset.
//
// reset_days > 0 is a backstop: with it zero, NextReset below is set to
// dt + 0 == dt, so the row matches again on the very next run of this job
// and the client's traffic is folded away and zeroed every minute. Its
// volume quota could then never be reached.
// reset_days > 0 is a backstop: at zero, NextReset becomes dt + 0 == dt, so
// the row matches every minute and the volume quota is never reached.
err = tx.Model(model.Client{}).
Where("delay_start = false AND auto_reset = true AND reset_days > 0 AND next_reset < ?", dt).Find(&resetClients).Error
if err != nil {

21
service/client_reset_test.go
View file

@ -45,11 +45,8 @@ func reload(t *testing.T, db *gorm.DB, id uint) model.Client {
return got
}
// A client saved with auto_reset on and reset_days zero matched the periodic
// reset query, then had NextReset set to dt + 0 == dt -- so it matched again on
// the very next run of the deplete job, every minute, forever. Its traffic was
// folded into the totals and zeroed each time, which meant up+down could never
// exceed its volume and a quota-limited account became unlimited.
// With reset_days zero, NextReset became dt + 0 == dt, so the row matched again
// every minute and a quota-limited account became effectively unlimited.
func TestPeriodicResetSkipsZeroResetDays(t *testing.T) {
db := clientTestDB(t)
s := &ClientService{}
@ -78,8 +75,7 @@ func TestPeriodicResetSkipsZeroResetDays(t *testing.T) {
}
}
// The normal path still has to work: a due client with a sane period is reset
// and its next boundary moved forward by exactly that many days.
// The normal path: a due client is reset and its boundary moved forward.
func TestPeriodicResetRollsTrafficForward(t *testing.T) {
db := clientTestDB(t)
s := &ClientService{}
@ -137,10 +133,8 @@ func TestPeriodicResetLeavesFutureBoundariesAlone(t *testing.T) {
}
}
// preserveServerOwnedFields has to restore the traffic counters as well as the
// timestamps. The stats job adds to up/down every ten seconds, so an admin who
// opened the client editor and saved a minute later used to write the stale
// values back and silently discard that minute of usage.
// The traffic counters have to be restored as well as the timestamps, or an
// editor left open for a minute writes stale values back over that usage.
func TestEditDoesNotRollBackTraffic(t *testing.T) {
db := clientTestDB(t)
s := &ClientService{}
@ -174,9 +168,8 @@ func TestEditDoesNotRollBackTraffic(t *testing.T) {
}
}
// The changes log stores the client name as JSON. It was built by string
// concatenation, so a name containing a quote or a backslash produced a row no
// reader could parse.
// Built by string concatenation, a name with a quote or backslash produced a
// changes-log row no reader could parse.
func TestClientNameJSONIsValid(t *testing.T) {
for _, name := range []string{
`plain`,

77
service/config.go
View file

@ -17,18 +17,11 @@ var (
LastUpdate int64
corePtr *core.Core
// lifecycleMu serialises whole start/stop/restart/maintenance sequences,
// not just a flag. It replaces the old startCoreInProgress bool, which was
// released before corePtr.Start ran: a maintenance toggle could land while
// a start was still in flight, see IsRunning() as false (it is only set
// once Start returns), stop nothing, and leave the core serving clients
// with the panel reporting maintenance.
//
// Ordering: this is the outer lock; core.Core.mu is the inner one.
// Serialises whole start/stop/restart/maintenance sequences, not just a
// flag: corePtr.IsRunning() is false for the seconds a start takes.
// Outer lock; core.Core.mu is the inner one.
lifecycleMu sync.Mutex
// failMu guards lastStartFailTime alone, which is read by the cooldown
// check and written after a failed start.
failMu sync.Mutex
lastStartFailTime time.Time
startCooldown = 15 * time.Second
@ -228,11 +221,9 @@ func (s *ConfigService) inMaintenance() bool {
return maintenance
}
// StartCore starts the core if it is not already running.
//
// It does not queue: the five-second watchdog calls this, and a start can take
// seconds, so a caller that finds a sequence already in flight returns instead
// of piling up behind it.
// StartCore starts the core if it is not already running. It does not queue:
// the five-second watchdog calls this, so a caller that finds a sequence in
// flight returns rather than piling up behind it.
func (s *ConfigService) StartCore() error {
if !lifecycleMu.TryLock() {
return nil
@ -242,16 +233,11 @@ func (s *ConfigService) StartCore() error {
}
// startCoreLocked starts the core. The caller must hold lifecycleMu.
//
// bypassCooldown is set for actions the operator asked for directly (an
// explicit restart, leaving maintenance). The cooldown exists to stop the
// watchdog hammering a config that fails to load; it should not make a button
// press silently do nothing.
// bypassCooldown is set for operator actions, so a recent failure cannot make
// a button press silently do nothing.
func (s *ConfigService) startCoreLocked(bypassCooldown bool) error {
// Re-checked here, under the lock, and not only at the entry to the outer
// call: maintenance can be switched on while this goroutine was waiting,
// and starting afterwards would put every client back online behind the
// operator's back.
// Re-checked under the lock: maintenance can be switched on while this
// goroutine was waiting.
if s.inMaintenance() {
return nil
}
@ -275,8 +261,7 @@ func (s *ConfigService) startCoreLocked(bypassCooldown bool) error {
logger.Error("start sing-box err:", err.Error())
return err
}
// Cleared on success, so one failure does not keep the cooldown armed for
// the rest of the process lifetime.
// Cleared on success, or one failure arms the cooldown forever.
failMu.Lock()
lastStartFailTime = time.Time{}
failMu.Unlock()
@ -285,20 +270,16 @@ func (s *ConfigService) startCoreLocked(bypassCooldown bool) error {
}
// coolingDown reports whether a start failed recently enough that the watchdog
// should hold off. Operator-initiated actions pass bypassCooldown and skip it,
// so a recent failure cannot make a button press silently do nothing.
// should hold off.
func coolingDown() bool {
failMu.Lock()
defer failMu.Unlock()
return time.Since(lastStartFailTime) < startCooldown
}
// RestartCore stops and starts the core as one sequence.
//
// It blocks for the whole thing rather than bailing out like StartCore: this is
// only reached from an explicit operator action, and the old version could stop
// the core, find a start already in progress, return nil, and report success
// with the core left down.
// RestartCore stops and starts the core as one sequence. It blocks rather than
// bailing out like StartCore, so an operator action cannot report success with
// the core left down.
func (s *ConfigService) RestartCore() error {
lifecycleMu.Lock()
defer lifecycleMu.Unlock()
@ -343,14 +324,10 @@ func (s *ConfigService) restartCoreWithConfig(config json.RawMessage) error {
return nil
}
// SetMaintenance takes the core out of service, or puts it back.
//
// The setting is written first, so once the lock is released the watchdog
// reads the new value and will not restart what was just stopped. Taking
// lifecycleMu blocks until any in-flight start has finished, which is what
// makes turning maintenance on during a start reliable: it then sees the core
// as running and stops it, instead of finding IsRunning() false and doing
// nothing.
// SetMaintenance takes the core out of service, or puts it back. The setting is
// written first so the watchdog will not restart what was just stopped, and the
// blocking lock means an in-flight start has finished before we look at
// IsRunning().
func (s *ConfigService) SetMaintenance(enabled bool) error {
lifecycleMu.Lock()
defer lifecycleMu.Unlock()
@ -397,18 +374,15 @@ func (s *ConfigService) CheckOutbound(tag string, link string) core.CheckOutboun
func (s *ConfigService) Save(obj string, act string, data json.RawMessage, initUsers string, loginUser string, hostname string) ([]string, error) {
var err error
var objs []string = []string{obj}
// Set when the config object changed. The restart is deferred until after
// the commit: it used to be spawned inside the switch, so a failure on the
// changes-log insert below rolled the write back while the core was
// already running a config that was never persisted.
// Set when the config object changed. The restart waits for the commit, or
// a later rollback leaves the core running a config that was never saved.
var restartWith json.RawMessage
db := database.GetDB()
tx := db.Begin()
defer func() {
// A panic leaves err nil, so the old defer took the err == nil branch
// and committed a half-applied transaction -- while gin recovered the
// request and told the operator the save had failed.
// A panic leaves err nil, which would otherwise commit a half-applied
// transaction while gin tells the operator the save failed.
if r := recover(); r != nil {
tx.Rollback()
panic(r)
@ -422,9 +396,8 @@ func (s *ConfigService) Save(obj string, act string, data json.RawMessage, initU
return
}
if restartWith != nil {
// Detached because this runs in an HTTP handler and a core restart
// takes seconds. The recover is not optional: a panic in a bare
// goroutine is outside gin's reach and would kill the process.
// Detached: a restart takes seconds and this is an HTTP handler.
// The recover is required -- a panic here is outside gin's reach.
go func() {
defer func() {
if r := recover(); r != nil {

52
service/config_lifecycle_test.go
View file

@ -9,9 +9,8 @@ import (
"github.com/alireza0/s-ui/database"
)
// lifecycleService gives the test a database and a real but unstarted Core, so
// the lifecycle functions can be called for their locking and guard behaviour
// without ever bringing a sing-box instance up.
// A database and a real but unstarted Core, so the lifecycle functions can be
// exercised for locking and guard behaviour without starting sing-box.
func lifecycleService(t *testing.T) *ConfigService {
t.Helper()
if err := database.InitDB(filepath.Join(t.TempDir(), "test.db")); err != nil {
@ -23,8 +22,7 @@ func lifecycleService(t *testing.T) *ConfigService {
return &ConfigService{}
}
// mustFinish fails the test rather than hanging the suite when a lifecycle call
// deadlocks, which is the failure mode a mistake in the locking would produce.
// Fails rather than hanging the suite, which is what a locking mistake does.
func mustFinish(t *testing.T, what string, fn func()) {
t.Helper()
done := make(chan struct{})
@ -39,10 +37,8 @@ func mustFinish(t *testing.T, what string, fn func()) {
}
}
// StopCore, RestartCore and SetMaintenance all take the lifecycle lock, and all
// three need to stop the core. If any of them called the public StopCore
// instead of the locked variant it would deadlock on itself, so this walks the
// three entry points that reach a stop.
// All three entry points that reach a stop take the lifecycle lock; one calling
// the public StopCore instead of the locked variant would deadlock on itself.
func TestLifecycleEntryPointsDoNotSelfDeadlock(t *testing.T) {
s := lifecycleService(t)
@ -52,9 +48,7 @@ func TestLifecycleEntryPointsDoNotSelfDeadlock(t *testing.T) {
}
})
// With maintenance on, RestartCore refuses before touching the core, and
// SetMaintenance(true) finds it already stopped. Both still take and
// release the lock, which is what is under test.
// Both still take and release the lock, which is what is under test.
if err := s.SettingService.SetMaintenance(true); err != nil {
t.Fatal(err)
}
@ -75,14 +69,9 @@ func TestLifecycleEntryPointsDoNotSelfDeadlock(t *testing.T) {
})
}
// The bug this locks down: SetMaintenance used to run while a start was still
// in flight. It read IsRunning(), which is only set once Start returns, saw
// false, stopped nothing and reported success -- leaving the panel showing
// maintenance while the core came up and served clients anyway.
//
// The fix is that a maintenance toggle takes the same lock a start holds for
// its whole sequence, so it cannot observe that half-started state. Holding the
// lock here stands in for the in-flight start.
// SetMaintenance used to run while a start was in flight, read IsRunning() as
// false, stop nothing and report success -- panel in maintenance, core serving
// clients. Holding the lock here stands in for that in-flight start.
func TestSetMaintenanceWaitsForAnInFlightStart(t *testing.T) {
s := lifecycleService(t)
@ -119,9 +108,8 @@ func TestSetMaintenanceWaitsForAnInFlightStart(t *testing.T) {
}
}
// StartCore is called by the five-second watchdog. It must not queue up behind
// a sequence that is already running, or a slow start would leave watchdog
// ticks stacked up waiting to run against a core that is already up.
// The five-second watchdog calls StartCore; it must not queue behind a sequence
// already running, or ticks stack up against a core that is already up.
func TestStartCoreSkipsWhenBusy(t *testing.T) {
s := lifecycleService(t)
@ -141,15 +129,10 @@ func TestStartCoreSkipsWhenBusy(t *testing.T) {
}
}
// The cooldown stops the five-second watchdog hammering a config that will not
// load. These cover the watchdog side, which is what the cooldown exists for.
//
// The other half -- that RestartCore and leaving maintenance pass
// bypassCooldown, so a recent failure cannot make a button press silently do
// nothing -- is deliberately not tested here: both paths go on to build and
// start a real sing-box instance, which binds ports and trips sing-box's own
// race in route.NetworkManager. That behaviour belongs in an integration test,
// not this package.
// The cooldown stops the watchdog hammering a config that will not load. Only
// the watchdog side is covered: the bypassCooldown paths go on to start a real
// instance, which binds ports and trips sing-box's own route.NetworkManager
// race, so they belong in an integration test.
func TestCooldownHoldsOffTheWatchdog(t *testing.T) {
s := lifecycleService(t)
@ -169,9 +152,8 @@ func TestCooldownHoldsOffTheWatchdog(t *testing.T) {
}
}
// The cooldown was never cleared on a successful start, so a single early
// failure kept it armed and every later watchdog tick was still comparing
// against that old timestamp.
// The cooldown was never cleared on success, so one early failure kept it armed
// for the rest of the process lifetime.
func TestCooldownClearsOnSuccess(t *testing.T) {
setFailTime(t, time.Now())
if !coolingDown() {

12
service/setting.go
View file

@ -112,14 +112,10 @@ func (s *SettingService) GetAllSetting() (*map[string]string, error) {
return &allSetting, nil
}
// ResetSettings restores the operator-facing settings to their defaults.
//
// It deliberately keeps the bookkeeping rows. Deleting every row took the
// schema version with it, and a database with no version reads as pre-1.2: the
// next `s-ui migrate` replayed the entire legacy chain against a current
// database and failed, every time, for good. The `migrated*` flags are the same
// story one level down -- without them the one-off data migrations in
// database/ all run again on the next start.
// ResetSettings restores the operator-facing settings to their defaults, and
// deliberately keeps the bookkeeping rows: without the version row the database
// reads as pre-1.2 and every `s-ui migrate` replays the whole legacy chain,
// and without the migrated* flags the one-off data migrations all run again.
//
// These are not settings the operator set, so resetting them is not what the
// command means in the first place.

30
service/stats.go
View file

@ -18,21 +18,14 @@ type onlines struct {
}
var (
// statsMu guards both of the values below. SaveStats runs on the ten-second
// cron while GetOnlines is read from a gin handler, and the slices were
// being rebuilt under the reader's feet.
// Guards both values below: SaveStats runs on the ten-second cron while
// GetOnlines is read from a gin handler.
statsMu sync.Mutex
onlineResources = &onlines{}
// pendingStats holds traffic drained from the core that has not reached the
// database yet.
//
// StatsTracker.GetStats is destructive -- it Swap(0)s every counter -- so
// the old code lost the whole ten-second window for every user whenever the
// transaction failed. With _txlock=immediate a single SQLITE_BUSY from the
// daily stats purge was enough, and the counters were already zeroed by
// then, so the traffic could not be recovered. Carrying it forward means a
// busy database delays accounting instead of dropping it.
// Traffic drained from the core that has not reached the database yet.
// GetStats is destructive (it Swap(0)s every counter), so without this a
// single SQLITE_BUSY loses the whole ten-second window for every user.
pendingStats []model.Stats
)
@ -229,8 +222,7 @@ func (s *StatsService) downsampleStats(stats []model.Stats, startTime, endTime i
func (s *StatsService) GetOnlines() (onlines, error) {
statsMu.Lock()
defer statsMu.Unlock()
// Copied, not returned by reference: the caller must not hold slices the
// next cron tick is about to replace.
// Copied: the caller must not hold slices the next cron tick replaces.
return onlines{
Inbound: append([]string(nil), onlineResources.Inbound...),
User: append([]string(nil), onlineResources.User...),
@ -242,13 +234,9 @@ func (s *StatsService) GetOnlines() (onlines, error) {
// released between chunks.
const delOldStatsChunk = 5000
// DelOldStats drops stats older than the retention window.
//
// It deletes in bounded chunks rather than one unbounded statement. On a panel
// with months of history that single DELETE held the write lock well past the
// ten-second busy timeout, and the stats job that fires meanwhile had already
// drained the core's counters -- so the daily cleanup was itself a cause of
// lost traffic accounting.
// DelOldStats drops stats older than the retention window, in bounded chunks.
// One unbounded DELETE held the write lock past the busy timeout, which made
// the daily cleanup itself a cause of lost traffic accounting.
func (s *StatsService) DelOldStats(days int) error {
oldTime := time.Now().AddDate(0, 0, -(days)).Unix()
db := database.GetDB()