[patrickelectric]

Port of #4180 to 1.4-dev.

1.4-dev still runs uvicorn.run directly, so the shutdown flush wraps it instead of the async main() from master.

[github-actions]

Automated PR Review

0. Summary

• Verdict: MINOR SUGGESTIONS ✏️

Backport of #4180 to 1.4-dev: keeps the bag_of_holding JSON database resident in memory, writes are debounced via asyncio.get_running_loop().call_later(FLUSH_INTERVAL, _flush) and persisted atomically through a .tmp + Path.replace() dance. Since 1.4-dev still boots via a synchronous uvicorn.run(...), the shutdown flush is wrapped in a try/finally around that call rather than around await server.serve() as on master.

1. Correctness & Implementation Bugs

• 1.1 [minor] core/services/bag_of_holding/main.py:66-72 — _flush()'s exception branch reschedules another retry every FLUSH_INTERVAL with no cap and no backoff. On a persistent failure (disk full, read-only filesystem, bad path) the service will log logger.exception("Failed to persist database") every second indefinitely, flooding the journal. Consider either an exponential backoff with a maximum interval, or stopping after N failures and only re-arming on the next mark_dirty().
• 1.2 [minor] core/services/bag_of_holding/main.py:46-53 — write_db() no longer round-trips json.dumps / json.loads to validate before touching disk (as the previous implementation did). With the atomic .tmp + replace pattern a serialization failure now leaves the stale db.json intact (good) but also silently drops the writer's data in the background flush (bad: user sees HTTP 200 on /set/... and the value is quietly lost on the next failed flush). Not critical since FastAPI's pydantic Body parsing already rejects non-JSON input, but worth noting.

4. Performance

• 4.1 [minor] core/services/bag_of_holding/main.py:46-53, 66-79 — write_db() performs blocking I/O (json.dump plus os.fsync) directly on the event loop: _flush() is a call_later callback and flush_now() is reachable from the /overwrite async handler. For the current db sizes this is sub-millisecond and almost certainly fine, but per AGENTS.md ("Common Pitfalls > Backend > Using blocking I/O") the pattern you'd normally want in this codebase is await asyncio.to_thread(write_db) (and a sync fallback in the shutdown finally). Flagging for future-proofing — if the bag grows into tens of MB, os.fsync() will stall every incoming request on the loop for the duration of the flush.

6. Code Quality & Style

• 6.1 [nit] core/services/bag_of_holding/main.py:57 — _pending = SimpleNamespace(handle=None) is used purely to sidestep a global _pending_handle declaration in mark_dirty/_flush/flush_now. A module-level _pending_flush: asyncio.TimerHandle | None = None with explicit global reads clearer and types better than an untyped SimpleNamespace attribute.
• 6.2 [nit] core/services/bag_of_holding/main.py:24 — FLUSH_INTERVAL = 1.0 is unitless at the call site; FLUSH_INTERVAL_SECONDS = 1.0 matches the convention used elsewhere in the repo (e.g. DISK_USAGE_POLL_INTERVAL_S-style naming) and removes the need for a reader to check call_later's docs.

7. Tests

• 7.1 [minor] No tests accompany the behavioral change. The port deviates from master's shutdown path (sync try/finally around uvicorn.run vs. await server.serve()), which is exactly the kind of divergence a regression test would catch — a pytest covering mark_dirty -> flush_now -> file-contents-match-memory, plus one for _flush retry-on-error, would make the backport self-verifying.

Generated by PR Review Bot. This is advisory, a human reviewer must still approve.