Skip to content

fix(orchestrator): Keep Home and settle honest when work ends - #5459

Open
mwolson wants to merge 2 commits into
pingdotgg:t3code/codex-turn-mappingfrom
mwolson:fix/orchestrator-settled-lifecycle
Open

fix(orchestrator): Keep Home and settle honest when work ends#5459
mwolson wants to merge 2 commits into
pingdotgg:t3code/codex-turn-mappingfrom
mwolson:fix/orchestrator-settled-lifecycle

Conversation

@mwolson

@mwolson mwolson commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Keeps the home list and thread cards honest when work finishes, archives, or
wakes back up. Users should not see archived threads reappear on Home, settle
a thread that is still Waiting on background work, or lose an explicit settle
when a delayed provider event races the pin.

Also stops the client from spinning forever on a thread that is already gone
(HTTP 404), so deleted or never-created threads do not keep the UI in a loading
retry loop.

Why this matters

Without these lifecycle fixes, day-to-day product failures look like:

  • Home list lies: an archived thread sits on the active list after reconnect
    or a dropped archive delta, so the inbox feels "sticky" and hard to clean up.
  • Settle is unsafe or ineffective: settle can park a thread that still has a
    queued turn or background Waiting work, or can leave a pin in place so the
    card never looks truly put away.
  • Delayed activity undoes the user: a late provider event can clear a fresh
    settle or rewind thread ordering after a rename, so the list jumps and settle
    state flickers.
  • Dead threads never finish loading: a definitive missing snapshot keeps
    retrying on the socket path, leaving empty detail views that never settle.

Problem and Fix

What the user sees Why it happened Fix
Opening a deleted or never-created thread spins or retries forever. HTTP 404 snapshots were treated as soft failures and fell through to socket retry. Treat 404 as definitive missing: mark the thread deleted and stop the subscription attempt; only fall back to the socket when HTTP is merely unavailable.
Archived threads reappear on Home after refresh or reconnect. Full shell snapshots could mis-partition active vs archive when the same id showed up in both lists or a delta was dropped. Normalize membership from archivedAt, and on dual-list conflicts prefer the fresher updatedAt copy so a stale archive cannot beat a newer unarchive.
Settle parks a thread that is still working, or settle does not stick. Settle allowed queued runs / background Waiting through; activity unsettle races could restore title/archive/model or skip clearing the pin; settle-only merge dropped pin clear. Block settle on queued runs, pending requests, and the background roster; always emit activity-unsettle candidates with ordering; merge only settlement fields (and clear pinnedAt on settle); do not rewind updatedAt behind a newer metadata bump.
Waiting and settled state disagree between sidebar and actions. Client settle eligibility ignored the background roster and Waiting invariants the server already had to enforce. Align canSettle / effectiveSettled with pending background work and the same lifecycle signals the server uses.

Defensive Fixes

Risk Fix
Archive/delete must not leave MCP credentials usable on a shared provider process. Keep revokeMcpCredential: true on terminal provider-session detaches.
Transient projection read failures during activity-unsettle prep either vanished or crashed the stream. Only treat a definitive missing thread as absent; other prep failures surface as ProviderEventPublishError so callers can retry.

Validation

  • Focused suites for settle, projection, archive membership, and missing-snapshot paths.
  • vp check and vp run typecheck clean on the published head.
  • Review findings on pin clear, activity updatedAt rewind, runs filter, and typed unsettle prep errors fixed on the branch.

Out of scope


Note

Medium Risk
Touches orchestration settle/unsettle, projection persistence, and provider ingest ordering—user-visible inbox and thread state—with broad test coverage but non-trivial concurrency edge cases.

Overview
Keeps the home list and settle/unsettle behavior aligned with real thread work, and stops dead threads from retrying forever.

Settlement and provider activity: New shared reduceThreadSettlementEvent merges only settlement fields (and clears pinnedAt on explicit settle) on server projection, client projection, and durable SQL writes—so stale full-thread unsettle payloads cannot rewind title, archive, or other metadata. ProviderEventIngestorV2 prepends synthetic thread.unsettled before qualifying provider activity (pending approvals/user input, live session states), orders clears with provider payload time so delayed events cannot beat a newer pin, and stamps ordinary provider events at ingestion time.

Settle guards: thread.settle rejects archived threads, queued runs (isSettleBlockingRun), pending runtime requests, and pending provider background tasks (via derivePendingBackgroundWork), with specific error messages.

Client shell and threads: canSettle / effectiveSettled block on pendingBackgroundTasks; shell snapshots use normalizeShellThreadMembership and trust archivedAt over stream location so archived threads do not stick on Home. HTTP thread snapshot loads return present / missing / unavailable; a definitive 404 marks the thread deleted and skips socket subscribe/retry.

Tests cover settle invariants, ingest races, projection merge, shell membership, legacy null settlement columns, and thread sync paths.

Reviewed by Cursor Bugbot for commit 5908b04. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix settlement state to reject stale overrides and block on active work in the orchestrator

  • thread.settled and thread.unsettled events now merge only settlement fields (settledOverride, settledAt, pinnedAt, updatedAt) with activity-ordering guards, preventing stale payloads from overwriting unrelated thread metadata on both client (orchestrationV2Projection.ts) and server (ProjectionStore.ts).
  • thread.settle is now rejected when a run is queued, a runtime request is pending, the thread is archived, or the active provider thread has pending background tasks.
  • ProviderEventIngestor prepends a synthetic thread.unsettled event before qualifying provider activity batches, using the activity timestamp for ordering so delayed events cannot clear a newer pin.
  • An HTTP 404 from the snapshot loader now definitively marks the thread as deleted and skips WebSocket subscription and retries; transient failures fall back to socket-only seeding.
  • Shell snapshots are normalized via normalizeShellThreadMembership to place threads in the correct active/archived list, resolving duplicates by preferring the fresher copy.

Macroscope summarized 5908b04.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. 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 UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e833b6dd-b8a9-469f-8d5b-9770617a5322

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.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XL 500-999 changed lines (additions + deletions). labels Aug 5, 2026

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One convention issue found in the new provider-ingest settlement code: a statically known tagged failure is recovered with Effect.catchTag instead of Effect.catchTags.

Posted via Macroscope — Effect Service Conventions

Comment thread apps/server/src/orchestration-v2/ProviderEventIngestor.ts
Comment thread packages/client-runtime/src/state/shellReducer.ts Outdated
Comment thread apps/server/src/orchestration-v2/Orchestrator.ts
@macroscopeapp

macroscopeapp Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR introduces significant runtime behavior changes to thread settlement lifecycle across server orchestration, projection stores, provider event ingestor, and client runtime. The changes control when threads settle/unsettle based on provider activity and background tasks, affecting user-visible thread state. The cross-cutting scope and behavioral nature warrant human review.

You can customize Macroscope's approvability policy. Learn more.

Comment thread apps/server/src/orchestration-v2/ProviderEventIngestor.ts Outdated
@mwolson
mwolson force-pushed the fix/orchestrator-settled-lifecycle branch from 5229063 to 6c322c7 Compare August 6, 2026 00:13
@mwolson mwolson changed the title fix(orchestrator): Stabilize recovery and settled-thread lifecycle fix(orchestrator): Stabilize settled-thread lifecycle Aug 6, 2026
Comment thread packages/shared/src/orchestrationV2Settled.ts
Comment thread packages/shared/src/orchestrationV2Settled.ts
@mwolson mwolson changed the title fix(orchestrator): Stabilize settled-thread lifecycle fix(orchestrator): Keep Home and settle honest when work ends Aug 6, 2026
mwolson added a commit to mwolson/t3code that referenced this pull request Aug 6, 2026
Port pingdotgg#5459 settled-thread lifecycle follow-ups: pin clear on settle, preserve
newer updatedAt on activity unsettle, fresher archive membership, settle runs
filter, and typed activity-unsettle prep errors.
@juliusmarminge
juliusmarminge force-pushed the t3code/codex-turn-mapping branch from 25de21d to 0af2a6e Compare August 7, 2026 12:10
mwolson added a commit to mwolson/t3code that referenced this pull request Aug 8, 2026
pingdotgg#5459's ProjectionSnapshotQuery.settled test must provide the same
ThreadBackgroundLiveness and ThreadPlanProgress layers the live snapshot
query now requires on CTM.
mwolson added 2 commits August 8, 2026 14:41
Stop retrying definitive missing thread snapshots, normalize V2 shell archive
membership, preserve provider retry metadata, and harden settled projection
ordering, activity unsettle, ingest timestamps, snapshot reads, and Waiting
lifecycle invariants.
Settlement-only merge now applies pinnedAt on explicit settle so pins still
clear, and activity unsettles no longer rewind updatedAt behind a newer
metadata bump.
@mwolson
mwolson force-pushed the fix/orchestrator-settled-lifecycle branch from 0a6ccc1 to 5908b04 Compare August 8, 2026 18:43

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 5908b04. Configure here.

const overrideMs = settledOverrideTimestampMs(state);
if (overrideMs === null) return true;
return activityAtMs >= overrideMs;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Keep-active pin clock drifts on metadata

Medium Severity

settledOverrideTimestampMs treats updatedAt as the keep-active pin time, but later renames and other metadata writes advance that same field. Provider activity that lands after the pin can then lose the ordering check and leave settledOverride: "active" in place, so auto-settle stays suppressed even though the server intended to clear the pin on real activity.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 5908b04. Configure here.

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

Labels

size:XL 500-999 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant