Repository navigation
Fix batch 1 of the v0.4.0 review: demo blocker, P1 and P2 findings - #23
Merged
Merged
Conversation
The bundled simulator answered with 0xsim-<hashCode> transaction hashes. Since 0.4.0 eth-sepolia accepts only 0x and 64 hex digits, and changeset 015 enables eth-sepolia with USDC, so a demo address on Sepolia registered fine and then failed every sync with "Provider returned an event with a malformed transaction hash". The simulator now answers with 0x and the SHA-256 of chain, address, and asset: still deterministic, and well formed on every chain. The demo base URL resolves server.port instead of the SERVER_PORT variable, so the self-call into the simulator follows the port however it is set, --server.port and IDE run configurations included. The demo seeder wrote outbox rows with a one-field payload, so their published log lines showed null fields, and with event types that did not match their transactions. Each row is now a complete lifecycle event of its transaction, with the payload, event type, and idempotency key that ingestion writes, and the seeded rows record the source demo:seed. The README demo flow generates its external reference, address, and transaction hash, so it can run again against the same database.
The demo sync test listed local-evm and eth-sepolia by hand. It now reads every enabled chain and asset from the registry, so a chain enabled later, or a stricter identity rule on one, fails the test instead of breaking the demo quietly.
Review of the demo fix found that the unscoped ON CONFLICT DO NOTHING let a restart add outbox events for a transaction the seeder did not write, and re-insert seed events that were published and then removed by outbox retention. A transaction's events are now seeded only when the transaction row itself is inserted. The seed test pins every event exactly: outbox id, event type, idempotency key with its version, and the full payload, and a restart after the events are gone brings none of them back. The wording on the simulator port (it needs a fixed port and no context path), on the seeded event types, and on the NULL source of rows seeded before this change now matches the code; the simulator test drops an assertion that could not fail.
…ected profile With SPRING_PROFILES_ACTIVE=prod,demo on a fresh database the demo-user guard ran first and found nothing, then the demo seeder wrote demo-reader and demo-operator with their public passwords, and the service ran with them until the next restart; the simulator path was open without credentials as well. prod combined with local failed, but on whichever bean it missed first. A bean factory post-processor now stops prod together with demo, local, or test before any bean is created, and names the active profiles. The demo seeder, the simulator, and the open /simulator path also require demo without prod. The demo-user guard was bound to the literal prod profile, so any other protected profile, e2e or a custom one such as staging, accepted the public demo passwords on a database that once ran demo. It now runs in every profile except demo, local, and test.
…r guard Review of the profile guard found that it only knew the literal prod: staging,demo still seeded the public demo users with the demo-user guard switched off, and staging,local started without authentication. It also ran after the configuration conditions, so prod,local without the prod secrets failed on an unresolved placeholder instead of naming the combination. The guard is now an environment post-processor that refuses demo, local, or test next to any other profile before the application context exists; the demo-only beans go back to plain demo, with this guard as the single point that keeps them out of prod. The demo-user guard is never lazy, skips a database without the users table instead of failing on its own query, names the default profile when none is active, and recommends an empty database, because a database the demo ran on also holds the demo dataset. The prod operator account cannot take a demo user name. The tests cover the profile matrix, start the refused combinations against an empty database that must stay empty, and assert the exact profiles in every message.
The API promised 503 database-unavailable while PostgreSQL is down, but only a DataAccessException got it. A transaction that cannot get a connection fails with CannotCreateTransactionException, and a rollback on a connection the outage broke fails with TransactionSystemException. Both are TransactionExceptions, so seven of the nine endpoints answered 500 in local. In demo and prod, HTTP Basic could not read its user store, and the entry point answered 401 with a challenge, blaming valid credentials for the outage. Both transaction failures now get the same 503; other transaction exceptions, such as an unexpected rollback, stay 500. The entry point answers a user store failure caused by the database with that 503 and any other with the generic 500, both without a challenge, and logs database_operation_failed like the API. pgjdbc gets a 40-second socket timeout, above the 30-second statement_timeout, and a 5-second connect timeout, so a query to a server that stopped answering ends instead of holding the request thread and its connection forever. A test stops a dedicated PostgreSQL under running test and e2e contexts and checks every endpoint, the anonymous 401, health, and the socket timeout of pooled connections.
The immutable-conflict ProblemDetail example in docs/api.md and docs/architecture.md still said "amount or direction did not match" and lacked the address, asset, and conflictingFields the service writes. The error list in docs/architecture.md left out the 404 for an unsupported asset and the 409 for a duplicate account. docs/failure-modes.md promised metrics "when metrics are implemented" three times, although the ingest and transition counters exist, and a stale-confirmation log line with stored and incoming counts that the service never writes. It now names the meters and log lines as they are.
Review of the outage fix found that any database error behind the user store lookup became the outage answer: a Basic username with a NUL character, which PostgreSQL refuses as a data error, got 503 and an ERROR database_operation_failed line on a healthy database, where it used to get 401. Such a username is a bad credential again. One rule now decides what a database failure is, for the API, the credential check, and the sync worker: a Spring data access exception, a transaction that could not begin or roll back, or an SQL error Spring could not translate, such as a write to a read-only database, which answered 500. A sync run that meets one records "Database error" instead of "Unexpected error". Both entry paths write the same log line. The outage test checks that the credentials worked before the stop, a NUL username on the healthy database, the connect timeout, and that the socket timeout stays above statement_timeout; it sends the requests in parallel with the shortest pool wait, from 29 to 9 seconds. The docs describe the outage once, in docs/failure-modes.md, with the COMMIT that the socket timeout can cut off and its retry, and the examples show the fields in the order the service writes them.
Validation of POST /api/v1/observed-events parsed amount into a BigDecimal even when the string broke its 80-character limit: the validator checks every constraint, and the parse takes time quadratic in the length. A million-digit amount held a request thread for about ten seconds before the 400, and Jackson accepted strings of up to 20 million characters. The amount check now leaves an over-long value to @SiZe alone. The application's ObjectMapper caps strings at 100 000 characters, far above the longest legitimate one, a provider cursor of at most 4096 by default; a longer string fails a request with 400 invalid-request and a bridge page as malformed JSON.
Review of the amount fix found the same cost next to it. The non-blank pattern of externalRef and label backtracked quadratically on a long value that ends in a line break, and validation ran it after @SiZe had failed: at the new 100 000-character cap it held a request thread for about ten seconds. It now lets the dot cross line breaks, which makes it linear; a value with a line break is no longer reported as blank. The string cap reached JSON only. Spring MVC also read application/yaml bodies, because springdoc brings the YAML data format, with no limit; that converter is removed, so YAML gets 415 as the docs always said. Request and bridge page DTOs skip unknown fields instead of buffering them until the known ones are complete, which held many times a body's size in heap and made the cap depend on key order. A hit on the cap now says so, in the API detail and in the bridge page error. The amount parse refuses an over-long value itself, so a caller that skips validation cannot reach the quadratic parse. The getters that leave a blank value to @notblank use its definition of blank, so a value of Unicode spaces gets validation-failed instead of passing validation and failing later without the errors list.
The HTTP bridge has no credential setting of its own, so its credentials can only live in base-url, in the userinfo or the path. Every transport failure passed Spring's I/O error text on, and that text quotes the request URL: the userinfo and the path reached the health details, the WARN log, and the lastError of sync runs that the READ role sees. A transport failure now names its kind, taken from the first known exception in the cause chain (connection refused, timeout, unknown host, TLS, I/O), with that exception's class, and carries no cause. The cause chain goes to the DEBUG log with every URL cut out.
…s other exits Review of the transport message fix found that it labelled a connect timeout "connection refused" whenever a ConnectException sat anywhere in the cause chain, and that it moved the only diagnosis to a DEBUG line. The kind now comes from the I/O error RestClient wraps, with a neutral "cannot connect" for a ConnectException, and the WARN line carries the cause chain with every URL cut out, the closing quote kept. The review also found where the URL still got out. HttpURLConnection never sends the userinfo of base-url, so a user name or password there only looked configured; it now stops startup with a message that leaves the URL out, and the docs say a token goes in the path or the query. At DEBUG and TRACE the JDK's HttpURLConnection and Spring's URI parser printed the URL with its token; both loggers are pinned to INFO, which the e2e test checks at a TRACE root level. Two failures on the same catch path are fixed on the way: a null element in events was an outage and is now a data error for the address, and a Retry-After beyond Instant's range turned a 429 into a failed request without its backoff.
The quickstart starts local and demo with ./gradlew bootRun, and both listened on every interface: anyone on the same network could use local without credentials, and demo with its public passwords. Round 1 closed this for compose only. Both profiles now bind server.address to 127.0.0.1, which SERVER_ADDRESS overrides for a remote demo on a trusted network. The Docker image sets SERVER_ADDRESS=0.0.0.0, because a container is reached through its published port, which docker-compose.yml keeps on the host's loopback.
Swagger is what a reviewer opens first, and it said 200 for every operation, although creations answer 201, sync requests 202 with Location, and ingest 201 or 200. It named no error at all, listed the demo chain simulator next to the API, and showed the validation getters isAmountValid, isDirectionValid, and isStatusValid as required request fields. The controllers declare their success codes, and one customizer adds the ProblemDetail errors every operation shares (400, 401, 403, 503) as application/problem+json with a ProblemDetail schema; the operation-specific errors stay in docs/api.md. The document covers /api only and responds in application/json, and the getters are ignored by Jackson, so they leave the schemas.
… out of health Under type=alchemy the seeded local-evm chain is enabled but has no Alchemy network. An address registered there got 201, its first sync turned the provider state to FETCH_FAILED and /actuator/health into 503, contrary to the 0.4.0 note that one bad address no longer does, and the next start stopped on the rollout preflight. The provider port says whether it serves a chain, and registration refuses one it cannot serve with 404 Unsupported chain; the detail now names that case. A configuration gap of one address, its chain without a network, its asset without an enabled config, or a block larger than a page, is an AddressConfigurationException: still terminal for its runs, but shown as lastDataError while health stays UP. A cursor another provider wrote, such as the demo simulator's, now fails with a message that says so and points to the runbook. The 0.4.0 notes also said that after an outage at startup sync runs retry until Alchemy answers; they retry up to max-attempts and then fail, and nothing probes again. The README, the runbook, and the preflight's description now say so.
… 127.0.0.1 Review of the loopback change found that the image-wide SERVER_ADDRESS=0.0.0.0 is an environment variable, so it outranked every config file: a prod server.address set in a file was silently replaced, and any container run of local or demo outside compose was open again. Compose now sets it for the application container only, next to the loopback port it publishes. The demo bridge called its own simulator on localhost, which can resolve to ::1 first, where a loopback bind does not listen; it now calls 127.0.0.1. The test task drops SERVER_ADDRESS and SPRING_PROFILES_ACTIVE from the environment, so a value exported in the shell cannot override what the tests set. The docs say that a container run by hand needs SERVER_ADDRESS=0.0.0.0 and that 0.0.0.0 also opens IPv6.
… API sends Review of the OpenAPI change found that the document described Location and ProblemDetail.instance as absolute URIs while the service sends relative references, left out the 500 every operation can answer, put a 403 on GETs that READ may call, and copied the four shared error responses into all nine operations, about 45 percent of the document. A plain OpenApiCustomizer would also have skipped any grouped document. The shared errors are now declared once under components.responses and referenced: 400, 401, 500, and 503 on every operation, 403 on the operations that change state. The customizer is global. Location and instance are uri-reference, and the ProblemDetail schema names the errors array of a validation failure. The test reads the document with properties() instead of the deprecated fields().
This was referenced Sep 23, 2026
Closed
…s by their keys The controllers always send Location with a new account and a sync run, but the document declared it optional, so a generated client had to handle a value the API never omits. The comment that explains why the springdoc endpoints are enabled everywhere sits by those flags again, and the new keys say why they are there.
The test task removed only SERVER_ADDRESS and SPRING_PROFILES_ACTIVE from the environment of the test JVM. Any other variable that Spring binds to a property still overrode the tests' own settings: ASSET_SYNC_PROVIDER_TYPE=alchemy, which the Alchemy runbook asks to export, switched every test context to the Alchemy provider, and SPRING_APPLICATION_JSON or SPRING_PROFILES_INCLUDE could change any of them. The task now drops every SPRING_, SERVER_, MANAGEMENT_, LOGGING_, and ASSET_SYNC_ variable, except the Alchemy API key, which the env-gated live smoke reads itself.
local and demo listen on 127.0.0.1 through
server.address=${SERVER_ADDRESS:127.0.0.1}. A variable that is set but
empty replaces that default with an empty value, which binds as no
address, so Tomcat listened on every interface again: local without
authentication, demo with its public passwords. An environment
post-processor next to the profile guard now stops such a start and
says how to get loopback or a named interface.
The README also says that demo calls its simulator on 127.0.0.1, so a
bind to one other interface needs ASSET_SYNC_PROVIDER_BASE_URL.
The regression review of the combined PR confirmed these, live on the jar of the PR or in the code: - PATCH of a disabled address to ACTIVE skipped the registration checks, so under type=alchemy an address on local-evm came back, failed every sync, and stopped the next start. Enabling now runs the chain, provider, and asset checks of registration. - Under start-mode=configured-block a mapped chain without a start block passed registration, and its first sync turned health DOWN (503). supportsChain now requires the start block, and a missing one is a configuration gap of one address, like an unmapped chain. - The foreign-cursor hint covered only a cursor that was not JSON or carried another provider's marker; a bare number, a JSON object of another shape, or a token over 256 characters from the HTTP bridge got none. Any token that is not an object marked "p":"alchemy" is now foreign, and a damaged Alchemy cursor never is. - The runbook fixed a foreign cursor by deleting the sync_cursors row, which also drops the event high-water that keeps the next sync from re-reading blocks whose events the old provider stored. It now clears only provider_cursor. - The docs promised health DOWN for Alchemy configuration failures and called a one-address gap "terminal for that address only", while the startup preflight refuses the same state at the next start. The runbook, the README, failure modes, architecture, and the changelog now say what health, the run, and the next start show, and that lastDataError lasts only until the next successful fetch. - Disabling the local-evm chain, as the runbook advised, let the service start while that chain's active addresses kept failing every account sync, and the error still said "or disable the chain". The runbook and the README disable the addresses too, and the messages name the address. - Tests now pin the oversized block as an AddressConfigurationException with health UP and a key rejected at fetch time as health DOWN. Reverting either classification passed the suite before. recordDataError logs its own value instead of re-reading the shared field, which a concurrent fetch may have changed.
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 1 of the fixes from the merged review of v0.4.0 in one PR: the demo blocker and every P1 and P2 finding the review scheduled before a demo. It replaces #15 to #22, which each carried one fix and conflicted in
CHANGELOG.md; their descriptions keep the detailed before-and-after evidence per fix. There is one commit or a short series per fix, in the order of those PRs. Two commits apply the regression reviews of #20 and #21, and the last three the final review of this PR. 0.4.1 is released once all batches are in.demosyncseth-sepolia: the simulator's hashes are 64 hex digits. The seed writes complete outbox events. The README demo flow can be repeated. The simulator URL followsserver.port.demoFAILEDdemo,local, andtestrun only alone. The demo-user guard runs in every protected profile. The prod admin cannot take a demo name.prod,demoseeded the public demo users503 database-unavailable, from MVC and from the credential check. pgjdbc gets socket and connect timeouts. One rule decides what a database failure is.500inlocal,401with a challenge indemo/prod, a hang on a paused databaseamountis checked by length before it is parsed, and the non-blank pattern is linear. JSON strings are capped at 100 000 characters, YAML bodies get415, and unknown fields are skipped.base-urlstops startup. The loggers that printed the URL are pinned to INFO.lastError, health, and logslocalanddemolisten on127.0.0.1on a host. Compose setsSERVER_ADDRESS=0.0.0.0for its container.localwithout authProblemDetailerrors once, and it leaves the simulator and the validation getters out.200everywhere, the simulator in the documenttype=alchemyan unmapped chain is refused at registration, and one address's configuration gap no longer turns healthDOWN. A cursor another provider wrote names its fix.201, then503health and a failed next startPATCHruns the registration checks again. Underconfigured-blockonly a chain with a start block is served. Every foreign cursor names the fix, which now clears onlyprovider_cursorand keeps the event high-water. An emptySERVER_ADDRESSstopslocalanddemo. The tests ignore configuration exported in the shell. The docs say what health, the run, and the next start show for one address's gap.PATCHbrought back an address Alchemy cannot serve, and an emptySERVER_ADDRESSopened every interfaceCHANGELOG.mdlists everything under[Unreleased], grouped as Changed, Fixed, and Security. The one Breaking change is the refusal of a user name or password in the bridgebase-url, which the HTTP client never sent.Verification
./gradlew clean checkon this branch: 325 tests, 0 failures, 2 skipped (the env-gated Alchemy live smoke), and no compiler warnings.demoagainst its own PostgreSQL: listens on127.0.0.1. Aneth-sepoliaaddress syncs through the simulator (SUCCEEDED, 1 event). A NUL username gets401, a YAML body415, and a million-digitamount400after 0.08 s./v3/api-docslists no simulator. With the database stopped a reader gets503 database-unavailable, and after the restart200.docker compose up --buildinlocal: health200through the published127.0.0.1port.prodwithtype=alchemyagainst the real Sepolia endpoint:local-evmgets404 Unsupported chain, and a Sepolia address registers and syncs (SUCCEEDED). Provider health isUP, and the key appears 0 times in the log.demoan emptySERVER_ADDRESSexits with the guard's message, andPATCHtoACTIVEon an address whose chain was disabled gets404and leaves itDISABLED. Underprodwithtype=alchemyon Sepolia,PATCHtoACTIVEon alocal-evmaddress gets404, a bridge cursor19000000fails the run with "…is not a JSON object, so another provider wrote it; clear its provider_cursor…", and after clearing onlyprovider_cursorthe next sync succeeds.Reviews
Every fix went through a regression review (
/code-review). The findings that held were fixed in the same PR (#17, #18, #19) or in the two review commits here (#20, #21). The final review of this PR ran once over #22 and the interactions of all fixes. It was stopped after its verifiers had confirmed the findings, and the confirmed ones are fixed in the last three commits without another round. Recorded in the plan for later batches rather than done here:DOWNwith the raw exception text), as before this PR;403rule, which follows the HTTP method rather than the security configuration;local-evmaddress under any provider, sodemowithtype=alchemystarts only once;*Validnames in validation errors and nullable required fields in the schemas (B25).