-
Notifications
You must be signed in to change notification settings - Fork 14
fix: remove implicit fallback behavior #2759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 13 commits
553bf94
500ed1d
987f02e
904416b
3796605
3e25844
6253aa9
f111e63
bc2c528
7c8996a
f2ccea0
bc959a3
34a93a2
741f68a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,12 +45,30 @@ func (c *ConnectionManager) CreateOutboundConn( | |
| if c.config.OutboundSourcePort > 0 { | ||
| // Setup connection to use our listening port as the source port | ||
| // This is required for peer sharing to be useful | ||
| clientAddr, _ = net.ResolveTCPAddr( | ||
| resolvedAddr, resolveErr := net.ResolveTCPAddr( | ||
| "tcp", | ||
| fmt.Sprintf(":%d", c.config.OutboundSourcePort), | ||
| ) | ||
| dialer.LocalAddr = clientAddr | ||
| dialer.Control = socketControl | ||
| if resolveErr != nil { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now observable, but it is still a fallback: an invalid configured source port logs and then dials without source-port reuse. Since source-port reuse is required for peer sharing to be useful, I think this should fail during config validation/startup, or be gated behind an explicit tolerant mode. Logging and continuing is better than silent behavior, but it does not satisfy the fail-fast part of #1649. |
||
| // internal/config.ValidateRuntimeConfig rejects an | ||
| // out-of-range relayPort (which OutboundSourcePort is | ||
| // derived from) once CLI/env/YAML have all merged, so this | ||
| // should be unreachable via normal configuration. Kept as a | ||
| // backstop for direct API callers of this package that set | ||
| // OutboundSourcePort without going through internal/config | ||
| // at all. | ||
| c.config.Logger.Warn( | ||
| "outbound: failed to resolve source port, dialing without source-port reuse", | ||
| "role", "client", | ||
| "address", address, | ||
| "outbound_source_port", c.config.OutboundSourcePort, | ||
| "error", resolveErr, | ||
| ) | ||
| } else { | ||
| clientAddr = resolvedAddr | ||
| dialer.LocalAddr = clientAddr | ||
| dialer.Control = socketControl | ||
| } | ||
| } | ||
| c.config.Logger.Debug( | ||
| "establishing TCP connection to: "+address, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| // Copyright 2026 Blink Labs Software | ||
| // | ||
| // 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 connmanager | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "log/slog" | ||
| "strings" | ||
| "testing" | ||
| "time" | ||
| ) | ||
|
|
||
| // TestCreateOutboundConn_InvalidSourcePortLogsWarning verifies that an | ||
| // out-of-range OutboundSourcePort (which makes net.ResolveTCPAddr fail) | ||
| // is logged instead of silently disabling source-port reuse and dialing | ||
| // without a source-port bind. | ||
| func TestCreateOutboundConn_InvalidSourcePortLogsWarning(t *testing.T) { | ||
| var logBuf bytes.Buffer | ||
| cm := NewConnectionManager(ConnectionManagerConfig{ | ||
| Logger: slog.New( | ||
| slog.NewTextHandler(&logBuf, nil), | ||
| ), | ||
| // Out of the valid 16-bit TCP port range, so | ||
| // net.ResolveTCPAddr fails. | ||
| OutboundSourcePort: 100000, | ||
| }) | ||
|
|
||
| ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) | ||
| defer cancel() | ||
|
|
||
| // The dial itself is expected to fail (nothing listening), but the | ||
| // resolve failure must be logged rather than silently ignored. | ||
| _, _ = cm.CreateOutboundConn(ctx, "127.0.0.1:1") | ||
|
|
||
| logOutput := logBuf.String() | ||
| if !strings.Contains(logOutput, "failed to resolve source port") { | ||
| t.Errorf( | ||
| "expected a source-port resolve warning in logs, got: %s", | ||
| logOutput, | ||
| ) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| // Copyright 2026 Blink Labs Software | ||
| // | ||
| // 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 connmanager | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "log/slog" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/blinklabs-io/dingo/event" | ||
| ) | ||
|
|
||
| // TestHandleConnectionRecycleRequestedEvent_UnexpectedTypeLogged verifies | ||
| // that a mismatched event payload is logged rather than silently dropped: | ||
| // a recycle request lost with no trace is exactly the kind of swallowed | ||
| // failure this handler used to allow. | ||
| func TestHandleConnectionRecycleRequestedEvent_UnexpectedTypeLogged(t *testing.T) { | ||
| var logBuf bytes.Buffer | ||
| cm := NewConnectionManager(ConnectionManagerConfig{ | ||
| Logger: slog.New(slog.NewTextHandler(&logBuf, nil)), | ||
| }) | ||
|
|
||
| // Wrong payload type: the handler asserts ConnectionRecycleRequestedEvent. | ||
| cm.HandleConnectionRecycleRequestedEvent(event.Event{Data: "not-a-recycle-event"}) | ||
|
|
||
| logOutput := logBuf.String() | ||
| if !strings.Contains(logOutput, "unexpected event data type") { | ||
| t.Errorf( | ||
| "expected an unexpected-event-type warning in logs, got: %s", | ||
| logOutput, | ||
| ) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,13 @@ import ( | |
| "github.com/prometheus/client_golang/prometheus" | ||
| ) | ||
|
|
||
| // DefaultConfig is the source of truth for New's plugin defaulting below, | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| // used by direct callers of this package (library usage, tests) that | ||
| // don't go through internal/config. The CLI path resolves BlobPlugin/ | ||
| // MetadataPlugin earlier via the internal/config.DefaultBlobPlugin/ | ||
| // DefaultMetadataPlugin constants, which are kept in sync with these | ||
| // values by TestDatabaseDefaultsMatchInternalConfigDefaults in | ||
| // internal/config. | ||
| var DefaultConfig = &Config{ | ||
| BlobPlugin: "badger", | ||
| DataDir: ".dingo", | ||
|
|
@@ -164,9 +171,21 @@ func New( | |
| // Apply defaults for empty fields | ||
| if configCopy.BlobPlugin == "" { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This still applies implicit plugin defaults when the caller passes an empty plugin name. The debug log helps, but #1649 specifically calls out database plugin initialization. If defaulting is intended, I would prefer it happen once in config defaulting with a clear source of truth; the database constructor should probably reject an empty plugin after config has been resolved, or at least this should be listed as remaining fallback work rather than part of a closed audit. |
||
| configCopy.BlobPlugin = DefaultConfig.BlobPlugin | ||
| if configCopy.Logger != nil { | ||
| configCopy.Logger.Debug( | ||
| "no blob plugin configured, using default", | ||
| "plugin", configCopy.BlobPlugin, | ||
| ) | ||
| } | ||
| } | ||
| if configCopy.MetadataPlugin == "" { | ||
| configCopy.MetadataPlugin = DefaultConfig.MetadataPlugin | ||
| if configCopy.Logger != nil { | ||
| configCopy.Logger.Debug( | ||
| "no metadata plugin configured, using default", | ||
| "plugin", configCopy.MetadataPlugin, | ||
| ) | ||
| } | ||
| } | ||
| if configCopy.StorageMode == "" { | ||
| configCopy.StorageMode = types.StorageModeCore | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // Copyright 2026 Blink Labs Software | ||
| // | ||
| // 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 database | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "log/slog" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TestNew_DefaultPluginSelectionIsLogged verifies that silently defaulting | ||
| // BlobPlugin/MetadataPlugin to badger/sqlite when unset is still observable | ||
| // via a debug log, rather than happening with zero trace. | ||
| func TestNew_DefaultPluginSelectionIsLogged(t *testing.T) { | ||
| var logBuf bytes.Buffer | ||
| config := &Config{ | ||
| DataDir: "", // in-memory | ||
| Logger: slog.New(slog.NewTextHandler(&logBuf, &slog.HandlerOptions{ | ||
| Level: slog.LevelDebug, | ||
| })), | ||
| } | ||
| db, err := New(config) | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { | ||
| require.NoError(t, db.Close()) | ||
| }) | ||
|
|
||
| logOutput := logBuf.String() | ||
| if !strings.Contains(logOutput, "no blob plugin configured, using default") { | ||
| t.Errorf("expected default blob plugin log, got: %s", logOutput) | ||
| } | ||
| if !strings.Contains(logOutput, "no metadata plugin configured, using default") { | ||
| t.Errorf("expected default metadata plugin log, got: %s", logOutput) | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.