Skip to content

fix(bench): write config.json atomically to prevent silent 404s#559

Open
regdocs wants to merge 1 commit into
developfrom
fix/atomic-bench-config-write
Open

fix(bench): write config.json atomically to prevent silent 404s#559
regdocs wants to merge 1 commit into
developfrom
fix/atomic-bench-config-write

Conversation

@regdocs

@regdocs regdocs commented Jul 8, 2026

Copy link
Copy Markdown
Member

Problem

A bench that clearly exists on disk was returning 404 for every request:

agent.exceptions.BenchNotExistsException: Bench bench-9052-000094-f16j does not exist
  File ".../agent/bench.py", line 61, in __init__
    self.docker_image = self.bench_config.get("docker_image")
  ...
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

On the server, config.json was 0 bytes while a valid config.json.bak sat right next to it.

Root cause

set_bench_config claimed to avoid partial writes by writing to a temp file first, but its final step used shutil.copy2(temp, config.json). copy2 opens the destination with open(dst, "wb"), which truncates config.json to 0 bytes and then streams into it. If the process dies in that window (OOM kill, worker/supervisor restart, deploy), you're left with a 0-byte config.json + a valid .bak.

Bench.__init__ then raises on json.load (at bench.py:61, before the existence guard), and Server.benches swallowed the exception via suppress(Exception) — so the bench silently disappeared from the dict and get_bench raised BenchNotExistsException404, with no hint of the real cause.

The temp file lived in /tmp (a different filesystem), which is why copy2 was used instead of an atomic os.replace — the latter fails with EXDEV across filesystems.

Fix

  • set_bench_config: write the temp file in the bench directory via tempfile.mkstemp(dir=self.directory), fsync, then os.replace onto config.json — an atomic same-filesystem rename with no truncation window. Removes the copy2/.bak dance.
  • bench_init: the initial config.json write for a new bench used open("w") (truncates immediately) — made atomic the same way, so an interrupted New Bench job can't leave a 0-byte stub.
  • Server.benches: replace suppress(Exception) with a try/except that logger.exceptions the skipped bench, so a corrupt config is diagnosable from logs instead of an opaque 404.

Testing

  • Both files compile.
  • Smoke-tested the atomic-write logic standalone: correct content, clean overwrite, no zero-byte window, no leftover temp files.

Recovering affected benches

For any bench already in this state, the good config is in the backup:

cd /home/frappe/benches/<bench-name>
install -o frappe -g frappe -m 600 config.json.bak config.json

set_bench_config wrote its temp file to /tmp and finished with
shutil.copy2(), which truncates config.json before streaming into it. A
crash in that window left a 0-byte config.json (plus a valid .bak), so
Bench.__init__ raised on json.load, Server.benches silently dropped the
bench, and every request 404'd with BenchNotExistsException despite the
bench existing on disk.

- set_bench_config: temp file via mkstemp(dir=self.directory) + fsync +
  os.replace (atomic same-filesystem rename); drops the copy2/.bak dance.
- bench_init: initial config.json write made atomic the same way so an
  interrupted New Bench job can't leave a 0-byte stub.
- Server.benches: log the skipped bench via logger.exception instead of
  suppress(Exception), so the real cause is diagnosable, not an opaque 404.
@mergify

mergify Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@greptile-apps

greptile-apps Bot commented Jul 8, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

The atomic-write fix is correct and removes the truncation window; the only rough edge is log verbosity for broken benches.

The core changes are solid. The logger.exception call inside an uncached property is the one thing worth a second look in production — a single bad bench directory will generate a log line on every property access across all callers.

agent/server.py — specifically the benches property and its many call sites.

Reviews (1): Last reviewed commit: "fix(bench): write config.json atomically..." | Re-trigger Greptile

Comment thread agent/server.py
Comment on lines +863 to +870
try:
benches[directory] = Bench(directory, self)
except Exception:
# A bench that fails to construct (e.g. missing/corrupt
# config.json) is skipped, which surfaces downstream as an
# opaque BenchNotExistsException -> 404. Log it so the real
# cause is diagnosable instead of silently swallowed.
logger.exception("Skipping bench %s: failed to load", directory)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Exception logged on every benches access for a broken bench

self.benches is an uncached @property called from ~15 sites (including hot paths like dump() and get_bench). A permanently corrupt config.json will fire logger.exception on every one of those accesses, flooding the log until the bench is manually repaired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant