feat(executors): keep worktree and local agents running across a backend restart - #3467
Open
nova28 wants to merge 83 commits into
Open
feat(executors): keep worktree and local agents running across a backend restart#3467nova28 wants to merge 83 commits into
nova28 wants to merge 83 commits into
Conversation
Requirements and system design for making worktree/local agent work survive a Kandev backend restart, frozen after 8 spec-review rounds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add the five agentctl.* catalog keys the agent-survival-across-restart spec requires (recoveryDeadline, recoveryReadTimeout, recoveryReadRetries, unownedPeriod, detachedEventLimit), with reject-not-clamp startup validation for out-of-range values from either YAML or environment, and thread unownedPeriod/detachedEventLimit through the managed-agentctl startup contract so a standalone child receives them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A platform-gated flag needs a third state beyond enabled/disabled: not supported on this host. Add a nil-able Available probe on RuntimeFlagDefinition (nil = always available, additive for every existing flag) and an optional Availability object on RuntimeFlagState that appears only when a flag is unavailable. An unavailable flag behaves as disabled regardless of its stored value, and SetOverride refuses to enable one rather than silently storing it.
Add features.agentSurvival (KANDEV_FEATURES_AGENT_SURVIVAL), off in every shipped profile per AC-EXECUTORS-SURVIVAL-005.2, gating worktree and local-executor agent sessions surviving a backend restart. It carries the new host-availability probe: unavailable on Windows per the platform-scope decision in system-design 02, since survival trades the platform's kill-on-job-close safeguard for an untested adoption handshake. Add a targeted Windows CI step so the unavailable branch is exercised on a real Windows runner instead of only compiling there.
… expects
agentctl.ControlClient.ListInstances has always decoded a
{"instances": [...]} envelope, but handleListInstances wrote the bare
[]*InstanceInfo array straight from instance.Manager.ListInstances, so
a real call always failed to decode. Add SessionID/TaskID to Instance
and InstanceInfo (server and client) so a recovered instance can be
correlated back to its owning session. Replace the client-side
hand-written response fixture with a real-handler, real-client test
that would have caught the mismatch.
Startup recovery needs to take its guard/correlation inventory (step 3) before any control-server contact, but the only read path returned every executors_running row regardless of runtime or terminal status. Add ListExecutorsRunningLiveStandalone, filtered to the standalone control server (worktree/local executors) and non-terminal rows, plus the lifecycle manager's optional-interface delegation (ListLiveStandaloneExecutorsRunning) matching the existing executorRunningReader pattern.
Adds the durable single-row control-server record (control endpoint, control-server identity, credential secret reference, observed capability set, diagnostic-output location) described in the agent-survival design: new control_server_records table, models.ControlServerRecord, and Get/UpsertControlServerRecord on the SQLite repository. Deliberately separate from executors_running, which has no row when no session is live and would leave a detached server unlocatable.
Adds storeControlServerCredential/revealControlServerCredential in the lifecycle package: the bearer token is written to internal/secrets (global scope) and only the resulting secret ID is meant to be persisted onto ControlServerRecord.CredentialSecretID, never the token itself. Rotation reuses the same secret ID via an in-place update, falling back to creating a new secret if the referenced one is missing.
Adds GET /identity, exempt from bearer-token auth per design 01's "Capability compatibility" (identity retrieval decides compatibility, so it cannot itself be gated on the answer). Reports the resolved Kandev home directory, a per-launch opaque server identity, and the advertised capability set. Adds the matching agentctl.ControlClient.GetIdentity so the wire contract is proven end-to-end rather than by a hand-fabricated fixture, following the same convention as the ListInstances envelope fix.
Adds POST /api/v1/ownership/claim plus an ownershipState tracking the last successful renewal on the control server's own clock (design 01, "Unowned shutdown"). The claim carries no instance identity -- a server with zero instances is still owned. The bootstrap handshake now also renews ownership, giving a fresh server's unowned-period timer a definite start. BeginShutdown/IsShuttingDown implement the one-way door: once a shutdown decision latches, claim (and future rotation/handshake renewals) refuse rather than reviving ownership underneath a teardown in progress. The actual unowned-period reaper goroutine that drives BeginShutdown from elapsed time lands in a later layer; this adds the shared state and the claim endpoint it renews.
Adopting a control server now rotates its credential in two phases: rotate replaces the fully-authenticating credential and demotes the presented one to adoption-only until confirm names the rotation as durably stored. Rotation is idempotent under retry (a presented superseded credential replays the same unconfirmed rotation) and the acceptable set is capped at the current credential plus the one it directly superseded. Rotate also renews control-server ownership, and both operations are refused once the unowned-shutdown latch has fired.
Expose an authenticated ownership-shutdown operation that a backend holding any credential in the acceptable set (including a superseded, unconfirmed one) can invoke with no prior adoption or rotation, so a backend stranded with only a superseded credential can still tear a survived control server down. The run loop now selects on this trigger alongside its existing OS-signal and parent-death sources and runs the same stop-every-instance-and-exit sequence.
Implements AC-EXECUTORS-CONTROL-OWNERSHIP-003.1/.2/.4/.6/.7/.9: a background goroutine that stops every instance and exits once no ownership claim has been current for the resolved unowned period, reusing the same one-way BeginShutdown/requestShutdown signal the ownership-shutdown operation (5.4) already drives the run loop through, rather than a second teardown path. resolveUnownedPeriod implements the real clamp/floor logic deferred from 0.1: clamp to half the idle timeout when idle reaping is on and the configured period isn't already shorter (003.4), skip the clamp entirely when idle reaping is disabled (003.6), and apply a 1-minute floor that takes precedence over the clamp (003.7). StartUnownedReaper/StopUnownedReaper follow the goroutine-ownership convention (own Start/Stop, WaitGroup-registered, idempotent Stop) but are deliberately not wired to auto-start yet: nothing periodically renews ownership until Layer 6's backend-side claim loop exists, and the capability-enabled gating boolean Layer 5.9 will add doesn't exist yet either. Auto-starting now would self-terminate every agentctl launch once the period elapses.
Implements AC-EXECUTORS-SURVIVAL-001.3's path/bounds half: derives agentctl's own diagnostic log path from the resolved home directory with a fixed filename (agentctl-diagnostic.log, distinct from the backend's own backend-logs.log), so the sink adds no configuration key and no environment variable of its own. Echoed on GET /identity alongside home_dir/server_identity/capabilities so an adopting or freshly-spawning backend can record it into the control-server record without independently re-deriving the same path formula. diagnosticLoggingConfig builds the file-backed logger.LoggingConfig with the F71 bound values Build already picked (MaxSizeMB=16, MaxBackups=10, MaxAgeDays=14). Not yet wired into runMain's actual logger construction: switching away from stdout only matters once the agent-survival capability is engaged for a launch, and that gating boolean is Layer 5.9's job (kill-path kdlbs#6, inherited stdout, is only a problem once the parent may have exited while agentctl keeps running).
…nces Adds an agentctl-local retention primitive (AC-EXECUTORS-SURVIVAL-004.1/.2/.6): each instance keeps its last terminal turn outcome under a control-server-local turn identifier, exposed via GET/POST /api/v1/instances/:id/turn-outcome[/ack]. Retrieval is repeatable and non-discarding; ack by the matching identifier discards it, and an unrelated or already-gone identifier is a safe no-op. Deliberately scoped to the retention primitive and wire contract only, not wiring RetainTurnOutcome into the terminal-event call sites -- Layer 5.8 already needs to touch those same 8 sites for the blocking-send conversion.
…ocking sends Converts the 8 COVERED producers identified in design 03 (agent ERROR, process-exit error, permission-cancelled, permission-request timer, and the 4 ACP-adapter session_models/context_window convergence emitters) from drop-on-full sends to sends that park until the channel has room or the instance stops, so events most likely to matter across a detached gap are no longer silently discarded (AC-EXECUTORS-SURVIVAL-001.5/.6). process/manager.go gains sendUpdateBlocking (parks against a lock-free stopChSnapshot so it can be called from the same goroutine Stop() waits on without deadlocking) and attachment tracking (MarkAttached/MarkDetached/ IsAttached, wired from the /agent/stream handler) so the permission-request site can keep its 5-second attached auto-cancel while parking instead while detached, where auto-cancelling would silently deny every permission the agent asks for. The updates channel is now sized from the agentctl.detachedEventLimit catalog tunable via InstanceConfig. The 4 ACP-adapter sites (adapter_session.go, adapter_updates.go) previously called sendUpdateLocked while holding a.mu, which would deadlock against Close() if converted in place. They now build their event under the lock, release it, and deliver via the existing sendUpdate helper -- already built for exactly this purpose and already used by sibling sites in this package -- rather than routing through the notification queue, which would reorder them relative to their true delivery point on updatesCh.
…ents Completes 5.7's turn-outcome primitive by actually populating it (AC-EXECUTORS-SURVIVAL-004.1): process.Manager gains a TurnOutcomeRecorder optional interface (satisfied structurally by instance.Manager.RetainTurnOutcome, since adapter.AgentEvent is a type alias for streams.AgentEvent) and recordTerminalOutcome, a single filter for the two terminal event types (EventTypeComplete, EventTypeError) called from every point an event is actually delivered onto updatesCh -- forwardUpdates (covers every ACP-adapter-originated terminal event: session/prompt completion, the async/steering completion path, and protocol-level errors, regardless of which of several independent adapter call sites produced it) and sendUpdateBlocking (covers this manager's own agent-ERROR and process-exit-error sends). Permission lifecycle events, MCP attachment evidence, and every other event type pass through the same filter unretained, since AC-004 is about the last turn's outcome, not every lifecycle event. instance.Manager.CreateInstance wires the recorder immediately after constructing the process manager, before anything could reach Start(), so the wiring is race-free without new synchronization on the process.Manager side beyond the mutex it already has.
…nfig Adds AgentSurvivalEnabled to AgentctlStartupConfig and agentctl's config.Config, sourced from the features.agentSurvival runtime flag via ManagedAgentctlStartupConfig(). Unlike UnownedPeriod/DetachedEventLimit, false is copied unconditionally rather than treated as "unresolved", since a managed launch always states it. This is the plumbing prerequisite for Layer 5.9's kill-path gating: it gives agentctl a per-launch boolean to gate StartUnownedReaper, the detached diagnostic log sink switch, and (on the launcher side) skipping the parent-liveness pipe, Pdeathsig, and the registered stop-on-shutdown cleanup.
…al flag Wires the two agentctl-side capabilities that were built dormant in Layers 5.5/5.6 to the AgentSurvivalEnabled boolean threaded through in the previous commit: - startUnownedReaperIfEnabled starts ControlServer.StartUnownedReaper only when the capability is engaged for this launch, matching its own caller contract (an unconditional start would self-terminate a healthy attached instance with nothing yet renewing ownership). - resolveRunLoggingConfig switches agentctl's own logger from stdout to the bounded diagnosticLoggingConfig sink only when the capability is engaged and a diagnostic log path resolved, closing kill-path kdlbs#6 ("inherited stdout") from design 01's kill-paths list. Every other case (disabled, or an unresolved diagnostic path) keeps today's stdout/no-reaper behavior unchanged.
…nabled Grounds Layer 5.9's kill-path removal in design 01's kill-paths list and AC-EXECUTORS-SURVIVAL-001.1/.2, which require the control server and its instances to keep running through both a graceful backend shutdown and an ungraceful exit (including SIGKILL) once the capability is engaged for a launch: - buildSysProcAttr no longer sets Pdeathsig on Linux (kill-path kdlbs#2) when AgentSurvivalEnabled -- it fires on any parent death, so it can't depend on a graceful shutdown step. Setpgid is unaffected on every platform. - buildAndStartProcess skips arming the parent-liveness pipe (kill-path kdlbs#1) for the same reason, and now also strips any KANDEV_PARENT_PIPE_FD the launcher process itself inherited from its own ambient environment -- without this, a stale inherited value would leak into the child even though no pipe backs FD 3, and agentctl would misread it as live. - Launcher.Stop becomes a no-op when the capability is engaged (kill-path kdlbs#5, "registered cleanup"): Stop is the only thing the backend's registered shutdown cleanup calls, so gating it there closes the path without a second branch at the call site. Windows (job-object kill-path kdlbs#3) and every other case are unchanged.
…tion Adds AttemptAdoptControlServer (Layer 6.1a of agent-survival-across-restart): reads the installation-scoped control-server record, evaluates the adoption gates in AC-EXECUTORS-CONTROL-OWNERSHIP-001.3's required order (home match, then authentication via credential rotation, then capability subset), and on success durably rotates and persists the ownership credential. Refusal reasons follow AC-001.6/001.9/004.3's precedence and stop-vs-leave-running rules: only an authenticated, identified, incompatible server is ever stopped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…adoption Adds RecordFreshControlServer: when adoption is refused or no record exists, the single installation-scoped control-server record must still be rewritten to name the newly spawned server (it cannot keep pointing at a survivor that is about to reap itself). Reuses the prior record's credential secret ID when one exists rather than orphaning it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
provideAgentctlLauncher now tries lifecycle.AttemptAdoptControlServer first when the agent-survival capability is enabled, adopting a surviving control server from a prior launch instead of always spawning fresh. On any refusal, or with the capability disabled, it falls back to today's spawn path unchanged; a freshly spawned server is durably recorded as the new control-server record when survival is enabled, so a later restart has something to adopt. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nal/common/ownershipperiod Both agentctl's reaper and the backend's upcoming ownership-renewal loop must agree on the exact same resolved unowned period (AC-EXECUTORS-CONTROL-OWNERSHIP-003.2/.4/.6/.7); duplicating or approximating the computation risks a renewal interval that isn't strictly inside a third of the actual resolved period. Moved verbatim per the cross-tier shared code convention.
…ewal loop AC-EXECUTORS-CONTROL-OWNERSHIP-003.2 requires a renewal cadence strictly shorter than one third of the resolved unowned period. Quartering the period leaves margin against jitter/latency instead of relying on integer-division rounding to satisfy "strictly shorter".
AC-EXECUTORS-CONTROL-OWNERSHIP-003.2/.8: a backend holding ownership of a surviving control server must keep renewing it via the dedicated ownership-claim operation, or the server begins an unowned shutdown even while it is actively supervising agents. Follows the healthpoll goroutine-ownership convention (explicit Start/Stop, WaitGroup-registered, idempotent both ends). Does not claim immediately on Start: the rotation or bootstrap handshake that preceded it already counted as the first renewal, so an immediate claim would be a redundant fourth renewing operation not listed by AC-003.8.
… spawn Wires lifecycle.OwnershipRenewer into provideAgentctlLauncher on both paths: after AttemptAdoptControlServer succeeds (rotation already counted as the first renewal) and after a fresh spawn (the bootstrap handshake counted as the first renewal). The interval is computed from ownershipperiod.Resolve(cfg.Agentctl.UnownedPeriod, cfg.Agentctl.IdleTimeout) so it can never disagree with the period agentctl itself enforces. Completes Layer 6.1's own stated scope: the reaper introduced earlier is now meaningful, since something periodically renews ownership against it.
AC-EXECUTORS-SURVIVAL-002.8/002.16/003.7: before contacting any control server, the backend must take an atomic acquire-or-observe guard for every live standalone session that might be re-tracked, so a concurrent launch is refused rather than racing recovery. RecoveryGuard is the in-memory, one-process-lifetime primitive; SessionsToGuard applies the passthrough exclusion of AC-EXECUTORS-SURVIVAL-005.3 (guarding on a failed read rather than excluding). A TDD red step caught a real bug: CheckLaunchAllowed read the zero value of a missing map entry as the guardHeld state, refusing launches for sessions that were never guarded at all. This is the guard mechanism only; the startup call site (reading 2.1's live-standalone list, taking guards before control-server contact) and the Launch-path integration land with Layer 6.3's recovery orchestration, which is what actually knows when guards should be taken and released.
Design-03's "Recovery record read port" changes ExecutorBackend.RecoverInstances to accept the live standalone recovery-inventory records read at startup step 3 (AC-EXECUTORS-SURVIVAL-002.8), rather than acquiring a database dependency itself. All six executor implementations change signature; five (Docker, K8s, SSH, Sprites, remote Docker) keep returning nil, nil and simply ignore the new parameter. ExecutorRegistry.RecoverAll and Manager.Start (which now reads the records via the existing ListLiveStandaloneExecutorsRunning, Layer 2.1) thread them through unchanged to every registered runtime. StandaloneExecutor.RecoverInstances itself is still the pre-existing stub; correlating those records to enumerated instances is the next commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…reak CorrelateRecoveryInstances joins enumerated adopted-server instances to recovery-inventory records by session identity (AC-EXECUTORS-SURVIVAL-002.1): a session with exactly one live instance re-tracks it regardless of ID match (002.2); an instance whose session has no record is stopped (002.6); a session with more than one live instance re-tracks only the one whose instance ID equals the record's agent execution identifier, stopping every other candidate on no-match or an ambiguous match (002.10); a record with no live instance is left untouched for the existing stale-execution repair path (002.7). Records are keyed by session identity so outcomes for different sessions never depend on each other (002.11). Pure function, not yet wired into StandaloneExecutor.RecoverInstances. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… a transient error Review round 4 finding: storeControlServerCredential reused an existing secret ID via a blind Update whenever adoption failed with CredentialUnavailable, without first confirming the existing secret was actually gone rather than just transiently unreadable. A Reveal failure caused by a passing infrastructure blip -- not a genuinely missing secret -- would overwrite and destroy the still-needed credential the moment a fresh server was then recorded, violating AC-EXECUTORS-CONTROL-OWNERSHIP-001.9's "shall not delete the stored credential" on a transient read error. tryReuseCredentialSecret now requires a successful reveal before reusing the ID via Update; when the reveal fails for any reason other than a confirmed ErrNotFound, it falls back to allocating a fresh secret instead of overwriting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…o end Round-4 Finding 2 (instanceAuth's dynamic per-request auth against the control server's rotating credential, and /agent/stream self-termination on rotation via credentialState.Invalidated()) shipped with zero test coverage: no test anywhere referenced credentialSource, SetCredentialSource, or instanceAuth. Add a table test proving instanceAuth accepts the current credential and rejects a superseded one on an ordinary request, and an integration test that dials a live /agent/stream connection, rotates the credential under it, and proves the server actively tears the connection down rather than merely refusing new ones. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The credential-rotation fencing mechanism (AC-EXECUTORS-CONTROL-OWNERSHIP-002.2) has two identical select-on-Invalidated call sites, /agent/stream and /workspace/stream, but only the former had a live-connection test. Add TestWorkspaceStreamTerminatesWhenCredentialRotates, mirroring the existing agent-stream test. While writing it, found the existing assertion pattern (bare `err == nil` check after a 2s read deadline) can't distinguish a deliberate server close from the deadline simply firing on an untouched connection -- both return a non-nil error. Extracted assertConnectionClosedByServer, which also rejects a timeout error, and applied it to both tests.
…al flag classifyStandaloneLiveness consulted an adopted-server enumeration scope unconditionally, so a disabled agentSurvivalEnabled capability still trusted absence-from-enumeration to mean "inherited, unknown" for a row whose process is provably dead. Fall back to the process-identifier probe whenever the capability is off, matching pre-standalone-liveness behavior.
…imeout AttemptAdoptControlServer's GetIdentity/RotateCredential/ConfirmCredentialRotation calls used the raw, undeadlined context instead of the recoveryReadTimeout-wrapped per-attempt pattern already used by shutdownControlServerWithRetry two calls away in the same file. A stalled control server could hold backend startup open past the configured AC-EXECUTORS-SURVIVAL-003.7 bound, up to the HTTP client's own much larger internal timeout.
instanceAuth's accept check and a stream handler's own Invalidated() call were two independent lock acquisitions, so a rotation landing in the gap between them could authenticate a request against the generation it just superseded while handing the handler a channel for the generation that replaced it -- a channel that never closes for the rotation that actually invalidated the accepted credential. AcceptsFullWithInvalidation captures both atomically under one lock; instanceAuth stores the result on the gin context for /agent/stream and /workspace/stream to read instead of calling Invalidated() independently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…claim TestDefinitionsIncludeAgentSurvivalMetadata's doc comment claimed the flag is "off by default" (AC-EXECUTORS-SURVIVAL-005.2) but nothing in the test body checked that -- only EnvVar, Stability, RiskLevel, RiskDescription, RestartRequired and Mutable. Assert profiles.yaml's resolved default for agent_survival is "false" so the claim is actually pinned. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…overy AC-EXECUTORS-SURVIVAL-002.5: recovery reads WorkspaceSourceRoots back from the adopted instance rather than pushing a value. If that comes back nil/empty, the allowlist gate must reject every durable-source symlink escape instead of treating "no roots configured" as "no restriction" -- ordinary in-workspace operations are unaffected either way.
…rupted AC-EXECUTORS-SURVIVAL-002.9: RecoverAll concatenates every registered runtime's Winners without cross-runtime session dedup, so two different runtime backends reporting a live instance for the same session ID in one Manager.Start recovery pass is reachable even though each runtime's own correlation is keyed by session ID. Proves the existing ExecutionStore.Add/ErrExecutionAlreadyExistsForSession guard already handles it: exactly one execution lands in the store, the loser is never independently reachable, and the collision is logged rather than silently dropped or corrupted.
… rotation AC-EXECUTORS-CONTROL-OWNERSHIP-002.4: two backends racing AttemptAdoptControlServer against the same agentctl both present its current credential to Rotate at effectively the same time. Only the sequential idempotent-retry case was covered before. Adds a genuine concurrent-goroutine test (run under -race) proving credentialState's mutex-serialized rotation converges both racers on the identical (rotationID, replacement) pair instead of erroring, double-allocating a rotation, or corrupting shared state.
… server identity The credential issued by the highest-numbered rotation is the only one that authenticates a stream, but only the agent and workspace streams observed their credential being superseded. The shell terminal stream, the LSP bridge, the port proxy, and the code-server proxy all outlived it: a superseded backend kept writing raw bytes into the workspace PTY and kept proxied sockets open long after every ordinary request from the same holder was refused. All four now take the invalidation channel from the gin context where the authenticating accept check placed it, never a fresh lookup, which would reopen the accept-to-capture window closed in d511c303b. Adoption also treated a control-server record carrying no server identity as proven, leaving the shared home directory as the only comparison, which every control server on the installation satisfies. An identity neither side can produce is now refused rather than assumed, and the comment justifying the open default described a state the schema cannot produce. Covers the capability gate itself on both branches: with survival disabled nothing is adopted and a detached server is reclaimed, with it enabled a recorded server is taken over by rotating its credential. Deleting the gate previously left the suite green.
…nds the credential
Review round 7 found that adoption authenticates in one direction only. The
backend presents the stored credential to prove itself, but the control server
proves nothing: identityMatchesRecordedServer compares server_identity and
home_dir, and both are returned verbatim by the unauthenticated /identity
endpoint. Any local process can read them, take the recorded port once the real
server releases it, be adopted, receive the durable credential, and have the
replacement it invents written to the secret store. Because an adopted endpoint
becomes cfg.Agent.Standalone{Host,Port,AuthToken} for the life of the backend,
that compromises every later agent launch, shell execution and workspace file
operation, not one session.
AC-EXECUTORS-CONTROL-OWNERSHIP-001.2 as frozen did not forbid this: it requires
only an identity match and successful authentication, and a hostile server
simply answers 200. Design 01 stated the right intent (presentation
authentication rather than comparison of an echoed value) but no criterion made
it a wire requirement.
Adds AC-EXECUTORS-CONTROL-OWNERSHIP-001.10 (per-attempt challenge, response
derivable only from the credential, required before the credential is sent or
any replacement is stored) and -001.11 (no filesystem paths on the
unauthenticated endpoint). Amends AC-EXECUTORS-CONTROL-OWNERSHIP-004.1, which
contradicted -001.3: authenticating is itself an operation, so no-operation-
before-capability-comparison and authenticate-before-compatibility could not
both hold. The proof, the rotation and identity retrieval are now excepted.
Adoption authenticated in one direction only: it read /identity, compared public values a hostile process can echo, then sent the recorded credential to whatever answered the recorded port. Both AttemptAdoptControlServer and ReclaimUnneededControlServer took that path, and the reclaim one runs with the capability off. An adopting backend now issues a per-attempt challenge and requires an HMAC response derivable only from the credential and the home directory. The credential is never the demonstration. /identity drops home_dir and diagnostic_log_path, which move behind authentication on /api/v1/ownership/details, so an unauthenticated caller learns no path. Also in this round: - Ignoring SIGPIPE is gated on the capability. It removes an inherited-pipe kill path, which must stay intact when nothing may outlive the backend. - The agentctl launcher's port check uses the dual-stack loopback probe the spec points at, now shared in internal/common/netprobe. A wildcard bind can succeed against an active listener on macOS/BSD. - A record inherited from an earlier launch is judged against the enumeration only when this backend adopted that server. When nothing answered at the recorded endpoint it falls to the process probe, so dead records are repaired instead of staying unknown forever. - Documents the five agent-survival config keys, the control-server routes and their three auth tiers, and the unavailable toggle state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lity AC-EXECUTORS-SURVIVAL-005.3 requires a passthrough session to behave as if the capability were disabled: never detached, never re-tracked, never represented as having survived. Its agent runs on a terminal owned by the backend process, so it dies with the backend whatever the control server does. Only the recovery-guard half of that was implemented. Two leaks followed. StopAllAgents calls StopAgentWithReason with StopReasonBackendShutdown for every tracked execution, and the detach gate tested only the capability flag, the reason, and the standalone runtime, so a passthrough execution was detached instead of stopped: no StopInstance, no agent.stopped, and an executors_running row left claiming a live agent that no later pass repairs. Separately, a passthrough session owns a real agentctl instance, so that instance genuinely outlives the backend, correlates to the session's recovery-inventory record, and was re-tracked and published as running while its PTY agent was gone. Exclude passthrough executions from the detach gate using the in-memory idiom this package already uses, and filter confirmed-passthrough records out of the recovery inventory using the classification SessionsToGuard has already computed for the pass. A session whose passthrough mode cannot be read stays both guarded and recoverable, which is the fail-safe direction the AC names. Also correct identityMatchesRecordedServer's docstring, which described a home-directory check the function does not perform, and the configuration docs, which advertised a recoveryReadTimeout upper bound the combined-budget validation makes unreachable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Too many files changed for review (222 files, 100 file limit). Bypass the limit by tagging |
nova28
temporarily deployed
to
opencode-review-trusted
September 7, 2026 09:54 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Important Review skippedToo many files! This PR contains 224 files, which is 74 over the limit of 150. To get a review, reduce the PR to 150 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Team Run ID: 📒 Files selected for processing (224)
You can disable this status message by setting the 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 |
Contributor
|
Claude finished @nova28's task in 9m 21s —— View job Code Review — feat(executors): keep worktree and local agents running across a backend restartReviewed the full diff. No blockers. One suggestion posted inline; notes below. Suggestion
What was checkedMutual ownership proof (security) Two-phase credential rotation Kill-path coverage Unowned-shutdown reaper Adoption flow DB schema
Test coverage Verdict✅ Approved — the security model, concurrency invariants, and kill-path coverage are all correct. The one suggestion above (platform-specific string match) is non-blocking. |
![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: 5b6435cff1
ℹ️ 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".
… seam CI caught two real defects the local run could not. The architecture lint refused a new allowlist entry: backendapp/agentctl.go imported internal/agent/runtime/lifecycle directly, and that baseline may only shrink. The repo's own convention says new consumers depend on the internal/agent/runtime seam rather than on lifecycle, so expose the adoption handshake, its record store, the inherited-record scope and the ownership renewer there, in the same shape as the existing SSHTaskDirReclaimer, and put the baseline back to what main has. The Postgres round-trip failed on CreatedAt: PostgreSQL TIMESTAMP stores microseconds while Go's time.Time carries nanoseconds, so the first write's in-memory stamp reads back truncated and a nanosecond-exact comparison can never hold on that dialect. Compare at microsecond granularity instead. The regression the assertion exists to catch is an upsert that resets created_at to now, which the test's deliberate 2ms sleep puts milliseconds away, so it is still caught with room to spare. No production code compares these timestamps for equality, so this was a test-precision defect rather than a Postgres bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nova28
temporarily deployed
to
opencode-review-trusted
September 7, 2026 11:07 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Two failures made an undeterminable input look like a determinate one, each turning a transient error into a destructive conclusion. A failed read of the live standalone recovery-inventory records was logged and recovery continued with a nil record set. Correlation cannot tell an empty inventory from an unread one, so every live instance matched no record and went down the AC-EXECUTORS-SURVIVAL-002.6 orphan-stop path: one database blip during startup stopped every agent that had just survived the restart. That criterion stops an instance because it is KNOWN to have no record, which a failed read never establishes. Recovery now reports nothing recovered and stops nothing on that path, the outcome AC-EXECUTORS-SURVIVAL-002.12 already sets for the mirror failure, the adopted server that cannot be enumerated. A successful read that returns no records stays authoritative and still stops orphans. Liveness classification fell back to the process-identifier probe whenever the enumeration scope was unreachable, conflating "nothing answered at the recorded endpoint" with "a server answered but could not be enumerated". A standalone row carries the SHARED control server's identifier, so in the latter case the probe reports a row live merely because that server is running, which AC-EXECUTORS-SURVIVAL-003.6 forbids, and the row escapes repair indefinitely. Presence is undeterminable there, so it is now unknown; the probe is retained only for the no-server case that criterion names. Both behaviours were confirmed red before the fix.
nova28
temporarily deployed
to
opencode-review-trusted
September 7, 2026 13:46 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
`features-contract.test.ts` asserts that the keys of `defaultFeatureFlags` equal the JSON keys of the backend's `FeaturesConfig`. This branch added `agent_survival` to that struct without adding the matching frontend default, so the contract test has been failing since the backend flag landed: 1 failed of 15950, `expected [...] to deeply equal ['agentSurvival', ...]`. The frontend half of the capability was otherwise present -- the Feature Toggles card already renders the unavailable state and translates the `platform_unsupported` reason code. Only the boot-payload feature map was missing the key. No web code reads `features.agentSurvival`, so this is contract compliance rather than a behaviour change; the toggle itself is served from the runtime flag registry. Appended last, matching the backend field order and the file's convention of appending new release toggles disabled.
nova28
temporarily deployed
to
opencode-review-trusted
September 7, 2026 15:14 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
Author
Fixup pass complete — all four review threads resolved; one flaky E2E shard needs a re-runTwo real defects were found by the review bots and are fixed, each confirmed red before the fix:
The remaining red check is not from this PR.
I could not clear it myself: Not rebased, deliberately: the workflows trigger One review comment was deferred rather than actioned: the failed |
AC-EXECUTORS-CONTROL-OWNERSHIP-003.8 names three renewing operations and then states the clause that carries the guarantee: beyond them, "no operation shall renew ownership: neither an instance operation, nor an open stream, nor enumeration". Only the three positive cases were pinned, so a renewal added to the shared credential middleware or to any instance route would have made a backend that is busy but has stopped renewing look present forever, defeating the unowned shutdown while every existing test stayed green. The new table drives real HTTP through the router so the middleware is on the path under test, covers enumeration, an instance operation, the ownership details read, health and identity, and asserts each call's own outcome before examining ownership -- a request that never reached the handler would also leave ownership unrenewed and would otherwise pass vacuously. Verified red by temporarily adding a renewing gin middleware to the control server: all five subtests failed with UnownedFor collapsing from the backdated hour to sub-millisecond. Production code is unchanged.
nova28
temporarily deployed
to
opencode-review-trusted
September 8, 2026 02:38 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tip
PR walkthrough: Open the visual walkthrough
Today: Restarting or upgrading Kandev kills every running agent. In-flight work is lost, and the task falls back to a cold resume that can cost up to ~31 minutes under load.
After this: On worktree and local executors the agent keeps running. The restarted backend takes the surviving control server over, reconnects to the sessions still on it, and picks up the turn that completed while it was gone.
Who hits this: Anyone who upgrades or restarts Kandev while a task is mid-turn, which is every upgrade and every
launchctl kickstart -k, for every task then running.Scope: Standalone, no sibling PRs. Worktree and
local_pcexecutors only, behind a runtime feature flag that is off in theprod,devande2eprofiles.Not here: Docker, SSH, Sprites and remote-docker survival. The remote case is tracked separately in #3457. Also not here: host-reboot survival, provider-native resume semantics, and transcript replay beyond the terminal turn outcome.
A backend restart terminated every agent because agentctl is killed three different ways on shutdown and nothing could re-adopt it afterwards; this lets one standalone control server outlive the backend and lets the next backend prove it owns that server, take it over, and reattach the sessions still running on it.
Important changes
Pdeathsigand Windows Job Object, andStopAllAgentsreachingagentctl.Stopbefore the executor'sStopInstance. Graceful backend shutdown now detaches a standalone execution instead of stopping it.GET /identitydeliberately discloses no filesystem path.GET /api/v1/instancesenvelope fix. The server returned a bare array while the client decoded{"instances": [...]}, so the pair had never worked. It now carriessession_idandtask_idfor the join againstexecutors_running.WorkspaceSourceRootsgates agentctl file operations, so guessing it would be a security regression rather than a missing nicety.control_server_recordstable holding the single installation-scoped record. The ownership credential lives in the secret store, never in the row.Validation
Run against this branch rebased onto
mainatcd7823631:make fmtclean;make typecheckclean;make lintclean;make lint-formatreports all files match Prettier style;cd apps/web && pnpm run i18n:ratchetreports the new-code ratchet clean and the guard allowlist intact at 644 entries.go vet ./...clean across the whole backend.go test ./...: 258 packages pass. Every residual failure was re-run at the merge basecd7823631in a scratch worktree and reproduces there identically, so none is attributable to this branch: the Kubernetes prepare-script pair, the sixTestWorktreePreparer_*cases,TestBuildAuthMethodsIdentityAgentOverridesEnvironment,TestManagedRuntimeCacheRepairUsesAgentEnvironmentAndExactTree,TestRepairManagedRuntimeCacheClearsPreviousStderrandTestCollectAgentEnvGitHubCLIShimSurvivesLoginShell. No survival, adoption, ownership or recovery test fails.apps/web/e2e/tests/session/agent-survival-restart.spec.tson the managed runner:1 passed (31.0s), proving a worktree session's in-flight turn survives a graceful restart, and asserting that re-tracking starts no new agent subprocess and issues no resume.6 files, 40 tests passed.Two gaps stated rather than implied. PostgreSQL was not exercised:
TestPostgresControlServerRecordRoundTripsself-skips withoutKANDEV_TEST_POSTGRES_DSNand no DSN was obtainable in this environment, so the new table's twoTIMESTAMPcolumns are unverified on Postgres. The DDL is a byte-for-byte copy of thedynamic_installation_keyssingleton pattern already in production. Separately, the local run covered the survival spec rather than the full five-project E2E suite; CI runs the whole suite.Screenshots
With the flag off, which is its state in every shipped profile, there is no user-visible change. The only new always-visible surface is the Feature Toggles row, captured on the managed E2E runner against synthetic data:
The two new session-view strings appear transiently during a recovery race with the flag on, and are not reachable in a default install.
Possible improvements
Medium risk, contained by the flag being off by default. The residual worth naming: if recording a freshly spawned control server fails, startup continues and that server outlives the backend with no durable record naming it, so the next backend spawns a second one and the orphan persists until its own unowned period elapses (10 minutes by default). Fixing it means choosing between failing startup and spawning non-survivably, which is a product decision rather than a fixup.
Design docs
docs/specs/executors/requirements/agent-survival-across-restart.mddocs/specs/executors/requirements/agent-survival-session-state.mddocs/specs/executors/requirements/standalone-control-server-ownership.mddocs/specs/executors/requirements/standalone-control-server-single-driver.mddocs/specs/executors/system-design/agent-survival-across-restart-01.md(and-02,-03)Preview Environment
c92fbca