amadan

carlos / platform Public

console: apischedule.go — surface cap-check read errors, dedupe 502 text

handleAPIScheduleSet's cap pre-check silently skipped past ANY read
error, not just a genuine not-found (ReadDeclaration already folds
that case into an empty, nil-error Declaration) — so a real bucket
failure fell through to UpsertSchedule's own re-read instead of being
answered by this door. Check the error explicitly.

Comment why handleAPIScheduleRemove discards RemoveSchedule's
`present`: declaredSchedule already confirmed the name moments
earlier, and RemoveSchedule is idempotent by design.

Dedupe the Set/Remove 502 sentences (identical but for one verb) into
scheduleWriteFailed(verb).

Test: TestAPIScheduleRemoveDropsAndAnnounces now also pins that the
delete response's `instances` array is [] (non-nil), matching the
existing check on `schedules`.
Paul Campbell pushed by paul@keymail.dev 68dbd51302e48704479144b60d5e0a47d8a3e59b parent b986e59
2 files changed, +37 −8
  • internal/console/apischedule.go +34 −8
  • internal/console/apischedule_test.go +3 −0
diff --git a/internal/console/apischedule.go b/internal/console/apischedule.go
index 4a7ee8cd0..ae63a84f5 100644
--- a/internal/console/apischedule.go
+++ b/internal/console/apischedule.go
@@ -65,6 +65,14 @@ func scheduleAnswer(acct, app string, d schedule.Declaration, insts []schedule.S
return out
}
+// scheduleWriteFailed is the 502 sentence handleAPIScheduleSet and
+// handleAPIScheduleRemove both give a bucket write failure — identical
+// shape, one verb apart — so a member reads the same "nothing changed" out
+// of either, and the wording lives in one place to say so.
+func scheduleWriteFailed(verb string) string {
+ return "could not " + verb + " the schedule — nothing changed; try again"
+}
+
// apiScheduleApp is the shared gate of all four doors, in one place so the
// ORDER cannot drift between them:
//
@@ -213,17 +221,29 @@ func (s *Server) handleAPIScheduleSet(w http.ResponseWriter, r *http.Request) {
}
ctx, cancel := context.WithTimeout(r.Context(), 15*time.Second)
defer cancel()
- if cur, _, err := schedule.ReadDeclaration(ctx, s.cfg.Releases, acct.Sqid, app); err == nil {
- if _, replacing := cur.Find(sc.Name); !replacing && len(cur.Schedules) >= schedule.MaxSchedulesPerApp {
- jsonError(w, http.StatusBadRequest, fmt.Sprintf(
- "an app may declare at most %d schedules — remove one first", schedule.MaxSchedulesPerApp))
- return
- }
+ // ReadDeclaration already folds a genuine not-found into an empty,
+ // nil-error Declaration (spec §4.1: absence is legal forever) — so a
+ // non-nil err here is a real read failure (bucket unreachable, bad
+ // JSON), not "this app has no schedules yet". Silently skipping the
+ // cap check in that case would let a request past the door's own
+ // answer and rely entirely on UpsertSchedule's CAS-guarded re-check,
+ // which is there for the GUARANTEE, not to cover for a 502 this door
+ // should have surfaced itself.
+ cur, _, err := schedule.ReadDeclaration(ctx, s.cfg.Releases, acct.Sqid, app)
+ if err != nil {
+ log.Printf("console: api schedule set %s/%s: %v", acct.Sqid, app, err)
+ jsonError(w, http.StatusBadGateway, scheduleWriteFailed("record"))
+ return
+ }
+ if _, replacing := cur.Find(sc.Name); !replacing && len(cur.Schedules) >= schedule.MaxSchedulesPerApp {
+ jsonError(w, http.StatusBadRequest, fmt.Sprintf(
+ "an app may declare at most %d schedules — remove one first", schedule.MaxSchedulesPerApp))
+ return
}
d, err := schedule.UpsertSchedule(ctx, s.cfg.Releases, acct.Sqid, app, sc, time.Now())
if err != nil {
log.Printf("console: api schedule set %s/%s %s: %v", acct.Sqid, app, sc.Name, err)
- jsonError(w, http.StatusBadGateway, "could not record the schedule — nothing changed; try again")
+ jsonError(w, http.StatusBadGateway, scheduleWriteFailed("record"))
return
}
s.audit(acct, app, addr, "schedule set", "recorded schedule "+sc.Name+" (cli)")
@@ -251,10 +271,16 @@ func (s *Server) handleAPIScheduleRemove(w http.ResponseWriter, r *http.Request)
if !ok {
return
}
+ // present (RemoveSchedule's second return) is ignored: declaredSchedule
+ // just confirmed sc.Name was declared, and RemoveSchedule is idempotent
+ // by design (its own doc comment — removing an absent name still
+ // rewrites updated_at). A concurrent remove landing between that read
+ // and this call is the same successful outcome either way, not
+ // something for this door to distinguish.
d, _, err := schedule.RemoveSchedule(ctx, s.cfg.Releases, acct.Sqid, app, sc.Name, time.Now())
if err != nil {
log.Printf("console: api schedule rm %s/%s %s: %v", acct.Sqid, app, sc.Name, err)
- jsonError(w, http.StatusBadGateway, "could not remove the schedule — nothing changed; try again")
+ jsonError(w, http.StatusBadGateway, scheduleWriteFailed("remove"))
return
}
s.audit(acct, app, addr, "schedule rm", "removed schedule "+sc.Name+" (cli)")
diff --git a/internal/console/apischedule_test.go b/internal/console/apischedule_test.go
index 0ef43eb3e..003d9d443 100644
--- a/internal/console/apischedule_test.go
+++ b/internal/console/apischedule_test.go
@@ -254,6 +254,9 @@ func TestAPIScheduleRemoveDropsAndAnnounces(t *testing.T) {
if res.Schedules == nil {
t.Error("delete body schedules is null; the CLI ranges over it")
}
+ if res.Instances == nil {
+ t.Error("delete body instances is null; the CLI ranges over it")
+ }
d, _ := declaredSchedules(t, fx, "titogo")
if _, ok := d.Find("sync"); ok {
t.Error("sync is still in the bucket declaration after a 200")