feat(handlers): repair bad lockfile states when enforcement is disabled (6a38fc4b) · Commits · GitLab.org / container-registry

registry/handlers/app.go

+36 −4

Original line number	Diff line number	Diff line
		@@ -506,8 +506,9 @@ func (app *App) handleFilesystemLockFile(ctx context.Context) error {
		return ErrDatabaseInUse
		}

		// FS is already locked, no reason to enumerate legacy metadata.
		if fsLocked {
		// Skip enumeration if the fs is already locked and there's no db lockfile - this is the expected state.
		// Otherwise continue to full validation and possible data repair.
		if fsLocked && !dbLocked {
		return nil
		}

		@@ -537,10 +538,26 @@ func (app *App) handleFilesystemLockFile(ctx context.Context) error {
		return err
		}

		dbLockedAfter := dbLocked

		// Repair lockfile state from https://gitlab.com/gitlab-org/container-registry/-/work_items/1776 bug
		// If the feature flag is disabled and we've locked the filesystem, we should
		// ensure that only the fslockfile is present.
		if !feature.EnforceLockfiles.Enabled() && dbLocked {
		// nolint: revive // max-control-nesting
		if err := dbLocker.Unlock(app.Context); err != nil {
		log.WithError(err).Error("failed to remove database-in-use lockfile")
		return err
		}

		dbLockedAfter = false
		}

		log.WithFields(dlog.Fields{
		"fs_locked_before": false,
		"fs_locked_after": true,
		"db_locked": dbLocked,
		"db_locked_before": dbLocked,
		"db_locked_after": dbLockedAfter,
		"ff_enforce_lockfiles": feature.EnforceLockfiles.Enabled(),
		}).Warn("lockfile state transition: creating filesystem lock")
		case errors.As(err, new(storagedriver.PathNotFoundError)):
		@@ -2584,11 +2601,26 @@ func (app App) initializeMetadataDatabase(ctx context.Context, config configur
		return nil, err
		}

		fsLockedAfter := fsLocked

		// Repair lockfile state from https://gitlab.com/gitlab-org/container-registry/-/work_items/1776 bug
		// If the feature flag is disabled and we've locked the filesystem, we should
		// ensure that only the dblockfile is present.
		if !feature.EnforceLockfiles.Enabled() && fsLocked {
		if err := app.Lockers.FS.Unlock(app.Context); err != nil {
		log.WithError(err).Error("failed to remove filesystem-in-use lockfile")
		return nil, err
		}

		fsLockedAfter = false
		}

		if !dbLocked {
		log.WithFields(dlog.Fields{
		"db_locked_before": false,
		"db_locked_after": true,
		"fs_locked": fsLocked,
		"fs_locked_before": fsLocked,
		"fs_locked_after": fsLockedAfter,
		"ff_enforce_lockfiles": feature.EnforceLockfiles.Enabled(),
		}).Warn("lockfile state transition: creating database lock")
		}

registry/handlers/app_integration_test.go

+74 −21

Original line number	Diff line number	Diff line
		@@ -155,16 +155,22 @@ func TestHandleFilesystemLockFile_BothLocksPresent(t *testing.T) {
		name string
		ffEnforceLockfiles bool
		expectedErr error
		expectDBLock bool
		expectFSLock bool
		}{
		{
		name: "both locks with ff enabled",
		ffEnforceLockfiles: true,
		expectedErr: handlers.ErrInvalidLockfiles,
		expectDBLock: true,
		expectFSLock: true,
		},
		{
		name: "both locks with ff disabled",
		ffEnforceLockfiles: false,
		expectedErr: nil, // No enforcement when FF disabled
		expectDBLock: false, // Data repair when FF disabled
		expectFSLock: true,
		},
		}

		@@ -185,13 +191,22 @@ func TestHandleFilesystemLockFile_BothLocksPresent(t *testing.T) {
		require.NoError(tt, l.FS.Lock(ctx))
		require.NoError(tt, l.DB.Lock(ctx))

		app, err := handlers.NewApp(context.Background(), &config)
		app, appErr := handlers.NewApp(context.Background(), &config)

		locked, err := l.FSIsLocked(ctx)
		require.NoError(tt, err)
		assert.Equal(tt, tc.expectFSLock, locked)

		locked, err = l.DBIsLocked(ctx)
		require.NoError(tt, err)
		assert.Equal(tt, tc.expectDBLock, locked)

		if tc.expectedErr != nil {
		assert.ErrorIs(tt, err, tc.expectedErr)
		assert.ErrorIs(tt, appErr, tc.expectedErr)
		return
		}

		require.NoError(tt, err)
		require.NoError(tt, appErr)
		assert.NotNil(tt, app)
		})
		}
		@@ -245,16 +260,22 @@ func TestHandleFilesystemLockFile_DBLockedOnly(t *testing.T) {
		name string
		ffEnforceLockfiles bool
		expectedErr error
		expectDBLock bool
		expectFSLock bool
		}{
		{
		name: "db locked only with ff enabled",
		ffEnforceLockfiles: true,
		expectedErr: handlers.ErrDatabaseInUse,
		expectDBLock: true,
		expectFSLock: false,
		},
		{
		name: "db locked only with ff disabled",
		ffEnforceLockfiles: false,
		expectedErr: nil, // No enforcement when FF disabled
		expectDBLock: false, // Data repair when FF disabled
		expectFSLock: true,
		},
		}

		@@ -275,13 +296,22 @@ func TestHandleFilesystemLockFile_DBLockedOnly(t *testing.T) {
		require.NoError(tt, l.FS.Unlock(ctx))
		require.NoError(tt, l.DB.Lock(ctx))

		app, err := handlers.NewApp(context.Background(), &config)
		app, appErr := handlers.NewApp(context.Background(), &config)

		locked, err := l.FSIsLocked(ctx)
		require.NoError(tt, err)
		assert.Equal(tt, tc.expectFSLock, locked)

		locked, err = l.DBIsLocked(ctx)
		require.NoError(tt, err)
		assert.Equal(tt, tc.expectDBLock, locked)

		if tc.expectedErr != nil {
		assert.ErrorIs(tt, err, tc.expectedErr)
		assert.ErrorIs(tt, appErr, tc.expectedErr)
		return
		}

		require.NoError(tt, err)
		require.NoError(tt, appErr)
		assert.NotNil(tt, app)
		})
		}
		@@ -384,13 +414,15 @@ func TestInitializeMetadataDatabase_BothLocksPresent(t *testing.T) {
		dbMode configuration.DatabaseEnabled
		expectedErr error
		expectDBLock bool
		expectFSLock bool
		}{
		{
		name: "both locks with db enabled and ff enabled",
		ffEnforceLockfiles: true,
		dbMode: configuration.DatabaseEnabledTrue,
		expectedErr: handlers.ErrInvalidLockfiles,
		expectDBLock: false,
		expectDBLock: true,
		expectFSLock: true,
		},
		{
		name: "both locks with db enabled and ff disabled",
		@@ -398,13 +430,15 @@ func TestInitializeMetadataDatabase_BothLocksPresent(t *testing.T) {
		dbMode: configuration.DatabaseEnabledTrue,
		expectedErr: nil, // No enforcement, continues initialization
		expectDBLock: true,
		expectFSLock: false, // Data repair when FF disabled
		},
		{
		name: "both locks with db prefer and ff enabled",
		ffEnforceLockfiles: true,
		dbMode: configuration.DatabaseEnabledPrefer,
		expectedErr: handlers.ErrInvalidLockfiles,
		expectDBLock: false,
		expectDBLock: true,
		expectFSLock: true,
		},
		{
		name: "both locks with db prefer and ff disabled",
		@@ -412,6 +446,7 @@ func TestInitializeMetadataDatabase_BothLocksPresent(t *testing.T) {
		dbMode: configuration.DatabaseEnabledPrefer,
		expectedErr: nil,
		expectDBLock: true,
		expectFSLock: false, // Data repair when FF disabled
		},
		}

		@@ -429,6 +464,7 @@ func TestInitializeMetadataDatabase_BothLocksPresent(t *testing.T) {
		ctx := context.Background()
		l := newTestLockers(t, &config)
		require.NoError(tt, l.DB.Lock(ctx))
		require.NoError(tt, l.FS.Lock(ctx))

		// Reconfigure with database enabled
		config.Database.Enabled = tc.dbMode
		@@ -436,21 +472,23 @@ func TestInitializeMetadataDatabase_BothLocksPresent(t *testing.T) {
		truncateTables := runMigrations(t, &config)
		tt.Cleanup(truncateTables)

		app, err := handlers.NewApp(context.Background(), &config)
		if tc.expectedErr != nil {
		assert.ErrorIs(tt, err, tc.expectedErr)
		return
		}
		app, appErr := handlers.NewApp(context.Background(), &config)

		locked, err := l.FSIsLocked(ctx)
		require.NoError(tt, err)
		assert.NotNil(tt, app)
		assert.Equal(tt, tc.expectFSLock, locked)

		// Verify DB lock state
		if tc.expectDBLock {
		locked, err := app.Lockers.DBIsLocked(ctx)
		locked, err = l.DBIsLocked(ctx)
		require.NoError(tt, err)
		assert.True(tt, locked, "DB lock should be created")
		assert.Equal(tt, tc.expectDBLock, locked)

		if tc.expectedErr != nil {
		assert.ErrorIs(tt, appErr, tc.expectedErr)
		return
		}

		require.NoError(tt, appErr)
		assert.NotNil(tt, app)
		})
		}
		}
		@@ -522,16 +560,22 @@ func TestInitializeMetadataDatabase_FSLockedEnabledMode(t *testing.T) {
		name string
		ffEnforceLockfiles bool
		expectedErr error
		expectDBLock bool
		expectFSLock bool
		}{
		{
		name: "fs locked enabled mode with ff enabled",
		ffEnforceLockfiles: true,
		expectedErr: handlers.ErrFilesystemInUse,
		expectDBLock: false,
		expectFSLock: true,
		},
		{
		name: "fs locked enabled mode with ff disabled",
		ffEnforceLockfiles: false,
		expectedErr: nil, // No enforcement, continues
		expectDBLock: true,
		expectFSLock: false, // Data repair when FF disabled
		},
		}

		@@ -557,13 +601,22 @@ func TestInitializeMetadataDatabase_FSLockedEnabledMode(t *testing.T) {
		truncateTables := runMigrations(t, &config)
		tt.Cleanup(truncateTables)

		app, err := handlers.NewApp(context.Background(), &config)
		app, appErr := handlers.NewApp(context.Background(), &config)

		locked, err := l.FSIsLocked(ctx)
		require.NoError(tt, err)
		assert.Equal(tt, tc.expectFSLock, locked)

		locked, err = l.DBIsLocked(ctx)
		require.NoError(tt, err)
		assert.Equal(tt, tc.expectDBLock, locked)

		if tc.expectedErr != nil {
		assert.ErrorIs(tt, err, tc.expectedErr)
		assert.ErrorIs(tt, appErr, tc.expectedErr)
		return
		}

		require.NoError(tt, err)
		require.NoError(tt, appErr)
		assert.NotNil(tt, app)
		})
		}