Improve Bond home-connector network failure handling#277
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughAdds a Bond request wrapper that retries between stored Changes
Sequence DiagramsequenceDiagram
participant Client
participant BondAdapter as Bond Adapter
participant Retry as withBondBridgeRequest
participant Local as Stored .local Host
participant IP as Discovered IP
participant Sentry
Client->>BondAdapter: request (e.g., getDeviceState)
BondAdapter->>Retry: execute request (prioritized URLs)
Retry->>Local: attempt request to stored .local URL
alt Network/DNS failure
Local-->>Retry: network/DNS error
Retry->>IP: retry with discovered IP URL
alt IP succeeds
IP-->>Retry: success response
Retry-->>BondAdapter: return result
BondAdapter-->>Client: success
else IP fails
IP-->>Retry: failure
Retry-->>BondAdapter: throw BondRequestError (with homeConnectorCaptureContext)
BondAdapter->>Sentry: captureHomeConnectorException(error, context)
Sentry-->>BondAdapter: recorded
BondAdapter-->>Client: error
end
else Non-network HTTP error
Local-->>Retry: HTTP error (e.g., 401)
Retry-->>BondAdapter: throw BondRequestError (preserve status/body)
BondAdapter->>Sentry: captureHomeConnectorException(error, context)
BondAdapter-->>Client: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🔎 Preview deployed: https://kody-pr-277.kentcdodds.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
packages/home-connector/src/adapters/bond/index.ts (2)
197-235: Minor: computeformatBondFailureReason(input.error)once.It's evaluated twice — once for the user-facing
errorMessage(line 207) and again forextra.bondFailureReason(line 231). They must produce identical output anyway, so caching avoids re-walking the cause chain and removes any risk of drift if the formatter is later changed.♻️ Tiny tweak
const connection = getBondBridgeConnectionContext(input.bridge) const guidance = connection.host.endsWith('.local') ? ' The stored Bond host ends in .local, ...' : ' Verify the bridge host/IP is still reachable ...' - const errorMessage = `Bond bridge "${input.bridge.bridgeId}" could not be reached while trying to ${input.operation} at ${input.baseUrlsTried.join(', ')}. ${formatBondFailureReason(input.error)}.${guidance}` + const failureReason = formatBondFailureReason(input.error) + const errorMessage = `Bond bridge "${input.bridge.bridgeId}" could not be reached while trying to ${input.operation} at ${input.baseUrlsTried.join(', ')}. ${failureReason}.${guidance}` @@ extra: { bondOperation: input.operation, bondBaseUrlsTried: input.baseUrlsTried, - bondFailureReason: formatBondFailureReason(input.error), + bondFailureReason: failureReason, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home-connector/src/adapters/bond/index.ts` around lines 197 - 235, In createBondActionableError, compute formatBondFailureReason(input.error) once into a local const (e.g. bondFailureReason) and reuse that variable when building errorMessage and when setting extra.bondFailureReason so the formatter is only invoked once and both places use the identical string; update references to formatBondFailureReason(input.error) in the errorMessage construction and in wrappedError.homeConnectorCaptureContext.extra.bondFailureReason to use the new bondFailureReason variable.
148-195: Refactor: extract a singlegetCauseMessage(error)helper.
isBondNetworkFailure(163-171) andformatBondFailureReason(183-191) duplicate the sameerror.causeladder (Error | string | object-with-message | other). The duplicated nested ternaries also have indentation that doesn't align with the parent branches (: ''and: nullvisually float), making the precedence harder to verify at a glance. Pulling the cause-extraction into one helper drops the duplication and clarifies intent.♻️ Suggested helper
+function getErrorCauseMessage(error: Error): string | null { + const cause = error.cause + if (cause instanceof Error) return cause.message + if (typeof cause === 'string') return cause + if (cause && typeof cause === 'object') { + const message = (cause as { message?: unknown }).message + return message == null ? null : String(message) + } + return null +} + function isBondNetworkFailure(error: unknown) { if (!(error instanceof Error)) { return false } const message = error.message.toLowerCase() if ( message.includes('fetch failed') || message.includes('enotfound') || message.includes('eai_again') || message.includes('econnrefused') || message.includes('ehostunreach') || message.includes('etimedout') ) { return true } - const causeMessage = - error.cause instanceof Error - ? error.cause.message.toLowerCase() - : typeof error.cause === 'string' - ? error.cause.toLowerCase() - : error.cause && typeof error.cause === 'object' - ? String((error.cause as { message?: unknown }).message ?? '') - .toLowerCase() - : '' + const causeMessage = (getErrorCauseMessage(error) ?? '').toLowerCase() return ( causeMessage.includes('enotfound') || causeMessage.includes('eai_again') || causeMessage.includes('econnrefused') || causeMessage.includes('ehostunreach') || causeMessage.includes('etimedout') || causeMessage.includes('getaddrinfo') ) } function formatBondFailureReason(error: unknown) { if (error instanceof Error) { - const causeMessage = - error.cause instanceof Error - ? error.cause.message - : typeof error.cause === 'string' - ? error.cause - : error.cause && typeof error.cause === 'object' - ? String((error.cause as { message?: unknown }).message ?? '') - : null + const causeMessage = getErrorCauseMessage(error) return causeMessage ? `${error.message}; cause=${causeMessage}` : error.message } return String(error) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home-connector/src/adapters/bond/index.ts` around lines 148 - 195, Extract a single helper function getCauseMessage(error: unknown): string | null that encapsulates the repeated logic for reading error.cause (handling Error, string, object-with-message, and other cases) and return the cause message lowercased where appropriate; then replace the duplicated ladders in isBondNetworkFailure and formatBondFailureReason with calls to getCauseMessage(error) (use the lowercased result in isBondNetworkFailure when checking contains like 'enotfound'/'etimedout' and use the raw string/nullable result in formatBondFailureReason to build the "message; cause=..." string). Ensure getCauseMessage returns null when no meaningful cause exists and remove the nested ternaries from isBondNetworkFailure and formatBondFailureReason so both read the cause via the new helper.packages/home-connector/src/sentry.node.test.ts (1)
176-178: Test isolation:isEnabledleft astrueafter this test.
sentryMock.isEnabled.mockReturnValue(true)is set here but never restored. Existing tests above this one defensively resetisEnabledper test, so they're fine, but any future test appended below that forgets to reset will inherittrueand silently exercise different code paths. Consider scoping withafterEach(() => sentryMock.isEnabled.mockReturnValue(false))to keep the suite resilient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home-connector/src/sentry.node.test.ts` around lines 176 - 178, The test captureHomeConnectorException sets sentryMock.isEnabled.mockReturnValue(true) but does not restore it, risking test leakage; update the suite to reset this mock after each test (e.g., add an afterEach that calls sentryMock.isEnabled.mockReturnValue(false) or restores the original mock implementation) so sentryMock.isEnabled is consistently false by default and tests remain isolated.packages/home-connector/src/sentry.ts (2)
53-73: Heads up:contextsis shallow-cloned only at the outer key level.
{ ...captureContext.contexts }clones the top-level keys, but each inner record (e.g.contexts.bond) is still shared by reference with the error. The current test only checks reference inequality at the top level, so it passes, but if any downstream code (today or later) mutates an inner context record after capture, the mutation will leak back to the original error. Consider a one-level-deeper clone for full isolation:Optional defensive clone
- ...(captureContext.contexts - ? { contexts: { ...captureContext.contexts } } - : {}), + ...(captureContext.contexts + ? { + contexts: Object.fromEntries( + Object.entries(captureContext.contexts).map(([key, value]) => [ + key, + { ...value }, + ]), + ), + } + : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home-connector/src/sentry.ts` around lines 53 - 73, getHomeConnectorErrorCaptureContext currently shallow-clones captureContext.contexts only at the top level so inner context objects remain shared with the original error; update getHomeConnectorErrorCaptureContext (and the handling of HomeConnectorErrorWithCaptureContext.captureContext) to perform a one-level-deeper defensive clone for contexts (i.e., for each key in captureContext.contexts clone its value object) before returning, so mutations to returned contexts won't mutate the original error; similarly consider doing the same for captureContext.extra if it can contain nested objects.
144-149: Sentry'sContextstype allowsundefinedvalues per key — verify the cast is sound.
captureContext.contextsfromParameters<typeof Sentry.captureException>[1]is typed asContexts(Record<string, Context | undefined>in@sentry/node). The cast toRecord<string, Record<string, unknown>> | undefineddrops that| undefinedfor entries.mergeContextRecordshappens to tolerateundefinedvalues via the?? {}fallback inside the loop, but the type assertion is still slightly loose. Worth tightening the parameter type ofmergeContextRecordsto mirror Sentry’s shape, or spelling out the cast explicitly.`@sentry/node` 10.45.0 Contexts type Record<string, Context | undefined>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home-connector/src/sentry.ts` around lines 144 - 149, The cast of captureContext.contexts to Record<string, Record<string, unknown>> | undefined is too loose because Sentry's Contexts is Record<string, Context | undefined>; update mergeContextRecords' parameter type to accept Record<string, Context | undefined> | undefined (or a dedicated SentryContexts type) and change the call site to pass captureContext.contexts without the unsafe cast; ensure mergeContextRecords treats undefined values per key (it already uses ?? {} inside the loop) so only the type signature needs tightening to mirror Sentry's shape and remove the assertion.packages/home-connector/src/adapters/bond/index.node.test.ts (2)
30-44: Nit: prefer the standardError(..., { cause })constructor form.
new TypeError('fetch failed', { cause: {...} })is the idiomatic shape Node's undici uses when emitting this exact error, so the synthetic error matches production more faithfully (e.g.,causeis non-enumerable in real undici failures).Object.definePropertyworks, but the constructor option is shorter and slightly more realistic.♻️ Suggested simplification
-function createDnsFetchError(message = 'getaddrinfo ENOTFOUND zpgi01117.local') { - const error = new TypeError('fetch failed') - Object.defineProperty(error, 'cause', { - value: { - code: 'ENOTFOUND', - errno: -3008, - syscall: 'getaddrinfo', - hostname: 'zpgi01117.local', - message, - }, - enumerable: true, - configurable: true, - }) - return error -} +function createDnsFetchError(message = 'getaddrinfo ENOTFOUND zpgi01117.local') { + return new TypeError('fetch failed', { + cause: { + code: 'ENOTFOUND', + errno: -3008, + syscall: 'getaddrinfo', + hostname: 'zpgi01117.local', + message, + }, + }) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home-connector/src/adapters/bond/index.node.test.ts` around lines 30 - 44, The test helper createDnsFetchError constructs a synthetic fetch error by manually defining a cause property; instead use the standard Error constructor form to match Node/undici shape by creating the error with new TypeError('fetch failed', { cause: { code: 'ENOTFOUND', errno: -3008, syscall: 'getaddrinfo', hostname: 'zpgi01117.local', message } }), so replace the Object.defineProperty block inside createDnsFetchError with the constructor { cause } form to make the error non-enumerable and more realistic.
155-160: Optional: assert both substrings against a single rejection.Calling
bond.getDeviceState('BONDTEST2', 'mockdev1')twice runs the full request pipeline (and the failing fetch) twice just to check two substrings of the same error. Capturing the rejection once is cheaper and removes the implicit assumption that a second invocation rejects identically.♻️ Suggested consolidation
- await expect(bond.getDeviceState('BONDTEST2', 'mockdev1')).rejects.toThrow( - 'Bond bridge "BONDTEST2" could not be reached while trying to fetch device mockdev1 state at http://zpgi01117.local', - ) - await expect(bond.getDeviceState('BONDTEST2', 'mockdev1')).rejects.toThrow( - 'If this connector runs in a NAS/container without mDNS, update the bridge host to a stable IP', - ) + const rejection = await bond + .getDeviceState('BONDTEST2', 'mockdev1') + .catch((error: unknown) => error) + expect(rejection).toBeInstanceOf(Error) + const message = (rejection as Error).message + expect(message).toContain( + 'Bond bridge "BONDTEST2" could not be reached while trying to fetch device mockdev1 state at http://zpgi01117.local', + ) + expect(message).toContain( + 'If this connector runs in a NAS/container without mDNS, update the bridge host to a stable IP', + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home-connector/src/adapters/bond/index.node.test.ts` around lines 155 - 160, Instead of calling bond.getDeviceState twice, capture the single promise and assert both substrings against that same rejection; e.g. create const p = bond.getDeviceState('BONDTEST2', 'mockdev1') and then await expect(p).rejects.toThrow('Bond bridge "BONDTEST2" could not be reached ...') and await expect(p).rejects.toThrow('If this connector runs in a NAS/container without mDNS, update the bridge host to a stable IP'), so the request pipeline only runs once while still verifying both messages from the error thrown by bond.getDeviceState.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/home-connector/src/adapters/bond/index.node.test.ts`:
- Around line 30-44: The test helper createDnsFetchError constructs a synthetic
fetch error by manually defining a cause property; instead use the standard
Error constructor form to match Node/undici shape by creating the error with new
TypeError('fetch failed', { cause: { code: 'ENOTFOUND', errno: -3008, syscall:
'getaddrinfo', hostname: 'zpgi01117.local', message } }), so replace the
Object.defineProperty block inside createDnsFetchError with the constructor {
cause } form to make the error non-enumerable and more realistic.
- Around line 155-160: Instead of calling bond.getDeviceState twice, capture the
single promise and assert both substrings against that same rejection; e.g.
create const p = bond.getDeviceState('BONDTEST2', 'mockdev1') and then await
expect(p).rejects.toThrow('Bond bridge "BONDTEST2" could not be reached ...')
and await expect(p).rejects.toThrow('If this connector runs in a NAS/container
without mDNS, update the bridge host to a stable IP'), so the request pipeline
only runs once while still verifying both messages from the error thrown by
bond.getDeviceState.
In `@packages/home-connector/src/adapters/bond/index.ts`:
- Around line 197-235: In createBondActionableError, compute
formatBondFailureReason(input.error) once into a local const (e.g.
bondFailureReason) and reuse that variable when building errorMessage and when
setting extra.bondFailureReason so the formatter is only invoked once and both
places use the identical string; update references to
formatBondFailureReason(input.error) in the errorMessage construction and in
wrappedError.homeConnectorCaptureContext.extra.bondFailureReason to use the new
bondFailureReason variable.
- Around line 148-195: Extract a single helper function getCauseMessage(error:
unknown): string | null that encapsulates the repeated logic for reading
error.cause (handling Error, string, object-with-message, and other cases) and
return the cause message lowercased where appropriate; then replace the
duplicated ladders in isBondNetworkFailure and formatBondFailureReason with
calls to getCauseMessage(error) (use the lowercased result in
isBondNetworkFailure when checking contains like 'enotfound'/'etimedout' and use
the raw string/nullable result in formatBondFailureReason to build the "message;
cause=..." string). Ensure getCauseMessage returns null when no meaningful cause
exists and remove the nested ternaries from isBondNetworkFailure and
formatBondFailureReason so both read the cause via the new helper.
In `@packages/home-connector/src/sentry.node.test.ts`:
- Around line 176-178: The test captureHomeConnectorException sets
sentryMock.isEnabled.mockReturnValue(true) but does not restore it, risking test
leakage; update the suite to reset this mock after each test (e.g., add an
afterEach that calls sentryMock.isEnabled.mockReturnValue(false) or restores the
original mock implementation) so sentryMock.isEnabled is consistently false by
default and tests remain isolated.
In `@packages/home-connector/src/sentry.ts`:
- Around line 53-73: getHomeConnectorErrorCaptureContext currently
shallow-clones captureContext.contexts only at the top level so inner context
objects remain shared with the original error; update
getHomeConnectorErrorCaptureContext (and the handling of
HomeConnectorErrorWithCaptureContext.captureContext) to perform a
one-level-deeper defensive clone for contexts (i.e., for each key in
captureContext.contexts clone its value object) before returning, so mutations
to returned contexts won't mutate the original error; similarly consider doing
the same for captureContext.extra if it can contain nested objects.
- Around line 144-149: The cast of captureContext.contexts to Record<string,
Record<string, unknown>> | undefined is too loose because Sentry's Contexts is
Record<string, Context | undefined>; update mergeContextRecords' parameter type
to accept Record<string, Context | undefined> | undefined (or a dedicated
SentryContexts type) and change the call site to pass captureContext.contexts
without the unsafe cast; ensure mergeContextRecords treats undefined values per
key (it already uses ?? {} inside the loop) so only the type signature needs
tightening to mirror Sentry's shape and remove the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c690e49c-a8ea-4f8b-a685-320f98500963
📒 Files selected for processing (4)
packages/home-connector/src/adapters/bond/index.node.test.tspackages/home-connector/src/adapters/bond/index.tspackages/home-connector/src/sentry.node.test.tspackages/home-connector/src/sentry.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/home-connector/src/sentry.node.test.ts (1)
17-19: Prefer test-local cleanup over a suite-levelafterEach.This hook only undoes per-test mutation of
sentryMock.isEnabled, so moving that reset into the tests that change it will satisfy the current lint rule and keep isolation explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home-connector/src/sentry.node.test.ts` around lines 17 - 19, The suite-level afterEach that resets sentryMock.isEnabled should be removed; instead, in any test that mutates sentryMock.isEnabled (e.g., tests that call sentryMock.isEnabled.mockReturnValue(true)), restore it to the desired default within that test (use a local reset at the end of the test or a try/finally to call sentryMock.isEnabled.mockReturnValue(false)). Remove the afterEach block and add per-test cleanup inside each test that changes sentryMock.isEnabled to keep isolation explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/home-connector/src/adapters/bond/index.ts`:
- Around line 241-271: withBondBridgeRequest currently retries all requests
(including non-idempotent writes like bondInvokeDeviceAction and
bondInvokeGroupAction) across multiple base URLs, which can double-actuate
devices; change the logic so it does not automatically retry writes: detect
whether the incoming operation is a write (e.g., by checking input.operation or
adding a new flag on the input such as isIdempotent/readOnly) and if it is not
read-only, do not attempt the fallback retry path—immediately rethrow
non-network errors and only consider retrying for safe, proven-local failures
(DNS/lookup errors) on read-only calls; update the call sites
(bondInvokeDeviceAction/bondInvokeGroupAction) to either pass a
readOnly/isIdempotent flag when safe to retry or leave it unset for writes so
withBondBridgeRequest will not retry them.
- Around line 97-120: getBridgeDiscoveredAddress currently returns the first
non-empty discovery string but must restrict fallbacks to actual IPv4 addresses
only; update getBridgeDiscoveredAddress to validate candidates (rawAddress,
mdnsAddress, and entries from mdnsAddresses) with an IPv4 check (e.g., a simple
regex or netmask parse) and ignore mdns hostnames or IPv6 strings so only true
IPv4 addresses are returned (otherwise return null); apply the same IPv4-only
validation to the other equivalent discovery branch referenced in the diff (the
mdns/addresses fallback) so both places use the same IPv4 validation logic.
---
Nitpick comments:
In `@packages/home-connector/src/sentry.node.test.ts`:
- Around line 17-19: The suite-level afterEach that resets sentryMock.isEnabled
should be removed; instead, in any test that mutates sentryMock.isEnabled (e.g.,
tests that call sentryMock.isEnabled.mockReturnValue(true)), restore it to the
desired default within that test (use a local reset at the end of the test or a
try/finally to call sentryMock.isEnabled.mockReturnValue(false)). Remove the
afterEach block and add per-test cleanup inside each test that changes
sentryMock.isEnabled to keep isolation explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d744e879-69cc-45f2-aae4-0927adf38219
📒 Files selected for processing (4)
packages/home-connector/src/adapters/bond/index.node.test.tspackages/home-connector/src/adapters/bond/index.tspackages/home-connector/src/sentry.node.test.tspackages/home-connector/src/sentry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/home-connector/src/adapters/bond/index.node.test.ts
Comment on lines
+97
to
+120
| function getBridgeDiscoveredAddress(bridge: BondPersistedBridge) { | ||
| const rawAddress = bridge.rawDiscovery?.['address'] | ||
| if (typeof rawAddress === 'string' && rawAddress.trim()) { | ||
| return rawAddress.trim() | ||
| } | ||
| const mdns = bridge.rawDiscovery?.['mdns'] | ||
| if (mdns && typeof mdns === 'object' && !Array.isArray(mdns)) { | ||
| const mdnsRecord = mdns as Record<string, unknown> | ||
| const mdnsAddress = mdnsRecord['address'] | ||
| if (typeof mdnsAddress === 'string' && mdnsAddress.trim()) { | ||
| return mdnsAddress.trim() | ||
| } | ||
| const mdnsAddresses = mdnsRecord['addresses'] | ||
| if (Array.isArray(mdnsAddresses)) { | ||
| const firstAddress = mdnsAddresses.find( | ||
| (entry) => typeof entry === 'string' && entry.trim(), | ||
| ) | ||
| if (typeof firstAddress === 'string') { | ||
| return firstAddress.trim() | ||
| } | ||
| } | ||
| } | ||
| return null | ||
| } |
There was a problem hiding this comment.
Restrict the fallback host to an actual IPv4 address.
The new helper returns the first non-empty discovery string, but this PR’s contract is “retry against the discovered IPv4 address.” If discovery surfaces an mDNS hostname or an IPv6 address first, the fallback can either duplicate the failing host or build an unusable base URL, so the .local recovery path still fails.
Also applies to: 136-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/home-connector/src/adapters/bond/index.ts` around lines 97 - 120,
getBridgeDiscoveredAddress currently returns the first non-empty discovery
string but must restrict fallbacks to actual IPv4 addresses only; update
getBridgeDiscoveredAddress to validate candidates (rawAddress, mdnsAddress, and
entries from mdnsAddresses) with an IPv4 check (e.g., a simple regex or netmask
parse) and ignore mdns hostnames or IPv6 strings so only true IPv4 addresses are
returned (otherwise return null); apply the same IPv4-only validation to the
other equivalent discovery branch referenced in the diff (the mdns/addresses
fallback) so both places use the same IPv4 validation logic.
Comment on lines
+241
to
+271
| async function withBondBridgeRequest<T>(input: { | ||
| bridge: BondPersistedBridge | ||
| operation: string | ||
| request: (baseUrl: string) => Promise<T> | ||
| }) { | ||
| const baseUrls = createBondBaseUrlCandidates(input.bridge) | ||
| const attemptedBaseUrls: Array<string> = [] | ||
| let lastError: unknown = null | ||
| for (let index = 0; index < baseUrls.length; index += 1) { | ||
| const baseUrl = baseUrls[index]! | ||
| attemptedBaseUrls.push(baseUrl) | ||
| try { | ||
| return await input.request(baseUrl) | ||
| } catch (error) { | ||
| lastError = error | ||
| if (!isBondNetworkFailure(error)) { | ||
| throw error | ||
| } | ||
| const canRetryWithFallback = | ||
| index === 0 && baseUrls.length > 1 | ||
| if (!canRetryWithFallback) { | ||
| break | ||
| } | ||
| } | ||
| } | ||
| throw createBondActionableError({ | ||
| bridge: input.bridge, | ||
| operation: input.operation, | ||
| error: lastError, | ||
| baseUrlsTried: attemptedBaseUrls, | ||
| }) |
There was a problem hiding this comment.
Do not automatically retry Bond action writes.
withBondBridgeRequest now wraps bondInvokeDeviceAction and bondInvokeGroupAction, so a timeout / generic transport failure can cause the same non-idempotent command to be sent twice—first via the stored host, then again via the fallback IP. That is a real double-actuation risk for shades and other devices.
Suggested direction
async function withBondBridgeRequest<T>(input: {
bridge: BondPersistedBridge
operation: string
+ allowFallbackRetry?: boolean
request: (baseUrl: string) => Promise<T>
}) {
const baseUrls = createBondBaseUrlCandidates(input.bridge)
const attemptedBaseUrls: Array<string> = []
let lastError: unknown = null
for (let index = 0; index < baseUrls.length; index += 1) {
const baseUrl = baseUrls[index]!
attemptedBaseUrls.push(baseUrl)
try {
return await input.request(baseUrl)
} catch (error) {
lastError = error
if (!isBondNetworkFailure(error)) {
throw error
}
- const canRetryWithFallback =
- index === 0 && baseUrls.length > 1
+ const canRetryWithFallback =
+ input.allowFallbackRetry === true &&
+ index === 0 &&
+ baseUrls.length > 1
if (!canRetryWithFallback) {
break
}
}
}Then opt in only from read-only call sites, or narrow retries to failures that prove the request never left the process (for example DNS resolution errors).
Also applies to: 564-575, 700-711
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/home-connector/src/adapters/bond/index.ts` around lines 241 - 271,
withBondBridgeRequest currently retries all requests (including non-idempotent
writes like bondInvokeDeviceAction and bondInvokeGroupAction) across multiple
base URLs, which can double-actuate devices; change the logic so it does not
automatically retry writes: detect whether the incoming operation is a write
(e.g., by checking input.operation or adding a new flag on the input such as
isIdempotent/readOnly) and if it is not read-only, do not attempt the fallback
retry path—immediately rethrow non-network errors and only consider retrying for
safe, proven-local failures (DNS/lookup errors) on read-only calls; update the
call sites (bondInvokeDeviceAction/bondInvokeGroupAction) to either pass a
readOnly/isIdempotent flag when safe to retry or leave it unset for writes so
withBondBridgeRequest will not retry them.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eee77ff. Configure here.
| causeMessage.includes('etimedout') || | ||
| causeMessage.includes('getaddrinfo') | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Network failure check doesn't distinguish TypeError from HTTP errors
Low Severity
isBondNetworkFailure checks error instanceof Error then matches substrings like 'fetch failed' in the message. Node.js fetch throws TypeError specifically for network errors, but bondRequestJson throws plain Error for HTTP-level failures (e.g., Bond HTTP 500 for /path: <body>). If a Bond API response body ever contains the phrase "fetch failed", the plain Error would be misclassified as a network failure, triggering an unwarranted retry against the fallback URL and wrapping the error with incorrect network-failure guidance and Sentry tags. The 'fetch failed' branch needs an error instanceof TypeError guard.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit eee77ff. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.local/network fetch failures surface actionable bridge context instead of a genericTypeError: fetch failedLikely root cause
The repeated production failures most likely come from the home-connector storing a Bond bridge hostname that ends in
.localand then trying to fetch Bond endpoints from inside a NAS/container that cannot resolve mDNS on the LAN. In that case Node fetch throwsTypeError: fetch failedwith an underlyinggetaddrinfo ENOTFOUND ...local, which matches the production/NAS logs.Operator guidance
If Bond failures mention a
.localhost orENOTFOUND, the immediate operator action is:bond_update_bridge_connectionThis change makes those failures much more actionable in Sentry/logs, uses the last discovered IPv4 address as a safe fallback when it is available, and avoids pointing operators at mDNS when the real issue is an HTTP-level Bond response.
Testing
npm test -- --run src/adapters/bond/index.node.test.ts src/sentry.node.test.tsnpm run typecheckArtifacts
Summary by CodeRabbit
Bug Fixes
Improvements
Tests