Basekick-Labs / Basekick-Labs/arc

applyBatchFileOps releases FSM lock between ops — concurrent readers can see partial batch state

Open
#447 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
677
Forks
53
Avg merge
9h 14m
Merged PRs (30d)
164

Description

## Summary

`internal/cluster/raft/fsm.go:applyBatchFileOps` runs each op through `apply*FileStruct`, each of which takes and releases `f.mu` individually. Between ops, concurrent readers (e.g. `GetFilesByDatabase`, `GetAllFiles`) can observe the FSM in a partially-updated state.

For compaction batches that delete N source files and register 1 compacted file in a single Raft command, a reader running mid-batch can briefly see:

- The compacted file already registered + some source files not yet deleted → query returns duplicate rows.
- Some source files already deleted + the compacted file not yet registered → query returns missing rows.

Both shapes break query correctness within the batch window. The window is short (microseconds typically) but it exists.

## Source

Flagged by Gemini Code Assist on PR #446 ([review comment](https://github.com/Basekick-Labs/arc/pull/446#discussion_r3284128432)). Pre-existing behaviour — `applyBatchFileOps` has had this property since it was introduced. Not introduced or made worse by the path-validation fix in PR #446, but worth fixing.

## Proposed fix

Refactor each `apply*FileStruct` to separate the locking from the state mutation:

1. Add `apply*FileLocked` variants that mutate `f.files` / `f.filesByDB` directly without taking `f.mu` — caller is responsible for holding the lock.
2. `apply*FileStruct` becomes a thin wrapper that acquires `f.mu` then calls `apply*FileLocked`.
3. `applyBatchFileOps` acquires `f.mu` ONCE for the whole apply loop, then calls `apply*FileLocked` for each op. Now the entire batch is atomic from the readers' perspective.
4. Callbacks (`onFileRegistered`, `onFileDeleted`) currently fire inside the lock-released window — preserve that property by deferring callback fires until after the batch lock is released. Buffer them in a slice during the apply loop, then iterate after `f.mu.Unlock()`.

## Why deferred from PR #446

PR #446 is a focused security fix for [GHSA-f85q-mvg8-qf37](https://github.com/Basekick-Labs/arc/security/advisories/GHSA-f85q-mvg8-qf37). The lock-isolation issue is real but pre-existing and the fix is non-trivial (touches every `apply*` method + callback fan-out logic). Better as its own PR with its own review surface.

## Testing strategy for the fix

A goroutine concurrent-reader race test: launch a reader on `GetFilesByDatabase` in a tight loop while `applyBatchFileOps` processes a batch that deletes 3 source files and registers 1 compacted file. Without the fix, the reader will observe at least one intermediate state (count != N before AND count != 1 after) over many iterations. With the fix, the reader sees either the pre-batch state (N files) or the post-batch state (1 file), never an intermediate.

## Severity

**Medium** per Gemini's classification. Not a security issue — correctness is eventually consistent, no data loss. But mid-batch query incorrectness is a real symptom for compaction-heavy workloads.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.