Skip to content

Correct chaos latency and percentage semantics - #331

Open
eyalb181 wants to merge 4 commits into
mainfrom
cor-1817-chaos-latency-docs
Open

eyalb181 wants to merge 4 commits into
mainfrom
cor-1817-chaos-latency-docs

Conversation

@eyalb181

Copy link
Copy Markdown
Member

Three corrections to the chaos testing page, all found while testing the feature against a live cluster. Each one describes behavior a reader would otherwise mistake for a bug.

percentage is per operation, not per connection

The page said "roughly how often a matched connection gets the effect". The roll actually happens on each matched operation, so a request that reads several times can be affected on some reads and not others, and the share of affected requests does not track the configured number as closely as that implied.

Measured: at percentage: 30 with an 8s read latency, twelve requests produced a mix of unaffected responses, requests whose first attempt was hit and recovered on retry, and one request hit on both attempts.

read_ms and write_ms are charged per operation

This is the most common surprise. A request that performs several reads pays read_ms on each one, so the delay a request sees can be a multiple of the configured value. Added a warning saying to set these by measuring the effect you want rather than assuming a single charge.

Measured: a Go service reading through a pooled Postgres connection took ~6.3s under read_ms: 3000, because the query performs more than one read.

after_ms above 0 prevents client-side timeouts from firing

The page documented after_ms only through an example value of 0, with no description. It now says what the field does, and warns that a value above 0 holds the calling thread for the length of the delay, so a timeout implemented as a timer will not fire while the fault is in effect.

Measured: connection_error type reset with after_ms: 5000 against a client bounded at 2s produced 7.25s and 16.04s per request, with the bound never taking effect. The same setup at after_ms: 0 returns in 0.19 to 0.39s.

Tracked as COR-1819. The guidance is to use after_ms: 0 when testing timeout handling, and it should be removed from the docs once that is fixed.

Note on the per-operation latency wording

A separate fix removes a second latency charge that was applied when a connection is opened, so once that ships a read_ms value costs its delay once per operation rather than twice on a new connection. The per-operation wording added here is correct both before and after that change, so this page does not need to be held back for it.

🤖 Generated with Claude Code

Three corrections to the chaos testing page, all found while testing the
feature against a live cluster.

`percentage` is rolled on each matched operation rather than once per
connection, so the share of affected requests does not track the
configured number as closely as the previous wording implied.

`read_ms` and `write_ms` are charged per operation. A request that reads
several times pays the delay on each read, which is the most common
surprise when a rule seems stronger than configured.

`after_ms` above 0 holds the calling thread for the length of the delay,
so client-side timeouts cannot fire while the fault is in effect. Readers
testing timeout handling should use `after_ms: 0`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@linear-code

linear-code Bot commented Aug 21, 2026

Copy link
Copy Markdown

COR-1817

@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR corrects the chaos-testing documentation to explain that percentage selection and latency apply per operation rather than per connection or request, and warns that delayed connection errors can prevent client-side timers from firing.

  • Clarifies operation-level percentage selection.
  • Explains cumulative read and write latency.
  • Documents after_ms behavior and recommends zero delay when testing timeout handling.

Important Files Changed

Filename Overview
docs/use-cases/chaos-testing.md The revised chaos semantics and timeout guidance are consistent with the documented intent and no actionable defect remains.

Reviews (1): Last reviewed commit: "Correct chaos latency and percentage sem..." | Re-trigger Greptile

Comment thread docs/use-cases/chaos-testing.md Outdated
Comment thread docs/use-cases/chaos-testing.md Outdated
Comment thread docs/use-cases/chaos-testing.md Outdated
Replaces "charged per operation" with what actually happens to a request,
and replaces "measuring the effect you want" with the concrete step:
try a value and time a real request.

Removes the after_ms warning at review request. The field description
stays.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread docs/use-cases/chaos-testing.md Outdated
eyalb181 and others added 2 commits August 21, 2026 18:24
Says what happens instead: the percentage is decided separately for each
read and each write. Matches the wording used for latency in the same
page.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3.250.0 removed the delayed send, so a connection_error now fires as soon
as the connection is attempted. Rules still accept after_ms and it is
ignored, which is worth saying rather than leaving readers to infer from
a value that does nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@trawler trawler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Checked the three claims against mirrord main (as of today) and tag 3.250.0. Percentage per read/write/connect is correct. Two wording issues inline, one on the after_ms hint that contradicts the code.

Also, the PR description is stale: it still says after_ms above 0 blocks client-side timeouts and should be removed once fixed. dd3d227 flipped the doc to say the opposite. Worth updating the body before merge so the history matches the change.

- `connection_error`: fails the connection. `type` can be one of: `reset` (can be applied to ongoing connections), `timed_out`, `refused`. The error fires as soon as the connection is attempted.

{% hint style="info" %}
Rules still accept an `after_ms`, which was intended to hold the connection open before failing it. It currently has no effect and the error fires immediately.

@trawler trawler Sep 3, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only true for new connections. On a connection that is already open, after_ms still delays the reset or the stall, see outgoing.rs:888 on main. metalbear-co/mirrord#4752 only removed the delay on the connect path and left this one unchanged.

Read from the code, not measured. Suggested wording:

after_ms is ignored when the rule fails a new connection: the error fires as soon as the connection is attempted. It still delays the error when the rule hits a connection that is already open.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now this could be a "warning" box.


{% hint style="warning" %}
`read_ms` and `write_ms` apply to each read and each write, not to each request. A request
that reads three times is delayed by three times `read_ms`, so a request is usually delayed

@trawler trawler Sep 3, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggest: "A request that does several round trips on one connection pays the delay on each of them, so a request is usually delayed by more than the number you set."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think just karen's text should be enough for this, the rest about "pick a value by yadda yadda yadda", I don't think it's necessary to say.

Also, do we want this to be a "warning"? Shouldn't it be an info box?

```

- `connection_error`: fails the connection. `type` can be one of: `reset` (can be applied to ongoing connections), `timed_out`, `refused`.
- `connection_error`: fails the connection. `type` can be one of: `reset` (can be applied to ongoing connections), `timed_out`, `refused`. The error fires as soon as the connection is attempted.

@trawler trawler Sep 3, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two things on this line, since it is already in the diff:

  1. timed_out also applies to ongoing connections, not only reset. The ongoing-connection path matches both Reset and TimedOut, see chaos_connection_error_for_ongoing_connection. Only refused is connect-only.
  2. "The error fires as soon as the connection is attempted" sits next to "can be applied to ongoing connections" and reads as a contradiction. Suggest moving the fires-immediately statement into the hint below, scoped to new connections.

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.

3 participants