Make TestActivityLog_MultipleFragmentsAndSegments timeout on its own (h…

…ashicorp#11490)

* The main driver for this change was to make the read from a.newFragmentCh timeout quickly rather than waiting for the test timeout (much longer).  While testing the change I observed a panic during shutdown, but it was swallowed and moreover there was no stack trace so it wasn't obvious.  I'm hoping we can get rid of the recover, so I fixed the issue in the activitylog tests that needed it.

vault/activity_log.go

@@ -456,7 +456,6 @@ func (a *ActivityLog) getLastEntitySegmentNumber(ctx context.Context, startTime
 func (a *ActivityLog) WalkEntitySegments(ctx context.Context,
  startTime time.Time,
  walkFn func(*activity.EntityActivityLog)) error {
-
  basePath := activityEntityBasePath + fmt.Sprint(startTime.Unix()) + "/"
  pathList, err := a.view.List(ctx, basePath)
  if err != nil {
@@ -486,7 +485,6 @@ func (a *ActivityLog) WalkEntitySegments(ctx context.Context,
 func (a *ActivityLog) WalkTokenSegments(ctx context.Context,
  startTime time.Time,
  walkFn func(*activity.TokenCount)) error {
-
  basePath := activityTokenBasePath + fmt.Sprint(startTime.Unix()) + "/"
  pathList, err := a.view.List(ctx, basePath)
  if err != nil {
@@ -995,7 +993,7 @@ func (c *Core) setupActivityLog(ctx context.Context, wg *sync.WaitGroup) error {
 
 // stopActivityLog removes the ActivityLog from Core
 // and frees any resources.
-func (c *Core) stopActivityLog() error {
+func (c *Core) stopActivityLog() {
  if c.tokenStore != nil {
  c.tokenStore.SetActivityLog(nil)
  }
@@ -1007,7 +1005,6 @@ func (c *Core) stopActivityLog() error {
  }
 
  c.activityLog = nil
- return nil
 }
 
 func (a *ActivityLog) StartOfNextMonth() time.Time {
@@ -1526,14 +1523,20 @@ func (a *ActivityLog) precomputedQueryWorker() error {
 
  // Cancel the context if activity log is shut down.
  // This will cause the next storage operation to fail.
- go func() {
+ a.l.RLock()
+ // doneCh is modified in some tests, so we don't want to access that member
+ // without a lock, but we don't want to hold the lock for the entire lifetime
+ // of this goroutine. Passing the channel to the goroutine works here because
+ // no tests depend on us accessing the new doneCh after modifying the field.
+ go func(done chan struct{}) {
  select {
- case <-a.doneCh:
+ case <-done:
  cancel()
  case <-ctx.Done():
  break
  }
- }()
+ }(a.doneCh)
+ a.l.RUnlock()
 
  // Load the intent log
  rawIntentLog, err := a.view.Get(ctx, activityIntentLogKey)

vault/activity_log_test.go

@@ -431,6 +431,11 @@ func TestActivityLog_MultipleFragmentsAndSegments(t *testing.T) {
 
  // Stop timers for test purposes
  close(a.doneCh)
+ defer func() {
+ a.l.Lock()
+ a.doneCh = make(chan struct{}, 1)
+ a.l.Unlock()
+ }()
 
  startTimestamp := a.GetStartTimestamp()
  path0 := fmt.Sprintf("sys/counters/activity/log/entity/%d/0", startTimestamp)
@@ -516,7 +521,11 @@ func TestActivityLog_MultipleFragmentsAndSegments(t *testing.T) {
  a.receivedFragment(fragment1)
  a.receivedFragment(fragment2)
 
- <-a.newFragmentCh
+ select {
+ case <-a.newFragmentCh:
+ case <-time.After(time.Minute):
+ t.Fatal("timed out waiting for new fragment")
+ }
 
  err = a.saveCurrentSegmentToStorage(context.Background(), false)
  if err != nil {
@@ -1376,6 +1385,9 @@ func TestActivityLog_refreshFromStoredLogWithBackgroundLoadingCancelled(t *testi
 
  var wg sync.WaitGroup
  close(a.doneCh)
+ defer func() {
+ a.doneCh = make(chan struct{}, 1)
+ }()
 
  err := a.refreshFromStoredLog(context.Background(), &wg, time.Now().UTC())
  if err != nil {
@@ -2249,7 +2261,8 @@ func TestActivityLog_PrecomputeCancel(t *testing.T) {
 
  // This will block if the shutdown didn't work.
  go func() {
- a.precomputedQueryWorker()
+ // We expect this to error because of BlockingInmemStorage
+ _ = a.precomputedQueryWorker()
  close(done)
  }()
 

vault/core.go

@@ -2154,9 +2154,7 @@ func (c *Core) preSeal() error {
  if err := c.stopExpiration(); err != nil {
  result = multierror.Append(result, errwrap.Wrapf("error stopping expiration: {{err}}", err))
  }
- if err := c.stopActivityLog(); err != nil {
- result = multierror.Append(result, errwrap.Wrapf("error stopping activity log: {{err}}", err))
- }
+ c.stopActivityLog()
  if err := c.teardownCredentials(context.Background()); err != nil {
  result = multierror.Append(result, errwrap.Wrapf("error tearing down credentials: {{err}}", err))
  }

vault/expiration_test.go

@@ -554,7 +554,8 @@ func benchmarkExpirationBackend(b *testing.B, physicalBackend physical.Backend,
 }
 
 func TestExpiration_Restore(t *testing.T) {
- exp := mockExpiration(t)
+ c, _, _ := TestCoreUnsealed(t)
+ exp := c.expiration
  noop := &NoopBackend{}
  _, barrier, _ := mockBarrier(t)
  view := NewBarrierView(barrier, "logical/")
@@ -601,7 +602,7 @@ func TestExpiration_Restore(t *testing.T) {
  }
 
  // Stop everything
- err = exp.Stop()
+ err = c.stopExpiration()
  if err != nil {
  t.Fatalf("err: %v", err)
  }

vault/testing.go

@@ -359,11 +359,6 @@ func testCoreUnsealed(t testing.T, core *Core) (*Core, [][]byte, string) {
  testCoreAddSecretMount(t, core, token)
 
  t.Cleanup(func() {
- defer func() {
- if r := recover(); r != nil {
- t.Log("panic closing core during cleanup", "panic", r)
- }
- }()
  core.Shutdown()
  })
  return core, keys, token
@@ -1871,7 +1866,6 @@ func (testCluster *TestCluster) newCore(t testing.T, idx int, coreConfig *CoreCo
 func (testCluster *TestCluster) setupClusterListener(
  t testing.T, idx int, core *Core, coreConfig *CoreConfig,
  opts *TestClusterOptions, listeners []*TestListener, handler http.Handler) {
-
  if coreConfig.ClusterAddr == "" {
  return
  }
@@ -2058,7 +2052,6 @@ func (tc *TestCluster) initCores(t testing.T, opts *TestClusterOptions, addAudit
 func (testCluster *TestCluster) getAPIClient(
  t testing.T, opts *TestClusterOptions,
  port int, tlsConfig *tls.Config) *api.Client {
-
  transport := cleanhttp.DefaultPooledTransport()
  transport.TLSClientConfig = tlsConfig.Clone()
  if err := http2.ConfigureTransport(transport); err != nil {

vault/token_store_test.go

@@ -1150,7 +1150,7 @@ func TestTokenStore_CreateLookup_ExpirationInRestoreMode(t *testing.T) {
  t.Fatalf("err: %v", err)
  }
 
- err = ts.expiration.Stop()
+ err = c.stopExpiration()
  if err != nil {
  t.Fatal(err)
  }