Skip to content

fix: stabilize run terminal cleanup - #1906

Open
4pmtong wants to merge 15 commits into
mainfrom
fix/run-terminal-cleanup
Open

fix: stabilize run terminal cleanup#1906
4pmtong wants to merge 15 commits into
mainfrom
fix/run-terminal-cleanup

Conversation

@4pmtong

@4pmtong 4pmtong commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Pull Request

Related Issue

No linked Issue. This bug was found internally while investigating a locally reproduced generated-web task that did not terminate.

Description

This PR fixes Runs that can remain visibly active after background terminal cleanup or workspace finalization fails. The canonical Run could already be terminal while the reusable legacy /chat transport, follow-up ownership, Trigger execution, or stale Project runtime still reflected an active or reusable state.

The final fix:

  • Reconciles the legacy ChatTask from canonical completed, failed, deadline, cancelled, and interrupted events.
  • Rebinds canonical terminal observation, ingress, reducer ownership, and the shared legacy SSE controller whenever the stream moves to a prepared follow-up Run.
  • Marks a completed Run's transport idle before asynchronous END finalization and guards the transition by Run ownership, so an older handler cannot deactivate a concurrently rebound follow-up.
  • Enqueues the Trigger terminal receipt immediately after the owning Run's local END projection, before artifact/history awaits, and reads the execution ID from that owning ChatStore even if a follow-up switches stores concurrently.
  • Separates logical Run activity from physical reusable SSE presence, keeping idle follow-up transports addressable without blocking app close or stale-runtime eviction.
  • Ensures stopTask, removeTask, and explicit SSE close invoked with a follow-up Run ID abort the actual shared transport.
  • Preserves canonical observation after an admitted legacy stream fails or closes, so the authoritative terminal event can still settle the UI and Trigger execution.
  • Uses subscribe-then-read against RunProjectionStore so an already-projected terminal state or an event racing observer setup cannot be missed.
  • Prevents canonical and legacy terminal interleaving from rendering duplicate errors or applying competing Trigger outcomes.
  • Retires a completed Project consumer before admitting a queued background trigger, and waits until the previous step_solve consumer has exited before starting a fresh /chat stream.
  • Serializes legacy chat admission per Project and rejects a second live consumer for the same TaskLock.queue, including requests with different Run IDs.
  • Starts a background trigger with its exact generated Run ID, waits for Brain admission before removing the durable queue item, and requeues a typed project_consumer_active race instead of losing the trigger.
  • Makes stale Project eviction verify and retire the backend consumer even after the renderer transport is gone, while preserving active Runs and reactivated Projects.
  • Sweeps inactive stale Project runtimes on later safe transitions so temporary status or retirement failures can converge without reopening the old Project.
  • Gates warm follow-up admission on older stale-runtime retirement; if no consumer remains, it uses the existing cold-admission path with the same Run ID.
  • Serializes every Trigger execution update per execution ID and makes the renderer's first terminal outcome durable and immutable.
  • Persists terminal Trigger receipts before the first network await and replays undelivered receipts on app startup, reconnect, focus, or a later duplicate receipt.
  • Bounds each Trigger status request to 10 seconds so a hung Running acknowledgement cannot permanently block a queued terminal receipt; terminal delivery still uses a bounded retry round and durable outbox.
  • Re-reads and locks an execution row before the timeout worker applies missed or failed, so a concurrently committed renderer terminal receipt cannot be overwritten; Redis cleanup and timeout counters run only when that transition wins.

The Web Deploy removal and the 30-second terminal checkpoint allowance were split into and merged through #1907, so they are intentionally not part of this PR after rebasing onto main.

Testing Evidence (REQUIRED)

Automated validation completed through commit 30cfae94b:

  • npx vitest run test/unit/hooks/useBackgroundTaskProcessor.test.tsx test/unit/store/chatStore.test.ts test/unit/store/projectStore.test.ts test/unit/components/ChatBox.test.tsx test/unit/service/triggerApi.test.ts — 172 passed. Coverage includes non-awaited END → NEW_TASK_STATE, the preceding Run's Trigger receipt, canonical/legacy deduplication, canonical settlement after legacy failure/close, exact-ID background admission, durable queue retention, typed admission races, backend consumer retirement, stale Project retry/reactivation, bounded Trigger delivery, and durable outbox replay.
  • uv run pytest tests/app/controller/test_chat_controller.py tests/app/run_runtime/test_coordinator.py -q in backend — 62 passed, 1 skipped (six existing AsyncMock warnings).
  • .venv/bin/pytest tests/test_trigger_execution_status.py -q in server with the test SQLite configuration — 7 passed (one dependency compatibility warning).
  • npm run type-check — passed.
  • ESLint for every changed frontend source and test file — passed.
  • Prettier check for every changed frontend source and test file — passed.
  • Ruff for every changed Brain Python source and test file — passed.
  • Server Ruff E/F/B checks for the touched Trigger service, scheduler, and regression test — passed after excluding the repository's pre-existing E501 line in trigger_service.py; that unrelated line was not mechanically reformatted.
  • npm run check:design-tokens — passed.
  • git diff --check origin/main...HEAD — passed.

Human UI evidence has not yet been attached. This PR changes visible frontend Run-state behavior, so the frontend/UI evidence checkbox remains unchecked until the local E2E scenario is verified and evidence is attached.

  • I have included human-verified testing evidence in this PR.
  • This PR includes frontend/UI changes, and I attached screenshot(s) or screen recording(s).
  • No frontend/UI changes in this PR.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Contribution Guidelines Acknowledgement

Pending confirmation by the human contributor; this is intentionally not checked on their behalf.

@4pmtong 4pmtong changed the title fix: finalize runs after background cleanup fix: finalize runs and disable remote deployment tools Sep 4, 2026
@4pmtong
4pmtong force-pushed the fix/run-terminal-cleanup branch from 71e39ce to f870a67 Compare September 4, 2026 16:04
@4pmtong
4pmtong force-pushed the fix/run-terminal-cleanup branch from f870a67 to 7514239 Compare September 4, 2026 16:05
@4pmtong 4pmtong changed the title fix: finalize runs and disable remote deployment tools fix: stabilize run terminal cleanup Sep 4, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.

Tip: disable this comment in your organization's Code Review settings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant