basecamp / basecamp/basecamp-sdk

Event feed: FileStore's per-path lock is keyed on casing, so one file can take two mutexes

Open
#761 0 comments 0 reactions 0 assignees View on GitHub
bug go
Dominant language
Go
Stars
49
Forks
12
Avg merge
20h 47m
Merged PRs (30d)
89

Description

Raised by Codex on #705 (`filestore.go`, the per-path mutex registry).

`FileStore` keys its mutex registry on `filepath.Abs(path)`, and `Abs`
preserves component casing. On a case-insensitive filesystem — macOS's default
APFS, and Windows — two stores constructed with paths differing only in casing
address **the same checkpoint file** but take **different mutexes**. Concurrent
`Save` calls then both read the old JSON, and both rename their independent
updates into place: one lineage is silently dropped, against the documented
per-path concurrency guarantee.

Naming the actor, because it decides whether this is worth fixing: there is no
adversary. It is a plain mistake — one path from an env var, another
hardcoded; a `~/Library` vs `~/library`; a config file edited by hand. The
consequence is a silently lost checkpoint lineage, which surfaces later as a
feed re-entering at a position it should have moved past.

### Why it wants a decision

Three shapes, and they are not equivalent:

- **Lowercase the key on case-insensitive platforms.** A `runtime.GOOS` check
(darwin, windows). Cheap, but wrong at the edges: macOS can be formatted
case-*sensitive*, and Linux can mount case-insensitive filesystems, so the
heuristic both over- and under-normalizes. Over-normalizing is the dangerous
direction — it would serialize two genuinely distinct files on a
case-sensitive macOS volume, which is merely slow, so the failure is benign.
- **Derive the key from OS-level file identity** (device + inode via
`os.SameFile`/`Stat`). Correct by construction, but the file need not exist
when the store is constructed, so it needs a create-or-stat at registration
and a fallback for the not-yet-created case.
- **Narrow the documented guarantee.** Say the per-path lock is per *exact*
path and that two stores addressing one file by different spellings are the
caller's problem. Free, and honest, but it moves a footgun into the docs
where nothing enforces it.

I lean toward the first for the reason its own edge case is benign, but it is a
published concurrency guarantee on a public type, so it wants a call rather
than a quiet heuristic.

A regression test wants care too: asserting the *lock identity* (a unit test on
the key derivation) is deterministic, whereas asserting the lost update is a
race and would be flaky.

Follows #606, #614, #645, #696; siblings #753, #758, #759.

Contributor guide

Open the contributing guide

Research direction

Start in filestore.go at the per-path mutex registry and read how FileStore derives its key from filepath.Abs(path). Compare the three proposed approaches and verify the documented per-path concurrency guarantee; completion should include a decided behavior and a deterministic unit test of lock identity rather than a flaky lost-update race test.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, distributed-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.