From c637d3e384b60da40dc2d74bb82dc5137996a1f2 Mon Sep 17 00:00:00 2001 From: steiler Date: Fri, 4 Sep 2026 14:31:34 +0200 Subject: [PATCH 1/4] netconf: classify mid-flight transport drops as ErrNotConnected Close the residual gap left by PR #466: the netconf target's three mid-flight transport-drop branches (internalGet's GetConfig, setToDevice's EditConfig and Commit) detected drops via a fragile 'EOF' substring check and returned the raw driver error, so it crossed the wire as codes.Unknown instead of the already-established codes.Unavailable/ErrNotConnected recoverable path. - Add isTransportError in the netconf package: checks errors.Is(io.EOF), errors.As(net.Error), and a substring fallback (EOF, broken pipe, connection reset, i/o timeout) for drivers that don't preserve that wrapping. - Normalize scrapligo's own opaque errConnectionError sentinel (which a real mid-flight EOF actually surfaces as, per tracing scrapligo v1.4.1's channel read loop) into io.EOF at the driver/scrapligo adapter boundary, so nc.go's isTransportError stays driver-agnostic and only scrapligo-specific code knows about scrapligo's sentinels. - Replace all three call sites' substring check with isTransportError, and wrap the returned error with targettypes.ErrNotConnected via a shared handleTransportError helper, so translateInternalToGrpcError maps it to codes.Unavailable like the pre-flight checks already do. - Leave the candidate Discard()-on-error path untouched for non-transport errors, per spec. See .scratch/netconf-midflight-eof-not-connected/spec.md. --- .../netconf/driver/scrapligo/scrapligo.go | 29 ++- .../driver/scrapligo/scrapligo_test.go | 99 ++++++++ pkg/datastore/target/netconf/nc.go | 27 ++- .../target/netconf/nc_transport_error_test.go | 214 ++++++++++++++++++ .../target/netconf/transport_error.go | 57 +++++ .../target/netconf/transport_error_test.go | 84 +++++++ 6 files changed, 496 insertions(+), 14 deletions(-) create mode 100644 pkg/datastore/target/netconf/driver/scrapligo/scrapligo_test.go create mode 100644 pkg/datastore/target/netconf/nc_transport_error_test.go create mode 100644 pkg/datastore/target/netconf/transport_error.go create mode 100644 pkg/datastore/target/netconf/transport_error_test.go diff --git a/pkg/datastore/target/netconf/driver/scrapligo/scrapligo.go b/pkg/datastore/target/netconf/driver/scrapligo/scrapligo.go index 424a8651..fd0b1578 100644 --- a/pkg/datastore/target/netconf/driver/scrapligo/scrapligo.go +++ b/pkg/datastore/target/netconf/driver/scrapligo/scrapligo.go @@ -15,7 +15,9 @@ package scrapligo import ( + "errors" "fmt" + "io" "github.com/beevik/etree" scraplinetconf "github.com/scrapli/scrapligo/driver/netconf" @@ -26,6 +28,27 @@ import ( "github.com/sdcio/data-server/pkg/datastore/target/netconf/types" ) +// normalizeTransportError translates scrapligo's own opaque connection-drop +// sentinel (util.ErrConnectionError) into io.EOF. +// +// scrapligo's channel read loop swallows a real transport io.EOF/connection +// drop and re-surfaces it as util.ErrConnectionError (see +// scrapligo/channel/read.go and driver/netconf/rpc.go), so callers that only +// understand the standard io/net error vocabulary would otherwise never +// recognize a mid-flight transport drop. This keeps that scrapligo-specific +// knowledge contained to this driver adapter, per the Driver interface +// abstraction: everything on the other side of the Driver interface (nc.go) +// stays driver-agnostic and only needs to know about io.EOF/net.Error. +func normalizeTransportError(err error) error { + if err == nil { + return nil + } + if errors.Is(err, util.ErrConnectionError) { + return fmt.Errorf("%w: %w", io.EOF, err) + } + return err +} + type ScrapligoNetconfTarget struct { driver *scraplinetconf.Driver } @@ -95,7 +118,7 @@ func (snt *ScrapligoNetconfTarget) EditConfig(target string, config string) (*ty // send the edit config rpc resp, err := snt.driver.EditConfig(target, xdoc) if err != nil { - return nil, err + return nil, normalizeTransportError(err) } if len(resp.ErrorMessages) > 0 { return nil, resp.Failed @@ -119,7 +142,7 @@ func (snt *ScrapligoNetconfTarget) GetConfig(source string, filter string) (*typ // execute the GetConfig rpc resp, err := snt.driver.GetConfig(source, filterDoc, options.WithNetconfForceSelfClosingTags()) if err != nil { - return nil, err + return nil, normalizeTransportError(err) } if resp.Failed != nil { return nil, resp.Failed @@ -151,7 +174,7 @@ func (snt *ScrapligoNetconfTarget) Commit() error { // execute the Commit rpc resp, err := snt.driver.Commit() if err != nil { - return err + return normalizeTransportError(err) } if resp.Failed != nil { return resp.Failed diff --git a/pkg/datastore/target/netconf/driver/scrapligo/scrapligo_test.go b/pkg/datastore/target/netconf/driver/scrapligo/scrapligo_test.go new file mode 100644 index 00000000..6fe7b118 --- /dev/null +++ b/pkg/datastore/target/netconf/driver/scrapligo/scrapligo_test.go @@ -0,0 +1,99 @@ +// Copyright 2024 Nokia +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package scrapligo + +import ( + "errors" + "fmt" + "io" + "testing" + + "github.com/scrapli/scrapligo/util" +) + +func Test_normalizeTransportError(t *testing.T) { + tests := []struct { + name string + err error + wantNil bool + wantEOF bool + wantSameAs error + }{ + { + name: "nil error", + err: nil, + wantNil: true, + }, + { + name: "scrapligo connection error is normalized to io.EOF", + err: util.ErrConnectionError, + wantEOF: true, + }, + { + name: "wrapped scrapligo connection error is still normalized to io.EOF", + err: fmt.Errorf("channel read loop failed: %w", util.ErrConnectionError), + wantEOF: true, + }, + { + name: "unrelated scrapligo error is returned unchanged", + err: util.ErrTimeoutError, + wantSameAs: util.ErrTimeoutError, + }, + { + name: "generic error is returned unchanged", + err: errors.New("some other failure"), + wantSameAs: nil, // checked by message equality below instead + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := normalizeTransportError(tt.err) + + if tt.wantNil { + if got != nil { + t.Fatalf("expected nil, got %v", got) + } + return + } + + if tt.wantEOF { + if !errors.Is(got, io.EOF) { + t.Errorf("expected errors.Is(got, io.EOF), got %v", got) + } + // the original scrapligo error must still be discoverable so + // logs/debugging don't lose the underlying detail. + if !errors.Is(got, util.ErrConnectionError) { + t.Errorf("expected the original util.ErrConnectionError to still be discoverable via errors.Is, got %v", got) + } + return + } + + if tt.wantSameAs != nil { + if !errors.Is(got, tt.wantSameAs) { + t.Errorf("expected error to be returned unchanged, got %v want %v", got, tt.wantSameAs) + } + if errors.Is(got, io.EOF) { + t.Errorf("did not expect a non-connection error to be normalized to io.EOF, got %v", got) + } + return + } + + // generic error case: returned unchanged (same error value). + if got != tt.err { + t.Errorf("expected the original error to be returned unchanged, got %v want %v", got, tt.err) + } + }) + } +} diff --git a/pkg/datastore/target/netconf/nc.go b/pkg/datastore/target/netconf/nc.go index c2ce441f..d46ba632 100644 --- a/pkg/datastore/target/netconf/nc.go +++ b/pkg/datastore/target/netconf/nc.go @@ -17,7 +17,6 @@ package netconf import ( "context" "fmt" - "strings" "sync" "time" @@ -130,9 +129,8 @@ func (t *ncTarget) internalGet(ctx context.Context, req *sdcpb.GetDataRequest) ( // execute the GetConfig rpc ncResponse, err := t.driver.GetConfig(source, filterDoc) if err != nil { - if strings.Contains(err.Error(), "EOF") { - _ = t.Close(ctx) - go t.reconnect(ctx) + if isTransportError(err) { + return nil, t.handleTransportError(ctx, err) } return nil, err } @@ -186,6 +184,16 @@ func (t *ncTarget) Status() *types.TargetStatus { return result } +// handleTransportError closes the current connection and kicks off a +// background reconnect, then wraps err in types.ErrNotConnected so the +// caller's caller (ultimately the TransactionSet gRPC handler, via +// translateInternalToGrpcError) can classify it as recoverable. +func (t *ncTarget) handleTransportError(ctx context.Context, err error) error { + _ = t.Close(ctx) + go t.reconnect(ctx) + return fmt.Errorf("%s: %w: %w", t.name, types.ErrNotConnected, err) +} + func (t *ncTarget) Close(ctx context.Context) error { if t == nil { return nil @@ -261,10 +269,8 @@ func (t *ncTarget) setToDevice(ctx context.Context, commitDatastore string, sour resp, err := t.driver.EditConfig(commitDatastore, xdoc) if err != nil { log.Error(err, "failed during edit-config") - if strings.Contains(err.Error(), "EOF") { - _ = t.Close(ctx) - go t.reconnect(ctx) - return nil, err + if isTransportError(err) { + return nil, t.handleTransportError(ctx, err) } // candidate should discard on error @@ -288,9 +294,8 @@ func (t *ncTarget) setToDevice(ctx context.Context, commitDatastore string, sour // commit the config err = t.driver.Commit() if err != nil { - if strings.Contains(err.Error(), "EOF") { - _ = t.Close(ctx) - go t.reconnect(ctx) + if isTransportError(err) { + return nil, t.handleTransportError(ctx, err) } return nil, err } diff --git a/pkg/datastore/target/netconf/nc_transport_error_test.go b/pkg/datastore/target/netconf/nc_transport_error_test.go new file mode 100644 index 00000000..5bae8d93 --- /dev/null +++ b/pkg/datastore/target/netconf/nc_transport_error_test.go @@ -0,0 +1,214 @@ +// Copyright 2024 Nokia +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package netconf + +import ( + "context" + "errors" + "io" + "sync" + "testing" + "time" + + "github.com/beevik/etree" + "github.com/sdcio/data-server/mocks/mocknetconf" + "github.com/sdcio/data-server/pkg/config" + nctypes "github.com/sdcio/data-server/pkg/datastore/target/netconf/types" + "github.com/sdcio/data-server/pkg/datastore/target/types" + sdcpb "github.com/sdcio/sdc-protos/sdcpb" + "go.uber.org/mock/gomock" +) + +// fakeTargetSource is a minimal types.TargetSource used to drive setToDevice +// without needing a real datastore tree. +type fakeTargetSource struct { + doc *etree.Document +} + +func newFakeTargetSource() *fakeTargetSource { + doc := etree.NewDocument() + if err := doc.ReadFromString("eth0"); err != nil { + panic(err) + } + return &fakeTargetSource{doc: doc} +} + +func (f *fakeTargetSource) ToJson(_ context.Context, _ bool) (any, error) { return nil, nil } +func (f *fakeTargetSource) ToJsonIETF(_ context.Context, _ bool) (any, error) { + return nil, nil +} + +func (f *fakeTargetSource) ToXML(_ context.Context, _, _, _, _ bool) (*etree.Document, error) { + return f.doc, nil +} + +func (f *fakeTargetSource) ToProtoUpdates(_ context.Context, _ bool) ([]*sdcpb.Update, error) { + return nil, nil +} + +func (f *fakeTargetSource) ToProtoDeletes(_ context.Context) ([]*sdcpb.Path, error) { + return nil, nil +} + +func (f *fakeTargetSource) ContainsChanges(_ context.Context) (bool, error) { + return true, nil +} + +// testSBIConfig returns a minimal SBI config good enough to let a background +// reconnect() attempt (and quickly fail/retry against a bogus address) +// without blocking the test itself, since reconnect is always launched via +// `go t.reconnect(ctx)`. +func testSBIConfig() *config.SBI { + return &config.SBI{ + Address: "127.0.0.1:0", + ConnectRetry: time.Millisecond, + NetconfOptions: &config.SBINetconfOptions{ + CommitDatastore: "candidate", + }, + } +} + +func rpcReplyDoc(t *testing.T) *etree.Document { + t.Helper() + doc := etree.NewDocument() + if err := doc.ReadFromString(""); err != nil { + t.Fatalf("building rpc-reply doc: %v", err) + } + return doc +} + +func Test_ncTarget_internalGet_TransportError(t *testing.T) { + mockCtrl := gomock.NewController(t) + d := mocknetconf.NewMockDriver(mockCtrl) + d.EXPECT().IsAlive().AnyTimes().Return(true) + d.EXPECT().GetConfig(gomock.Any(), gomock.Any()).Return(nil, io.EOF) + d.EXPECT().Close().Return(nil) + + tr := &ncTarget{ + name: "dev1", + m: new(sync.Mutex), + driver: d, + sbiConfig: testSBIConfig(), + } + + _, err := tr.internalGet(context.Background(), &sdcpb.GetDataRequest{}) + if err == nil { + t.Fatal("expected an error, got nil") + } + if !errors.Is(err, types.ErrNotConnected) { + t.Errorf("expected errors.Is(err, types.ErrNotConnected), got %v", err) + } + if !errors.Is(err, io.EOF) { + t.Errorf("expected the original io.EOF to still be discoverable via errors.Is, got %v", err) + } +} + +func Test_ncTarget_internalGet_NonTransportError(t *testing.T) { + mockCtrl := gomock.NewController(t) + d := mocknetconf.NewMockDriver(mockCtrl) + d.EXPECT().IsAlive().AnyTimes().Return(true) + nonTransportErr := errors.New("invalid filter") + d.EXPECT().GetConfig(gomock.Any(), gomock.Any()).Return(nil, nonTransportErr) + // Close must NOT be called for a non-transport error; the strict mock + // controller will fail the test if it is. + + tr := &ncTarget{ + name: "dev1", + m: new(sync.Mutex), + driver: d, + sbiConfig: testSBIConfig(), + } + + _, err := tr.internalGet(context.Background(), &sdcpb.GetDataRequest{}) + if !errors.Is(err, nonTransportErr) { + t.Errorf("expected the original error to be returned unwrapped, got %v", err) + } + if errors.Is(err, types.ErrNotConnected) { + t.Errorf("did not expect errors.Is(err, types.ErrNotConnected), got %v", err) + } +} + +func Test_ncTarget_setToDevice_EditConfig_TransportError(t *testing.T) { + mockCtrl := gomock.NewController(t) + d := mocknetconf.NewMockDriver(mockCtrl) + d.EXPECT().IsAlive().AnyTimes().Return(true) + d.EXPECT().EditConfig(gomock.Any(), gomock.Any()).Return(nil, io.EOF) + d.EXPECT().Close().Return(nil) + // Discard must NOT be called for a transport error. + + tr := &ncTarget{ + name: "dev1", + m: new(sync.Mutex), + driver: d, + sbiConfig: testSBIConfig(), + } + + _, err := tr.setToDevice(context.Background(), "candidate", newFakeTargetSource()) + if !errors.Is(err, types.ErrNotConnected) { + t.Errorf("expected errors.Is(err, types.ErrNotConnected), got %v", err) + } + if !errors.Is(err, io.EOF) { + t.Errorf("expected the original io.EOF to still be discoverable via errors.Is, got %v", err) + } +} + +func Test_ncTarget_setToDevice_EditConfig_NonTransportError_DiscardsCandidate(t *testing.T) { + mockCtrl := gomock.NewController(t) + d := mocknetconf.NewMockDriver(mockCtrl) + d.EXPECT().IsAlive().AnyTimes().Return(true) + nonTransportErr := errors.New("rpc-error: data-exists") + d.EXPECT().EditConfig(gomock.Any(), gomock.Any()).Return(nil, nonTransportErr) + d.EXPECT().Discard().Return(nil) + // Close must NOT be called for a non-transport error. + + tr := &ncTarget{ + name: "dev1", + m: new(sync.Mutex), + driver: d, + sbiConfig: testSBIConfig(), + } + + _, err := tr.setToDevice(context.Background(), "candidate", newFakeTargetSource()) + if !errors.Is(err, nonTransportErr) { + t.Errorf("expected the original error to be returned unwrapped, got %v", err) + } + if errors.Is(err, types.ErrNotConnected) { + t.Errorf("did not expect errors.Is(err, types.ErrNotConnected), got %v", err) + } +} + +func Test_ncTarget_setToDevice_Commit_TransportError(t *testing.T) { + mockCtrl := gomock.NewController(t) + d := mocknetconf.NewMockDriver(mockCtrl) + d.EXPECT().IsAlive().AnyTimes().Return(true) + d.EXPECT().EditConfig(gomock.Any(), gomock.Any()).Return(&nctypes.NetconfResponse{Doc: rpcReplyDoc(t)}, nil) + d.EXPECT().Commit().Return(io.EOF) + d.EXPECT().Close().Return(nil) + + tr := &ncTarget{ + name: "dev1", + m: new(sync.Mutex), + driver: d, + sbiConfig: testSBIConfig(), + } + + _, err := tr.setToDevice(context.Background(), "candidate", newFakeTargetSource()) + if !errors.Is(err, types.ErrNotConnected) { + t.Errorf("expected errors.Is(err, types.ErrNotConnected), got %v", err) + } + if !errors.Is(err, io.EOF) { + t.Errorf("expected the original io.EOF to still be discoverable via errors.Is, got %v", err) + } +} \ No newline at end of file diff --git a/pkg/datastore/target/netconf/transport_error.go b/pkg/datastore/target/netconf/transport_error.go new file mode 100644 index 00000000..5d16e45f --- /dev/null +++ b/pkg/datastore/target/netconf/transport_error.go @@ -0,0 +1,57 @@ +// Copyright 2024 Nokia +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package netconf + +import ( + "errors" + "io" + "net" + "strings" +) + +// isTransportError reports whether err represents a transport-level connection +// drop (e.g. the southbound device restarting mid-RPC) rather than a +// semantic/validation failure returned by the device itself. +// +// It relies only on the standard io/net error vocabulary (io.EOF, net.Error) +// plus a substring fallback for drivers that don't preserve that wrapping. +// Driver implementations are responsible for normalizing their own +// library-specific transport sentinels into io.EOF (or a net.Error) at the +// Driver interface boundary, so this function stays driver-agnostic. +func isTransportError(err error) bool { + if err == nil { + return false + } + if errors.Is(err, io.EOF) { + return true + } + var netErr net.Error + if errors.As(err, &netErr) { + return true + } + // Substring fallback: a safety net for drivers other than scrapligo (or a + // future scrapligo version) that don't preserve io.EOF/net.Error wrapping + // through their own error paths. Driver implementations should still + // prefer normalizing at their own boundary (see driver/scrapligo's + // normalizeTransportError) so this fallback ideally never fires in + // practice. + msg := err.Error() + for _, sub := range []string{"EOF", "broken pipe", "connection reset", "i/o timeout"} { + if strings.Contains(msg, sub) { + return true + } + } + return false +} diff --git a/pkg/datastore/target/netconf/transport_error_test.go b/pkg/datastore/target/netconf/transport_error_test.go new file mode 100644 index 00000000..75785805 --- /dev/null +++ b/pkg/datastore/target/netconf/transport_error_test.go @@ -0,0 +1,84 @@ +// Copyright 2024 Nokia +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package netconf + +import ( + "errors" + "fmt" + "io" + "net" + "testing" +) + +func Test_isTransportError(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + { + name: "nil error", + err: nil, + want: false, + }, + { + name: "raw io.EOF", + err: io.EOF, + want: true, + }, + { + name: "wrapped io.EOF", + err: fmt.Errorf("reading response: %w", io.EOF), + want: true, + }, + { + name: "net.Error", + err: &net.OpError{Op: "read", Err: errors.New("connection reset by peer")}, + want: true, + }, + { + name: "broken pipe substring", + err: errors.New("write: broken pipe"), + want: true, + }, + { + name: "connection reset substring", + err: errors.New("read: connection reset by peer"), + want: true, + }, + { + name: "i/o timeout substring", + err: errors.New("read: i/o timeout"), + want: true, + }, + { + name: "literal EOF substring", + err: errors.New("unexpected EOF while parsing"), + want: true, + }, + { + name: "unrelated error", + err: errors.New("invalid value"), + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isTransportError(tt.err); got != tt.want { + t.Errorf("isTransportError(%v) = %v, want %v", tt.err, got, tt.want) + } + }) + } +} From 10606bb0e47b8c2f0d5ff7cf631ebc5bdad1af47 Mon Sep 17 00:00:00 2001 From: steiler Date: Fri, 4 Sep 2026 14:39:58 +0200 Subject: [PATCH 2/4] netconf: record ADRs and CONTEXT.md for transport-error decisions - ADR 0001: normalize driver-library errors (scrapligo's util.ErrConnectionError) to io.EOF at the driver/scrapligo adapter boundary, not in nc.go, so isTransportError stays driver-agnostic. - ADR 0002: deliberately exclude scrapligo's util.ErrTimeoutError from transport-error classification, to avoid config-server retrying transactions against slow-but-alive devices. - CONTEXT.md: document the netconf target and note that ErrNotConnected is intentionally reused for both the pre-flight (never connected) and mid-flight (transport dropped during an in-progress RPC) scenarios. --- pkg/datastore/target/netconf/CONTEXT.md | 11 +++++++++++ ...001-normalize-driver-errors-at-adapter-boundary.md | 9 +++++++++ ...de-timeouts-from-transport-error-classification.md | 9 +++++++++ 3 files changed, 29 insertions(+) create mode 100644 pkg/datastore/target/netconf/CONTEXT.md create mode 100644 pkg/datastore/target/netconf/docs/adr/0001-normalize-driver-errors-at-adapter-boundary.md create mode 100644 pkg/datastore/target/netconf/docs/adr/0002-exclude-timeouts-from-transport-error-classification.md diff --git a/pkg/datastore/target/netconf/CONTEXT.md b/pkg/datastore/target/netconf/CONTEXT.md new file mode 100644 index 00000000..057c1470 --- /dev/null +++ b/pkg/datastore/target/netconf/CONTEXT.md @@ -0,0 +1,11 @@ +# NETCONF target + +Implements the `Target`/`Driver` interfaces (see `pkg/datastore/target/`) for southbound NETCONF devices, translating `sdcpb` get/set requests into NETCONF RPCs (`get-config`, `edit-config`, `commit`, `discard-changes`) and back. + +## Language + +**Transport error**: +A failure of the underlying connection to the device *during* an in-flight RPC (e.g. the device restarts mid-`edit-config`) — detected by `isTransportError` — as opposed to a semantic/validation error the device itself returned (e.g. a rejected `edit-config`). Only transport errors trigger `Close`+`reconnect` and get wrapped in `ErrNotConnected`. + +**ErrNotConnected** (defined in `pkg/datastore/target/types`, not this package): +Within this context, `ErrNotConnected` is deliberately reused for two distinct scenarios under one sentinel: a **pre-flight** check (the target was never connected when a request started, via `Status().Err()`) and a **mid-flight** transport error (the target *was* connected but the transport dropped during an in-progress RPC, via `isTransportError`). Both are wrapped the same way so `translateInternalToGrpcError` maps either to `codes.Unavailable` and config-server's retry logic treats them identically — this is an intentional unification, not an accidental conflation of two different states. diff --git a/pkg/datastore/target/netconf/docs/adr/0001-normalize-driver-errors-at-adapter-boundary.md b/pkg/datastore/target/netconf/docs/adr/0001-normalize-driver-errors-at-adapter-boundary.md new file mode 100644 index 00000000..4d55c598 --- /dev/null +++ b/pkg/datastore/target/netconf/docs/adr/0001-normalize-driver-errors-at-adapter-boundary.md @@ -0,0 +1,9 @@ +--- +Status: accepted +--- + +# Normalize driver library errors to io.EOF/net.Error at the driver adapter boundary, not in nc.go + +`nc.go`'s `isTransportError` classifies transport-level connection drops using only the standard `io`/`net` error vocabulary (`io.EOF`, `net.Error`), so it stays agnostic of which concrete `Driver` implementation is in use. scrapligo (the only current `Driver` implementation, in `driver/scrapligo/`) does not preserve that vocabulary: a real mid-flight transport `EOF` gets swallowed by scrapligo's own channel read loop and re-surfaces as scrapligo's opaque `util.ErrConnectionError` sentinel. + +We considered checking `errors.Is(err, scrapligoutil.ErrConnectionError)` directly inside `nc.go`, but decided instead to normalize it to `io.EOF` inside `driver/scrapligo/scrapligo.go` (see `normalizeTransportError`), at the point the error crosses the `Driver` interface. This keeps scrapligo-specific error knowledge contained to its own adapter, consistent with the `Driver` interface abstraction: `nc.go` never needs to import or know about a specific driver library's error types, even as more `Driver` implementations are added. diff --git a/pkg/datastore/target/netconf/docs/adr/0002-exclude-timeouts-from-transport-error-classification.md b/pkg/datastore/target/netconf/docs/adr/0002-exclude-timeouts-from-transport-error-classification.md new file mode 100644 index 00000000..6fa5f1fe --- /dev/null +++ b/pkg/datastore/target/netconf/docs/adr/0002-exclude-timeouts-from-transport-error-classification.md @@ -0,0 +1,9 @@ +--- +Status: accepted +--- + +# Exclude scrapligo's timeout sentinel from transport-error classification + +`isTransportError`/`normalizeTransportError` treat a connection drop (`io.EOF`, `net.Error`, or scrapligo's `util.ErrConnectionError`) as a mid-flight transport error that triggers `t.Close`+`reconnect` and gets wrapped in `targettypes.ErrNotConnected`, which `translateInternalToGrpcError` maps to `codes.Unavailable` so config-server retries the transaction. + +We deliberately did **not** extend this to scrapligo's `util.ErrTimeoutError` (channel/write/read op timeouts), even though a hung operation against a dead socket can look similar to a dropped one. A timeout can just as easily mean a slow-but-alive device (e.g. a large `commit` under load), and misclassifying that as "not connected" would make config-server retry transactions against devices that are still processing the original one — a different, worse failure mode than the one this fix addresses. If a real-world case shows scrapligo timeouts reliably correlating with dead connections, revisit this by distinguishing "timeout because socket is dead" from "timeout because device is slow" rather than lumping all timeouts into `ErrNotConnected`. From b39fb7835e0387e61a512fe5f52f594b2d4a31df Mon Sep 17 00:00:00 2001 From: steiler Date: Fri, 4 Sep 2026 16:09:07 +0200 Subject: [PATCH 3/4] chore: retrigger CI to pick up Pairs-with tag From 074a2eaad3e0cf093349f60971aa6476b6fb498f Mon Sep 17 00:00:00 2001 From: Markus Vahlenkamp Date: Mon, 14 Sep 2026 09:25:50 +0200 Subject: [PATCH 4/4] Delete pkg/datastore/target/netconf/docs/adr directory cleanup --- .../0001-normalize-driver-errors-at-adapter-boundary.md | 9 --------- ...clude-timeouts-from-transport-error-classification.md | 9 --------- 2 files changed, 18 deletions(-) delete mode 100644 pkg/datastore/target/netconf/docs/adr/0001-normalize-driver-errors-at-adapter-boundary.md delete mode 100644 pkg/datastore/target/netconf/docs/adr/0002-exclude-timeouts-from-transport-error-classification.md diff --git a/pkg/datastore/target/netconf/docs/adr/0001-normalize-driver-errors-at-adapter-boundary.md b/pkg/datastore/target/netconf/docs/adr/0001-normalize-driver-errors-at-adapter-boundary.md deleted file mode 100644 index 4d55c598..00000000 --- a/pkg/datastore/target/netconf/docs/adr/0001-normalize-driver-errors-at-adapter-boundary.md +++ /dev/null @@ -1,9 +0,0 @@ ---- -Status: accepted ---- - -# Normalize driver library errors to io.EOF/net.Error at the driver adapter boundary, not in nc.go - -`nc.go`'s `isTransportError` classifies transport-level connection drops using only the standard `io`/`net` error vocabulary (`io.EOF`, `net.Error`), so it stays agnostic of which concrete `Driver` implementation is in use. scrapligo (the only current `Driver` implementation, in `driver/scrapligo/`) does not preserve that vocabulary: a real mid-flight transport `EOF` gets swallowed by scrapligo's own channel read loop and re-surfaces as scrapligo's opaque `util.ErrConnectionError` sentinel. - -We considered checking `errors.Is(err, scrapligoutil.ErrConnectionError)` directly inside `nc.go`, but decided instead to normalize it to `io.EOF` inside `driver/scrapligo/scrapligo.go` (see `normalizeTransportError`), at the point the error crosses the `Driver` interface. This keeps scrapligo-specific error knowledge contained to its own adapter, consistent with the `Driver` interface abstraction: `nc.go` never needs to import or know about a specific driver library's error types, even as more `Driver` implementations are added. diff --git a/pkg/datastore/target/netconf/docs/adr/0002-exclude-timeouts-from-transport-error-classification.md b/pkg/datastore/target/netconf/docs/adr/0002-exclude-timeouts-from-transport-error-classification.md deleted file mode 100644 index 6fa5f1fe..00000000 --- a/pkg/datastore/target/netconf/docs/adr/0002-exclude-timeouts-from-transport-error-classification.md +++ /dev/null @@ -1,9 +0,0 @@ ---- -Status: accepted ---- - -# Exclude scrapligo's timeout sentinel from transport-error classification - -`isTransportError`/`normalizeTransportError` treat a connection drop (`io.EOF`, `net.Error`, or scrapligo's `util.ErrConnectionError`) as a mid-flight transport error that triggers `t.Close`+`reconnect` and gets wrapped in `targettypes.ErrNotConnected`, which `translateInternalToGrpcError` maps to `codes.Unavailable` so config-server retries the transaction. - -We deliberately did **not** extend this to scrapligo's `util.ErrTimeoutError` (channel/write/read op timeouts), even though a hung operation against a dead socket can look similar to a dropped one. A timeout can just as easily mean a slow-but-alive device (e.g. a large `commit` under load), and misclassifying that as "not connected" would make config-server retry transactions against devices that are still processing the original one — a different, worse failure mode than the one this fix addresses. If a real-world case shows scrapligo timeouts reliably correlating with dead connections, revisit this by distinguishing "timeout because socket is dead" from "timeout because device is slow" rather than lumping all timeouts into `ErrNotConnected`.