diff --git a/modules/mesh-issues/README.md b/modules/mesh-issues/README.md index f595a354..71aa3743 100644 --- a/modules/mesh-issues/README.md +++ b/modules/mesh-issues/README.md @@ -48,7 +48,9 @@ bounded number of times with a short random wait; past the bound the open is ref - **A refused open takes no number**: an issue is checked before its number is taken. - **A number is never handed out twice, and may be skipped.** A number taken and then not kept — the bus failing between the counter and the issue — stays unused, and the answer says which. A write whose answer - was lost is recognised as its own when tried again, so it leaves no gap. + was lost is recognised as its own when tried again, so it leaves no gap; when it cannot be confirmed either + way, the answer says the issue may have been written and to `get` that number before opening it again, + never that nothing was opened. - **An issue is never overwritten.** It is kept by a write that is refused when the key exists; if the counter ever hands out a number already kept, the open is refused loudly, the number stays unused, and nothing is overwritten. @@ -85,8 +87,9 @@ verb takes an argument naming a caller, and none can be given one. - `first-number` — the number the first issue gets: the one after the highest issue number on hq's main and every open branch at the moment the module is switched on. Read once. - `owners` — the registered list `located-in` may name: repositories, and modules as - `mesh-catalog modules/`. An entry of `located-in` is an owner, or an owner followed by a space and - where in it (`mesh-controller internal/broker`). It starts with the mesh's repositories and the modules + `mesh-catalog modules/`. An entry of `located-in` is an owner, or an owner followed by a space or a + slash and where in it (`mesh-controller internal/broker`, `mesh-catalog modules/nats/module.json`). A module + must itself be registered: `mesh-catalog modules/` is never taken as a place in `mesh-catalog`. It starts with the mesh's repositories and the modules the catalogue held when this module was written; a module added since is added here through the controller's settings verb. diff --git a/modules/mesh-issues/cmd/mesh-issues/bus_test.go b/modules/mesh-issues/cmd/mesh-issues/bus_test.go index 11295de2..218feeeb 100644 --- a/modules/mesh-issues/cmd/mesh-issues/bus_test.go +++ b/modules/mesh-issues/cmd/mesh-issues/bus_test.go @@ -43,9 +43,11 @@ func TestMain(m *testing.M) { type bus struct { // lost counts the compare-and-sets another writer won: proof the openers collided. lost atomic.Int64 - nc *nats.Conn - js jetstream.JetStream - kvs map[string]jetstream.KeyValue + // counterLost counts only the counter's updates another opener won: proof the counter itself was contended. + counterLost atomic.Int64 + nc *nats.Conn + js jetstream.JetStream + kvs map[string]jetstream.KeyValue } func newBus(t *testing.T) *bus { @@ -243,6 +245,9 @@ func (i *instance) answer(m rpc) { errors.As(err, &api) && api.ErrorCode == jetstream.JSErrCodeStreamWrongLastSequence { code = -32010 i.b.lost.Add(1) + if m.Method == "mesh/state.update" && p.State == CountersState { + i.b.counterLost.Add(1) + } } i.say(map[string]any{"id": m.ID, "error": map[string]any{"code": code, "message": err.Error()}}) return @@ -336,9 +341,10 @@ func TestFiftyOpensAtOnceFromTwoInstancesGetFiftyDistinctConsecutiveNumbers(t *t } } // The openers collided: writes were refused because another opener had moved the counter first. - t.Logf("%d writes lost to another opener and tried again", b.lost.Load()) - if b.lost.Load() == 0 { - t.Fatal("no opener ever lost a write: the test did not make them collide") + t.Logf("%d writes lost to another opener and tried again, %d of them the counter's updates", b.lost.Load(), b.counterLost.Load()) + if b.counterLost.Load() < opens { + t.Fatalf("only %d of the counter's updates were lost to another opener: the test did not make the openers "+ + "contend for the counter", b.counterLost.Load()) } ctx := context.Background() e, err := b.kvs[CountersState].Get(ctx, IssueCounter) diff --git a/modules/mesh-issues/cmd/mesh-issues/check.go b/modules/mesh-issues/cmd/mesh-issues/check.go index bc7e180f..9d8a1542 100644 --- a/modules/mesh-issues/cmd/mesh-issues/check.go +++ b/modules/mesh-issues/cmd/mesh-issues/check.go @@ -58,6 +58,9 @@ func plainText(field, s string, max int, lines bool) error { if lines && (r == '\n' || r == '\t') { continue } + if r == '\u2028' || r == '\u2029' { + return fmt.Errorf("%s carries the line separator %U; break lines with a newline", field, r) + } if unicode.IsControl(r) || unicode.Is(unicode.Cf, r) && r != '‍' { if lines { return fmt.Errorf("%s carries the unprintable character %U; only line breaks and tabs are kept", field, r) @@ -138,15 +141,20 @@ func entries(field string, list []string, one func(string) (string, error)) ([]s // ` modules/` (to-be 53 §3). type Owners []string -// owns is the registered owner an entry names — the longest one it is or starts with, followed by a space -// and where in it — or "". +// owns is the registered owner an entry names — the longest one it is, or starts with followed by a space +// or a slash and where in it — or "". An entry that names a module of a repository (` +// modules/`) is owned only by that module registered, never by the repository alone, so a module +// not on the list is not taken as a place in the repository. func (o Owners) owns(entry string) string { best := "" for _, owner := range o { - if (entry == owner || strings.HasPrefix(entry, owner+" ")) && len(owner) > len(best) { + if (entry == owner || strings.HasPrefix(entry, owner+" ") || strings.HasPrefix(entry, owner+"/")) && len(owner) > len(best) { best = owner } } + if repo, rest, ok := strings.Cut(entry, " "); ok && strings.HasPrefix(rest, "modules/") && best == repo { + return "" + } return best } @@ -180,7 +188,7 @@ func IsCore(locatedIn []string) bool { } // replayID is a replay's id in mesh-lab's replays register, optionally followed by words. -var replayID = regexp.MustCompile(`^R[0-9]{1,6}( .*)?$`) +var replayID = regexp.MustCompile(`^R[0-9]{1,6}\b`) // designPath is a design document in hq, as `amended-design` names one. var designPath = regexp.MustCompile(`^03-DESIGN/(00-as-is|01-to-be)/[0-9]{2}-[a-z0-9]+(-[a-z0-9]+)*\.md$`) @@ -240,5 +248,8 @@ func checkTarget(kind, target string) (string, error) { if !ok { return "", fmt.Errorf("a %s link's target is %s, not %q", kind, shape, t) } + if what := secretShape(t); what != "" { + return "", fmt.Errorf("the target looks like it carries %s; a link names a thing, never a secret", what) + } return t, nil } diff --git a/modules/mesh-issues/cmd/mesh-issues/tracker.go b/modules/mesh-issues/cmd/mesh-issues/tracker.go index 0d3f340d..96222c49 100644 --- a/modules/mesh-issues/cmd/mesh-issues/tracker.go +++ b/modules/mesh-issues/cmd/mesh-issues/tracker.go @@ -40,7 +40,7 @@ const ( // caller is told, and nothing was written. const ( // counterRounds × counterAttempts is how many times one open tries to advance the counter. - counterRounds = 12 + counterRounds = 20 counterAttempts = 8 // issueAttempts is how many times a change to one issue is read and written again. issueAttempts = 16 @@ -129,9 +129,6 @@ func (t *Tracker) Open(a OpenAsk) (*Opened, error) { if record == "" { is.Status = StatusOpen } - if size, _ := json.Marshal(is); len(size) > MostIssueBytes-1024 { - return nil, fmt.Errorf("the issue is %d bytes; one issue is kept in at most %d", len(size), MostIssueBytes) - } n, err := t.next() if err != nil { @@ -161,13 +158,22 @@ func (t *Tracker) create(is *Issue) error { key := keyOf(is.Number) var err error for i := 0; i < createAttempts; i++ { + if i > 0 { + t.Sleep(jitter(time.Duration(i) * 50 * time.Millisecond)) + } _, err = t.Store.Create(IssuesState, key, is) if err == nil { return nil } if errors.Is(err, stdio.ErrChanged) { // Ours, written by an attempt whose answer was lost? - if there, _, rerr := read(t.Store, is.Number); rerr == nil && there.Opening == is.Opening { + there, _, rerr := read(t.Store, is.Number) + if rerr != nil { + t.Logf("[mesh-issues] issue %d is kept and could not be read back: %v", is.Number, rerr) + return fmt.Errorf("issue %d may have been written by this open: the key is taken and could not be read "+ + "back (%v). Call get %d before opening it again, so it is not opened twice", is.Number, rerr, is.Number) + } + if there.Opening == is.Opening { return nil } t.Logf("[mesh-issues] the counter handed out %d and an issue %d is already kept: not overwritten", is.Number, is.Number) @@ -176,9 +182,13 @@ func (t *Tracker) create(is *Issue) error { "and tell the operator the counter is behind the issues kept", is.Number, is.Number, is.Number) } } - t.Logf("[mesh-issues] number %d was taken and the issue could not be written: %v", is.Number, err) - return fmt.Errorf("number %d was taken and the issue could not be written (%v); nothing was opened and %d "+ - "stays unused. Open it again", is.Number, err, is.Number) + // A write that timed out may still have landed: say so, never "nothing was opened". + t.Logf("[mesh-issues] number %d was taken and the issue's write was not confirmed: %v", is.Number, err) + if there, _, rerr := read(t.Store, is.Number); rerr == nil && there.Opening == is.Opening { + return nil + } + return fmt.Errorf("number %d was taken and the issue's write was not confirmed (%v): it may have been written. "+ + "Call get %d before opening it again; if there is no issue %d, it stays unused", is.Number, err, is.Number, is.Number) } // next takes the next issue number: the counter advanced only at the revision read, so two openers — in diff --git a/modules/mesh-issues/cmd/mesh-issues/tracker_test.go b/modules/mesh-issues/cmd/mesh-issues/tracker_test.go index 0e3d6552..eedbe248 100644 --- a/modules/mesh-issues/cmd/mesh-issues/tracker_test.go +++ b/modules/mesh-issues/cmd/mesh-issues/tracker_test.go @@ -3,11 +3,14 @@ package main import ( "encoding/json" "errors" + "fmt" "reflect" "strings" "sync" "testing" "time" + + stdio "git.novox.be/novox/mesh-sdk/go" ) func TestTheFirstIssueGetsTheSettingsNumberAndEachNextOneMore(t *testing.T) { @@ -113,7 +116,7 @@ func TestANumberTakenAndNotWrittenIsSaidAndNeverHandedOutAgain(t *testing.T) { failing := &failingStore{memory: h.mem, failCreates: createAttempts} h.Store = failing _, err := h.Open(OpenAsk{Title: "third", Report: "r"}) - if err == nil || !strings.Contains(err.Error(), "367 stays unused") { + if err == nil || !strings.Contains(err.Error(), "may have been written") || !strings.Contains(err.Error(), "get 367") { t.Fatalf("a write that never landed: %v", err) } h.Store = h.mem @@ -160,6 +163,47 @@ func TestAWriteWhoseAnswerWasLostIsKnownAsItsOwn(t *testing.T) { } } +// lostEvery writes the issue on its first create and then answers every create with a failure. +type lostEvery struct{ *memory } + +func (l lostEvery) Create(state, key string, value any) (uint64, error) { + if state == IssuesState { + _, _ = l.memory.Create(state, key, value) + return 0, errors.New("no answer in 30 s") + } + return l.memory.Create(state, key, value) +} + +// unreadable answers a taken key with a refusal and cannot read it back. +type unreadable struct{ *memory } + +func (u unreadable) Create(state, key string, value any) (uint64, error) { + if state == IssuesState { + return 0, fmt.Errorf("%s: %w", key, stdio.ErrChanged) + } + return u.memory.Create(state, key, value) +} +func (u unreadable) Get(state, key string) (*Entry, error) { + if state == IssuesState { + return nil, errors.New("no answer in 30 s") + } + return u.memory.Get(state, key) +} + +// An open whose write may have landed never says nothing was opened (the review of 2026-10-10). +func TestAnOpenWhoseWriteMayHaveLandedNeverSaysNothingWasOpened(t *testing.T) { + h := newHarness(t) + h.Store = lostEvery{h.mem} + if o, err := h.Open(OpenAsk{Title: "t", Report: "r"}); err != nil || o.Number != 365 { + t.Fatalf("a write that landed, every answer lost: %v %v", o, err) + } + h.Store = unreadable{h.mem} + _, err := h.Open(OpenAsk{Title: "t", Report: "r"}) + if err == nil || strings.Contains(err.Error(), "nothing was opened") || !strings.Contains(err.Error(), "get 366") { + t.Fatalf("a taken key that cannot be read back: %v", err) + } +} + func TestOpenSaysAnEventWithWhatChangedAndWhoChangedIt(t *testing.T) { h := newHarness(t) o, err := h.Open(OpenAsk{Title: "a title", Report: "a report", LocatedIn: []string{"mesh-controller internal/broker"}}) @@ -407,6 +451,7 @@ func TestLinksAreTypedAddedOnceAndRemoved(t *testing.T) { {LinkAsk{Kind: LinkDecision, Target: "275"}, "four-digit"}, {LinkAsk{Kind: LinkDesign, Target: "03-DESIGN/../../etc/passwd"}, "design document"}, {LinkAsk{Kind: LinkCondition, Target: "a key\nwith a line"}, "condition"}, + {LinkAsk{Kind: LinkCondition, Target: "token=abcdefgh123"}, "secret"}, {LinkAsk{Kind: LinkRelatesTo, Target: keyOf(b), Remove: true}, ""}, {LinkAsk{Kind: LinkRelatesTo, Target: keyOf(b), Remove: true}, "no relates-to link"}, } @@ -549,7 +594,9 @@ func TestAnOwnersEntryIsTheRegisteredOneOrStartsWithIt(t *testing.T) { for entry, want := range map[string]string{ "mesh-catalog": "mesh-catalog", "mesh-catalog modules/nats": "mesh-catalog modules/nats", - "mesh-catalog modules/nats/module.go": "mesh-catalog", + "mesh-catalog modules/nats/module.go": "mesh-catalog modules/nats", + "mesh-catalog modules/does-not-exist": "", + "mesh-catalog checks/words": "mesh-catalog", "mesh-catalog modules/nats (the gate)": "mesh-catalog modules/nats", "mesh-controller cmd/mesh-controller": "mesh-controller", "mesh-controllers": "",