test(gateway): add coverage for hybrid LINE reply/push dispatch#622
Closed
chaodu-agent wants to merge 16 commits intomainfrom
Closed
test(gateway): add coverage for hybrid LINE reply/push dispatch#622chaodu-agent wants to merge 16 commits intomainfrom
chaodu-agent wants to merge 16 commits intomainfrom
Conversation
This implementation adds a stateful token cache in the gateway to prioritize the free LINE Reply API. It also includes an auto-fill logic for the reply_to field to avoid modifying the OAB core.
- Remove auto-fill reply_to mechanism that could attach replies to wrong LINE messages under concurrent traffic (blocker from shaun-agent, 超渡法師, 普渡法師). If OAB sends empty reply_to, gateway now correctly falls back to Push API instead of guessing the latest event. - Extract reply token from cache before making HTTP call so Mutex is not held across network I/O (blocker from shaun-agent, 超渡法師, 普渡法師). - Unify sweep TTL with handler TTL (REPLY_TOKEN_TTL_SECS) to eliminate dead-weight cache entries in 50-60s range (普渡法師). - Add secrets.env to .gitignore (shaun-agent, 超渡法師). - Run rustfmt --edition 2021 (shaun-agent). - Update ADR Consequences to reflect hybrid reply/push strategy (超渡法師).
…antics LINE reply tokens are single-use. Document that the first OAB client to remove() a cached token gets the free Reply API call; others fall back to Push API. This is correct behavior but was not documented, which could confuse maintainers (普渡法師).
The Per-Client Last Event Tracker was removed during review because it caused incorrect reply-token association in group chats. Update the feature request doc to match the actual implementation: gateway does not infer reply_to, it falls back to Push API when reply_to is empty or not found in cache (普渡法師 doc nit).
The 4-step hybrid strategy description still referenced the removed Per-Client Last Event Tracker auto-fill mechanism. Updated to accurately describe the actual implementation: event_id-keyed cache lookup with empty reply_to falling back to Push API.
- Switch ReplyTokenCache from tokio::sync::Mutex to std::sync::Mutex; critical sections are short and never held across .await (超渡法師). - Log LINE Reply API 400 response body for easier debugging (超渡法師). - Guard secrets.env source in run_gateway.sh with existence check (超渡法師). - Align sweep interval with REPLY_TOKEN_TTL_SECS (50s) (超渡法師).
- Only fallback to Push API on 4xx (token invalid/expired). For 5xx and network errors, the message may have already been delivered — do NOT fallback to avoid duplicate delivery (擺渡法師 major finding). - Update ADR message flow step 7 to reflect hybrid Reply/Push strategy instead of Push-only (擺渡法師 doc nit).
Only fallback to Push API when LINE's 400 response body explicitly indicates the reply token is invalid or expired. All other failures (5xx, other 4xx, network/timeout errors) are treated as ambiguous delivery — logged as error and dropped to prevent duplicate delivery (擺渡法師 refined major finding, 普渡法師 concurred).
- Fix operator precedence bug in Reply API 400 fallback: add explicit parentheses so (invalid AND reply token) OR expired is evaluated correctly - Change lock().unwrap() to lock().unwrap_or_else(|e| e.into_inner()) at all 3 cache lock sites to recover from mutex poison instead of panicking - Add REPLY_TOKEN_CACHE_MAX (10k) bound: skip insert and log warning when cache is full, gracefully degrading to Push API
Feature is now implemented and documented in docs/adr/line-adapter.md. Keeping a feature request doc for a shipped feature is misleading.
…elog - Status: Proposed → Accepted - Add sequence diagram showing hybrid Reply/Push dispatch flow - Update changelog to v0.2 with #608 and #619 references - Add @iamninihuang as co-author
- Remove run_gateway.sh (README already documents how to run; script used port 9090 conflicting with README default 8080) - Add LINE_CHANNEL_SECRET and LINE_CHANNEL_ACCESS_TOKEN to env vars table - Add POST /webhook/line to endpoints table
Extract dispatch_line_reply() into a testable function and add 6 tests covering the full decision matrix: - cache hit → Reply API only - cache miss → Push API fallback - expired token → Push API fallback - Reply 400 invalid token → Push API fallback - Reply 5xx → no fallback (duplicate risk) - network error → no fallback (duplicate risk) Uses wiremock for HTTP mocking with scoped expectations. Closes #620
github-actions
Bot
added
closing-soon
|
All PRs must reference a prior Discord discussion to ensure community alignment before implementation. Please edit the PR description to include a link like: This PR will be automatically closed in 3 days if the link is not added. |
3 tasks
Collaborator
Author
|
Superseded by #623 — rebased onto a clean branch from main to remove unrelated diff noise. |
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
2 participants
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.
What problem does this solve?
Issue #620 — the hybrid LINE reply/push strategy from PR #608 lacks focused test coverage for the dispatch decision logic. This is correctness-sensitive because it decides when to use the free Reply API, when to fall back to Push API, and when not to fall back to avoid duplicate delivery.
Closes #620
Proposed Solution
Extract
dispatch_line_reply()— pulled the LINE dispatch branch out of the WebSocket recv_task closure into a standalone async function with anapi_baseparameter for testability.6 test cases using
wiremockfor HTTP mocking with scoped expectations (expect(N)auto-verified on drop):cache_hit_uses_reply_apicache_miss_uses_push_apiexpired_token_uses_push_apireply_400_invalid_token_falls_back_to_pushreply_5xx_does_not_fallbackreply_network_error_does_not_fallbackWhy this approach?
api_baseparameter is the minimal refactor needed to make the logic testable without changing behavior.wiremockscoped mocks verify both positive (was called) and negative (was NOT called) expectations, which is critical for the "no fallback on 5xx/network error" safety invariant.Test Plan
cargo buildpassescargo testpasses — all 6 tests green