Skip to content

fix(pull): the #1559 rebuild bakes the pull-worker env; one owner for the opt-in and a drift predicate on the flag (#3590) - #3632

Merged
pavshulin merged 3 commits into
devfrom
feature/3590-rebuild-pull-env
Oct 13, 2026
Merged

pavshulin merged 3 commits into
devfrom
feature/3590-rebuild-pull-env

Conversation

@pavshulin

@pavshulin pavshulin commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • The bug(recovery): soft-delete recovery dead-ends — recover tells you to start, but start 404s when the container is gone #1559 rebuild-from-nothing path (recreate_missing_container) never applied the pull-worker opt-in, so a pilot whose container was lost came back with no TRINITY_PULL_MODE / TRINITY_MAX_PARALLEL_TASKS while the backend kept routing its work to the durable queue: every task sat queued, no log line, agent healthy.
  • The set-or-clear idiom now has one owner, pull_mode.apply_pull_mode_env, called from all three env assemblers (crud._build_env_vars, the config-drift recreate, the rebuild). An AST guard pins the three known assemblers on the call and finds an unknown fourth: every caller of the shared provision tail must apply the helper, and every containers_run(..., environment=) site under services/ (rglob) must be a registered runner. system_agent_service._create_system_agent is registered with its reason: trinity-system assembles its own env and is not a pull pilot; if an operator ever lists it, the predicate recreates it on its next cold start through the drift path, which applies the helper.
  • A tenth drift predicate, check_pull_mode_env_matches, joins the start_agent_internal chain: a pilot running a pre-flag container (allowlist edited after create, or any assembler omission) and a de-piloted agent still carrying the flag are both recreated on the next POST /start. Flag only; the pool size is feat(pull): resize a pilot's worker pool live when its parallel limit changes #3039's live resize.
  • Two warnings where the path was silent: the pool-size settings fallback to 3, and a rebuild whose MCP-key mint failed (pull env without a credential to poll with, the same stall with a different cause).

Changes

  • src/backend/services/agent_service/pull_mode.py — apply_pull_mode_env (in place, pop then update, module-global lookup so existing stubs keep intercepting); fallback warning.
  • src/backend/services/agent_service/helpers.py — check_pull_mode_env_matches (fail-safe to match on unobserved attrs).
  • src/backend/services/agent_service/lifecycle.py — predicate wired into the start chain; both recreate paths call the helper; mint-failure warning on the rebuild path (the env is final there; the drift path may still receive the key through env_overrides, feature: first-class agent-scoped MCP keys — scope visibility, regeneration, and validation/enforcement of agent-key usage from agent containers #1854).
  • src/backend/services/agent_service/crud.py — create path calls the helper (no behaviour change; the pop is a no-op on a fresh dict).
  • tests/unit/test_3590_rebuild_pull_env.py — 21 tests; tests/unit/test_agent_readiness_probe.py — the helpers stub lists the new predicate; tests/registry.json.
  • Docs: docs/memory/feature-flows/agent-lifecycle.md (predicate list, "Pull-worker opt-in — one owner" section, change-log row), docs/planning/PULL_MIGRATION_TESTING.md (B1 row, rollback step), learnings fragment 2026-10-12-a-literal-key-parity-guard-is-blind-to-a-helper-supplied-key.md.

Why the existing guard missed it

test_1811_recovery_container_parity.py compares string-literal env keys and scans _build_base_env, one of six create sub-builders. The pull keys reach create through a helper call, so they were invisible on both sides. Extending that scan transitively surfaces five more keys that need individual verification; deferred and registered as debt:2026-10-12-1811-parity-scan-transitive.

Adoption note

After deploy, the first POST /start on an existing pilot whose container predates the flag recreates it. That is the same behaviour as every other config-drift predicate.

Test Plan

  • cd tests && pytest unit/test_3590_rebuild_pull_env.py -v — 19 passed
  • Red for the right reason: before the fix the rebuild test fails on KeyError: 'TRINITY_PULL_MODE' from the env handed to the provision tail; with the fix reverted from a scratch copy, four go red — test_rebuild_from_nothing_bakes_pull_env_for_a_pilot, test_rebuild_with_a_failed_key_mint_names_the_stall, test_every_env_assembler_applies_pull_env_through_the_one_helper, test_every_caller_of_the_provision_tail_applies_pull_env (4 failed, 17 passed), file restored byte-identical
  • Independent validation on the rc2 PostgreSQL stack (2026-10-12, PR head reloaded with --reload): docker rm -f + POST /start → rebuild line, TRINITY_PULL_MODE=true + TRINITY_MAX_PARALLEL_TASKS=2 + MCP key baked, pull pool started: 2 worker(s), POST /task → success in ~10 s, 0 queued. De-pilot via the allowlist + backend restart → recreated: true, config_drift, flag gone, agent logs pull mode OFF; a second start returns recreated: false (no recreate loop). Re-pilot → flag back, pool started. Non-pilot b9-push rebuilt with no TRINITY_PULL_* key and reports running. The named workspace volume is reused across every recreate.
  • Full unit island: 25481 passed, 113 skipped, 14 xfailed, 0 failed (local, Python 3.13)
  • Manual verification on the rc2 PG stack (PostgreSQL, from-source, PULL_MODE_PILOT_AGENTS=b9-pull, backend reloaded onto 220ca09d8): docker rm -f agent-b9-pull + POST /api/agents/b9-pull/start at 13:34:43Z → backend Rebuilding missing container for recovered agent b9-pull (#1559); the new container (Created 13:34:43Z) carries TRINITY_PULL_MODE=true TRINITY_MAX_PARALLEL_TASKS=2 and TRINITY_MCP_API_KEY; agent log [#1081] pull pool started: 2 worker(s) for agent b9-pull; 0 queued rows. On rc2 the same steps left the env absent and no pool line (the issue's step 4). No task was sent (no model turn). Also seen again, unrelated to this fix and tracked separately: Could not read template.yaml from volume at rebuild time and Circuit OPENED … after 3 failures 5 s after the rebuild.

Fixes #3590

🤖 Generated with Claude Code

… the opt-in and a drift predicate on the flag

recreate_missing_container re-derived an agent's container env from nothing and
was the one of three assemblers that never applied the pull-worker opt-in, so
`docker rm -f` + `POST /start` rebuilt a pilot with no TRINITY_PULL_MODE while
the backend kept routing its work to the durable queue: every task sat `queued`,
no log line, agent healthy. The drift recreate and creation both applied it.

- pull_mode.apply_pull_mode_env is the one owner of the set-or-clear idiom
  (pop PULL_MODE_ENV_KEYS, then update; in place; module-global lookup so the
  suites that stub pull_mode_env_vars keep intercepting), called from
  crud._build_env_vars, recreate_container_with_updated_config and
  recreate_missing_container. An AST guard pins that writer set.
- helpers.check_pull_mode_env_matches joins the start_agent_internal drift
  chain: a pilot on a pre-flag container, or a de-piloted agent still carrying
  the flag, is recreated on the next start instead of needing a manual
  recreate (canary B-08 workers_missing). Flag only; the pool size is #3039.
  Unobserved attrs match, never a 500 on the start path.
- Two warnings where the path was silent: the pool-size settings fallback to 3,
  and a rebuild whose MCP-key mint failed (pull env with no credential to poll
  with). The latter lives on the rebuild path, where the env is final; the
  drift path may still receive the key through env_overrides (#1854).

Why the #1811 parity guard stayed green: it compares string-literal env keys and
scans _build_base_env, one of six create sub-builders; the pull keys reach
create through a helper call. Extending it transitively surfaces five more
keys needing verification; registered as debt:2026-10-12-1811-parity-scan-transitive.

Red for the right reason: before the fix test_rebuild_from_nothing_bakes_pull_env_for_a_pilot
fails on KeyError 'TRINITY_PULL_MODE' from the env handed to the provision
tail. Mutation: with the rebuild-path call removed from a scratch copy, that
test and test_every_env_assembler_applies_pull_env_through_the_one_helper go
red (2 failed, 16 passed). Full unit island: 25481 passed, 0 failed.

Fixes #3590

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ssembler; fix counts and a stale comment

Validation of #3632 found the AST guard pinned the three known env
assemblers but could not detect a fourth it was not told about. Add the
structural half over services/ (rglob): every caller of the shared
provision tail must call apply_pull_mode_env, and every
containers_run(..., environment=) site must be a registered runner.
system_agent_service._create_system_agent is registered with its reason
(trinity-system builds its own env and is not a pull pilot; the drift
predicate heals it on a cold start if ever listed). Negative controls:
removing the rebuild call turns 4 tests red; a planted run site turns
the runner scan red.

Docs: the predicate block now shows the #1854 mcp_key_match term it
omitted, the change-log row counts 21 tests, and the pull_mode.py module
comment names apply_pull_mode_env.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

…gistry.json append conflict)

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

@obasilakis obasilakis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated at bfc6b99c3 (Lane B). Approving.

Verified

  • Tier 1 green: pytest (head, seed 12345), regression diff, the four required contexts, gitleaks. journey-smoke and prod-image-smoke also pass. dev-ci is green.
  • Local run of unit/test_3590_rebuild_pull_env.py + unit/test_agent_readiness_probe.py: 26 passed.
  • Mutation: with the rebuild block in recreate_missing_container removed and check_pull_mode_env_matches returning True, 7 tests fail and 14 pass. The failures include test_rebuild_from_nothing_bakes_pull_env_for_a_pilot and test_start_recreates_a_pilot_whose_container_lacks_the_flag, which drive the real code paths.
  • The predicate and apply_pull_mode_env both key on is_pull_pilot_agent, so a recreated container always satisfies the predicate. No recreate loop.
  • The new predicate sits inside the #1816 wholesale deferral, so a running trinity-system is still never recreated.
  • Secret, email, IP, host-path, symlink and env-var packaging checks are clean.

Findings (non-blocking)

  1. A start issued against a running agent now recreates it when the allowlist changed. The adoption note covers the first POST /start; this also applies to MCP ensure-running calls on a live container. A pilot missing the flag has no pull worker to interrupt. A de-piloted agent still carrying the flag loses its in-flight pulled turns. Worth one line in the adoption note.
  2. tests/registry.json reformats the neighbouring #3526 entry (categories array expanded to multi-line). Unrelated to this change, left over from the merge resolution.
  3. The body says "19 passed" in the Test Plan and "21 tests" under Changes. The file has 21.
  4. The nightly-unit-suite bot comment about a merge conflict is stale. The PR is mergeable.

Scope of this review
I read the diff of the four source files and ran the tests. I did not read the full test file line by line, and I did not reproduce the rc2 PostgreSQL validation.

@github-actions

Copy link
Copy Markdown

⚠️ Live-instance suite skipped — merge conflict against dev.

Resolve by merging dev locally and pushing the result; the next nightly re-tests.

@pavshulin
pavshulin merged commit c49b4d3 into dev Oct 13, 2026
25 checks passed
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