amadan

carlos / platform Public

objstore: the consent chain answers with a name, not a bool

Task 11's precondition (spec §5.1.1). The enqueue door and the drain
have to refuse on the same six conditions, and they draw DIFFERENT
conclusions from the same answer: the door turns it into an HTTP state,
the drain turns it into keep-or-delete. A bool cannot carry that, so
each caller was left re-deriving the reason from the record it was
handed back — which is the exact shape in which two callers start
disagreeing, and not disagreeing is the entire point of §5.1.1.

The old code separated the one REVERSIBLE refusal from the permanent
ones by returning it in the ERROR slot: `disabled` came back as
ErrScanningDisabled, so the caller's `if err != nil` caught it and the
deletion branch below never saw it. That is correct, and it is correct
by accident. Any refusal added later that happened to return an error
would have been silently spared deletion; any that did not would have
been silently deleted on. Nothing in the code said which was intended,
because the distinction lived in a return slot rather than in a word.

Now: RefusalNotGranted, RefusalTrashed, RefusalNotRequested,
RefusalEncrypted, RefusalNotEnabled, RefusalDisabled, and RefusalNone
for yes. Permanent() is §7.5's delete-vs-keep split in the one place
both callers read it from, written as an explicit list rather than
`r != RefusalDisabled` — so a refusal added later is permanent only
because somebody said so, and the default for a new name is "keep the
item", which is the survivable mistake.

A READ ERROR STILL HAS NO NAME. Nothing about consent was learned, and
§7.5's "deletion is driven by absence, never by error" depends on those
two staying apart: give a bad bucket moment a name and something will
eventually delete on it. The test asserts the empty refusal alongside
the error, not just the error.

The second call site — the purge re-read before writing a result object
— now refuses on ANY refusal rather than on permanence. The question
there is not "will this item ever be useful" but "may this app be
scanned right now", and a result written after the answer turned to no
is a verdict no door may answer from.

Three mutations, each a plausible way to write it, each caught:

  Permanent() = r != RefusalDisabled  -> RefusalNone becomes permanent
                                         and the drain deletes every item
                                         it was allowed to scan
  read error named not_granted        -> TestQueuePermittedReportsAReadErrorAsAnError
  encrypted read as not_requested     -> TestQueueRefusalNamesEachLinkAndItsConsequence

The two locally-failing packages are internal/activator and
internal/edge, both `listen unix ...: socket: operation not permitted` —
the sandbox refuses unix socket binds. Named rather than counted, per
what that habit cost this branch earlier tonight. CI passes both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PEkJh1ESqiA84quAHJV4K6
Paul Campbell pushed by paul@keymail.dev 2d4d0abf80e8e0b9fdf59f29813426985ed12403 parent 25f2784
2 files changed, +254 −16
  • internal/objstore/queue.go +90 −16
  • internal/objstore/queue_test.go +164 −0
diff --git a/internal/objstore/queue.go b/internal/objstore/queue.go
index e696b5b4e..17674eb8d 100644
--- a/internal/objstore/queue.go
+++ b/internal/objstore/queue.go
@@ -273,11 +273,20 @@ func DrainApp(ctx context.Context, s release.Store, sc Scanner, idx *QueueIndex,
// The consent chain, re-checked here and not only at the door: the record
// can change between enqueue and drain, and THIS is where bytes are read.
- rec, permitted, err := queuePermitted(ctx, s, acct, app)
+ rec, refusal, err := queuePermitted(ctx, s, acct, app)
if err != nil {
return nil, false, err
}
- if !permitted {
+ // The reversible refusal, kept apart from the permanent ones BY NAME now
+ // rather than by which return slot it arrived in. It used to come back as
+ // an error, so the `err != nil` line above caught it and the deletion
+ // below never saw it — correct, and correct by accident: any later
+ // refusal that returned an error would have been silently spared, and any
+ // that did not would have been silently deleted on.
+ if refusal == RefusalDisabled {
+ return nil, false, ErrScanningDisabled
+ }
+ if refusal.Permanent() {
// Not a retry — none of these can change back in a way that makes the
// items useful. Drop them and say so.
for _, k := range keys {
@@ -464,7 +473,11 @@ func drainOne(ctx context.Context, s release.Store, sc Scanner, acct, app, bucke
// The verdict could not be written to the object. Re-read the record
// first: the writeSidecar race applies, and a result object stranded under
// a purged prefix is found by nothing and resurrects the app.
- if _, permitted, err := queuePermitted(ctx, s, acct, app); err != nil || !permitted {
+ // ANY refusal stops the write, permanent or not: the question here is not
+ // "will this item ever be useful" but "may this app be scanned right now",
+ // and a result object written after the answer turned to no is a verdict
+ // no door may answer from.
+ if _, refusal, err := queuePermitted(ctx, s, acct, app); err != nil || refusal != RefusalNone {
return out, err
}
if err := WriteScanResult(ctx, s, acct, app, &ScanResult{
@@ -478,35 +491,96 @@ func drainOne(ctx context.Context, s release.Store, sc Scanner, acct, app, bucke
// queuePermitted is the consent chain, in one place so the door and the drain
// cannot answer it differently.
-func queuePermitted(ctx context.Context, s release.Store, acct, app string) (*Record, bool, error) {
+func queuePermitted(ctx context.Context, s release.Store, acct, app string) (*Record, QueueRefusal, error) {
rec, _, err := Get(ctx, s, acct, app)
if err != nil {
- return nil, false, err
+ return nil, RefusalNone, err
}
if rec == nil || rec.Provisioned == nil {
- return nil, false, nil
+ return nil, RefusalNotGranted, nil
}
tomb, err := release.ReadAppTombstone(ctx, s, acct, app)
if err != nil {
- return nil, false, err
+ return nil, RefusalNone, err
}
if tomb != nil {
- return nil, false, nil
+ return nil, RefusalTrashed, nil
}
- if rec.Scanning == nil || rec.Scanning.Requested == nil ||
- rec.Scanning.Requested.Encrypted || rec.Scanning.Enabled == nil {
- return nil, false, nil
+ switch {
+ case rec.Scanning == nil || rec.Scanning.Requested == nil:
+ return nil, RefusalNotRequested, nil
+ case rec.Scanning.Requested.Encrypted:
+ return nil, RefusalEncrypted, nil
+ case rec.Scanning.Enabled == nil:
+ return nil, RefusalNotEnabled, nil
}
on, err := release.AntivirusEnabled(ctx, s)
if err != nil {
- return nil, false, err
+ return nil, RefusalNone, err
}
if !on {
- // Reversible: the caller KEEPS the items. Distinguished from the
- // refusals above by the record still being intact.
- return rec, false, ErrScanningDisabled
+ // The record comes back because this refusal is REVERSIBLE and the
+ // caller may want to say something about the store it is still
+ // refusing to scan. Everything above returns nil: there is nothing
+ // there to talk about.
+ return rec, RefusalDisabled, nil
}
- return rec, true, nil
+ return rec, RefusalNone, nil
+}
+
+// QueueRefusal is why the consent chain said no, and RefusalNone is the only
+// value that means yes.
+//
+// It is a NAME rather than a bool because the enqueue door and the drain draw
+// different conclusions from the same answer — the door turns it into an HTTP
+// state (§5.1), the drain turns it into keep-or-delete (§7.5) — and a bool
+// forces each of them to re-derive the reason from the record they were handed
+// back. Two callers re-deriving the same fact from the same data is the exact
+// shape in which they start disagreeing, and the whole point of §5.1.1 is that
+// they must not.
+//
+// A read ERROR is not a refusal and never gets a name here. Nothing about
+// consent was learned, and §7.5's "deletion is driven by absence, never by
+// error" depends on those two staying separate: give a bad bucket moment a
+// name and something will eventually delete on it.
+type QueueRefusal string
+
+const (
+ RefusalNone QueueRefusal = ""
+ RefusalNotGranted QueueRefusal = "not_granted"
+ RefusalTrashed QueueRefusal = "trashed"
+ RefusalNotRequested QueueRefusal = "not_requested"
+ RefusalEncrypted QueueRefusal = "encrypted"
+ RefusalNotEnabled QueueRefusal = "not_enabled"
+ RefusalDisabled QueueRefusal = "disabled"
+)
+
+// Permanent reports whether this refusal will still be true in a way that
+// makes the queued item worthless — §7.5's delete-vs-keep split, in the one
+// place both callers read it from.
+//
+// Everything upstream of the deployment switch is permanent: a store that was
+// never granted, an app in the trash, scanning never requested, an attestation
+// saying the bytes are ciphertext, an operator who never enabled it. None of
+// those can change back in a way that makes THIS item useful — if the state is
+// later repaired the sweep covers the object anyway, and the requester is long
+// gone.
+//
+// The deployment switch is not. An operator runs `carlos system antivirus
+// enable` and every item still in the queue is worth draining. A deployment
+// disabled for longer than the TTL drops its queue, which is right: both doors
+// answered `disabled` at the time.
+//
+// Written as an explicit list rather than `r != RefusalDisabled`, so that a
+// refusal added later is permanent only because somebody said so. The default
+// for a new name is "keep the item", which is the survivable mistake.
+func (r QueueRefusal) Permanent() bool {
+ switch r {
+ case RefusalNotGranted, RefusalTrashed, RefusalNotRequested,
+ RefusalEncrypted, RefusalNotEnabled:
+ return true
+ }
+ return false
}
func listQueue(ctx context.Context, s release.Store, acct, app string, cap int) ([]string, error) {
diff --git a/internal/objstore/queue_test.go b/internal/objstore/queue_test.go
index 1f1a8e429..dccb34ab3 100644
--- a/internal/objstore/queue_test.go
+++ b/internal/objstore/queue_test.go
@@ -895,3 +895,167 @@ func (r *recordRaceStore) Get(ctx context.Context, key string) ([]byte, string,
func (r *recordRaceStore) ListPage(ctx context.Context, prefix, startAfter string, limit int) ([]string, error) {
return r.Store.(release.PageLister).ListPage(ctx, prefix, startAfter, limit)
}
+
+// THE CONSENT CHAIN IS ONE FUNCTION AND THIS IS THE TABLE THAT SAYS SO.
+//
+// Spec §5.1.1: the enqueue door and the drain must both refuse unless every
+// link holds, and the reason they must not drift is that they have different
+// consequences for the same answer — the door turns it into an HTTP state, the
+// drain turns it into keep-or-delete (§7.5). A bool cannot carry that. It has
+// already been a bool through ten tasks, and every caller reconstructed the
+// reason from the record it was handed back, which is exactly the shape where
+// two callers start disagreeing.
+//
+// So the refusal is named, and the two consequences are read off the name. The
+// split this asserts is §7.5's, verbatim: everything upstream of the
+// deployment switch is permanent, because none of it can change back in a way
+// that makes THIS item useful; the switch itself is reversible, because an
+// operator turns it on again and the item is still worth draining.
+func TestQueueRefusalNamesEachLinkAndItsConsequence(t *testing.T) {
+ ctx := context.Background()
+
+ cases := []struct {
+ name string
+ // break mutates a fully permitted fixture to snap one link.
+ snap func(t *testing.T, s release.Store)
+ want QueueRefusal
+ permanent bool
+ }{
+ {
+ name: "nothing is broken",
+ snap: func(*testing.T, release.Store) {},
+ want: RefusalNone,
+ },
+ {
+ name: "the store was never granted",
+ snap: func(t *testing.T, s release.Store) {
+ mutateRecord(t, s, func(r *Record) { r.Provisioned = nil })
+ },
+ want: RefusalNotGranted, permanent: true,
+ },
+ {
+ name: "the app is in the trash",
+ snap: func(t *testing.T, s release.Store) {
+ if err := release.TrashApp(ctx, s, "bab", "titogo", "op@example.com", time.Now().UTC()); err != nil {
+ t.Fatal(err)
+ }
+ },
+ want: RefusalTrashed, permanent: true,
+ },
+ {
+ name: "the member never requested scanning",
+ snap: func(t *testing.T, s release.Store) {
+ mutateRecord(t, s, func(r *Record) { r.Scanning.Requested = nil })
+ },
+ want: RefusalNotRequested, permanent: true,
+ },
+ {
+ name: "the member attested the store client-encrypted",
+ snap: func(t *testing.T, s release.Store) {
+ mutateRecord(t, s, func(r *Record) { r.Scanning.Requested.Encrypted = true })
+ },
+ want: RefusalEncrypted, permanent: true,
+ },
+ {
+ name: "the operator never granted scanning",
+ snap: func(t *testing.T, s release.Store) {
+ mutateRecord(t, s, func(r *Record) { r.Scanning.Enabled = nil })
+ },
+ want: RefusalNotEnabled, permanent: true,
+ },
+ {
+ // The one reversible link, and the reason Permanent() exists at
+ // all: an operator runs `carlos system antivirus enable` and every
+ // item still in the queue is worth draining. Delete these and a
+ // deployment that was off for ten minutes loses a tenant's
+ // accepted requests.
+ name: "the deployment switch is off",
+ snap: func(t *testing.T, s release.Store) {
+ disableDeployment(t, s)
+ },
+ want: RefusalDisabled, permanent: false,
+ },
+ }
+
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ s, _, _, _ := queueFixture(t, 1)
+ tc.snap(t, s)
+
+ rec, refusal, err := queuePermitted(ctx, s, "bab", "titogo")
+ if err != nil {
+ t.Fatalf("queuePermitted: %v", err)
+ }
+ if refusal != tc.want {
+ t.Fatalf("refusal = %q, want %q", refusal, tc.want)
+ }
+ if refusal.Permanent() != tc.permanent {
+ t.Fatalf("%q.Permanent() = %v, want %v — this decides whether the drain deletes the item or keeps it (§7.5)",
+ refusal, refusal.Permanent(), tc.permanent)
+ }
+ if tc.want == RefusalNone && rec == nil {
+ t.Fatal("a permitted app came back with no record")
+ }
+ })
+ }
+}
+
+// RefusalNone must never be Permanent: "keep going" is not a deletion reason,
+// and a Permanent() written as `r != RefusalDisabled` would say it was.
+func TestPermittedIsNotAPermanentRefusal(t *testing.T) {
+ if RefusalNone.Permanent() {
+ t.Fatal("RefusalNone.Permanent() is true — the drain would delete every item it was allowed to scan")
+ }
+}
+
+// A READ ERROR IS NOT A REFUSAL. It has no name in the chain because nothing
+// about consent was learned: §7.5's "deletion is driven by absence, never by
+// error". The error comes back as an error, and the refusal stays empty — so a
+// caller that forgets to check err cannot mistake a bad bucket moment for
+// permission, and equally cannot mistake it for a permanent no.
+func TestQueuePermittedReportsAReadErrorAsAnError(t *testing.T) {
+ s, _, _, _ := queueFixture(t, 1)
+ broken := &queueStore{Store: s, failGetKey: "apps/bab/titogo/store.json"}
+
+ rec, refusal, err := queuePermitted(context.Background(), broken, "bab", "titogo")
+ if err == nil {
+ t.Fatal("a failing store.json read came back as a clean answer")
+ }
+ if refusal != RefusalNone {
+ t.Fatalf("refusal = %q on a read error, want the empty refusal — a named refusal here would be deleted on (§7.5)", refusal)
+ }
+ if rec != nil {
+ t.Fatal("a record came back from a read that failed")
+ }
+}
+
+// mutateRecord reads, edits and writes back one store.json with the real
+// etag, so a test can snap exactly one link of the consent chain and leave
+// every other link intact.
+func mutateRecord(t *testing.T, s release.Store, edit func(*Record)) {
+ t.Helper()
+ ctx := context.Background()
+ rec, etag, err := Get(ctx, s, "bab", "titogo")
+ if err != nil || rec == nil {
+ t.Fatalf("re-read the record: %+v, %v", rec, err)
+ }
+ edit(rec)
+ if _, err := Put(ctx, s, "bab", "titogo", rec, etag); err != nil {
+ t.Fatal(err)
+ }
+}
+
+// disableDeployment turns the deployment switch off the way `carlos system
+// antivirus disable` does — the record, not an env var.
+func disableDeployment(t *testing.T, s release.Store) {
+ t.Helper()
+ ctx := context.Background()
+ av, etag, err := release.GetAntivirus(ctx, s)
+ if err != nil || av == nil {
+ t.Fatalf("antivirus record: %+v, %v", av, err)
+ }
+ av.Enabled = false
+ if _, err := release.PutAntivirus(ctx, s, av, etag); err != nil {
+ t.Fatal(err)
+ }
+}