Skip to content

Commit 33f24b3

Browse files
aapelivclaude
andcommitted
Backend/experimentation: simplify atomic cache write, drop needless lock
Use pathlib + NamedTemporaryFile for the atomic cache write instead of the manual mkstemp/fdopen/replace dance, and remove the unnecessary lock around _last_fetch_time (a single reference read/write is atomic under the GIL). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 6f677e6 commit 33f24b3

1 file changed

Lines changed: 20 additions & 28 deletions

File tree

app/backend/src/couchers/experimentation.py

Lines changed: 20 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,11 @@
1818

1919
import json
2020
import logging
21-
import os
22-
import tempfile
2321
import threading
2422
import time
2523
from collections.abc import Callable
24+
from pathlib import Path
25+
from tempfile import NamedTemporaryFile
2626
from typing import Any
2727

2828
import urllib3
@@ -86,15 +86,13 @@ def _apply_response(response: dict[str, Any]) -> None:
8686

8787
def _set_last_fetch_time(when: float) -> None:
8888
global _last_fetch_time
89-
with _state_lock:
90-
_last_fetch_time = when
89+
_last_fetch_time = when
9190

9291

9392
def seconds_since_last_fetch() -> float | None:
9493
"""Seconds since features were last successfully pulled from GrowthBook, or None if never pulled
9594
(e.g. experimentation disabled). Drives the staleness metric so a stalled refresh is observable."""
96-
with _state_lock:
97-
when = _last_fetch_time
95+
when = _last_fetch_time
9896
if when is None:
9997
return None
10098
return max(0.0, time.time() - when)
@@ -103,36 +101,30 @@ def seconds_since_last_fetch() -> float | None:
103101
def _write_cache(response: dict[str, Any]) -> None:
104102
"""Persist a freshly fetched payload to disk for use as a cold-start fallback.
105103
106-
Written atomically (temp file + os.replace) so a concurrent reader never sees a partial file, and
107-
so concurrent writers from the api/worker/scheduler processes just resolve to a last-writer-wins of
108-
identical content. Failures are not swallowed: a disk problem here is real and must surface.
104+
Written to a temp file in the same directory and then renamed (Path.replace is atomic), so a
105+
concurrent reader never sees a partial file and concurrent writers from the api/worker/scheduler
106+
processes resolve to a last-writer-wins of identical content. Failures are not swallowed: a disk
107+
problem here is real and must surface.
109108
"""
110-
path = config["GROWTHBOOK_CACHE_PATH"]
111-
data = json.dumps({"fetched_at": time.time(), "response": response}).encode("utf-8")
112-
directory = os.path.dirname(os.path.abspath(path))
113-
fd, tmp_path = tempfile.mkstemp(dir=directory, prefix=".growthbook-cache-", suffix=".json.tmp")
114-
try:
115-
with os.fdopen(fd, "wb") as f:
116-
f.write(data)
117-
os.replace(tmp_path, path)
118-
except BaseException:
119-
# Clean up the temp file, but never mask the underlying write error.
120-
try:
121-
os.unlink(tmp_path)
122-
except FileNotFoundError:
123-
pass
124-
raise
109+
path = Path(config["GROWTHBOOK_CACHE_PATH"])
110+
data = json.dumps({"fetched_at": time.time(), "response": response})
111+
# Write to a temp file in the same directory, then rename over the target: rename is atomic only
112+
# within a filesystem, so the temp must live next to the target, and a reader either sees the whole
113+
# old file or the whole new one - never a half-written cache that would fail our strict parse.
114+
with NamedTemporaryFile("w", dir=path.parent, prefix=".growthbook-cache-", suffix=".tmp", delete=False) as f:
115+
f.write(data)
116+
tmp = Path(f.name)
117+
tmp.replace(path)
125118

126119

127120
def _read_cache() -> tuple[dict[str, Any], float] | None:
128121
"""Load the persisted payload as (response, fetched_at). Returns None only when no cache file
129122
exists yet (e.g. the very first deploy). A file that exists but can't be parsed raises - a corrupt
130123
cache is a hard failure, not something to paper over by falling through to in-code defaults."""
131-
path = config["GROWTHBOOK_CACHE_PATH"]
132-
if not os.path.exists(path):
124+
path = Path(config["GROWTHBOOK_CACHE_PATH"])
125+
if not path.exists():
133126
return None
134-
with open(path, "rb") as f:
135-
payload = json.loads(f.read().decode("utf-8"))
127+
payload = json.loads(path.read_text())
136128
return payload["response"], payload["fetched_at"]
137129

138130

0 commit comments

Comments
 (0)