fix(channels): drain the .env write before releasing the config lock

[leonlaiyc]

Rescoped on 2026-08-24. The defect this PR originally opened against — three of six channel saves calling _write_env_updates inline on the gateway loop — was fixed upstream by #5269 and siblings. That half is dropped, not re-landed: main already offloads all six. What remains is the part the offload did not bring with it, described below.

Problem / Motivation

Every channel token save runs its .env write inside async with _get_config_lock(), and on current main all six reach it as a bare offload:

async with _get_config_lock():
    ...
    await asyncio.to_thread(_write_env_updates, env_updates)

A thread cannot be cancelled. When the request is cancelled — a client disconnecting mid-save, a gateway shutdown — the await raises CancelledError and the async with unwinds while the worker is still rewriting .env. The lock is released; the worker is not.

The next channel save then enters the critical section against a file that is still being replaced, and writes it back from lines it read before the first write landed. Whichever credential the cancelled save was persisting is discarded. Both saves answer 200.

All six call sites on main have this shape: _slack_config_save_locked, _discord_config_save_locked, _telegram_config_save_locked, api_teams_config_save, api_webex_config_save, _wecom_config_save_locked.

Why it matters

The lost write is a credential, and the loss is silent — the save that gets clobbered has already returned success to the user, so the dashboard shows the token as installed while .env holds the other one. Recovery requires noticing that a channel stopped authenticating and re-entering a token by hand.

Cancellation here is ordinary, not exotic: an aiohttp client disconnect during a save that is validating a token against a remote API (which is what makes the save slow enough to overlap another) is enough.

What changed (motivation → approach → change)

Motivation — keep the lock's guarantee intact across the hop the offload introduced.

Approach — do not widen the lock or make the write cancellable; put the lock release after the worker. Shield the future so the cancellation does not propagate into it, drain it, then re-raise. This is the same shape run_config_write uses in dashboard/chat_utils.py, so the two config-write paths behave alike.

Change — one helper, _write_env_off_loop, and all six call sites route through it:

async def _write_env_off_loop(updates: dict[str, str | None]) -> None:
    fut = asyncio.ensure_future(asyncio.to_thread(_write_env_updates, updates))
    try:
        await asyncio.shield(fut)
    except asyncio.CancelledError:
        await asyncio.wait([fut])
        raise

Draining cannot change whether the write happens — the thread runs to completion either way — so the only thing it decides is whether the lock outlives it. The CancelledError is re-raised, never swallowed.

All six, not a subset. The bare offload is the hole, so the three sites that were already offloading before this PR carry it too. Covering only some would leave the same window open in the rest and re-create a per-site split.

The caller contract is now stated on _write_env_updates itself, so the next channel inherits the reason and not just the shape.

Tests

test/test_channel_env_write_off_loop.py (new) pins two distinct properties:

1. Cancellation drain — test_cancelling_a_save_drains_the_env_write_before_releasing_the_lock. Ordering is forced with events, never slept for: the worker parks inside the write, the caller is cancelled while it is parked, a second writer then tries to take the lock, and must not get through until the first worker finishes.

   Fail-before, verified on this same tree by replacing the helper body with a bare await asyncio.to_thread(...):

   FAILED test_cancelling_a_save_drains_the_env_write_before_releasing_the_lock
     AssertionError: the config lock was handed to the next channel save while
     the cancelled one's worker was still rewriting .env
   1 failed, 5 passed
   

   Restored: 6 passed.

2. Off-loop guard — four parametrised cases (slack, discord, webex, telegram) drive each save over a real HTTP client and assert on the thread the real _write_env_updates executed on. These pass on main as it stands and are a guard, not a fail-before: they exist so the next channel cannot reintroduce an inline write, plus a meta-test that fails if a channel is quietly dropped from the table.

Gates run locally on the rebased head: flake8 · isort --check-only · scripts/check_black_formatting.py (2 files in scope, passed) · scripts/check_loop_bound_locks.py (passed).

Manual verification

N/A — unit coverage sufficient: the regression turns on the interleaving of a cancellation and a worker thread, which the event-forced ordering reproduces deterministically and a hand test cannot.

Related Issues

#5065 reported the inline-write half. That half is already fixed on main by #5269 and siblings, so this PR does not claim to close it — the issue can be closed independently of this change.

Same family as #4118 / #3803 / #4550, and the run_config_write drain in #5059.

Checklist

• Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — N/A: no documented behaviour changes. The caller contract is stated on _write_env_updates itself.
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

🤖 Generated with Claude Code

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed c532a044f8fa93e1abd6596c4aa1130c765d7003 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c532a04

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed c532a044f8fa93e1abd6596c4aa1130c765d7003 via the fork AI-review pipeline; updated in place on each push.

Review details

I've reviewed the diff, the _write_env_off_loop helper, all six converted call sites, and the drain/cancel semantics against the candidate.

Candidate 1 claims the cancel-drain path (await asyncio.wait([fut]) then raise) leaves the worker's exception unretrieved. The mechanism is technically accurate — asyncio.wait does not consume a Task's exception, and the normal path retrieves it only via shield. But falsifying it:

• (a) requires a conjunction of two independent, rare conditions: the request being cancelled precisely during the write window AND _write_env_updates raising. The write's atomic_write(..., restrict_on_error="warn") deliberately does not raise on the common failure (lockdown), so only a genuine disk/IO error qualifies — an "if the write were to fail" condition.
• (c) the observable outcome is a single benign "Task exception was never retrieved" log line on an already-cancelled request where no user is present. That is cosmetic log noise, not a wrong outcome affecting correctness, security, or data.

The candidate's own author scored it "low." Under the 80+ bar for a real defect it does not survive: the trigger is a compound rare event and the consequence is benign. Dropped.

No new grounded findings emerge from the diff — the shield/drain/re-raise idiom is the correct pattern, all six sites hold the lock across the offloaded write, and the normal path propagates exceptions correctly.

No findings.

[OPUS-REVIEWED] c532a04

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of c532a044f8fa93e1abd6596c4aa1130c765d7003 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

The fix closes the cancel-under-lock hole for .env only; the identical hole sits one line above on every channel's config.json write.

Watch

• The root cause is "cancellable await wrapping a non-cancellable worker inside _get_config_lock()", not ".env specifically". Each fixed handler also does await asyncio.to_thread(_atomic_json_write, path, data) under the same lock (messaging.py:3342, 3682, 5036), and config.json is read-modify-written under that lock too (_read_config at 3558/4950) — so a cancelled save can still release the lock mid-config.json-write and let the next writer persist a snapshot read before the rename lands, dropping staged channel settings by exactly the mechanism this PR's docstring describes for .env. Shipping the helper scoped to _write_env_updates fixes one instance of the class and leaves the pattern to drift back per-site.

Suggestions

• Generalize to one _drained_to_thread(fn, *args) (or wrap the whole locked critical section in a shield-and-drain) and route _atomic_json_write through it too — same mechanism, one helper, closes the class instead of the instance.

[DESIGN-REVIEWED] c532a04

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of c532a044f8fa93e1abd6596c4aa1130c765d7003 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

First-Principles-Verdict: CONCERNS

The drain is derived and earns its place, but the named cause — a bare offload under _get_config_lock() — has four unfixed sibling writers, one holding the same credential.

What this change ships

Intent: stop a cancelled channel-token save from silently losing a credential by releasing the config lock while the .env write is still running — a FIX.

1. A cancelled save now holds the config lock until the .env worker finishes, all six channels — justified
2. New internal helper _write_env_off_loop all six saves route through — justified
3. Caller contract added to _write_env_updates docstring — declared, rides along
4. New cancellation-drain test with fail-before evidence — justified
5. Off-loop guard tests (4 channels) plus a meta-test on the table — declared; coverage overstated in code

Watch

• Point patch on a counted cause. Grepped asyncio.to_thread under async with _get_config_lock(): 4 writer sites keep the bare offload — weixin_qr.py:336 (writes the weixin token to the same .env via _write_env_secret, the identical credential-clobber), whatsapp_setup.py:132, chat_utils.py:88 (run_config_write, ~26 callers), server.py:3354. The PR's own premise — "covering a subset would leave the same window open in the rest" — indicts its own boundary; weixin is in scope of the stated harm. Accepted-and-deferred at minimum, but say so.
• Stale precedent claim. "the same shape run_config_write uses in dashboard/chat_utils.py" — on this base, chat_utils.py:87-88 is a bare await asyncio.to_thread(...), no shield/drain (fix(memory): run the config read-modify-write off the gateway loop #5059 evidently unmerged).
• Docstring overstates the test. _write_env_updates now says the test "pins both properties for all six channels"; the table drives four (teams, wecom absent).

Subtractions

• Drop test_the_family_is_covered_here — PINNED and CHANNELS sit three lines apart in one file; the meta-test only fires if an edit removes a row from exactly one of them.

[FIRST-PRINCIPLES-REVIEWED] c532a04