Skip to content

Make the docker_start_app throttle per-app instead of global#176

Open
ClaydeCode wants to merge 3 commits into
mainfrom
fix/clayde/per-app-start-throttle
Open

Make the docker_start_app throttle per-app instead of global#176
ClaydeCode wants to merge 3 commits into
mainfrom
fix/clayde/per-app-start-throttle

Conversation

@ClaydeCode

Copy link
Copy Markdown
Contributor

Closes #132.

Problem

docker_start_app was decorated @throttle(5). The throttle kept a single shared timestamp, so the 5s window was global across all apps, not per-app. Starting app B within 5s of app A silently returned None — B stayed on its splash screen until the next request or the next control_apps tick (which only starts always_on apps).

Change

  • throttle gains an optional key function. Windows are stored in a dict keyed by key(*args); key=None (default) preserves the old single global window, so the only other behaviour is byte-for-byte unchanged.
  • docker_start_app is now @throttle(5, key=lambda name: name) — each app has its own 5s dedupe window.
  • The decorator exposes wrapper.reset() (clears all windows) as a test hook, replacing the old closure-cell-poking fixture.
  • Fixed the now-stale "global" wording in app_lifecycle.py.

Same-app silent-drop semantics within one window are kept — that is the intended dedupe.

Tests

  • test_throttle_per_key_windows_are_independent, test_throttle_reset_clears_all_windows (unit).
  • test_docker_start_app_starts_each_app_despite_throttle — two different apps back-to-back both invoke docker (fails on the old global throttle).
  • test_docker_start_app_throttles_repeated_start_of_same_app — same app twice within 5s invokes docker once; the app is reset to a startable status between calls so the throttle, not status gating, is the sole reason the second start drops.

All throttle + compose-pinning tests pass; the app-lifecycle / last-access / traefik-dyn blast-radius suites pass locally.

Review panel

Ran adversarial (always), test adversary (logic + tests changed), DevEx/readability (non-trivial decorator + tests). No specialist for security/DB/API/UX — the diff touches none of those surfaces.

No blocking findings from any reviewer. Both new integration tests were mutation-verified to fail on the pre-change (global) throttle.

Advisories addressed:

  • Test adversary: repeat-start test's teeth relied indirectly on RUNNING being in the startable set (since docker_start_app flips status to RUNNING). Fixed in 2f45a37 — reset the app to STOPPED between calls so the throttle is the unambiguous gate.
  • DevEx + adversarial: throttle lacked a docstring for the new key/reset contract, and the key lambda carried unreachable *args/**kwargs. Fixed in 2f45a37 — added docstring, simplified to lambda name: name (both callers pass name positionally).

Advisories noted, not acted on: last_call dict retains one entry per distinct key for the process lifetime — bounded here by installed-app count, only a concern if throttle is later reused with an unbounded key (documented in the new docstring caveat). Missing functools.wraps on the wrapper is pre-existing and out of scope.

Recommended reading order

  1. shard_core/util/misc.py — the decorator change
  2. shard_core/service/app_tools.py — apply per-app key
  3. shard_core/service/app_lifecycle.py — stale-comment fix
  4. tests/test_throttle.py, tests/test_app_compose_pinning.py — tests

🤖 Generated with Claude Code

ClaydeCode and others added 3 commits July 22, 2026 01:00
The throttle decorator kept a single last_call timestamp shared across all
invocations, so the window was global per decorated function. Add an optional
key function so each key gets its own window; key=None preserves the old
global behaviour. Store windows in a dict and expose wrapper.reset() to clear
them (used by tests that would otherwise inherit a stale window across cases).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@Throttle(5) on docker_start_app was global, so a request to start app B
within 5s of app A starting was silently dropped and B stayed on its splash
until the next request or lifecycle tick. Key the throttle on the app name so
each app has its own 5s dedupe window. Update the reset fixture to the new
reset() hook and fix the now-stale "global" wording in app_lifecycle.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Review panel follow-ups: document the throttle key/reset contract; drop the
unreachable *args/**kwargs from the app-name key lambda (both callers pass
name positionally); and in the repeat-start test reset the app to a startable
status between calls so the throttle, not status gating, is the sole reason
the second start is dropped.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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.

Make the docker_start_app throttle per-app instead of global

1 participant