Skip to content

Add per-engine spawn_stagger_s to serialize shared-state engine cold starts#27

Open
mlava wants to merge 2 commits into
NateBJones-Projects:mainfrom
mlava:feat/spawn-stagger
Open

Add per-engine spawn_stagger_s to serialize shared-state engine cold starts#27
mlava wants to merge 2 commits into
NateBJones-Projects:mainfrom
mlava:feat/spawn-stagger

Conversation

@mlava

@mlava mlava commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Problem

Engines whose instances share local state fail their cold start when several workers spawn in the same instant. OpenCode is the motivating case: every process opens one SQLite state DB (~/.local/share/opencode/opencode.db), so a 4-wide spawn wave has exactly one winner — the rest die in ~2s with database is locked (or a generic "Unexpected server error") before spending a single token. Ringer then retries all losers simultaneously, which reproduces the collision: one more winner, and the remaining tasks burn both attempts on harness failures that pollute the model scoreboard with FAIL rows the model never earned.

Observed in the field (2026-07-12, four-model bakeoff): two spawn waves, exactly one survivor each; the two twice-dead tasks passed first-try when rerun serially.

Change

engines.<name>.spawn_stagger_s (float seconds, default 0) sets a minimum gap between consecutive spawns of that engine:

  • A SpawnGate on the runner reserves spawn slots per engine in claim order — no barrier, workers of other engines are unaffected, and concurrent waiters sleep toward their own slots.
  • Applies to every attempt, so retries stagger too (the second collision wave above was simultaneous retries).
  • Logs [ringer.py] spawn stagger: waited N.Ns to the worker log whenever a spawn actually waited, so post-mortems can see the gate at work.
  • Default 0 = spawn-all-at-once, zero behavior change for every existing config.
  • Documented on the sample config's opencode block with the collision rationale.

Validation

  1. Unit tests (tests/test_spawn_stagger.py): config parsing (defaults, float/int, rejects negative/string/bool) and the gate's slot math via injectable clock/sleep — three simultaneous claims spawn at t+0/+2/+4, engines don't block each other, no wait once the gap has already elapsed.
  2. End-to-end (same file): a real ringer.py run over three mock-engine tasks at max_parallel 3 with spawn_stagger_s = 0.6, asserting spawn spacing from the attempt 1 started timestamps in the worker logs and exactly two stagger log lines.
  3. Live probe: three OpenCode workers (GLM-5.2) fired at max_parallel 3 with a 2s stagger — spawns at +0s/+2.002s/+4.001s, 3/3 first-try passes, zero lock errors. Same shape that lost 3 of 4 workers to the collision the same morning.

Full suite: 190 tests, failure set identical to the pre-change baseline.

🤖 Generated with Claude Code

…starts

Engines whose instances share local state fail their cold start when
several workers launch in the same instant. OpenCode is the motivating
case: every process opens one SQLite state DB
(~/.local/share/opencode/opencode.db), so a 4-wide spawn wave has exactly
one winner — the rest die in ~2s with "database is locked" (or a generic
"Unexpected server error") before spending a single token, and ringer's
simultaneous retries reproduce the collision. Observed in the field
2026-07-12: two waves, one survivor each; the two twice-dead tasks passed
first-try when rerun serially.

engines.<name>.spawn_stagger_s sets a minimum gap in seconds between
consecutive spawns of that engine. A SpawnGate on the runner reserves
spawn slots per engine (claim order, no barrier), applies to every
attempt so retries stagger too, and logs a line to the worker log when a
spawn actually waited. Default 0 preserves spawn-all-at-once for every
existing config.

Validated three ways: unit tests on the gate's slot math (injectable
clock/sleep), an end-to-end mock-engine run asserting real spawn spacing
from worker logs, and a live 3-wide opencode probe (stagger 2s): spawns
at +0s/+2.0s/+4.0s, 3/3 first-try passes, zero lock errors — the same
shape that lost 3 of 4 workers to the collision the same morning.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@justfinethanku

Copy link
Copy Markdown
Contributor

This is a solid feature — the shared-SQLite cold-start collision is real (we've hit it), the zero-by-default design is exactly right, and the test coverage (including the end-to-end subprocess case) is the standard we want. We'd like to merge it after four changes:

  1. Keep stagger wait out of duration_ms. This is the important one, and it's a project principle worth stating: the eval log is model-performance evidence, and queue time a worker spends waiting for its spawn slot is orchestration overhead, not model speed. As written, a worker that waited 6s in the gate looks 6s slower on the scoreboard's Speed column. Start the duration clock at actual process launch (or record the wait as its own field on the row) — the worker-log line you already write is the right place for the human-facing trace.

  2. Gate at the real spawn boundary. On current main there's command build + steering preparation between where the gate waits and where the process launches, so the effective gap can be shorter than configured. Move the wait_turn() call to just before the subprocess spawn.

  3. Reject non-finite values. spawn_stagger_s = inf (or nan) currently validates and would wedge a run. Reject them in load_engines() with tests alongside the existing negative/boolean cases.

  4. Soften the "other engines unaffected" claim (or fix the scheduling): staggered same-engine workers still hold max_parallel slots while they wait, so a large OpenCode batch can starve other engines' workers during the stagger. A doc note in config.sample.toml is fine if you'd rather not touch the semaphore.

Happy to review a revision quickly — and heads-up that your #35/#36 touch adjacent code, so whichever lands second will want a small rebase. Thanks for the steady stream of well-scoped PRs; this is exactly the kind of contribution that's easy to say yes to.

- duration_ms now excludes gate-queue time; the wait is recorded on the
  eval row as spawn_wait_ms (JSONL only) — queue time is orchestration
  overhead, not model speed
- wait_turn() moved after command build + steering prep, immediately
  before the subprocess spawn, so the configured gap is the effective gap
- spawn_stagger_s rejects inf/nan in load_engines, with tests beside the
  existing rejection cases
- config.sample.toml notes that staggered workers still hold max_parallel
  slots while waiting, so a wide staggered batch can starve other engines

Per review on PR NateBJones-Projects#27.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mlava

mlava commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

All four landed in ea105cc.

  1. Stagger wait is out of duration_ms. The gate wait comes back on the worker result, gets subtracted from the attempt clock, and is recorded as its own field on the eval row (spawn_wait_ms, JSONL only — no schema change). The worker-log line stays as the human-facing trace.
  2. Gate moved to the real spawn boundarywait_turn() now runs after command build + steering prep, immediately before the subprocess spawn, so the configured gap is the effective gap. The e2e's attempt 1 started timestamps remain a valid spacing proxy.
  3. Non-finite values rejected in load_engines() (must be a finite number of seconds), tests beside the negative/string/bool cases.
  4. Doc note added to the sample config's stagger block: staggered workers still hold max_parallel slots while waiting, so a wide staggered batch can starve other engines during the window. Left the semaphore semantics alone — agreed engine-aware slot scheduling is its own PR if ever wanted.

The e2e now also asserts the row evidence: exactly one row with spawn_wait_ms == 0, two ≥ 0.75×stagger, and every row's duration_ms under 0.9×2×stagger — so a reintroduced wait-in-duration leak fails loudly. Full suite: failure set identical to the pre-change baseline (the two known test_design_reference / test_scoreboard_page failures).

On the rebase heads-up: noted — happy to rebase whichever of this and the #33/#35/#36 unification lands second.

@mlava

mlava commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Heads-up on the windows harness (non-blocking) job failure on ea105cc: it's unrelated to this PR (the crash is in test_active_runs, which this diff doesn't touch), but while confirming that I found the root cause, and it's worth having on record because the traceback actively lies about it.

The visible AttributeError: 'WindowsPath' object has no attribute '_drv' is a decoy — that's pathlib's normal lazy-init control flow. The fatal exception underneath is a KeyboardInterrupt delivered asynchronously (it lands mid-ntpath.splitroot). The chain:

  1. test_dead_pid_pruned_on_read_and_write writes a marker with dead PID 99999999, then read_active_runs() probes it via pid_is_alive()os.kill(pid, 0).
  2. os.kill(pid, 0) is the POSIX liveness idiom, but on Windows signal 0 is signal.CTRL_C_EVENT, so CPython calls GenerateConsoleCtrlEvent instead of probing — the "probe" generates a console CTRL-C that gets delivered to the test process itself.
  3. The interrupt kills the unittest runner at the second test alphabetically, so the Windows job produces no signal beyond "it compiles" — every run of it dies before the other ~190 tests execute, this branch's included.

Fix shape, if you want it: a sys.platform == "win32" branch in pid_is_alive() using ctypes OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION) + GetExitCodeProcess — stdlib only, no new dependencies. Happy to send that as a small separate PR; say the word and I'll put it up.

seanpmgallagher added a commit to seanpmgallagher/ringer that referenced this pull request Jul 23, 2026
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.

2 participants