Skip to content

fix(remote): make Windows sends reach bus before return - #32

Merged
orangewk merged 2 commits into
mainfrom
feat/issue31-outbound-delivery
Jul 23, 2026
Merged

fix(remote): make Windows sends reach bus before return#32
orangewk merged 2 commits into
mainfrom
feat/issue31-outbound-delivery

Conversation

@orangewk

Copy link
Copy Markdown
Owner

Closes #31.

What changed

  • default remote sends to foreground push on Windows/MSYS managed shells
  • preserve the existing Unix background behavior
  • allow explicit override with AGMSG_REMOTE_PUSH_SYNC (1 foreground, other values background)
  • return non-zero when a foreground bus push fails, while keeping the message durable in the local store
  • document the platform-specific delivery semantics
  • add a regression test that delays the remote receive hook and requires the event to exist on the bus before send.sh returns

Why

The field store contained 27 local galaxias rows with UUIDs, but the bus had no events/galaxias/ writer. Managed Windows shells can terminate the background remote.sh push after UUID assignment and before writer append. send.sh then reported success although the event never reached the bus.

Verification

Not included

  • no production bus/backlog mutation
  • no global skill installation
  • no remote status accounting change (follow-up slice)

@orangewk

Copy link
Copy Markdown
Owner Author

レビュー

Windows/MSYS で send.sh のバス push がバックグラウンド実行のため、管理下シェルがコマンド終了と同時に子プロセスを kill してしまい「送信成功」と表示されてもイベントが bus に届かない (#31) 問題への対処。root cause の特定・実装・ドキュメント(ja/en)・回帰テストの一式が揃っており、CIも green です。以下、気になった点です。

良い点

  • 原因特定が的確: UUID 付与(ローカル)と writer file 追記(bus 到達)が非同期処理の中で分離しており、その間で背面プロセスが死ぬと「成功したのに届いていない」状態になる、という診断は sync_export/sync_push_remotelib/sync.sh)の実装と整合しています。
  • || true の除去は副次的だが重要な修正: 旧コードは AGMSG_REMOTE_PUSH_SYNC=1 を明示指定した場合でも push 失敗を >/dev/null 2>&1 || true で握りつぶし、常に exit 0 していました。今回それを撤去して失敗を伝播させるのは、Windows対応の副産物という以上に単体で正しい修正だと思います。
  • テスト設計: pre-receive hook に sleep 2 を仕込んで「push が完了するまで send.sh が返らない」ことを実測する回帰テスト(tests/test_remote_sync.bats#L488-L505)は、単に分岐を通すだけでなく実際のレース条件を再現していて良い設計です。

懸念点

1. sync_push_best_effort(既読レシート送信経路)が同じ脆弱性を残したまま — 優先度高

lib/sync.sh#L161-L168sync_push_best_effort()inbox.sh / check-inbox.sh の既読マーク時に呼ばれますが、今回のPRでは直っておらず、これと全く同じロジック(Windows含め常に背面実行、AGMSG_REMOTE_PUSH_SYNC=1 明示時のみ前面)のままです。

issue #31 のクローズコメントの滞留データが message_sent 27件 に加え message_read 23件 とあるのは、送信だけでなく既読レシートも同じ理由でバスに届いていない可能性を示唆しています。このPRは send.sh 側の症状だけを塞ぎ、同根と思われる既読側は未対応のまま #31 をクローズすることになりそうです。同じ修正をここにも当てるか、明示的にフォローアップ issue を切ってから閉じるか、判断をお勧めします。

2. 新しい失敗パスにテストが無い — 優先度中

send.sh の目玉変更の一つである「フォアグラウンド push 失敗時に exit 1 + "message saved locally..." エラー」(send.sh#L107-L113)を検証するテストが見当たりません。近い既存テスト(tests/test_remote_sync.bats#L232-L245 push: a failing remote surfaces a non-zero exit)は remote.sh push を直接叩いており、途中の send.sh 呼び出しは || true で exit code を捨てています。全テストが setup()AGMSG_REMOTE_PUSH_SYNC=1 を export しているため(L26)、この既存テストにアサーションを1行足すだけで拾えるはずです。

3. MSYSTEM 判定パターンが既存の慣習と不一致 — 優先度低

同リポジトリ内で MSYS 系判定は case "${MSYSTEM:-}" in MINGW*|MSYS*|CLANGARM*) の形で lib/idle-ttl.sh#L63-64lib/instance-id.sh#L42-43 など既に4箇所に出現しています。今回の send.sh#L102-L103CYGWIN* を追加し uname -s フォールバックを足した5つ目の亜種で、検出範囲が微妙に異なります。誤りではないですが、共有ヘルパーに切り出さず定義が分散していく点は気になります。

4. (nit) ドキュメントに遅延の言及がない

Windows でのデフォルトがフォアグラウンドになったことで、send.sh の所要時間はネットワーク push 込みになります(sync_push_remote はレース時最大4回リトライ、都度 sleep 1/2/3 秒)。「bus 到達を保証する」ことは明記されていますが「体感の待ち時間が伸びうる」ことは書かれていないので、一言あってもよさそうです。

結論

コアの修正方針・実装・ドキュメントは妥当でCIもgreenです。ただし懸念点1(既読レシート経路の同型バグ)はissue #31の実データ(message_read 23件滞留)と直接符合するため、これを含めずに#31をクローズするのは早計に見えます。マージ前にこの点だけ判断されることをお勧めします(2, 3は追って/別PRでも問題ないレベルです)。


🤖 Claude Code によるレビュー

@orangewk

Copy link
Copy Markdown
Owner Author

追レビュー(最新Push分)

対象コミット: ca2ea5d "fix(remote): flush Windows read receipts before exit"(前回レビューの直後にpush)

前回指摘の**優先度「高」**だった「sync_push_best_effort()(既読レシート経路)が同型の脆弱性を残したまま」への対応を確認しました。

修正内容 — 妥当

scripts/lib/sync.sh#L171-L186sync_push_best_effort() に、send.sh と完全に同一のロジックを移植しています。

  • unset判定に ${AGMSG_REMOTE_PUSH_SYNC+x} イディオムを使い、「未設定→プラットフォーム判定」「明示設定(空文字含む)→その値を尊重」を正しく分岐
  • MSYS判定 MINGW*|MSYS*|CYGWIN*|CLANGARM* も send.sh と一致
  • override意味論(1=前面, それ以外=背面)も維持
  • コメントも新しい挙動に合わせて更新済み

scripts/send.sh#L92-L113 と突き合わせましたが、ロジックの乖離はありません。sync_push_best_effort が常に || true で失敗を握りつぶす点(send.shは前面push失敗時にexit 1)が唯一の違いですが、既読レシートのbest-effort設計として妥当です。

テスト — 前回テストと同じ手法で妥当

新規テスト inbox: MSYS waits for its read receipt to reach the bus は、send.sh側テストと同じ手法(pre-receive hookにsleep 2env -u AGMSG_REMOTE_PUSH_SYNC MSYSTEM=MINGW64でプラットフォームデフォルト分岐を強制)を踏襲しています。

呼び出し関係も実コードで確認しました。inbox.sh#L86-88check-inbox.sh#L198,209 の両方が sync_mark_readsync_push_best_effort を呼んでおり、既読側も同根という前回の指摘は裏付けられます。新テストは inbox.sh 経由のみのカバレッジで、check-inbox.sh の呼び出し経路自体への直接テストはありません(同一共有関数なのでリスクは低いです)。

新たに生じた懸念(低優先度)

前回指摘3(MSYS判定パターンが idle-ttl.sh / instance-id.sh 等と不統一)について、今回の修正は同じ6行のブロックを send.shsync.sh に一字一句コピペした形です。ロジック一致なのでバグではありませんが、同一PR内で重複が2箇所になったため、共有ヘルパーへの切り出しの根拠はむしろ強まりました。

未対応の前回指摘

  • 指摘2(優先度中): send.sh の前面push失敗時 exit 1 パスへのテストなし
  • 指摘3: 既存4箇所との不統一(上記の通りむしろ重複が増加)
  • 指摘4(nit): 前面push化による体感待ち時間増加のドキュメント補記

いずれも前回レビューが「マージ後/別PRでも可」としていた水準で、ブロッカーではありません。

結論

マージ前に判断すべきとした唯一の指摘(既読レシート経路の同型バグ)は、send.shと一貫したロジックと相応の回帰テストで解消されています。CIも全green。残り3点は軽微で、対応不要と判断するなら現状でマージ可と考えます。


🤖 Claude Code によるレビュー

@orangewk
orangewk marked this pull request as ready for review July 23, 2026 14:35
@orangewk
orangewk merged commit 93de420 into main Jul 23, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

galaxias team: outbound 片方向 — 送信は bus に push されるのに受信側 inbox に届かない

1 participant