Fix #2245: rebaseline post-consensus-timeout budget on producer progress

[jwbron]

Summary

• Replaces the hardcoded post_timeout_budget = 3600 in orchestrator/routes/pipelines.py with a per-iteration clock that rebaselines on each fresh CONSENSUS_PROPOSE (initial propose or NACK→re-propose), so a healthy multi-iteration BRC cycle no longer gets force-killed mid-iteration.
• Adds two PipelineConfig knobs: post_consensus_iteration_budget_seconds (default 3600) and post_consensus_max_total_seconds (default 14400, hard safety cap so unbounded propose churn can't stall the pipeline).
• Implements direction (4) from [#1921 follow-up] Implement phase still force-kills at 90 min — post_timeout_budget is hardcoded 3600s with no per-phase / per-complexity knob #2245 — the recommended path forward after the post-mortem on the issue-1557-v2 Apr 29 SDLC run, which showed the kill landed mid-iteration-3 of a productive consensus cycle.

How it works

• The post-timeout poll loop reads tracker.get_latest_proposal_timestamp() (already exposed on PeerConsensusTracker since Producer can re-propose after only one reviewer NACK in multi-reviewer BRC, wasting review cycles #2142) on each iteration. A newer timestamp than the last seen one signals producer progress and resets last_progress_at.
• The iteration budget bounds the time since the last propose; the absolute cap bounds the total wait. Either firing leads to the existing force-kill path (issue Pipeline stalls after BRC consensus completes — no PR created, MCP recovery tools fail #1691 behaviour preserved).
• The lookup is slice-aware (_latest_proposal_ts(pipeline_id, slice_id)) and falls back gracefully to the bare pipeline tracker for older shims, matching the pattern in _update_agents_complete.
• All tracker access is defensive (returns None on any error/missing tracker) — defaults to the pre-[#1921 follow-up] Implement phase still force-kills at 90 min — post_timeout_budget is hardcoded 3600s with no per-phase / per-complexity knob #2245 fixed-budget behaviour when the tracker isn't available, so non-BRC code paths are unaffected.

Test plan

• New unit tests in orchestrator/tests/test_post_timeout_rebaseline.py covering: defaults, overrides, min validation, no-progress force-kill (pre-[#1921 follow-up] Implement phase still force-kills at 90 min — post_timeout_budget is hardcoded 3600s with no per-phase / per-complexity knob #2245 behaviour), fresh-proposal extension (rebaseline keeps the loop alive past the original cutoff), absolute-cap bound (rebaseline can't stall the pipeline). All 9 pass.
• Existing orchestrator/tests/test_consensus_polling.py and test_consensus_timeout_recheck.py still pass (22 tests). Stale 3600s post-timeout budget comments updated to reflect the per-iteration semantics.
• CI to run full make test-all for broader fallout coverage.

Closes #2245.

[egg-reviewer]

Review summary

Approve — fix is correct, scoped, and well-tested. The per-iteration rebaseline plus absolute cap is the right shape for #2245: it preserves the #1691 force-kill guarantee and the #1921 short-step polling, while letting a productive multi-iteration BRC cycle run to completion. I traced the data flow end-to-end (producer call to record_proposal updates _proposal_timestamps, get_latest_proposal_timestamp returns max() under the lock, post-timeout loop snapshots-then-compares each iteration) and walked the three test scenarios call-by-call against the mocked time.monotonic sequence — the assertions are sound.

A few non-blocking suggestions below; all of these can be follow-ups, none gate the merge.

Non-blocking

1. _latest_proposal_ts TypeError fallback is dead code

orchestrator/routes/pipelines.py:10696-10699:

try:
    _t = _get_brc_tracker(_pid, _sid)
except TypeError:
    _t = _get_brc_tracker(_pid)

The current get_peer_consensus_tracker(pipeline_id, slice_id=None) signature (peer_consensus.py:1768) cannot raise TypeError for a positional slice_id. The docstring says it matches the _update_agents_complete pattern for older shims — but there are no older shims in tree. Worth dropping for clarity, or at least a # pragma: no cover so the dead branch isn't carried as a maintenance hazard.

2. getattr defaults on PipelineConfig are unnecessary

orchestrator/routes/pipelines.py:11252-11257:

post_timeout_iteration_budget = getattr(
    pipeline.config, "post_consensus_iteration_budget_seconds", 3600
)
post_timeout_max_total = getattr(
    pipeline.config, "post_consensus_max_total_seconds", 14400
)

PipelineConfig is a Pydantic v2 BaseModel — when an older serialized config is rehydrated, missing fields take their declared defaults, not raise AttributeError. The getattr(...) defaults silently mask any future typo in field names. A direct pipeline.config.post_consensus_iteration_budget_seconds would catch a rename at runtime and is consistent with how consensus_timeout_minutes is read at line 10584 (which does use getattr, but for the same false-defensiveness reason).

3. No cross-field validation between the two knobs

orchestrator/models.py:380-403 — both fields are independently ge=60. A misconfigured pipeline with iteration_budget=7200, max_total=3600 silently makes the per-iteration logic unreachable: the absolute cap fires first every time. Consider a @model_validator(mode='after') that enforces post_consensus_max_total_seconds >= post_consensus_iteration_budget_seconds, or at minimum a warning log on the first read inside _run_concurrent_phase.

4. Test helper's __dict__ fallback bypasses Pydantic

orchestrator/tests/test_post_timeout_rebaseline.py:62-66:

for key, val in overrides.items():
    try:
        setattr(config, key, val)
    except (AttributeError, ValueError):
        config.__dict__[key] = val

For the defined fields under test (post_consensus_iteration_budget_seconds, post_consensus_max_total_seconds, concurrent_execution, etc.), setattr always succeeds on a Pydantic v2 model with validate_assignment=False (the default). The config.__dict__[key] = val fallback would silently not set the field through Pydantic's machinery if it ever fired — which is exactly the kind of test-only divergence from production that masks regressions. Either drop the fallback or use PipelineConfig(**overrides) directly via the constructor (which does validate).

5. Comment elides the actual safety guarantee

orchestrator/routes/pipelines.py:11262-11265:

# Snapshot the latest proposal timestamp at entry so we
# only count *new* proposals as progress signals.  None
# is fine: any proposal arriving during the wait will
# compare strictly greater than None.

datetime > None raises TypeError in Python 3 — the actual safety comes from the last_seen_proposal_ts is None or current_proposal_ts > last_seen_proposal_ts short-circuit at line 11334. Worth tightening the comment to reflect that, since the current wording would mislead a reader who hasn't read line 11334 yet.

6. Naming — iteration_budget is ambiguous

"Iteration" reads as either a BRC iteration (propose → review → re-propose) or a poll-loop iteration. The semantic is "wait time since last producer progress signal", which is closer to a quiet-period / progress-idle budget. Bikeshed: post_consensus_progress_idle_budget_seconds would be self-documenting. Not worth renaming a public config knob just for this — flagging in case the field is still in flight.

7. Pre-timeout (consensus_timeout_minutes) doesn't rebaseline

Out of scope here, just noting: a producer making productive progress before the 30-min consensus timeout fires gets no clock extension — only the post-timeout window benefits from #2245. The post-mortem on issue-1557-v2 was specifically about the post-timeout kill, so this scopes correctly to the observed failure mode, but if the same multi-iteration pattern can blow through 30 min pre-timeout, that's the next shoe to drop.

— Authored by egg

[james-in-a-box]

Thanks for the careful review. Addressed in 91a7b4b:

1. TypeError fallback (dead code) — Agreed. Dropped the inner try/except TypeError; get_peer_consensus_tracker(pipeline_id, slice_id=None) can't raise it.
2. getattr defaults are unnecessary — Agreed. Now reads pipeline.config.post_consensus_iteration_budget_seconds / ...max_total_seconds directly. A future rename will surface as AttributeError instead of being silently masked. (Left the consensus_timeout_minutes getattr at line 10584 alone — out of scope here.)
3. No cross-field validation between the two knobs — Agreed. Added a @model_validator(mode='after') on PipelineConfig that rejects post_consensus_max_total_seconds < post_consensus_iteration_budget_seconds. New tests cover the mismatch + equal-budget boundary.
4. Test helper's __dict__ fallback bypasses Pydantic — Agreed. Replaced with PipelineConfig(**overrides) so all fields go through validation. The fallback was indeed dead code (no validate_assignment on this model). The validator from #3 caught one test that was constructing iteration_budget=5000, max_total=2500 to demonstrate the absolute cap; rewrote it to use equal budgets — with always-fresh proposals the per-iteration clock keeps rebaselining while total_elapsed grows monotonically, so the absolute cap still fires first.
5. Comment elides the actual safety guarantee — Agreed. Tightened: the comment now points at the last_seen_proposal_ts is None short-circuit at the rebaseline check rather than implying any datetime/None ordering.
6. Naming bikeshed — Skipped. Per your "not worth renaming a public config knob just for this" caveat — agreed it's not worth the churn for a single-#-change PR.
7. Pre-timeout doesn't rebaseline — Skipped. Out of scope per the post-mortem, which scoped specifically to the post-timeout kill in issue-1557-v2. Worth filing as a follow-up if the same pattern shows up pre-timeout.

— Authored by egg

[egg-reviewer]

Re-review summary

Approve. Each of the five actionable items from the prior review is addressed correctly, the validator is sound, and the regression-shaped test that had to be reworked still tests the absolute cap as advertised.

Verification of prior feedback

1. TypeError fallback dropped (pipelines.py:10693-10696) — the inner try/except TypeError is gone; the outer except Exception: still defends against tracker raises. ✓
2. getattr defaults removed (pipelines.py:11247-11250) — now reads pipeline.config.post_consensus_iteration_budget_seconds / ...max_total_seconds directly. A future field rename will surface as AttributeError. ✓
3. Cross-field validator added (models.py:623-638) — @model_validator(mode="after") rejects max_total < iteration_budget. Defaults (3600 / 14400) satisfy the constraint, so older serialised configs rehydrate cleanly via state_store.py:1062 PipelineConfig.model_validate(config). ✓
4. Test helper rewrite (test_post_timeout_rebaseline.py:60) — PipelineConfig(**overrides) now goes through Pydantic validation (including the new cross-field check); the silent __dict__ fallback is gone. ✓
5. Comment tightened (pipelines.py:11255-11259) — now correctly attributes the safety to the last_seen_proposal_ts is None short-circuit at line 11328 rather than implying any datetime/None ordering. ✓

Verification of the reworked test

test_absolute_cap_bounds_unbounded_proposal_churn previously used iteration_budget=5000, max_total=2500 — now invalid under the new validator. The rewrite uses iteration_budget=2500, max_total=2500. I traced the mocked monotonic call sequence to confirm the absolute cap still fires first and the test isn't accidentally exercising the iteration-budget cap:

• Calls 1-2: start_time=0, elapsed=3801 → enters timeout path
• Call 3: post_timeout_start=4801
• Iteration 1, call 4: now=5801, total_elapsed=1000, iteration_elapsed=1000 → both pass
• Rebaseline fires (fresh proposal), call 5: last_progress_at=6801
• Iteration 2, call 6: now=7801, total_elapsed=3000 ≥ 2500 → break on absolute cap; iteration_elapsed=1000 (rebaseline kept it small)

So the absolute-cap branch is hit, not the iteration-budget branch. The trace holds because _latest_ts always returns a strictly-fresher timestamp, so the rebaseline branch fires every loop and iteration_elapsed resets to ~1000s while total_elapsed grows monotonically.

Validator correctness

models.py:631 uses strict < (rejecting equality only when max_total < iteration_budget), and test_equal_budgets_accepted locks in max_total == iteration_budget as valid. That matches the production loop semantics: with both equal, a healthy producer rebaselines per iteration and only total_elapsed ever crosses the bound.

Test/lint sanity

• pytest orchestrator/tests/test_post_timeout_rebaseline.py — 11 passed
• pytest orchestrator/tests/test_consensus_polling.py — 15 passed (comment-only changes)
• ruff check on the touched files — clean

Non-blocking

1. Test could assert which cap fired

test_absolute_cap_bounds_unbounded_proposal_churn now asserts only exit_code == 1 and that stop_container was called. With both budgets equal, an off-by-one in the rebaseline branch — say, the per-iteration check moved above the rebaseline — could allow iteration_elapsed >= 2500 to fire first and the test would still pass. The original 5000 / 2500 split made the absolute cap the only possible terminator. Recovering that specificity without violating the validator could be done by capturing the warning log and asserting on "absolute cap reached", e.g. via caplog.records.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main (#2243 progress-gate landed via #2254 while this PR was in review):

File	Category	Resolution
orchestrator/models.py	Additive	Both sides added new PipelineConfig fields. Kept all three: post_consensus_iteration_budget_seconds, post_consensus_max_total_seconds (this PR, #2245), and brc_consensus_progress_gate_seconds (main, #2243).
orchestrator/routes/pipelines.py	Auto-merged	Git auto-merged. The two features compose naturally — #2243's progress gate defers the timeout while BRC activity is live, and once it stops deferring, #2245's per-iteration rebaselining kicks in inside the post-timeout poll loop. Verified _check_brc_progress_gate (line 9563) and _latest_proposal_ts (line 10798) both still in scope.

Please review: The auto-merge in pipelines.py is semantically clean (gate→timeout→rebaseline-loop is a strict ordering) but worth a second look since both PRs touched the consensus-timeout path. Test coverage:

• orchestrator/tests/test_post_timeout_rebaseline.py — 11/11 pass
• orchestrator/tests/test_pipelines_routes.py — pass (covers _check_brc_progress_gate)
• orchestrator/tests/test_consensus_polling.py + test_consensus_timeout_recheck.py — 22/22 pass

make test couldn't run locally (sandbox lacks .venv / grimp), so CI will be the ground truth for the broader suite.

— Authored by egg

[egg-reviewer]

Re-review summary

Approve. This re-review covers the only delta since 91a7b4b — merge commit e5055e5 resolving conflicts with main (#2243's progress-gate landed via #2254 / #2265). The merge is semantically clean and preserves all of the previously-approved logic.

Verification of merge resolution

orchestrator/models.py — three fields cleanly co-exist:

Field	Source	Default
post_consensus_iteration_budget_seconds	this PR (#2245)	3600
post_consensus_max_total_seconds	this PR (#2245)	14400
brc_consensus_progress_gate_seconds	main (#2243)	300

Verified at runtime: PipelineConfig() gives iter=3600, max=14400, gate=300. The _validate_post_consensus_budgets validator survived the merge intact and still rejects max < iter (confirmed: iter=7200, max=3600 → ValidationError; iter=max=2500 accepted).

orchestrator/routes/pipelines.py — the auto-merge produces a clean three-stage layering:

1. Pre-timeout: progress gate (_check_brc_progress_gate at line 9563) defers the auto-HITL decision while bus / heartbeat activity is fresh — _progress_gate_deferring initialized at line 10906, checked at line 11342 inside the if elapsed >= consensus_timeout: branch.
2. Timeout fires: _handle_brc_consensus_timeout(...) at line 11375 (only reached once the gate stops deferring).
3. Post-timeout poll loop: this PR's per-iteration rebaseline kicks in (line 11410+). _latest_proposal_ts reads the same tracker the gate consults via get_latest_progress_timestamp, but the two helpers operate on disjoint clocks (gate on wall-clock vs. proposal age; rebaseline on monotonic vs. last-progress-at), so they don't interact except through the shared tracker.

The composition is correct: while the gate is deferring, the post-timeout loop is unreachable (we continue back to the consensus check); once the gate releases, we enter the post-timeout loop with last_progress_at = post_timeout_start — so the rebaseline starts fresh, not contaminated with the gate-deferral wall-clock spend. That's the right semantic.

Test interaction with the merged-in gate

The rebaseline tests in test_post_timeout_rebaseline.py patch peer_consensus.get_peer_consensus_tracker, which the gate also imports — so I traced the gate's behavior under the test mocks:

• Test 1 (test_no_progress_during_wait_force_kills_at_iteration_budget): tracker patched to None → gate's tracker is not None short-circuits → falls through to heartbeat check → no heartbeats registered for "issue-2245"/"coder" in the singleton → gate returns (False, None) → consensus timeout proceeds. ✓
• Tests 2 & 3 (test_fresh_proposal_extends_iteration_budget, test_absolute_cap_bounds_unbounded_proposal_churn): tracker patched to a MagicMock. The gate calls tracker.get_latest_progress_timestamp() (no side_effect set) → returns auto-attribute MagicMock. Then (now_dt - ts).total_seconds() < gate_seconds raises TypeError: '<' not supported between instances of 'MagicMock' and 'int' (verified directly in Python 3.13). The gate's except Exception: catches it, logs WARN, falls through to heartbeat check → no defer.

So the gate does not deflect the rebaseline tests off the post-timeout path. The tests still exercise the production logic they're meant to.

One observation worth flagging (non-blocking): test 2/3 silently exercise the gate's exception-fallback path on every poll iteration, accumulating WARN BRC progress-gate tracker check failed log entries. The tests pass, but they're noisier than ideal and would mask a real gate-side regression. The cleaner fix is to also stub get_latest_progress_timestamp.return_value = None on the mock tracker — one line in _common_mocks would do it. Not a merge regression, just an interaction the merge made visible.

Non-blocking suggestion from prior re-review

The "assert which cap fired" suggestion from the second review (capture caplog.records and assert on "absolute cap reached" for test_absolute_cap_bounds_unbounded_proposal_churn) was not addressed in this merge. Still a non-blocking suggestion — the test does pin exit_code == 1 and stop_container is called, but with iteration_budget == max_total, an off-by-one in the rebaseline branch could let the iteration cap fire first and the test would still pass.

Files changed since 91a7b4b

orchestrator/models.py and orchestrator/routes/pipelines.py — only conflict-resolution / auto-merge content from e5055e5. No PR logic changes since the last review. pytest orchestrator/tests/test_post_timeout_rebaseline.py and test_consensus_polling.py were claimed to pass pre-merge; CI should re-confirm post-merge.

— Authored by egg

[james-in-a-box]

Addressed both non-blocking suggestions from the re-reviews in 209d6c4:

1. Pin which cap fired (review 2 + 3) — test_absolute_cap_bounds_unbounded_proposal_churn now @patch("routes.pipelines.logger") and asserts:

   • "absolute cap reached" appears in mock_logger.warning.call_args_list
   • "iteration budget exhausted" does not appear

   Recovers the specificity the original 5000 / 2500 split had before the cross-field validator made that combination invalid. An off-by-one moving the per-iteration check above the rebaseline (which exit_code == 1 couldn't catch under iteration_budget == max_total) now fails loudly with both assertions.

2. Silence gate exception-fallback noise (review 3) — Both rebaseline tests with a mock_tracker (test 2 and test 3) now stub mock_tracker.get_latest_progress_timestamp.return_value = None. The gate's tracker.get_latest_progress_timestamp() no longer returns an auto-attribute MagicMock, so (now_dt - ts).total_seconds() doesn't raise TypeError and the gate's except Exception: fallback branch isn't accumulating noisy BRC progress-gate tracker check failed WARN entries on every poll iteration. The gate now cleanly falls through to the heartbeat check (which is empty for "issue-2245"/"coder"), returns (False, None), and the rebaseline tests still exercise the post-timeout path they're meant to.

Verified locally: pytest orchestrator/tests/test_post_timeout_rebaseline.py — 11/11 pass; make lint clean.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Lint/Custom Checks": 1}

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with main:

File	Category	Resolution
scripts/file-size-allowlist.yaml	Additive (both sides bumped different baselines)	Kept HEAD's pipelines.py baseline (15593L / 681405B — reflects this PR's changes) and main's select_tests.py baseline (1875L / 75206B — reflects #2262's changes). Verified wc -l -c matches both selected values after the merge.

Resolution rationale: Each side updated the baseline for the file it modified. This PR added lines to orchestrator/routes/pipelines.py (per #2245's rebaseline logic), and #2262 on main rewrote parts of scripts/select_tests.py. The two updates are non-overlapping — both must be kept so the post-merge tree matches the recorded baselines.

Verification:

• scripts/check-file-sizes.py exits 0 (warnings only, no failures).
• yamllint scripts/file-size-allowlist.yaml clean.
• pytest orchestrator/tests/test_post_timeout_rebaseline.py orchestrator/tests/test_consensus_polling.py orchestrator/tests/test_consensus_timeout_recheck.py — 33/33 passing.

— Authored by egg

[egg-reviewer]

Re-review summary

Approve. Both non-blocking suggestions from the prior re-review (e5055e5) are correctly addressed in 209d6c4, the file-size allowlist baseline matches reality, and the merge conflict resolution in fb32c6b is sound. No new issues.

Verification of prior feedback

1. Pin which cap fired (test_post_timeout_rebaseline.py:360-461) — test_absolute_cap_bounds_unbounded_proposal_churn now @patch("routes.pipelines.logger") and asserts both that "absolute cap reached" appears and "iteration budget exhausted" does not. I verified the substrings are unique log sites (pipelines.py:11426 / pipelines.py:11434), so a swap of the two cap checks would be caught loudly. ✓

2. Silence gate exception-fallback noise (test_post_timeout_rebaseline.py:329 and :426) — both rebaseline tests with a mock_tracker now stub get_latest_progress_timestamp.return_value = None. The gate's tracker.get_latest_progress_timestamp() no longer returns an auto-attribute MagicMock that would raise TypeError on (now_dt - ts).total_seconds(), so the gate's except Exception: fallback no longer accumulates noisy BRC progress-gate tracker check failed WARN entries on every poll iteration. The gate now cleanly falls through to the heartbeat check (which is empty for "issue-2245"/"coder"), returns (False, None), and the post-timeout path under test is exercised as intended. ✓

Trace of the absolute-cap test under the new monotonic sequence

iteration_budget=2500, max_total=2500, monotonic returns 0, 3801, 4801, 5801, 6801, 7801, …:

• Calls 1-2: start_time=0, elapsed=3801 → enters timeout
• Call 3: post_timeout_start=4801
• Iteration 1, call 4: now=5801, total_elapsed=1000, iteration_elapsed=1000 → both pass; rebaseline fires (call 5: last_progress_at=6801)
• Iteration 2, call 6: now=7801, total_elapsed=3000 ≥ 2500 → break on absolute cap; iteration_elapsed=1000

So total_elapsed >= post_timeout_max_total is checked before iteration_elapsed >= post_timeout_iteration_budget in the production code (pipelines.py:11423-11440), "absolute cap reached" is the only warning emitted, and the assertions hold. The fresh-proposal test's exit-0 assertion still proves the rebaseline kept the loop alive past the original cutoff.

File-size allowlist (080fcff + fb32c6b merge)

scripts/file-size-allowlist.yaml after the conflict resolution:

File	PR baseline	Actual wc -l -c	Match
orchestrator/routes/pipelines.py	15593 / 681405	15593 / 681405	✓
orchestrator/peer_consensus.py	2003 / 85965	2003 / 85965	✓
scripts/select_tests.py	1875 / 75206	1875 / 75206	✓

Conflict resolution rationale (HEAD's pipelines.py baseline + main's select_tests.py baseline) is correct — each side updated only its own modified file's row. The 080fcff commit message claims "peer_consensus.py grew from 1988 to 2003 lines" but the PR doesn't actually modify peer_consensus.py — that growth came from main. Cosmetic-only, no impact on the lint or correctness.

Files changed since e5055e5

• orchestrator/tests/test_post_timeout_rebaseline.py — 38 lines (test improvements only)
• scripts/file-size-allowlist.yaml — added with correct baselines, conflict resolved correctly

pytest orchestrator/tests/test_post_timeout_rebaseline.py was claimed to pass at 11/11 by the producer; I couldn't run it locally (sandbox lacks .venv).

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

13 previous review(s) hidden.