Skip to content

refactor: walk a subtree in steps, with its termination rules in one place - #46

Merged
ilyam8 merged 6 commits into
engine-sendfrom
engine-walk-loop
Oct 11, 2026
Merged

ilyam8 merged 6 commits into
engine-sendfrom
engine-walk-loop

Conversation

@ilyam8

@ilyam8 ilyam8 commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Summary

Restructures the walk behind Walk, WalkAll, BulkWalk and BulkWalkAll. No behavior or exported API change.

walk was one labeled loop whose break and continue carried the decisions, with one case per error status and the subtree, exception and increasing rules interleaved in one loop body.

  • The walk API moves from gosnmp.go into walk.go unchanged.
  • walk is the loop: it sends the walk's request, has visit judge the answer, and acts on an explicit walkStep (ask for the objects after the answer's last name, get the root as a leaf, or end).
  • walker.visit holds the rules for an answer in order: an empty answer, the error statuses (one range, tooBig to inconsistentName, logged by their generated names), exception values, the subtree, the increasing check, walkFn. walker.outside decides at a value outside the subtree (the leaf fallback, the root itself, the end); walkRoot gives the default root and the leading dot; inSubtree replaces strings.HasPrefix(name, root+".") without building the prefix for every value.
  • MaxRepetitions and AppOpts are still read once per walk. walkFn stays a parameter: as a field of walker it escaped with the client and moved WalkAll's closure and results to the heap.
  • The walk's log lines are guarded by Logger.enabled(). Two dead conditions are gone: the NoSuchInstance check of the root branch (the exception check returns first) and the request-type condition of the AppOpts c check (the walk runs only GetNext and GetBulk).

Each known walk bug has one known-bug comment where it happens, including one found in review: after the leaf fallback the walk keeps sending Gets.

The test commits pin what mutation probes and two reviews found unobserved, each passing on the previous walk: a sibling arc that shares the root's text (gosnmp/gosnmp#78, #93), walkFn changing MaxRepetitions or AppOpts mid-walk, a first answer echoing the root, the leaf fallback's Get answered inside the subtree, AppOpts{"c": nil}, and an empty answer with an unnamed error status. BenchmarkWalk runs WalkAll and BulkWalkAll over a 100-row column. The engine test harness now fails a scenario whose transcript passes 5000 lines, so a change that makes the engine loop fails in milliseconds instead of growing memory until the test timeout.

Testing

  • go test ./... on darwin/arm64 and GOARCH=386 (386 at the restructuring commit; the later commits change tests and comments); -race; the FIPS 140-only tests; golangci-lint v2.14.0 (incl. end2end): 0 issues; GOOS=windows go vet.
  • Goldens unchanged by the refactor commits. Logger output unchanged: the engine suites rerun with a recording Logger (method, format, arguments) on the old and new walk give the same calls (918 in the walk suite, all 18 status names).
  • Mutation probes on the new code (50, plus 8 from review): all caught except the unreachable request-type default and 4 that change only the Logger call (caught by the Logger comparison).
  • An independent equivalence review ran the old and new walk side by side: every pinned scenario with and without a Logger, 1.2M random scenarios (MIBs, roots, answers with every error status, exception values, siblings, the root, equal and decreasing names, dropped answers, v1 and v2c), and a matrix of walkFn, hook and Logger actions at every call point. No difference.
  • Interleaved A/B against the previous walk: time within noise; allocs/op WalkAll 2432 -> 2229 and BulkWalkAll 400 -> 295 (the guarded per-answer log line and the per-value prefix); BenchmarkGet identical.

@ilyam8
ilyam8 added this pull request to stack #38 October 9, 2026 22:46
@ilyam8 ilyam8 changed the title engine walk loop refactor: walk a subtree in steps, with its termination rules in one place Oct 9, 2026
@ilyam8
ilyam8 force-pushed the engine-walk-loop branch 2 times, most recently from 40d8000 to 5d6cac1 Compare October 11, 2026 09:46
WalkFunc, Walk, WalkAll, BulkWalk and BulkWalkAll move from gosnmp.go
into walk.go unchanged, as the request methods live in request.go.

No behavior change.
…arkWalk

All pass on the walk before its restructuring:

- sibling-arc: an answer whose name extends the root's last arc
  (.1.3.6.1.2.1.2.2.1.20.1 when walking .1.3.6.1.2.1.2.2.1.2) is outside
  the subtree and ends the walk.
- user-code/walkfn-sets-max-repetitions: a walkFn that changes
  MaxRepetitions does not change the walk's later GetBulk requests.
- user-code/walkfn-sets-app-opts-c: a walkFn that sets the AppOpts 'c'
  option does not turn off the walk's increasing check.

BenchmarkWalk runs WalkAll and BulkWalkAll, as the Netdata Agent's SNMP
collector does, over a 100-row table column on the in-memory agent.
…e place

walk was one labeled loop whose break and continue carried the decisions,
with one case per error status. It is now:

- walk: the loop. It sends the walk's request, has visit judge the
  answer, and acts on the returned walkStep: ask for the objects after
  the answer's last name, get the root (a leaf), or end.
- walkRoot: the default root and the leading dot.
- walker.request: GetBulk, GetNext or Get of the walk's OID.
- walker.visit: the rules for an answer: an empty answer, the error
  statuses (one range, TooBig to InconsistentName, logged by their
  generated names), exception values, the subtree, the increasing check,
  walkFn.
- walker.outside: an object outside the subtree: the leaf fallback, the
  root itself, the end.
- inSubtree: strings.HasPrefix(name, root+".") without building the
  prefix for every object.

MaxRepetitions and AppOpts are still read once per walk. walkFn stays a
parameter: as a field of walker it escapes with the client and moves
WalkAll's closure and results to the heap. The walk's log lines are
guarded by Logger.enabled(). Two dead conditions are gone: the
NoSuchInstance check of the root branch (the exception check returns
first) and the request-type condition of the AppOpts 'c' check (walk
runs only GetNext and GetBulk). Each pinned walk bug has a known-bug
comment where it happens.

No behavior change: goldens and logger output unchanged.
A change that makes the engine loop (a regression or a mutation probe)
used to grow the scenario's transcript until the test timed out, holding
gigabytes when several such runs had no timeout. The transcript now fails
its test once it passes 5000 lines, about a hundred times the longest
scenario (53 lines), so such a change fails in milliseconds.
All pass on the walk before its restructuring:

- leaf/root-echo: a first answer naming the root itself makes the walk
  take the root for a leaf and get it.
- leaf/get-answer-in-subtree: after the leaf fallback, an answer inside
  the subtree is walked and the walk sends a Get for that name (known
  bug: net-snmp's snmpwalk makes one Get).
- not-increasing/app-opts-c-nil: the AppOpts 'c' key turns the
  increasing check off whatever its value, nil included.
- error-status/19-unknown-empty: an answer with an unnamed error status
  and no varbinds ends the walk as an empty answer does.
From the step's reviews:

- The error-status rule names its range, tooBig to inconsistentName,
  in its condition and its comment instead of relying on the noError
  case before it.
- The leaf-root known bug sits where it happens: an endOfMibView first
  answer, or SNMPv1's noSuchName, ends the walk before the fallback.
- A new known-bug comment where the fallback switches to Get: the walk
  keeps sending Gets after it.
- The default root's comment cites X.690 alone; RFC 2578 section 7.1.3
  sets no minimum number of arcs.
- The fallback cites gosnmp/gosnmp#78, which added it; the subtree
  example names the excess names correctly.

No behavior change: goldens and logger output unchanged.
@ilyam8
ilyam8 merged commit 6aa79e5 into master Oct 11, 2026
21 checks passed
@ilyam8
ilyam8 deleted the engine-walk-loop branch October 11, 2026 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants