sql: deadlock when shutting down SQL instance

[jaylim-crl]

Describe the problem

Related to #66600.

We currently terminate the SQL process whenever a SQL instance session expires.
At the moment when that happens, the stopper gets stuck in the quiescing state,
and the process will appear to stuck until it is forcibly terminated through a signal.

The root cause of this problem is that we're calling stopper.Stop within an async
task. During the quiescing phase, the stopper waits for all tasks to terminate before
returning, but that will never happen because the goroutine that called stopper.Stop
is an async task itself.

heartbeatLoop is registered as an async task:

cockroach/pkg/sql/sqlliveness/slinstance/slinstance.go

Line 294 in 617beac

_ = l.stopper.RunAsyncTask(ctx, "slinstance", l.heartbeatLoop)

When a session expires, we invoke all the registered callbacks:

cockroach/pkg/sql/sqlliveness/slinstance/slinstance.go

Lines 137 to 147 in 617beac

	func (l *Instance) clearSession(ctx context.Context) {
	l.mu.Lock()
	defer l.mu.Unlock()
	if expiration := l.mu.s.Expiration(); expiration.Less(l.clock.Now()) {
	// If the session has expired, invoke the session expiry callbacks
	// associated with the session.
	l.mu.s.invokeSessionExpiryCallbacks(ctx)
	}
	l.mu.s = nil
	l.mu.blockCh = make(chan struct{})
	}

The registered callbacks, which happens only to be shutdownSQLInstance, invoke
stopper.Stop to tell the process to terminate:

cockroach/pkg/sql/sqlinstance/instanceprovider/instanceprovider.go

Line 143 in 9208567

p.stopper.Stop(ctx)

A side effect to this is that we can also get deadlocks on mutexes. Before invoking
the callbacks, we grab l.mu as shown above. But l.mu is needed to obtain a
live session from the SQL instance:

cockroach/pkg/sql/sqlliveness/slinstance/slinstance.go

Lines 298 to 312 in 9208567

	// Session returns a live session id. For each Sqlliveness instance the
	// invariant is that there exists at most one live session at any point in time.
	// If the current one has expired then a new one is created.
	func (l *Instance) Session(ctx context.Context) (sqlliveness.Session, error) {
	if l.testKnobs.SessionOverride != nil {
	if s, err := l.testKnobs.SessionOverride(ctx); s != nil || err != nil {
	return s, err
	}
	}
	l.mu.Lock()
	if !l.mu.started {
	l.mu.Unlock()
	return nil, sqlliveness.NotStartedError
	}
	l.mu.Unlock()

Async tasks that go into the stopper can come in many forms. Based on my
experiments, the problematic ones that I've seen are:

1. cockroach/pkg/sql/catalog/lease/lease.go

   Lines 384 to 391 in 617beac

   	// Release to the store asynchronously, without the descriptorState lock.
   	if err := m.stopper.RunAsyncTask(
   	ctx, "sql.descriptorState: releasing descriptor lease",
   	func(ctx context.Context) {
   	m.storage.release(ctx, m.stopper, lease)
   	}); err != nil {
   	log.Warningf(ctx, "error: %s, not releasing lease: %q", err, lease)
   	}

2. cockroach/pkg/jobs/registry.go

   Lines 887 to 911 in 617beac

   	return stopper.RunAsyncTask(context.Background(), "jobs/adopt", func(ctx context.Context) {
   	ctx, cancel := stopper.WithCancelOnQuiesce(ctx)
   	defer cancel()
   	lc, cleanup := makeLoopController(r.settings, adoptIntervalSetting, r.knobs.IntervalOverrides.Adopt)
   	defer cleanup()
   	for {
   	select {
   	case <-lc.updated:
   	lc.onUpdate()
   	case <-stopper.ShouldQuiesce():
   	return
   	case shouldClaim := <-r.adoptionCh:
   	// Try to adopt the most recently created job.
   	if shouldClaim {
   	claimJobs(ctx)
   	}
   	processClaimedJobs(ctx)
   	case <-lc.timer.C:
   	lc.timer.Read = true
   	claimJobs(ctx)
   	processClaimedJobs(ctx)
   	lc.onExecute()
   	}
   	}
   	})

Both of these will eventually try to obtain a live session through the
sqlinstance.Session here and here.

These two tasks may be present in the stopper when it stops as a result of the
expiry of a SQL instance session. The code to stop the process grabs l.mu,
but these tasks require l.mu to proceed, so we have a deadlock.

To Reproduce

Trying to reproduce a situation where only one task left (root caller) in the stopper
is difficult. However, a deadlock scenario on the mutexes can easily be reproduced
by running TPCH queries with a high concurrency for a few minutes on the SQL pod.
This assume a tenant with just one SQL pod, so all queries will be hitting it.

cockroach workload run tpch '<URL>'  --queries 1 --concurrency=1024 --tolerate-errors

The query above is going to cause overload on the SQL pod with a memory limit of 16GB.
When running that query, current SQL memory ramps up to hit 4GB, which is 25% of
16GB based on the --max-sql-memory flag.

Expected behavior

The SQL pod should terminate properly whenever a SQL instance session has expired.

Additional context

Some slack conversation can be found here: https://cockroachlabs.slack.com/archives/C01CDD4HRC5/p1633564911120200. I did not post logs as they contain CC specific information. This is causing instability in Serverless within the custom autoscaler logic because the pods will not shutdown. Marking it as a release-blocker as per @andy-kimball.

/cc @rimadeodhar @ajwerner @dhartunian

[jaylim-crl]

One solution that was discussed is to run callbacks here asynchronously in goroutines. I'm not super familiar with the code, and I don't know if that is safe. Running callbacks in goroutines will solve the issue above because the root caller will terminate so the heartbeatLoop task in the stopper can finish, and it won't cause deadlocks because returning from the root caller will release l.mu, allowing tasks that require it to proceed.

[ajwerner]

We should just have the callback launch a goroutine.

[ajwerner]

I think if we put. a go here, it'll fix our problem:

cockroach/pkg/sql/sqlinstance/instanceprovider/instanceprovider.go

Line 143 in 9208567

p.stopper.Stop(ctx)

[dhartunian]

@ajwerner i was going to add a go prior to the callback calls here:

cockroach/pkg/sql/sqlliveness/slinstance/slinstance.go

Line 89 in 76b02ba

callback(ctx)

Do you think it's better to just put it inside the shutdownSQLInstance callback only?

[jaylim-crl]

Thanks @dhartunian!