Skip to content

fix(server): threads swept by old desktop builds show their real settled time again - #15948

Open
shivamhwp wants to merge 2 commits into
mainfrom
fix/repair-client-settlement-timestamps
Open

shivamhwp wants to merge 2 commits into
mainfrom
fix/repair-client-settlement-timestamps

Conversation

@shivamhwp

Copy link
Copy Markdown
Collaborator

Problem

Desktop builds 0.0.35 to 0.0.38 settled idle threads from the client in one burst and stamped each thread's settled time with the sweep time instead of its last activity. Those threads still sit in the sidebar's Settled section with the wrong age, bunched together as if they were all closed at the same moment. Migration 046 repaired the server-attributed version of this, but it skips client-attributed events on purpose, so these were never fixed. The V1 to V2 import then copied the bad values into V2 (#10937).

Change

New migration 057 finds the client-attributed sweeps and puts the real last-activity time back:

  • Candidates: thread.settled V1 events with actor_kind = 'client', an app version from 0.0.35 to 0.0.38, before 2026-09-05, where the event's settledAt equals its occurred_at (the sweep's stamp).
  • A sweep is 10 or more distinct threads whose events are each no more than 30s apart, within two minutes overall. Grouping by the gap between events, rather than "anything within two minutes", keeps a later manual settle from getting pulled into the sweep. That was the review finding on the earlier attempt.
  • It repairs both places the value lives: V1 projection_threads.settled_at for threads not imported yet, and settledAt inside orchestration_v2_projection_threads.payload_json for threads already imported into V2.
  • Event history is untouched. A second run changes nothing.

This rebuilds #10975 by @Gigioxx on current main, which has one migration chain for V1 and V2. 056 was the last migration on main.

Scope and approval

Closes #10937. The issue is triaged and accepted. Julius's triage confirmed the gap in 046 and the discriminators used here: client actor, the affected app versions, the cutoff date, and a dense burst.

Verification

There's no new UI. The bug is old data in existing databases, so the evidence is the migration test run against both branches. The test seeds a 12-thread client sweep plus the cases that must not change, then runs every migration.

Before (main 1e2ecbd, with the new test file copied in): the swept thread keeps the sweep time. expected '2026-09-04T13:33:00.000Z' to equal '2026-04-01T00:00:00.000Z'.

10975-before.mp4

After (this PR): passes.

10975-after.mp4

vp test run apps/server/src/persistence/Migrations/057_RepairClientSettlementTimestamps.test.ts covers:

  • the 12-thread sweep, on both the V1 and the already-imported V2 rows
  • a manual settle 45s after the sweep, which stays as it was
  • a cohort smaller than 10 threads
  • one thread settled 10 times
  • a cohort spread over more than two minutes
  • the wrong app version, missing version metadata, and events after the cutoff
  • running the migration again, and event history staying unchanged

The neighboring migration tests (046, 054, 055, 056) pass. 055_OrchestrationV2.test.ts changes only because it lists the migrations by count. Scoped tsc and lint are clean.

Not checked: a real database from an affected 0.0.35 to 0.0.38 install. An independent review also replayed the original audit fixture through migration 57 and saw every swept thread repaired.

Made with Claude Opus 5.5 in Claude Code (T3 Code).

🤖 Generated with Claude Code

Old desktop builds (0.0.35-0.0.38) swept idle threads into "settled" and
stamped settled_at with the sweep time instead of the thread's real last
activity. Migration 046 only caught the server-side version of this bug
(#10937). The client-attributed sweeps stayed broken, both in the legacy
V1 table and, after import, in the V2 projection.

Add migration 057. It detects a burst of at least 10 distinct threads
settled within two minutes by one of the affected builds, using gap-based
clustering so a later, unrelated manual settle just outside the sweep
does not get pulled in. That is the correctness gap Macroscope flagged on
the earlier, now-closed PR #10975. The migration repairs
projection_threads.settled_at for threads not yet imported into V2, and
patches orchestration_v2_projection_threads' JSON copy for threads
already imported before this fix shipped.

Reuses the detection shape (affected app versions, cutoff date, burst
threshold) from Guillermo Casanova's closed PR, rewritten for the V2
projection and with a non-chaining cluster test instead of the
pivot-window check that PR's review flagged.

Co-Authored-By: Guillermo Casanova <75276669+Gigioxx@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 5, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This PR adds a startup migration that uses substantial heuristic SQL to classify historical settlement bursts and permanently rewrites timestamps in both V1 and V2 projections. The cross-projection data repair and durable user-visible impact warrant human review.

Notes:

  • No code objects were reviewed. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

⚠️ The thread fixture changed, so impact percentages are not directly comparable to the main baseline.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire — 5.0 KiB — 6.8 KiB ✅
Codex Thread snapshot wire — 3.8 KiB — 4.9 KiB ✅
Codex Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Codex Live turn WebSocket decoded — 20.9 KiB — 29.3 KiB ✅
Codex Live turn messages — 2 — 8 ✅
Claude Total thread wire — 5.0 KiB — 6.8 KiB ✅
Claude Thread snapshot wire — 3.8 KiB — 4.9 KiB ✅
Claude Live turn WebSocket wire — 1.2 KiB — 2.0 KiB ✅
Claude Live turn WebSocket decoded — 21.2 KiB — 29.3 KiB ✅
Claude Live turn messages — 2 — 8 ✅

Baseline: 408ff8a · PR result: 5fe5cf4 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 5, 2026
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
docs/internals/effect-services.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 17a18e84-6b4f-4336-8eb5-77a20a8a67c4
📥 Commits

Reviewing files that changed from the base of the PR and between bed1938 and 5fe5cf4.

📒 Files selected for processing (1)
  • apps/server/src/persistence/reconcileV2PreviewMigration.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

Migration 57 repairs qualifying client-attributed settlement timestamps in the V1 and V2 thread projections. It leaves historical event payloads unchanged and adds coverage for migration registration, repaired cases, excluded cases, and repeat execution.

Changes

Settlement timestamp repair

Layer / File(s) Summary
Detect settlement bursts and repair projections
apps/server/src/persistence/Migrations/057_RepairClientSettlementTimestamps.ts
The migration identifies qualifying client-attributed settlement bursts and sets projection timestamps to the latest valid activity before settlement, or the thread creation time. It updates both V1 and V2 projections; it does not change event history.
Register and validate migration 57
apps/server/src/persistence/Migrations.ts, apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts, apps/server/src/persistence/Migrations/057_RepairClientSettlementTimestamps.test.ts, apps/server/src/persistence/reconcileV2PreviewMigration.test.ts
The migration runner registers migration 57. Tests cover migration ordering, repaired timestamps, excluded cases, preserved V2 payload fields, unchanged event history, and a no-op second run.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: 🔵 Low · up to 5fe5c

A legacy manual archive interleaved within a qualifying sweep could appear to have settled earlier in the projection. This is a narrow data-quality risk; the original event history remains available.

Architecture Summary

Architecture risk: 🔵 Low · up to 5fe5c

The change affects 1 system.

Changed systems: apps/server

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — apps/server (service) was modified; 5 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in apps/server/src/persistence/Migrations.ts: The migration runner now statically imports migration 57, RepairClientSettlementTimestamps.
  • observed — Modified behavior in apps/server/src/persistence/Migrations.ts: The migration entries now register migration 57 as RepairClientSettlementTimestamps.
  • observed — Modified behavior in apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts: The contiguous migration count expectation increases from 56 to 57.
  • observed — Modified behavior in apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts: The expected migrations executed after upgrading from schema 53 now include migration 57, RepairClientSettlementTimestamps.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the primary change: restoring correct settled times for threads affected by older desktop builds.
Description check ✅ Passed The description covers the problem, change, scope and approval, and verification. It also states the untested real-database case and provides focused test results.
Linked Issues check ✅ Passed Migration 057 addresses #10937. It selects affected client-attributed settlement events, identifies dense bursts, and restores timestamps from thread activity in both V1 and imported V2 projections. T…
Out of Scope Changes check ✅ Passed The changes support #10937. Migration registration, migration-count expectations, and regression tests are directly related to the timestamp repair. The current incremental changes only add migration …
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

This branch has not been deployed

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

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:L 100-499 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

046_RepairAutomaticSettlementTimestamps misses pre-fix auto-settle events tagged actor_kind: 'client'

2 participants