basecamp / basecamp/basecamp-sdk
Event feed: FileStore.Save is atomic but not durable — no fsync of the temp file or the parent directory
- Dominant language
- Go
- Stars
- 49
- Forks
- 12
- Avg merge
- 20h 47m
- Merged PRs (30d)
- 89
Description
Raised by Copilot on #705, in a **suppressed** block — it never became a review
thread and never moved the unresolved count, which is worth noting on its own.
`FileStore.Save` writes a temp file, closes it, and renames it into place, then
reports success. Neither the temp file's contents nor the parent directory's
rename is synced, so a host crash or power loss can lose a checkpoint the
connector already reported as durably saved. `CheckpointStore.Save`'s contract
is durability; write-then-rename gives atomicity, which is a different
property.
The standard remedy is `f.Sync()` before close and an `fsync` on the parent
directory after the rename, with the usual platform caveats (directory fsync is
a no-op or an error on Windows, and `O_DIRECTORY` handling differs).
### Severity, and why it is filed rather than fixed in #705
The consequence is **bounded and self-correcting**, which is the line I have
been holding for what gets fixed in the PR versus filed. A lost checkpoint
means the next run re-enters at an OLDER position, so the feed replays rather
than skips — and the dedupe LRU suppresses the overlap it can. Duplicate
delivery is the safe direction; the dangerous direction is a *skip*, and this
cannot cause one.
Contrast with the found-but-empty position case Copilot raised in the same
round, which I fixed in #705 (`d379f2e11`): that one silently skipped history,
so it was inside the bar. This one is not.
### Worth deciding, not just doing
- **Cost.** An fsync per checkpoint save on the poll lane's per-page cadence is
a real write-amplification change for a connector that saves on every page.
If the durability contract is genuinely wanted, the option of an explicitly
relaxed mode (or documenting the store as atomic-but-not-durable) should be
weighed rather than assumed away.
- **Contract wording.** `CheckpointStore.Save`'s doc should say which of
atomicity and durability it promises, since the built-in store is what every
consumer will read as the reference implementation. That part binds all six
SDKs.
Follows #606, #614, #645, #696; siblings #753, #758, #759, #760, #761, #762,
#763.
Contributor guide
Research direction
Read FileStore.Save and the CheckpointStore.Save contract first, then review the durability and atomicity distinction described here. Decide whether the contract requires durable saves, including the per-page fsync cost and Windows directory-sync caveats; done means the contract and implementation behavior are aligned, or the relaxed behavior is explicitly documented.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100