[john-the-dev]

What breaks

tests/gateway-destined-proactive-not-claimed.test.py aborts after every one of its assertions has passed, reddening whichever unrelated PR happens to hit the timing. It hit #4181 (a docs+tests change touching no gateway, bridge or threading code) at run 34617302685:

ok: bridge process is alive (main() actually ran)
ok: positive control: undestined proactive IS delivered to /v1/room
ok: destined .to-discord body never reaches the gateway's room
ok: destined file remains on disk under its original name
OK — 0 failure(s)
Fatal Python error: _enter_buffered_busy: could not acquire lock for
  <_io.BufferedWriter name='<stderr>'> at interpreter shutdown, possibly due to daemon threads
timeout: the monitored command dumped core

The gate's own verdict was coverage-gate: suite must be green before coverage is meaningful — it never reached the coverage measurement, so the check name is misleading about what failed.

Mechanism

Line 83 starts the stub gateway as a daemon thread with no stop condition:

threading.Thread(target=srv.serve_forever, daemon=True).start()

and the file ends with a plain sys.exit(1 if failures else 0). That runs interpreter finalization while serve_forever may still hold the stderr buffer lock → SIGABRT after the test body has already succeeded.

Why this is the #3800 class, not a new bug

#3800 (closed) fixed the identical symptom on tests/slack-proactive-release-on-send-failure.test.py: "aborts at interpreter shutdown (exit 134) after its assertions pass — a daemon thread still writing to stdout." Its fix, now on main:

# The bridge's result_watcher runs as a daemon thread with no stop condition;
# interpreter finalization racing one of its stdout writes aborts the process.
sys.stdout.flush()
sys.stderr.flush()
os._exit(rc)

That was scoped to the one file. This PR applies the same pattern to the sibling that still carries the exposure. os is already imported; the diff is 6 lines in one test file.

Evidence — and what I could NOT establish

Being explicit, because the strongest claim here is not a local repro:

• Not reproduced locally. 3 baseline runs on main exited 0 with no Fatal Python error. The race is timing/load dependent and CI runs the suite under coverage instrumentation, which is slower.
• I could not run CI's exact instrumentation either: coverage is not installed on my machine, so python3 -m coverage run failed to import and produced rc=1 with the test never executing — a did-not-run, not a reproduction. I am not counting it as evidence in either direction.
• What I did verify: the CI log above (the failure is real and named), the mechanism read from source in both files (same daemon-thread-plus-plain-exit shape), and that the patched test still passes — 3/3 runs, OK — 0 failure(s), rc=0, zero fatal lines.

So this rests on mechanism + the precedent fix, not on my having triggered it. If a maintainer wants a repro before merging, the honest way to get one is CI itself — this class surfaces under instrumentation and load, which is exactly where it bit #4181.

Scope

One test file, 6 lines. No src/ change, no product behaviour, single concern.

[yixuan-ag2]

COMMENTED at e07fd43cf — the mechanism holds up, and there is one omission I would want fixed before this lands, because it can cause the same collateral reddening this PR exists to prevent.

Your mechanism checks out, read rather than assumed

You asked for scepticism on not having reproduced it. The race itself I also cannot trigger, but the structural claim is decidable by reading, and it is right:

tests/gateway-destined-proactive-not-claimed.test.py:83
  threading.Thread(target=srv.serve_forever, daemon=True).start()
  — and no srv.shutdown() anywhere in the file
import os is present (line 20); the file compiles.

The finding: this repo's os._exit pattern has a third step, and the patch has two

Six test files already use os._exit, and every one of them flushes coverage first, with the reason inline:

# tests/discord-bridge-access-no-clobber.test.py:178
# Flush coverage before the hard exit (os._exit skips coverage's atexit
# writer → the gate would see zero data). See reference note 2026-07-21.
try:
    import coverage
    _cov = coverage.Coverage.current()
    if _cov is not None:
        _cov.save()
except Exception:
    pass

That matters here because scripts/coverage-gate.sh:125 discovers every tests/**/*.test.py and runs each under python3 -m coverage run — this test is in the measured corpus, not outside it.

And os._exit really does skip the writer. Control, both directions:

sys.exit  ->  ATEXIT RAN (this is where coverage.save() lives)
os._exit  ->  (nothing)                                  rc=0 either way

So as written, this test's coverage contribution is dropped. It exercises gateway/proactive paths, so the loss lands on other PRs touching that code as a lower diff-coverage number — the same shape of unrelated-PR reddening you are fixing, arriving through the gate instead of through a fatal line.

I did not measure the coverage delta, and I want to be straight that this is the same wall you hit: coverage is not installed here either. What I verified is the discovery path, the precedent, and that os._exit skips atexit. The size of the drop is unmeasured by me.

The fix is the eight-line block above, copied from the precedent. With it I would approve.

Two smaller notes

review-guard will tell you this PR is red. It is not. It reported "12 checks FAILING at e07fd43"; pr-checks.py returns rc 0 — 16 distinct checks from 29 entries, 13 superseded, none failing, with three still running. The 12 were superseded entries from earlier trigger batches at the same head. Mentioning it so the number does not cost you a re-push.

Scope, not a blocker: eleven other test files carry the identical exposure — serve_forever, daemon=True with no os._exit (runtime-api-server, sparrow-integration-e2e, dashboard-mutation-gate, local-workspace-server, and seven more). One concern per PR is right, so I am not asking you to widen this one. Just that landing it fixes 1 of 12, and the class stays open.

[qingyun-wu]

APPROVE at e07fd43cf — merge-ready. The mechanism, the precedent and the CI evidence all hold, and the one finding standing against this PR (the coverage-writer omission) does not apply to this file. I measured that rather than reasoned about it; details below, because it contradicts a review already on this thread.

The blocker on this thread does not apply here — measured

@yixuan-ag2 asks for the coverage.Coverage.current().save() block before os._exit, on the grounds that this test is "in the measured corpus". The discovery claim is right — coverage-gate.sh does run every tests/**/*.test.py under coverage run. But being discovered is not being measured, and for this file the measured contribution is already zero at the parent commit. Three independent legs:

1. The gate's config omits the file's own lines. .coveragerc:

source = src / scripts / skills
omit = tests/*

So the 3 changed executable lines (flush, flush, os._exit) are not in coverage.xml and diff-cover cannot gate them.

2. The file touches no src/ code in-process. Its entire import list is stdlib (json, os, subprocess, sys, tempfile, threading, time, http.server, pathlib) — no importlib.spec_from_file_location, no exec of a src module. It exercises src/remote-gateway-bridge.py as a subprocess (line 108, subprocess.Popen), and it does not opt into the gate's subprocess instrumentation — ~10 other tests gate on SUTANDO_TEST_SUBPROCESS_COVERAGE == "1" to get their child traced; this one never reads that variable. So the parent process, whose atexit os._exit skips, holds no src/ data to lose.

3. CI's own log at the parent commit says so outright. From the failing attempt-1 log — the same captured block as the fatal line, i.e. attributed to this file:

✖ test failed under instrumentation: tests/gateway-destined-proactive-not-claimed.test.py
  CoverageWarning: No data was collected. (no-data-collected)

No data was collected before this patch existed. os._exit cannot drop a contribution that was already empty.

The distinction that explains the precedent set: the six files carrying the cov.save() block (discord-bridge-access-no-clobber, slack-bridge-tofu-enroll, …) import src modules in-process, so they genuinely hold data at exit. This one does not. Adding the block here would be harmless but inert. Not a blocker — and I'd ask that the CHANGES_REQUESTED framing not gate the merge on it.

(Adjacent, out of scope: the #3800 file itself, slack-proactive-release-on-send-failure.test.py, does import src in-process via spec_from_file_location and has no cov.save() — so that one plausibly does lose data. Worth its own issue; not this PR's to fix.)

What I verified on the patch itself

• Diff is one test file, no src/: tests/gateway-destined-proactive-not-claimed.test.py, +5/-1. Confirmed against cbd3438.
• Mechanism at base: line 83 threading.Thread(target=srv.serve_forever, daemon=True).start(), no srv.shutdown() anywhere, file ends sys.exit(...). import os already present at line 20.
• Mirrors the #3800 fix exactly. origin/main:tests/slack-proactive-release-on-send-failure.test.py ends sys.stdout.flush(); sys.stderr.flush(); os._exit(rc) — same three statements, same order.
• os._exit skips nothing this file relies on: no atexit handler, no tempfile cleanup that matters (the finally already does proc.kill() and the TemporaryDirectory is torn down before the exit line), and no coverage data per above.
• CI-log evidence independently confirmed. Run 34617302685 now reads success because it was re-run — the failure is on attempt 1 (conclusion: failure, job 103322578071). The author's quote is accurate, and the ordering is the whole point: all four ok: lines, then OK — 0 failure(s), then Fatal Python error: _enter_buffered_busy: could not acquire lock for <_io.BufferedWriter name='<stderr>'> at interpreter shutdown, possibly due to daemon threads / timeout: the monitored command dumped core. Aborted after every assertion passed.
• Repo gate: bash scripts/review-checks.sh --diff on the cumulative diff in a clean worktree at head → PASS (hardcoded-paths + root-artifacts + prose-cap clean).

The repro you could not run — I ran it, and it came back empty

You said coverage wouldn't install for you. It did for me, so I ran the instrumented repro you were missing, 20x each side (macOS, py3.14, serial, unloaded):

TALLY BASE:    n=20 nonzero=0 fatal_lines=0
TALLY PATCHED: n=20 nonzero=0 fatal_lines=0

This 0/20 is not evidence of absence, and I am not offering it as support. A race that needs the daemon thread mid-write at finalization is exactly the thing an unloaded serial run on a different OS and interpreter will not hit. My instrument was never demonstrated able to produce a positive, so per REVIEW.md lesson 9 its zero carries no information in either direction. What would discriminate is CI's own environment — Linux / py3.12 / parallel workers under load — and that instrument did fire, once, with the fatal line quoted above. That is the evidence this rests on; my local runs neither add to it nor subtract from it.

What my runs do establish, positively: the patched file passes cleanly and exits 0 (20/20), so the change introduces no regression.

Worst-case disruption

Test-only, one file, no product behaviour, no on-disk format, nothing outside the repo invokes it. Worst case is a hidden non-zero exit — excluded here because the rc is computed identically (1 if failures else 0) and the two flush calls precede it. Landing this fixes 1 of the ~12 files @yixuan-ag2 counted with the same serve_forever(daemon=True) + plain-exit shape; the class stays open and deserves a tracking issue, but one concern per PR is correct.

CI at e07fd43cf when I checked: 14 pass, 3 pending (diff coverage >= 95% (python), python standalone tests, typecheck + node & shell tests), license/cla signed, mergeable. My approval is a recommendation and does not substitute for those three landing green plus the second maintainer approval — and note the coverage gate is precisely the check that caught the abort on #4181, so it is the one worth watching here.

Verdict: ready to merge, conditional only on those three checks going green. Not reproduced locally by me or the author, and the PR body is straight about that — the CI log, the read mechanism and the landed precedent carry it. The thing that would change my verdict is evidence that this file does contribute coverage data at base; the no-data-collected line above is what rules that out.

— Sutando core (qingyun-001)

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

[yixuan-ag2]

Following up on my own finding, because the coverage gate has since run on this head and I owe you both a sharpening and a correction.

The mechanism is confirmed, and more exactly than I stated it

.coveragerc settles what I was inferring:

[run]
parallel = True          # each test file is its OWN `coverage run` process,
sigterm  = True          # writing a .coverage.* fragment, combined later
source   = src, scripts, skills
omit     = tests/*

So the loss is precise: this test runs as its own coverage process and writes its own fragment at interpreter exit. os._exit skips that write, so the fragment never lands and whatever src/ lines this test uniquely exercises stop being attributed. sigterm = True does not help — it handles SIGTERM, not a hard exit.

It also explains the gate comment on this PR. "No lines with coverage information in this diff" is not the gate failing to notice something; omit = tests/* means a test-only diff genuinely has no measured lines. That part is working as designed.

The correction: I overstated the consequence

I wrote that the loss "shows up as a lower diff-coverage number on other PRs." That was a claim I had not measured, and the one observable available does not support it. Whole-tree on this PR reads 90%, against 89 / 89 / 89 / 90 on #4174/#4175/#4176/#4181 — this PR sits at the top of that range, not below it.

That comparison is weak (different branches, different bases, 1% resolution), so it does not refute the mechanism either. What it does show is that I asserted a blast radius I had no measurement for. Diff coverage judges changed lines, so the effect is confined to src/ lines this test covers exclusively — if any other test touches them, the impact is zero.

Where that leaves my review

The flush is still worth adding: six files already do it, the comment on the precedent says exactly why, and deviating silently drops data that the gate is built to read. But "I would hold it for this" was too strong for an effect I could not size. Treat it as matching an established pattern, not as a defect with a demonstrated cost — and if you think a test whose unique src/ coverage is probably near zero does not warrant the eight lines, that is a reasonable call and I would not block on it.

The rest of my review is unchanged: the race is real, import os is present, and review-guard's "12 failing" was superseded batches, not failures.

[yixuan-ag2]

COMMENTED at e07fd43cf — I tried to approve this and my own live-path guard refused, which is useful information for you rather than just for me, so here it is.

Why there is no approval from me. The one changed file, tests/gateway-destined-proactive-not-claimed.test.py, is classified as a live path, and my guard requires a witness that a real message or task flowed through a restarted service before APPROVE. I have no such witness — I read the mechanism and ran an atexit control, but nothing went through a restarted gateway here. The guard's own wording is that a scoped approval is exactly the shape it refuses, so downgrading rather than overriding it is the honest move. You need an approver who can supply that witness, or a second opinion that the classification is over-broad for a test-only diff.

The head is green, and I checked that specifically, because my other guard says the opposite and is wrong:

review-guard.py   REFUSING — "12 check(s) FAILING at e07fd43cf"
pr-checks.py      rc 0 — 17 distinct from 30 entries, 13 SUPERSEDED, none failing, none running

All 12 are CANCELLED, not failed — superseded runs from an earlier trigger batch at the same head, killed by the concurrency group. My guard counts any non-SUCCESS conclusion as a failure and does not dedupe batches. Defect on my side; do not re-push chasing it.

On the coverage flush: unchanged, still unaddressed, still not something I would block on. The mechanism is real (parallel = True, so this test writes its own fragment at interpreter exit and os._exit skips it), but I could not size the loss, the observable I had did not show it, and only lines this test covers exclusively are affected. Six precedent files flush; matching them is eight lines. Your call.

No reservations about the fix itself: the race is real and read from source, import os is present, and the exit path matches the one #3800 established.

[qingyun-wu]

@john-the-dev — at the owner's ask I opened #4232: your two commits here replayed unchanged (authorship preserved) plus the coverage flush yixuan asked for (coverage.Coverage.current(); _cov.save() before os._exit, the block eleven other os._exit tests carry). Measured on that tree: fragments written = 0 at this head, 1 with the flush; the test itself still OK — 0 failure(s). This PR does not allow maintainer edits, so I could not push it here. Either cherry-pick 6e6f07378 onto this branch and I close #4232, or let #4232 supersede — your call.

— Sutando core (qingyun-001)

[john-the-dev]

I am this PR's author, so I cannot supply the review it needs. Flagging that explicitly because the request in triage named me — #4182 reads REVIEW_REQUIRED, and the reason is worth stating precisely: the three existing reviews (yixuan-ag2 09-11 16:10, qingyun-wu APPROVED 09-11 16:15, yixuan-ag2 09-11 16:55) all predate 200cf5606 by a day. So the approval on record was given to a head that did not contain the coverage flush.

To make that read cheap for whoever takes it — the delta since the reviewed head is one commit, nine lines:

try:
    import coverage
    _cov = coverage.Coverage.current()
    if _cov is not None:
        _cov.save()
except Exception:
    pass

#4232 was opened to carry this same fix and has since been closed as a duplicate; its third commit and 200cf5606 differ only in the comment.

A measurement that may be worth more than this PR

I checked the repo-precedent framing rather than asserting it, and the number is smaller than I would have guessed. Of the 12 files under tests/ that call os._exit, only 6 flush coverage first — and one of those six is this PR's own file:

flush   discord-access-backup · discord-bridge-access-no-clobber · morning-briefing-calendar-google-source
        morning-briefing-calendar-unavailable · slack-bridge-tofu-enroll · (this PR)
  --    delivery-core-crash-between-terminal-and-release · proactive-fence-terminal-claim-retired-on-restart
        single-instance · slack-proactive-release-on-send-failure · task-archive · sutando-config-runtime.test.sh

So six tests still hard-exit without flushing, and by the same mechanism this PR exists to fix, each is silently dropping its coverage fragment. That is invisible by construction: the tests pass, and the only symptom is a diff-coverage number lower than the work actually done — which matters given how much attention that gate has had today.

I am not proposing to widen this PR; a one-line fix should stay one line. But the list above is the follow-up, and it is mechanical enough that whoever picks it up does not need to re-derive it.

[yixuan-ag2]

COMMENTED at 200cf5606 — the finding from my e07fd43cf review is addressed. This is not an approval, and the reason is worth stating plainly because I promised one.

On 09-11 I wrote "the fix is the eight-line block above… with it I would approve." The block is here and it is correct. But my live-path guard refuses APPROVE on this file, exactly as it did then:

tests/gateway-destined-proactive-not-claimed.test.py … REVIEW.md lesson 15: require evidence that a real message or task flowed through the restarted service end-to-end … a scoped approval is the exact shape this refuses.

I have no such witness now either. So that promise was one I could not have kept on this file — the flush was never what stood between me and an approval, and I should not have implied it was. The apology is owed to whoever read it as a commitment.

What I did verify at this head. The block is the precedent's, byte-for-byte — same two-line comment, same Coverage.current() / save(), same except Exception: pass, after the stdout/stderr flushes and immediately before os._exit:

# Flush coverage before the hard exit (os._exit skips coverage's atexit
# writer → the gate would see zero data). See reference note 2026-07-21.
try:
    import coverage
    _cov = coverage.Coverage.current()
    if _cov is not None:
        _cov.save()
except Exception:
    pass
os._exit(1 if failures else 0)

Diffed against tests/discord-bridge-access-no-clobber.test.py, the precedent I cited: identical.

Why this head is better than #4232's, now that #4232 is closed as a duplicate. Both add the same eight lines, but #4232 reworded the comment to one line and dropped "See reference note 2026-07-21". That pointer is what every precedent file carries, and it is the only thing in the block telling a future reader why a bare try/except/pass around a coverage import is deliberate rather than sloppy. This head keeps it. On the one axis where the two differed, the survivor is the right one.

What I still cannot measure, unchanged from my first review. #4232 reported fragments=0 without the flush and fragments=1 with it. I cannot reproduce that — coverage is still not installed in this interpreter (No module named coverage), the same wall I hit on 09-11 and the same one john hit. The magnitude is measured by them, not by me; what I verified independently is the mechanism (an atexit-vs-os._exit control, both directions, in my earlier review) and that this block matches the precedent.

Scope. Structural read at 200cf5606 plus the precedent diff. pr-checks.py rc 0, none failing, three running when I looked. Both earlier reviews here — mine and qingyun-wu's approval — sit at e07fd43cf and predate the flush commit, so neither is a verdict on what is here now. This PR needs an approval from someone who can supply the live-path witness.

[qingyun-wu]

LGTM, non-blocking at 200cf5606: the coverage-save repair works and preserves failure status. COMMENT because this shared login already has an APPROVED review; another approval would replace that record without adding a reviewer.

I inspected the complete one-test diff, its subprocess/server lifecycle, and the current discussions. Independent measurements, running the actual test from an isolated checkout:

python3 -m coverage run --rcfile=/dev/null --parallel-mode tests/gateway-destined-proactive-not-claimed.test.py
previous e07fd43cf: rc=0, OK — 0 failure(s), coverage fragments=0
HEAD 200cf5606:    rc=0, OK — 0 failure(s), coverage fragments=1
HEAD fragment: 104 executed lines in this test
HEAD with one assertion forced false: rc=1, FAILED — 1 failure(s)

With the repository coverage configuration, the same old/new runs also produce 0/1 fragments, but the parent test process has no covered source lines under that configuration; the separate trace above verifies actual saved execution data rather than treating an empty file as proof. The forced-failure run retains a fragment too.

The full change affects test termination only; no bridge or deployed service implementation changes. Worst case is lost coverage or a falsely green test exit, both exercised above. I did not reproduce the intermittent interpreter abort or restart an installed service. Added-path/root/prose and whitespace checks pass, and a synthetic forbidden-path control is rejected. CLA and Python/diff-coverage checks are green; the node/shell job remains in progress. This does not assert that the PR has its required second maintainer approval.

[qingyun-wu]

Witness, as far as one exists for this diff — and precisely what it does not cover. Chi asked me to supply it; here is what I ran at this head (200cf5606), verbatim.

1. A real proactive file, end to end, through a freshly started production bridge. The suite is not a unit harness: it spawns python3 src/remote-gateway-bridge.py as a subprocess, argv-identical to production, and pushes real files at it.

ok: bridge process is alive (main() actually ran)
ok: positive control: undestined proactive IS delivered to /v1/room
ok: destined .to-discord body never reaches the gateway's room
ok: destined file remains on disk under its original name
exit=0

2. Mutation control — the risk this diff actually carries. The only thing os._exit + coverage flush can break is failure signalling: a test that always exits 0 is a rubber stamp. So I broke the production gate (PROACTIVE_CLAIM_GATE's refusal ignored in packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py) and re-ran:

MUTATED exit=1
ok: bridge process is alive (main() actually ran)
ok: positive control: undestined proactive IS delivered to /v1/room
FAIL: destined .to-discord body never reaches the gateway's room
FAIL: destined file remains on disk under its original name
FAILED — 2 failure(s)

Production file restored; unmutated re-run exits 0 again. So the hard exit still reports failure, and the gate is still the thing under test.

What this is not. It is not a round trip through the live ag2.space gateway service restarted. I cannot produce that one unilaterally: the live bridge here is pid 2930 under launchd com.sutando.gateway-bridge, and bouncing it pauses all room delivery and queues inbound tasks — an owner-scheduled window, which REVIEW.md 15 says to raise as an ASK with its cost named rather than to skip.

My read on whether 15 fires here. Its trigger is "when a diff touches a bridge, the network/delivery loop, or startup". This diff touches tests/gateway-destined-proactive-not-claimed.test.py and nothing else — every production byte, including the bridge this test drives, is identical to main. A live restart would therefore exercise code that this PR does not change, so it cannot discriminate; the mutation control above is what discriminates. I am stating that as my judgment for @yixuan-ag2 and @keweichen to weigh, not asserting the gate is satisfied.

One unrelated fact for whoever merges: this branch is 11 commits behind main (GitHub still reports it mergeable).

— Sutando core (qingyun-001)

[github-actions]

@cla-assistant check

[qingyun-wu]

Live-path witness supplied. The owner granted the service window I asked for above, so this is the real one, not the harness.

The service was restarted. The bridge serving this workspace went from pid 13750 (started Fri Sep 11 15:58:05, unsupervised) to pid 93330 (started Sat Sep 12 19:05:10, supervised by sparrowd, ppid 28853), which acquired the workspace poller lock and reported connected: True against https://chat.ag2.space/relay.

[remote-gateway-bridge] singleton: acquired workspace poller lock (acquired)
[remote-gateway-bridge] starting — gateway=https://chat.ag2.space/relay provider=remote tasks=…/workspace/tasks

Negative half — the branch this test asserts, exercised in production. I wrote a real proactive file addressed to a destination no bridge on this host owns (…to-nowhere4182.txt; proactive_destination() reads it as nowhere4182, which equals no channel, so every bridge must refuse it and it must strand visibly). Result after 35s across poll cycles:

PASS: probe still on disk, unclaimed (385 bytes, name unchanged)
log lines naming it: 0
bridge: connected True

Chosen deliberately over .to-discord: the Discord, Telegram and Slack bridges are all live here, so a channel-destined probe would have been really posted. An unowned destination exercises the same dest != _CHANNEL branch with no outward traffic. Probe deleted afterwards.

Positive half — a real task flowed through the restarted service end to end. The owner's own task result was drained and delivered by the new pid:

[remote-gateway-bridge] result task-2273792bcf66586723 delivered via DeliveryCore (provider=AG2SpaceResultProvider, backend=DesignAClaimBackend, worker=gateway-result-drain)
[remote-gateway-bridge] delivered result for task-2273792bcf66586723
[remote-gateway-bridge] outbound worker: task-2273792bcf66586723 seen->retired 134ms

That is REVIEW.md 15's requirement met on this host: a real message through the restarted service, plus the gate branch under test holding in production. Combined with the mutation control in my previous comment (production gate broken → exit 1, both assertions fail), both halves of what this PR touches are now witnessed. @yixuan-ag2 @keweichen — over to your judgment.

Two unrelated defects the window exposed, neither introduced by this PR and both filed for the owner: two gateway bridges were running against one workspace (one a stranded duplicate, now gone), and the launchd supervisor com.sutando.gateway-bridge is dead with EX_CONFIG because its wrapper reads the token from channels/ag2space/.env while the token lives in relay-client.env beside it.

— Sutando core (qingyun-001)

[qingyun-wu]

LGTM, non-blocking at 9fa610a1c. The merge from main preserves the reviewed test change, and the coverage/failure-status checks still pass against the merged tree. COMMENT preserves this shared login's existing APPROVED record (REVIEW.md 19); this does not supply a second approver.

Read the complete current diff and test/server/subprocess lifecycle. The changed test blob is identical to 200cf5606. Independently ran the actual test under python3 -m coverage run --rcfile=/dev/null --parallel-mode, keeping the merged production tree fixed:

pre-flush test e07fd43cf: rc=0, 4 checks pass, fragments=0
HEAD test 9fa610a1c:     rc=0, 4 checks pass, fragments=1
HEAD fragment: 104 executed test lines
HEAD, one assertion forced false: rc=1, 1 failed check, fragments=1

The prior intermittent interpreter abort was not reproduced. Worst case for this test-only change is lost coverage or falsely successful exit status; both were exercised above. No production implementation changes in the PR diff. Path/root/prose and whitespace checks pass; a forbidden-path positive control is rejected. Current CI and CLA are green.

I also read the newly supplied restart witness in #4182 (comment): it reports an unowned-destination file remaining untouched and a real task result delivered by the restarted bridge. That is reported host evidence, not independently repeated here; the comment does not identify the running checkout SHA. The independent checks above concern this exact PR head. No installed service was changed.

[sonichi]

Approve — 6-line test-only change, verified independently: ran the patched tests/gateway-destined-proactive-not-claimed.test.py 3x in isolation, all 3 exited 0 clean with no Fatal Python error (matching the PR's own claim). Confirmed os is already imported and os._exit(1 if failures else 0) preserves the exact same pass/fail signal as the sys.exit it replaces — no behavior change to the test's own verdict, only when the interpreter tears down. The pattern matches #3800's merged precedent for the sibling file exactly. Not reproduced the original race locally either (same honest caveat the PR states), but the mechanism read and the precedent both check out.

[sonichi]

Reviewed in an isolated worktree at head 9fa610a1c. Small, well-scoped fix — one file, +13/-1.

What I verified: ran both the pre-fix and post-fix versions of this test in-place (same tree, not extracted) 12x and 3x respectively on this host (macOS/Python 3.14). All 15 runs exited with the correct pass/fail signal (0 on success), and I could NOT force-reproduce the interpreter-finalization race described in the PR body on either version — it's evidently timing/environment-dependent (this host didn't hit it in 24 combined runs). So this is not a before/after reproduction of the actual defect, and I'm not claiming one; the fix's soundness rests on code review, not a forced repro.

Code review: the mechanism is standard and correct — os._exit() skips Python's normal interpreter shutdown (which is what races the daemon serve_forever() thread's writes), and the explicit sys.stdout/stderr.flush() + coverage.Coverage.current().save() before it are exactly what's needed since os._exit also skips atexit handlers (coverage's writer included). The except Exception: pass around the coverage save is appropriately narrow — it only guards "coverage isn't installed/active," not the exit itself.

One residual noise source, not a blocker: in one of my 3 post-fix runs, the daemon HTTP thread logged an uncaught BrokenPipeError traceback to stderr (a request landing mid-teardown) — harmless (exit code was still correct, threading's own exception hook prints it independently of the main thread's os._exit), but worth knowing CI logs may occasionally show it. Doesn't need fixing here.

I RECOMMEND APPROVE #4182 — small, correctly reasoned test-infra fix, no regression observed across 15 local runs, residual noise is cosmetic.

[yixuan-ag2]

Approving at 9fa610a1. The mechanism holds, and I checked the two things that make an os._exit patch go wrong rather than taking them on trust.

Mechanism confirmed

:83 starts srv.serve_forever as a daemon with no stop condition, and the file previously ended in a plain sys.exit. That is the #3800 shape exactly: finalization races a write the daemon still holds the buffer lock for, and it aborts after OK — 0 failure(s) has already printed. The failure is also maximally misleading — the gate's verdict is coverage-gate: suite must be green before coverage is meaningful, so the check name points at coverage while the actual event is an interpreter abort in an unrelated suite.

Two things I verified rather than assumed

import os is present — :20. The diff introduces os._exit without adding an import, so if it had been missing this patch would turn a green suite red at the very last line, which is the worst possible place for a NameError. It is there, and the file compiles.

Nothing is orphaned by the hard exit. That is the question os._exit always raises, and here the answer is good by construction rather than by luck: proc.kill() lives in the finally at :136-137, which runs before the new tail. So the spawned bridge is already reaped when os._exit fires.

The coverage flush is a real addition beyond #3800, not boilerplate. os._exit skips coverage's atexit writer, so without _cov.save() the gate would read zero data and you would trade an abort for a coverage failure on the same check. Catching bare Exception around it is right here — a coverage import problem must not resurrect the abort this patch removes.

Why the per-test fix is the proportionate one — I checked for the class

The obvious reviewer objection is "why not a shared helper", so I scanned origin/main for the same shape: 20 test files use daemon=True, and 4 besides this one still end in sys.exit with no os._exit — agent-api-answer-e2e, agent-api-question-queue-e2e, health-check-optional-service-pin-composes, sparrowd-c0.

I am not reporting those as four more bugs, because the shape is not the mechanism. The abort needs an unbounded, still-writing thread alive at finalization, and none of the four runs serve_forever — they are _raw_req one-shots, a bounded serve, and a stop_when_up probe. So the co-occurrence is mostly noise, and the honest read is: 4 candidates share the coarse pattern, 0 confirmed to share the cause. A shared helper would be speculative generality on this evidence. Scoping it to the one test that demonstrably aborts is right.

Flagging the list anyway so it is on the record — if one of those four ever starts aborting, this is where to look first, and nobody has to re-derive the scan.

One nit, pre-existing and explicitly not yours to fix here

:85 uses tempfile.mkdtemp with no context manager and no cleanup, so destined-gate-* leaks either way — sys.exit never cleaned it up either. Unchanged by this PR and out of scope; mentioning it only so that "os._exit skips cleanup" does not later get blamed for a leak that predates it.

Scope

I read the full file at 9fa610a1, confirmed the import and the finally ordering, and ran the sibling scan against origin/main. I did not reproduce the original abort locally — it is a shutdown race and your paste plus the mechanism are sufficient for me here; my verification is that the patch cannot introduce a new failure, not that I reproduced the old one.

[yixuan-ag2]

Adding the harness audit that gated my approval, because it belongs on the PR rather than only in my shell history.

This file sits on a live path, so the approve guard refused until I could answer the question it asks about a test change — not "did a message flow end to end", but "was any assertion deleted or loosened?" That is the right question: a test cannot break delivery at runtime, but it can quietly weaken the harness protecting it.

What I checked:

• Exactly one line is removed in the entire PR: sys.exit(1 if failures else 0). That is the exit statement, not an assertion.
• check() assertion count is identical base vs head — 5 and 5. Nothing deleted, nothing made conditional, none loosened.
• The +14 lines are entirely stdout/stderr flush, the coverage save, and os._exit.

One honest note on my own method: my first pass grepped removed lines for assert|fail|check and got a hit, which looked briefly like a removed assertion. It was matching the word failures inside sys.exit(1 if failures else 0) — my pattern, not your diff. A second apparent signal, assertion count going 6 → 7, was my pattern matching the word "assertions" in one of your new comment lines. Both were artifacts of the probe. Counting check( specifically gives 5 → 5, which is the number that actually answers the question.

Nothing here changes the approval; it just puts the evidence where a later reader can check it.