Repository navigation
Conversation
…ly home The resume fence that stops a settled orchestration worker being relaunched had two homes and a directional gap between them. `sleepingRecordFromEntry` derived `automaticResumeBlockedBy` solely from `automaticResumeBlockedPaneKeys`, which starts empty on every renderer boot and is never persisted, so the first `setAgentStatus` after any renderer start rebuilt the record without the flag. The hibernation planner then admitted the pane, killed a live worker PTY, and worktree activation relaunched it with `--resume`. The record is now the durable home. Every rebuild path resolves the flag through one predicate that reads the record first and the pane-key map second, gated on session identity, so `carryOverAutomaticResumeBlock` and the open-coded carry in `recordAgentProviderSession` collapse into it. `recoveryRecordMatches` treats the flag as significant, so a rebuild that would drop it is no longer accepted as an equal record. The planner and the hibernation coordinator's re-check now read the pane-key map too, which closes the window where a pane is fenced before any record exists. Main stops announcing each fence once per process. `prepare` re-announces every blocked pane on every pass and the renderer-startup handshake returns the fenced pane set, so a renderer that reloads, crashes or opens a second window re-derives its whole map instead of losing the fence. The announce and the lift edge are now committed only after the session write succeeds, so a staging failure retries rather than dropping the fence forever.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change moves legacy worker resume fences from sleeping-session records into a runtime-authored pane-keyed workspace-session map. Recovery now publishes complete fence sets and sends invalidation notifications. Main and preload APIs expose fence reads through IPC. Renderer hydration and refresh logic preserve ordering and scoped updates. Resume, hibernation, parking, and terminal access checks use the fence map. Persistence, compatibility projection, restart recovery, and end-to-end tests cover the new flow. Merge Risk: 🟡 Moderate · up to A stale resume fence can prevent automatic resume for new work, and the recovery retry path lacks effective regression coverage. Resolve these before merging unless the risk is explicitly accepted. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed change rationale, design details, validation results, limitations, and references issue Full details: Docstring CoverageExplanation Docstring coverage is 21.82% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 55 functions across 68 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 1d4ebf05-71fe-4b8d-aeac-00dd222bc99b
📒 Files selected for processing (27)
src/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-04.spec.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-runner.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-types.tssrc/main/runtime/runtime-legacy-worker-terminal-resume-fence.test.tssrc/main/startup/legacy-worker-renderer-recovery.tssrc/preload/api/app-api.tssrc/preload/api/app-bridge.tssrc/renderer/src/app-shell/use-app-startup-hydration.tssrc/renderer/src/lib/agent-hibernation-coordinator.tssrc/renderer/src/lib/agent-hibernation-pane-eligibility-resume-fence.test.tssrc/renderer/src/lib/agent-hibernation-pane-eligibility.tssrc/renderer/src/lib/agent-hibernation-planner-snapshot.tssrc/renderer/src/lib/agent-hibernation-planner.tssrc/renderer/src/lib/live-resume-anchor-record.tssrc/renderer/src/startup/legacy-worker-resume-fence-seed.test.tssrc/renderer/src/startup/legacy-worker-resume-fence-seed.tssrc/renderer/src/startup/startup-degraded-recovery.tssrc/renderer/src/store/slices/agent-status-hydrated-resume-fence.test.tssrc/renderer/src/store/slices/agent-status-provider-session-actions.tssrc/renderer/src/store/slices/agent-status-recovery-collection.tssrc/renderer/src/store/slices/agent-status-recovery-equivalence.tssrc/renderer/src/store/slices/agent-status-sleeping-records.tssrc/renderer/src/store/terminals/terminal-pane-hibernation.tssrc/renderer/src/web/preload-api/web-app-api.tssrc/shared/agent-session-resume.tstests/e2e/settled-worker-resume-fence-restart.spec.ts
💤 Files with no reviewable changes (1)
- src/renderer/src/store/slices/agent-status-recovery-collection.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| if (state.automaticResumeBlockedPaneKeys?.[next.paneKey]) { | ||
| return true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate session identity before accepting the pane-key fallback.
retireAgentPaneAuthority does not clear automaticResumeBlockedPaneKeys. If a pane key is reused before an explicit lift, carriesAutomaticResumeBlock returns true without checking the new agent or provider session. sleepingRecordFromEntry or recordAgentProviderSession can then stamp automaticResumeBlockedBy onto new work and suppress its automatic resume. Make the volatile fence identity-aware and require a matching agent/provider session before carrying it. Add a regression test that reuses the same pane key with a different provider session.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ One rough edge on the session-identity gate — inline comment. Otherwise this is a clean, well-tested fix.
Reviewed changes
- Record is the fence's only durable home —
carriesAutomaticResumeBlockunifies three separate carry-over paths into one predicate (record first, volatile map second) andrecoveryRecordMatches/sleepingRecordsEquivalentIgnoringCaptureTimenow treatautomaticResumeBlockedByas significant, so a hydrated fence survives the first status write after a renderer boot. - Announce-every-pass replaces the once-per-process push — main's
prepare()re-announces every blocked pane and commitslastPlanBlockedPaneKeys(and the lift edge) only after the session write succeeds, so a reload or later-pairing client is fenced and a staging failure no longer consumes the announcement. - Renderer pulls the fence at startup —
recoverLegacyWorkerTerminalsForRendererStartupnow returns{ blockedPaneKeys }, and a new renderer helper seeds the volatile map additively on every startup and degraded-recovery pass. - Planner sees pre-record fences —
AgentHibernationPlannerSnapshotandisAutomaticHibernationAllowedread the blocked-pane map, closing the settled-with-tab-still-open hole. - Tests that can actually fail — three new unit specs plus a new restart/reload e2e spec pin exact flag values (
'legacy-orchestration-worker',undefined), not truthiness.
ℹ️ Nitpicks
- The "Not to be merged without explicit sign-off" paired/web gap (empty snapshot, no wire method) and the reveal-half dependence on #19320 are explicitly flagged in the description, so no further action needed here — just calling out that the fence is effectively local-only until the follow-up lands.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…ed state Codex review of #19344 found the startup reply could re-fence a retired pane. Two paths, one defect: the reply was built somewhere other than the object that owns fence publication. The runner read `plan.blockedPanes` captured before it awaited workspace resolution, inventory and persistence, so a release or takeover landing inside that window produced a reply older than its own lift; and a pass whose `setWorkspaceSession` threw published nothing through the push channel yet still returned those keys, fencing a pane no later pass could retire. The persistence object is now the only publisher. It exposes the set it has actually committed, counted by a commit generation, and the runner reads that after the pass finishes rather than deriving anything from the starting plan. A pass that commits nothing reports the previous committed state, which also keeps an unreadable plan from ever reading as "no pane needs its fence". The generation travels with both channels, so the renderer applies main's whole committed set and drops a reply that a newer lift has already overtaken. That makes the seed authoritative instead of additive, which is what finally retires a fence on a pane that has no sleeping record to sweep. Manual sleep and hibernation rollback both committed a record captured before the await, erasing a fence delivered during the stop. Both now resolve the flag from state at commit time through the same predicate as every other rebuild. Also: the degraded-startup routing gate searched for the call the seed helper replaced, so its ordering assertion silently passed on -1; it now follows the helper and asserts the indirection still reaches main. The shared fake-CLI e2e fixture is re-materialized on demand, because the first spec file's teardown deleted it out from under the next file in the same worker.
…one home The fence was main-owned state stored inside a renderer-owned field. `sleepingAgentSessionsByPaneKey` is co-authored: the renderer emits the whole map and main applies it as a wholesale replace, so main's stamp survived only until the renderer's next write. That inversion is what produced the original P0 — the fence was erased on disk, not just in memory — and every guard added since was compensation for it: a carry predicate at each record builder, two equality hunks behind that predicate, a volatile pane-key map with a seeded startup reply, and main-side bookkeeping to synthesise lift edges for panes with no record. The fence now lives in `legacyWorkerResumeFencesByPaneKey`, a runtime-authored session field classified like `clientHostedBrowserPagesByWorktree`. It is absent from the renderer's patch builder, so the renderer cannot write it, and the persistence layer carries it across any write that never mentions it. Main rewrites the whole set every recovery pass, record or no record. Level-triggered: no announcement to consume once, no edge to miss, no generation to compare. The renderer reads it through ordinary session hydration and re-reads it when main pings that it changed; the ping carries no state, and reads are coalesced so the last read is the last write. Hibernation eligibility, the resume decision, the reattach choice and the park exemption all consult that one field. `automaticResumeBlockedBy` stays on the record as an outbound projection for clients too old to read the new field, stamped in `buildSleepingAgentSessionData` alone — the single function both persistence paths build records through. Deleted: the pane-key map and its writer, the startup seed helper and the reply shape it needed, `carriesAutomaticResumeBlock` and its call in every record writer, both equivalence hunks, `lastPlanBlockedPaneKeys`, the fence generation, and the record-sweeping half of `liftRetiredFences`. All five Codex findings and all five Opus findings are now unreachable rather than handled.
There was a problem hiding this comment.
ℹ️ One consistency gap on the repo-removal disposition — inline comment. Otherwise the round-2 redesign is a clean, well-tested resolution of the two-home inversion found in round 1.
Reviewed changes
- Moved the fence off the sleeping record onto a runtime-authored session field —
legacyWorkerResumeFencesByPaneKeyis now main's only durable home, classified likeclientHostedBrowserPagesByWorktree(RUNTIME_AUTHORED_FIELDS, ownershippaneKeyed), so the renderer's patch/payload builders never emit it andpreserveRuntimeAuthoredWorkspaceSessionFieldscarries it across writes that omit it. - Replaced the once-per-process push with a level-triggered whole-set rewrite plus an invalidation ping —
prepare()rebuilds the fenced set per host on every pass (content-compared viasameFenceSetto avoid waking subscribers) and pingslegacyWorkerTerminalResumeFencesChanged; the renderer pulls via the newapp:getLegacyWorkerResumeFencesinvoke, unioned across host partitions. - Deleted the volatile pane-key map and every renderer-side carry path —
carriesAutomaticResumeBlock,automaticResumeBlockedPaneKeys, the seed helper,lastPlanBlockedPaneKeys, and both equivalence hunks are gone; the record flag is now only an outbound projection stamped at one site inbuildSleepingAgentSessionData. - Routed every decision through the fence set — hibernation eligibility, resume (
resumeSleepingAgentSessionsForWorktree), reattach (sleeping-record-access.ts), and park exemption all readlegacyWorkerResumeFencesByPaneKey; the record flag is never an input. - Rewrote the unit and e2e specs around the new field — exact-value assertions on the fence set, including the multi-host (ssh/remote) recovery specs and a restart/reload e2e that pins the fence to disk.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| // transferred project carries no dispatch rows to re-derive it from. The destination runtime | ||
| // re-stamps whatever its own DB still claims on its first recovery pass. | ||
| legacyWorkerResumeFencesByPaneKey: { | ||
| onRepoRemoval: 'prunedByBespokeRule', |
There was a problem hiding this comment.
prunedByBespokeRule promises a removal rule that removeRepoFromWorkspaceSession never implements for this field. terminalPtyIncarnationsByPaneKey gets its pane-key filter rule, and this field is pane-keyed the same way, so removing a repo leaves its fence entries behind as inert residue — which contradicts the census system whose whole point is that a classified field is handled by construction.
Technical details
# legacyWorkerResumeFencesByPaneKey has no bespoke repo-removal rule
## Affected sites
- src/main/orca-profiles/profile-project-session-field-disposition.ts:115 — classified `onRepoRemoval: 'prunedByBespokeRule'`
- src/main/orca-profiles/profile-project-session-state.ts — `removeRepoFromWorkspaceSession` has bespoke rules for `terminalSurfaceTombstonesByPaneKey` and `terminalPtyIncarnationsByPaneKey` but none for this field
## Required outcome
- A removed repo must not leave fence entries for its panes in the session, or the classification must say so honestly (`notRepoScoped` with a comment noting main's level-triggered rewrite owns the field's lifecycle).
## Suggested approach (optional)
- Mirror the `terminalPtyIncarnationsByPaneKey` rule: filter entries by `paneKey.slice(0, paneKey.lastIndexOf(':'))` against `removedTerminalTabIds`. Add a census test case so a future reclassification can't silently drop the rule.
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 75faa862-44d4-4fe2-9839-b61940b6a475
📒 Files selected for processing (78)
config/reliability-gates.jsoncsrc/main/orca-profiles/profile-project-session-field-disposition.tssrc/main/persistence/legacy-worker-session-projection.tssrc/main/persistence/loading-store/session-host-partitions.tssrc/main/persistence/loading-store/store-runtime-authored-session-fences.test.tssrc/main/persistence/restoring-sessions/session-worktree-ownership.tssrc/main/persistence/runtime-authored-workspace-session-fields.test.tssrc/main/persistence/runtime-authored-workspace-session-fields.tssrc/main/runtime/orca-runtime-fence-automation-owner.tssrc/main/runtime/orca-runtime-preserved-branch-cleanup.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-02.spec.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-03.spec.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-04.spec.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-05.spec.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-06.spec.tssrc/main/runtime/runtime-legacy-worker-host-write-publication.test.tssrc/main/runtime/runtime-legacy-worker-recovery-publication-order.test.tssrc/main/runtime/runtime-legacy-worker-session-publication.test.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.tssrc/main/runtime/runtime-legacy-worker-terminal-resume-fence.test.tssrc/main/runtime/runtime-notifier-contract.tssrc/main/startup/main-process-ipc-bootstrap.tssrc/main/window/runtime-window-lifecycle.tssrc/preload/api/agent-status-api.tssrc/preload/api/agent-status-bridge.tssrc/preload/api/app-api.tssrc/preload/api/app-bridge.tssrc/renderer/src/components/terminal-pane/pty-connection-agent-session-resume.test.tssrc/renderer/src/components/terminal-pane/pty-connection-test-store-fixtures.tssrc/renderer/src/components/terminal-pane/pty-connection/sleeping-record-access-recordless-fence.test.tssrc/renderer/src/components/terminal-pane/pty-connection/sleeping-record-access.tssrc/renderer/src/components/terminal-pane/sleeping-record-park-exemption.test.tssrc/renderer/src/components/terminal-pane/sleeping-record-park-exemption.tssrc/renderer/src/components/terminal-pane/terminal-cold-park-subscription-narrowing.react185.test.tsxsrc/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.tssrc/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.tssrc/renderer/src/hooks/ipc-events/agent-status-listeners.tssrc/renderer/src/hooks/useIpcEvents-lifecycle.test.tssrc/renderer/src/lib/agent-hibernation-coordinator.tssrc/renderer/src/lib/agent-hibernation-pane-eligibility-resume-fence.test.tssrc/renderer/src/lib/agent-hibernation-pane-eligibility.tssrc/renderer/src/lib/agent-hibernation-planner-snapshot.tssrc/renderer/src/lib/agent-hibernation-planner.test.tssrc/renderer/src/lib/agent-hibernation-planner.tssrc/renderer/src/lib/legacy-worker-old-source-resume.test.tssrc/renderer/src/lib/legacy-worker-resume-fence-refresh.test.tssrc/renderer/src/lib/legacy-worker-resume-fence-refresh.tssrc/renderer/src/lib/live-resume-anchor-record.tssrc/renderer/src/lib/resume-sleeping-agent-session-legacy-worker.test.tssrc/renderer/src/lib/resume-sleeping-agent-session.tssrc/renderer/src/lib/runtime-session-application.test.tssrc/renderer/src/lib/runtime-session-application.tssrc/renderer/src/lib/runtime-session-refresh-hydration-order.test.tssrc/renderer/src/lib/workspace-session-host-hydration.tssrc/renderer/src/lib/workspace-session-snapshot.tssrc/renderer/src/lib/workspace-session.tssrc/renderer/src/store/slices/agent-sleep-resume-fence-races.test.tssrc/renderer/src/store/slices/agent-status-manual-sleep-capture.test.tssrc/renderer/src/store/slices/agent-status-open-tab-resume-fence.test.tssrc/renderer/src/store/slices/agent-status-provider-session-actions.tssrc/renderer/src/store/slices/agent-status-provider-session.test.tssrc/renderer/src/store/slices/agent-status-recovery-actions.tssrc/renderer/src/store/slices/agent-status-sleeping-records.tssrc/renderer/src/store/slices/agent-status-slice-contract.tssrc/renderer/src/store/slices/agent-status.tssrc/renderer/src/store/slices/legacy-worker-resume-fences.tssrc/renderer/src/store/slices/workspace-scoped-resume-fence-hydration.test.tssrc/renderer/src/store/terminals/terminal-pane-hibernation.tssrc/renderer/src/store/terminals/workspace-terminal-hydration.tssrc/renderer/src/web/preload-api/web-agent-status-api.tssrc/renderer/src/web/preload-api/web-app-api.tssrc/shared/workspace-session-host-field-ownership.tssrc/shared/workspace-session-resume-fences.tssrc/shared/workspace-session-schema-field-coverage.test.tssrc/shared/workspace-session-schema.tssrc/shared/workspace-session-state-types.tstests/e2e/helpers/completed-worker-retirement-fixture.tstests/e2e/settled-worker-resume-fence-restart.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| "command": "ORCA_BACKGROUND_LAUNCH=1 SKIP_BUILD=1 npx playwright test tests/e2e/settled-worker-resume-fence-restart.spec.ts tests/e2e/completed-worker-retirement-resume.spec.ts --config tests/playwright.config.ts --project=electron-headless --workers=1", | ||
| "result": "passed", | ||
| "durationSeconds": 24.8, | ||
| "summary": "Three hidden Electron tests pass on a fresh build, with isolated homes/profiles." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 '\bSKIP_BUILD\b|fresh build' . \
--glob '!node_modules/**' \
--glob '!dist/**'Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- gate excerpt ---'
sed -n '1,95p' config/reliability-gates.jsonc
printf '%s\n' '--- SKIP_BUILD definitions and consumers outside the gate ledger ---'
rg -n -C 5 'SKIP_BUILD' . \
--glob '!config/reliability-gates.jsonc' \
--glob '!node_modules/**' \
--glob '!dist/**' \
--glob '!build/**' \
--glob '!coverage/**' \
| head -n 240
printf '%s\n' '--- relevant package scripts/config references ---'
rg -n -C 4 'playwright|electron-vite build|test:e2e|SKIP_BUILD' package.json tests/playwright.config.ts scripts . \
--glob '!config/reliability-gates.jsonc' \
--glob '!node_modules/**' \
--glob '!dist/**' \
--glob '!build/**' \
--glob '!coverage/**' \
| head -n 320Repository: stablyai/orca
Length of output: 47971
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions
Length of output: 10408
Align the build evidence with the command.
When out/main/index.js exists, SKIP_BUILD=1 makes globalSetup reuse it. The recorded command therefore does not prove a fresh build. Record an explicit pnpm exec electron-vite build --mode e2e step in the evidence run, or revise the summary to describe reuse of an existing build.
| return projectLegacyWorkerSession( | ||
| this[sessionHostPartitionOperationsContext].runtime.state.workspaceSession ?? | ||
| getDefaultWorkspaceSession() | ||
| getDefaultWorkspaceSession() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 15 'function removeWorkspaceSessionOwner|removeWorkspaceSessionOwner\(' \
src/main/persistence
rg -n -C 8 'scheduleSave|workspaceSession =' \
src/main/persistence/loading-store/session-host-partitions.tsRepository: stablyai/orca
Length of output: 23109
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions
Length of output: 18363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- session-host-partitions.ts ---'
cat -n src/main/persistence/loading-store/session-host-partitions.ts | sed -n '1,175p'
printf '%s\n' '--- projection and field references ---'
rg -n -C 12 'projectLegacyWorkerSession|automaticResumeBlockedBy|cloneWorkspaceSessionState|deleteOwnerKeyedSessionFields' src/main/persistence
printf '%s\n' '--- save and persistence boundary references ---'
rg -n -C 10 'workspaceSessionsByHostId|workspaceSession|scheduleSave|serialize|persist|toJSON' src/main/persistence/loading-store src/main/persistence | head -n 500Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/main/persistence/loading-store/session-host-partitions.ts | sed -n '1,175p'
printf '\n--- symbols ---\n'
rg -n -C 15 'projectLegacyWorkerSession|automaticResumeBlockedBy|cloneWorkspaceSessionState|deleteOwnerKeyedSessionFields' src/main/persistence
printf '\n--- save boundary ---\n'
rg -n -C 10 'scheduleSave|writeStore|workspaceSessionsByHostId|workspaceSession' src/main/persistence/loading-store src/main/persistence | head -n 500Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- session owner removal implementation ---'
cat -n src/main/persistence/restoring-sessions/session-owner-removal.ts | sed -n '1,180p'
printf '%s\n' '--- legacy projection definitions and callers ---'
rg -n -C 20 'projectLegacyWorkerSession|automaticResumeBlockedBy' src
printf '%s\n' '--- persistence writer definitions ---'
rg -n -C 20 'scheduleSave|workspaceSessionsByHostId|workspaceSession' src/main/persistence/loading-store src/main/persistence | tail -n 500Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- session partition writes ---'
cat -n src/main/persistence/loading-store/session-host-partitions.ts | sed -n '174,360p'
printf '%s\n' '--- projection-related write helpers and callers ---'
rg -n -C 12 'preserveRuntimeAuthoredWorkspaceSessionFields|withoutRedundantGlobalFields|sanitizeWorkspaceSessionTerminalRetirements|setWorkspaceSession\(' src/main/persistence/loading-store src/main/persistence
printf '%s\n' '--- scheduleSave implementation ---'
cat -n src/main/persistence/loading-store/write-scheduling.ts | sed -n '1,180p'Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- fence write tests around owner removal ---'
cat -n src/main/persistence/loading-store/store-runtime-authored-session-fences.test.ts | sed -n '180,280p'
printf '%s\n' '--- save scheduling and state flush ---'
cat -n src/main/persistence/loading-store/write-scheduling.ts | sed -n '1,120p'
rg -n -C 12 'flushOrThrow|scheduleSave|state\.workspaceSession|workspaceSessionsByHostId' src/main/persistence/loading-store/store.ts src/main/persistence/loading-store/store-runtime-state.ts src/main/persistence/loading-store 2>/dev/null | head -n 300Repository: stablyai/orca
Length of output: 37189
Keep the compatibility projection out of the persisted session.
getWorkspaceSession adds automaticResumeBlockedBy from the canonical fence map. removeWorkspaceSessionOwnerInPartition passes this projected session to removeWorkspaceSessionOwner, then stores the clone directly in runtime state and schedules a write. This path can persist the compatibility field. Mutate the canonical partition session, or strip automaticResumeBlockedBy before storing the result.
| fail = false | ||
| plan = { | ||
| ...plan, | ||
| blockedPanes: [{ paneKey: 'a', worktreeId: 'local', contractVersion: 1, settled: true }] | ||
| } | ||
| p.prepare() | ||
| expect(notify).toHaveBeenCalled() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the retry assertion discriminating.
On the first pass, local writes { a: true } and calls notify. The ssh:x write then fails. On the second pass, local already matches and ssh:x still appears in the host list with its unchanged default session, so no write or notification occurs. The final toHaveBeenCalled() only observes the first-pass call.
Remove ssh:x before the retry and assert the exact notification count.
💚 Proposed fix
fail = false
+ delete sessions['ssh:x']
+ store.getWorkspaceSessionHostIds = () => ['local']
plan = {
...plan,
blockedPanes: [{ paneKey: 'a', worktreeId: 'local', contractVersion: 1, settled: true }]
}
p.prepare()
- expect(notify).toHaveBeenCalled()
+ expect(notify).toHaveBeenCalledTimes(1)
+ expect(sessions.local.legacyWorkerResumeFencesByPaneKey).toEqual({ a: true })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fail = false | |
| plan = { | |
| ...plan, | |
| blockedPanes: [{ paneKey: 'a', worktreeId: 'local', contractVersion: 1, settled: true }] | |
| } | |
| p.prepare() | |
| expect(notify).toHaveBeenCalled() | |
| fail = false | |
| delete sessions['ssh:x'] | |
| store.getWorkspaceSessionHostIds = () => ['local'] | |
| plan = { | |
| ...plan, | |
| blockedPanes: [{ paneKey: 'a', worktreeId: 'local', contractVersion: 1, settled: true }] | |
| } | |
| p.prepare() | |
| expect(notify).toHaveBeenCalledTimes(1) | |
| expect(sessions.local.legacyWorkerResumeFencesByPaneKey).toEqual({ a: true }) |
There was a problem hiding this comment.
ℹ️ No new issues in the round-3 delta. The one prior finding — the
prunedByBespokeRulerepo-removal disposition forlegacyWorkerResumeFencesByPaneKeystill has no bespoke rule inremoveRepoFromWorkspaceSession— is unchanged by these commits and remains open (non-blocking; the residue is inert and main's level-triggered rewrite self-heals).
Reviewed changes
Round 3 (57d0ef6 + 6720908) resolves the six round-2 follow-ups and the runtime-gate correction.
- Moved the old-client record flag from the renderer into main's session getter —
projectLegacyWorkerSessionwrapsSessionHostPartitionOperations.getWorkspaceSessionfor local and nonlocal partitions, soautomaticResumeBlockedByis a read-time projection.prepare()'s write-back of the projected session is inert on reads: every flag reader (projectLegacyWorkerSession,readWorkspaceSessionResumeFences) short-circuits on the runtime-authored field, and the projection re-derives the flag from it on every read. - Unified the fence reader behind
readWorkspaceSessionResumeFences— one shared helper preferslegacyWorkerResumeFencesByPaneKeyand falls back to legacy record flags; used by bothprepare()andgetLegacyWorkerResumeFences(), so hydration distinguishes an absent old-source authority from an authoritative empty set. - Serialized the renderer read/apply lane —
runtime-session-application.tsroutes startup hydration, the ping refresh, and scoped hydration through one ordered lane, so a newer ping cannot be rolled back by stale startup hydration and a pending read cannot overwrite newer hydration. - Closed the publication gap —
prepare()pings after each successful host commit (a failing sibling no longer suppresses the committed host's invalidation), andsameFenceSetskips untouched partitions with neither canonical nor legacy protection while still retiring legacy-only records. - Preserved fences across mixed-host normalization and scoped retirement — old-host legacy flags normalize only on the viewer mirror;
applyReadRuntimeSessionretires only in-scope tabs, preserving sibling fences and absent-SSH authority. - Routed recordless reattach and park exemption through the fence set —
isLegacyWorkerAutomaticResumeBlockedfalls back tosession.cacheKey, andselectSleepingRecordParkExemptTabIdsnow consults the fence set for panes whose worker settled with no sleeping record.
The new specs are real characterizations (real-Store write-path coverage, publication-order races, sibling-preservation scoping, mutation-oracle naming) rather than tautologies.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No functional issues in the round-4 delta — one inline nit in the new spec.
Reviewed changes
Round 4 (7f36880) publishes sleeping records and their resume fences atomically and makes supersede read-lifetime scoped, fixing the remaining P1 at hydration.
- Single-transition hydration —
hydrateRuntimeSessionFieldsnow returns the fence patch instead of callingset()itself, sohydrateWorkspaceSessionmerges sleeping records andlegacyWorkerResumeFencesByPaneKeyinto one subscriber notification. - Read-lifetime supersede —
hydratedDuringReadtracks scopes (or a whole-sessionnull) hydrated while a read is in flight;applyReadRuntimeSessiondrops the read on a full-hydration supersede and preserves in-scope panes on a scoped supersede, then clears the map so a later refresh can retire. - Removed the renderer writer —
setLegacyWorkerResumeFences, the store-sidesameFenceSetmodule, and thesetSleepingAgentAutomaticResumeBlockedcarry are gone; the fence is read-only on the renderer, and main'sruntime-legacy-worker-terminal-recovery-persistence.tskeeps its own comparison. - Stub rename + new specs — web/pair stub listener renamed to
onLegacyWorkerTerminalResumeFencesChanged;runtime-session-pending-hydration.test.ts(canonical + legacy branches) and a single-notification assertion characterize the atomic publish.
The supersede direction (hydration wins over an in-flight read) errs toward keeping a fence rather than lifting it, consistent with the "loss of contact is never evidence a fence can lift" invariant.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| } | ||
| resolve({}) | ||
| await pending | ||
| console.log(kind, observed) |
There was a problem hiding this comment.
Leftover debug logging: console.log(kind, observed). The observed snapshot's tabs, claims, and recordStillPresent fields are never asserted (only count is), so both the log and the snapshot are dead debug code that will spam CI test output.
Jinwoo-H
changed the title
There was a problem hiding this comment.
ℹ️ No critical issues in the round-5 delta — minor debug-logging nits inline.
Reviewed changes
Round 5 (2197d23) refines the read-lifetime supersede tracking so a pending runtime read is only superseded by the authority the overlapping hydration actually supplied.
- Two-part hydration authority —
hydratedDuringReadnow records{ tabIds, paneKeys }instead of a scope-only set. A snapshot carrying the canonical field registers tab-scope (or whole-sessionnull) authority; a legacy-only snapshot registers only the specific panes its records protect, never scope-wide retirement authority. - Absent-authority snapshot no longer suppresses in-flight authority — a direct-SSH snapshot with no fence field and no legacy flags records nothing during a read, so the pending reply's acquisition/retirement is left intact (the reproduced round-4 loss).
- Legacy pane precision —
authorityIncludesPaneprefers explicitpaneKeysover scope, so other panes in the same tab can still be acquired or retired by the in-flight reply while the protected pane stays fenced. - New oracle and interleavings —
runtime-session-ssh-projection-authority.test.tsreproduces the SSH race with real projection/merge/hydration/resume code, andruntime-session-application.test.tsadds 8 interleaving cases plus 5 legacy-only authority cases; the reliability manifest was updated to match.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| scoped() | ||
| d.resolve({ [pane]: true, [sibling]: true }) | ||
| await first | ||
| console.log( |
There was a problem hiding this comment.
Stray debug logging. console.log('absent-source acquisition', …) here and console.log('retired main, post-reply sweep', …) at line 314 dump store state to CI output and are never asserted — only count and the fence set are. Delete both (same pattern flagged in the round-4 spec, still open).
| resolve(kind === 'acquire' ? { [paneKey]: true } : {}) | ||
| await pending | ||
| const count = resumeSleepingAgentSessionsForWorktree(worktreeId) | ||
| console.log(kind, { |
There was a problem hiding this comment.
Stray debug logging. This console.log(kind, …) (and the tabs/claims fields it prints) is never asserted — the spec only checks get and count. Delete it.
|
Superseded: the settled-worker resume fence is being deleted as a product decision (finished workers resume exactly like any other agent terminal). Replacement branch: settled-worker-policy. |
1 participant
Summary
Settled orchestration workers must stay fenced from automatic resume across status
writes, renderer reloads, and app restarts. Their fence now lives in the
runtime-authored workspace-session field
legacyWorkerResumeFencesByPaneKey.Renderer-authored sleeping records cannot erase that policy on their next write.
This addresses the settled-worker recovery and resume findings from #16904.
Current design
invalidation after each successful write. Unchanged untouched partitions need
no empty write; explicit retirement still clears existing canonical or legacy
protection.
session payloads omit the fence field. Main's session read boundary derives
automaticResumeBlockedByfor older clients, including headless publication.store transition. Runtime reads serialize; consumed startup snapshots cannot
replay older authority after a later refresh.
full or scoped policy, legacy-only records protect their specific pane keys, and
snapshots with neither contribute no supersession. Older replies can still
update untouched scopes and panes. Read-lifetime metadata clears at settlement.
mirror. An absent field preserves known protection and normalizes old-source
flags; a present empty map can retire them.
The canonical field is optional and adds no wire opcode. Direct-SSH and narrower
paired/web/mobile projections are not extended to carry full fence authority here.
No renderer stamping, carry predicate, seeded startup fence map, generation
counter, or second persisted fence authority remains.
Validation
Current local follow-up:
2197d23bba1350ec71627791c29f697a570cd45b(not pushed; lead verification pending).checks with
--no-stash: passed.2/2, and paired 3/3, with isolated profiles/homes and
ORCA_BACKGROUND_LAUNCH=1.The SSH race uses real projection/import, payload, merge, hydration, refresh, and
resume code in a deterministic store test. It is not a live SSH duplicate-process
E2E. No physical Windows/Linux, independently versioned paired-runtime, or
combined #19358 validation is claimed. Main execution admission remains a
complementary responsibility to this durable policy and UI planning change.
Round-4 review follow-ups
Fixed the reproduced acquisition/retirement loss when a direct-SSH snapshot with
no fence authority arrived during a runtime read. Such a snapshot now leaves the
pending reply's authority intact. Legacy-only input supersedes only protected
pane keys within the hydrated scope, allowing other panes in the same tab to be
acquired or retired. The atomic hydration transition remains unchanged.
The SSH oracle reproduced 2 failures and now passes with unchanged assertions
as
runtime-session-ssh-projection-authority.test.ts. All 8 reviewerinterleavings are permanent in
runtime-session-application.test.ts, includingfull-then-scoped and two-scoped accumulation. Six further cases cover legacy pane
precision, scope filtering, accumulation, and subsequent canonical authority.
All 6 new mutations fail assertions: forgetting full scope, forgetting earlier
scopes, superseding absent authority, dropping legacy pane protection, forgetting
earlier legacy panes, and including out-of-scope legacy records. Sources were
restored after mutation testing. The reliability manifest registers the new oracle
and accumulation assertions.
The title and earlier superseded descriptions have been rewritten for the current
design. Prior review history and the Round 5 Boundary explanation are retained in
fix-reports/19344-round2.md; current logs and hidden-renderer proof are infix-reports/19344-round5-evidence/.Not to be merged without explicit sign-off.