diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 30ce5f838..1e2ebd232 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -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() diff --git a/internal/connector/admission/matrix.go b/internal/connector/admission/matrix.go index 5ddc4728c..a06ce9837 100644 --- a/internal/connector/admission/matrix.go +++ b/internal/connector/admission/matrix.go @@ -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 ` still runs one at once. + // ordinary. ReasonConfigUnreadable Reason = "config_unreadable" ) diff --git a/internal/connector/admission/verdict.go b/internal/connector/admission/verdict.go index 680454059..b0333d767 100644 --- a/internal/connector/admission/verdict.go +++ b/internal/connector/admission/verdict.go @@ -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 ` 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 diff --git a/internal/connector/handoff.go b/internal/connector/handoff.go index af32832b9..f76cca1bd 100644 --- a/internal/connector/handoff.go +++ b/internal/connector/handoff.go @@ -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 diff --git a/internal/connector/intake.go b/internal/connector/intake.go index d881c63fe..7c42ae881 100644 --- a/internal/connector/intake.go +++ b/internal/connector/intake.go @@ -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 ` 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 diff --git a/internal/connector/ledger.go b/internal/connector/ledger.go index 28d7f4b7b..0f3e26678 100644 --- a/internal/connector/ledger.go +++ b/internal/connector/ledger.go @@ -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 `. + // admission.NextBlockedRetry's schedule (Intake.sweepBlockedRetries). StateBlocked RecordState = "blocked" // StateDispatched was handed to a worker. StateDispatched RecordState = "dispatched" diff --git a/internal/connector/ledger_admission.go b/internal/connector/ledger_admission.go index 73dd1885b..cb788ead1 100644 --- a/internal/connector/ledger_admission.go +++ b/internal/connector/ledger_admission.go @@ -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). // diff --git a/internal/connector/ledger_decisions.go b/internal/connector/ledger_decisions.go index e7f5afd6c..80e854077 100644 --- a/internal/connector/ledger_decisions.go +++ b/internal/connector/ledger_decisions.go @@ -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 != "" diff --git a/internal/connector/ledger_tasks.go b/internal/connector/ledger_tasks.go index 7f6067ed4..608f80937 100644 --- a/internal/connector/ledger_tasks.go +++ b/internal/connector/ledger_tasks.go @@ -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 diff --git a/internal/connector/setup/private.go b/internal/connector/setup/private.go index 7c81b9d87..1e0549830 100644 --- a/internal/connector/setup/private.go +++ b/internal/connector/setup/private.go @@ -52,12 +52,9 @@ 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: @@ -65,33 +62,27 @@ var ErrSetupRunning = errors.New("another command holds this profile's connector // 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)