Skip to content

fix(chat): init retry, 10-min dedupe TTL, propagate_handler_errors, webhook dedupe option (#191) - #266

Merged
patrick-chinchill merged 6 commits into
mainfrom
sync/4.41-c1b
Sep 30, 2026
Merged

patrick-chinchill merged 6 commits into
mainfrom
sync/4.41-c1b

Conversation

@patrick-chinchill

@patrick-chinchill patrick-chinchill commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

This ports four Chat lifecycle fixes from the 4.41 wave:

  • Init retry. initialize() now retries only after a failed state connection. An adapter-init failure is cached until shutdown().
  • Dedupe TTL. Raised from 5 to 10 minutes. An explicit ChatConfig.dedupe_ttl_ms=0 is now honoured.
  • propagate_handler_errors. By default, wait_until now gets a Task that swallows handler errors. WebhookOptions.propagate_handler_errors=True restores the raw task.
  • deduplicate=False. New WebhookOptions.deduplicate=False skips chat-level dedupe.

The last two are the core halves of upstream's Telegram polling fix.

Upstream commits mapped

Upstream Python
f233ffe8 fix(chat): retry initialization after a failed attempt (#924, chat@4.41.0) _ensure_initialized shares one attempt task across callers. _run_init_attempt awaits state.connect(), and on failure clears _init_promise only if self._init_promise is asyncio.current_task() (upstream: this.initPromise === attempt). Adapter failures stay cached. The _init_lock is removed because it was redundant: there is no await between the check and the assignment.
c21ccbc0 fix(chat): propagate handler errors through waitUntil (#943, chat@4.41.0) WebhookOptions.propagate_handler_errors. _hand_to_wait_until / _tracked apply it to message, action and slash-command. The Slack agent-view rethrow is out of scope (#214).
91683e52 fix(telegram): wait for polling handlers… (#942), core half WebhookOptions.deduplicate. handle_incoming_message is split into _route_incoming_message(…, deduplicate=True) and _dispatch_incoming_message. process_reaction / process_action / process_slash_command return the raw task, and reaction always hands wait_until the wrapper. Telegram polling itself is #227.
0b63791b fix(slack): process Socket Mode retry envelopes (#667), core half DEDUPE_TTL_MS = 10 * 60 * 1000. The Socket Mode half is #209.

Tests ported (tests/test_chat_faithful.py, fidelity-mapped)

  • TestChatInitializationRetry: all 6 tests from describe("Chat initialization retry (#922)").
  • test_should_use_default_dedupe_ttl_of_10_minutes: renamed from ..._5_minutes. It now asserts set_if_not_exists is awaited with 600_000, via an AsyncMock(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_admission

Python-specific tests:

  • test_dedupe_ttl_ms_zero_is_honoured
  • test_cancelling_wait_until_wrapper_leaves_handler_running
  • test_shutdown_cancels_handler_and_wait_until_wrapper
  • test_cancelled_caller_does_not_cancel_shared_attempt
  • tests/test_teams_native_streaming.py::TestHandleMessageActivityWithRealChat: a real Chat combined with the Teams adapter, parametrised on propagate_handler_errors.

Mutation checks: I broke each of the following in turn, and each break fails at least one of the new tests:

  • the identity guard
  • the _tracked shield
  • the _ensure_initialized shield
  • the propagate switch
  • the deduplicate bypass
  • the Teams options spread

Fidelity 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_activity only works if wait_until receives an asyncio.Task: it hooks that task's add_done_callback to know when the handler has finished. Anything else releases processing_done immediately and closes the streamer early.

  • wait_until receives a real Task either way. The wrapper is created with _create_task(..., self._active_tasks) and awaits asyncio.shield(task), so it finishes when the handler finishes. With propagate_handler_errors=True it gets the raw handler task. So the gate still holds the streamer open for the whole handler.
  • The shim used to build WebhookOptions(wait_until=_chained_wait_until) from scratch, which dropped every other caller option. It now uses dataclasses.replace(options or WebhookOptions(), wait_until=_chained_wait_until) (upstream spreads ...baseOptions), so propagate_handler_errors and deduplicate reach Chat.process_message.
  • The new adapter-level test runs a real Chat with an on_direct_message handler. The handler yields several times, then streams, then raises. The test asserts:
    • the stream went native (streamer still registered);
    • emitted == ["hello"] and close_calls == 1;
    • the caller's wait_until got a finished Task that returns None by default, or raises RuntimeError("handler boom") with propagate_handler_errors=True.
  • Reverting the options spread fails the propagate case.
  • All existing native-streaming tests pass unchanged.
  • The Teams dialog-open parts of c21ccbc0 / 91683e52 don'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.md under "Chat lifecycle: init retry, dedupe TTL, wait_until errors". None of them is a new non-parity-table divergence.

  • Callers await the init attempt through asyncio.shield, so a cancelled caller does not cancel the attempt that other callers share (an asyncio cancellation hazard).
  • _do_initialize logs an adapter-init failure at error level; upstream only rejects. The issue asked for this because a transient failure now wedges the instance until shutdown().
  • _tracked is a shielded wrapper Task instead of a .catch promise. It must be a Task for the Teams gate, and cancelling it must not cancel the handler.
  • The deduplicate=False path has no runInConversation wrapper, because Python has no conversation context yet ([4.41/C3] Conversation context + AI tool scoping (read & write guards, strict_scope) #195).
  • Log metadata keys stay snake_case: "Action processing error" gains action_id / message_id, and "Slash command processing error" gains command / text.

Consumer impact (high)

  • wait_until now swallows handler errors by default. Hosts that awaited the wait_until awaitable to see handler errors must pass WebhookOptions(propagate_handler_errors=True). The task returned by process_* still raises.
  • process_reaction / process_action / process_slash_command return the handler task instead of None, and the ChatInstance protocol types are updated to match.
  • The dedupe TTL is now 10 minutes. ChatConfig.dedupe_ttl_ms defaults to None.
  • An adapter initialize() failure is cached until shutdown(). Before, any failure was retried, which re-initialized adapters that were already running. A state-connect failure is still retried.

Validation

  • ruff check and format: clean.
  • audit_test_quality: 0 hard failures.
  • --check-docs: OK.
  • --strict at chat@4.31.0: 733/733.
  • pytest: 6222 passed, 24 skipped.
  • pyrefly check: 0 errors.

Closes #191
Part of #184

Merge gate

Independent review findings (6): all fixed, none declined.

  • Shutdown test now asserts tasks[0].cancelled(), which kills the "untracked wrapper" mutation.
  • dedupe_ttl_ms=0 is documented in CHANGELOG, the types.py comment and UPSTREAM_SYNC: the bundled backends treat a 0 TTL as no expiry (upstream parity, no code change).
  • New test test_reaction_and_lifecycle_wait_until_ignores_propagate_handler_errors kills mutations M5 and M6.
  • New test test_cancelled_handler_completes_wait_until_wrapper_normally kills the "always re-raise" mutation (M1).
  • Orphaned init failure: the attempt's exception is now marked retrieved with a done-callback, so "Task exception was never retrieved" no longer fires. Pinned by test_orphaned_failed_attempt_does_not_leak_unretrieved_exception, which installs a loop exception handler.
  • The adapter-init error log is now asserted in test_does_not_restart_an_initialized_adapter_when_another_adapter_fails, which kills M4.
  • A mutation check confirmed that each new or tightened test fails when its mutation is applied.

gpt-6-astra: 3 rounds, final verdict clean on fe98a1b.

Round 1 raised 3 findings:

  • Fixed: an eager task factory could cache a failed connect(). The attempt is now built with asyncio.Task(...), and the fix is pinned by test_retries_state_connection_under_eager_task_factory.
  • Fixed: the Teams DM gate was tied to the cancellable wrapper. It now gates on the handler task that process_message returns, pinned by test_cancelled_wait_until_wrapper_keeps_dm_streamer_open.
  • Rebutted as upstream parity: "cancel the shielded init during shutdown". Upstream chat.ts shutdown() (4.41.1 L542-565) only nulls initPromise and never cancels the in-flight attempt, and the ported test test_keeps_a_newer_attempt_when_a_preshutdown_state_connection_rejects requires that attempt to settle for its own caller. A comment in shutdown() 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:

  • ruff check and format
  • audit (0 hard failures)
  • --check-docs
  • strict fidelity (all TS tests have Python equivalents)
  • pytest: 6342 passed, 24 skipped
  • pyrefly: 0 errors
  • target fidelity delta: missing 234 -> 234 (+0), unchanged by these follow-ups

…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
@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 65c84768-c6db-48da-bbb7-fee3b7078ef7

📥 Commits

Reviewing files that changed from the base of the PR and between 438a35c and f00b27d.

📒 Files selected for processing (8)
  • CHANGELOG.md
  • docs/UPSTREAM_SYNC.md
  • scripts/fidelity_target.json
  • src/chat_sdk/adapters/teams/adapter.py
  • src/chat_sdk/chat.py
  • src/chat_sdk/types.py
  • tests/test_chat_faithful.py
  • tests/test_teams_native_streaming.py
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@patrick-chinchill
patrick-chinchill marked this pull request as ready for review September 30, 2026 18:57
@patrick-chinchill

Copy link
Copy Markdown
Collaborator Author

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).

@patrick-chinchill
patrick-chinchill merged commit 4b1e7c5 into main Sep 30, 2026
7 checks passed
@patrick-chinchill
patrick-chinchill deleted the sync/4.41-c1b branch September 30, 2026 19:23
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.

[4.41/C1b] Core lifecycle: init retry, dedupe TTL 10min, propagate_handler_errors, webhook dedupe option

1 participant