Skip to content

fix(sessions): settle executions after runner or sandbox loss - #6501

Merged
mmabrouk merged 58 commits into
feat/session-controlfrom
feat/session-execution-watchdog
Sep 4, 2026
Merged

fix(sessions): settle executions after runner or sandbox loss#6501
mmabrouk merged 58 commits into
feat/session-controlfrom
feat/session-execution-watchdog

Conversation

@mmabrouk

@mmabrouk mmabrouk commented Sep 2, 2026

Copy link
Copy Markdown
Member

Context

When a runner died mid-turn, the session stayed "running" forever. When the runner came back after the watchdog had already ended the turn, its late output landed in the transcript beside the ending, so one turn showed two endings. This PR is the watchdog half of the Stop package in the session-control design (PR #6495), stacked on the durable Stop command in PR #6503.

Changes

The branch carries three groups of commits. Read them in order.

Watchdog and quarantine (the original slice). A sweep every 60 s ends any turn whose runner heartbeat is older than 90 s with one execution_lost error and one done, keyed on heartbeat age rather than on the owner lease. Records that arrive after that ending are kept but marked quarantined_at and hidden from every transcript read, so one ending stays effective. reject is one setting away.

Two sweep fixes found on the integrated stack. A stopped row whose runner died before its terminal record got no ending; now the sweep writes it and releases the dead turn's alive lock. Each sweep pass is bounded, and a pass that times out is logged.

Review fixes from 3 September. These came from the staff review of the design and touch the command path as well, so they sit here rather than on #6503:

  • The rollback switch AGENTA_SESSIONS_DURABLE_STOP and the legacy cancel response moved down into PR feat(sessions): deliver durable Stop directly to the runner #6503 on 2026-09-04, so feat(sessions): deliver durable Stop directly to the runner #6503 is safe alone. This PR keeps AGENTA_SESSIONS_LATE_OUTPUT (quarantine default or reject) and everything below. Rebased on the new feat(sessions): deliver durable Stop directly to the runner #6503 head; 26 commits; head 43d77f1989.
  • A pending Stop whose runner is still alive is redelivered with the same command id, bounded by max_deliveries; a Stop whose runner is gone settles lost. Before this, the sweep skipped that case and the session read "stopping" forever.
  • One terminal outcome per execution is enforced by the database: a new core table session_executions takes a compare-and-set from the runner outcome route and from the watchdog; the loser gets a clear "lost the race" result. Records ingest only reads that state, fails open when core Postgres is down, and quarantines only output written after an involuntary ending (lost, or stopped by the other writer). A usage that trails its own done is ordinary history.
  • Command settle, execution terminal claim, stopping-marker clear, liveness mirror, and interaction cancel commit in one core transaction; the Redis write happens after commit and the sweep repairs a missed one. The cancel notice is published after commit.
  • Runner: a Stop on a session parked on an approval now rejects and clears the gates, waits for the prompt to settle inside the cancel window, and parks warm. A fresh user message after a denied tool part goes to session.prompt with the new text instead of resuming the old prompt. Before this, the next message after an approval Stop answered the tool denial and never saw the new question, on both providers.

Before: POST /sessions/{id}/cancel during an approval → 202, gate cancelled, next message → "The command was refused. How would you like to proceed?"
After: the same Stop → 202, gate cleared and parked warm, next message → the answer to the new question in the same sandbox.

Tests

What to QA

Live cells run on the integrated stack with the release-gate driver from PR #6518 (session_control.py). The morning report on PR #6505 lists the results per harness and provider. Watch for: stop-approval on Pi local and Daytona (the fix above), post-stop-row (is_running false within seconds of the Stop), stale-tail (late done quarantined), restart-after-stop (native session survives).

Fixes of 2026-09-04 (found by the new failure cells)

  • Runner-gone: the watchdog was started without the commands service, so an abandoned Stop was never settled lost and a resurrected runner's late report won the compare-and-set. The service is wired at startup with a lifespan test (6d3add4b58).
  • Runner restart: the ending-only sweep branch never cleared the dead runner's owner lease, so the next Send was refused "already running a turn" for up to 120 s. The branch now clears the lease, only for the dead turn's own owner (8b809fafef).
  • Concurrent Stops: the sweep only saw the session's current turn, so a stopped turn of a session that moved on never got its ending. The ending selection is now execution-centric over session_executions (bd5b330b89), with a nullable ending_written_at mark set by the watchdog and by records ingest, a partial index, descending order, and a stopped-shaped ending for a user Stop (migration oss000000026, eee7fd7d33).
    All three were reviewed by the agents that found them. Live re-runs on the merged head follow.

Fixes of 2026-09-04, afternoon (found by the full matrix on the merged head)

  • Stop during a parked approval: the Stop required a settled harness cancel; on a client without cancelSession it threw, tore down the warm sandbox, and wrote no execution row. When no cancel can be sent, the Stop now reparks the sandbox warm and settles as stopped (3e9d5c1651).

  • Watchdog loop dead on the first error: the loop's except branch called log.exception on MultiLogger, which had no such method, so the first failed pass killed the loop for the life of the process with zero log lines. The loop logs with exc_info and MultiLogger gained exception (6e0962cd14, d9df2f2cd4).

  • Sweep poisoned by one unknown command row: a row with a kind this build cannot map raised inside the batch and left every abandoned Stop pending. Both the claim and the abandoned paths now skip unmappable rows through one shared mapper and warn once per batch (757900145c, c663b43b50).

  • Lost settlement left the row running: the watchdog settled the execution lost but session_streams.is_running stayed true until the runner's own beat. The same pass now clears the flag, the running lock, and the mirror on the row that still names the dead turn (d40dd7c9d0).

  • Returning runner re-beat a dead turn: after the sweep cleared the row, an unpaused runner's late heartbeat set it running again. The sweep tombstones every swept turn, and the heartbeat path refuses a tombstoned turn (a8e7920e0f).

  • Hard kill keeps the session affinity: a runner killed with no grace never released owner:session:<id>, so the restarted runner's first heartbeat for the next turn lost the non-stealing owner claim and refused the message with "already running a turn" for up to 120 s; the earlier fix only covered a turn the sweep declared lost. The heartbeat now reclaims affinity from a departed replica when the running lock is free or its own; the alive lock stays the single arbiter. Not behind the flag, because the key and the lock primitives are shared with the legacy path (15d58add1c).

  • The lost settlement did not clear the running flag (finding 7): the watchdog pass wrote the session row's flags through the ORM after nested sessions had detached the row, so the write never reached the database while a plain UPDATE in the same pass did. Both session_streams writes in the sweep are now plain UPDATEs from captured ids; a Postgres test replays the real pass (2cbd1d3cdd, 2613968468).

  • A deleted Daytona sandbox left a dead turn beating for 30 minutes: the provider's proxy keeps answering "not found" for a deleted sandbox, which the liveness probe counted as alive, and the transport's error died unhandled inside the protocol library. A "not found" answer now ends the turn with one error record within seconds; the detector arms only after the sandbox is acquired so a start-up race cannot trip it (1ddea5e81e). Follow-ups: the two-minute first-byte timer never fires; the Python SDK adapter's idle timeout (180 s) is shorter than the runner's own (360 s).

Live on the integration stack after these fixes (Pi local and Codex local, last-message shape): stop-approval, runner-gone-late, stale-tail, sandbox-gone, concurrent-stops, repeat-stop, stop-during-completion, and records-outage pass. The Codex runner-gone-late re-run on 15d58add1c and the runner-gone read-while-paused cell are still open.

Agent-generated, low weight. Not merged.

https://claude.ai/code/session_01GAqSs7fw6QRi2n1ZJ2tmAV

@vercel

vercel Bot commented Sep 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
agenta-documentation Ready Ready Preview Sep 4, 2026 9:52pm UTC

Request Review

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)
  • release/.*

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Team

Run ID: d2c4edab-607f-46af-93e6-7ad4e91b5f73

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mmabrouk mmabrouk changed the title [overnight] feat(sessions): settle executions whose runner or sandbox cannot report an outcome fix(sessions): settle executions after runner or sandbox loss Sep 3, 2026
@mmabrouk
mmabrouk force-pushed the feat/session-execution-watchdog branch from 3f25f06 to 80ad140 Compare September 3, 2026 20:37
@mmabrouk
mmabrouk changed the base branch from agent/session-execution-rfc to feat/session-durable-cancel September 3, 2026 20:37

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

AGENT-GENERATED review, low weight. A read guide and the points to confirm; not an approval.

Read in this order

  1. api/oss/src/utils/env.py + apis/fastapi/sessions/router.py — the durable_stop flag and the legacy-path fallback (the rollback contract).
  2. api/oss/src/core/sessions/commands/service.py settle — the one-transaction settlement across four DAOs.
  3. api/oss/src/dbs/postgres/sessions/executions/dao.py — the terminal compare-and-set and xmax winner detection.
  4. api/oss/src/tasks/asyncio/sessions/orphan_sweep.py — the watchdog, its second selection, and the abandoned-command sweep.
  5. services/runner/src/sessions/turn-settle.ts + engines/sandbox_agent/sandbox-liveness.ts — the runner-side hang and dead-sandbox guards.

Top findings

  • Large PR (27 commits): watchdog + atomic settlement + quarantine + liveness probe + hang guard + heartbeat timeout + the approval-decision-then-prompt fix. All serve one invariant, but a human may want the approval change reviewed on its own.
  • The rollback flag and all new settings are in env.py; migrations are additive with downgrades and chain cleanly.
  • Confirm the shared-transaction settlement rolls back atomically, and that xmax = 0 reliably marks the insert winner under on_conflict_do_update.
  • No records_closed_at leftover in the tree.

Comment thread api/oss/src/apis/fastapi/sessions/router.py
Comment thread api/oss/src/core/sessions/commands/service.py
Comment thread api/oss/src/dbs/postgres/sessions/executions/dao.py
Comment thread api/oss/src/tasks/asyncio/sessions/orphan_sweep.py
Comment thread services/runner/src/sessions/turn-settle.ts
Comment thread services/runner/src/engines/sandbox_agent/sandbox-liveness.ts
Comment thread services/runner/src/lifecycle/session-coordinator.ts
Comment thread api/oss/src/core/sessions/records/service.py
@mmabrouk

mmabrouk commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Fix: parked-approval Stop on the local provider (3e9d5c1651)

What the matrix found. Cell stop-approval on Pi with the local sandbox provider and the last-message client shape: the runner logged harness_cancel sent=false reason=client-has-no-cancelSession, the command settled state=applied outcome=failed, and session_executions had zero rows for the stopped execution. The same cell passed on Daytona.

Cause. A regression in this branch's stopParkedApprovalSession (commit 3264d18cd9, "settle parked approvals before repark"). It required the harness to confirm an ACP session/cancel before reparking. On a client without cancelSession the cancel never leaves the runner, so it threw, tore down (evicted) the warm sandbox, and reported the Stop failed. A failed outcome is not terminal, so SessionCommandsService.settle never writes the execution row (only stopped/lost do). The permission gate is already rejected before the cancel, and that reject is the real stop signal for a parked approval, which runs no turn.

Fix. Split the settle check in stopParkedApprovalSession. When a cancel WAS sent but not confirmed (!settled && requested), still fail closed, unchanged. When no cancel could be sent (!requested), log and repark the warm environment instead of throwing. The Stop then settles stopped, which writes the one terminal execution row, and the sandbox and native session survive (no teardown on that path).

Tests (services/runner/tests/unit/control-command-apply.test.ts):

  • reparks a parked approval warm when the sandbox client has no cancelSession — reproduces the local case; asserts no throw, no teardown, gates cleared.
  • stops a local parked approval, staying warm, and reports it stopped end to end — through applyCommand; asserts outcome applied/stopped, reparked, never evicted.

Runner unit suite: 2690 passed, 4 failed. The 4 failures are pre-existing in gateway-run-turn-composition.test.ts (confirmed failing on the pristine head before this change), unrelated.

Residual race (follow-up, not a blocker). On an unpatched client, reparking without a confirmed cancel can leave the parked prompt open; stray frames from the rejected prompt could demux into a fast follow-up turn. Every current build ships cancelSession, so the confirmed-cancel path is taken in practice and this stays a follow-up. The dev stack that surfaced this ran an older runner image whose baked client had no cancel method.

@mmabrouk

mmabrouk commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Execution watchdog died on its first failing pass

The final QA matrix found the watchdog was not running: the runner-gone and stale-tail cells failed, a command stayed pending past the settle window, and docker logs ... | grep -ci watchdog returned 0 across three hours of a stack that started at 11:41:56Z (json-file driver, no rotation, so the log is complete).

Cause

orphan_sweep_loop caught a failed pass with except Exception: and then called log.exception(...). But log is a MultiLogger, which had every level method except exception and no __getattr__. So the handler itself raised AttributeError, which escaped the while True loop and killed the watchdog task for the life of the process. No error log, no timeout log, no further pass, and stale is_alive rows were never settled.

On the QA stack the first pass ran during the window when migration 026 had not yet added session_executions.ending_written_at. The terminal-execution SELECT raised UndefinedColumnError, the handler crashed on log.exception, and the loop never ran again even after the column was added.

Fix (two commits)

  1. fix(api): keep the execution watchdog alive when a sweep pass raises (6e0962cd14): the error branch now uses log.error("watchdog: error during sweep pass", exc_info=True), the same shape the file's other error logs use. The traceback still renders and the loop goes round again.
  2. fix(api): give MultiLogger an exception method (d9df2f2cd4): a class-wide guard so log.exception(...) never crashes a handler again. This also protects oss/src/utils/emailing.py:136 and :203, the other callers of log.exception.

Tests

  • New loop test drives orphan_sweep_loop over multiple passes: a pass that raises, a pass that times out, and a pass that blocks forever are each logged and the loop survives. Reverting only the fix makes the raising-pass test fail with the exact live AttributeError.
  • New MultiLogger test: the method exists, calling it from an except block does not raise, and it forwards to error with exc_info=True.
  • pytest oss/tests/pytest/unit/sessions plus the MultiLogger test: 566 passed, 70 skipped (the skips are Postgres-gated DAO integration tests, unrelated to this change). ruff@0.15.12 format and check: clean.

Operational note

The task is recreated only by the FastAPI lifespan. Any stack whose API process started while a sweep pass could raise still has a dead watchdog task and needs an API restart to recreate it. With migration 026 applied, a restarted loop settles the backlog and stays up.

@mmabrouk

mmabrouk commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Follow-up: one unmappable command row no longer poisons the whole settle

With the watchdog running again (run 1b settled a lost execution at 141.7 s), every pass then logged watchdog: failed to settle abandoned commands and a Stop stayed pending. Cause: the abandoned-command sweep mapped the entire claimed batch to DTOs before settling any of it. expire_claims ended in [map_command_dbe_to_dto(dbe) for dbe in rows], and a continue_interaction row (increment 6, not on this head) raised ValueError: 'continue_interaction' is not a valid SessionCommandKind. The ValueError escaped the comprehension, so no command was ever settled. Production hits the same shape on any rolling deploy where a newer API writes a command kind an older API's watchdog reads.

Fix (757900145c)

expire_claims now maps the batch through _map_settle_candidates, which skips the rows this API cannot map, warns once per pass with the kinds and count, and returns the rest. The unknown row is left untouched for a replica that knows its kind. The enum and the write path are unchanged.

Test

A unit test seeds an unknown-kind claimed row next to a known abandoned Stop and asserts the Stop survives as a settle candidate, the unknown row is dropped, and the skip is warned exactly once. Verified the old comprehension raises the exact live ValueError while the new path survives it.

Other watchdog reads: no same exposure

  • Execution path (the terminal-execution SELECT, executions.settle, list_redis_unreconciled): reads terminal_outcome as a string compared to literals, no enum built from row data.
  • Stream collapse path (the session_streams SELECT): reads flags as a dict and builds a fresh SessionStreamFlags, no enum parsed from the row.
  • records.settled_turns: selects only session_id, turn_id and compares record_type to a constant, no DTO mapping.
    Only the abandoned-command path parsed enums from row data, so this was the one exposure. Related, out of scope: the DAO's claim path also maps a batch through map_command_dbe_to_dto, so a runner that claims a kind an older replica cannot map would hit the same ValueError on the live delivery path.

Checks

ruff@0.15.12 format and check: clean. pytest oss/tests/pytest/unit/sessions: 566 passed, 70 skipped (the skips are Postgres-gated DAO integration tests, unrelated).

@mmabrouk

mmabrouk commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Two more watchdog hardenings

1. A runner's claim survives a row it cannot map (c663b43b50)

claim_commands ended in the same batch map that broke the abandoned-command sweep: [map_command_dbe_to_dto(dbe) for dbe in claimed]. A newer replica can write a command kind this older replica's enum does not know, and one such row in a claimed batch threw away the whole claim, including a Stop the runner could act on. The sweep's defensive mapper is now shared as _map_commands_skipping_unmappable(rows, context=...) and both the claim and abandoned paths route through it: skip the rows this replica cannot map, warn once per batch naming the kinds, count, and which batch (claimed or abandoned), and return the rest. A unit test claims an unknown-kind row next to a claimable Stop and asserts the Stop is returned and the unknown row dropped.

2. The watchdog clears is_running when it marks an execution lost (d40dd7c9d0)

Run 1c, cell runner-gone (session 5a1df540, execution 4d6df0b3): the execution was settled lost by the watchdog at 13:23:56, but the session_streams row still read is_running true; the flag flipped only at 13:24:52 when the paused runner returned and beat. The SEND gate reads is_running, so with no runner return the next Send is refused forever.

Cause: the lost settlement wrote the terminal records but cleared is_running only on the rows the orphan query collapsed. A lost turn whose row that query did not return (a different row for the session, or an older execution whose row has advanced) kept is_running true. The ending-without-collapse branch now, for every lost turn not collapsed, clears is_running on the row that STILL names the dead turn (keeping is_alive so the session stays resumable), clears the Redis running lock (release_running, turn-guarded), and publishes the mirror change. All guarded on turn_id, so a row that has advanced to a newer running turn is untouched. Two unit tests cover both the clear and the newer-turn guard.

Checks

ruff@0.15.12 format and check: clean. pytest oss/tests/pytest/unit/sessions: 570 passed, 70 skipped (Postgres-gated DAO integration tests, unrelated).

Caveat worth a second look

The live trace also fits a heartbeat-vs-sweep race: a beat from the not-yet-fully-paused runner could re-set is_running true just after the collapse committed. This fix guarantees the sweep leaves is_running false for a lost turn, but does not itself close a racing late beat (that path is the mark_turn_superseded tombstone). Flagging in case run 1c repeats the symptom after this lands.

Symptom: an isolated re-run showed the full cost of the gap. The sandbox was
deleted at 16:26:31, the runner's own socket was told SANDBOX_NOT_FOUND at
16:26:37, and the turn still beat running=true for thirty minutes. What ended it
was the 30 minute per-tool-call deadline, and only then did the turn's error and
done records persist, 27 minutes after the client had given up.

Cause: everything downstream of the turn ending was already correct. The error
terminal, the records, the running=false beat that clears the row and the
teardown all happen within two seconds of the trip. The only defect was WHEN the
turn ended, so the previous commit's unit tests pinned the trigger without
showing the terminal it produces.

Fix: drive the whole path through the real environment wiring with a fake
socket. One test proves a provider answer naming the sandbox ends the turn as a
sandbox_gone error terminal and reclaims the sandbox in the teardown. Its
control proves an ordinary 502 on the same socket leaves the turn running, so a
proxy blip cannot kill a healthy run.

Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Keep quarantine metrics tolerant of records-worker test doubles, align the atomic settlement fixture with the current interaction kind, and retain Ruff formatting after the conflict resolution.

Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
@mmabrouk
mmabrouk force-pushed the feat/session-execution-watchdog branch from 755ef53 to 4aecd1f Compare September 4, 2026 19:58
@mmabrouk
mmabrouk changed the base branch from feat/session-durable-cancel to feat/session-control September 4, 2026 19:58
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

📘 Docs preview

Status ✅ Ready
Preview https://pr-6501-agenta-docs-preview.mahmoud-637.workers.dev/docs
Inspect Actions run
Commit 960923706acdee61e3fc67e24d3917563408f413

This comment updates in place on every push.

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

🤖 The AI agent says:

This PR adds execution settlement, watchdog recovery, and protection against late output.

Focused simplification review using the new simplify skill: one non-blocking suggestion below. This is a static design/code review, not a rerun of the live release gate or a replacement for existing correctness reviews.

Comment thread api/oss/src/tasks/asyncio/sessions/orphan_sweep.py
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Railway Preview Environment

Status Destroyed (PR closed)

Updated at 2026-09-04T21:57:29.819Z

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agent-generated Codex review, low weight.

Verdict: CHANGES NEEDED

I found two additional P1 races in the watchdog's plain session_streams updates; details are inline. Both allow a stale sweep snapshot to overwrite or evict a stream generation that became live while the pass was awaiting other services.

Two earlier review-body findings also remain applicable at this head, so I did not duplicate them as new inline threads: the 30-minute idle/per-tool defaults still abort legitimate quiet tool or subagent work before the supported total deadline, and a settled_turns() lookup exception still lets the caller collapse the row and permanently hide a missing ending. See review #6501 (review).

The rebase otherwise preserves the requested compatibility points: flag-off cancellation still routes through request_cancel_legacy, flag-off late-record quarantine still uses the records fallback, the #6503 admission/replay/Stop fixes remain present, and the #6496 preflight-cancel and scoped-reap files match the base. The sandbox-gone latch is armed only after admission and keeps ordinary HTTP responses non-terminal.

Validation: the API sessions suite passed 684 tests with 9 warnings in 11.05s at nice -n 19. The supplied databases were not empty at their migration ledgers: core was already at OSS revision 027 while this branch ends at 026, and tracing was at 005, so this was a forward-schema run. Current CI also has one failing TypeScript-format gate on packages/agenta-chat/tests/unit/assets/agentTurn.test.ts; other completed checks are green, with acceptance jobs still pending at review time.

Comment thread api/oss/src/tasks/asyncio/sessions/orphan_sweep.py Outdated
Comment thread api/oss/src/tasks/asyncio/sessions/orphan_sweep.py Outdated
Use project, session, turn, and observed heartbeat predicates for each stream write. Only reconcile Redis and watches after a guarded update commits, and leave rows that advanced during the sweep untouched.

Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Track terminal-record lookup failures as deferred candidates. Keep their stream rows and Redis ownership intact so a later sweep can retry safely, and cover the failure-then-recovery sequence.

Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Keep the 30-minute idle and per-tool defaults as the watchdog product bounds. Forward both existing runner environment variables through every OSS and EE Compose service and document the defaults beside them.

Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Teach the Redis sweep fake to return the affected-row count produced by SQLAlchemy updates. This keeps the existing cleanup regressions aligned with the watchdog compare-and-set contract.

Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
@mmabrouk

mmabrouk commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

Agent-generated, low weight. Regarding the run-limit finding in review 5117444093: the 30-minute idle and per-tool defaults are intentionally retained for this round, and the final default remains a product decision for Mahmoud. The trade-off is that 30 minutes bounds a quiet wedged turn (including the Daytona failure this watchdog addresses), while a legitimately silent long-running tool or subagent must opt into a longer window through AGENTA_RUNNER_RUN_IDLE_TIMEOUT_MS and AGENTA_RUNNER_TOOL_CALL_TIMEOUT_MS. Commit c0a5ab9 forwards both overrides into every OSS and EE runner Compose service and documents the defaults.

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agent-generated Codex review, low weight.

Verdict: CHANGES NEEDED

The requested guards, rowcount filtering, post-commit Redis ordering, per-project lookup deferral, and configurable 30-minute idle/tool defaults are present. Two P1 interleavings remain inline.

Comment thread api/oss/src/tasks/asyncio/sessions/orphan_sweep.py
Comment thread api/oss/src/tasks/asyncio/sessions/orphan_sweep.py Outdated
Move the guarded stream update ahead of lost execution settlement and share its transaction. Release only the swept Redis generation with one atomic compare-and-delete script, and cover both ordering races.

Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agent-generated Codex review, low weight.

Verdict: CHANGES NEEDED

The primary rowcount ordering and the fully-installed turn-B cleanup case are fixed, but three race/recovery gaps remain inline.

Comment thread api/oss/src/tasks/asyncio/sessions/orphan_sweep.py
Comment thread api/oss/src/dbs/redis/sessions/contract.py
Comment thread api/oss/src/tasks/asyncio/sessions/orphan_sweep.py
Make established-turn heartbeat writes conditional on the row still naming that turn and on the execution having no terminal outcome. Treat a refused write as a dead turn and cover the sweep-commit-before-heartbeat-write interleaving against Postgres.\n\nClaude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Include the heartbeat turn in Redis owner values while preserving the replica-shaped public API and legacy values. Snapshot the full owner generation for watchdog cleanup, and cover the same-replica refresh gap before the new turn installs its locks.\n\nClaude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Route collapsed legacy rows without a durable turn id through unconditional Redis cleanup. Tombstone the turn ids recovered from stale locks and restore the null-turn regression coverage.\n\nClaude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Keep the nested-session watchdog regression attached to the full owner-value lookup introduced by the affinity generation fence.\n\nClaude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agent-generated Codex review, low weight.

Verdict: CHANGES NEEDED

Two P1 race windows remain inline. The sweep now compares the full generated owner value, legacy bare owner values remain readable/upgradable, and null-turn cleanup plus its tests are restored. I found no separate flag-off regression in this delta.

Comment thread api/oss/src/dbs/postgres/sessions/streams/dao.py
Comment thread api/oss/src/dbs/redis/sessions/locks.py
Require an established-turn heartbeat to observe the stream row as alive and running before it can update the durable mirror. Cover the lock-wait interleaving where the statement snapshot predates the sweep commit and assert the heartbeat update affects no rows.

Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk
Carry the full owner value returned by a failed affinity claim into the departed-replica reclaim. Compare and delete that exact generation so a same-replica new-turn refresh cannot be removed in the reclaim gap.

Claude-Session: https://claude.ai/code/session_0164kzT6ttwpBtzvcDC6YzYk

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agent-generated Codex review, low weight.

Verdict: SHIP

The heartbeat UPDATE is fenced by the stream row’s own turn and alive/running flags, so PostgreSQL’s post-lock row recheck rejects the collapsed row; the real-Postgres test confirms the UPDATE is blocked on the sweep lock before commit and returns rowcount 0. Departed-replica reclaim compare-deletes the exact owner generation returned by the failed claim without re-reading it. No regression found in 5c5355e..9609237.

@mmabrouk
mmabrouk merged commit 1460da7 into feat/session-control Sep 4, 2026
67 of 70 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested lgtm This PR has been approved by a maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant