Resolve builds a dead runner abandoned, and forward bare-redirect stdin !124

merged merged by cmc on 2026-08-30 18:11 UTC · krz/gitbay:reap-stale-builds into main

5 files changed, +188 −7

Layout: unified · split

cmd/gitbay/coverage_test.go +36

	@@ -55,3 +55,39 @@ func TestEveryCommandIsReachable(t *testing.T) {
55	}	55	}
56	}	56	}
57	}	57	}
		58
		59	// Commands whose payload is stdin with no flag naming it: `gitbay keys add <
		60	// key.pub`, not `--file -`. They must be wired alwaysStdin, because the
		61	// stdinOK path only forwards stdin when it sees `--file -` in the arguments —
		62	// so a bare redirect reached the server as an empty body and the command
		63	// failed on input the SSH API accepts.
		64	var bareStdin = []string{
		65	"keys add",
		66	"repo deploy-key add",
		67	"repo secret set",
		68	"release asset add",
		69	}
		70
		71	func TestBareRedirectCommandsAlwaysForwardStdin(t *testing.T) {
		72	modes := map[string]string{}
		73	var walk func(*cobra.Command)
		74	walk = func(c *cobra.Command) {
		75	if p := c.Annotations[serverPath]; p != "" {
		76	modes[p] = c.Annotations[stdinMode]
		77	}
		78	for _, sub := range c.Commands() {
		79	walk(sub)
		80	}
		81	}
		82	walk(newRoot())
		83
		84	for _, path := range bareStdin {
		85	switch modes[path] {
		86	case "always":
		87	case "":
		88	t.Errorf("%q is not wired in the CLI", path)
		89	default:
		90	t.Errorf("%q is wired %s; a bare redirect needs alwaysStdin", path, modes[path])
		91	}
		92	}
		93	}

cmd/gitbay/main.go +24 −7

 // serverPath is the annotation key holding a passthrough command's
 // server-side path, so the tree can be checked against the registry.
 const serverPath = "gitbay.server_path"
+const stdinMode = "gitbay.stdin_mode"
 // passOpts describes how one CLI command maps onto the server command.
 type passOpts struct {
 	server      []string // server-side command path
 	needsRepo   bool     // prepend inferred owner/name unless given
-	stdinOK     bool     // wire local stdin through (keys add, --file -)
+	stdinOK     bool     // wire local stdin through when --file - asks for it
-	alwaysStdin bool     // stdin is the payload (release asset add)
+	alwaysStdin bool     // stdin is the payload, named by no flag: a bare redirect
 	editor      string   // open $EDITOR for a body when none given
 }
 // pass builds a passthrough command. Flags are parsed by the server, which
 // is the single source of truth for them; the CLI stays thin.
+// stdinModeName reports how this command takes stdin, so the coverage test can
+// check that a command reading a bare redirect is not left waiting for a
+// `--file -` that its callers never type.
+func (o passOpts) stdinModeName() string {
+	switch {
+	case o.alwaysStdin:
+		return "always"
+	case o.stdinOK:
+		return "flag"
+	}
+	return "none"
+}
 func pass(use, short string, o passOpts) *cobra.Command {
 	return &cobra.Command{
-		Use:                use,
+		Use:   use,
-		Short:              short,
+		Short: short,
-		Annotations:        map[string]string{serverPath: strings.Join(o.server, " ")},
+		Annotations: map[string]string{
+			serverPath: strings.Join(o.server, " "),
+			stdinMode:  o.stdinModeName(),
+		},
 		DisableFlagParsing: true,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			// cobra still owns `forge <cmd> --help`.
 func authCmd() *cobra.Command {
 	keysAdd := pass("add", "register an SSH public key (reads the key from stdin or --file -)",
-		passOpts{server: []string{"keys", "add"}, stdinOK: true})
+		passOpts{server: []string{"keys", "add"}, alwaysStdin: true})
 	// keys add always reads stdin on the server; wire it through directly.
 	keysAdd.RunE = func(cmd *cobra.Command, args []string) error {
 		t, err := resolveTarget()
 		pass("import-issues", "import GitHub issue/PR history: --from <ghowner/ghrepo> [--token-stdin]",
 			passOpts{server: []string{"repo", "import-issues"}, needsRepo: true, stdinOK: true}),
 		group("deploy-key", "repository-bound CI keys",
-			pass("add", "bind a key: [--rw] < key.pub", passOpts{server: []string{"repo", "deploy-key", "add"}, needsRepo: true, stdinOK: true}),
+			pass("add", "bind a key: [--rw] < key.pub", passOpts{server: []string{"repo", "deploy-key", "add"}, needsRepo: true, alwaysStdin: true}),
 			pass("list", "list deploy keys", passOpts{server: []string{"repo", "deploy-key", "list"}, needsRepo: true}),
 			pass("remove", "remove a deploy key: <fingerprint>", passOpts{server: []string{"repo", "deploy-key", "remove"}, needsRepo: true}),
 		),

internal/control/build.go +15

	@@ -301,6 +301,21 @@ func runRunnerNext(c *Ctx, args []string) int {
301	if code := requireRunner(c); code >= 0 {	301	if code := requireRunner(c); code >= 0 {
302	return code	302	return code
303	}	303	}
		304	// Resolve anything a previous runner claimed and never reported, so a
		305	// killed runner does not leave a build running and a commit pending forever.
		306	if stale, err := c.Store.ReapStaleBuilds(); err != nil {
		307	return c.fail(protocol.ExitFailure, "%v", err)
		308	} else {
		309	for _, sb := range stale {
		310	repo, err := c.Store.RepoByID(sb.RepoID)
		311	if err != nil {
		312	continue
		313	}
		314	url := fmt.Sprintf("%s/%s/builds/%d", c.Cfg.Server.SiteURL, repo.Path(), sb.Number)
		315	c.Store.SetCommitStatus(repo.ID, sb.SHA, "ci/"+sb.Job, "failure",
		316	"build abandoned", url, c.User.ID)
		317	}
		318	}
304	b, ok, err := c.Store.ClaimBuild()	319	b, ok, err := c.Store.ClaimBuild()
305	if err != nil {	320	if err != nil {
306	return c.fail(protocol.ExitFailure, "%v", err)	321	return c.fail(protocol.ExitFailure, "%v", err)

internal/store/builds.go +42

	@@ -3,6 +3,7 @@ package store
3	import (	3	import (
4	"database/sql"	4	"database/sql"
5	"errors"	5	"errors"
		6	"time"
6	)	7	)
7		8
8	// Build is one CI job execution for one commit.	9	// Build is one CI job execution for one commit.
	@@ -83,6 +84,47 @@ func (s *Store) ClaimBuild() (Build, bool, error) {
83	return b, true, tx.Commit()	84	return b, true, tx.Commit()
84	}	85	}
85		86
		87	// StaleBuildDeadline is how long a claimed build may stay running before the
		88	// server gives up on it. Comfortably longer than the runner's own -timeout
		89	// (45m by default), so this only fires when the runner never reported at all —
		90	// it was killed, restarted, or lost the network mid-build.
		91	const StaleBuildDeadline = 90 * time.Minute
		92
		93	// ReapStaleBuilds fails every build that has been running past the deadline and
		94	// returns them, so the caller can resolve their commit statuses. A runner that
		95	// dies between claiming a build and reporting it otherwise leaves the row
		96	// claimed forever, and the commit pending forever with it.
		97	func (s *Store) ReapStaleBuilds() ([]Build, error) {
		98	cutoff := time.Now().UTC().Add(-StaleBuildDeadline).Format("2006-01-02T15:04:05Z")
		99	rows, err := s.DB.Query(buildSelect+
		100	" WHERE status = 'running' AND started_at != '' AND started_at < ?", cutoff)
		101	if err != nil {
		102	return nil, err
		103	}
		104	defer rows.Close()
		105	var stale []Build
		106	for rows.Next() {
		107	b, err := scanBuild(rows)
		108	if err != nil {
		109	return nil, err
		110	}
		111	stale = append(stale, b)
		112	}
		113	if err := rows.Err(); err != nil {
		114	return nil, err
		115	}
		116	for _, b := range stale {
		117	if err := s.AppendBuildLog(b.ID, []byte(
		118	"\nbuild abandoned: the runner never reported an outcome\n")); err != nil {
		119	return nil, err
		120	}
		121	if err := s.FinishBuild(b.ID, "failure"); err != nil {
		122	return nil, err
		123	}
		124	}
		125	return stale, nil
		126	}
		127
86	// AppendBuildLog adds a chunk to the build's log, dropping bytes past the cap.	128	// AppendBuildLog adds a chunk to the build's log, dropping bytes past the cap.
87	func (s *Store) AppendBuildLog(id int64, chunk []byte) error {	129	func (s *Store) AppendBuildLog(id int64, chunk []byte) error {
88	_, err := s.DB.Exec(`	130	_, err := s.DB.Exec(`

internal/store/builds_test.go added +71

@@ -0,0 +1,71 @@
	1	package store
	2
	3	import (
	4	"strings"
	5	"testing"
	6	)
	7
	8	// A runner that dies between claiming a build and reporting it leaves the row
	9	// claimed. The next claim resolves it rather than leaving the build running and
	10	// the commit pending forever.
	11	func TestReapStaleBuilds(t *testing.T) {
	12	s := open(t)
	13	if err := s.MigrateUp(); err != nil {
	14	t.Fatal(err)
	15	}
	16	uid, err := s.CreateUser("cmc", true)
	17	if err != nil {
	18	t.Fatal(err)
	19	}
	20	if _, err := s.CreateRepo("user", uid, "orgo", "public"); err != nil {
	21	t.Fatal(err)
	22	}
	23
	24	stuck, err := s.CreateBuild(1, "test", "abc123", "main", `["true"]`)
	25	if err != nil {
	26	t.Fatal(err)
	27	}
	28	fresh, err := s.CreateBuild(1, "pages", "abc123", "main", `["true"]`)
	29	if err != nil {
	30	t.Fatal(err)
	31	}
	32
	33	// Claim both, then age only the first past the deadline.
	34	for range 2 {
	35	if _, ok, err := s.ClaimBuild(); err != nil \|\| !ok {
	36	t.Fatalf("claim: %v ok=%v", err, ok)
	37	}
	38	}
	39	if _, err := s.DB.Exec(
	40	`UPDATE builds SET started_at = '2020-01-01T00:00:00Z' WHERE number = ?`, stuck); err != nil {
	41	t.Fatal(err)
	42	}
	43
	44	reaped, err := s.ReapStaleBuilds()
	45	if err != nil {
	46	t.Fatal(err)
	47	}
	48	if len(reaped) != 1 \|\| reaped[0].Number != stuck {
	49	t.Fatalf("reaped %+v, want only build %d", reaped, stuck)
	50	}
	51
	52	b, err := s.BuildByNumber(1, stuck)
	53	if err != nil {
	54	t.Fatal(err)
	55	}
	56	if b.Status != "failure" \|\| b.FinishedAt == "" {
	57	t.Fatalf("stale build is %s finished %q, want failure with a timestamp", b.Status, b.FinishedAt)
	58	}
	59	log, err := s.BuildLog(b.ID)
	60	if err != nil {
	61	t.Fatal(err)
	62	}
	63	if !strings.Contains(string(log), "abandoned") {
	64	t.Fatalf("log does not say why it failed: %q", log)
	65	}
	66
	67	// A build still inside the deadline is left alone.
	68	if b, err := s.BuildByNumber(1, fresh); err != nil \|\| b.Status != "running" {
	69	t.Fatalf("fresh build is %v (%v), want running", b.Status, err)
	70	}
	71	}