補丁加入 sleep：如何在合併前審查計時與重試改動

The Patch Added a Sleep: Reviewing Timing and Retry Hunks Before Merge

Conclusion first: most arguments about "flaky tests" start after merge. The cheaper intervention is earlier — read what the patch did to timing, retries, and skips, then repeat only the touched tests on both trees. That is one regex pass, one sweep, and one waiver file that expires.

1. Why the timing surface deserves its own review step

A patch that turns a red test green by adding a retry has not made the code deterministic. It has converted a reproducible failure into a probability distribution, and probability distributions are much harder to review.

After the retry lands, the test's pass/fail signal no longer means what its name says. A later regression smaller than the retry budget becomes invisible: the test still passes, just slower and after two extra attempts.

This failure mode is measurable, which is the useful part. Pass rate goes up, latency distribution goes up, and the assertion set usually stays the same or shrinks.

2. Step 1 — Extract the timing surface from the diff

The first pass is static and takes no test execution. Scan added lines only, because a patch's risk lives in what it introduces, not in what it inherits.

#!/usr/bin/env python3
"""timing_surface.py — extract the timing/retry surface of a patch.

Usage: python timing_surface.py <base-ref> <head-ref>
Output: JSON on stdout. Exit code 1 when any hit exists, 0 when clean.
"""
import json
import re
import subprocess
import sys

RULES = {
    "sleep":   re.compile(r"\b(?:time\.sleep|Thread\.sleep|sleep|setTimeout|time\.Sleep)\s*\(\s*[0-9.]"),
    "retry":   re.compile(r"\b(?:retry|retries|max_attempts|backoff|attempts)\b", re.I),
    "timeout": re.compile(r"\b(?:timeout|deadline|ttl)\b\s*[=:]\s*[0-9]"),
    "poll":    re.compile(r"\b(?:poll_interval|interval_ms|wait_for|eventually)\b", re.I),
    "skip":    re.compile(r"(pytest\.mark\.(?:flaky|skip)|@unittest\.skip|t\.Skip\()"),
}


def added_lines(base, head):
    patch = subprocess.run(
        ["git", "diff", "-U0", "--no-color", f"{base}...{head}"],
        check=True, capture_output=True, text=True,
    ).stdout
    path, lineno = None, 0
    for raw in patch.splitlines():
        if raw.startswith("+++ b/"):
            path = raw[len("+++ b/"):]
        elif raw.startswith("@@"):
            m = re.search(r"\+(\d+)", raw)
            lineno = int(m.group(1)) if m else 0
        elif raw.startswith("+") and not raw.startswith("+++"):
            yield path, lineno, raw[1:]
            lineno += 1


def main(base, head):
    hits = [
        {"rule": name, "file": path, "line": lineno, "text": text.strip()[:200]}
        for path, lineno, text in added_lines(base, head)
        for name, rule in RULES.items()
        if rule.search(text)
    ]
    json.dump({"base": base, "head": head, "hits": hits}, sys.stdout, indent=2)
    print()
    return 1 if hits else 0


if __name__ == "__main__":
    sys.exit(main(sys.argv[1], sys.argv[2]))

Run it as python timing_surface.py origin/main HEAD. The output is a review queue, not a verdict — most hits are legitimate.

3. Step 2 — Classify each hit before looking at test results

Classification is the part that decides whether the sweep is even worth running. Label every hit as test-only accommodation, product-level change, or removal of an oracle.

A patch with zero hits can skip steps 3 and 4 entirely. That is the common case, and skipping it is why this stays cheap.

4. Step 3 — Repeat the touched tests on both trees

Create two worktrees so base and head never share a cache directory, then run the touched node IDs several times in each.

git worktree add /tmp/tf-base "$(git merge-base origin/main HEAD)"
git worktree add /tmp/tf-head HEAD
python timing_repeat.py /tmp/tf-base /tmp/tf-head 5 \
  tests/test_upload.py::test_retries_on_503 \
  tests/test_queue.py::test_drain

#!/usr/bin/env python3
"""timing_repeat.py — repeat touched tests on a base and a head worktree.

Usage: python timing_repeat.py <base-tree> <head-tree> <runs> <node-id>...
"""
import json
import statistics
import subprocess
import sys
import time


def once(tree, node_id):
    cmd = [sys.executable, "-m", "pytest", node_id, "-q", "-p", "no:cacheprovider"]
    start = time.monotonic()
    proc = subprocess.run(cmd, cwd=tree, capture_output=True, text=True)
    return proc.returncode == 0, round(time.monotonic() - start, 3)


def sweep(tree, node_id, runs):
    results = [once(tree, node_id) for _ in range(runs)]
    durations = [d for _, d in results]
    return {
        "passes": sum(ok for ok, _ in results),
        "runs": runs,
        "p50_seconds": statistics.median(durations),
        "max_seconds": max(durations),
    }


def main(base_tree, head_tree, runs, node_ids):
    rows = []
    for node_id in node_ids:
        b, h = sweep(base_tree, node_id, runs), sweep(head_tree, node_id, runs)
        rows.append({
            "test": node_id,
            "base": b,
            "head": h,
            "p50_ratio": round(h["p50_seconds"] / b["p50_seconds"], 2) if b["p50_seconds"] else None,
        })
    json.dump(rows, sys.stdout, indent=2)
    print()


if __name__ == "__main__":
    main(sys.argv[1], sys.argv[2], int(sys.argv[3]), sys.argv[4:])

Five runs per tree per test is a deliberate compromise: enough to separate "always passes" from "sometimes passes," cheap enough to run on every patch that has a hit. Records look like this (illustrative shape, not a measured result):

[{"test": "tests/test_upload.py::test_retries_on_503",
  "base": {"passes": 3, "runs": 5, "p50_seconds": 0.41, "max_seconds": 0.52},
  "head": {"passes": 5, "runs": 5, "p50_seconds": 6.87, "max_seconds": 7.10},
  "p50_ratio": 16.76}]

5. Step 4 — Freeze with an owner and an expiry date

A timing change that gets accepted without a record becomes permanent infrastructure debt within two sprints. Write it down in a file the gate can read.

# timing_freeze.yaml
freezes:
  - test: tests/test_upload.py::test_retries_on_503
    owner: "@team-storage"
    reason: "patch added 3 attempts with 2s backoff; base 3/5 -> head 5/5, p50 0.41s -> 6.87s"
    expires: "2026-11-20"
    max_p50_ratio: 3.0

# gate excerpt — run before merge
import datetime, yaml
today = datetime.date.today()
for f in yaml.safe_load(open("timing_freeze.yaml"))["freezes"]:
    if datetime.date.fromisoformat(f["expires"]) < today:
        raise SystemExit(f"expired timing freeze: {f['test']} (owner {f['owner']})")

The expiry is the whole mechanism. Without it, freeze files become a second, quieter skip list.

6. Reading the result

The third row is the one that matters. It is the only row where a green suite is a symptom rather than a result.

7. Limitations, and who should not use this

Five runs resolve pass rate to steps of 20%. A test at 90% true pass rate will look perfect in this sweep and still fail weekly in CI. If you need finer resolution, raise the run count for that single test rather than for the whole batch.

Timing on shared CI is noisy. Treat p50_ratio as an order-of-magnitude signal, which is why the table uses thresholds like 2 and 3 instead of percentages.

The regex rules are a starting set, not a language model of nondeterminism. They miss framework-specific waits, condition-variable loops, and most Go channel patterns. Extend RULES for your stack before trusting a clean exit code.

Do not use this workflow for suites where a single test takes minutes. Ten minutes of repeated runs for an end-to-end test that legitimately retries on a slow third-party service proves nothing and frustrates everyone. Those suites need a different control: a documented retry policy at the boundary, not a per-patch sweep.

8. Running the sweep without occupying local CI

Step 2 is a text classification problem over diff hunks, and a free model is sufficient for a first pass: feed it the JSON from step 1 and ask for one of the four class labels plus a one-line justification per hit. You still review the labels; the model just removes the blank page.

Step 3 is the expensive half — five runs across two worktrees for every touched test. That is what the free server option is useful for: move the sweep off the machine that also runs your editor, and keep the free model access for the classification pass. Availability, model list, and runner limits change; check the current terms rather than trusting this section.

Disclosure: This article was prepared as part of MonkeyCode's product outreach.

Start with the script, not the platform

If you adopt only one piece, adopt the extraction script. Forty lines of regex catch the retry that would otherwise be reviewed by nobody, and they run in under a second on a normal diff.

Add the freeze file when the first accepted timing change starts making the gate feel optional. If you want to try the repeat sweep before wiring anything permanent, the free tier is enough for one patch — run it, read the p50_ratio, and decide whether the freeze file earns its place.