Skip to content

fix(discord): tolerate poisoned delivery-state mutex - #352

Merged
Yeachan-Heo merged 1 commit into
devfrom
fix/351-discord-poison-tolerant-state
Sep 3, 2026
Merged

Yeachan-Heo merged 1 commit into
devfrom
fix/351-discord-poison-tolerant-state

Conversation

@Yeachan-Heo

Copy link
Copy Markdown
Owner

Fixes #351.

What

src/discord.rs was the only production path in the crate locking its shared Mutex with expect(...) (5 sites: allow_request, rate_limit_delay, record_success, record_failure, DLQ bury). All five now go through one poison-tolerant state() accessor using PoisonError::into_inner.

Why

DiscordState guards the rate limiter, the per-target circuit breakers, and the DLQ buffer. One panic unwinding while that guard was held poisoned the mutex, so every later Discord delivery decision and DLQ bury panicked for the rest of the daemon's lifetime — permanently losing the whole Discord lane, including the DLQ capture that exists precisely to preserve undelivered messages.

The rest of the crate already tolerates poisoning (daemon.rs, dispatch.rs, gjc_lane.rs, source/subscription.rs, source/git.rs, source/tmux.rs, lifecycle.rs), so this aligns Discord with the existing convention rather than introducing a new policy. All three guarded structures are individually recoverable, so recovery degrades to possibly-stale counters instead of an unrecoverable panic loop.

Verification

  • cargo test --bin clawhip → 1101 passed, 0 failed
  • cargo fmt --check → clean
  • cargo clippy --all-targets -- -D warnings → clean
  • New regression test discord::tests::poisoned_delivery_state_still_serves_limiter_circuit_and_dlq poisons the state from a panicking thread, then asserts the limiter delay, the circuit-breaker open transition, and the DLQ path all still work.
  • Negative control: with the accessor temporarily reverted to expect(...), that test fails with panicked at src/discord.rs:751: discord state lock: PoisonError { .. }, confirming the test actually covers the regression.

Base dev@c4774562c6b073d4d6e1481aeb10ee8aa68afbad. No config/schema/behavioral change on the healthy path.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

`src/discord.rs` was the only production path locking its shared state with
`expect(...)`. Since that state guards the rate limiter, the per-target
circuit breakers, and the DLQ buffer, a single panic unwinding inside any
critical section poisoned the mutex and made every later `allow_request`,
`rate_limit_delay`, `record_success`, `record_failure`, and DLQ bury panic
for the remaining lifetime of the daemon -- permanently destroying the whole
Discord delivery lane, including the DLQ capture meant to preserve
undelivered messages.

Route all five sites through a single poison-tolerant `state()` accessor
(`PoisonError::into_inner`), matching the convention already used by the
daemon, dispatch, lane, subscription, git, tmux, and lifecycle paths. All
three guarded structures are individually recoverable, so recovery degrades
to possibly-stale counters instead of an unrecoverable panic loop.

Adds a regression test that poisons the state from a panicking thread and
asserts the limiter, circuit-breaker open transition, and DLQ paths keep
working. The test panics with `PoisonError` against the previous code.

Fixes #351
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Yeachan-Heo
Yeachan-Heo merged commit 7bad9d2 into dev Sep 3, 2026
13 checks passed
@Yeachan-Heo
Yeachan-Heo deleted the fix/351-discord-poison-tolerant-state branch September 3, 2026 10:05
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.

1 participant