Skip to content

Commit 72ee2b0

Browse files
committed
config: drop the cross-process config.toml lock and filelock dependency
The filelock-based serialization of config.toml's read-modify-write was a recurring source of Windows CI flakiness, and the lost-update race it closed (two concurrent `assembly` processes clobbering each other's profile/telemetry writes) isn't worth that cost for a single-user CLI. Remove core/config_lock.py, core/locking.py, and the filelock dependency. Writes still go through a load -> mutate -> dump (now config._update) whose _dump does a temp-file + atomic os.replace, so a reader never sees a torn file; writers and readers are otherwise unsynchronized (last write wins). The Windows os.replace sharing-window retry (config._retry_on_sharing_violation) stays, since the lock-free read/replace race it guards is independent of the removed lock. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JrDRFsdAYyXwWSudM2da8g
1 parent e5c7519 commit 72ee2b0

7 files changed

Lines changed: 60 additions & 208 deletions

File tree

aai_cli/AGENTS.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ between layers is enforced — higher may import lower, never the reverse:
3434
`config_builder`, `keyring_store`, `environments`, `env`, `errors`, `llm`,
3535
`telemetry`, `debuglog`, `remotefs`, `sync_stt`, `signals`, `ws`, `youtube`,
3636
`wer`, `argscan`, `jsonshape`, `timeparse`, `microphone`, `procs`, `stdio`,
37-
`choices`, `locking`, `config_lock`. Contract 4 also forbids `rich` here, so
37+
`choices`. Contract 4 also forbids `rich` here, so
3838
"no Rich below the UI layer" is structural.
3939

4040
Three things sit *beside* the stack, intentionally unlisted in the layers
@@ -139,7 +139,7 @@ heavily-reworked commands with long bodies; small commands keep the inline
139139
### Cross-cutting state (resolution order matters)
140140

141141
- **`app/context.py`**`AppState` (profile, env) is attached to the Typer context in the root `@app.callback()`. `run_command` is the standard command wrapper.
142-
- **`core/config.py`** — profiles persisted in `config.toml` (via `platformdirs`); the **API key lives only in the OS keyring**, never in a dotfile. The keyring access itself is factored into **`core/keyring_store.py`** (the single importer of `keyring`, holding `KEYRING_SERVICE = "assemblyai-cli"` + `set_secret`/`get_secret`/`restore_secret`/`delete_secret`/`usable`), so the "secrets never touch the dotfile" split is structural; `config` reads/writes secrets through it and only `config.keyring_usable` re-surfaces the probe on the auth facade. Key resolution order: `--api-key` flag (validation paths only) → `ASSEMBLYAI_API_KEY` env → keyring. **Run commands deliberately expose no `--api-key` flag** so keys can't leak into `ps`/shell history. Every `config.toml` write is a read-modify-write (`_load` → mutate → `_dump`): `_dump` is a temp-file + atomic `os.replace` (a reader never sees a torn file), and the whole RMW runs under a cross-process `filelock` (`config_lock.update`/`.locked`, built on `core/locking.py`) so two concurrent `assembly` processes can't lose each other's updates. Readers stay lock-free. The lock helpers live in `config_lock.py` (not `config.py`) only to keep the latter under the file-length gate; reuse one cached `FileLock` per path so nested writers (`persist_login`) stay reentrant.
142+
- **`core/config.py`** — profiles persisted in `config.toml` (via `platformdirs`); the **API key lives only in the OS keyring**, never in a dotfile. The keyring access itself is factored into **`core/keyring_store.py`** (the single importer of `keyring`, holding `KEYRING_SERVICE = "assemblyai-cli"` + `set_secret`/`get_secret`/`restore_secret`/`delete_secret`/`usable`), so the "secrets never touch the dotfile" split is structural; `config` reads/writes secrets through it and only `config.keyring_usable` re-surfaces the probe on the auth facade. Key resolution order: `--api-key` flag (validation paths only) → `ASSEMBLYAI_API_KEY` env → keyring. **Run commands deliberately expose no `--api-key` flag** so keys can't leak into `ps`/shell history. Every `config.toml` write is a read-modify-write (`_load` → mutate → `_dump`) via the `config._update` context manager: `_dump` is a temp-file + atomic `os.replace`, so a reader never sees a torn file. Writers and readers are otherwise unsynchronized — last write wins (there is **no** cross-process lock; an earlier `filelock`-based serialization was removed because it was a recurring Windows CI flake and the lost-update race it closed isn't worth the cost for a single-user CLI). On Windows the atomic replace has no replace-over-open guarantee, so both the lock-free read and the `os.replace` ride out the transient `PermissionError` through `config._retry_on_sharing_violation` (a no-op on POSIX).
143143
- **`core/environments.py`** — a frozen `Environment` (api_base, streaming_host, llm_gateway_base, ams_base, stytch_*). `DEFAULT_ENV` is **`production`**; use `--sandbox` (or `--env sandbox000` / `AAI_ENV`) to target the sandbox. The active environment is a process-global set once at startup; precedence: `--env``AAI_ENV` → profile's stored env → default. A credential is only valid against the environment that minted it.
144144
- **`core/client.py`** — thin wrappers over the `assemblyai` SDK (`transcribe`, `list_transcripts`, `stream_audio`, etc.). It normalizes SDK exceptions: auth failures become a single clean `auth_failure()` `CLIError`; everything else becomes `APIError`. New SDK calls should follow this try/except shape.
145145
- **`core/errors.py`** — the `CLIError` hierarchy (each with `error_type` + `exit_code`). `ui/output.py` emits errors to **stderr**; stdout stays clean for pipelines. `--json` switches to machine-readable output; it is never auto-enabled — `output.resolve_json()` deliberately keeps human text the default even when piped or agent-run.

aai_cli/core/config.py

Lines changed: 53 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,14 @@
88
import time
99
import tomllib
1010
import uuid
11-
from collections.abc import Callable
11+
from collections.abc import Callable, Generator
1212
from pathlib import Path
1313

1414
import platformdirs
1515
import tomli_w
1616
from pydantic import BaseModel, ConfigDict, Field, ValidationError
1717

18-
from aai_cli.core import config_lock, debuglog, env, keyring_store
18+
from aai_cli.core import debuglog, env, keyring_store
1919
from aai_cli.core.errors import CLIError, NotAuthenticated
2020

2121
ENV_API_KEY = "ASSEMBLYAI_API_KEY"
@@ -193,6 +193,17 @@ def _dump(cfg: Config) -> None:
193193
raise
194194

195195

196+
@contextlib.contextmanager
197+
def _update() -> Generator[Config]:
198+
"""Run a config.toml read-modify-write: ``_load`` -> mutate the yielded config ->
199+
``_dump``. The dump runs on clean exit; an exception in the block propagates and
200+
skips it. The atomic os.replace in ``_dump`` keeps a reader from ever seeing a torn
201+
file (writers and readers are otherwise unsynchronized: last write wins)."""
202+
cfg = _load()
203+
yield cfg
204+
_dump(cfg)
205+
206+
196207
def get_active_profile() -> str:
197208
return _load().active_profile or DEFAULT_PROFILE
198209

@@ -210,7 +221,7 @@ def set_active_profile(name: str) -> None:
210221
with no hint why, so the typo is rejected here with the known names listed.
211222
"""
212223
validate_profile(name)
213-
with config_lock.update(_load, _dump) as cfg:
224+
with _update() as cfg:
214225
if name not in cfg.profiles:
215226
known = ", ".join(sorted(cfg.profiles)) or "none yet"
216227
raise CLIError(
@@ -225,7 +236,7 @@ def set_active_profile(name: str) -> None:
225236
def set_api_key(profile: str, api_key: str) -> None:
226237
validate_profile(profile)
227238
keyring_store.set_secret(profile, api_key)
228-
with config_lock.update(_load, _dump) as cfg:
239+
with _update() as cfg:
229240
cfg.profiles.setdefault(profile, Profile())
230241
if cfg.active_profile is None:
231242
cfg.active_profile = profile
@@ -255,7 +266,7 @@ def get_profile_env(profile: str) -> str | None:
255266
def set_profile_env(profile: str, env: str) -> None:
256267
"""Bind a backend environment to a profile so its key and hosts stay matched."""
257268
validate_profile(profile)
258-
with config_lock.update(_load, _dump) as cfg:
269+
with _update() as cfg:
259270
cfg.profiles.setdefault(profile, Profile()).env = env
260271

261272

@@ -268,7 +279,7 @@ def get_profile_email(profile: str) -> str | None:
268279
def set_profile_email(profile: str, email: str) -> None:
269280
"""Persist the login email for a profile (gates internal-environment access)."""
270281
validate_profile(profile)
271-
with config_lock.update(_load, _dump) as cfg:
282+
with _update() as cfg:
272283
cfg.profiles.setdefault(profile, Profile()).email = email
273284

274285

@@ -294,7 +305,7 @@ def set_session(profile: str, *, session_jwt: str, session_token: str, account_i
294305
_session_username(profile),
295306
StoredSession(jwt=session_jwt, token=session_token).model_dump_json(),
296307
)
297-
with config_lock.update(_load, _dump) as cfg:
308+
with _update() as cfg:
298309
cfg.profiles.setdefault(profile, Profile()).account_id = account_id
299310

300311

@@ -318,12 +329,11 @@ def get_account_id(profile: str) -> int | None:
318329

319330
def clear_session(profile: str) -> None:
320331
keyring_store.delete_secret(_session_username(profile))
321-
with config_lock.locked():
322-
cfg = _load()
323-
prof = cfg.profiles.get(profile)
324-
if prof and prof.account_id is not None:
325-
prof.account_id = None
326-
_dump(cfg)
332+
cfg = _load()
333+
prof = cfg.profiles.get(profile)
334+
if prof and prof.account_id is not None:
335+
prof.account_id = None
336+
_dump(cfg)
327337

328338

329339
def persist_login(
@@ -346,32 +356,29 @@ def persist_login(
346356
restored best-effort.
347357
"""
348358
validate_profile(profile)
349-
# Hold the write lock across the whole snapshot -> writes -> rollback so a concurrent
350-
# writer can't slip a change between the snapshot and a rollback dump. The set_*
351-
# helpers re-take the same (reentrant) lock, so the nesting is safe.
352-
with config_lock.locked():
353-
prior_api_key = keyring_store.get_secret(profile)
354-
prior_session = keyring_store.get_secret(_session_username(profile))
355-
prior_cfg = _load()
356-
done = False
357-
try:
358-
set_api_key(profile, api_key)
359-
set_profile_env(profile, env)
360-
set_session(
361-
profile,
362-
session_jwt=session_jwt,
363-
session_token=session_token,
364-
account_id=account_id,
365-
)
366-
# Within the same atomic rollback so the sandbox gate can't read stale identity.
367-
if email is not None:
368-
set_profile_email(profile, email)
369-
done = True
370-
finally:
371-
if not done:
372-
keyring_store.restore_secret(profile, prior_api_key)
373-
keyring_store.restore_secret(_session_username(profile), prior_session)
374-
_dump(prior_cfg)
359+
# Snapshot the prior state so a mid-sequence failure rolls back cleanly to it.
360+
prior_api_key = keyring_store.get_secret(profile)
361+
prior_session = keyring_store.get_secret(_session_username(profile))
362+
prior_cfg = _load()
363+
done = False
364+
try:
365+
set_api_key(profile, api_key)
366+
set_profile_env(profile, env)
367+
set_session(
368+
profile,
369+
session_jwt=session_jwt,
370+
session_token=session_token,
371+
account_id=account_id,
372+
)
373+
# Within the same atomic rollback so the sandbox gate can't read stale identity.
374+
if email is not None:
375+
set_profile_email(profile, email)
376+
done = True
377+
finally:
378+
if not done:
379+
keyring_store.restore_secret(profile, prior_api_key)
380+
keyring_store.restore_secret(_session_username(profile), prior_session)
381+
_dump(prior_cfg)
375382

376383

377384
def has_device_id() -> bool:
@@ -385,12 +392,11 @@ def get_device_id() -> str:
385392
"""A stable anonymous install id for telemetry: a random UUID minted locally on
386393
first use and persisted in config.toml. Carries nothing derivable from the
387394
machine or account."""
388-
with config_lock.locked():
389-
cfg = _load()
390-
if cfg.device_id is None:
391-
cfg.device_id = str(uuid.uuid4())
392-
_dump(cfg)
393-
return cfg.device_id
395+
cfg = _load()
396+
if cfg.device_id is None:
397+
cfg.device_id = str(uuid.uuid4())
398+
_dump(cfg)
399+
return cfg.device_id
394400

395401

396402
def get_telemetry_enabled() -> bool | None:
@@ -400,7 +406,7 @@ def get_telemetry_enabled() -> bool | None:
400406

401407

402408
def set_telemetry_enabled(*, enabled: bool) -> None:
403-
with config_lock.update(_load, _dump) as cfg:
409+
with _update() as cfg:
404410
cfg.telemetry_enabled = enabled
405411

406412

@@ -413,7 +419,7 @@ def get_update_cache() -> tuple[float | None, str | None]:
413419
def set_update_cache(*, last_check: float, latest_version: str | None) -> None:
414420
"""Persist the update-notifier cache. ``latest_version`` is None when the last
415421
fetch failed — the timestamp is still recorded so we don't re-spawn every run."""
416-
with config_lock.update(_load, _dump) as cfg:
422+
with _update() as cfg:
417423
cfg.update_last_check = last_check
418424
cfg.update_latest_version = latest_version
419425

aai_cli/core/config_lock.py

Lines changed: 0 additions & 51 deletions
This file was deleted.

aai_cli/core/locking.py

Lines changed: 0 additions & 43 deletions
This file was deleted.

pyproject.toml

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,6 @@ dependencies = [
6565
# lazily). Strips boilerplate down to the readable body; ships prebuilt wheels
6666
# (lxml included), so it adds no source-compile step to Homebrew bottling.
6767
"trafilatura>=2.1.0",
68-
# Cross-process advisory lock around config.toml's read-modify-write (config.py),
69-
# so two concurrent `assembly` processes can't lose each other's profile/telemetry
70-
# updates (last-writer-wins). Pure-Python, no compiled deps; already arrived
71-
# transitively via the dev toolchain, so the lock pins the same release.
72-
"filelock>=3.16.0",
7368
]
7469

7570
[project.urls]

tests/test_concurrency.py

Lines changed: 5 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,10 @@
33
Two real concurrency surfaces exist in the CLI, and neither was directly exercised:
44
55
1. ``core.config`` persists ``config.toml`` with a temp-file + atomic ``os.replace``
6-
(`config._dump`) so a reader never observes a truncated file, and serializes its
7-
read-modify-write under a cross-process ``filelock`` (`config._write_lock`) so two
8-
concurrent ``assembly`` processes can't lose each other's updates. These tests pin
9-
both: the at-rest atomicity under thread contention, and that distinct concurrent
10-
updates all survive (no last-writer-wins clobber).
6+
(`config._dump`) so a reader never observes a truncated file. Writers and readers are
7+
otherwise unsynchronized (last write wins), and on Windows the replace window is ridden
8+
out by a small retry (`config._retry_on_sharing_violation`). These tests pin the at-rest
9+
atomicity under thread contention and that retry helper.
1110
2. ``streaming.StreamSession.on_turn`` runs on the SDK reader thread, and the
1211
``--system-audio`` path drives two of those threads at once (`session._drive`). The
1312
turn write is serialized by ``_callback_lock`` so two sources can't interleave a
@@ -22,7 +21,7 @@
2221

2322
import pytest
2423

25-
from aai_cli.core import config, config_lock
24+
from aai_cli.core import config
2625

2726
# --- config.toml: the Windows os.replace sharing-window retry -----------------------
2827

@@ -112,58 +111,6 @@ def reader() -> None:
112111
assert sorted(p.name for p in tmp_config.iterdir()) == ["config.toml"] # no temp leftover
113112

114113

115-
def test_concurrent_writers_do_not_lose_distinct_updates(tmp_config):
116-
# The cross-process write lock makes the read-modify-write atomic, so many threads
117-
# each adding a DISTINCT profile through the public API all survive. Without the lock
118-
# the interleaved RMW would drop some (two writers _load the same config; the second
119-
# _dump clobbers the first's new profile) — this is the lost-update race the lock closes.
120-
workers = 16
121-
barrier = threading.Barrier(workers) # release all writers at once for max contention
122-
123-
def add(i: int) -> None:
124-
barrier.wait()
125-
config.set_profile_env(f"p{i:02d}", f"sandbox{i:03d}")
126-
127-
with ThreadPoolExecutor(max_workers=workers) as pool:
128-
for f in [pool.submit(add, i) for i in range(workers)]:
129-
f.result()
130-
131-
# Every concurrent update is present with its own value — none clobbered.
132-
assert config.list_profiles() == {f"p{i:02d}": f"sandbox{i:03d}" for i in range(workers)}
133-
134-
135-
def test_write_lock_targets_the_config_dir_lock_file(tmp_config):
136-
# The lock guards config.toml via a sibling lock file in the same dir.
137-
assert config_lock.write_lock().lock_file == str(tmp_config / "config.toml.lock")
138-
139-
140-
def test_write_lock_rebuilds_when_config_dir_changes(monkeypatch, tmp_path):
141-
# The instance is cached per path, but a changed config dir (the suite repoints it per
142-
# test) must yield a fresh lock pointed at the new dir — not the stale one.
143-
first = config_lock.write_lock()
144-
moved = tmp_path / "moved"
145-
moved.mkdir()
146-
monkeypatch.setattr(config, "config_dir", lambda: moved)
147-
second = config_lock.write_lock()
148-
assert second is not first
149-
assert second.lock_file == str(moved / "config.toml.lock")
150-
151-
152-
def test_update_holds_the_write_lock_during_the_dump(tmp_config, monkeypatch):
153-
# The mutate -> dump runs inside the lock: while _dump executes, the lock is held.
154-
# (Drop the `with locked()` and is_locked would be False here.)
155-
seen: dict[str, bool] = {}
156-
real_dump = config._dump
157-
158-
def spy_dump(cfg):
159-
seen["locked"] = config_lock.write_lock().is_locked
160-
return real_dump(cfg)
161-
162-
monkeypatch.setattr(config, "_dump", spy_dump)
163-
config.set_profile_env("default", "sandbox000")
164-
assert seen["locked"] is True
165-
166-
167114
# --- streaming: on_turn serialization under _callback_lock -------------------------
168115

169116

0 commit comments

Comments
 (0)