Skip to content

fix: Prevent concurrent write corruption in savings.jsonl#195

Merged
Pringled merged 7 commits into
mainfrom
fix-savings-bug
Jun 12, 2026
Merged

fix: Prevent concurrent write corruption in savings.jsonl#195
Pringled merged 7 commits into
mainfrom
fix-savings-bug

Conversation

@Pringled

@Pringled Pringled commented Jun 12, 2026

Copy link
Copy Markdown
Member

Multiple MCP server processes appending to savings.jsonl simultaneously caused a small amount of lines to contain interleaved JSON. Added a lock for unix, Windows can technically still write malformed lines but it won't break the CLI, just show a warning (if we want to do it for windows we need to add a dependency which I don't really want to do for this).

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/semble/stats.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@greptile-apps

greptile-apps Bot commented Jun 12, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

The change is safe to merge; it adds write serialisation for a non-critical stats file and degrades gracefully to an unlocked write on Windows.

The core locking logic is correct and the lock is properly released via file-close. The two issues — an intra-function import and flock OSErrors being absorbed by the outer handler — are quality improvements rather than correctness blockers for a low-stakes stats file.

src/semble/stats.py — the flock error-handling path and module-level import placement are worth a second look.

Comments Outside Diff (1)

  1. src/semble/stats.py, line 1-6 (link)

    P2 import fcntl is placed inside the with block, re-running on every call to save_search_stats. Python's import system caches it, so it isn't expensive, but the idiomatic pattern is a module-level conditional import so the availability check happens once at startup and the rest of the function stays clean.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "Add no cover" | Re-trigger Greptile

Comment thread src/semble/stats.py
@Pringled Pringled requested a review from stephantul June 12, 2026 06:47

@stephantul stephantul left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments, not sure how easy it is.

Comment thread src/semble/stats.py Outdated
stats_file.parent.mkdir(parents=True, exist_ok=True)
with stats_file.open("a") as f:
try:
import fcntl

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue I can see here is that this import is always attempted (and always fails) on Windows and systems that do not support it.

Maybe move it somewhere else or cache the result of the import operation? I honestly don't know how long a failed import takes, but this is tried on every save.

Comment thread src/semble/stats.py
fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB)
except ImportError: # pragma: no cover
pass # Windows has no fcntl, write unlocked
except OSError: # pragma: no cover

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way to distinguish between these failure cases? I.e., just not being supported or a lock contention? The former should not be retried.

@Pringled Pringled merged commit e3f986b into main Jun 12, 2026
16 checks passed
@Pringled Pringled deleted the fix-savings-bug branch June 12, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants