{{ message }}
Repository navigation
Conversation
The queue between intake and admission decides four edges: the backlog crossing its warning depth, an offer waiting for room (which stops the feed read), that wait ending, and the backlog falling back below the warning depth. It handed them only to optional callbacks, and runConnect sets none, so in production every edge went nowhere. The package says the pause is reported rather than hidden; it was hidden. The queue now reports each edge on its logger before running any callback, with the depth and the threshold crossed, and says plainly when reading the feed is paused and why. Intake already adopts the connector's logger for the queue, so every caller that builds intake gets the report and none can forget to wire it. A callback that panics is contained after the report, so it cannot silence it.
A running connector learned that its agent was disconnected in Basecamp, or connected on another computer, only from the event feed. The feed asks Basecamp for something only when it connects, and while its live connection stays up it asks for nothing. Admission and intake ask all the time, but to them a refusal was one failed read among others: admission blocked the mention it was deciding as read_failed, intake kept the projects it last read, and the connector ran on. Every mention addressed to the agent was blocked, and nothing said why, for as long as the socket held. Reproduced end to end against the fake Basecamp: disconnect the agent, or connect it on another computer and let its token come due, while the connector streams; the operator's next mention is blocked read_failed and the connector does not exit. A credential watch now sees the reads admission and intake make. A token renewal the token endpoint refuses is Basecamp's answer already, and stops the run at once. A read answered 401 is only a reason to ask, since something other than Basecamp can answer 401: the watch asks Basecamp itself, as the feed's own stop does, and stops the run only when it confirms. Either way the connector exits 3 with the words the feed's stop uses, and status records it as disconnected.
Every fault is one Basecamp can produce, met the way the connector would meet it, with nothing in the CLI told a test is running: - SIGINT on a running connector logs that it is shutting down and exits 130. - A second connector for the same agent, from another directory sharing the state directory, exits 5 at once, names the process holding the agent, and asks Basecamp for nothing. - Two agents connected on one computer run side by side, and each hands off only its own mention. - An agent disconnected while its connector runs stops it with exit 3, whether the next read or the feed's reconnect meets it first. An agent connected on another computer stops it at its next token renewal. - A token endpoint rate-limited at start is waited out, said, and the connector comes up and hands off a mention. - An event committed while the cable is down arrives by the reconnect's catch-up, once. - A cable that stops pinging is called stale and replaced. - A recording read that fails twice then serves is admitted in one decision. - A mention admitted under --hold by a connector then killed is handed off once after `connect release` and a restart. - A backlog past its warning depth, behind held reads, is reported, and its recovery too. The fake gains Disconnect (the agent's tokens revoked, its secret cleared) and AgentClient.TokenLifetime, so a renewal comes due within a test; the harness gains a profile parameter for a second agent and AnotherComputer for a second home against the same Basecamp.
github-actions
Bot
added
commands
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Credential monitoring changes concurrent production shutdown behavior and warrants final human review.
Review effort: Balanced
Findings: None
What changed in this PR
Extends #823’s end-to-end harness with connector failure and recovery scenarios, and fixes detection of refused agent credentials while the feed stays connected.
Changes:
- Stops the connector when credential refusal is confirmed.
- Reports backlog warning, pause, resume, and recovery transitions.
- Adds failure scenarios and supporting fake-server and harness capabilities.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 14 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/connector/fakebasecamp/world.go">
<violation number="1" location="internal/connector/fakebasecamp/world.go:148">
P3: A negative `TokenLifetime` silently falls through to the one-hour default, although only zero is documented as the default; tests can then miss the expected renewal. Reject negative values in `World.check` so invalid setup cannot silently change the token lifetime.</violation>
</file>
<file name="internal/commands/connect_credential.go">
<violation number="1" location="internal/commands/connect_credential.go:69">
P2: A transient confirmation failure consumes this 401 signal just like a confirmed-valid credential; `confirmAgentRefused` discards the error, and the watch retries only after another 401. Preserve or retry unconfirmed suspects with backoff so a one-off outage cannot leave a disconnected connector running.</violation>
<violation number="2" location="internal/commands/connect_credential.go:98">
P3: The watch fires on any error carrying `auth.ErrAgentCredentialRefused`, but the auth layer also attaches that cause to stored refusal *holds* — including time-limited ones (`invalid_grant`, bare 401/403) that `Manager` reports as refreshable and expiring on their own (agent_hold.go: `e.Cause = errors.Join(ErrAgentCredentialRefused, errMintHeld)` for `mintHoldRefused` regardless of `Until`). While such a hold is stored, every `AccessToken` through `watchedTokens` stops the connector with exit 3 and status `disconnected`, telling the user to re-run setup, even though the hold would resolve without reconnecting. The refusal is not always definitive; only a permanent `invalid_client` hold is.</violation>
</file>
<file name="internal/connector/queue.go">
<violation number="1" location="internal/connector/queue.go:336">
P2: Repair-worker offers can raise these edges while the feed loop keeps consuming, so the pause/resume messages misstate feed state; describe queue admission waiting instead.</violation>
</file>
<file name="e2e/connect/failures_test.go">
<violation number="1" location="e2e/connect/failures_test.go:202">
P2: This test passes if the connector retries immediately: `limited.Times = 1` lets the next token request succeed, and the assertion only checks the warning. Assert that renewal is not retried until the 1-second `Retry-After` expires.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| case err := <-w.refused: | ||
| return err | ||
| case <-w.suspect: | ||
| if confirm(ctx) { |
Contributor
There was a problem hiding this comment.
P2: A transient confirmation failure consumes this 401 signal just like a confirmed-valid credential; confirmAgentRefused discards the error, and the watch retries only after another 401. Preserve or retry unconfirmed suspects with backoff so a one-off outage cannot leave a disconnected connector running.
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_credential.go, line 69:
<comment>A transient confirmation failure consumes this 401 signal just like a confirmed-valid credential; `confirmAgentRefused` discards the error, and the watch retries only after another 401. Preserve or retry unconfirmed suspects with backoff so a one-off outage cannot leave a disconnected connector running.</comment>
<file context>
@@ -0,0 +1,114 @@
+ case err := <-w.refused:
+ return err
+ case <-w.suspect:
+ if confirm(ctx) {
+ return errAgentCredentialNotTaken
+ }
</file context>
| log.Info("the backlog fell back below its warning depth", | ||
| "depth", edge.depth, "warn_at", q.warnAt) | ||
| case edgePause: | ||
| log.Warn("the backlog reached its pause depth; reading the feed is paused until admission makes room, so the backlog cannot outgrow memory", |
Contributor
There was a problem hiding this comment.
P2: Repair-worker offers can raise these edges while the feed loop keeps consuming, so the pause/resume messages misstate feed state; describe queue admission waiting instead.
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/queue.go, line 336:
<comment>Repair-worker offers can raise these edges while the feed loop keeps consuming, so the pause/resume messages misstate feed state; describe queue admission waiting instead.</comment>
<file context>
@@ -310,6 +320,27 @@ func (q *Queue) fire(edge queueEdge) {
+ log.Info("the backlog fell back below its warning depth",
+ "depth", edge.depth, "warn_at", q.warnAt)
+ case edgePause:
+ log.Warn("the backlog reached its pause depth; reading the feed is paused until admission makes room, so the backlog cannot outgrow memory",
+ "depth", edge.depth, "pause_at", pauseAt)
+ case edgeResume:
</file context>
| for _, l := range out.Logs { | ||
| waited = waited || (l.Level == "WARN" && strings.HasPrefix(l.Msg, "connector: could not get the agent's token yet")) | ||
| } | ||
| assert.True(t, waited, "the wait is said\n%s", out.Transcript()) |
Contributor
There was a problem hiding this comment.
P2: This test passes if the connector retries immediately: limited.Times = 1 lets the next token request succeed, and the assertion only checks the warning. Assert that renewal is not retried until the 1-second Retry-After expires.
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 e2e/connect/failures_test.go, line 202:
<comment>This test passes if the connector retries immediately: `limited.Times = 1` lets the next token request succeed, and the assertion only checks the warning. Assert that renewal is not retried until the 1-second `Retry-After` expires.</comment>
<file context>
@@ -0,0 +1,404 @@
+ for _, l := range out.Logs {
+ waited = waited || (l.Level == "WARN" && strings.HasPrefix(l.Msg, "connector: could not get the agent's token yet"))
+ }
+ assert.True(t, waited, "the wait is said\n%s", out.Transcript())
+ assert.Equal(t, 1, fault.Hits(), "the token was rate-limited once")
+ assert.Len(t, out.RequestsFor(ev.ID), 1)
</file context>
| // whole seconds of at least one; zero is Basecamp's hour. The fake | ||
| // does not retire a token when it runs out: the client renews by what | ||
| // it was told, and a test can wait for that to come due. | ||
| TokenLifetime time.Duration |
Contributor
There was a problem hiding this comment.
P3: A negative TokenLifetime silently falls through to the one-hour default, although only zero is documented as the default; tests can then miss the expected renewal. Reject negative values in World.check so invalid setup cannot silently change the token lifetime.
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/fakebasecamp/world.go, line 148:
<comment>A negative `TokenLifetime` silently falls through to the one-hour default, although only zero is documented as the default; tests can then miss the expected renewal. Reject negative values in `World.check` so invalid setup cannot silently change the token lifetime.</comment>
<file context>
@@ -141,12 +141,20 @@ type AgentClient struct {
+ // whole seconds of at least one; zero is Basecamp's hour. The fake
+ // does not retire a token when it runs out: the client renews by what
+ // it was told, and a test can wait for that to come due.
+ TokenLifetime time.Duration
}
</file context>
Contributor
There was a problem hiding this comment.
P3: The watch fires on any error carrying auth.ErrAgentCredentialRefused, but the auth layer also attaches that cause to stored refusal holds — including time-limited ones (invalid_grant, bare 401/403) that Manager reports as refreshable and expiring on their own (agent_hold.go: e.Cause = errors.Join(ErrAgentCredentialRefused, errMintHeld) for mintHoldRefused regardless of Until). While such a hold is stored, every AccessToken through watchedTokens stops the connector with exit 3 and status disconnected, telling the user to re-run setup, even though the hold would resolve without reconnecting. The refusal is not always definitive; only a permanent invalid_client hold is.
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_credential.go, line 98:
<comment>The watch fires on any error carrying `auth.ErrAgentCredentialRefused`, but the auth layer also attaches that cause to stored refusal *holds* — including time-limited ones (`invalid_grant`, bare 401/403) that `Manager` reports as refreshable and expiring on their own (agent_hold.go: `e.Cause = errors.Join(ErrAgentCredentialRefused, errMintHeld)` for `mintHoldRefused` regardless of `Until`). While such a hold is stored, every `AccessToken` through `watchedTokens` stops the connector with exit 3 and status `disconnected`, telling the user to re-run setup, even though the hold would resolve without reconnecting. The refusal is not always definitive; only a permanent `invalid_client` hold is.</comment>
<file context>
@@ -0,0 +1,114 @@
+func (t watchedTokens) AccessToken(ctx context.Context) (string, error) {
+ token, err := t.provider.AccessToken(ctx)
+ if errors.Is(err, auth.ErrAgentCredentialRefused) {
+ t.watch.refuse(err)
+ }
+ return token, err
</file context>
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR 3 of a stack of 8 that tests
basecamp connectend to end. Stacked on #823 (the harness). It adds the failure and recovery scenarios, and fixes one real bug they found.The first commit is the same change as #820 (the connector reports its backlog edges on stderr). The backlog scenario needs it. When #820 merges, that commit drops out on rebase.
Why
#823 holds the connector to the happy path. A connector has to live through more than that: a Ctrl-C, a second copy started from another repo, a disconnect in Basecamp, a rate-limited token endpoint, a dropped or silent cable, a flaky read, a crash in the middle of a handoff, and a backlog. Each of these is a fault that Basecamp or the operator can really cause. Unit tests prove the parts. They do not prove what a person sees when the real binary meets the fault.
What
A product fix (its own commit, with unit tests): the connector now stops when Basecamp no longer takes its agent's credential.
The bug: a running connector learned of a disconnect only from the event feed. The feed sends a request to Basecamp only when it connects. While its live connection stays up, it sends nothing. Admission and intake send reads all the time, but to them a refusal was only one failed read. Admission blocked the mention as
read_failed, intake kept its last project list, and the connector ran on. Every mention to the agent was blocked, with no explanation, for as long as the socket stayed up.Reproduced end to end: while the connector streams, disconnect the agent, or connect it on another computer and let its token come due. Then the operator mentions the agent. The mention is blocked
read_failedand the connector does not exit.The fix is a credential watch (
internal/commands/connect_credential.go) on the reads that admission and intake make:invalid_client), that is already Basecamp's answer. The run stops at once.confirmAgentRefusedthe feed's stop uses). It stops the run only if Basecamp confirms.In both cases the connector exits 3 with the message the feed's stop uses ("... was disconnected in Basecamp, or connected on another computer", and the
connect setuphint), and status records it asdisconnected.The scenarios (
e2e/connect/failures_test.go). No knobs, no sleeps used to sync. Every wait is on a line or on the fake's state, with a deadline that only bounds a failure.Fake changes (only additions):
Server.Disconnect(revokes the agent's tokens and clears its secret, so a read gets 401 and a mint getsinvalid_client), andAgentClient.TokenLifetime(tokens that say they last as little as a second, so a renewal comes due during a test). Both have tests in the fake's own package.Harness changes:
Profile/SetupProfile/ConnectProfileto connect a second agent,AnotherComputer(a second home against the same Basecamp), andConnector.Pid.Tests
connector: shutting downwithsignal=interrupt, exits 130, and logs no ERROR.not_addressedfor the other agent's mention.invalid_clienttrigger): the world's tokens last one second. Another home runsauth agent connect, which rotates the secret, as Basecamp does, and leaves this computer's token alive. When the token comes due, the next mention's read renews it, the renewal getsinvalid_client, and the connector exits 3. The one wait on the clock is the token's own expiry, whichauth status --jsonreports. That expiry is the premise of the scenario, not a guess about timing.Retry-After: 1, once, with the stored token due): it logsconnector: could not get the agent's token yet, comes up, and hands off a mention.poll) and gives exactly one request.StopPings): the connector reconnects and hands off a later live mention. This runs once, because staleness is 7.5s and nothing can make it shorter.admittedevent line, one request, and three reads of the comment.connect --hold, mention, wait for itsadmittedevent line (no request), kill -9,basecamp connect release -P agent, restart. The request for that event_id is printed once (within the grace period).DefaultBacklogWarn + admission.DefaultWorkersevents are emitted live (each worker holds one event at its read). The warn line arrives withwarn_at=1000. The reads are released and the recover line arrives.The 11 scenarios and the 5 from #823 run in parallel in about 13s (the stale cable sets the floor).
go test -race -count=10 ./e2e/connectpassed in 154s, andgo test -count=20 ./e2e/connectpassed in 190s, with no failures.bin/ciis green.Summary by cubic
Fixes the connector running on after its agent is disconnected in Basecamp or connected on another computer, and tests how
basecamp connectfails and recovers, end to end.The fix
read_failedand nothing said why.disconnected.The tests
Written for commit 93b287a. Summary will update on new commits.