Repository navigation
Conversation
HandoffOptions gets Grace: how long before the run started a request may have been made and still be handed off. Zero means HandoffGrace, the minute it always was. runConnect sets it from BASECAMP_CONNECT_HANDOFF_GRACE, a Go duration, read only in a dev build (connect_handoff_grace_dev.go). The !dev stub reads no environment, so a release build cannot be affected. A value that is not a positive duration is a usage error, and a run with the override says so on stderr. The handoff now logs a before_this_run close after the ledger write, not before it: a run stopped between the two left the record admitted while its log said it was closed. e2e/connect/grace_test.go holds the crash between admission and handoff past the grace: --hold, a mention, its event line, kill -9, connect release, then a restart once the request is older than a 2s grace. No request line is written, the record is closed as before_this_run, and status shows nothing left to hand off. The fake stamps the request 10s in the past instead of the test waiting: the handoff judges only the event's created_at against the run's start. The test skips, saying why, in a build without -tags dev; make test and CI always pass it.
github-actions
Bot
added
commands
There was a problem hiding this comment.
Copilot review overview
🟢 Approved
The implementation preserves release defaults and provides focused unit and end-to-end coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Adds a dev-only handoff-grace override to enable fast restart testing while preserving release behavior.
Changes:
- Makes handoff grace configurable, retaining the one-minute default.
- Adds dev-only environment parsing and override logging.
- Tests grace boundaries and crash/restart behavior end to end.
[!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}
There was a problem hiding this comment.
2 issues found across 7 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_handoff_grace_dev.go">
<violation number="1" location="internal/commands/connect_handoff_grace_dev.go:33">
P2: An explicit zero override is rejected, despite zero being the documented way to keep the one-minute default. Reject only negative durations so `0s` can reach the handoff's default behavior.</violation>
</file>
<file name="internal/commands/connect_run.go">
<violation number="1" location="internal/commands/connect_run.go:192">
P3: Shadow runs never invoke `RunHandoff`, so this warning claims an override is active when it has no effect. Suppress it for shadow runs.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if err != nil { | ||
| return 0, fmt.Errorf("%s is not a duration: %w", connectHandoffGraceEnv, err) | ||
| } | ||
| if d <= 0 { |
There was a problem hiding this comment.
P2: An explicit zero override is rejected, despite zero being the documented way to keep the one-minute default. Reject only negative durations so 0s can reach the handoff's default 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/commands/connect_handoff_grace_dev.go, line 33:
<comment>An explicit zero override is rejected, despite zero being the documented way to keep the one-minute default. Reject only negative durations so `0s` can reach the handoff's default behavior.</comment>
<file context>
@@ -0,0 +1,37 @@
+ if err != nil {
+ return 0, fmt.Errorf("%s is not a duration: %w", connectHandoffGraceEnv, err)
+ }
+ if d <= 0 {
+ return 0, fmt.Errorf("%s must be a positive duration, not %s", connectHandoffGraceEnv, v)
+ }
</file context>
| if d <= 0 { | |
| if d < 0 { |
| } | ||
|
|
||
| logger := slog.New(slog.NewTextHandler(cmd.ErrOrStderr(), nil)) | ||
| if grace > 0 { |
There was a problem hiding this comment.
P3: Shadow runs never invoke RunHandoff, so this warning claims an override is active when it has no effect. Suppress it for shadow runs.
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_run.go, line 192:
<comment>Shadow runs never invoke `RunHandoff`, so this warning claims an override is active when it has no effect. Suppress it for shadow runs.</comment>
<file context>
@@ -184,6 +189,9 @@ func runConnect(cmd *cobra.Command, f *connectRunFlags) error {
}
logger := slog.New(slog.NewTextHandler(cmd.ErrOrStderr(), nil))
+ if grace > 0 {
+ logger.Warn("connector: the handoff grace is overridden for this dev build", "grace", grace, "default", connector.HandoffGrace)
+ }
</file context>
| if grace > 0 { | |
| if grace > 0 && !f.shadow { |
|
🤖 Lint is red only because of #822's G120 finding. I checked locally: with that fixed and #822, #823 and this PR rebased onto main, lint is clean, and the |
PR 5 of a stack of 8 that tests
basecamp connectend to end. Stacked on #823 (the e2e harness), which is stacked on #822 (the fake Basecamp). It changes no behaviour of a release build.Why
The connector hands off a request only if it was made at most
HandoffGrace(one minute) before the run started. A request older than that is closed asbefore_this_run: nobody was there to take it, and it is not run late.That matters most after a crash between admission and handoff. The connector admits a request, dies before it writes the request line, and starts again. Within the grace, the request is handed off (PR 3 tests that, with the default grace). After the grace, it must be closed and never written. Nothing tested that second half end to end, because a test would have to wait out a minute.
What
HandoffOptions.Grace(internal/connector/handoff.go). Zero meansHandoffGrace, so every caller that does not set it keeps the minute. The handoff reads it through a smallgrace()method, as it reads its logger.connect_handoff_grace_dev.go(//go:build dev) readsBASECAMP_CONNECT_HANDOFF_GRACEas a Go duration.connect_handoff_grace_stub.go(//go:build !dev) reads no environment at all and returns zero, so a release build (goreleaser builds without-tags dev) cannot be affected. This is the same pattern asdev_tag.go/dev_tag_stub.go. There is no hidden flag.runConnectreads it before it touches the account. A value that is not a positive duration is a usage error. A run with the override logsconnector: the handoff grace is overridden for this dev buildon stderr, with the grace and the default.a request from before this run started is not handed offbefore the ledger write. A run stopped between the two left the record admitted while its log said it was closed. The e2e test found this. The log now comes after the write.Tests
e2e/connect/grace_test.go(a new file; no harness changes). With a 2s grace:connect --hold, a trusted mention, wait for itsadmittedevent line,kill -9,basecamp connect release -P agent, then a restart. On the restart there is no request line, the connector logs the close for that event id, andconnect status --jsonshows no admitted or queued record.WithClock) stamps the request 10s in the past instead of the test waiting out the grace. To the connector it is the same request: the handoff judges only the event'screated_at, as the feed served it, against when the run started, and nothing else in intake or admission readscreated_at. Before the restart, the test checks from the pointer line'screated_atthat the request is already older than the grace.-tags devthe test skips and says why: a release build reads no environment for the grace.make test,bin/ci, the unit test job, the race shards and the release workflow all rungo test -tags dev, so it always runs there.handoff_test.go: a grace of 30s hands off a request made 30s before the run (the boundary included), a grace of 29s closes it asbefore_this_run, and no grace meansHandoffGrace.connect_handoff_grace_dev_test.go(dev-tagged): the parse accepts Go durations and empty (zero), refuses2,soon,0sand-1swith the variable's name in the error, and reads the environment.go test -tags dev -race -count=10passes on the new tests.bin/ciis green.Summary by cubic
Lets dev builds shorten the connector's handoff grace so an end-to-end test can reach a crash-after-grace restart in seconds; release builds read no environment for the grace and keep the one-minute default.
HandoffOptions.Grace; zero means the existingHandoffGrace, and only a dev build sets it fromBASECAMP_CONNECT_HANDOFF_GRACE, with invalid values a usage error.before_this_runlog after the ledger write, so a stopped run no longer logs a close it has not yet written.Written for commit 0045407. Summary will update on new commits.