c986a35 · crush · git.iko.soy

git.iko.soy/crush

on main help sign in

fix(db): only enforce the data directory lock in client server mode

c986a35Christian Rocha5 months ago

internal/backend/backend.go+1−1

compare with main

unifiedsplit

@@ -223,7 +223,7 @@ func (b *Backend) CreateWorkspace(args proto.Workspace) (*Workspace, proto.Works

223 return nil, proto.Workspace{}, fmt.Errorf("failed to create data directory: %w", err)

224 }

225

226− conn, err := db.Connect(b.ctx, cfg.Config().Options.DataDirectory)

226+ conn, err := db.Connect(b.ctx, cfg.Config().Options.DataDirectory, db.WithDataDirLock(true))

227 if err != nil {

228 return nil, proto.Workspace{}, fmt.Errorf("failed to connect to database: %w", err)

229 }

internal/db/connect.go+45−9

@@ -56,16 +56,39 @@ var (

56 poolMu sync.Mutex

57)

59+// ConnectOption configures a Connect call. Options are applied in

60+// order; later options override earlier ones for the same field.

61+type ConnectOption func(*connectOptions)

62+

63+// connectOptions holds the resolved configuration for a Connect call.

64+type connectOptions struct {

65+ lockDataDir bool

66+}

67+

68+// WithDataDirLock toggles acquisition of the per-data-directory lock

69+// for this Connect call. The lock is off by default so local-mode

70+// invocations do not regress today's behavior; the server's

71+// workspace-bootstrap path opts in. CRUSH_SKIP_DATADIR_LOCK still

72+// bypasses acquisition even when this option is set.

73+func WithDataDirLock(enable bool) ConnectOption {

74+ return func(o *connectOptions) { o.lockDataDir = enable }

75+}

76+

59// Connect opens a SQLite database connection for the given data

77// Connect opens a SQLite database connection for the given data

60// directory and runs migrations. If a connection to the same database

78// directory and runs migrations. If a connection to the same database

61// file already exists, the existing connection is returned with its

79// file already exists, the existing connection is returned with its

62// reference count incremented. Callers must pair each Connect with a

80// reference count incremented. Callers must pair each Connect with a

63// [Release] when they no longer need the connection.

81// [Release] when they no longer need the connection.

64−func Connect(ctx context.Context, dataDir string) (*sql.DB, error) {

82+func Connect(ctx context.Context, dataDir string, opts ...ConnectOption) (*sql.DB, error) {

65 if dataDir == "" {

83 if dataDir == "" {

66 return nil, fmt.Errorf("data.dir is not set")

84 return nil, fmt.Errorf("data.dir is not set")

67 }

85 }

87+ var cfg connectOptions

88+ for _, opt := range opts {

89+ opt(&cfg)

90+ }

91+

69 dbPath := filepath.Join(dataDir, "crush.db")

92 dbPath := filepath.Join(dataDir, "crush.db")

71 // Resolve to an absolute path so that different relative paths to

94 // Resolve to an absolute path so that different relative paths to

@@ -88,18 +111,25 @@ func Connect(ctx context.Context, dataDir string) (*sql.DB, error) {

88 // crush process on the same SQLite file. The lock is released when

111 // crush process on the same SQLite file. The lock is released when

89 // the matching Release call drops the refcount to zero. Ensuring

112 // the matching Release call drops the refcount to zero. Ensuring

90 // the data directory exists is required because the lock file

113 // the data directory exists is required because the lock file

91− // lives inside it.

114+ // lives inside it. Locking is opt-in via WithDataDirLock so that

115+ // local-mode invocations do not refuse a second crush against the

116+ // same data dir until client/server becomes the default.

92 if err := os.MkdirAll(dataDir, 0o700); err != nil {

117 if err := os.MkdirAll(dataDir, 0o700); err != nil {

93 return nil, fmt.Errorf("failed to create data directory %q: %w", dataDir, err)

118 return nil, fmt.Errorf("failed to create data directory %q: %w", dataDir, err)

94 }

119 }

120+ var lock *dataDirLock

121+ if cfg.lockDataDir && !skipDataDirLock() {

95− lock, err := acquireDataDirLock(dataDir)

122+ lock, err = acquireDataDirLock(dataDir)

96− if err != nil {

123+ if err != nil {

97− return nil, err

124+ return nil, err

125+ }

98 }

126 }

127

100 conn, err := openDB(dbPath)

128 conn, err := openDB(dbPath)

101 if err != nil {

129 if err != nil {

130+ if lock != nil {

102− lock.release()

131+ lock.release()

132+ }

103 return nil, err

133 return nil, err

104 }

134 }

105

135

@@ -110,22 +140,28 @@ func Connect(ctx context.Context, dataDir string) (*sql.DB, error) {

110 // resulting in SQLITE_NOTADB (26) on the next open.

140 // resulting in SQLITE_NOTADB (26) on the next open.

111 conn.SetMaxOpenConns(1)

141 conn.SetMaxOpenConns(1)

112

142

143+ releaseLock := func() {

144+ if lock != nil {

145+ lock.release()

146+ }

147+ }

148+

113 if err = conn.PingContext(ctx); err != nil {

149 if err = conn.PingContext(ctx); err != nil {

114 conn.Close()

150 conn.Close()

115− lock.release()

151+ releaseLock()

116 return nil, fmt.Errorf("failed to connect to database: %w", err)

152 return nil, fmt.Errorf("failed to connect to database: %w", err)

117 }

153 }

118

154

119 if err := initGoose(); err != nil {

155 if err := initGoose(); err != nil {

120 conn.Close()

156 conn.Close()

121− lock.release()

157+ releaseLock()

122 slog.Error("Failed to initialize goose", "error", err)

158 slog.Error("Failed to initialize goose", "error", err)

123 return nil, fmt.Errorf("failed to initialize goose: %w", err)

159 return nil, fmt.Errorf("failed to initialize goose: %w", err)

124 }

160 }

125

161

126 if err := goose.Up(conn, "migrations"); err != nil {

162 if err := goose.Up(conn, "migrations"); err != nil {

127 conn.Close()

163 conn.Close()

128− lock.release()

164+ releaseLock()

129 slog.Error("Failed to apply migrations", "error", err)

165 slog.Error("Failed to apply migrations", "error", err)

130 return nil, fmt.Errorf("failed to apply migrations: %w", err)

166 return nil, fmt.Errorf("failed to apply migrations: %w", err)

131 }

167 }

internal/db/connect_test.go+46−7

@@ -69,7 +69,7 @@ func TestConnect_FailsWhenDataDirLocked(t *testing.T) {

69 require.NoError(t, err, "expected to take the data-dir lock for the first time")

70 t.Cleanup(release)

72− _, err = Connect(context.Background(), dataDir)

72+ _, err = Connect(context.Background(), dataDir, WithDataDirLock(true))

73 require.Error(t, err, "Connect must refuse to open a locked data dir")

74 require.ErrorIs(t, err, ErrDataDirLocked)

75}

@@ -85,12 +85,12 @@ func TestConnect_SucceedsAfterContenderReleases(t *testing.T) {

85 release, err := tryFileLock(lockPath)

86 require.NoError(t, err)

88− _, err = Connect(context.Background(), dataDir)

88+ _, err = Connect(context.Background(), dataDir, WithDataDirLock(true))

89 require.ErrorIs(t, err, ErrDataDirLocked)

91 release()

93− conn, err := Connect(context.Background(), dataDir)

93+ conn, err := Connect(context.Background(), dataDir, WithDataDirLock(true))

94 require.NoError(t, err, "Connect should succeed once the contender releases the lock")

95 require.NoError(t, conn.PingContext(context.Background()))

96 require.NoError(t, Release(dataDir))

@@ -105,7 +105,7 @@ func TestConnect_LockReleasedOnFinalRelease(t *testing.T) {

105 dataDir := t.TempDir()

106 lockPath := filepath.Join(dataDir, dataDirLockFile)

107

108− conn, err := Connect(context.Background(), dataDir)

108+ conn, err := Connect(context.Background(), dataDir, WithDataDirLock(true))

109 require.NoError(t, err)

110 require.NoError(t, conn.PingContext(context.Background()))

111

@@ -135,10 +135,10 @@ func TestConnect_SharedPoolDoesNotReacquireLock(t *testing.T) {

135 dataDir := t.TempDir()

136 lockPath := filepath.Join(dataDir, dataDirLockFile)

137

138− _, err := Connect(context.Background(), dataDir)

138+ _, err := Connect(context.Background(), dataDir, WithDataDirLock(true))

139 require.NoError(t, err)

140

141− _, err = Connect(context.Background(), dataDir)

141+ _, err = Connect(context.Background(), dataDir, WithDataDirLock(true))

142 require.NoError(t, err)

143

144 // Drop one reference; lock must still be held.

@@ -163,8 +163,47 @@ func TestConnect_SkipLockEnvBypassesAcquisition(t *testing.T) {

163

164 t.Setenv("CRUSH_SKIP_DATADIR_LOCK", "1")

165

166− conn, err := Connect(context.Background(), dataDir)

166+ conn, err := Connect(context.Background(), dataDir, WithDataDirLock(true))

167 require.NoError(t, err, "skip-lock env should bypass contention")

168 require.NoError(t, conn.PingContext(context.Background()))

169 require.NoError(t, Release(dataDir))

170}

171+

172+// TestConnect_DefaultIgnoresContendedLock confirms that without

173+// WithDataDirLock(true) the lock file is irrelevant: a contender can

174+// hold tryFileLock and Connect still succeeds. This pins the

175+// local-mode default to its pre-lock behavior.

176+func TestConnect_DefaultIgnoresContendedLock(t *testing.T) {

177+ t.Cleanup(ResetPool)

178+

179+ dataDir := t.TempDir()

180+ lockPath := filepath.Join(dataDir, dataDirLockFile)

181+

182+ release, err := tryFileLock(lockPath)

183+ require.NoError(t, err, "expected to take the data-dir lock for the first time")

184+ t.Cleanup(release)

185+

186+ conn, err := Connect(context.Background(), dataDir)

187+ require.NoError(t, err, "default Connect must not take the lock and must succeed under contention")

188+ require.NoError(t, conn.PingContext(context.Background()))

189+ require.NoError(t, Release(dataDir))

190+}

191+

192+// TestConnect_ServerPathFailsWhenDataDirLocked is the server's

193+// workspace-bootstrap analogue of TestConnect_FailsWhenDataDirLocked:

194+// passing WithDataDirLock(true) must surface ErrDataDirLocked when a

195+// contender already holds the lock.

196+func TestConnect_ServerPathFailsWhenDataDirLocked(t *testing.T) {

197+ t.Cleanup(ResetPool)

198+

199+ dataDir := t.TempDir()

200+ lockPath := filepath.Join(dataDir, dataDirLockFile)

201+

202+ release, err := tryFileLock(lockPath)

203+ require.NoError(t, err, "expected to take the data-dir lock for the first time")

204+ t.Cleanup(release)

205+

206+ _, err = Connect(context.Background(), dataDir, WithDataDirLock(true))

207+ require.Error(t, err, "server-path Connect must refuse to open a locked data dir")

208+ require.ErrorIs(t, err, ErrDataDirLocked)

209+}