Basekick-Labs / Basekick-Labs/arc
applyBatchFileOps releases FSM lock between ops — concurrent readers can see partial batch state
- 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
Assessment
This issue has not been assessed yet.