diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 9817404..f10f55d 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -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 diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 0085ce9..22c191a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -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 diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml deleted file mode 100644 index fd2bb59..0000000 --- a/.github/workflows/windows.yml +++ /dev/null @@ -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 diff --git a/build-tags.sh b/build-tags.sh index c178e10..0c37118 100755 --- a/build-tags.sh +++ b/build-tags.sh @@ -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 diff --git a/cmd/migration/main.go b/cmd/migration/main.go index 5765230..660b4d7 100644 --- a/cmd/migration/main.go +++ b/cmd/migration/main.go @@ -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") diff --git a/cmd/migration/main_test.go b/cmd/migration/main_test.go index 69f4894..ff73c22 100644 --- a/cmd/migration/main_test.go +++ b/cmd/migration/main_test.go @@ -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) diff --git a/core/lifecycle_test.go b/core/lifecycle_test.go index a86e950..00e27e0 100644 --- a/core/lifecycle_test.go +++ b/core/lifecycle_test.go @@ -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{}) diff --git a/core/main.go b/core/main.go index 9371349..bf6492d 100644 --- a/core/main.go +++ b/core/main.go @@ -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) } diff --git a/cronjob/cronJob.go b/cronjob/cronJob.go index 1a796b5..9227738 100644 --- a/cronjob/cronJob.go +++ b/cronjob/cronJob.go @@ -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) diff --git a/cronjob/resetTrafficJob.go b/cronjob/resetTrafficJob.go index 4f0695f..b028bfa 100644 --- a/cronjob/resetTrafficJob.go +++ b/cronjob/resetTrafficJob.go @@ -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) } diff --git a/database/backup.go b/database/backup.go index 3b6a1ca..023d1ba 100644 --- a/database/backup.go +++ b/database/backup.go @@ -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 } diff --git a/database/backup_test.go b/database/backup_test.go index f1ba0f1..002b506 100644 --- a/database/backup_test.go +++ b/database/backup_test.go @@ -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 diff --git a/database/db.go b/database/db.go index 7c7f30a..b6d028b 100644 --- a/database/db.go +++ b/database/db.go @@ -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 diff --git a/database/model/model.go b/database/model/model.go index e86fddc..104886d 100644 --- a/database/model/model.go +++ b/database/model/model.go @@ -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"` diff --git a/database/schema.go b/database/schema.go index f303a9f..36f59fb 100644 --- a/database/schema.go +++ b/database/schema.go @@ -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]}, diff --git a/logger/logger_test.go b/logger/logger_test.go index bd966b2..06545dd 100644 --- a/logger/logger_test.go +++ b/logger/logger_test.go @@ -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") diff --git a/service/client.go b/service/client.go index 5b321e2..674724c 100644 --- a/service/client.go +++ b/service/client.go @@ -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 { diff --git a/service/client_reset_test.go b/service/client_reset_test.go index c858ee5..3239f0c 100644 --- a/service/client_reset_test.go +++ b/service/client_reset_test.go @@ -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`, diff --git a/service/config.go b/service/config.go index b29c58f..56130c6 100644 --- a/service/config.go +++ b/service/config.go @@ -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 { diff --git a/service/config_lifecycle_test.go b/service/config_lifecycle_test.go index afc7231..8e89b7b 100644 --- a/service/config_lifecycle_test.go +++ b/service/config_lifecycle_test.go @@ -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() { diff --git a/service/setting.go b/service/setting.go index 416a172..aa7839d 100644 --- a/service/setting.go +++ b/service/setting.go @@ -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. diff --git a/service/stats.go b/service/stats.go index 6b9bc81..f2be06f 100644 --- a/service/stats.go +++ b/service/stats.go @@ -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()