Repository navigation
factory r3: fix server-rest test drift and the factory_changed timer leak - #142
Merged
Merged
Conversation
LittleChimera
force-pushed
the
fix/server-rest-r3
branch
3 times, most recently
from
October 28, 2026 02:18
9c478c1 to
9b2da69
Compare
test_sessions_factory_labels minted its runner and person bearers at collection time with a 600 s TTL. Under xdist the server-rest lane runs the module ~15 min after collection, so every parametrized case got 401 instead of the 403 it asserts. Mint the headers inside the test. Add a case that a runner bearer still creates a session carrying factory labels (sys_factory_spawn's POST /v1/sessions). test_github_injection still passed github_token / github_login / ssh_authorized_keys to start_host, which Sync5 (#83) removed in favour of the server-side credential broker that the host configures at startup. Assert the launch carries no GitHub credentials instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…test The runner-create case now sends the heartbeat label that _spawn_member always sends. The start_host test no longer asserts on authorized_keys or frames SSH keys as handled by the credential broker: the broker covers GitHub tokens only, and SSH key setup is currently unwired. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The lead and ic refusals were logged on the same FakeClock millisecond, and find_events breaks ts ties by event id, which is minted in a worker thread. Under xdist load the two came back in either order. Advance the clock one second before the ic post so the events sort by ts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
announce_factory_changed parks a debounce task per (loop, factory) in a module-level dict and only removed it after the 0.5 s sleep. A loop that closes inside that window (every async test that emits a factory event) cancels the task and leaves the entry behind, and the entry pins the task, its loop, the app state, the stores and their engines with the compiled migration statements. In the server-rest lane that is ~5 MB per factory test; the workers reached ~8 GB and the runner was killed at 91-97% on every run since the round-3 merges. A done callback now drops the entry however the timer ends: the lane peaks at ~4.3 GB locally, and a 19-test factory slice runs in 40 s instead of 300 s. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LittleChimera
force-pushed
the
fix/server-rest-r3
branch
from
October 28, 2026 03:04
9b2da69 to
f66d441
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue
N/A (Test / CI)
Summary
The
Pytest (server-rest)lane is red on staging and on every round-3 PR. The two original failures were test drift, not product regressions: factory label writes, factory session creation and GitHub credentials in sandboxes all still work. Once those were fixed, a flake and a memory leak surfaced in the same lane. The leak was killing the runner.test_sessions_factory_labels(added in factory r3: Tier-0 hardening (push-ref allowlist, merge lockout, human origin, bundle fence, label audit) #135): the parametrized cases built their runner and person bearers at collection time, and those bearers have a 600 s TTL. Under xdist the lane reaches this module about 16 minutes after collection, so every case got401instead of the403it asserts. The fix mints the headers inside the test. I also added a positive case: a runner bearer (org: runner) still creates a session throughPOST /v1/sessionscarrying exactly the factory labels_spawn_membersends (id, role, parent, supervisor, state, heartbeat_s, project).test_start_host_injects_ssh_and_env: this one has been failing since Sync5 (Sync5: merge upstream/main into staging #83), well before round 3. Sync5: merge upstream/main into staging #83 removed thegithub_token/github_login/ssh_authorized_keysparameters fromstart_host. GitHub tokens moved to the server-side credential broker (omnigent hostrunsconfigure_host_git/configure_host_ghat startup). The test now asserts that the launch carries no GitHub credentials. It makes no claim about SSH keys, because the broker does not handle them (see below).test_a_looping_sender_logs_one_refusal_per_recipient_per_minute(added in factory cockpit: read API and truthful supervision (snapshot/events/decisions/preflight, mail replay, quiet beats, preview keepalive + TTL, server decisions, reconcile) #123): this flake failed the lane on this PR. Theleadandicrefusals were logged on the same FakeClock millisecond, andfind_eventsbreakststies by an event id minted insideasyncio.to_thread, so under xdist load they came back in either order. The test now advances the clock 1 s before theicpost, well under the 60 s refusal interval.omnigent/server/factory/live.py): since factory r3: telemetry core (P0 retention, claim_mismatch, PR facts, versions, spans, cost ledger, heal, read routes, export) #139/factory r3: projects through proposals, server-spawned PLs and dependency gating (R3-07) #140/factory r3: people (roster, grant mirroring, viewer block, attribution, presence, decision routing and escalation, access requests) #141, every server-rest job on every branch has died 14-19 minutes in. The symptom was "runner has received a shutdown signal", "operation was canceled", or the 30 min timeout, with no failing test and no common test in flight.announce_factory_changedparks a debounce task per (loop, factory) in a module-level dict and removed it only after the 0.5 s sleep. When a loop closes inside that window, which every async test that emits a factory event does, the task is cancelled and the entry stays. That entry pins the task, its loop, the app state, the stores and their engines, including the compiled migration statements. The cost was about 5 MB per factory test: locallytests/server/factoryalone peaked at 5.8 GB across 4 workers and the whole lane at about 8 GB. A done callback now drops the entry however the timer ends. The lane peaks at about 4.3 GB locally (with more tests, since factory r3: work graph (factory_tasks as server truth, binder, graph computations, graph API, human task add/edit, sys_factory_task, PL playbook) (R3-09) #143/factory r3: ops — live 'now', fleet API, interventions on any agent, issues and detectors (R3-11) #144/factory r3: the Room (threads, routing, coalesced delivery, reply fallback, cards, sys_factory_chat, planner) (R3-08) #145 are included), and a 19-test factory slice runs in 40 s instead of 300 s. In production the loop never closes, so the only effect there is that a cancelled timer no longer lingers.Factory flows I checked against the code:
sys_factory_spawn) sendsPOST /v1/sessionswith factory labels. The factory r3: Tier-0 hardening (push-ref allowlist, merge lockout, human origin, bundle fence, label audit) #135 guard (_reject_runner_factory_labels) only covers the genericPATCH /v1/sessions/{id}, so spawn is unaffected. The new test covers this case.sys_factory_setand mutual supervision go throughPATCH /v1/factory/sessions/{id}. That route is not guarded, and its tests pass.omnigent.factory.*labels through the generic PATCH.Out of scope, found in review (both predate this PR):
~/.ssh/authorized_keys. Nothing in omnigent writes it on staging now, but the deployed SSHPiper config still reads the sandbox'sclient_authorized_keys. VS Code Remote through SSHPiper has probably been broken since Sync5.ssh_authorized_keys_setup_commandsandresolve_sandbox_identityshould stay until someone decides whether SSHPiper is still supported.POST /v1/sessionswith server-only labels (e.g.omnigent.factory.engine_mode,def_version,role=root) and get 201. The R3-04 checks only guard the PATCH routes.sys_factory_spawnnever sends these labels, so a follow-up can refuse them on runner-origin creates without breaking spawn.Test Plan
Scriptobjects retained, all rooted inlive._pending. After the fix it found 0 / 0 / 0.test_a_loop_closed_inside_the_debounce_leaves_no_pending_timerfails without the fix and passes with it.-n 4as in CI, rebased on staging 760d0ab: 6077 passed, peak worker RSS 4.3 GB (8 GB before). The only failures are local-env gaps (databricks/asgirefnot installed).tests/server/factory, the labels, github-injection andtests/runner/test_factory_spawn.pymodules on the final rebase: 1447 passed.tests/server/routes/test_host_credentials.py tests/test_git_credential_github.py tests/onboarding/sandboxes: 669 passed.Demo
N/A
Type of change
Test coverage
Coverage notes
github_sandbox_envandgit_identity_envare unused since #83.ssh_authorized_keys_setup_commandsandresolve_sandbox_identityare also unwired, but they are tied to the open SSHPiper question above, so this PR leaves them in place.🤖 Generated with Claude Code