fix(#6611): make regeneration start atomic - #6677
Conversation
🎬 Cutter preview — PR #6677
|
nesquena-hermes
left a comment
There was a problem hiding this comment.
Thanks @rodboev — the regeneration identity, display-coordinate, gateway-settlement, cancel, and stale-recovery work is substantively strong, and the final no-token correction is valid. A focused exact-head warm-up found one containment gap before this can advance.
CORE: _active_turn_token escapes public message boundaries
The PR intentionally persists _active_turn_token as server-owned recovery authority. _public_session_messages() strips it on selected routes, but the shared redactor does not: api/helpers.py::redact_session_data() recursively redacts values without removing this key. As a result, /api/session/export serializes redact_session_data(s.__dict__) and exposes the token from both messages and context_messages. The authenticated share create/revoke responses and several direct message-bearing session responses also bypass the route-local projection. A sandboxed production-function probe reproduced the JSON-export leak at exact head 40b59f081d9f6e131f7d78bfd60db2ebabc4bcd3.
Fix specification
- Put the removal in one shared, non-mutating public response projection (the shared redaction layer or a helper it always invokes), covering both
messagesandcontext_messages. - Route every outbound message-bearing boundary through it: primary GET, terminal/gateway payloads, JSON/HTML export inputs, share create/revoke, duplicate/update/truncate/branch/compression-recovery, and siblings.
- Strip
_active_turn_tokenfrom public JSON/session imports before persistence so callers cannot inject server-minted authority. - Add boundary tests proving the token remains on disk for recovery but is absent from all public responses/exports and imported/reloaded sessions.
- Rebase the resulting patch onto current master; the present branch conflicts in
api/routes.py.
Focused safe-wrapper evidence was otherwise strong: 235 passed, 2 skipped, 0 failed across regeneration, gateway, cancellation, run-journal, sidecar, and stale-pending recovery slices. The two skips are test-portability cases whose base-commit git show cannot access the outer worktree Git dir in the sandbox. This review is intentionally narrow: do not reopen the converged regeneration identity design.
40b59f0 to
58f8f10
Compare
|
Thanks for catching this. The rework keeps the regeneration identity and display-coordinate design unchanged and closes the missing public containment boundary.
The boundary tests cover exports, shares, local and gateway terminal responses, mutation responses, import and reload, runner and compression payloads, recovery after a partial assistant tail, strict coordinate validation, and the split between durable recovery state and public output. |
|
| Filename | Overview |
|---|---|
| api/routes.py | Moves regeneration mutation, stream registration, persistence, and compensation into the locked start transaction. |
| api/session_ops.py | Adds canonical regeneration planning, revision hashing, selected-turn ownership checks, and aligned display/context truncation. |
| api/helpers.py | Redacts private turn identity while projecting the active user row through a public boolean marker. |
| api/streaming.py | Emits regeneration authority on terminal payloads and preserves retained active-turn identity during settlement. |
| api/gateway_chat.py | Reuses active-turn materialization and preserves regeneration context semantics in gateway settlement. |
| static/ui.js | Prefers the public active-turn marker when reconciling a pending prompt before existing identity and timestamp fallbacks. |
| static/messages.js | Routes browser regeneration through the revision-based chat-start contract. |
| static/sessions.js | Integrates the projected pending-turn identity into session transcript reconciliation. |
| tests/test_issue6611_regeneration_transaction.py | Covers locked settlement, rollback, and competing normal-send orderings. |
| tests/test_issue6611_regeneration_authority.py | Exercises revision issuance and canonical regeneration authority. |
| tests/test_issue6611_regenerate_turn_identity.py | Verifies durable ownership and rejection behavior across WebUI, imported, and forked turns. |
| tests/test_issue6611_regenerate_browser.py | Covers browser reconciliation using the projected active-turn marker despite timestamp drift. |
Sequence Diagram
sequenceDiagram
participant Browser
participant Route as api/routes.py
participant Authority as api/session_ops.py
participant Session as Session persistence
participant Worker as Streaming worker
Browser->>Route: POST /api/chat/start (regenerate + revision)
Route->>Authority: Plan regeneration
Authority->>Authority: Reconcile transcript and validate ownership
Route->>Route: Acquire session lock and revalidate revision
Route->>Session: Truncate to retained user turn and stamp stream identity
Route->>Worker: Start gated worker
Route->>Session: Save accepted regeneration state
Route->>Worker: Release worker
Worker-->>Browser: Stream regenerated response
Reviews (15): Last reviewed commit: "ci: re-trigger (Playwright install hung ..." | Re-trigger Greptile
🔬 Gate certification — RED ⛔Certified head: The direction is right and the token containment (the prior CR basis) is genuinely CLOSED — but the redaction and the regeneration-identity mechanism collide. What I ran
MUST-FIX (blocker, reproduced) — refresh during regeneration duplicates the user row
Reproduced first-party (extracted the real ui.js helpers, drove them with a redacted refresh payload): Codex independently verified the same via the real frontend merge ( Fix spec (Codex + reviewer agree): project a non-secret active-row marker (e.g. a redaction-safe boolean/opaque id that is NOT the raw stream token) before stripping MUST-FIX (blocker, reproduced) — regeneration rejected for string-timestamp sessions
Reachability note: most acute for imported sessions carrying string timestamps; if those are read-only they can't regenerate (lower blast radius), but the validator also rejects any legitimately non-numeric timestamp a real row could carry. Fix spec: accept non-empty string timestamps in BOTH validators and compare the original value exactly (the comparison downstream is Verified SOUND (token containment — the prior CR basis is CLOSED)
Recommendation to the next agentDo not merge. Both are contributor-level fixes reconciling the security containment with the regeneration-identity mechanism: (1) a redaction-safe active-row marker for the frontend dedup, (2) accept string timestamps in both regeneration validators. Gate leg: threat scan → full suite w/ frozen-master baseline reconciliation → Codex reproduce → first-party reproduction of finding #1 + master-delta on finding #2. Opus 4.8 senior leg hit an advisor-infra hang this pass and is being re-attempted; the RED verdict is already established by two independent reproductions (Codex + reviewer) of a reachable regression, so it does not depend on the Opus leg. |
6368e13 to
2c972ac
Compare
|
Thanks, the gate found two separate identity regressions: redacting
I’ll preserve the existing cancellation, recovery, gateway, lineage, provider, and ordinary-send behavior; these changes stay within the active-row projection, regeneration validation, and shared truncation-replay helpers. |
🔬 Gate certification — RED ⛔ (re-gate)Certified head: Prior findings CLOSED (confirmed 3 ways)
What I ran
MUST-FIX (NEW CORE — data loss on runner-backed regenerate)
if runtime_adapter_enabled() or runtime_adapter_runner_enabled():
if regenerate_target is not None and runtime_adapter_runner_enabled():
return {"error": "Regeneration is not supported by the runner backend.",
"code": "unsupported_regeneration_backend", "_status": 409}But the frontend regenerate flow (ui.js) does Non-blocking (Opus advisories, do not gate)
Verified SOUND
Recommendation to the next agentDo not merge. The regeneration-identity fix I bounced for is genuinely resolved and the marker design is sound — but the re-push introduced a data-loss path on runner-backed sessions (delete-then-409). One targeted fix (remove the runner rejection so the runner regenerates, OR guard the frontend truncate behind a capability check). Gate leg: threat scan → full suite w/ frozen-master baseline reconciliation → Codex reproduce (isolated truncate/start repro) → Opus 4.8 senior (GREEN on marker/redaction/watermark) → reviewer diff + ordering confirmation. Backend crown-jewel streaming/regeneration; no browser/UX leg (the regression is a backend guard + frontend ordering, confirmed by code + Codex's runtime repro). |
cd08f80 to
ed4d8d2
Compare
|
I rebuilt this around a single server-owned start transaction. Regeneration now resolves the latest writable local exchange under the session lock, keeps the original user row, and releases a local or gateway worker only after the updated session is saved. Runner-backed regeneration returns before session lookup, workspace recovery, or transcript mutation, while preparation, registration, thread-start, and save failures restore the complete prior state. Settlement also reuses the exact retained row in display and model context, including structured content, attachments, source, stable identity, and metadata. Error, cancel, reload, and state.db recovery therefore keep one user row without losing the replacement assistant. |
|
Fixed in Regeneration stays a single server-owned operation. Runner-backed requests are refused before session lookup or mutation, the route now cleans stale persisted streams before selecting a turn, and workspace recovery waits until start acceptance before updating the sidecar. Regeneration also rejects external session and row sources, while legacy-journal, gateway, and persisted runner-refusal paths have direct coverage. The CI regressions are fixed as well: ordinary gateway pause recovery keeps its prior context fallback, regeneration marks its authoritative empty context explicitly, active-turn checkpoint lookup fails closed without a token, and the affected static regression expectations now match the server-owned flow. |
🔬 Gate certification — RED ⛔ (re-gate — data-loss CLOSED, 2 new CORE from the rewrite)Certified head: CLOSED — cycle-3 data-loss + atomicity (confirmed 3 ways)
What I ran
MUST-FIX #1 (CORE, reproduced) — regenerate silently dies after every normal turn
MUST-FIX #2 (CORE, Codex-reproduced) — wrong-prompt selection + recovered-history data-loss on state.db-reconciled sessions
Verified SOUND (all three legs)
Recommendation to the next agentDo not merge. The hard part — the cycle-3 data-loss and the atomicity token — is genuinely done and race-safe. Two fixes remain, both concrete: (1) emit Gate leg: threat scan → full suite w/ frozen-master baseline reconciliation → Codex reproduce (2 CORE, data-loss+atomicity confirmed closed) → Opus 4.8 senior atomicity (GREEN, race-safe) → first-party reproduction of finding #1 (revision GET-only + wholesale done-replace) + read of the atomicity path. Mixed backend + visible regenerate; the two blockers are backend/wiring, confirmed by code + Codex's runtime repro. |
aeae998 to
20716c2
Compare
|
Thanks, I rebuilt the flow around one canonical transcript instead of adding another turn-specific guard. The server now reads one state.db-reconciled snapshot for the terminal payload, GET authority, revision, turn selection, and truncation. Terminal done, error, and cancel payloads carry the same rows they hash, and the browser waits for the full transcript before requiring the revision. Parent-only, partial, foreign, stale, active, pending, and trailing-tool states fail closed. The start path snapshots before canonical assignment and restores that snapshot on stale truncation or any pre-acceptance failure. Retained timestamps and complete attachment metadata survive regeneration. The focused issue and adjacent tests pass locally. The hosted checks will provide the served browser, runner-refusal, and compensation evidence after this head is pushed. |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Warm-up re-gate — changes requested
Reviewed head: 20716c21db722ddacee7ca6e6f5a4b96d5a25b82
Thanks for rebuilding this around one canonical transcript. The two blockers from the prior gate are closed: terminal payloads now carry a fresh regeneration revision, and revision/turn selection use one state.db-reconciled transcript. The private _active_turn_token boundary also remains closed.
A focused exact-head re-gate found two deterministic state-integrity blockers in the new transaction:
1. The authorized canonical context is discarded before truncation
api/routes.py::_start_regeneration_stream_locked() computes and validates both canonical_rows and canonical_context (21697-21707), but installs only canonical_rows on the session (21708-21710). apply_regeneration_truncation() then truncates session.context_messages, which can still be the stale sidecar context (api/session_ops.py:284-320).
On a state.db-recovered session where canonical context contains rows absent from the sidecar, the revision authorizes the recovered pair but the accepted mutation persists a prefix derived from stale context. That can omit recovered model history from the regenerated turn.
Fix: install both authorized values before mutation:
s.messages = copy.deepcopy(canonical_rows)
s.context_messages = copy.deepcopy(canonical_context)Alternatively, pass the immutable pair into one apply helper that verifies and truncates that exact pair without another authority read. Add a production-composed regression where canonical state.db context differs from sidecar context and prove the recovered prefix survives local and gateway settlement.
2. Rejected starts no longer roll back exactly
api/session_ops.py::snapshot_regeneration_state() snapshots a fixed field allowlist (53-68), but this same start path mutates fields outside it:
_prepare_chat_start_session_for_stream()clearspost_compression_context_tokens_estimate.Session.save()changesupdated_atand_anchor_scene_indexbefore an I/O failure can escape.set_last_workspace(workspace)runs before the acceptings.save()and is not compensated.
The exception path restores only the allowlist and can persist that incomplete restoration with touch_updated_at=False (api/routes.py:21838-21855). A failed pre-acceptance start can therefore return failure while leaving session/global state changed. The previous non-ancestor revision used a complete session.__dict__ snapshot and had a failure-stage matrix; both protections were removed in this rewrite.
Fix: restore a complete transaction snapshot (prefer a deep copy of the mutable session state, or a mechanically complete transaction object), and move set_last_workspace() until after successful s.save() but before worker release. Restore the prepare/journal/registration/thread/save failure matrix and assert exact in-memory plus persisted rollback, including non-null post-compression state and the original timestamps/fingerprints.
Gate evidence
Threat scan: CLEAN. Two mandatory bubblewrap-sandboxed targeted slices passed: 20 passed for the issue-6611 files and 14 passed across existing settlement, terminal-payload, redaction, and share coverage. Those tests do not compose either failing path above; the deleted failure/recovered-context tests are the gap.
No merge or contributor-branch write was performed.
20716c2 to
369f481
Compare
|
Thanks for the exact re-gate. I rebuilt the regeneration start around one lock-scoped display/context pair and a complete session snapshot. The accepted save is now the boundary before workspace updates and worker release, stale plans leave newer accepted state untouched, and failed preparation restores the full pre-acceptance session. The canonical context row receives the same private turn marker as the retained display row, including recovered rows without metadata, and fork-child markers are bound to the current child session. The issue artifact, recovered-context, rollback, lineage, settlement, and adjacent-operation checks pass in the focused suite. |
7dfe500 to
909a41e
Compare
|
Thanks, the production blockers are closed. I kept that implementation unchanged and fixed the two CI integration failures.
I also corrected the PR verification text to describe the tracked fixture and the current focused coverage. |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Deep re-gate: test-portability findings are resolved — two CORE data-integrity regressions remain
Thanks @rodboev — the round-3 test-portability blockers are both closed: the issue-6611 reproduction is now self-contained (tests/fixtures/issue6611_regeneration_rows.json + tests/_issue6611_fixture.py, no more .claude/pr-sweep dependency) and the extracted newSession() harness contract is preserved. The atomic-plan/settlement architecture (single authority read in plan_regeneration, re-validation before mutation in apply_regeneration_plan, full-__dict__ snapshot/restore) is sound in the single-caller case.
Deep review (adversarial reproduction) surfaced two CORE regressions that the green suite does not cover:
1. [CORE] Concurrent send is erased by the request-time rollback — api/routes.py:23393
regeneration_snapshot = snapshot_regeneration_state(s) is captured at request time, before the session LOCK, and _restore_regeneration_preacceptance() restores that whole Session.__dict__. If a concurrent send acquires the session between the snapshot and the restore (setting active_stream_id + pending_user_message), a regeneration that then loses revision validation restores the stale snapshot and wipes the concurrent winner.
Reproduced: regeneration returned 409 while winner-stream and its pending prompt were reset to None — a silent loss of the concurrent turn.
Fix: remove the request-time whole-session rollback. Make the locked regeneration settlement the sole rollback owner — snapshot and restore only inside the per-session lock, after revision revalidation. Keep the pre-lock validation side-effect-free, or revert only transaction-owned fields via compare-and-swap. Add a concurrent-winner reproduction (regeneration loses the revision race → the concurrent send's stream + pending prompt survive intact).
2. [CORE] Regeneration disabled for all writable imported sessions — api/session_ops.py:216
The blanket is_cli_session / foreign-source rejection at lines 216, 277, 287 disables regeneration for every writable imported CLI/TUI/Desktop session — including a completed WebUI-owned turn in such a session. Those sessions remain sendable, so regeneration becoming unavailable is a new functionality loss, not a safety guard.
Fix: replace the blanket rejection with selected-turn ownership validation — permit regeneration of writable imported sessions only when the final user row carries a durable WebUI turn marker (e.g. _active_turn_token), while continuing to reject unmarked foreign turns. This keeps the foreign-turn source guard intact without denying provably WebUI-owned turns.
Both findings independently reproduced at head da6a5cb9. The direction is right and the atomicity core is solid — these are the last two before this critical-capability fix can land. Happy to re-gate immediately once the rollback is lock-scoped and the imported-session gate is ownership-based.
|
Thanks for the deep re-gate. The two remaining data-integrity cases are fixed in
The added coverage exercises both lock orderings, save/reload, GET and terminal revision consumers, and public token redaction. |
nesquena-hermes
left a comment
There was a problem hiding this comment.
Deep re-gate: both data-integrity CORE findings are CLOSED — but a new server→client marker mismatch double-renders the active prompt
Excellent progress @rodboev — both cycle-6 blockers are verified closed (reproduced in real Chromium + memory):
- Concurrent-send erasure (finding #1): CLOSED. The concurrent-send winner now survives the losing regeneration's 409, both in memory and after reload. The whole-
__dict__rollback remains, but the snapshot and every restore now occur inside the session lock after revision revalidation, so no concurrent writer can interleave.lock_held=Truecorrectly usesnullcontext()— no re-acquire of the non-reentrant lock (avoids the #6955/#7107 deadlock class), and the worker is gated only through start settlement, not the streaming lifetime. - Imported-turn regeneration loss (finding #2): CLOSED. Completed WebUI-owned imported turns are accepted; unmarked, malformed, earlier-only, read-only, and foreign final turns remain correctly rejected — precise, not blanket.
- Stale-revision rejection, plan/apply TOCTOU protection, atomic display/context installation all intact. The race test is substantive. 51 regeneration + 41 neighboring tests pass.
Must fix: the new _active_turn_user marker isn't recognized client-side → duplicate pending prompt on mid-stream restore — static/ui.js:10799
The backend now replaces the raw _active_turn_token with a boolean _active_turn_user: true in the public projection (api/helpers.py:1144) — good, it doesn't leak the token. But _pendingActiveTurnUserMessage() (ui.js:10800) still checks only the raw token:
if(!msg||typeof msg._active_turn_token!=='string') return false;For a public-projected message (token stripped, _active_turn_user:true set), this early-returns false → the active-turn identity isn't recognized → mid-stream restore/reattach falls through and materializes another pending user row when timestamps drift. Reproduced: the active user prompt renders twice.
Fix: in _pendingActiveTurnUserMessage, treat msg._active_turn_user === true as authoritative active-turn identity before the token/timestamp fallback, retaining raw-token compatibility for the non-projected path. Add a backend-public-projection→browser probe proving a drifted timestamp does not create a duplicate pending prompt.
This is a clean cross-layer contract mismatch — the server-side projection changed the marker shape and the client reconciliation wasn't updated to read it. Independently confirmed at the code level (helpers.py:1144 sets the boolean; ui.js:10800 only reads the string token). Everything else is solid — this is the last gap. Re-request review once the client recognizes the marker.
|
Thanks for catching the projection boundary mismatch. The server correctly replaces the private token with I’ll make |
Resolved — 5th gate at this head is SAFE TO SHIP (Codex + full suite + independent verify; both CORE data-integrity findings closed, client-marker residual fixed, no regression).
|
Shipped in exp-v0.52.248 — thanks @rodboev. This landed after a thorough multi-round convergence: both core data-integrity guarantees are now in place (a concurrent send during regeneration survives instead of being rolled back, and completed imported CLI/TUI/desktop turns can be regenerated while malformed/read-only/foreign finals stay rejected), plus the active-turn marker is carried through the public projection so a mid-stream reload renders the pending prompt once. Gate: Codex SAFE + full suite green + independent code-level verification of the snapshot/restore-under-lock and marker spoof-safety. Appreciate the persistence through the review rounds. |

Thinking Path
_active_turn_user: true, and browser reconciliation now consumes that marker before timestamp fallback.What Changed
api/routes.pyremoves request-time regeneration rollback and prevents imported-message refresh from replacing the sidecar transcript during regeneration preview. GET revision issuance uses the shared authority after a cheap imported-marker check.api/session_ops.pycentralizes selected-turn ownership. Writable imported sessions require a valid_active_turn_token; read-only sessions, unmarked foreign turns, malformed markers, and invalid fork lineage remain rejected.static/ui.jsrecognizes the projected_active_turn_usermarker when restoring a pending active prompt, while retaining private-token and precision timestamp matching for other transcript shapes.Why It Matters
Regeneration can no longer erase a concurrent user turn, and a writable imported session can regenerate a completed WebUI-owned turn without authorizing an unowned foreign turn. Mid-stream restore also reuses the projected active prompt instead of rendering a duplicate when timestamps drift.
Verification
The public-projection browser regression fails on the prior head because the projected row is not selected, then passes after the marker fix. The focused regeneration, authority, browser, identity, and settlement tests pass with 35 tests. Python, JavaScript syntax, runtime ESLint, diff hygiene, and invariant coverage checks pass; the full suite and hosted browser validation remain CI-owned.
Risks / Follow-ups
Read-only, unmarked foreign, malformed-marker, fork-lineage, and runner-backed regeneration remain unavailable by design. Full CI and hosted browser validation remain the repository's CI checks; this change has no visible UI change and needs no screenshot section.
Contract Routing
Task type: bug fix, server-owned regeneration turn identity and transaction safety.
Touched areas: regeneration preview, locked start settlement, selected-turn ownership, GET and terminal revision consumers, browser pending-row reconciliation, and focused regression tests.
Relevant public docs:
docs/CONTRACTS.md,docs/GUIDELINES.md, and the session SSE contract.Scope boundaries: generic session operations, runner regeneration, browser request shape, schemas, session origin metadata, and public private-field redaction remain unchanged.
Evidence needed before claiming done: current hosted CI, browser smoke, and maintainer re-review must pass on the pushed head.
Model Used
GPT-5.6 via Codex CLI