robzolkos · Oct 23, 2026
@@ -687,9 +687,11 @@ func newConnectServed(path string, file setup.File, log *slog.Logger) *connectSe
 // read, or the reason it could not be read. "As of the last read" is the
 // honest span: a reading is reused for connectServedTTL, and no lock is
 // taken, so a `connect setup --unserve` can complete between any read and
-// whatever the caller goes on to do with it. Dispatch treats an error as authorizing
-// nothing; admission holds the record as a configuration error rather than
-// answering that the project is not served.
+// whatever the caller goes on to do with it — for the handoff, a boost and a
+// line later (#952; #771 closed the same window for the dispatcher, under the
+// lock). The handoff treats an error as authorizing nothing; admission holds
+// the record as a configuration error rather than answering that the project
+// is not served.
 func (r *connectServed) Current() (map[int64]admission.Project, error) {
 	r.mu.Lock()
 	defer r.mu.Unlock()

@@ -197,6 +197,6 @@ const (
 	// internal/connector Intake.sweepBlockedRetries). A day is the right bound
 	// for a failing server, which is not coming back on its own after one; a
 	// broken connect.json is a local file, and an operator away for a week is
-	// ordinary. `basecamp connect redispatch <id>` still runs one at once.
+	// ordinary.
 	ReasonConfigUnreadable Reason = "config_unreadable"
 )
@@ -330,8 +330,8 @@ func (a *Admitter) Decide(ctx context.Context, ev Event) (out Verdict, err error
 			// discarded: a discard is the one outcome repairing the file
 			// cannot reverse, and a blocked record can still be run. It
 			// comes round on the blocked schedule until the file is back,
-			// with no window (NextBlockedRetry), and a person may run
-			// `basecamp connect redispatch <id>` sooner.
+			// with no window (NextBlockedRetry), so repairing the file is
+			// all a person needs to do.
 			return v.end(StateBlocked, ReasonConfigUnreadable), nil
 		}
 		// Every other gate discard turns on the trust set, the matrix or the

@@ -262,8 +262,12 @@ func handOff(ctx context.Context, opts HandoffOptions, record Record) error {
 	if line.Ack, err = acknowledge(ctx, opts, record); err != nil {
 		return err
 	}
-	// A hold can land while the boost is in flight. A record it moved is not
-	// handed off; its ack stays recorded for a later redispatch to reuse.
+	// A record that moved while it was acknowledged is not handed off. No
+	// hold moves one today: `connect --hold` sets the marker as a run starts,
+	// before any part runs, and `shadow promote` needs the connector stopped.
+	// A hold that could land beside a running handoff would need more than
+	// this re-check, which a hold between it and the close gets past: that
+	// record is left held, and the close from held is refused.
 	current, ok, err := opts.Ledger.Get(ctx, record.ID)
 	if err != nil {
 		return err

@@ -897,8 +897,8 @@ const blockedRetryBatch = 100
 // This is the whole of the blocked-record recovery schedule that
 // admission.NextBlockedRetry describes: without it that function computes a
 // time nothing acts on, and a read that failed during an outage, a throttle,
-// or an unreadable connect.json waits for somebody to notice and run
-// `basecamp connect redispatch <id>` on each one.
+// or an unreadable connect.json is never run again: no command re-runs a
+// blocked record.
 //
 // The scope is the run's --project buckets, passed down rather than assumed.
 // A connector restricted to project A that offered a blocked record from

@@ -42,8 +42,7 @@ const (
 	StateQueued RecordState = "queued"
 	// StateBlocked is retained and retried: a reason, never a transport
 	// failure dressed up as a verdict. Retried by the intake sweep, on
-	// admission.NextBlockedRetry's schedule (Intake.sweepBlockedRetries), and
-	// by a person at any time with `basecamp connect redispatch <id>`.
+	// admission.NextBlockedRetry's schedule (Intake.sweepBlockedRetries).
 	StateBlocked RecordState = "blocked"
 	// StateDispatched was handed to a worker.
 	StateDispatched RecordState = "dispatched"

@@ -46,9 +46,10 @@ var undecided = []RecordState{StateSeen, StateBlocked}
 //
 // A blocked record is loaded only when something has given it an attempt to
 // make, and only once that attempt is due — whoever handed the id over. The
-// queue carries an id and not the revision it was claimed at, and `basecamp
-// connect redispatch` runs beside a live connector, so a person can re-decide
-// a record between the sweep's offer and admission taking it; without this,
+// queue carries an id and not the revision it was claimed at, so a record can
+// be re-decided between the sweep's offer and admission taking it — by a
+// duplicate offer decided first, and, when this was written, by `connect
+// redispatch`, which went with the worker side (#815); without this,
 // admission would load the newer revision and decide it at once, inside the
 // interval that re-decision had just written (Copilot on #770).
 //

@@ -190,11 +190,11 @@ func (l *Ledger) redispatch(ctx context.Context, eventID int64, by string, serve
 	// reported success and left the record for a dispatcher that will refuse
 	// to launch it is worse than one that refuses here (Copilot on #765).
 	//
-	// served is a reading its caller took under connect.json's own lock and
-	// holds until this commits (`connect redispatch`'s authorizedRedispatch,
-	// in internal/commands), so an unserve lands wholly before that read or
-	// wholly after this write. This checks the set it was given; the caller
-	// is what makes the set current.
+	// This checks the set it was given; the caller is what makes the set
+	// current, by reading it under connect.json's own lock and holding the
+	// lock until this commits, so an unserve lands wholly before that read
+	// or wholly after this write. `connect redispatch` did, and it went with
+	// the worker side (#815); nothing outside tests calls this now.
 	dispatchable := !record.ContentDropped && len(record.Decision.Snapshot) > 0 &&
 		record.Decision.Served && slices.Contains(served, record.BucketID) &&
 		record.Decision.ConversationKey != ""

@@ -331,13 +331,11 @@ func (l *Ledger) launchTask(ctx context.Context, spec LaunchSpec, attemptID stri
 		// non-dispatcher caller fail closed.
 		//
 		// What closes the window between reading connect.json and
-		// committing this transaction is the caller's, and Dispatcher.launch
-		// — the only caller that reaches this one — now holds it: it reads
-		// the served set under connect.json's own lock and keeps the lock
-		// until this commits, so an unserve lands wholly before the reading
-		// or wholly after the task exists. This check is what that reading
-		// is spent on. (`connect redispatch` does the same thing for its own
-		// write, which is Ledger.Redispatch and not this.)
+		// committing this transaction is the caller's: read the served set
+		// under connect.json's own lock and keep the lock until this
+		// commits, so an unserve lands wholly before the reading or wholly
+		// after the task exists. Dispatcher.launch did, and it went with the
+		// worker side (#815); nothing outside tests calls this now.
 		return Launch{}, fmt.Errorf("connector: launch event %d in project %d, which is not served: %w", spec.EventID, record.BucketID, ErrNotStartable)
 	}
 	var busy bool

@@ -52,46 +52,37 @@ func ensurePrivateDirs(path string) error {
 	return nil
 }
 
-// ErrSetupRunning reports the per-profile policy lock held by somebody else.
-// Three callers take it, and which one is holding decides whether a wait is
-// worth anything: `connect setup` across its whole load-change-save, network
-// checks included; `connect redispatch`, a one-shot operator command, across
-// one read and one ledger write; and the running connector, for the moment
-// it authorizes a launch. See the block above LockWait.
+// ErrSetupRunning reports the per-profile policy lock held by somebody else:
+// another `connect setup`, guided or not, across its whole load-change-save,
+// network checks included. See the block above LockWait.
 var ErrSetupRunning = errors.New("another command holds this profile's connector policy")
 
 // ErrLockUnavailable reports a host that cannot take the setup lock at all:
 // a filesystem without flock, a lock file this user does not own. Setup's
 // guarantee is the lock, so it refuses rather than run without one.
 var ErrLockUnavailable = errors.New("this host cannot lock the connector's policy")
 
-// Who takes this lock, and for how long. Two holders, and a third form for
-// the one caller that must never wait:
+// Who takes this lock, and for how long. `connect setup` and guided setup
+// (Lock) hold it across their whole load, change and save, network checks
+// included: seconds, and on a slow or unreachable account longer than that.
+// Nothing else does. `connect redispatch` held it across one read and one
+// ledger write, and the running connector took it (TryLock) once per launch;
+// both went with the worker side (#815), and TryLock has no caller now. The
+// connector reads connect.json without it, through a short cache, which is
+// the window #952 asks about.
 //
-//   - `connect setup` (Lock) holds it across its whole load, change and
-//     save, network checks included. Seconds, and on a slow or unreachable
-//     account longer than that.
-//   - `connect redispatch` (Lock) holds it across one file read and one
-//     ledger transaction, then lets go before it re-runs a prerequisite
-//     against Basecamp. Milliseconds. It is a one-shot operator command,
-//     not the connector.
-//   - the running connector (TryLock) takes it once per launch, across one
-//     file read and one ledger transaction, and never waits: a pass that
-//     cannot take it dispatches nothing and asks again on its next tick.
+// Other locks are taken UNDER this one and never the other way about: the
+// credential key's lock on setup's final write. That one order is what keeps
+// this from deadlocking, and a caller that waits on this while holding
+// something a holder waits on is how it stops being one. There is no such
+// caller today.
 //
-// Setup is the only one that holds it across a network call. Other locks are
-// taken UNDER this one and never the other way about: the ledger's SQLite
-// locks on a redispatch and on a launch, and the credential key's lock on
-// setup's final write. That one order is what keeps this from deadlocking,
-// and a fourth caller that waits on this while holding something a current
-// holder waits on is how it stops being one. There is no such caller today.
-//
-// LockWait bounds Lock's wait for a holder to finish. The connector takes
-// this lock too now, and what it does under it is one file read and one
-// SQLite transaction — bounded by the ledger's own five-second busy retry —
-// so a setup that refused the instant a dispatcher pass overlapped it would
-// fail for no reason a person could act on. Ten seconds is that worst case
-// with room, and a setup that waits it out reports a real holder.
+// LockWait bounds Lock's wait for a holder to finish. It was sized for the
+// connector's per-launch hold, one file read and one SQLite transaction, so
+// that a setup overlapping a dispatcher pass would wait rather than fail for
+// no reason a person could act on. With that holder gone, what it waits for
+// is another setup finishing within the bound; one still going after it is
+// refused.
 //
 // A variable so tests can shorten it.
 var LockWait = 10 * time.Second
@@ -113,25 +104,22 @@ const lockPoll = 20 * time.Millisecond
 // a refusal, never a fall-through: "the policy could not be checked" is not
 // permission to change it.
 //
-// The wait is for the connector's hold, which is milliseconds. It cannot
-// tell one holder from another, though, so a second `connect setup` whose
-// predecessor finishes inside LockWait is now queued rather than refused —
-// two setups run one after the other where they used to be one setup and one
-// refusal. That is a real change, and the price of not failing a setup that
-// happens to overlap a dispatcher pass. Nothing is written twice by it: each
-// one loads, changes and saves under the lock in turn, which is what the
-// lock is for. A second setup whose predecessor is still going after
-// LockWait is still refused.
+// The wait was for the connector's hold, which was milliseconds and is gone
+// (#815). What it still does: a second `connect setup` whose predecessor
+// finishes inside LockWait is queued rather than refused, so two setups run
+// one after the other. Nothing is written twice by it: each one loads,
+// changes and saves under the lock in turn, which is what the lock is for. A
+// second setup whose predecessor is still going after LockWait is refused.
 func Lock(ctx context.Context, path string) (unlock func(), err error) {
 	return lock(ctx, path, LockWait)
 }
 
 // TryLock takes the lock if it is free and reports ErrSetupRunning if it is
-// not, without waiting. It is the dispatcher's form: a pass that cannot take
-// the lock authorizes nothing and tries again on its next tick, which costs
-// one tick of latency. Waiting instead would park the dispatcher behind a
-// `connect setup`'s network checks, and a dispatcher that blocks on a file
-// another process writes is its own hazard.
+// not, without waiting. It was the dispatcher's form, and nothing calls it
+// since the dispatcher went (#815): a pass that could not take the lock
+// authorized nothing and tried again on its next tick, because waiting would
+// have parked the dispatcher behind a `connect setup`'s network checks. A
+// connector that held the lock again (#952) would want the same.
 // It takes no context because it does not wait: there is nothing to cancel.
 func TryLock(path string) (unlock func(), err error) {
 	return lock(context.Background(), path, 0)