Repository navigation
fix(chat): init retry, 10-min dedupe TTL, propagate_handler_errors, webhook dedupe option (#191) - #266
Conversation
…ebhook dedupe option (#191) Ports upstream f233ffe8 (#924), c21ccbc0 (#943) and the core halves of 91683e52 (#942) and 0b63791b (#667). - Init: retry only after a failed state connect (identity-guarded); adapter init failures stay cached until shutdown(). - DEDUPE_TTL_MS 5 -> 10 min; ChatConfig.dedupe_ttl_ms defaults to None and resolves with 'is not None'. - wait_until receives an error-swallowing wrapper Task by default; WebhookOptions.propagate_handler_errors hands over the raw task for message/action/slash-command. process_reaction/action/slash_command return the handler task. - WebhookOptions.deduplicate=False bypasses chat-level dedupe; handle_incoming_message split into _route/_dispatch. - Teams DM shim spreads caller WebhookOptions. Closes #191
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 33 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ts, document dedupe_ttl_ms=0
…ndler task, shutdown parity note
# Conflicts: # docs/UPSTREAM_SYNC.md
|
Merge gate: CI green on f00b27d (Analyze actions/python, CodeQL, Lint & Type Check, test 3.12, test 3.13). Local Codex review (gpt-6-astra, xhigh, --base origin/main) on fe98a1b: "No actionable regressions found." That took 3 astra rounds. f00b27d only merges origin/main (#212, #220, #270) into fe98a1b. The one conflict was in docs/UPSTREAM_SYNC.md: I kept both sections. src/, tests/ and scripts/ merged cleanly, and no module overlaps with the PR (chat.py, types.py and teams/adapter.py are untouched on main). fidelity_target.json regenerated with no change (missing 234 -> 234). Local full validation: 6485 passed, strict fidelity 733/733, 0 pyrefly errors. Bots: CodeRabbit rate-limited, no reviews. Merging with --admin (Protect Main requires a code-owner approval). |
Summary
This ports four
Chatlifecycle fixes from the 4.41 wave:initialize()now retries only after a failed state connection. An adapter-init failure is cached untilshutdown().ChatConfig.dedupe_ttl_ms=0is now honoured.propagate_handler_errors. By default,wait_untilnow gets a Task that swallows handler errors.WebhookOptions.propagate_handler_errors=Truerestores the raw task.deduplicate=False. NewWebhookOptions.deduplicate=Falseskips chat-level dedupe.The last two are the core halves of upstream's Telegram polling fix.
Upstream commits mapped
f233ffe8fix(chat): retry initialization after a failed attempt (#924, chat@4.41.0)_ensure_initializedshares one attempt task across callers._run_init_attemptawaitsstate.connect(), and on failure clears_init_promiseonly ifself._init_promise is asyncio.current_task()(upstream:this.initPromise === attempt). Adapter failures stay cached. The_init_lockis removed because it was redundant: there is no await between the check and the assignment.c21ccbc0fix(chat): propagate handler errors through waitUntil (#943, chat@4.41.0)WebhookOptions.propagate_handler_errors._hand_to_wait_until/_trackedapply it to message, action and slash-command. The Slack agent-view rethrow is out of scope (#214).91683e52fix(telegram): wait for polling handlers… (#942), core halfWebhookOptions.deduplicate.handle_incoming_messageis split into_route_incoming_message(…, deduplicate=True)and_dispatch_incoming_message.process_reaction/process_action/process_slash_commandreturn the raw task, and reaction always handswait_untilthe wrapper. Telegram polling itself is #227.0b63791bfix(slack): process Socket Mode retry envelopes (#667), core halfDEDUPE_TTL_MS = 10 * 60 * 1000. The Socket Mode half is #209.Tests ported (
tests/test_chat_faithful.py, fidelity-mapped)TestChatInitializationRetry: all 6 tests fromdescribe("Chat initialization retry (#922)").test_should_use_default_dedupe_ttl_of_10_minutes: renamed from..._5_minutes. It now assertsset_if_not_existsis awaited with600_000, via anAsyncMock(wraps=...)spy. At the 4.31 pin, the old TS name still fuzzy-matches it, so strict stays at 733/733.test_should_optionally_propagate_handler_errors_through_waituntil: message, action and slash-command × propagate False/True. Each case checks the direct result, the background outcome and the error-log metadata.test_lets_transports_own_deduplication_when_retrying_admissionPython-specific tests:
test_dedupe_ttl_ms_zero_is_honouredtest_cancelling_wait_until_wrapper_leaves_handler_runningtest_shutdown_cancels_handler_and_wait_until_wrappertest_cancelled_caller_does_not_cancel_shared_attempttests/test_teams_native_streaming.py::TestHandleMessageActivityWithRealChat: a realChatcombined with the Teams adapter, parametrised onpropagate_handler_errors.Mutation checks: I broke each of the following in turn, and each break fails at least one of the new tests:
_trackedshield_ensure_initializedshielddeduplicatebypassFidelity target:
Delta vs committed report (HEAD): missing 242 -> 234 (-8)(packages/chat/src/chat.test.ts: 23 -> 15). The 10-minute TTL test was already fuzzy-matched, so it is not counted in the delta.Teams interaction
The Teams DM native-streaming gate in
_handle_message_activityonly works ifwait_untilreceives anasyncio.Task: it hooks that task'sadd_done_callbackto know when the handler has finished. Anything else releasesprocessing_doneimmediately and closes the streamer early.wait_untilreceives a real Task either way. The wrapper is created with_create_task(..., self._active_tasks)and awaitsasyncio.shield(task), so it finishes when the handler finishes. Withpropagate_handler_errors=Trueit gets the raw handler task. So the gate still holds the streamer open for the whole handler.WebhookOptions(wait_until=_chained_wait_until)from scratch, which dropped every other caller option. It now usesdataclasses.replace(options or WebhookOptions(), wait_until=_chained_wait_until)(upstream spreads...baseOptions), sopropagate_handler_errorsanddeduplicatereachChat.process_message.Chatwith anon_direct_messagehandler. The handler yields several times, then streams, then raises. The test asserts:native(streamer still registered);emitted == ["hello"]andclose_calls == 1;wait_untilgot a finished Task that returnsNoneby default, or raisesRuntimeError("handler boom")withpropagate_handler_errors=True.c21ccbc0/91683e52don't apply here, because dialog-open inbound is not ported. I added a row for this to the non-parity table.Divergences / Python-specific choices
These are documented in
docs/UPSTREAM_SYNC.mdunder "Chat lifecycle: init retry, dedupe TTL,wait_untilerrors". None of them is a new non-parity-table divergence.asyncio.shield, so a cancelled caller does not cancel the attempt that other callers share (an asyncio cancellation hazard)._do_initializelogs an adapter-init failure at error level; upstream only rejects. The issue asked for this because a transient failure now wedges the instance untilshutdown()._trackedis a shielded wrapper Task instead of a.catchpromise. It must be a Task for the Teams gate, and cancelling it must not cancel the handler.deduplicate=Falsepath has norunInConversationwrapper, because Python has no conversation context yet ([4.41/C3] Conversation context + AI tool scoping (read & write guards, strict_scope) #195).action_id/message_id, and "Slash command processing error" gainscommand/text.Consumer impact (high)
wait_untilnow swallows handler errors by default. Hosts that awaited thewait_untilawaitable to see handler errors must passWebhookOptions(propagate_handler_errors=True). The task returned byprocess_*still raises.process_reaction/process_action/process_slash_commandreturn the handler task instead ofNone, and theChatInstanceprotocol types are updated to match.ChatConfig.dedupe_ttl_msdefaults toNone.initialize()failure is cached untilshutdown(). Before, any failure was retried, which re-initialized adapters that were already running. A state-connect failure is still retried.Validation
audit_test_quality: 0 hard failures.--check-docs: OK.--strictatchat@4.31.0: 733/733.pyrefly check: 0 errors.Closes #191
Part of #184
Merge gate
Independent review findings (6): all fixed, none declined.
tasks[0].cancelled(), which kills the "untracked wrapper" mutation.dedupe_ttl_ms=0is documented in CHANGELOG, thetypes.pycomment and UPSTREAM_SYNC: the bundled backends treat a0TTL as no expiry (upstream parity, no code change).test_reaction_and_lifecycle_wait_until_ignores_propagate_handler_errorskills mutations M5 and M6.test_cancelled_handler_completes_wait_until_wrapper_normallykills the "always re-raise" mutation (M1).test_orphaned_failed_attempt_does_not_leak_unretrieved_exception, which installs a loop exception handler.test_does_not_restart_an_initialized_adapter_when_another_adapter_fails, which kills M4.gpt-6-astra: 3 rounds, final verdict clean on
fe98a1b.Round 1 raised 3 findings:
connect(). The attempt is now built withasyncio.Task(...), and the fix is pinned bytest_retries_state_connection_under_eager_task_factory.process_messagereturns, pinned bytest_cancelled_wait_until_wrapper_keeps_dm_streamer_open.chat.tsshutdown()(4.41.1 L542-565) only nullsinitPromiseand never cancels the in-flight attempt, and the ported testtest_keeps_a_newer_attempt_when_a_preshutdown_state_connection_rejectsrequires that attempt to settle for its own caller. A comment inshutdown()records this.Rounds 2 and 3 (round 3 ran after merging
origin/main) found no actionable issues.Bots: CodeRabbit was rate-limited and left no review. No gemini comments and no inline comments.
CI: all green on
fe98a1b: Lint & Type Check, test (3.12), test (3.13), CodeQL and Analyze.Local validation was run on
fe98a1b. Everything passed:--check-docsmissing 234 -> 234 (+0), unchanged by these follow-ups