Repository navigation
fix: stop Parlant session processing and preamble generation - #791
AlexanderZ-Band merged 3 commits into
Conversation
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
Review of PR #791 (STOP/INTERRUPT ends the room's turn on every path).
Blocking: two confirmed stop-before-bind gaps (codex turn/start window, a2a continuation), the lost exception detail on failed backend stops, and test-quality issues (tautological/over-broad letta checks, scheduler-dependent opencode check, missing bridge ERROR-branch and scenario 14/16/18 coverage).
Edge-case/logic hypotheses dropped after reproduction: participant events during hydration (never reach adapters), claude_sdk retire ordering (worker cancelled first), _admit_turn closing re-check, overlapping running blocks, codex teardown drain (_retire_room refuses while stopping), bridge nudge /next window (pre-existing).
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
Cycle 2 (fix-area review of 8674d21..a972bc7): converged, with no confirmed defect.
Dropped after reproduction:
- a2a continuation answered with a different task id: the a2a-sdk reference handlers reject a mismatched or terminal task id.
- codex
turn/startslower than the 5s stop budget: the mechanism reproduces, but no production path reaches it (no request timeout, codex-rs answers after in-process setup, retry backoff is at most about 0.85s).
The remaining nits and suggestions are posted inline.
2f28309 to
4397d79
Compare
bandzalkin
left a comment
There was a problem hiding this comment.
Scope
Adversarial and architectural review of 6546c2c against 69e15a2. Four new reproduced blocking findings are inline (F1–F4). Prior resolved findings and accepted decisions were reconciled, not duplicated.
Constraints
Cancellation must stop backend work without blocking control processing, preserve originating-turn identity, and fence replacement work until stopping settles. Current main (d7ab342) also contains the retained-room ownership cutover from #709.
Decisions
Keep BackendTurns as the remote-cancellation mechanism, but enforce settlement before preparing replacement turn state, retain ownership across timeout transitions, and include engine-owned child generation in completion. Integrate with main's canonical retained-room owner rather than restoring obsolete retirement machinery.
Findings and proof
F1–F4 are P2 merge blockers: OpenCode timeout-abort ownership, Parlant child-generation ownership, ACP rollover isolation, and A2A bootstrap identity. Details and smallest corrections are inline. Focused tests: 510 passed, 4 skipped. Broader affected-consumer tests: 3,681 passed, 72 skipped. Ruff lint/format and Pyrefly passed. Additional behavioral probes reproduced every finding; F1 was compared against the base. Four real-engine Parlant probe outputs matched their expectations, but the additional child-task probe exposed F2. The Parlant pytest lane could not collect in the partial-dependency environment, so engine probes ran directly. No live-provider billing or production late delivery was exercised; no full-repository or merged-result sign-off is claimed.
Runtime admission/setup, synthetic STOP draining and bridge cancellation-isolation attacks were disproved. The PR also currently has merge conflicts in lifecycle owners and related tests; the resolved integration needs fresh verification and review.
Architectural verdict
HELD — not merge-ready. Resolve F1–F4 and the current-main integration conflicts, then verify and re-review the resulting revision.
bandzalkin
left a comment
There was a problem hiding this comment.
Scope
Re-review of 4d8c5cffa789fdc980df2a59eaefa04ce2ffb5d6, focusing on the fixes since 6546c2cc; aggregate PR base 69e15a20. Previous discussions and dispositions were reconciled.
Constraints
Backend cancellation must retain ownership until settlement, fence replacement-turn preparation, and include engine-owned child generation.
Decisions
Keep the shared BackendTurns mechanism. The OpenCode timeout-abort ownership fix (F1), ACP rollover barrier (F3), and A2A bootstrap barrier (F4) address the prior findings in the exercised paths. Parlant's child ownership remains incomplete.
Findings and proof
One failed-fix finding is inline on the current diff: F2 remains blocking, linked to its original discussion. No separate new blocker was confirmed. OpenCode held-abort STOP/INTERRUPT probes passed; ACP/A2A rollover regressions passed. The real Parlant 3.3.2 probe failed at preamble discovery, and independent real-engine diagnostics confirmed surviving child generation and accumulation across turns. Details are inline.
Focused checks: 371 passed, 4 skipped. Broader core/runtime/bridge/ACP/conformance checks: 3,867 passed, 73 skipped. Changed-file Ruff lint/format and project Pyrefly passed. The dev-environment skips include Parlant; its engine was exercised separately with a parked model boundary. The Parlant pytest lane could not collect in the isolated dev-parlant environment because conformance setup required the absent langgraph extra. No full-repository, live-provider, or merged-result sign-off is claimed.
GitHub still reports CONFLICTING / DIRTY; the resolved integration requires fresh verification and review.
Architectural verdict
HELD — not merge-ready. Correct F2, resolve the merge conflicts, and verify/re-review the resulting revision.
|
The merge-conflict note in the 2026-10-15 re-review is real: this branch still conflicts with main after the retained-room owner (#709), stopped-room polling (#804), and the OpenAI adapter split (#802). The overlapping lifecycle files are listed on INT-1809. That integration is outside the Parlant preamble-identity fix. Tracked in https://linear.app/thenvoi/issue/INT-1809/integrate-pr-791s-backend-stop-with-mains-retained-room-owner |
30c1c6a to
3b38739
Compare
3b38739 to
a4b55e1
Compare
a4b55e1 to
8f994dc
Compare
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
Blocking — would be REQUEST_CHANGES on a PR by someone else. Full-PR cycle 1 found one verified issue; root-cause fix and regression validation follow.
8f994dc to
d4866b9
Compare
There was a problem hiding this comment.
Cycle 2 found no production issue, but verification confirmed a clock-fixture regression: virtual ticks coarser than asyncio's native timer tolerance could round a positive preamble timer remainder to zero indefinitely. The hang also reproduced without Parlant.
The confirmed finding is fixed in 32386c1 after the fixture design review. All five actual-engine cases now use host-aligned binary virtual time through cleanup; owned turns are cancelled and joined on exceptional exits. The Parlant lane uses pytest-randomly with a printed replay seed. Four modeled host-clock regressions pass and fail under the old-clock counterfactual. Exact CI selection passes in three distinct seeded orders with no registry leaks. All six live Parlant CI cells pass on Ubuntu/Windows Python 3.11-3.13.
This closes the clock finding. Full-PR cycle 3 is underway; separate ACP local-suite and Strands/NOOA collection-watchdog failures are recorded in INT-1133 and are not being hidden by this fix.
AlexanderZ-Band
left a comment
There was a problem hiding this comment.
Full-PR cycle 3: converged, with no new verified findings across groups A-F. Scope: Parlant process-session/preamble cancellation on STOP, INTERRUPT, timeout and cleanup, plus backend-stop conformance coverage. Both earlier Test Quality findings are fixed; no new unresolved feedback remains.
The independent skeptic dropped the early-reply/untracked-engine hypothesis because the relevant sequence predates this PR and no introduced or newly reachable failure was established.
Not examined: final model generation before the cancellation latch enables; final-message emission after the latch enables; external native-API session restarts concurrent with a Band stop.
All six Parlant CI cells pass on this head. Validation evidence and the separate unresolved ACP local-suite and Strands/NOOA collection-watchdog failures are recorded in INT-1133. Those broader failures remain visible; this review is not an all-CI-green claim.
bandzalkin
left a comment
There was a problem hiding this comment.
Scope
Re-review of 32386c1164472561d3a5849360723ab41e4d4e2a against stack base c81118eabb38006b24f97464c248b3072829fb2f, focusing on the actual Parlant child-task fix and portable/randomized timing coverage since 4d8c5cff. All prior review bodies and 32 thread dispositions reconciled.
Constraints
Shared BackendTurns owns bounded cancellation and settlement. Ending a turn must stop its actual engine-owned preamble, not merely make production discovery stop finding it, while preserving neighboring sessions. Previously accepted final-latch/provider limitations are not reopened.
Decisions
Keep the shared owner and narrow Parlant 3.3.2 compatibility workaround. The corrected AlphaEngine coroutine identity, independently retained child handles and timeout settlement address F2. Architecture fits this scope; dependency-private registry/frame coupling remains a compatibility constraint for future Parlant upgrades.
Findings and proof
No new code findings; previous F2 is verified fixed in the exercised paths. Four attacks covered actual-child identity/session isolation, same-session rollover/cleanup, timeout/shutdown and virtual-clock/randomized isolation. A real installed-engine/native-clock probe completed four independently captured children across STOP, INTERRUPT, replacement, timeout and cleanup while preserving neighboring work.
Seeded dev-parlant execution: 284 passed, two failed, four skipped, 866 deselected; all selected Parlant behaviors and four clock regressions passed. The two failures were global discovery guards selected outside that lane, importing absent google-genai; those guards pass separately in the supported dev environment (two passed). An initial dev invocation rejected the Parlant-only seed option before collection; its corrected invocation produced those passes.
Current-head CI remains red: 40 successful, one failed, one skipped. All six Parlant OS/Python cells passed. The inspected Ubuntu/Python 3.13 Strands/NOOA log shows TestStrandsNooaJobCoverage.test_job_selects_every_required_node timing out after 30 seconds during subprocess collection: 221 passed, one failed, nine skipped. The underlying collection stall was not diagnosed or rerun; it is not asserted to be a new Parlant defect. No full local or paid-provider cancellation proof is claimed.
Architectural and overall verdict
HELD for merge verification, not an outstanding F2 finding. Obtain a green required CI gate or an evidenced resolution of that collection failure before overall sign-off. No duplicate inline issue is posted.
|
The CI verification concern in this review is cleared: the failed Ubuntu/Python 3.13 Strands/NOOA job passed on targeted rerun at unchanged head |
32386c1 to
77e5a2d
Compare
Dismissed as stale: all 32 threads are resolved, the reviewer's later re-reviews (10-15 16:07, 10-18 10:02) found no outstanding code finding, and the branch is rebased onto the current main (conflicts resolved, gate and CI re-run).
|
The merge conflicts raised in the 2026-10-15 re-review are resolved. After #835 landed, the three commits of this PR were replayed onto main; the conflicts were in the Parlant message and response modules and in two CI/test file lists, resolved by keeping main's new logging and this PR's stop return value. On the rebased head 77e5a2d: ruff format and pyrefly are clean and 9281 tests pass. The two pydanticai provider test failures and the ruff findings are in code this PR does not touch and fail the same way on main. CI: 41 checks passed, 1 skipped. All 32 review threads are resolved, and the stale CHANGES_REQUESTED was dismissed. |
Why
Stopping the local Parlant handler leaves engine-owned processing and preamble generation running, allowing stopped-turn work to survive into later turns.
What
Use BackendTurns to stop and join session processing and its preamble child, including response timeouts and cleanup. Align response deadlines with Parlant's event-loop clock and verify real-engine cancellation using controlled time and independently captured processing tasks. Complete the exported-adapter backend-stop conformance guard.
Scope
Final layer of the backend cancellation stack: #834 → #835 → #791. Contributes to INT-1133; routing controls to rooms still starting remains INT-1782. Runtime #832 and bridge #833 are independent.