Remove what the connector's worker side left behind by robzolkos · Pull Request #829 · basecamp/basecamp-cli

robzolkos · 2026-10-02T19:30:02Z

Stacked on #821, which reworded the comments around this code and listed it as dead.

Why

#815 removed the connector's worker side, but left the code that worker side used. No command reaches it:

Ledger.Redispatch and its helpers (RedispatchResult, LiveWorker, authorizeBlocked, AuthorizedBlocked).
The ledger's whole write side for tasks and attempts: LaunchTask, EndAttempt, CreateTask, SupersedeTask, Dispatch with TaskDispatch (the worker's get_dispatch, ack_dispatch and complete_dispatch), MarkRunning, RecordTaker, ExposeEvent, JoinConversation, StartableRecords, adoption, refusals and still-running.
The outbox's readers and resolver (Intents, Intent, ResolveIntent, IsLifecycleReceipt). The outbox table is now written only by its migrations and triggers.
The ledger's Hooks. Only the removed dispatcher set them, so every call site ran a nil check and nothing more.
OpenExistingLedger and ResolveStateDir (the worker's MCP server), Held (the outbox sender), UnfinishedFromOthers (dangerous mode).
setup.TryLock and setup.Duration.
Almost all of internal/connector/driver: StartWorker, the preflight probe, the redactor, the environment builder, TerminateRecorded, ConfirmGroupGone and the process-group signals.

make deadcode finds the driver package, TryLock and Duration. It does not find the ledger methods: *Ledger reaches reflect, so deadcode keeps every exported method alive (-whylive says "reachable only through reflection"). I found those with a reference walk that ignores reflection, and confirmed each one with grep.

What

Removed, with the tests that only tested them: about 8,900 lines out, 1,100 in, across 56 files.

internal/connector/driver is now internal/connector/procid. The package kept only the process-identity code: Process, OwnsWorker, LookupProcess, ErrGroupOutlivedLeader and ErrIdentityUnknown, with the per-platform start-time readers. "driver" named an agent boundary that no longer exists, so a small package named for what it does is easier to read. OwnsWorker sends only the zero signal now; the signals that ended process groups went with TerminateRecorded.

Simplified where a removal left a branch with no caller:

openLedger loses its non-owner branch, because OpenExistingLedger was the only non-owner caller.
operatorEdges keeps only the edges a discard and an import take (into discarded). The edges out of completed and held belonged to redispatch.
eventTask, which a discard and an import read, keeps only the outcome and when it settled.
setup.Lock inlines its helper, because TryLock was the only other caller.
InstructionReply is now HandoffReply, beside HandoffLine, which is the only thing that uses it. The JSON is the same.

Comments. The dispatch lifecycle in ledger_dispatch.go, the tasks header in ledger_tasks.go, the outbox invariants and the hold invariants now say what this build does and what an older build's ledger can hold. They no longer describe functions that are gone.

Also fixed: internal/commands tests did not compile for Windows (a pre-existing problem since #811). Two tests in connect_start_wait_test.go use Unix-only fixtures. They move to connect_start_wait_unix_test.go, so GOOS=windows go vet ./... is clean.

No behavior change.

What stays and why

The schema, every migration, table, trigger and index, unchanged. Older ledgers hold tasks, attempts, task events, outbox intents and waiting redispatches, and the triggers still guard every write to those rows.
Status and doctor reporting of worker-era state: tasks with their worker and token taker (through procid.OwnsWorker), dispatches, intents, authorized_blocked and redispatch_pending. --json output is unchanged.
discard, import, shadow promote and the hold, including how they withdraw a redispatch an older build left waiting, and how they cancel a pending guard acknowledgement or holding reply.
move's guard on records a worker was handed (ErrHeldByWorker) and the lifecycle's edges into dispatched, because older ledgers hold dispatched records.
The IntentKind, IntentState, MessageKind and Outcome vocabularies, because status reads intents and outcomes through them. AttemptState, StopReason and Delivery had no reader left and are removed.
Non-worker test seams that deadcode still reports (Queue.Depth, Intake.CheckpointKey, admission.WithClock and others), and DropContent, which no command calls. These were dead before Remove the connector's worker side #815 and are not worker code, so this PR leaves them.

Tests

worker_era_test.go is new. It writes the rows the removed dispatcher wrote, with its statements and in its order, so the schema's triggers check them. The tests for status, discard, import, the hold, retention and move now build older-ledger state with it instead of calling the removed methods.
The tests that remain check behavior that still exists. For example: a hold, an import and a discard each withdraw a waiting redispatch; retention keeps a waiting redispatch's snapshot; status shows a worker-era task and an unaccounted token holder.
The unreadable connect.json test now waits for the record's timed retry instead of a redispatch.
procid keeps the identity tests: reused pid, no kernel start time, a group that outlived its leader, and zombie leaders on Linux and macOS.
make deadcode, before and after, on all five release platforms. Shipped binary: 157 to 85 unreachable functions (151 to 85 on Windows); internal/connector: 79 to 7 (74 to 8 on Windows). The -tags dev build drops the same set. The 7 that remain are the non-worker test seams above, and InstanceLock.Path. Windows adds the non-Unix procid.OwnsWorker stub: status asks about a recorded worker only on Unix, and the stub keeps the package's API the same on every platform.
go vet ./... is clean for linux, darwin, windows, freebsd and openbsd, and with -tags dev.
go test -race passes for internal/connector/... and internal/commands.
bin/ci is green.

Summary by cubic

Removes the code no command reaches since #815 removed the connector's worker side: the ledger's task and attempt writers, Redispatch and its helpers, the outbox readers and resolver, the never-installed ledger hooks, setup.TryLock and setup.Duration, and most of internal/connector/driver. No behavior change — about 8,900 lines out and 1,100 in, across 56 files.

What stays

The schema, migrations and triggers are unchanged; ledgers an older build wrote still hold tasks, attempts, outbox intents and waiting redispatches.
Status, doctor, discard, import, shadow promote and the hold still read and clear worker-era rows; the new worker_era_test.go writes those rows the way the removed dispatcher did, so the schema's triggers check them.
The process-identity code moves into internal/connector/procid, a package named for what it does.
Two Unix-only start-wait tests move behind a build tag, fixing a pre-existing Windows compile break.

^{Written for commit 4707e29. Summary will update on new commits.}

#815 removed the worker side but left the code it used, and no command reaches it: Ledger.Redispatch and its helpers, the ledger's task and attempt writers (LaunchTask, EndAttempt, CreateTask, SupersedeTask, Dispatch and TaskDispatch, MarkRunning, RecordTaker, ExposeEvent, JoinConversation, StartableRecords, adoption, refusals, still-running), the outbox readers and resolver, the never-installed ledger hooks, OpenExistingLedger, ResolveStateDir, Held, UnfinishedFromOthers, setup.TryLock, setup.Duration, and almost all of the driver package. The process-identity code status and guided setup use moves from internal/connector/driver to internal/connector/procid. The schema, migrations and triggers are unchanged; status, doctor, discard, import, shadow promote and the hold still read and decide about what an older build left. Tests for those build worker-era state with worker_era_test.go, which writes the rows the removed dispatcher wrote. Also moves two Unix-only start-wait tests behind a build tag, so the commands tests compile for Windows again.

cubic-dev-ai

6 issues found across 58 files

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/commands/connect_start_wait_unix_test.go">

<violation number="1" location="internal/commands/connect_start_wait_unix_test.go:63">
P2: The timeout path can still hang the suite indefinitely if cancellation fails to stop the command. Bound this receive as the second test does, then fail with a cancellation-specific message.</violation>
</file>

<file name="internal/connector/procid/procid.go">

<violation number="1" location="internal/connector/procid/procid.go:15">
P3: `processGone` returns gone for a missing PID, while non-Unix builds return `errUnsupported`; this promises `ErrIdentityUnknown` too broadly. Limit the claim to a present process and describe an unknown identity without promising that sentinel on every platform.</violation>
</file>

<file name="internal/connector/procid/procid_unix_test.go">

<violation number="1" location="internal/connector/procid/procid_unix_test.go:26">
P2: This interpolates the temporary path into shell code, so a temp directory containing spaces or shell metacharacters makes the child PID redirection fail. Pass `pidFile` as a positional argument and quote `"$1"` in the script.</violation>
</file>

<file name="internal/connector/outbox.go">

<violation number="1" location="internal/connector/outbox.go:16">
P3: Discard and import also run ordinary `UPDATE outbox` statements, so migrations and triggers are not the only write paths. Name those operations here to keep the comment aligned with the ledger behavior.</violation>

<violation number="2" location="internal/connector/outbox.go:30">
P3: The schema also permits `canceled` → `pending` when the old note is the refusal note. Include that edge; otherwise this list contradicts the trigger it summarizes.</violation>
</file>

<file name="internal/connector/ledger_decisions.go">

<violation number="1" location="internal/connector/ledger_decisions.go:31">
P3: `loadEventTask` still joins `tasks t` but selects only `te.outcome` and `te.completed_at`; every column the join fed (`t.superseded_at`, `t.ended_at`) was removed in this PR. Drop the join: nothing reads `t` anymore.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Re-trigger cubic}

cubic-dev-ai · 2026-10-02T19:34:11Z

+		assert.Contains(t, refusal.Hint, "One connector serves every repo")
+	case <-time.After(10 * time.Second):
+		cancel()
+		<-done


P2: The timeout path can still hang the suite indefinitely if cancellation fails to stop the command. Bound this receive as the second test does, then fail with a cancellation-specific message.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/commands/connect_start_wait_unix_test.go, line 63: <comment>The timeout path can still hang the suite indefinitely if cancellation fails to stop the command. Bound this receive as the second test does, then fail with a cancellation-specific message.</comment> <file context> @@ -0,0 +1,113 @@ + assert.Contains(t, refusal.Hint, "One connector serves every repo") + case <-time.After(10 * time.Second): + cancel() + <-done + t.Fatalf("a second connector waited for a token instead of refusing: %s", buf.String()) + } </file context>

Suggested change

<-done

select {

case <-done:

case <-time.After(10 * time.Second):

t.Fatal("the waiting start did not stop when canceled")

}

cubic-dev-ai · 2026-10-02T19:34:12Z

+func startWithChild(t *testing.T) (*exec.Cmd, Process, int) {
+	t.Helper()
+	pidFile := filepath.Join(t.TempDir(), "child")
+	cmd := exec.CommandContext(context.Background(), "/bin/sh", "-c", "sleep 300 & echo $! > "+pidFile+"; exec sleep 300")


P2: This interpolates the temporary path into shell code, so a temp directory containing spaces or shell metacharacters makes the child PID redirection fail. Pass pidFile as a positional argument and quote "$1" in the script.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/connector/procid/procid_unix_test.go, line 26: <comment>This interpolates the temporary path into shell code, so a temp directory containing spaces or shell metacharacters makes the child PID redirection fail. Pass `pidFile` as a positional argument and quote `"$1"` in the script.</comment> <file context> @@ -0,0 +1,153 @@ +func startWithChild(t *testing.T) (*exec.Cmd, Process, int) { + t.Helper() + pidFile := filepath.Join(t.TempDir(), "child") + cmd := exec.CommandContext(context.Background(), "/bin/sh", "-c", "sleep 300 & echo $! > "+pidFile+"; exec sleep 300") + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + require.NoError(t, cmd.Start()) </file context>

Suggested change

cmd := exec.CommandContext(context.Background(), "/bin/sh", "-c", "sleep 300 & echo $! > "+pidFile+"; exec sleep 300")

cmd := exec.CommandContext(context.Background(), "/bin/sh", "-c", "sleep 300 & echo $! > \"$1\"; exec sleep 300", "sh", pidFile)

cubic-dev-ai · 2026-10-02T19:34:12Z

+//     is left. Status reads a worker-era task's worker and taker through it.
+//
+// Where the kernel will not give a start time, or a record carries none,
+// nothing here guesses: the answer is ErrIdentityUnknown, never "gone".


P3: processGone returns gone for a missing PID, while non-Unix builds return errUnsupported; this promises ErrIdentityUnknown too broadly. Limit the claim to a present process and describe an unknown identity without promising that sentinel on every platform.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/connector/procid/procid.go, line 15: <comment>`processGone` returns gone for a missing PID, while non-Unix builds return `errUnsupported`; this promises `ErrIdentityUnknown` too broadly. Limit the claim to a present process and describe an unknown identity without promising that sentinel on every platform.</comment> <file context> @@ -0,0 +1,50 @@ +// is left. Status reads a worker-era task's worker and taker through it. +// +// Where the kernel will not give a start time, or a record carries none, +// nothing here guesses: the answer is ErrIdentityUnknown, never "gone". +package procid + </file context>

cubic-dev-ai · 2026-10-02T19:34:12Z

 //
-// # Invariants
+// Since #815 removed the worker side, this build writes no intent and sends
+// none: the table is written only by its migrations and by the triggers below.


P3: Discard and import also run ordinary UPDATE outbox statements, so migrations and triggers are not the only write paths. Name those operations here to keep the comment aligned with the ledger behavior.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/connector/outbox.go, line 16: <comment>Discard and import also run ordinary `UPDATE outbox` statements, so migrations and triggers are not the only write paths. Name those operations here to keep the comment aligned with the ledger behavior.</comment> <file context> @@ -1,87 +1,40 @@ // -// # Invariants +// Since #815 removed the worker side, this build writes no intent and sends +// none: the table is written only by its migrations and by the triggers below. +// What an older build left in it is still read and still decided about. +// Status lists the intents waiting for a person (statusIntents), and a </file context>

Suggested change

// none: the table is written only by its migrations and by the triggers below.

// none: migrations, triggers, discard, and import update the table.

cubic-dev-ai · 2026-10-02T19:34:12Z

+//  3. States move along the lifecycle's edges only (outbox_state_edges):
+//     pending to sending or canceled; sending to sent, indeterminate, or
+//     canceled when Basecamp refused the request; indeterminate to sent,
+//     abandoned or pending, those three only by a person.


P3: The schema also permits canceled → pending when the old note is the refusal note. Include that edge; otherwise this list contradicts the trigger it summarizes.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/connector/outbox.go, line 30: <comment>The schema also permits `canceled` → `pending` when the old note is the refusal note. Include that edge; otherwise this list contradicts the trigger it summarizes.</comment> <file context> @@ -1,87 +1,40 @@ +// 3. States move along the lifecycle's edges only (outbox_state_edges): +// pending to sending or canceled; sending to sent, indeterminate, or +// canceled when Basecamp refused the request; indeterminate to sent, +// abandoned or pending, those three only by a person. +// 4. get_dispatch canceled the guard: a trigger moved the guard intent from // pending to canceled in get_dispatch's own transaction, and a guard that </file context>

Suggested change

// abandoned or pending, those three only by a person.

// abandoned or pending, those three only by a person; a canceled intent

// with the refusal note may also return to pending.

cubic-dev-ai · 2026-10-02T19:34:12Z

+SELECT te.outcome, te.completed_at
 FROM task_events te
 JOIN tasks t ON t.id = te.task_id
-LEFT JOIN attempts a ON a.task_id = t.id AND a.state <> 'ended'
 WHERE te.event_id = ? AND te.withdrawn_at IS NULL


P3: loadEventTask still joins tasks t but selects only te.outcome and te.completed_at; every column the join fed (t.superseded_at, t.ended_at) was removed in this PR. Drop the join: nothing reads t anymore.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/connector/ledger_decisions.go, line 31: <comment>`loadEventTask` still joins `tasks t` but selects only `te.outcome` and `te.completed_at`; every column the join fed (`t.superseded_at`, `t.ended_at`) was removed in this PR. Drop the join: nothing reads `t` anymore.</comment> <file context> @@ -5,363 +5,45 @@ import ( err := tx.QueryRowContext(ctx, ` -SELECT te.task_id, te.delivery, te.outcome, te.completed_at, t.superseded_at, t.ended_at, - a.id, a.pid, a.pgid, a.process_started +SELECT te.outcome, te.completed_at FROM task_events te JOIN tasks t ON t.id = te.task_id </file context>

Suggested change

SELECT te.outcome, te.completed_at

FROM task_events te

JOIN tasks t ON t.id = te.task_id

LEFT JOIN attempts a ON a.task_id = t.id AND a.state <> 'ended'

WHERE te.event_id = ? AND te.withdrawn_at IS NULL

SELECT te.outcome, te.completed_at

FROM task_events te

WHERE te.event_id = ? AND te.withdrawn_at IS NULL

github-actions Bot added commands CLI command implementations tests Tests (unit and e2e) labels Oct 2, 2026

cubic-dev-ai Bot reviewed Oct 2, 2026

View reviewed changes

Navigation Menu

Repository navigation

Remove what the connector's worker side left behind - #829

Remove what the connector's worker side left behind#829
robzolkos wants to merge 1 commit into
connect-e2e/lock-docsfrom
connect-e2e/remove-worker-remnants

robzolkos commented Oct 2, 2026 •

edited by cubic-dev-ai Bot

Loading

Uh oh!

cubic-dev-ai Bot left a comment

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-		<-done
+		select {
+		case <-done:
+		case <-time.After(10 * time.Second):
+			t.Fatal("the waiting start did not stop when canceled")
+		}

	cmd := exec.CommandContext(context.Background(), "/bin/sh", "-c", "sleep 300 & echo $! > "+pidFile+"; exec sleep 300")
	cmd := exec.CommandContext(context.Background(), "/bin/sh", "-c", "sleep 300 & echo $! > \"$1\"; exec sleep 300", "sh", pidFile)

	// none: the table is written only by its migrations and by the triggers below.
	// none: migrations, triggers, discard, and import update the table.

	// abandoned or pending, those three only by a person.
	// abandoned or pending, those three only by a person; a canceled intent
	// with the refusal note may also return to pending.

Conversation

robzolkos commented Oct 2, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

What stays and why

Tests

Summary by cubic

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robzolkos commented Oct 2, 2026 •

edited by cubic-dev-ai Bot

Loading