Repository navigation
Fix batch 2 of the v0.4.0 review: the HTTP bridge contract - #24
Merged
Merged
Conversation
The cursor went into the request URI as part of the template. A `+`, common in base64, stayed as is and the bridge decoded it as a space; braces, as in a JSON cursor, were taken as template variables, and the request failed before it was sent with "Not enough variable values", classified as a retryable provider outage. Every path and query value is now a URI variable, which RestClient encodes strictly, so the bridge gets its cursor back byte for byte.
SimpleClientHttpRequestFactory follows redirects for GET, so the bridge client went wherever a 3xx pointed, with any token from base-url. Both provider clients now share one request factory that keeps the configured timeouts and never follows a redirect. A 3xx from the bridge or Alchemy is a ProviderConfigurationException: the run fails at once, and provider health turns DOWN, since the operator's endpoint is wrong for every address and retries cannot fix it. Alchemy's 3xx was retried as an outage. The Alchemy client tests now build the production RestClient.
A slow provider body held a provider thread long after the provider timeout. future.cancel(true) interrupts the thread, but a blocked socket read ignores the interrupt, and a body that trickles in keeps every read under the read timeout; readBounded never looked at the interrupt. Worse, Spring's SimpleClientHttpResponse.close() drains the rest of the body before closing it, so even the byte limit and error statuses left the thread reading an oversized or endless body. Four such responses took the whole pool. readBounded now checks the interrupt before every read and closes the body unread on any failure, and both adapters close the bodies of error statuses unread, so Spring's drain fails fast. The thread comes back within one read timeout of the cancel. Response headers that trickle in stay bounded only by the read timeout of each read. The provider pool test that claimed to cover a leak exercised the fake provider only; the slow-response tests replace it.
validatePage refused a final page without nextCursor unless it carried events or a safe or latest height above the stored one, and failed the run terminally otherwise. A bridge that answers "nothing new" without repeating its cursor did exactly that whenever two syncs of an address fell into one finality epoch, even with a stored cursor. The rule guarded against losing the cursor, which the service has not done since it started keeping the stored cursor after an empty final page and sending the checkpoint with every request, so it is gone. Stored heights are kept as a maximum, so a lower height changes nothing.
Since 0.4.0 every 4xx from the bridge counted as bad data of one address: the run failed terminally, but health stayed UP. A wrong or revoked bridge credential therefore failed every address behind a green health indicator, while Alchemy's 401 and 403 turn health DOWN. The bridge's 401 and 403 are now ProviderConfigurationException, terminal without retries and DOWN until a fetch succeeds. A 404 and the other 4xx stay the error of that one address: a bridge may not know an address.
The natural key of an observed transaction has no direction, so a bridge that sends a transfer of the address to itself as an INBOUND and an OUTBOUND row of one log gave a page whose first row was stored and whose second failed as a conflict with stored immutable fields. The checkpoint stayed, and every later sync failed on the same page. validatePage now refuses a page that repeats (txHash, eventIndex) with another direction or amount, before anything is written, and names the event. An exact repeat stays harmless, as ingest is idempotent.
When the provider pool had no free thread, the run failed with SyncCapacityExceededException through requeueFailure, which spends a failure_attempt, so five rejections under load failed the run although the provider was never asked. It now goes through the budget-neutral requeue that a full worker pool already uses, with backoff by claim count.
0.4.1 refused a user name or password in base-url, because the HTTP client never sends them, which left the path or query of base-url as the only place for a bridge token. asset-sync.provider.auth-header-name (Authorization by default) and auth-header-value now put the credential in a header of every bridge request. The value is a secret: the properties mask it in toString, and a rejected credential names the property, never the value. A header name that is not an HTTP token or a value with a line break stops startup, since the request could not carry it.
Jackson reads a provider body that is already in memory, yet bytes it cannot decode as text, such as invalid UTF-32, raise CharConversionException, an IOException rather than a JsonProcessingException. RestClient wrapped it as an I/O error, so the bridge and Alchemy reported it as a transport failure: retried as an outage with health DOWN. Both adapters now treat it as bad data of that address. Alchemy transport failures are named by their kind, like the bridge's, through a shared ProviderHttpSupport.transportKind. Their text used to quote the request URL with the key scrubbed, so anything else a custom endpoint template held reached last_error, health, and the logs. The WARN line keeps the cause chain with every URL cut out.
The contract section lists every kind of bridge answer in one table: its class, what happens to the run, provider health, and the retry budget. It also states that a bridge scanning block windows must return a cursor on every page, because the service only passes the checkpoint of the last ingested event. The failure modes name the redirect as a configuration failure, the testing guide lists the contract tests, and the changelog entries of this batch follow the order of the contract.
A ServerSocket(0) binds the wildcard address, and on macOS it can get a port that a listener bound to 127.0.0.1 already holds, such as the Tomcat of a cached local or demo test context. The "closed" port then answered with that server's 404 instead of refusing the connection, and the transport test failed. The ports now come from a socket bound to the loopback address the clients connect to.
The regression review of #24 confirmed these, two by probes: - A transfer of the address to itself whose two rows fall on either side of a page boundary still stored the first row and failed with the old conflict text. The conflict now names the event and the fields that differ, and the docs say that only a repeat inside one page fails before anything is written. The in-page message also fits the 240 characters of last_error with a real transaction hash. - A full provider pool went through the budget-neutral requeue, whose backoff grows with every claim (731 s at the eighth) and never ends. It is now a continuation with the new reason PROVIDER_BUSY (changeset 017): retried after cursor-lease-retry-delay, bounded by max-continuations-per-run, and an account pass resumes at the address that met it. - Account syncs isolated every ProviderConfigurationException, so a bridge 401, 403, or redirect cost one request per address. A configuration failure of the whole provider now ends the run at once, while a gap of one address stays isolated, including a chain disabled at ingest, now an AddressConfigurationException. - An empty ASSET_SYNC_PROVIDER_AUTH_HEADER_NAME stopped every start. A blank name now means Authorization, and the name is checked only when there is a credential to send. - Alchemy's WARN cause chain cut URLs before scrubbing the key, so a key with a quote could leak its tail. It scrubs first now. - The request factory comes from Boot's ClientHttpRequestFactoryBuilder with Redirects.DONT_FOLLOW instead of a subclass. - Docs: a page that breaks a rule the service checks leaves provider health alone instead of setting lastDataError; a redirect answering the Alchemy startup probe stops the process; the removed final-page rule and the old capacity accounting are gone from architecture.md, database.md, and the page KDoc. - The bridge contract tests shut down their server executor.
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
1 participant
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
Batch 2 of the fixes from the merged review of v0.4.0: the HTTP bridge contract, B09 to B16, together with the items the review of #19 deferred to it. A short contract spec came first, so the fixes change the published contract (
docs/architecture.md, HTTP Bridge Page Contract) as one piece instead of one by one. There is one commit per fix.+, as in base64, arrived as a space, and a JSON cursor failed before the request, as a retryable outage3xxis a configuration failure: the run fails at once and health turnsDOWN.base-url; Alchemy's3xxwas retriedclose()drained the body, so a trickling or endless body held a provider thread; four took the whole pool401or403fails the run at once as configuration, with healthDOWN. A404stays that address's error.PROVIDER_BUSY, changeset 017), like a busy cursor lease, bounded bymax-continuations-per-run. An account pass resumes at that address.ASSET_SYNC_PROVIDER_AUTH_HEADER_NAMEandASSET_SYNC_PROVIDER_AUTH_HEADER_VALUEsend the bridge credential in a header; no message, log line, or health detail quotes it.base-urllast_errorThe contract section now lists every kind of bridge answer in one table. A test commit takes the "closed" ports of the provider tests from loopback: on macOS a wildcard
ServerSocket(0)could get the port of a Tomcat bound to127.0.0.1and answer404instead of refusing the connection. The last commit applies the review of this PR (see below).Verification
./gradlew clean check: 344 tests, 0 failures, 2 skipped (the env-gated Alchemy live smoke), and no compiler warnings.prodagainst a stub bridge that takes its credential inX-Bridge-Key:c+1/==unchanged, the paged sync succeeded, and two idle resyncs without a cursor at the same height succeeded.401failed the run at once with healthDOWN, and the next successful sync brought health back toUP.302failed the run with healthDOWN, and the redirect target was never requested.demoon the same jar: aneth-sepoliaaddress syncs through the simulator (SUCCEEDED), and an emptySERVER_ADDRESSstill stops the start.Review
One
/code-reviewround ran on this PR. Its findings that held are fixed in1818dda, without another round:PROVIDER_BUSYcontinuation.ASSET_SYNC_PROVIDER_AUTH_HEADER_NAMEno longer stops the start.Recorded for batch 4 rather than done: B41, closing responses unread and disconnecting from the timeout path in the request factory itself instead of at each call site, and B42, one status table shared by both adapters.
Left out
disconnect()from the timeout path.