shutdown: close the sockets, or the database never gets sealed
mavend seals its encrypted database in `defer st.Close()` when run() returns. It had not returned since 2026-07-21. Every restart since then decrypted the same eleven-day-old ciphertext and rolled back everything written in between: the Telegram nudge that kept firing was a fact being un-written on each boot. The goroutine dump named it. main → srv.Close() → ipc.(*Server).Close → wg.Wait(), waiting on per-connection goroutines parked in readFrame. Close shut the listener and nothing else, so the idle persistent sockets held by mavweb, mavpoll, mavcaldav and mavmaild blocked shutdown forever. `docker compose stop -t 60` spent the whole sixty seconds and then took a SIGKILL. So: track the accepted conns and close them, in ipc and in voice, which had the identical defect. Bound all three waits — the two per-server ones and the worker wait in main — because the seal matters more than any single in-flight call. A dropped RPC costs one reply; a missed seal costs a session. The regression test leaves a client connected and idle, which is the case the old tests avoided by closing the client first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TrVSBKe3RFDF4fGYKWYQnX
This commit is contained in:
@@ -640,3 +640,30 @@ func TestSwapModel_Hook(t *testing.T) {
|
||||
t.Fatalf("swap to a non-allowlisted path = %v; want ErrForbidden", err)
|
||||
}
|
||||
}
|
||||
|
||||
// The eleven-day bug: Close shut the listener but not the accepted conns, so
|
||||
// an idle client left serveConn parked in readFrame and wg.Wait never
|
||||
// returned. mavend deadlocked before `defer st.Close()` could re-encrypt the
|
||||
// database, and every write since the last clean stop was rolled back on the
|
||||
// next boot. The client here stays connected and idle on purpose.
|
||||
func TestCloseReturnsWithAnIdleClientConnected(t *testing.T) {
|
||||
_, srv, cli, _ := newServerWithStore(t)
|
||||
// Prove the conn is live and then leave it alone — no cli.Close().
|
||||
if _, err := cli.RecentFacts(context.Background(), 1); err != nil {
|
||||
t.Fatalf("warm-up call: %v", err)
|
||||
}
|
||||
|
||||
done := make(chan error, 1)
|
||||
go func() { done <- srv.Close() }()
|
||||
select {
|
||||
case err := <-done:
|
||||
if err != nil {
|
||||
t.Fatalf("close: %v", err)
|
||||
}
|
||||
// Deliberately shorter than closeGrace: the grace timer is the backstop,
|
||||
// not the mechanism. Closing the conns is what makes readFrame return, and
|
||||
// if that regresses this waits out the full grace and fails here.
|
||||
case <-time.After(closeGrace / 2):
|
||||
t.Fatal("Close blocked on an idle connection — the shutdown deadlock is back")
|
||||
}
|
||||
}
|
||||
|
||||
+87
-1
@@ -6,6 +6,7 @@ import (
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"log"
|
||||
"net"
|
||||
"os"
|
||||
"sync"
|
||||
@@ -451,6 +452,19 @@ type Server struct {
|
||||
done chan struct{}
|
||||
accept sync.Mutex // guards wg.Add vs Close's wg.Wait sequence
|
||||
|
||||
// conns — every accepted connection still being served. Close needs these
|
||||
// because closing the listener does nothing to a connection already
|
||||
// accepted: serveConn is parked in readFrame waiting for a peer that may
|
||||
// never say anything again, and the wg.Wait below would block forever.
|
||||
//
|
||||
// This was not theoretical. mavweb, mavpoll, mavcaldav and mavmaild all
|
||||
// hold a long-lived connection open, so on 2026-08-01 mavend deadlocked on
|
||||
// every single shutdown, never returned from run(), and never reached the
|
||||
// `defer st.Close()` that seals the database. The deployed ciphertext was
|
||||
// eleven days stale before anyone noticed.
|
||||
connMu sync.Mutex
|
||||
conns map[net.Conn]struct{}
|
||||
|
||||
// Check — optional authorization hook. dispatch runs it BEFORE method
|
||||
// dispatch, with the raw params, so the auth layer can make verdicts
|
||||
// that depend on the call's shape (e.g. WriteFact's source). A non-nil
|
||||
@@ -648,8 +662,10 @@ func (s *Server) Serve() error {
|
||||
s.accept.Lock()
|
||||
s.wg.Add(1)
|
||||
s.accept.Unlock()
|
||||
s.trackConn(c)
|
||||
go func(c net.Conn) {
|
||||
defer s.wg.Done()
|
||||
defer s.untrackConn(c)
|
||||
defer c.Close()
|
||||
s.serveConn(c)
|
||||
}(c)
|
||||
@@ -1231,18 +1247,88 @@ func (s *Server) Close() error {
|
||||
close(s.done)
|
||||
}
|
||||
err := s.ln.Close()
|
||||
// Closing the listener stops new connections; it does nothing to the ones
|
||||
// already accepted. Close those too, or every serveConn parked in readFrame
|
||||
// waits on a peer that has no reason to hang up and the Wait below never
|
||||
// returns. See the comment on Server.conns.
|
||||
s.closeConns()
|
||||
// Under accept lock: after the listener closes, no new Accept can complete,
|
||||
// so no new wg.Add will be called. The Wait is safe to observe the wg
|
||||
// counter because any in-flight Accept that already got a conn either
|
||||
// already called wg.Add (before releasing the lock) or will see the closed
|
||||
// listener error and not call wg.Add at all.
|
||||
s.accept.Lock()
|
||||
s.wg.Wait()
|
||||
waited := waitTimeout(&s.wg, closeGrace)
|
||||
s.accept.Unlock()
|
||||
if !waited {
|
||||
// Bounded on purpose. A dispatch can be mid-call into the resident
|
||||
// model, which has its own timeout measured in tens of seconds, and the
|
||||
// caller of Close is on its way to sealing the database with whatever
|
||||
// grace the supervisor allows. Abandoning one in-flight RPC is cheap;
|
||||
// missing the seal costs every write since the last clean shutdown.
|
||||
log.Printf("ipc: %d connection(s) still busy after %s, closing anyway", s.liveConns(), closeGrace)
|
||||
}
|
||||
_ = os.Remove(s.path)
|
||||
return err
|
||||
}
|
||||
|
||||
// closeGrace — how long Close waits for in-flight dispatches to finish before
|
||||
// giving up on them. Well inside the ten seconds docker allows by default, so
|
||||
// the caller still has time to seal.
|
||||
const closeGrace = 3 * time.Second
|
||||
|
||||
func (s *Server) trackConn(c net.Conn) {
|
||||
s.connMu.Lock()
|
||||
defer s.connMu.Unlock()
|
||||
if s.conns == nil {
|
||||
s.conns = make(map[net.Conn]struct{})
|
||||
}
|
||||
s.conns[c] = struct{}{}
|
||||
}
|
||||
|
||||
func (s *Server) untrackConn(c net.Conn) {
|
||||
s.connMu.Lock()
|
||||
defer s.connMu.Unlock()
|
||||
delete(s.conns, c)
|
||||
}
|
||||
|
||||
func (s *Server) liveConns() int {
|
||||
s.connMu.Lock()
|
||||
defer s.connMu.Unlock()
|
||||
return len(s.conns)
|
||||
}
|
||||
|
||||
// closeConns closes every live connection, which is what unblocks the reads.
|
||||
// The serveConn goroutines see the resulting error and return.
|
||||
func (s *Server) closeConns() {
|
||||
s.connMu.Lock()
|
||||
live := make([]net.Conn, 0, len(s.conns))
|
||||
for c := range s.conns {
|
||||
live = append(live, c)
|
||||
}
|
||||
s.connMu.Unlock()
|
||||
for _, c := range live {
|
||||
_ = c.Close()
|
||||
}
|
||||
}
|
||||
|
||||
// waitTimeout waits on wg for at most d, reporting whether it finished. The
|
||||
// abandoned goroutines are still holding a wg count, so nothing may reuse the
|
||||
// WaitGroup afterwards — Close is terminal, which is what makes this safe.
|
||||
func waitTimeout(wg *sync.WaitGroup, d time.Duration) bool {
|
||||
done := make(chan struct{})
|
||||
go func() {
|
||||
wg.Wait()
|
||||
close(done)
|
||||
}()
|
||||
select {
|
||||
case <-done:
|
||||
return true
|
||||
case <-time.After(d):
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
// Path returns the filesystem path of the listening socket.
|
||||
func (s *Server) Path() string { return s.path }
|
||||
|
||||
|
||||
@@ -268,3 +268,34 @@ func zero(b []byte) {
|
||||
b[i] = 0
|
||||
}
|
||||
}
|
||||
|
||||
// SealPlaintext encrypts an existing plaintext sqlite file at plainPath and
|
||||
// writes the ciphertext to cipherPath, atomically. key must be 32 bytes. The
|
||||
// plaintext file is left alone: this is a recovery path, and deleting the only
|
||||
// good copy of the data on the strength of a write that just succeeded is not
|
||||
// a trade worth making here.
|
||||
//
|
||||
// It exists for the case closeAndSeal cannot cover: a daemon that was killed
|
||||
// rather than shut down, leaving a live working copy in tmpfs and a stale
|
||||
// ciphertext on disk. mavseal folds the WAL in first, so what arrives here is
|
||||
// a single complete database.
|
||||
//
|
||||
// Nothing else should call this. The normal path is Close, which seals and
|
||||
// then wipes the plaintext and the key.
|
||||
func SealPlaintext(plainPath, cipherPath string, key []byte) error {
|
||||
if len(key) != keyLen {
|
||||
return ErrKeyLen
|
||||
}
|
||||
plain, err := os.ReadFile(plainPath)
|
||||
if err != nil {
|
||||
return fmt.Errorf("read working copy: %w", err)
|
||||
}
|
||||
blob, err := encrypt(key, plain)
|
||||
if err != nil {
|
||||
return fmt.Errorf("encrypt: %w", err)
|
||||
}
|
||||
if err := atomicWrite(cipherPath, blob); err != nil {
|
||||
return fmt.Errorf("seal ciphertext: %w", err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -57,8 +57,20 @@ type Server struct {
|
||||
ln net.Listener
|
||||
wg sync.WaitGroup
|
||||
done chan struct{}
|
||||
|
||||
// Accepted conns, tracked so Close can shut them. Same defect as
|
||||
// ipc/server.go had: closing only the listener leaves every idle client
|
||||
// parked in readFrame, wg.Wait never returns, and the daemon dies to
|
||||
// SIGKILL without sealing the database.
|
||||
connMu sync.Mutex
|
||||
conns map[net.Conn]struct{}
|
||||
}
|
||||
|
||||
// closeGrace — how long Close waits for in-flight dispatches before dropping
|
||||
// them. A push-to-talk turn can be mid-inference; abandoning one costs a reply,
|
||||
// hanging costs every write since the last clean shutdown.
|
||||
const closeGrace = 3 * time.Second
|
||||
|
||||
// NewServer builds a Server bound to addr (e.g. "127.0.0.1:9100" for a
|
||||
// local-only smoke; production: a wg-tunnel address). handler is the
|
||||
// reactive handler; sessions is shared with the voicesink (the daemon
|
||||
@@ -111,8 +123,10 @@ func (s *Server) Serve() error {
|
||||
}
|
||||
}
|
||||
s.wg.Add(1)
|
||||
s.trackConn(c)
|
||||
go func(c net.Conn) {
|
||||
defer s.wg.Done()
|
||||
defer s.untrackConn(c)
|
||||
s.serveConn(c)
|
||||
}(c)
|
||||
}
|
||||
@@ -212,10 +226,65 @@ func (s *Server) Close() error {
|
||||
if s.ln != nil {
|
||||
err = s.ln.Close()
|
||||
}
|
||||
s.wg.Wait()
|
||||
// Close the accepted conns too, or a client that is merely idle keeps
|
||||
// serveConn blocked in readFrame forever.
|
||||
s.closeConns()
|
||||
if !waitTimeout(&s.wg, closeGrace) {
|
||||
log.Printf("voice: %d connection(s) still busy after %s, closing anyway", s.liveConns(), closeGrace)
|
||||
}
|
||||
return err
|
||||
}
|
||||
|
||||
func (s *Server) trackConn(c net.Conn) {
|
||||
s.connMu.Lock()
|
||||
defer s.connMu.Unlock()
|
||||
if s.conns == nil {
|
||||
s.conns = make(map[net.Conn]struct{})
|
||||
}
|
||||
s.conns[c] = struct{}{}
|
||||
}
|
||||
|
||||
func (s *Server) untrackConn(c net.Conn) {
|
||||
s.connMu.Lock()
|
||||
defer s.connMu.Unlock()
|
||||
delete(s.conns, c)
|
||||
}
|
||||
|
||||
func (s *Server) liveConns() int {
|
||||
s.connMu.Lock()
|
||||
defer s.connMu.Unlock()
|
||||
return len(s.conns)
|
||||
}
|
||||
|
||||
// closeConns unblocks every parked reader. serveConn's own defer closes the
|
||||
// conn again; a second Close on a net.Conn is a harmless error.
|
||||
func (s *Server) closeConns() {
|
||||
s.connMu.Lock()
|
||||
conns := make([]net.Conn, 0, len(s.conns))
|
||||
for c := range s.conns {
|
||||
conns = append(conns, c)
|
||||
}
|
||||
s.connMu.Unlock()
|
||||
for _, c := range conns {
|
||||
_ = c.Close()
|
||||
}
|
||||
}
|
||||
|
||||
// waitTimeout waits on wg, but not forever. Reports whether it finished.
|
||||
func waitTimeout(wg *sync.WaitGroup, d time.Duration) bool {
|
||||
done := make(chan struct{})
|
||||
go func() {
|
||||
wg.Wait()
|
||||
close(done)
|
||||
}()
|
||||
select {
|
||||
case <-done:
|
||||
return true
|
||||
case <-time.After(d):
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
func unmarshalParams(raw json.RawMessage, v any) error {
|
||||
if len(raw) == 0 {
|
||||
raw = []byte("null")
|
||||
|
||||
Reference in New Issue
Block a user