You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
basecamp connect setup --unserve <project> takes a project away from the agent. Until now it could take it away a moment too late. The connector read which projects it served and then acted on that reading, with nothing stopping the file from changing in between: a dispatcher pass read connect.json once and handed that reading down to the launch, and basecamp connect redispatch read the file when the command started and wrote its decision after opening the ledger. An unserve completing inside either gap still started one task in a project the operator had just withdrawn.
The exposure was one task: the next tick re-reads and stops the follow-ups, and a redispatch authorizes one record per invocation. It was also not a regression — before The connector runs where it is started: no directory is a project's the originating record was authorized at selection and never re-checked at launch, exactly the same shape. What changed is that the same PR made the served set the whole boundary between "the agent may act here" and "it may not", and a boundary that is read but never held is a boundary with a gap in it.
The shape
There is already a lock for this. .connect.lock sits beside connect.json and connect setup holds it across its whole load, change and save. Both sites now take it, read the file fresh under it, and keep it until the ledger has committed — so an unserve lands wholly before the reading, or wholly after the task exists.
The reader's short-lived cache is not replaced by the lock; it sits under it. The cache stays what admission and each dispatcher pass read: admission decides once per event, and a file lock per event is not a thing to put on that path. A pass still reads the cache to choose the records worth trying, and the launch re-reads under the lock to decide. A record the pass offered and the launch then refuses is the mechanism working, not two halves disagreeing.
When the lock cannot be taken
The dispatcher never waits. It tries, and a pass that loses gives up its turn: nothing is lost, and the record is still waiting on the next pass. Waiting would park the dispatcher's hot path behind a connect setup's network checks, which is the hazard the card named. It does mean that while a setup is running the connector starts nothing — every pass in that window gives up — so the latency is the length of that setup, not a tick, and it is spent queueing rather than acting on a policy somebody is in the middle of changing.
connect redispatch is an operator's command, so it does wait — bounded — and reports the profile busy and retryable if a setup outlasts the wait. Setup's own lock now waits briefly as well rather than refusing on sight, so a dispatcher pass that happens to overlap a setup cannot make connect setup fail for a reason nobody could act on.
A lock nobody could take is kept apart from a lock somebody holds. Both authorize nothing; only one of them means something is wrong, and only that one is worth saying out loud. And a wait honours its context: an operator who presses Ctrl-C during contention stops waiting rather than sitting out the bound, and a command whose context has already ended does not go on to take the lock and act under it. A caller that cannot get the lock always gets a refusal — "the policy could not be checked" is never permission to change it.
The lock now has three callers with three very different holds — setup across its network checks, redispatch across one ledger write, the connector across one launch and never waiting — so who holds it and for how long is written down where the lock is defined, next to the rule that nothing takes another lock while holding it.
What is held under the lock is one file read and one SQLite transaction. It is released before a session directory, a token socket or a worker process exists.
What is deliberately left open
A worker that is already running keeps being handed follow-ups against the reader's cached set, not against a locked one, so a task whose project stops being served stops being fed within one TTL rather than instantly. That is on purpose. A follow-up runs on the task's own goroutine, where "the lock was busy" and "there is no more work" are the same answer — a launch can give up its turn and try again next tick, and this cannot, so contention there would end tasks early. The task was authorized at launch; what the TTL bounds is how long one already running keeps going after an unserve. Invariant 2 in the dispatcher now says that rather than claiming "the moment".
A blocked record's redispatch re-runs its prerequisite against Basecamp, and that read does not happen under the lock — nothing that waits on the network does. So the rerun's verdict is taken against the policy the redispatch was authorized with, not against the policy as it stands when the verdict is written, and an unserve landing while the prerequisite runs can still be written as an admitted record. The consequence is bounded and known: the launch under the lock refuses it, and the stranded report names it. It is the same residue as admission's own cached read, which is what the TTL bounds, and the comments there say so rather than claiming current-policy admission.
The second card does not dissolve
The launch window closes. The TTL window does not: admission still reads through the cache, so a record can be admitted into a project that stopped being served within the last couple of seconds, which dispatch then refuses and the stranded report names. That bound was documented and never asserted — raising the constant to a minute would have left every test passing while quietly widening it. It is pinned now, and so is the fact that the cache is real, because a number nothing obeys is not a bound either.
Tests
Every new test was proved red before it was proved green, against a mutation that removes the mechanism it claims to cover: the launch back on the pass's snapshot, the authorized read without its lock, the redispatch back on the start-up copy, the release held past the launch, the TTL raised to a minute, and the cache made to reload every time.
A launch and a `connect setup --unserve` could interleave: the dispatcher
read connect.json once per pass and handed that reading down to LaunchTask,
and `connect redispatch` read the file at start-up and passed those buckets
to Ledger.Redispatch with the ledger opened in between. An unserve landing
in either window still started one task. The exposure was one — the next
tick re-reads and stops the follow-ups — and it was pre-existing in kind
rather than a regression, but the served set is the whole boundary now, so
the window is worth closing.
Both sites now read the served set under `.connect.lock`, the per-profile
lock `connect setup` already holds across its load, change and save, and
keep the lock until the ledger has committed. The reading is fresh under
the lock: the TTL cache stays what admission and each dispatcher pass read,
and reusing it here would reintroduce the gap the lock is taken to close.
Where the lock goes and what happens when it cannot be taken:
- connectServed.Authorize takes the lock, reloads, and hands back the
release. Launch and redispatch use it; nothing else does. Admission
keeps Current and its TTL — it decides once per event, and a file lock
per event is not a thing to put on that path.
- The dispatcher never waits: setup.TryLock, and a pass that loses gives
up its turn for one tick. Waiting would park the hot path behind a
`connect setup`'s network checks. Nothing is lost; the record is still
waiting next pass.
- `connect redispatch` is an operator command, so it waits, bounded, and
reports busy and retryable when a setup outlasts the wait.
- setup.Lock now waits up to ten seconds rather than refusing on sight,
so a dispatcher pass that overlaps a setup cannot fail `connect setup`
for a reason nobody could act on.
- A lock nobody could take is kept apart from a lock somebody holds:
ErrPolicyUnreadable and ErrPolicyBusy. Both authorize nothing; only one
means something is wrong.
What is held under the lock is one file read and one SQLite transaction,
released before a session directory, a token socket or a worker process
exists.
The TTL is now asserted as well as documented (the second card). The window
it bounds does not close: admission still reads through the cache, so a
record can be admitted into a project unserved within the TTL, which
dispatch then refuses and the stranded report names. Two tests pin it — the
constant at two seconds, and that the cache is real, so the constant is the
bound rather than a number nothing obeys.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes policy authorization races by locking fresh connect.json reads through ledger commits.
Changes:
Adds waiting and non-blocking policy lock modes.
Reauthorizes launches and redispatches against fresh policy.
Adds lock, cache-TTL, and race regression tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Five findings from Copilot on #771, and one from an adversarial pass that
Copilot did not raise.
The one that mattered: a blocked record's redispatch re-runs what blocked
it, and `Ledger.Redispatch` sets that rerun without consulting the served
set at all. The fresh locked reading only narrowed the ledger's decision;
the rerun still built its policy from the file the command loaded at
start-up, so an unserve landing in between could produce an admitted
verdict for a project the operator had withdrawn. `servedBucketsUnderLock`
is now `authorizedProfile`, which returns the whole profile with the locked
reading in it, and the caller reassigns its own profile from it — the
start-up reading is gone from the scope rather than merely unused.
The wait honors its context. `Lock` takes one now and polls on a select, with
a check before each attempt as well as around the sleep, so a person who
stops the command during contention stops waiting, and a context that has
already ended takes the lock from nobody rather than acquiring it and going
on to act under it. What a caller that cannot take the lock gets is a
refusal, never a fall-through: "the policy could not be checked" is not
permission to change it. `TryLock` stays context-free, because it does not
wait.
Who holds this lock, and for how long, is written where the lock is defined:
setup across its whole run including network checks, redispatch across one
read and one ledger write, the connector once per launch and never waiting —
with the rule a fourth caller needs, that the ledger's locks are taken under
this one and never the other way about.
`connect redispatch` no longer borrows setup's recovery text: a lock failure
there names the command that actually failed, and stops claiming only setup
needs the lock.
Two contention tests were inheriting the production wait and spending twenty
seconds proving the clock works; they shorten it now, and a duration
assertion of my own came out of a test about what an answer is.
Two things the adversarial pass caught and Copilot did not. Every earlier
test still passed when the release was moved to before the launch
transaction — the lock taken rather than held — so the release now reads the
ledger and the test asserts the record had already moved. And invariant 2
claimed a worker stops being handed follow-ups "the moment" its project
stops being served, which was never true and is not true now: a follow-up
runs on the task's own goroutine, where a busy lock and "no more work" are
the same answer, so contention there would end tasks early. The follow-up
path keeps the reader's TTL deliberately, and the invariant says so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This fallback turns a broken Authorize implementation into permission to launch without holding the policy lock. A nil release violates the documented contract and should fail closed; otherwise a future wiring error silently recreates the unserve race this change is intended to eliminate.
The guard added last round failed open. `Authorize` answering with a served
set, no error and no release meant "the contract broke", and the dispatcher's
response was to substitute a no-op and launch anyway — a task started against
connect.json while nothing held it, recorded as authorized. That is the exact
thing this branch exists to make impossible, reachable through the defensive
branch written to make it safe, and the comment above it said the case could
not happen while the code below quietly carried on when it did. It is
ErrPolicyUnreadable now, and nothing launches.
The wait's own cancellation had two races left. A context ending between the
look and the flock came away holding the lock; a wait that ran out at the
same moment its context ended reported the holder rather than the
interruption. Both are checked now, and both are tested against a context
that is live when the wait looks and over when it looks again — the race
scheduled rather than hoped for.
The redispatch's lock is one function with the release deferred inside it, so
"held across the write, let go before the prerequisite" is structural rather
than a matter of reading the command carefully. Its test is behavioral now,
against a real connect.json and a real ledger on both paths out, and a
separate one pins that the command binds the authorized profile back over its
own, which every other test was blind to.
Comments that stopped matching the code: the follow-up path is the reader's
TTL and no longer claims otherwise in either place it is described; the
contention error names all three callers rather than two; the lock-order note
says what is actually taken under this lock, credential lock included, rather
than claiming nothing is; and a rename left a dead symbol in a comment.
What the rerun cannot promise is now written down rather than implied: it
reads Basecamp, so it cannot run under the lock, and its verdict is taken
against the policy the redispatch was authorized with. An unserve landing
while the prerequisite runs can still be written as admitted — refused at the
launch and named by the stranded report, the same residue as admission's own
cached read.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🤖 On the "previously missed" finding from the last review body — Fail closed when authorization release is nil, internal/connector/dispatcher.go. It carries no thread of its own, so the answer goes here.
It is right, and it is the sharpest thing said about this branch. The code was:
A nil error with no release means the contract broke. Substituting a no-op and returning success meant the launch went on believing it held connect.json's lock while nothing held it — an unlocked launch recorded as an authorized one, which is the single outcome this whole branch exists to make impossible, reached through the branch I added to make it safe. Two things made it worse rather than better: the doc comment above it asserted the case could not happen, and the inline comment justified the no-op as avoiding a panic, which is choosing to proceed unauthorized over crashing. On an authorization path that trade is backwards.
It fails closed now. err == nil && release == nil returns ErrPolicyUnreadable and launches nothing; the no-op substitution survives only on the error path, where there is genuinely just something to call on the way out. The doc says what the code does rather than what I hoped the caller would do.
Proved red first, and it is the useful kind of red: TestAnAuthorizeWithNoLockToHoldLaunchesNothing starts a worker under the old guard and starts none under the new one, and TestAnAuthorizeWithNoLockToHoldIsUnreadableNotBusy pins which kind of nothing it is — nobody could take the lock, not somebody holding it.
The adversarial pass on the same branch found four more of the same species, all fixed in the same commit: two cancellation races in the wait (a context ending between the look and the flock came away holding the lock; a wait expiring at the same moment as its context reported the holder rather than the interruption), the redispatch's release now deferred inside one function so "held across the write, let go before the network" is structural, and a set of comments about the follow-up path that claimed a freshness the reader's TTL does not give.
An adversarial pass over the finished branch found no hole left in the
locking, and five places where the words around it were wrong. A comment
that lies is the thing this change has been about since the first round, so
they are worth their own commit.
`connect redispatch`'s help now says what its re-run cannot promise, in the
place a person reads before running it: the prerequisite talks to Basecamp,
so it cannot hold the policy lock, and it decides against the served
projects as they were when the redispatch was authorized. An unserve landing
while it runs is not seen by that verdict. Nothing starts on it — the launch
reads connect.json under the lock and refuses a project no longer served,
and status counts what is left waiting. `rerunPrerequisite` said it decided
"exactly as the connector's admission would", which is true of the rules and
false of the reading; it now says which.
`ErrPolicyBusy` said connect.json was being changed. A redispatch holding
the same lock is writing SQLite and changing nothing, and the dispatcher
cannot tell the two apart anyway — the lock is held, which is all it knows
and all it needs to.
`LaunchTask` named `connect redispatch` as one of the callers holding the
lock across its transaction. Redispatch does that for its own write, which
is `Ledger.Redispatch` and not this one.
The note above `Lock` claimed a running setup takes far longer than the
wait, so two setups would still be refused rather than queued. A setup can
finish inside ten seconds. What is true is that queued setups each load,
change and save under the lock in turn, which is what the lock is for.
And one test claim: the helper test said the lock was held "until the
decision has been written" while writing no decision. It now says what it
checks, and a new source-level test carries the part only the source can
answer — that the release is deferred before the ledger's write and taken
nowhere else, which a test that only sees the lock free afterwards cannot
tell from a release taken too early.
A subtest name that had stopped being English is English again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
commandsCLI command implementationsskillsAgent skillstestsTests (unit and e2e)
2 participants
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.
github-actions
Bot
added
commands
basecamp connect setup --unserve <project>takes a project away from the agent. Until now it could take it away a moment too late. The connector read which projects it served and then acted on that reading, with nothing stopping the file from changing in between: a dispatcher pass read connect.json once and handed that reading down to the launch, andbasecamp connect redispatchread the file when the command started and wrote its decision after opening the ledger. An unserve completing inside either gap still started one task in a project the operator had just withdrawn.Originally tracked in The served set is read but never locked, so an unserve can land between check and commit and The served-set TTL is documented but never asserted.
Why now
The exposure was one task: the next tick re-reads and stops the follow-ups, and a redispatch authorizes one record per invocation. It was also not a regression — before The connector runs where it is started: no directory is a project's the originating record was authorized at selection and never re-checked at launch, exactly the same shape. What changed is that the same PR made the served set the whole boundary between "the agent may act here" and "it may not", and a boundary that is read but never held is a boundary with a gap in it.
The shape
There is already a lock for this.
.connect.locksits beside connect.json andconnect setupholds it across its whole load, change and save. Both sites now take it, read the file fresh under it, and keep it until the ledger has committed — so an unserve lands wholly before the reading, or wholly after the task exists.The reader's short-lived cache is not replaced by the lock; it sits under it. The cache stays what admission and each dispatcher pass read: admission decides once per event, and a file lock per event is not a thing to put on that path. A pass still reads the cache to choose the records worth trying, and the launch re-reads under the lock to decide. A record the pass offered and the launch then refuses is the mechanism working, not two halves disagreeing.
When the lock cannot be taken
The dispatcher never waits. It tries, and a pass that loses gives up its turn: nothing is lost, and the record is still waiting on the next pass. Waiting would park the dispatcher's hot path behind a
connect setup's network checks, which is the hazard the card named. It does mean that while a setup is running the connector starts nothing — every pass in that window gives up — so the latency is the length of that setup, not a tick, and it is spent queueing rather than acting on a policy somebody is in the middle of changing.connect redispatchis an operator's command, so it does wait — bounded — and reports the profile busy and retryable if a setup outlasts the wait. Setup's own lock now waits briefly as well rather than refusing on sight, so a dispatcher pass that happens to overlap a setup cannot makeconnect setupfail for a reason nobody could act on.A lock nobody could take is kept apart from a lock somebody holds. Both authorize nothing; only one of them means something is wrong, and only that one is worth saying out loud. And a wait honours its context: an operator who presses Ctrl-C during contention stops waiting rather than sitting out the bound, and a command whose context has already ended does not go on to take the lock and act under it. A caller that cannot get the lock always gets a refusal — "the policy could not be checked" is never permission to change it.
The lock now has three callers with three very different holds — setup across its network checks, redispatch across one ledger write, the connector across one launch and never waiting — so who holds it and for how long is written down where the lock is defined, next to the rule that nothing takes another lock while holding it.
What is held under the lock is one file read and one SQLite transaction. It is released before a session directory, a token socket or a worker process exists.
What is deliberately left open
A worker that is already running keeps being handed follow-ups against the reader's cached set, not against a locked one, so a task whose project stops being served stops being fed within one TTL rather than instantly. That is on purpose. A follow-up runs on the task's own goroutine, where "the lock was busy" and "there is no more work" are the same answer — a launch can give up its turn and try again next tick, and this cannot, so contention there would end tasks early. The task was authorized at launch; what the TTL bounds is how long one already running keeps going after an unserve. Invariant 2 in the dispatcher now says that rather than claiming "the moment".
A blocked record's redispatch re-runs its prerequisite against Basecamp, and that read does not happen under the lock — nothing that waits on the network does. So the rerun's verdict is taken against the policy the redispatch was authorized with, not against the policy as it stands when the verdict is written, and an unserve landing while the prerequisite runs can still be written as an admitted record. The consequence is bounded and known: the launch under the lock refuses it, and the stranded report names it. It is the same residue as admission's own cached read, which is what the TTL bounds, and the comments there say so rather than claiming current-policy admission.
The second card does not dissolve
The launch window closes. The TTL window does not: admission still reads through the cache, so a record can be admitted into a project that stopped being served within the last couple of seconds, which dispatch then refuses and the stranded report names. That bound was documented and never asserted — raising the constant to a minute would have left every test passing while quietly widening it. It is pinned now, and so is the fact that the cache is real, because a number nothing obeys is not a bound either.
Tests
Every new test was proved red before it was proved green, against a mutation that removes the mechanism it claims to cover: the launch back on the pass's snapshot, the authorized read without its lock, the redispatch back on the start-up copy, the release held past the launch, the TTL raised to a minute, and the cache made to reload every time.