dreampiler/kandev
1
0
Fork 0
Code Issues 43 Pull Requests Actions 6 Packages Projects Releases Wiki Activity

fix: reuse inherited task environment on session resume (#3081)

Browse Source
This commit is contained in:
João Salavisa
2026-08-27 11:12:47 +01:00
committed by GitHub
parent b3cdbf858c
commit 0e4ae865dc
3 changed files with 245 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/backend/internal/orchestrator/executor/executor_execute.go
+17
View File
@@ -2238,6 +2238,23 @@ func (e *Executor) persistTaskEnvironment(
if existingEnv != nil {
materializationSessionID := existingEnv.MaterializationSessionID
isInitialMaterializer := existingEnv.Status == models.TaskEnvironmentStatusCreating && materializationSessionID == session.ID
// A session attaching to an environment owned by a *different* task is a
// guest: office inherit_parent / shared_group bind every member session
// to one canonical environment (see handoff_inheritance.go), and the
// members share the owner's worktree rather than owning any
// task_environment_repos rows of their own. The owner already
// materialized (or will materialize) that inventory, so a guest must not
// rewrite the owner's repo rows or re-evaluate its ready status. Doing so
// on resume tripped "ready status requires repository inventory": the
// guest's request carries no repo specs (repos empty) and, when the
// owner's canonical inventory is empty, the repo-backed guard below failed
// a resume the guest has no authority to fix. The one exception is a guest
// session elected to materialize a still-CREATING canonical environment
// (shared_group), which must run the normal finalize path below.
if existingEnv.TaskID != "" && existingEnv.TaskID != taskID && !isInitialMaterializer {
session.TaskEnvironmentID = existingEnv.ID
return nil
}
previousStatus := existingEnv.Status
previousMaterializationSessionID := existingEnv.MaterializationSessionID
repos := environmentReposForLaunch(req, resp)
apps/backend/internal/orchestrator/executor/executor_resume.go
+25
View File
@@ -15,6 +15,7 @@ import (
"github.com/kandev/kandev/internal/integrations/cloneauth"
"github.com/kandev/kandev/internal/repoclone"
"github.com/kandev/kandev/internal/task/models"
"github.com/kandev/kandev/internal/task/repository/repoerrors"
"github.com/kandev/kandev/internal/worktree"
v1 "github.com/kandev/kandev/pkg/api/v1"
"go.uber.org/zap"
@@ -1124,6 +1125,30 @@ func (e *Executor) resolveResumeTaskEnvironment(ctx context.Context, taskID stri
return nil, fmt.Errorf("lookup existing task environment: %w", err)
}
if env == nil {
// Inherited-environment fallback: a child task created by an office
// task-handoff may have had session.TaskEnvironmentID rewritten to point
// at the parent's / shared group's env row, which is owned by a
// *different* task. GetTaskEnvironmentByTaskID misses that row because it
// indexes by the child task id, so without this lookup the resume path
// treats the env as absent and persistTaskEnvironment tries to CREATE a
// new row using the inherited ID — which already exists, producing
// "UNIQUE constraint failed: task_environments.id".
//
// Unlike the launch path (executor_execute.go), which hard-errors with
// ErrWorkspaceReuseUnsafe when the referenced row is absent, resume
// falls through to (nil, nil): the ID is then free, so the create path
// produces a valid fresh environment rather than a failing resume. A
// missing row surfaces as ErrTaskEnvironmentNotFound, which is a miss,
// not a fatal error; any other lookup error still propagates.
if session.TaskEnvironmentID != "" {
inherited, inhErr := e.repo.GetTaskEnvironment(ctx, session.TaskEnvironmentID)
if inhErr != nil && !errors.Is(inhErr, repoerrors.ErrTaskEnvironmentNotFound) {
return nil, fmt.Errorf("lookup inherited task environment: %w", inhErr)
}
if inherited != nil {
return inherited, nil
}
}
return nil, nil
}
if session.TaskEnvironmentID != env.ID {
apps/backend/internal/orchestrator/executor/executor_resume_inherited_env_test.go
+203
View File
@@ -0,0 +1,203 @@
package executor
import (
"context"
"testing"
"github.com/kandev/kandev/internal/task/models"
"github.com/kandev/kandev/internal/task/repository/repoerrors"
)
// TestResolveResumeTaskEnvironment_ReusesInheritedEnvironment is a regression
// test for "UNIQUE constraint failed: task_environments.id" on resume. A child
// task created by an office task-handoff (inherit_parent / shared_group) has its
// session.TaskEnvironmentID rewritten to point at the parent's / group's env
// row, which is owned by a *different* task. GetTaskEnvironmentByTaskID(childID)
// therefore returns nil, and without the inherited-env fallback the resume path
// treated the env as absent and re-created a row using the inherited ID, which
// already exists. The resolver must instead return the inherited row so the
// persist path takes its update branch.
func TestResolveResumeTaskEnvironment_ReusesInheritedEnvironment(t *testing.T) {
repo := newMockRepository()
exec := newTestExecutor(t, &mockAgentManager{}, repo)
repo.taskEnvironments["env-parent"] = &models.TaskEnvironment{
ID: "env-parent",
TaskID: "task-parent",
ExecutorType: string(models.ExecutorTypeLocal),
Status: models.TaskEnvironmentStatusReady,
}
session := &models.TaskSession{
ID: "sess-child",
TaskID: "task-child",
TaskEnvironmentID: "env-parent",
}
env, err := exec.resolveResumeTaskEnvironment(context.Background(), "task-child", session)
if err != nil {
t.Fatalf("resolveResumeTaskEnvironment: %v", err)
}
if env == nil || env.ID != "env-parent" {
t.Fatalf("resolved env = %+v, want inherited env-parent", env)
}
// Provenance: the returned row must be the parent's, not a freshly created
// child-owned row that happened to reuse the inherited ID.
if env.TaskID != "task-parent" {
t.Fatalf("env.TaskID = %q, want task-parent (should be the inherited parent row)", env.TaskID)
}
if len(repo.createTaskEnvironmentCalls) != 0 {
t.Fatalf("expected no CreateTaskEnvironment calls, got %d", len(repo.createTaskEnvironmentCalls))
}
if session.TaskEnvironmentID != "env-parent" {
t.Fatalf("session.TaskEnvironmentID = %q, want env-parent", session.TaskEnvironmentID)
}
}
// TestResolveResumeTaskEnvironment_MissingReferenceFallsThroughToCreate covers a
// session that references an env ID with no matching row (deleted env, or an env
// that was never persisted). The SQLite repo signals this with the production
// ErrTaskEnvironmentNotFound sentinel (not a nil,nil miss), so the resolver must
// treat that sentinel as absent, return nil, and let the caller create a fresh
// row using the free ID — preserving resume rather than aborting it.
func TestResolveResumeTaskEnvironment_MissingReferenceFallsThroughToCreate(t *testing.T) {
repo := newMockRepository()
exec := newTestExecutor(t, &mockAgentManager{}, repo)
// Mirror the sqlite repository, which returns ErrTaskEnvironmentNotFound
// (wrapped) rather than (nil, nil) when the referenced row is absent.
repo.getTaskEnvironmentFunc = func(_ context.Context, _ string) (*models.TaskEnvironment, error) {
return nil, repoerrors.ErrTaskEnvironmentNotFound
}
session := &models.TaskSession{
ID: "sess-child",
TaskID: "task-child",
TaskEnvironmentID: "env-missing",
}
env, err := exec.resolveResumeTaskEnvironment(context.Background(), "task-child", session)
if err != nil {
t.Fatalf("resolveResumeTaskEnvironment: %v", err)
}
if env != nil {
t.Fatalf("resolved env = %+v, want nil so the create path runs", env)
}
}
// TestResolveResumeTaskEnvironment_OwnedEnvironmentUnchanged locks in the
// pre-existing behavior for a normal (non-inherited) session: the env is found
// by task id and no inherited fallback lookup occurs.
func TestResolveResumeTaskEnvironment_OwnedEnvironmentUnchanged(t *testing.T) {
repo := newMockRepository()
exec := newTestExecutor(t, &mockAgentManager{}, repo)
repo.taskEnvironments["env-own"] = &models.TaskEnvironment{
ID: "env-own",
TaskID: "task-1",
ExecutorType: string(models.ExecutorTypeLocal),
Status: models.TaskEnvironmentStatusReady,
}
session := &models.TaskSession{ID: "sess-1", TaskID: "task-1"}
env, err := exec.resolveResumeTaskEnvironment(context.Background(), "task-1", session)
if err != nil {
t.Fatalf("resolveResumeTaskEnvironment: %v", err)
}
if env == nil || env.ID != "env-own" {
t.Fatalf("resolved env = %+v, want env-own", env)
}
if session.TaskEnvironmentID != "env-own" {
t.Fatalf("session.TaskEnvironmentID = %q, want env-own", session.TaskEnvironmentID)
}
}
// TestResolveResumeTaskEnvironment_NoEnvironmentAndNoReference returns nil so the
// caller can create a fresh environment (the ordinary cold-resume case).
func TestResolveResumeTaskEnvironment_NoEnvironmentAndNoReference(t *testing.T) {
repo := newMockRepository()
exec := newTestExecutor(t, &mockAgentManager{}, repo)
session := &models.TaskSession{ID: "sess-1", TaskID: "task-1"}
env, err := exec.resolveResumeTaskEnvironment(context.Background(), "task-1", session)
if err != nil {
t.Fatalf("resolveResumeTaskEnvironment: %v", err)
}
if env != nil {
t.Fatalf("resolved env = %+v, want nil", env)
}
}
// TestPersistTaskEnvironment_GuestDoesNotMutateOwnerEnvironment is a regression
// test for "persist task environment: ready status requires repository
// inventory" on resume. A guest session (office inherit_parent / shared_group)
// attaches to a canonical environment owned by a *different* task. The guest's
// resume request carries no repo specs and the owner's inventory can be empty,
// so the repo-backed ready-status guard used to fail a resume the guest has no
// authority to fix. The guest must attach without touching the owner's row.
func TestPersistTaskEnvironment_GuestDoesNotMutateOwnerEnvironment(t *testing.T) {
repo := newMockRepository()
exec := newTestExecutor(t, &mockAgentManager{}, repo)
// The child is repo-backed, which is what previously tripped the guard.
repo.taskRepositories["tr-child"] = &models.TaskRepository{
ID: "tr-child", TaskID: "task-child", RepositoryID: "repo-a",
}
owner := &models.TaskEnvironment{
ID: "env-parent",
TaskID: "task-parent",
ExecutorType: string(models.ExecutorTypeLocal),
Status: models.TaskEnvironmentStatusReady,
}
repo.taskEnvironments["env-parent"] = owner
session := &models.TaskSession{ID: "sess-child", TaskID: "task-child", TaskEnvironmentID: "env-parent"}
req := &LaunchAgentRequest{TaskID: "task-child", ExecutorType: string(models.ExecutorTypeLocal)}
resp := &LaunchAgentResponse{}
if err := exec.persistTaskEnvironment(context.Background(), "task-child", session, owner, req, resp, executorConfig{}); err != nil {
t.Fatalf("persistTaskEnvironment (guest): %v", err)
}
if len(repo.updateTaskEnvironmentCalls) != 0 {
t.Fatalf("guest must not update the owner env, got %d UpdateTaskEnvironment calls", len(repo.updateTaskEnvironmentCalls))
}
if len(repo.createTaskEnvironmentCalls) != 0 {
t.Fatalf("guest must not create an env, got %d CreateTaskEnvironment calls", len(repo.createTaskEnvironmentCalls))
}
if len(repo.finalizeTaskEnvironmentCalls) != 0 {
t.Fatalf("guest is not the materializer, got %d finalize calls", len(repo.finalizeTaskEnvironmentCalls))
}
if len(repo.writeCallLog) != 0 {
t.Fatalf("guest must not write repo rows, write log = %v", repo.writeCallLog)
}
if session.TaskEnvironmentID != "env-parent" {
t.Fatalf("session.TaskEnvironmentID = %q, want env-parent", session.TaskEnvironmentID)
}
}
// TestPersistTaskEnvironment_GuestMaterializerRunsFinalizePath proves the guest
// short-circuit does not swallow the shared_group case where a member session is
// elected to materialize a still-CREATING canonical environment. Even though the
// environment is owned by a different task, the elected materializer must run the
// normal finalize path so the group's inventory publishes atomically.
func TestPersistTaskEnvironment_GuestMaterializerRunsFinalizePath(t *testing.T) {
repo := newMockRepository()
exec := newTestExecutor(t, &mockAgentManager{}, repo)
owner := &models.TaskEnvironment{
ID: "env-group",
TaskID: "task-owner",
ExecutorType: string(models.ExecutorTypeLocal),
Status: models.TaskEnvironmentStatusCreating,
MaterializationSessionID: "sess-member",
}
repo.taskEnvironments["env-group"] = owner
session := &models.TaskSession{ID: "sess-member", TaskID: "task-member", TaskEnvironmentID: "env-group"}
req := &LaunchAgentRequest{TaskID: "task-member", ExecutorType: string(models.ExecutorTypeLocal), RepositoryID: "repo-a"}
resp := &LaunchAgentResponse{WorktreeID: "wt-a", WorktreePath: "/tasks/group/repo-a", WorktreeBranch: "feat/x"}
if err := exec.persistTaskEnvironment(context.Background(), "task-member", session, owner, req, resp, executorConfig{}); err != nil {
t.Fatalf("persistTaskEnvironment (guest materializer): %v", err)
}
if len(repo.finalizeTaskEnvironmentCalls) != 1 {
t.Fatalf("elected materializer must finalize, got %d finalize calls", len(repo.finalizeTaskEnvironmentCalls))
}
}