test: make request timeout test deterministic

[andreatnvidia]

📋 Summary

Makes the expired request-admission waiter test deterministic by controlling the clock used by the controller. The existing wall-clock timing failed intermittently in unrelated PR #812 and PR #813, while equivalent checks also passed on main and other PR runs.

🔗 Related Issue

N/A. Flakes observed on PR #812 and PR #813.

🔄 Changes

• Replace real sleeps and thread scheduling with a controlled controller clock.
• Preserve coverage that an expired waiter is not assigned a released lease and leaves no waiter or capacity state behind.

🧪 Testing

• make test passes (not run; the full engine suite passed)
• Unit tests added/updated
• E2E tests added/updated (not applicable)
• .venv/bin/ruff check --fix .
• .venv/bin/ruff format .
• .venv/bin/pytest -p no:cacheprovider packages/data-designer-engine/tests/engine/models/request_admission/test_controller.py (25 passed)
• .venv/bin/pytest -p no:cacheprovider packages/data-designer-engine/tests (2,209 passed)

✅ Checklist

• Follows commit message conventions
• Commits are signed off (DCO)
• Architecture docs updated (not applicable; test-only change)

[greptile-apps]

Greptile Summary

This PR makes one request-admission timeout test deterministic. The main changes are:

• Replaces wall-clock sleeps and a release thread with a controlled monotonic clock.
• Advances the mocked clock before releasing capacity to simulate an expired waiter.
• Keeps assertions for timeout behavior and cleaned-up waiter state.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.

Important Files Changed

Filename	Overview
packages/data-designer-engine/tests/engine/models/request_admission/test_controller.py	Updates the expired-waiter async admission test to use a mocked controller clock instead of real timing.

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into andreatgretel/f..." | Re-trigger Greptile}

[github-actions]

Thanks for tackling this flake, @andreatgretel — trading wall-clock timing for a controlled clock is exactly the right move here.

Summary

This test-only change makes test_acquire_async_does_not_assign_expired_waiter_after_release deterministic by monkeypatching the controller's time reference with a controllable SimpleNamespace(monotonic=...) closure, and replacing the real time.sleep + threading.Thread release choreography with a direct clock advance (now = 101.0) followed by a synchronous release. The implementation matches the stated intent: it removes the intermittent failure source while preserving the assertions that an expired waiter is denied with queue_timeout and leaves no residual waiter/capacity state behind.

Findings

Suggestions — Take it or leave it

packages/data-designer-engine/tests/engine/models/request_admission/test_controller.py:434 — Prefer the narrower patch pattern already used in this file

• What: The new test replaces the entire controller.time module with SimpleNamespace(monotonic=lambda: now). Two neighboring tests (lines 249 and 283) instead patch just the attribute in use: monkeypatch.setattr("...controller.time.monotonic", lambda: now).
• Why: Replacing the whole module is slightly more brittle — if the controller ever starts calling another time attribute (time.time(), time.sleep(), etc.), this test would surface as an unrelated AttributeError rather than continuing to work. Matching the existing narrower pattern also keeps the file internally consistent, which is one of the project's structural invariants ("match the conventions of the module you're editing").
• Suggestion: Consider monkeypatch.setattr("data_designer.engine.models.request_admission.controller.time.monotonic", lambda: now), which drops the from types import SimpleNamespace import entirely. Fully optional — the current form is correct and reads fine.

What Looks Good

• Clean determinism via a reassignable closure: The lambda: now capturing a rebindable now (100.0 → 101.0) is a tidy way to step the clock past the waiter's deadline exactly when release runs. No sleeps, no thread join, no polling window to flake on.
• Coverage preserved, not weakened: The rewrite keeps the meaningful assertions — queue_timeout reason plus waiters == 0, active_lease_count == 0, in_flight_count == 0 — so the determinism gain doesn't come at the cost of what the test actually verifies.
• Tighter enqueue check: Swapping the 20-iteration polling loop for a single await asyncio.sleep(0) + direct waiters == 1 assertion is a nice simplification that's now safe precisely because the clock is frozen.

Structural Impact

(graphify, 2.5s)

Risk: LOW (localized change)

• 1 Python files, 0 AST entities, 0/83 clusters

Verdict

Ship it (with nits) — Solid, well-scoped determinism fix with coverage intact. The only note is an optional consistency suggestion to match the narrower time.monotonic patch pattern used elsewhere in the same file.

Note: ruff and pytest were not available in this review environment, so linting and the test run were not executed here — the PR description reports the full engine suite (2,209 passed) and ruff check/format all green, and CI enforces linting on merge.

---

This review was generated by an AI assistant.

[johnnygreco]

Thanks for tightening this up, @andreatgretel!

Summary

This replaces wall-clock sleeps and thread scheduling in the expired-waiter regression test with a controller-local fake monotonic clock, then advances that clock before releasing capacity. The implementation matches the PR's stated intent and preserves coverage that the expired waiter receives queue_timeout without retaining waiter or capacity state.

Findings

No findings.

What Looks Good

• Replacing the controller module's time binding with SimpleNamespace keeps clock control isolated to the controller and avoids freezing asyncio's own monotonic clock.
• The test verifies that the waiter is actually queued before advancing time, then checks the timeout reason and all relevant cleanup counters afterward.
• Removing the helper thread and real sleeps makes the scenario substantially faster and eliminates the scheduling sensitivity that caused the flake.

Verdict

Ship it — Ready to merge as-is.

---

This review was generated by an AI assistant.