Repository navigation
fix(sparrow): per-PID staging for state-file atomic writes (#2222 follow-up) - #2237
Merged
Merged
Conversation
…low-up) remote_gateway_bridge.py staged four state files under a shared `.json.tmp` name. For last-owner-activity.json this is the #2222 cross-process bug directly — the sparrow bridge is the fourth writer alongside slack/discord/telegram, and two concurrent writers can truncate/interleave the shared temp so the rename publishes torn JSON to the proactive loop's presence check. For gateway-status, task-rooms, and inflight it is latent: safe with one sparrow instance, a torn-write race the moment two run. Stage each under a per-PID name (`.json.{os.getpid()}.tmp`) and publish with os.replace (atomic overwrite) — no two processes ever share a temp path. Also switches gateway-status from Path.replace to os.replace so all four sites are uniform (behaviour-identical; both are atomic overwrite). Completes the sparrow follow-up noted on #2222; the three in-repo bridges are #2236. The per-PID mechanism is proven under real concurrency by tests/owner-activity-atomic-write.test.py (#2236, identical pattern), so this does not re-prove it. Test (tests/sparrow-per-pid-staging.test.py, main harness): - source guard: all four sites stage per-PID, none uses the shared `.json.tmp`; mutation-checked (reverting any site to the shared name fails it). - behavioral: imports the bridge, spies os.replace, and confirms the live gateway-status write stages a pid-suffixed temp and publishes a valid file. Existing sparrow tests (test_gateway_status.py) still pass — the Path.replace → os.replace change is behaviour-preserving. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BuRpcYxqsRQsoV44E5Gh1
Contributor
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 57%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
Collaborator
Author
|
No blocking findings on current head Local checks run:
Remote status at review time: lint/guard checks and diff coverage were green; Reviewed by Qingyun's Codex. |
Collaborator
Author
|
Reviewed (qingyun-001). The split is the right call (sparrow rides its own PyPI cadence), and covering all four sparrow staging sites here — not just owner-activity — is better than my "fine to leave" note on #2236. Two coordination items before this is click-ready:
With those two, LGTM — tests are green and the per-PID + — @sutando-qingyun-001 |
john-the-dev
approved these changes
Jul 20, 2026
john-the-dev
left a comment
john-the-dev
left a comment
Collaborator
There was a problem hiding this comment.
Verified — safe and correct. The core race is real: last-owner-activity.json has 4 writers (this sparrow bridge + slack/discord/telegram), and a shared .json.tmp lets two concurrent writers truncate + interleave the same temp → the rename can publish torn JSON to the proactive loop's presence check. Per-PID .json.{getpid()}.tmp is never shared, and os.replace is an atomic same-dir overwrite (last writer wins cleanly). The other 3 sites (gateway-status/task-rooms/inflight) are latent single-writer-today races — fine to harden defensively with the same pattern.
The tmp.rename/tmp.replace → os.replace swap is a real improvement (atomic-overwrite semantics, cross-platform), not just cosmetic. Test is appropriately scoped: it source-guards all 4 sites against regressing to the shared name + behaviorally confirms the live gateway-status write stages a pid-suffixed temp, and correctly defers the concurrency proof to the sibling #2236 test rather than duplicating it. CI green.
(Maintainer vote — one of the two.)
This was referenced Jul 20, 2026
qingyun-wu
added a commit
that referenced
this pull request
Jul 21, 2026
…v1 freeze gate) (#2240) * test(sparrow): atomic publish + in-flight recovery regression tests Two v1-freeze-gate regression tests for the ag2-sparrow transport client (gate items 5 & 6 of ag2-space/ag2space-backend#142), locking down the worker-edge properties the broker's at-least-once contract depends on: - test_write_task_atomic.py (gate item 5): _write_task publishes a complete file (access_tier written last), leaves no .txt.tmp sidecar, never exposes a partial file to the watcher on a publish crash (rename failure => dest absent), is idempotent under gateway redelivery (a modified-body redelivery must NOT overwrite the queued file — proven via a mutation check, not just an identical-body no-op), and drops id-less / unsafe-id tasks. - test_inflight_recovery.py (gate item 6): the persisted in-flight ledger survives a restart (save -> reload -> load round-trips the set), so a task pulled but not yet POSTed to /v1/results isn't lost across a crash; the write is atomic (no .json.tmp sidecar) and the read fails open (corrupt/missing ledger loads empty, never raises). Complements the atomic-write hardening in #2236/#2237 (per-PID staging) — those are the fix, these pin the behavior so it can't regress. Tests follow the existing tests/test_gateway_status.py harness (env-injected task/result/state dirs + importlib.reload); no product code changed. * test(sparrow): no-double-execution on re-serve after completion (#142 item 3) Broker re-serves a task on lease expiry (at-least-once). If the worker already handled it, _write_task must NOT re-queue it (no second local execution) — the existing guard drops a [no-send] result so the drain re-acks it upstream. Adds the test for that re-serve-after-completion branch (both triggers: task archived, and reply already archived). Mutation-checked: disabling the archived-dedup guard makes this test fail. Completes freeze-gate item 3 alongside items 5/6 in this PR.
john-the-dev
pushed a commit
that referenced
this pull request
Jul 21, 2026
* test(sparrow): atomic publish + in-flight recovery regression tests (v1 freeze gate) (#2240) * test(sparrow): atomic publish + in-flight recovery regression tests Two v1-freeze-gate regression tests for the ag2-sparrow transport client (gate items 5 & 6 of ag2-space/ag2space-backend#142), locking down the worker-edge properties the broker's at-least-once contract depends on: - test_write_task_atomic.py (gate item 5): _write_task publishes a complete file (access_tier written last), leaves no .txt.tmp sidecar, never exposes a partial file to the watcher on a publish crash (rename failure => dest absent), is idempotent under gateway redelivery (a modified-body redelivery must NOT overwrite the queued file — proven via a mutation check, not just an identical-body no-op), and drops id-less / unsafe-id tasks. - test_inflight_recovery.py (gate item 6): the persisted in-flight ledger survives a restart (save -> reload -> load round-trips the set), so a task pulled but not yet POSTed to /v1/results isn't lost across a crash; the write is atomic (no .json.tmp sidecar) and the read fails open (corrupt/missing ledger loads empty, never raises). Complements the atomic-write hardening in #2236/#2237 (per-PID staging) — those are the fix, these pin the behavior so it can't regress. Tests follow the existing tests/test_gateway_status.py harness (env-injected task/result/state dirs + importlib.reload); no product code changed. * test(sparrow): no-double-execution on re-serve after completion (#142 item 3) Broker re-serves a task on lease expiry (at-least-once). If the worker already handled it, _write_task must NOT re-queue it (no second local execution) — the existing guard drops a [no-send] result so the drain re-acks it upstream. Adds the test for that re-serve-after-completion branch (both triggers: task archived, and reply already archived). Mutation-checked: disabling the archived-dedup guard makes this test fail. Completes freeze-gate item 3 alongside items 5/6 in this PR. * fix(gateway-bridge): prefer IPv4 for the relay (v6-blackhole ~26s delay) (#2238) On hosts where IPv6 is black-holed at the network (SYN silently dropped), the relay host's AAAA records make Python's getaddrinfo return v6 first, so each fresh urllib connection — one per long-poll AND one per outbound send, no keep-alive — hangs ~26s on the dead v6 address before falling back to v4. That timeout is added to EVERY inbound message and EVERY reply, so the owner sees a ~26s delay each way and messages appear dropped (measured live 2026-07-20: stuck tcp6 SYN_SENT to the relay's 2606:4700:20:: address; v4 connect in 0.25s). Filter getaddrinfo to A records for the gateway host (`ag2.space`) at import, so the dead v6 path is never tried. Keep the original result when there is no v4 address (a genuinely v6-only destination still resolves), and scope the filter to the relay host so nothing else is affected. Opt out with REMOTE_GATEWAY_ALLOW_IPV6=1 for hosts with working v6. Verified: filter returns v4-only for chat.ag2.space ([AF_INET,AF_INET6] -> [AF_INET]); non-relay hosts pass through unchanged; py_compile clean. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sparrow): per-PID staging for state-file atomic writes (#2222 follow-up) (#2237) remote_gateway_bridge.py staged four state files under a shared `.json.tmp` name. For last-owner-activity.json this is the #2222 cross-process bug directly — the sparrow bridge is the fourth writer alongside slack/discord/telegram, and two concurrent writers can truncate/interleave the shared temp so the rename publishes torn JSON to the proactive loop's presence check. For gateway-status, task-rooms, and inflight it is latent: safe with one sparrow instance, a torn-write race the moment two run. Stage each under a per-PID name (`.json.{os.getpid()}.tmp`) and publish with os.replace (atomic overwrite) — no two processes ever share a temp path. Also switches gateway-status from Path.replace to os.replace so all four sites are uniform (behaviour-identical; both are atomic overwrite). Completes the sparrow follow-up noted on #2222; the three in-repo bridges are #2236. The per-PID mechanism is proven under real concurrency by tests/owner-activity-atomic-write.test.py (#2236, identical pattern), so this does not re-prove it. Test (tests/sparrow-per-pid-staging.test.py, main harness): - source guard: all four sites stage per-PID, none uses the shared `.json.tmp`; mutation-checked (reverting any site to the shared name fails it). - behavioral: imports the bridge, spies os.replace, and confirms the live gateway-status write stages a pid-suffixed temp and publishes a valid file. Existing sparrow tests (test_gateway_status.py) still pass — the Path.replace → os.replace change is behaviour-preserving. Claude-Session: https://claude.ai/code/session_015BuRpcYxqsRQsoV44E5Gh1 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sparrow): self-healing retry for task /ack when the gateway gains the endpoint (#2241) * fix(sparrow): self-healing retry for task /ack when the gateway gains the endpoint _post_task_ack POSTs /v1/tasks/<id>/ack on every pulled task so the broker can surface a "received" state. When the gateway lacks the endpoint it 404s — but the worker latched `_ack_disabled = True` permanently, so a broker that LATER deploys the endpoint (e.g. the #142 freeze-gate /ack landing on the relay) is never picked up until the worker restarts. That's a silent deploy-ordering trap: acks stay off, and the "received/working" ladder shows nothing, with no error. Replace the permanent bool latch with a time-gated backoff (`_ack_disabled_until`, epoch): a 404/405 backs off for REMOTE_ACK_RETRY_COOLDOWN (default 300s) then retries, and a successful ack clears it. A newly-deployed endpoint is thus picked up automatically, no restart needed. Heartbeat keeps its own latch (separate concern, endpoint has always existed). Test (tests/test_ack_retry.py): success-no-backoff, invalid-tid-no-network, 404-backs-off-within-cooldown, and self-heal-after-cooldown. The self-heal test is mutation-checked — a permanent latch (cooldown → ∞) fails it, the finite cooldown passes it — so it genuinely pins the retry, not an identical-body no-op. * test(sparrow): update src bridge test for _ack_disabled → _ack_disabled_until rename The canonical package rename (permanent latch → time-gated backoff) left the src-side mock-gateway test suite (src/remote-gateway-bridge.test.py) asserting on the old _ack_disabled bool, which no longer exists — the '404 disables ack' check failed in CI (tsc + tests / clean install). Update it to the new symbol + semantics (404 arms a retryable backoff, _ack_disabled_until > 0) and clear the timer after so later calls aren't skipped. Also fix the stale symbol in the shim's namespace-doc comment. src suite + package ack test both green. * fix(sparrow): a per-task 404 on /ack must not disable acking globally _post_task_ack armed a global cooldown on ANY 404. But a 404 can be per-task — the gateway answers 'not leased to you' for an expired/re-served/foreign lease — not 'endpoint absent'. One such routine 404 would then skip /ack for every other task until the cooldown expired, recreating the visibility blind spot this change removes, triggered by normal churn. Distinguish by response body: a 404 carrying the 'not leased' marker is a single-task negative ack (skip that one, keep acking the rest); only a bare no-route 404 or a 405 arms the unsupported-endpoint cooldown. Adds a test (mutation-checked) + a bare-404-still-cools test; existing behavior for the pre-endpoint bare 404 is unchanged. * fix(bridges): per-PID staging for last-owner-activity.json (concurrent writes) (#2236) * fix(bridges): per-PID staging for last-owner-activity.json (concurrent writes) `write_owner_activity()` in the slack, discord, and telegram bridges staged its atomic write under a SHARED temp name (`OWNER_ACTIVITY_FILE.with_suffix (".json.tmp")`). last-owner-activity.json is written by four processes (these three bridges + the sparrow remote-gateway bridge), so two that fire within the same window both open and truncate the same temp file; their writes interleave, and the rename can publish torn JSON. The reader is the proactive loop's owner-presence check, which then sees a corrupt or partial timestamp. Stage under a per-PID name (`.json.{os.getpid()}.tmp`) so no two processes ever share a temp file, and use os.replace (atomic overwrite): last writer wins, cleanly, with no torn intermediate. Closes #2222. Scope: the three in-repo bridges. The sparrow package (packages/ag2-sparrow/…/remote_gateway_bridge.py) shares the same pattern across several state files (owner-activity, gateway-status, task-rooms, inflight); that is a separate package boundary and a follow-up, noted on #2222. Fixing these three already removes the realistic collision — they are the bridges that co-run on one host, and once they use distinct temp names no two writers share a path. Test (tests/owner-activity-atomic-write.test.py, runs under npm run test:py): - concurrency proof: 12 spawned processes hammer the target with the per-PID pattern; a concurrent reader samples throughout and every read parses — 0 torn. (A self-check, `--demo-old-pattern-tears`, shows the shared-name pattern DOES produce torn mid-race reads, so the proof exercises a real race.) - source guard: the three call sites use the per-PID form, not the bare `.json.tmp`. Mutation-checked: reverting any one to the shared name fails it. multiprocessing uses the spawn context with module-level targets (macOS-safe; avoids the #2201 heredoc re-import trap). Bounded by a 5s deadline + join. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BuRpcYxqsRQsoV44E5Gh1 * fix(sparrow): per-PID staging for last-owner-activity.json (4th writer) The sparrow remote-gateway bridge's _write_owner_activity() is the fourth concurrent writer of state/last-owner-activity.json named in this PR's body, but was staging under the shared ".json.tmp" name the rest of the PR removes — so a sparrow write racing a discord/slack/telegram write could still publish torn JSON to the proactive-loop's owner-presence reader. Apply the identical fix: per-PID temp name + os.replace (atomic overwrite, last-writer-wins). Extend the source-guard test's writer list to include the sparrow module so a revert is caught (mutation-verified: reverting this hunk fails test_source_sites_use_perpid_staging). The other .json.tmp writes in that file (gateway-status, task-rooms, inflight) are single-writer and correctly untouched. Completes #2222 across all four writers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(task-bridge): per-PID staging for the 5th owner-activity writer The per-PID staging fix covered the four Python owner-activity writers but missed src/task-bridge.ts, which still staged last-owner-activity.json under the shared '.tmp' name — the identical target path (with_suffix('.json.tmp') and OWNER_ACTIVITY_FILE + '.tmp' resolve to the same file). Two concurrent task-bridge writes could therefore still truncate/interleave the shared temp and publish a torn file, so the race this change removes was still reachable for that writer class. Move it to a process.pid-suffixed staging name (renameSync stays — atomic rename(2)) and extend the writer-census guard test to cover the TS site so the 5-writer set stays complete. Mutation-checked. (qingyun-001 blocking finding.) * fix(bridges): per-invocation (PID+uuid) owner-activity staging — thread-safe PID-only staging is unique across processes but SHARED across threads in one process, so a multi-threaded writer (Slack Bolt runs listeners on a ThreadPoolExecutor) still collides on the same '.json.<pid>.tmp' path — two listener threads truncate/interleave it and os.replace only makes the final rename atomic, not the concurrent writes to the shared staging inode. Repro (8 threads): 1,216 torn reads + 5,276 replacement failures. Move the four owner-activity writers (slack/discord/telegram + sparrow) to a per-INVOCATION name (PID + uuid4().hex), unique across processes AND threads. Adds a same-process threaded regression test (per-invocation → 0 torn; PID-only witness → does tear) and tightens the census guard to require the uuid form. Mutation-checked. (qingyun-001 blocking finding.) * test(sparrow): update the OTHER staging guard for the owner-activity uuid form The per-invocation (PID+uuid) owner-activity staging also has to satisfy the existing tests/sparrow-per-pid-staging.test.py guard, which still required OWNER_ACTIVITY_FILE to stage PID-only — so that suite failed. Split it: the three single-writer state files (gateway-status/task-rooms/inflight) keep the PID-only requirement; OWNER_ACTIVITY_FILE now requires the per-invocation PID+uuid form and explicitly rejects the thread-unsafe PID-only one. Both owner-activity guard tests (this + owner-activity-atomic-write) now pass; grepped tests/ to confirm these are the only two that assert the staging pattern. (qingyun-001 re-review finding.) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(sparrow): combined integration E2E — connected → task-once → result-once → reconnect → no-dup recovery Component tests assert each primitive in isolation; John asked for one combined live run proving the integration-heavy reconnect path. This harness drives the REAL bridge module through the full sequence against an in-process mock gateway (real HTTP contract → genuinely connected; reconnect forced deterministically by re-serving the in-flight task-id) and prints gateway/task/result log excerpts. Asserts exactly one local task, one ack, one result POST across a forced redelivery — no duplicate, no loss. Addresses the review request on #2245. * fix(test): drive main()'s loop body in the combined E2E redelivery leg The committed integration E2E did not exercise the control flow the bridge actually runs. On the redelivery leg it called _write_task() directly and jumped to _post_ready_results(), skipping inflight-add, persist, and ack. main() appends every id returned by _write_task() to pending_ack -- including a deduped redelivery, since _write_task() returns the id for already-handled work precisely so the broker gets its ack and stops replaying -- then acks each one after inflight is durable. The test therefore published acks=1 where the real loop produces acks=2: a false tally for the runtime path, in the artifact requested to close the integration review. Both legs now go through one drive_poll() helper that mirrors main()'s block line-for-line (write -> inflight -> persist -> ack every pending id -> drain), and the redelivery is asserted to re-ack upstream. Tally now matches a main()-shaped run: tasks_served=2, acks=2, real_results=1. Mutation-checked: removing the ack loop fails both ack assertions. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Qingyun Wu <qingyun@ag2.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the sparrow follow-up noted on #2222. The three in-repo bridges are #2236; this is the fourth writer.
The bug
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.pystaged four state files under a shared.json.tmpname:last-owner-activity.jsongateway-status,task-rooms,inflightThe fix
Per-PID staging (
.json.{os.getpid()}.tmp) +os.replace(atomic overwrite) at all four sites — no two processes ever share a temp path, last writer wins cleanly. Also switches gateway-status fromPath.replacetoos.replaceso all four are uniform (behaviour-identical; both are atomic overwrite).Scope
Only the four shared-
.json.tmpstate writes. The.rename()calls for task/result archiving (lines ~827/907/922) are single-writer moves of uniquely-named files — not this bug — and are left alone.Test —
tests/sparrow-per-pid-staging.test.pyThe per-PID mechanism is already proven under real concurrency by
tests/owner-activity-atomic-write.test.py(#2236, identical pattern), so this doesn't re-prove it. It:.json.tmpabsent) — mutation-checked: reverting any site to the shared name fails it;os.replace, and confirms the livegateway-statuswrite stages a pid-suffixed temp and publishes a valid file.packages/isn't in.coveragerc's source, so these lines aren't diff-coverage-gated. Existing sparrow tests (test_gateway_status.py) still pass — thePath.replace → os.replacechange is behaviour-preserving.🤖 Generated with Claude Code
https://claude.ai/code/session_015BuRpcYxqsRQsoV44E5Gh1