fix(backend): don't flag healthy sessions as orphaned after upgrading - #3524
fix(backend): don't flag healthy sessions as orphaned after upgrading#3524nova28 wants to merge 4 commits into
Conversation
…liation Startup reconciliation flipped every pre-kdlbs#3362 dynamic route to action_required on first restart after upgrade: those routes are durably "starting" only because the "active" status didn't exist yet, not because anything is stuck. A one-time migration backfills legacy starting+IDLE routes to active behind a self-detecting schema marker, so the sweep's IDLE branch still catches genuine post-upgrade orphans. Also close a second guard gap: reconcileOrphanedDynamicStartingRoutes only checked profileExecutionResolver == nil, but the resolver is always constructed regardless of the feature flag, so the sweep ran (and wrote action_required) even with dynamic routing disabled - the default in every shipped profile. It now also checks Enabled(), matching startDynamicPolicyRecovery. Follow-up from PR kdlbs#3362 review (thread r3931855722). Pre-commit hook skipped with explicit user authorization: golangci-lint, gofmt, and sqlguard were all independently verified clean (0 issues) outside the hook; the hook itself was repeatedly timing out at its internal 5-minute budget due to extreme host-wide CPU contention (load average 220-310+ from many concurrent agent sessions), unrelated to this change.
…ts projection write PR kdlbs#3362 review round 1 (F1, F2): the one-time backfill migration lacked the Postgres advisory-lock serialization its sibling one-time migrations take, so two racing initializers could abort the required task store on duplicate-column. It also flipped task_sessions.route_state for any IDLE session reading "starting", not just the rows it had just backfilled to "active" — capable of erasing a live action_required Retry banner on a genuinely broken route. Scope the projection write to the backfilled rows and add the Postgres lock + in-tx marker re-probe, matching git_snapshot_environment_migration.go and worktree_ownership_migration.go.
The already-exists branch returned success without committing, so a failed backfill could leave the marker column present with the deferred rollback discarding the two backfill UPDATEs, permanently skipping the repair on every future boot.
GitHub Actions
Inactive
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe change prevents orphan recovery from modifying routes when dynamic routing is disabled. It adds a marker-gated migration that converts eligible legacy ChangesDynamic route recovery
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The migration and disabled-routing recovery behavior are covered without an identified current-head regression. Sequence Diagram(s)sequenceDiagram
participant Repository
participant Database
participant PostgreSQL
Repository->>Database: Probe for the marker column
Repository->>Database: Begin migration transaction
Repository->>PostgreSQL: Acquire advisory lock when using PostgreSQL
Repository->>Database: Convert eligible starting routes to active
Repository->>Database: Update task session projections
Repository->>Database: Add marker column and commit
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. A rabbit guards the route tonight Comment |
|
Claude finished @nova28's task in 3m 47s —— View job FindingsSuggestion (recommended, doesn't block)
What I checkedMigration logic (
Schema (
Migration position (
Orchestrator guard (
Tests
Summary
Verdict: Ready to merge. The migration is correctly idempotent, the PostgreSQL concurrency story is sound, the orchestrator guard fix is straightforward, and the test coverage is thorough. The single suggestion is a comment-only documentation nit that doesn't affect correctness. |
|
| Filename | Overview |
|---|---|
| apps/backend/internal/orchestrator/dynamic_policy_recovery.go | Adds the disabled-feature guard, but this can make a STARTING dynamic route unrecoverable after general reconciliation changes its session state. |
| apps/backend/internal/task/repository/sqlite/dynamic_route_legacy_active_migration.go | Implements a transactional, one-time cross-dialect legacy-route backfill; its production documentation violates the repository's invariant-only comment rule. |
| apps/backend/internal/task/repository/sqlite/base_migrations.go | Runs the backfill after required dynamic-route and session columns have been migrated. |
| apps/backend/internal/task/repository/sqlite/base_schema.go | Adds the marker column to fresh-install schema so new databases correctly bypass the legacy backfill. |
| apps/backend/internal/orchestrator/dynamic_route_recovery_test.go | Covers disabled routing as an immediate no-op, but not the subsequent disabled-restart and re-enable lifecycle. |
| apps/backend/internal/task/repository/sqlite/dynamic_route_legacy_active_migration_test.go | Thoroughly covers migration, replay, fresh-install, and projection edge cases on SQLite. |
| apps/backend/internal/persistence/storeconformance/upgrade_test.go | Adds cross-dialect previous-stable upgrade coverage for the legacy route shape. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Backend startup] --> B{Legacy marker exists?}
B -- No --> C[Backfill starting plus IDLE routes to active]
C --> D[Add marker in same transaction]
B -- Yes --> E[Skip backfill]
D --> F{Dynamic routing enabled?}
E --> F
F -- Yes --> G[Sweep orphaned starting routes]
F -- No --> H[Skip dynamic-route sweep]
G --> I[General session reconciliation]
H --> I
I --> J{Session was STARTING?}
J -- Yes --> K[Session becomes WAITING_FOR_INPUT]
K --> L[Later enabled restart skips route because state is not orphanable]
Reviews (1): Last reviewed commit: "fix(backend): fail the legacy dynamic-ro..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 745da64683
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Round-1 PR fixup on kdlbs#3524: the comment described the projection UPDATE as scoped to rows the prior UPDATE just backfilled, but the IN-subquery matches every already-active dynamic_route_states row, including ones active before this migration ran. Behavior was already correct (the outer route_state = 'starting' filter excludes them); only the comment was misleading. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Tip
PR walkthrough: Open the visual walkthrough
Before PR #3362, a successful dynamic launch never durably advanced past the
startingroute state (there was noMarkActive), so on the first backend restart after upgrading to that PR,reconcileOrphanedDynamicStartingRoutesmisread every pre-existing healthy IDLE dynamic route as orphaned and flipped it toaction_required— a false-positive Retry banner on otherwise-healthy Office sessions. This adds a one-time backfill so those legacy routes are markedactivebefore the sweep ever sees them.Today: On the first restart after upgrading past PR #3362, every pre-existing healthy IDLE dynamic-routed Office session gets a spurious
action_requiredRetry banner, because the orphan sweep can't distinguish "never marked active because the marking mechanism didn't exist yet" from "genuinely orphaned mid-launch."After this: A one-time migration backfills
activeonto legacystarting+IDLE routes before the orphan sweep runs, so the upgrade is silent for healthy sessions; the sweep still catches genuine post-upgrade orphans.Who hits this: Any existing install with in-flight Office dynamic-routed sessions at the moment it upgrades past PR #3362 — a one-time event on first restart, not a recurring issue.
Scope: Standalone follow-up from PR #3362 review (thread
r3931855722,dynamic_policy_recovery.go:106), deliberately deferred out of that PR's scope. No sibling PRs.Not here: The sweep's IDLE-is-orphanable classification itself is unchanged and correct for real post-upgrade orphans — only pre-existing legacy routes are backfilled. Also unchanged:
Engine.MarkActive/MarkActionRequiredsemantics, the Retry/Try-next recovery UI, and the feature flag default.Also fixes a second, independent defect found while investigating:
reconcileOrphanedDynamicStartingRoutesonly guarded onprofileExecutionResolver == nil, but that resolver is always constructed regardless of the feature flag — so the sweep ran (and could writeaction_required) even with dynamic routing disabled, which is every shipped profile's default. The guard now also checks!resolver.Enabled(), matching the siblingstartDynamicPolicyRecoveryguard.Validation
go build ./...go run ./cmd/sqlguard ./internalgo test ./internal/task/repository/sqlite/... -run TestBackfillLegacyActiveDynamicRoutes -count=1 -v— 4/4 PASSgo test ./internal/orchestrator/... -run TestReconcileOrphanedDynamicStartingRoutes -count=1 -v— all PASS (in-flight STARTING/IDLE sweep, disabled-flag skip, ordering before general startup reconciliation, 6 non-orphan-state skips)go test ./internal/persistence/storeconformance/... -run TestPreviousStableUpgrade_BackfillsLegacyActiveDynamicRoutes -count=1 -v— sqlite PASS, pgx self-skips locally (noKANDEV_TEST_POSTGRES_DSN); the pgx variant runs in CI's PG16 persistence job (.github/workflows/backend-tests.yml, full-package run, no-runfilter). Note: the PG18 job's-runanchor does not match this test name, so PG18 does not additionally exercise it — PG16 does, and that job is unmodified by this PR.go test ./internal/orchestrator/... ./internal/task/repository/sqlite/... ./internal/persistence/storeconformance/...(full packages, post-rebase) — all greenmake typecheck,make lint(golangci-lint run ./...→ 0 issues),make lint-format,cd apps/web && pnpm run i18n:ratchet— all greenapps/backend/internal/, none underapps/web/main(401947f); no conflicts, diff scope unchanged (7 files, 506 insertions / 2 deletions)Possible Improvements
Low risk: the backfill runs once (gated on a schema marker column, following the existing
worktree_ownership_migration.go/git_snapshot_environment_migration.gopattern), is scoped to IDLE routes still instartingstate, and cannot make anything worse than the pre-#3362 status quo it preserves. It genuinely cannot distinguish a healthy legacy route from a true pre-upgrade orphan (the two are byte-identical on a legacy DB) — this was raised and rejected during review because the alternative (no backfill) false-positives on the healthy majority instead of the stranded minority, and the session is IDLE so the next prompt claims a new generation regardless.Checklist
apps/web/), I have added or updated Playwright e2e tests inapps/web/e2e/and verified them withmake test-e2e.docs/public/**and updated them or noted why no docs change is needed.Related: #3362 (comment)
Preview Environment
6bd3978