Skip to content

feat(servo): add poisoning mechanism for repeated render panics#379

Merged
streamer45 merged 1 commit intomainfrom
devin/1777128736-servo-poison-and-tests
Apr 25, 2026
Merged

feat(servo): add poisoning mechanism for repeated render panics#379
streamer45 merged 1 commit intomainfrom
devin/1777128736-servo-poison-and-tests

Conversation

@staging-devin-ai-integration
Copy link
Copy Markdown
Contributor

Summary

Adds a poisoning mechanism to the Servo plugin's shared renderer thread to protect against instances that repeatedly panic during rendering.

After POISON_THRESHOLD (5) consecutive render panics, an instance is marked as poisoned: Servo calls are skipped entirely and the cached frame is returned. This prevents a single broken page from repeatedly triggering panics that consume resources on the shared thread.

Changes (servo_thread.rs)

  • POISON_THRESHOLD constant (5) — configurable threshold for consecutive panics.
  • consecutive_panic_count / poisoned fields on InstanceState — per-instance tracking.
  • Render path: poisoned instances short-circuit to send_fallback_frame before catch_unwind. On panic, the counter increments; at threshold, tracing::error! is emitted and the instance is poisoned. On success, the counter resets to 0.
  • UpdateConfig path: a URL change resets poisoned and consecutive_panic_count with tracing::info!, giving the instance a fresh start with the new page.
  • Module doc updated to describe the poisoning hardening.

Config tests (#347)

All requested config validation and runtime update logic tests already exist on main in config.rs (34 tests covering validate(), parse_resolution(), merge_update(), effective_viewport_width/height(), needs_scaling()). They all pass with this change.

Review & Testing Checklist for Human

  • Verify the poisoning logic in the Render match arm: counter increments on panic, resets on success, poisons at threshold
  • Verify the UpdateConfig poison reset only triggers on actual URL changes (not CSS-only updates)
  • Confirm POISON_THRESHOLD = 5 is a reasonable default for your use case

Notes

Link to Devin session: https://staging.itsdev.in/sessions/712b2767284e427a8dc6544eafeb87ef
Requested by: @streamer45

After POISON_THRESHOLD (5) consecutive render panics, mark the instance
as poisoned.  Poisoned instances skip Servo calls entirely and return
the cached frame until a URL change (UpdateConfig) resets the state.

- Add consecutive_panic_count and poisoned fields to InstanceState
- Check poison state before Render catch_unwind; skip to fallback
- Increment counter on panic; mark poisoned at threshold with
  tracing::error!
- Reset counter to 0 on successful render
- Reset poison state + counter on URL change with tracing::info!
- Update module doc to describe the poisoning hardening

Closes #348
Closes #347

Signed-off-by: StreamKit Devin <devin@streamkit.dev>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
@staging-devin-ai-integration
Copy link
Copy Markdown
Contributor Author

staging-devin-ai-integration Bot commented Apr 25, 2026

✅ Reviewed on bbe68f3

View review

@staging-devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown
Contributor Author

@staging-devin-ai-integration staging-devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review (Staging)
Debug

Playground

@staging-devin-ai-integration
Copy link
Copy Markdown
Contributor Author

Review

Overall: Good design. The poisoning mechanism is well-scoped — per-instance tracking, threshold-based, auto-reset on URL change.

Must fix before merge

  1. Unrelated CI commits in branch. Same issue as the other PRs — includes d3acb21 (PR ci: enable sccache GHA backend and optimize rust-cache keys #372) and 30d0b28 (PR ci: remove SCCACHE_GHA_ENABLED to stop cache entry explosion #373). Please rebase onto main.

Should address (or file tracking issue)

  1. No tests for the new poisoning logic. The PR description notes that the 34 existing config tests pass, but none of them cover the new poisoning mechanism. Per repo conventions, fixes should include tests that prevent the same class of bug from recurring. The servo_thread_main function is hard to unit test directly (requires Servo), but at minimum:

    • Extract the poisoning state machine (increment → threshold check → poison → reset on URL change) into a testable struct/method
    • Or add integration-level tests that simulate consecutive panics and verify the fallback behavior

    If this is too invasive for this PR, file a tracking issue.

Design questions (non-blocking)

  1. URL-only reset. Currently only a URL change resets poison state. If a Servo page panics due to a CSS rendering bug and the user fixes the CSS (same URL, different content), the instance stays poisoned until the page is re-registered with a different URL. Is this the intended behavior, or should any UpdateConfig reset the state?

  2. The POISON_THRESHOLD = 5 constant is well-documented. Consider whether this should be configurable via ServoConfig in the future.

@streamer45 streamer45 merged commit 29c096e into main Apr 25, 2026
12 checks passed
@streamer45 streamer45 deleted the devin/1777128736-servo-poison-and-tests branch April 25, 2026 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants