Repository: kopia/kopia
Pull request #4408 (2025-02-13 03:26:09)

fix(server): prevent maintenance task on read-only repos
Avoid starting a maintenance task on the server when the repository connection (configuration) is read-only.

- Fixes: #4373

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kopia/kopia/pull/4408?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) Report
Attention: Patch coverage is `72.72727%` with `6 lines` in your changes missing coverage. Please review.
> Project coverage is 76.29%. Comparing base [(`cb455c6`)](https://app.codecov.io/gh/kopia/kopia/commit/cb455c699a04e54cb47e25a676d7d65063ce883e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) to head [(`ea199b3`)](https://app.codecov.io/gh/kopia/kopia/commit/ea199b3176071029a0a26ae4cb65b6dc9bcfada2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).
> Report is 417 commits behind head on master.

| [Files with missing lines](https://app.codecov.io/gh/kopia/kopia/pull/4408?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) | Patch % | Lines |
|---|---|---|
| [internal/server/server\_maintenance.go](https://app.codecov.io/gh/kopia/kopia/pull/4408?src=pr&el=tree&filepath=internal%2Fserver%2Fserver_maintenance.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-aW50ZXJuYWwvc2VydmVyL3NlcnZlcl9tYWludGVuYW5jZS5nbw==) | 83.33% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/kopia/kopia/pull/4408?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) |
| [...napshot/snapshotmaintenance/snapshotmaintenance.go](https://app.codecov.io/gh/kopia/kopia/pull/4408?src=pr&el=tree&filepath=snapshot%2Fsnapshotmaintenance%2Fsnapshotmaintenance.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-c25hcHNob3Qvc25hcHNob3RtYWludGVuYW5jZS9zbmFwc2hvdG1haW50ZW5hbmNlLmdv) | 0.00% | [2 Missing and 1 partial :warning: ](https://app.codecov.io/gh/kopia/kopia/pull/4408?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #4408      +/-   ##
==========================================
+ Coverage   75.86%   76.29%   +0.42%     
==========================================
  Files         470      527      +57     
  Lines       37301    39935    +2634     
==========================================
+ Hits        28299    30467    +2168     
- Misses       7071     7458     +387     
- Partials     1931     2010      +79     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/kopia/kopia/pull/4408?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).


Patch:
diff --git a/internal/server/server.go b/internal/server/server.go
index 8760c229..e49867f5 100644
--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -632,11 +632,7 @@ func (s *Server) SetRepository(ctx context.Context, rep repo.Repository) error {
 		return err
 	}
 
-	if dr, ok := s.rep.(repo.DirectRepository); ok {
-		s.maint = startMaintenanceManager(ctx, dr, s, s.options.MinMaintenanceInterval)
-	} else {
-		s.maint = nil
-	}
+	s.maint = maybeStartMaintenanceManager(ctx, s.rep, s, s.options.MinMaintenanceInterval)
 
 	s.sched = scheduler.Start(context.WithoutCancel(ctx), s.getSchedulerItems, scheduler.Options{
 		TimeNow:        clock.Now,
diff --git a/internal/server/server_maintenance.go b/internal/server/server_maintenance.go
index 1703d1e8..68a0df4e 100644
--- a/internal/server/server_maintenance.go
+++ b/internal/server/server_maintenance.go
@@ -110,12 +110,31 @@ func (s *srvMaintenance) nextMaintenanceTime() time.Time {
 	return s.cachedNextMaintenanceTime
 }
 
-func startMaintenanceManager(
+// Starts a periodic, background srvMaintenance task. Returns nil if no task was created.
+func maybeStartMaintenanceManager(
 	ctx context.Context,
-	rep repo.DirectRepository,
+	rep repo.Repository,
 	srv maintenanceManagerServerInterface,
 	minMaintenanceInterval time.Duration,
 ) *srvMaintenance {
+	// Check whether maintenance can be run and avoid unnecessarily starting a task that
+	// would fail later. Don't start a task when the repo is either:
+	// - not direct; or
+	// - read only.
+	// Note: the repo owner is not checked here since the repo owner can be externally
+	// changed while the server is running. The server would pick up the new onwer
+	// the next time a maintenance task executes.
+	dr, ok := rep.(repo.DirectRepository)
+	if !ok {
+		return nil
+	}
+
+	if rep.ClientOptions().ReadOnly {
+		log(ctx).Warnln("the repository connection is read-only, maintenance tasks will not be performed on this repository")
+
+		return nil
+	}
+
 	mctx, cancel := context.WithCancel(ctx)
 
 	m := srvMaintenance{
@@ -124,7 +143,7 @@ func startMaintenanceManager(
 		srv:                    srv,
 		cancelCtx:              cancel,
 		minMaintenanceInterval: minMaintenanceInterval,
-		dr:                     rep,
+		dr:                     dr,
 	}
 
 	m.wg.Add(1)
@@ -145,7 +164,7 @@ func startMaintenanceManager(
 
 				t0 := clock.Now()
 
-				if err := srv.runMaintenanceTask(mctx, rep); err != nil {
+				if err := srv.runMaintenanceTask(mctx, dr); err != nil {
 					log(ctx).Debugw("maintenance task failed", "err", err)
 					m.afterFailedRun()
 
diff --git a/snapshot/snapshotmaintenance/snapshotmaintenance.go b/snapshot/snapshotmaintenance/snapshotmaintenance.go
index 35d1ea13..eee65d59 100644
--- a/snapshot/snapshotmaintenance/snapshotmaintenance.go
+++ b/snapshot/snapshotmaintenance/snapshotmaintenance.go
@@ -11,8 +11,15 @@
 	"github.com/kopia/kopia/snapshot/snapshotgc"
 )
 
+// ErrReadonly indicates a failure when attempting to run maintenance on a read-only repository.
+var ErrReadonly = errors.New("not running maintenance on read-only repository connection")
+
 // Run runs the complete snapshot and repository maintenance.
 func Run(ctx context.Context, dr repo.DirectRepositoryWriter, mode maintenance.Mode, force bool, safety maintenance.SafetyParameters) error {
+	if dr.ClientOptions().ReadOnly {
+		return ErrReadonly
+	}
+
 	//nolint:wrapcheck
 	return maintenance.RunExclusive(ctx, dr, mode, force,
 		func(ctx context.Context, runParams maintenance.RunParameters) error {


Test patch:
diff --git a/internal/repotesting/repotesting.go b/internal/repotesting/repotesting.go
index de3acce5..095ffde4 100644
--- a/internal/repotesting/repotesting.go
+++ b/internal/repotesting/repotesting.go
@@ -38,6 +38,7 @@ type Environment struct {
 
 // Options used during Environment Setup.
 type Options struct {
+	ConnectOptions       func(*repo.ConnectOptions)
 	NewRepositoryOptions func(*repo.NewRepositoryOptions)
 	OpenOptions          func(*repo.Options)
 }
@@ -62,6 +63,7 @@ func (e *Environment) setup(tb testing.TB, version format.Version, opts ...Optio
 	ctx := testlogging.Context(tb)
 	e.configDir = testutil.TempDirectory(tb)
 	openOpt := &repo.Options{}
+	connectOpt := &repo.ConnectOptions{}
 
 	opt := &repo.NewRepositoryOptions{
 		BlockFormat: format.ContentFormat{
@@ -86,6 +88,10 @@ func (e *Environment) setup(tb testing.TB, version format.Version, opts ...Optio
 		if mod.OpenOptions != nil {
 			mod.OpenOptions(openOpt)
 		}
+
+		if mod.ConnectOptions != nil {
+			mod.ConnectOptions(connectOpt)
+		}
 	}
 
 	var st blob.Storage
@@ -106,7 +112,7 @@ func (e *Environment) setup(tb testing.TB, version format.Version, opts ...Optio
 	err := repo.Initialize(ctx, st, opt, e.Password)
 	require.NoError(tb, err)
 
-	err = repo.Connect(ctx, e.ConfigFile(), st, e.Password, nil)
+	err = repo.Connect(ctx, e.ConfigFile(), st, e.Password, connectOpt)
 	require.NoError(tb, err, "can't connect")
 
 	e.connected = true
diff --git a/internal/server/server_maintenance_test.go b/internal/server/server_maintenance_test.go
index 1d478549..e2c8ff85 100644
--- a/internal/server/server_maintenance_test.go
+++ b/internal/server/server_maintenance_test.go
@@ -75,7 +75,7 @@ func TestServerMaintenance(t *testing.T) {
 
 	ts := &testServer{log: t.Logf}
 
-	mm := startMaintenanceManager(ctx, env.RepositoryWriter, ts, time.Minute)
+	mm := maybeStartMaintenanceManager(ctx, env.RepositoryWriter, ts, time.Minute)
 	require.Equal(t, time.Time{}, mm.nextMaintenanceNoEarlierThan)
 
 	defer mm.stop(ctx)
@@ -102,3 +102,39 @@ func TestServerMaintenance(t *testing.T) {
 	// after a failure next maintenance time should be deferred by a minute.
 	require.Greater(t, mm.nextMaintenanceTime().Sub(clock.Now()), 50*time.Second)
 }
+
+func TestServerMaintenanceReadOnlyRepoConnection(t *testing.T) {
+	ctx, env := repotesting.NewEnvironment(t, repotesting.FormatNotImportant)
+
+	require.NoError(t, repo.DirectWriteSession(ctx, env.RepositoryWriter, repo.WriteSessionOptions{}, func(ctx context.Context, dw repo.DirectRepositoryWriter) error {
+		return maintenance.SetParams(ctx, dw, &maintenance.Params{
+			Owner: env.Repository.ClientOptions().UsernameAtHost(),
+			QuickCycle: maintenance.CycleParams{
+				Enabled:  true,
+				Interval: 5 * time.Second,
+			},
+			FullCycle: maintenance.CycleParams{
+				Enabled:  true,
+				Interval: 10 * time.Second,
+			},
+		})
+	}))
+
+	dr, ok := env.Repository.(repo.DirectRepository)
+	require.True(t, ok, "not a direct repository connection")
+
+	dr.Refresh(ctx)
+
+	// make repo read-only
+	co := env.Repository.ClientOptions()
+	co.ReadOnly = true
+
+	repo.SetClientOptions(ctx, env.ConfigFile(), co)
+
+	env.MustReopen(t)
+
+	ts := &testServer{log: t.Logf}
+	mm := maybeStartMaintenanceManager(ctx, env.RepositoryWriter, ts, time.Minute)
+
+	require.Nil(t, mm, "maintenance task should not be created on read-only repo")
+}
diff --git a/snapshot/snapshotmaintenance/snapshotmaintenance_test.go b/snapshot/snapshotmaintenance/snapshotmaintenance_test.go
index bb787fb5..68eee9f3 100644
--- a/snapshot/snapshotmaintenance/snapshotmaintenance_test.go
+++ b/snapshot/snapshotmaintenance/snapshotmaintenance_test.go
@@ -322,6 +322,18 @@ func (s *formatSpecificTestSuite) TestMaintenanceAutoLiveness(t *testing.T) {
 	require.NotEmpty(t, sched.Runs[maintenance.TaskSnapshotGarbageCollection], maintenance.TaskSnapshotGarbageCollection)
 }
 
+func TestNoMaintenanceReadOnly(t *testing.T) {
+	ctx, env := repotesting.NewEnvironment(t, repotesting.FormatNotImportant, repotesting.Options{
+		ConnectOptions: func(o *repo.ConnectOptions) {
+			o.ReadOnly = true
+		},
+	})
+
+	err := snapshotmaintenance.Run(ctx, env.RepositoryWriter, maintenance.ModeAuto, true, maintenance.SafetyFull)
+
+	require.ErrorIs(t, err, snapshotmaintenance.ErrReadonly)
+}
+
 func (th *testHarness) fakeTimeOpenRepoOption(o *repo.Options) {
 	o.TimeNowFunc = th.fakeTime.NowFunc()
 }