Skip to content

Quieten expected Platform-connectivity errors in the token watcher#1085

Merged
pditommaso merged 4 commits into
masterfrom
fix-watcher-connectivity-logging
Jul 16, 2026
Merged

Quieten expected Platform-connectivity errors in the token watcher#1085
pditommaso merged 4 commits into
masterfrom
fix-watcher-connectivity-logging

Conversation

@pditommaso

Copy link
Copy Markdown
Collaborator

Problem

Observed in prod after the #782 rollout: when the token watcher's describeWorkflow lookup fails because the run's Platform endpoint is briefly unreachable — e.g. a paired self-hosted instance whose pairing WebSocket times out (HTTP 408 / TimeoutException on pairing-inbound-queue) — the exception propagated to the generic catch-all in watch() and was logged at ERROR with a full stack trace on every retry.

The behavior is already correct (the token is not renewed → lapses fail-closed → next check sees gone and stops tracking), so the stack trace is pure noise. For a chronically-unreachable paired endpoint it repeats every refresh-interval per token, floods the logs, and inflates error-rate signals.

Change

In the watcher's catch-all, distinguish an expected Platform-connectivity failure (TimeoutException or HttpResponseException anywhere in the cause chain) from a genuinely unexpected error:

  • connectivity failure → concise one-line WARN (request id, workflow id, root-cause message), no stack trace;
  • anything else → unchanged ERROR + stack trace.

Retry (scheduleRefresh) and the wave.tokens.refresh{result=error} counter are unchanged for both paths, so the metric-based failure signal is preserved — only the log verbosity changes.

Tests

Added a data-driven spec for isConnectivityError (wrapped TimeoutException / HttpResponseException / ExecutionException → true; plain RuntimeException / NPE → false) and rootCauseMessage.

🤖 Generated with Claude Code

When the workflow-status lookup fails because the run's Platform endpoint is
briefly unreachable (e.g. a paired self-hosted instance whose pairing
WebSocket times out), the watcher caught it in the generic catch-all and
logged a full ERROR stack trace on every retry. For chronically-unreachable
paired endpoints this floods the logs and inflates error-rate signals, even
though the behavior is correct (the token is not renewed and lapses
fail-closed, then the entry stops on the next 'gone' check).

Distinguish these transient connectivity failures (TimeoutException /
HttpResponseException in the cause chain) from genuinely unexpected errors:
log the former as a concise one-line warning, keep the ERROR+stack trace for
the latter. Retry and the wave.tokens.refresh{result=error} counter are
unchanged for both.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@pditommaso
pditommaso force-pushed the fix-watcher-connectivity-logging branch from 7dc45ad to 12be528 Compare July 16, 2026 15:21
pditommaso and others added 2 commits July 16, 2026 17:39
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gavinelder

gavinelder commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

@pditommaso The nested loops still don't look correct and the e == e.cause check feels redundant.

I cannot do code suggestions over multiple lines however if you could look at something like the following instead , i just tested it on the codebase and tests appear to be passing.

Testing with Claude it gives the following feedback

  • Dropped the if( e == e.cause ) break self-cycle guard in isConnectivityError — redundant, since MAX_CAUSE_DEPTH
    already bounds the loop.
  • Added a null guard to rootCauseMessage (if( t == null ) return null) so it no longer NPEs on a null argument.
  • Simplified the MAX_CAUSE_DEPTH comment to just the field, and matched your Javadoc wording ("cyclical cause
    chains").
private static final int MAX_CAUSE_DEPTH = 50

/**
 * Whether the given error is an expected Platform connectivity failure (a timeout or an HTTP
 * error reaching Tower) as opposed to an unexpected bug. Such failures are transient and retried,
 * so they should not be logged at ERROR with a full stack trace. The cause chain is walked up to
 * MAX_CAUSE_DEPTH to guard against cyclical cause chains.
 */
protected static boolean isConnectivityError(Throwable t) {
    Throwable e = t
    for( int i = 0; e != null && i < MAX_CAUSE_DEPTH; e = e.cause, i++ ) {
        if( e instanceof TimeoutException || e instanceof HttpResponseException )
            return true
    }
    return false
}

protected static String rootCauseMessage(Throwable t) {
    if( t == null )
        return null
    Throwable e = t
    int i = 0
    while( e.cause != null && e.cause != e && i++ < MAX_CAUSE_DEPTH )
        e = e.cause
    return e.message ?: e.class.simpleName
}

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@pditommaso

Copy link
Copy Markdown
Collaborator Author

Agreed on all three — applied in 5842660: dropped the redundant e == e.cause break (the depth cap already bounds it), added the null guard to rootCauseMessage, and trimmed the comment. Thanks!

@pditommaso

Copy link
Copy Markdown
Collaborator Author

Great, thanks Gavin! 🙏 All applied.

@pditommaso
pditommaso merged commit a4c6176 into master Jul 16, 2026
4 checks passed
@pditommaso
pditommaso deleted the fix-watcher-connectivity-logging branch July 16, 2026 16:49
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