Keep shutdown bounded, and do not start listeners after Stop

Now that the signal handler waits for Stop, two holes in Stop matter:

- Stop waits for every store to close, and those waits have no deadline of
  their own: attachment.Store.Close waits for the sync loop, whose
  AttachmentsWithSizes query runs against the database (and S3 deletes are in
  there too). A wedged database parked Stop before it ever reached the message
  cache, whose own 5s bound was then useless. Stop now gives up after 10s.
- A signal can arrive before Run has set up its listeners. Stop closed the
  stores and consumed stopOnce, and Run then started listeners against closed
  stores and blocked on the listener error, so the process neither served nor
  exited and a second signal could not stop it. Run bails out when the server
  is already stopped, outside the lock (a stuck shutdown holds it) and again
  under it.
This commit is contained in:
binwiederhier committed 2026-10-03 16:18:06 +00:00
1 parent c3c7d6eff1
commit 2153f646be
2 files changed
+92 -1

No files matched your search

+57
View File
@@ -10,6 +10,7 @@ import (
"errors"
"fmt"
"io"
"net"
"net/http"
"net/http/httptest"
"net/netip"
@@ -26,6 +27,7 @@ import (
"github.com/prometheus/client_golang/prometheus/promhttp"
"github.com/stretchr/testify/require"
"golang.org/x/crypto/bcrypt"
"heckel.io/ntfy/v2/attachment"
"heckel.io/ntfy/v2/db/pg"
dbtest "heckel.io/ntfy/v2/db/test"
"heckel.io/ntfy/v2/log"
@@ -5497,3 +5499,58 @@ func TestServer_StopFlushesBatchedMessages_Postgres(t *testing.T) {
require.Nil(t, host.DB.QueryRow(`SELECT COUNT(*) FROM message WHERE topic = 'mytopic'`).Scan(&count))
require.Equal(t, 2, count)
}
func TestServer_RunDoesNotStartListenersAfterStop(t *testing.T) {
// A signal can arrive before Run has set up its listeners. Run must not start them then: it
// would serve from closed stores and never return, so the process would hang until SIGKILL
ln, err := net.Listen("tcp", "127.0.0.1:0")
require.Nil(t, err)
addr := ln.Addr().String()
require.Nil(t, ln.Close())
conf := newTestConfig(t, "")
conf.ListenHTTP = addr
s := newTestServer(t, conf)
s.Stop()
done := make(chan error, 1)
go func() { done <- s.Run() }()
select {
case <-done:
case <-time.After(10 * time.Second):
t.Fatal("Run did not return after Stop")
}
_, err = net.DialTimeout("tcp", addr, time.Second)
require.Error(t, err, "listener was started after Stop")
}
func TestServer_StopIsBoundedWhenAStoreBlocks(t *testing.T) {
// Stop waits for every store to close, and those waits have no deadline of their own: the
// attachment sync loop queries the database, so a wedged database can park it indefinitely.
// Shutdown must give up rather than wait for systemd to SIGKILL us
s := newTestServer(t, newTestConfig(t, ""))
syncing := make(chan struct{})
var calls atomic.Int32
store, err := attachment.NewFileStore(t.TempDir(), 1024, time.Hour, func() (map[string]int64, error) {
if calls.Add(1) > 1 {
close(syncing)
select {} // Block forever, like a query against a wedged database
}
return map[string]int64{}, nil
})
require.Nil(t, err)
<-syncing
s.mu.Lock()
s.attachment = store
s.mu.Unlock()
stopped := make(chan struct{})
go func() {
s.Stop()
close(stopped)
}()
select {
case <-stopped:
case <-time.After(60 * time.Second):
t.Fatal("Stop did not return while a store was stuck")
}
}