amadan
schedule: fix review findings — package-doc leak, curl-heredoc test, zero-route strength
Important: spec.go/declaration.go/status.go's new file-level doc
comments sat directly above `package schedule` with no blank line, so
go doc treats any such comment as a package-doc contribution — it
doesn't distinguish "file-level" from "package-level" by content.
`go doc ./internal/schedule` was concatenating all four files' blurbs
(cron.go's real package doc plus three unrelated ones) into one
incoherent paragraph. Added the blank line each needed; verified
before/after with `go doc ./internal/schedule` — only cron.go's
comment (part of the same unbroken block as the canonical `// Package
schedule is …` line) still appears.
Minor: TestControlPutRejectsTrailingJSON only exercised the reject
side of dec.More(); added the accept side — a body ending in a
newline, or with leading/trailing whitespace (the curl-heredoc shape
the brief was worried about) — so the "must not reject" direction is
pinned, not just correct by inspection.
Minor: TestConvergeZeroRouteRegistryIsANoop tested a wholly empty
registry, where "the routeless app is skipped" was trivially true
because nothing converged at all. Replaced with
TestConvergeZeroRouteAppDoesNotBlockASiblingApp: a declared app with
zero routes ("ghost") alongside a declared sibling app that has a
real route (via convergeFixture's "other"), asserting the sibling
still converges normally in the same pass.
Scope: touched only internal/schedule/{spec,declaration,status,
control_test,converge_test}.go, per the coordinator's file list —
did not touch runner.go, registry, or any internal/activator|host
file two other agents are concurrently committing in.
5 files changed,
+42
−14
internal/schedule/control_test.go+14 −0internal/schedule/converge_test.go+25 −14internal/schedule/declaration.go+1 −0internal/schedule/spec.go+1 −0internal/schedule/status.go+1 −0
diff --git a/internal/schedule/control_test.go b/internal/schedule/control_test.go| index 1c97afd3b..23909e324 100644 |
| --- a/internal/schedule/control_test.go |
| +++ b/internal/schedule/control_test.go |
| @@ -129,6 +129,20 @@ func TestControlPutRejectsTrailingJSON(t *testing.T) { |
| if rec := do(mux, "PUT", "/schedule/clean", "Bearer tok", `{"at":"2026-09-01T08:00:00Z","path":"/p"}`); rec.Code != http.StatusOK { |
| t.Fatalf("clean body → %d (%s)", rec.Code, rec.Body) |
| } |
| + // dec.More() must not trip on WHITESPACE after the object — a curl |
| + // heredoc (`curl -d @- <<EOF`) appends exactly this, a trailing |
| + // newline, and sometimes leading/trailing spaces too. Only a second |
| + // TOKEN counts as trailing data. |
| + for i, body := range []string{ |
| + "{\"at\":\"2026-09-01T08:00:00Z\",\"path\":\"/p\"}\n", |
| + "{\"at\":\"2026-09-01T08:00:00Z\",\"path\":\"/p\"} \n \t", |
| + " {\"at\":\"2026-09-01T08:00:00Z\",\"path\":\"/p\"} ", |
| + } { |
| + name := "clean-ws-" + itoa(int64(i)) |
| + if rec := do(mux, "PUT", "/schedule/"+name, "Bearer tok", body); rec.Code != http.StatusOK { |
| + t.Errorf("PUT with trailing/leading whitespace %q → %d, want 200 (%s)", body, rec.Code, rec.Body) |
| + } |
| + } |
| } |
| // A member's clock is not UTC; `at` must accept an offset and the row must |
diff --git a/internal/schedule/converge_test.go b/internal/schedule/converge_test.go| index d3ee5093e..e451c2273 100644 |
| --- a/internal/schedule/converge_test.go |
| +++ b/internal/schedule/converge_test.go |
| @@ -480,26 +480,37 @@ func TestConvergeRunSignalGoingAbsentIsNotAFire(t *testing.T) { |
| } |
| } |
| -// A registry with zero routes at all (nothing adopted yet, or every route |
| -// gone) must not be distinguished from "an app with no instance routes": |
| -// byApp never gains an entry, convergeApp is never called, and Pass must |
| -// still return cleanly rather than erroring on an empty registry. |
| -func TestConvergeZeroRouteRegistryIsANoop(t *testing.T) { |
| - reg, err := registry.Open(filepath.Join(t.TempDir(), "r.db")) |
| - if err != nil { |
| - t.Fatal(err) |
| - } |
| - t.Cleanup(func() { reg.Close() }) |
| - st := release.NewDirStore(t.TempDir()) |
| +// A declaration can exist for an app the registry has never heard of |
| +// (nothing shipped there yet, or every instance since removed) — byApp |
| +// never gains an entry for it, convergeApp is never called for it, and |
| +// Pass must return cleanly rather than erroring. The weak version of this |
| +// test is a wholly empty registry, where that's true trivially because |
| +// NOTHING converges; the real claim is that a routeless app is skipped |
| +// while a SIBLING app with live routes still converges normally in the |
| +// same pass, so this seeds both: "ghost" (declared, zero routes) beside |
| +// "other" (declared here, and the fixture already gives it a real route). |
| +func TestConvergeZeroRouteAppDoesNotBlockASiblingApp(t *testing.T) { |
| + reg, st := convergeFixture(t) |
| ctx := context.Background() |
| now := at("2026-08-23T13:07:00Z") |
| - // A declaration exists for an app the registry has never heard of. |
| - UpsertSchedule(ctx, st, "bab", "app", Schedule{Name: "sync", Every: "6h", Path: "/jobs/sync"}, now) |
| + UpsertSchedule(ctx, st, "bab", "ghost", Schedule{Name: "sync", Every: "6h", Path: "/jobs/sync"}, now) |
| + UpsertSchedule(ctx, st, "bab", "other", Schedule{Name: "sync", Every: "6h", Path: "/jobs/sync"}, now) |
| c := NewConverger(reg, st, "box1", nil) |
| c.Now = func() time.Time { return now } |
| if err := c.Pass(ctx); err != nil { |
| - t.Fatalf("Pass on a route-less registry: %v", err) |
| + t.Fatalf("Pass with one routeless app declared alongside a routed sibling: %v", err) |
| + } |
| + rows, err := reg.TimersFor("other.bab.oncarlos.com") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(rows) != 1 || rows[0].Name != "sync" || rows[0].Kind != KindSchedule { |
| + t.Fatalf("the routed sibling app (other) did not converge: %+v", rows) |
| } |
| + // "ghost" has no host anywhere to hold a row, so there is nothing to |
| + // query for it directly — Pass returning cleanly with "other" actually |
| + // converged above IS the assertion that byApp skipped "ghost" without |
| + // that skip taking its sibling down with it. |
| } |
| // A declaration whose cron never matches (written past Validate — an older |
diff --git a/internal/schedule/declaration.go b/internal/schedule/declaration.go| index f427704ec..50e356dc5 100644 |
| --- a/internal/schedule/declaration.go |
| +++ b/internal/schedule/declaration.go |
| @@ -3,6 +3,7 @@ |
| // round trip nothing; the CAS-guarded read/modify/write cycle with one |
| // retry on a conflicting writer; and schedule-run.json (spec §4.2), whose |
| // etag movement alone — never its content — is the "fire it now" signal. |
| + |
| package schedule |
| import ( |
diff --git a/internal/schedule/spec.go b/internal/schedule/spec.go| index 36bb981ac..b6aa78938 100644 |
| --- a/internal/schedule/spec.go |
| +++ b/internal/schedule/spec.go |
| @@ -3,6 +3,7 @@ |
| // one's next fire from now (spec §6.1's coalescing — always from now, never |
| // from the scheduled time), and render the canonical text back for |
| // Converge and Runner to store and for `carlos schedule ls` to show. |
| + |
| package schedule |
| import ( |
diff --git a/internal/schedule/status.go b/internal/schedule/status.go| index 2919eceb9..b70b352a3 100644 |
| --- a/internal/schedule/status.go |
| +++ b/internal/schedule/status.go |
| @@ -4,6 +4,7 @@ |
| // that changed a host's rows. It is what makes `carlos schedule ls` honest |
| // for a member with no box access — the declaration alone says what was |
| // asked; these objects say what is actually going to happen. |
| + |
| package schedule |
| import ( |