basecamp / basecamp/basecamp-sdk

Event feed: FileStore.Save is atomic but not durable — no fsync of the temp file or the parent directory

Open
#764 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 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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.