Skip to content

Remove dead code, wire up unfinished integrations, make settlement exactly-once - #67

Merged
cinar merged 7 commits into
mainfrom
chore/dead-code-cleanup
Oct 5, 2026
Merged

cinar merged 7 commits into
mainfrom
chore/dead-code-cleanup

Conversation

@parlakisik

Copy link
Copy Markdown
Contributor

Summary

A dead-code sweep found code that was never removed, and some that was never wired up. Wiring it up showed that completed contracts were never settled. Making settlement work then exposed several money-path defects. This PR fixes them.

Seven commits, each self-contained. Reviewing commit by commit is easiest.

1. chore: remove unused code and modules (−1,942 lines)

Candidates came from golang.org/x/tools/cmd/deadcode across all 28 modules, checked on darwin and linux and with the integration,e2e tags. Each was then checked against the specs, open tasks and git history before deletion.

  • Modules removed:
    • src/internal/testutil: never imported.
    • src/internal/agentcard and the root src/internal module: replaced by provider-registry's push registration.
  • Functions removed:
    • httpclient auth helpers and the NewClientWithRetry/NewClientWithCircuitBreaker duplicates
    • ap2 mandate checks (never called, and no signature check)
    • token-bank's unused auth middleware
    • work-publisher's BidGatewayClient, which called a route that doesn't exist
    • duplicate settlement methods, nats.StreamForSubject and RouteScopes.Keys
    • hack/integration DatabaseCleaner and three deprecated stubs
  • Kept on purpose:
    • the internal/certauth offline verifier (pending a decision)
    • certauth RecalculateAll
    • test seams
    • the integration-test harness API
  • Also updates the CI matrix, the Makefile and the docs.

2. feat(gateway): configurable CORS origins

The router used CORSAllowAll. It now uses middleware.CORS(cfg.AllowedOrigins), with ALLOWED_ORIGINS read as a comma-separated list (default *). The default behaves exactly as before, including preflight.

3. feat(bid-evaluator): fetch the work spec from work-publisher

  • Wires up WORK_PUBLISHER_URL. When a request sends no budget, the evaluator fetches the work. A budget in the request still wins.
  • The client's JSON fields were wrong (id/state where work-publisher sends work_id/status).
  • A missing work maps to 404 and other upstream failures to 502. A negative max_price gets 400.

4. fix(contract-engine): settle completed contracts

  • Bug: contract-engine never notified settlement, and settlement doesn't subscribe to contract.completed. Completed contracts were never paid.
  • Fix: contract-engine now posts to /internal/settlement/complete in the background once a completion is saved. A 409 counts as success, and a settlement failure never fails the provider's completion.
  • Real consumer: on award, contract-engine looks up the real consumer_id from work-publisher. It used to store "unknown", and settlement is skipped while the consumer is still unknown.
  • Failed outcomes are not charged.
  • /complete only from AWARDED or EXECUTING. Anything else gets 409. Before this, a FAILED contract could be completed and paid.

5. fix(httpclient): resend the request body on retries

Retried POSTs and PUTs went out with an empty body: the same request was re-sent after its body had been read, and doJSON used io.Pipe, so GetBody was never set. Each attempt now clones the request and gets a fresh body from GetBody. A request with a body but no GetBody is not retried.

6. fix(settlement): settle each contract exactly once

What was wrong:

  • A lookup error was read as "not settled".
  • Check-then-insert raced, with no unique index on contract_id.
  • The execution was saved before balances moved, so a failed update turned retries into a false 409.
  • Unknown tenants were created with negative balances.

The fix, without transactions (the compose and ECS stacks run standalone Mongo):

  • A unique contract_id enforces one execution per contract.
  • Each execution starts PENDING. The steps are: payment outcome (saved once), debit, credit, two ledger entries, SETTLED.
  • Each balance change is a single-document atomic update keyed by <contract_id>:debit or <contract_id>:credit, so replaying it is a no-op. Ledger ids are deterministic, and the final change to SETTLED is compare-and-set.

Recovery:

  • Settlement work runs on its own context, so a caller disconnecting can't cut it off.
  • A resumer drives stranded PENDING executions to SETTLED, safely across replicas. Executions too old to replay safely are left out of the scan and logged at ERROR for manual reconciliation.
  • Startup rejects duration settings that contradict each other.

Rules enforced in settlement itself:

  • Empty or "unknown" consumer, provider or contract → 400.
  • success=false → recorded, no money moves. A later success=true settles once.
  • A consumer with no account → 402.

New settings: SETTLEMENT_RESUME_INTERVAL, SETTLEMENT_RESUME_GRACE, SETTLEMENT_PENDING_ALERT_AGE, SETTLEMENT_OP_RETENTION.

7. fix(work-publisher): require a real consumer identity

  • Bug: the gateway sets X-Tenant-ID but never X-Consumer-ID, so every submission was stored with consumer default_consumer. With settlement live, that fake tenant would have been charged.
  • Work-publisher:
    • Reads X-Tenant-ID first, then X-Consumer-ID for direct service calls. With neither, it returns 401 CONSUMER_ID_REQUIRED.
    • Returns 404 only for real not-found; other store errors become 500.
  • Gateway: removes any client-sent X-Consumer-ID.
  • Callers updated: hack/integration, hack/tests/e2e_test.sh and the demo UI. The integration SettleContract helper called a route that doesn't exist; its test now checks balances and the 409 replay.

Behaviour changes to note

  • Settlement is now active. SETTLEMENT_URL and WORK_PUBLISHER_URL are set for contract-engine in compose, k8s, AWS, GCP and the hack deploy scripts.
  • 402 for unfunded consumers: a consumer with no settlement account now gets 402. The demo stacks never deposit, so their settlements return 402.
  • Duplicate contract_id rows block startup: settlement fails to start if it cannot create the unique contract_id index. Check existing executions data for duplicates before deploying.
  • 401 for direct submissions without identity: a direct work-publisher submission with no consumer identity now gets 401. Requests through the gateway are unaffected.

Test plan

  • GOWORK=off go build, go vet and go test pass in every module (src/aex-*, src/internal/*, hack/*).
  • go test -race passes for aex-settlement and internal/httpclient.
  • go vet -tags integration,e2e passes, and the tagged tests compile, in hack/integration and hack/tests. Plain go vet/go test in hack/tests fails with "matched no packages", which is already the case on main.
  • Each change was reviewed by independent agents, including adversarial reviews of the money path (crash at each step, concurrent requests, resumer across replicas, retries).
  • Not yet done, needed before merge: run hack/integration and the e2e suite against hack/docker-compose.yml. The new Mongo store code (pipeline update with upsert, duplicate-key mapping, conditional updates, the partial index, and the WithTransaction fallback on standalone) has only been exercised through the in-memory store.

Follow-ups (not in this PR)

  • Contract-engine shutdown waits up to 65s, longer than the k8s default grace period of 30s.
  • Production ALLOWED_ORIGINS overlay.
  • Demo stacks need a deposit step and auth.
  • AP2 mandate signature validation.
  • Decide whether to wire up or delete the certauth offline verifier, and whether to schedule or remove RecalculateAll.

Remove code that no service or test reaches. Each item was checked
against specs, open tasks and git history before deletion.

Modules:
- src/internal/testutil: never imported since it was added.
- src/internal/agentcard and the root src/internal module: replaced by
  provider-registry's push registration, which has its own copy of
  the card logic.

Functions:
- httpclient: auth.go (AuthProvider and its Bearer, APIKey and Basic
  implementations, ClientWithAuth), NewClientWithRetry and
  NewClientWithCircuitBreaker, all duplicates of NewClient.
- ap2: GetPaymentMethods, ValidateMandates and Validate*Mandate. None
  were ever called, and they did no signature check. Real mandate
  validation is tracked as a separate task.
- settlement: GetPaymentMethods and GetPaymentProviderBids. Both
  duplicate live code and neither is routed.
- token-bank: AuthMiddleware, OptionalAuthMiddleware and the context
  helpers that always returned empty. The router does its own token
  lookup.
- work-publisher: BidGatewayClient, which called an endpoint that
  bid-gateway does not serve.
- nats StreamForSubject and gateway RouteScopes.Keys.
- hack/integration: DatabaseCleaner and three deprecated stubs. This
  drops mongo-driver from that module.

Also update the CI matrix, the Makefile, docs/PROJECT.md and the
httpclient README, mark the old A2A plans as superseded, and gofmt
hack/integration/client.go.
The gateway router used CORSAllowAll even though config.AllowedOrigins
and middleware.CORS already existed. The roadmap and gateway spec ask
for CORS that is restricted in production.

- The router now uses middleware.CORS(cfg.AllowedOrigins).
- ALLOWED_ORIGINS is read as a comma-separated list. Spaces are
  trimmed, a trailing "/" is stripped, and the default is "*".
- "*" behaves exactly as CORSAllowAll did, including preflight, so the
  default is unchanged. CORSAllowAll is removed.
- A listed origin is echoed back with credentials and Vary: Origin. An
  unlisted origin gets no Access-Control-Allow-Origin header.

The production overlay still needs the real UI origins; that is
tracked as a task.
Evaluation rejected any request without budget.max_price ("work-publisher
not integrated yet"). The phase-a spec has the evaluator fetch the work
from work-publisher instead.

- Fix WorkPublisherClient to match work-publisher's model. It read
  "id"/"state" while the service sends "work_id"/"status", and
  constraints were untyped. Work IDs are now path-escaped.
- Add optional WORK_PUBLISHER_URL. When the request has no max_price,
  the evaluator fetches the work. Request fields override fetched ones,
  so callers that send a budget behave as before.
- A missing work maps to 404 WORK_NOT_FOUND; other upstream failures
  map to 502 BAD_GATEWAY. A negative max_price is rejected with 400
  before any fetch.
- With WORK_PUBLISHER_URL unset, a missing budget is still a 400.
- CloseBidWindow is not wired; the spec does not have the evaluator
  close the window.
Completed contracts were never settled. contract-engine never called
aex-settlement, and settlement does not subscribe to contract.completed,
so the existing SettlementClient sat unused.

Completion:
- After a successful completion is saved, contract-engine sends the
  contract.completed event to POST /internal/settlement/complete in
  the background (async, as in ARCHITECTURE_FLOWS).
- The completion response reports settlement_initiated.
- A 409 from settlement counts as success. A settlement error never
  fails the provider's completion; it is logged with the full event
  so it can be replayed.

Charging rules:
- Only successful outcomes are settled. A success:false completion is
  recorded, but no money moves.
- /complete is accepted only from AWARDED or EXECUTING. Any other
  status gets 409 INVALID_CONTRACT_STATE, with no second settlement
  call and no overwrite of the stored outcome. Before this, a FAILED
  contract could be completed and paid.

Consumer:
- On award, the real consumer_id is looked up from work-publisher
  (GET /v1/work/{id}, 5s cap) instead of storing the "unknown"
  placeholder.
- If the lookup fails, the award still succeeds, and settlement is
  skipped for contracts whose consumer is unknown.

Shutdown and config:
- On shutdown, contract-engine waits for in-flight settlement calls
  with its own timeout.
- New optional SETTLEMENT_URL and WORK_PUBLISHER_URL settings, wired
  into compose, k8s, AWS, GCP and the hack deploy scripts. The env
  tables in DEPLOYMENT.md and the deploy READMEs also gain the
  bid-evaluator WORK_PUBLISHER_URL row.
doWithRetry re-sent the same *http.Request after its body had been
read, so every retried POST or PUT went out with an empty body.
doJSON also streamed bodies through an io.Pipe, so GetBody was never
set.

- Each attempt now sends req.Clone(ctx), with a fresh body from
  GetBody. If GetBody fails, the error is returned.
- A request that has a body but no GetBody is sent once and never
  retried, rather than retried with an empty body.
- doJSON now uses a bytes.Reader, so GetBody is set.
- A response that triggers a retry is drained and closed first.
- GET and DELETE retries are unchanged.
Settlement could pay twice or report success without moving money:
- A lookup error was treated as "not settled".
- Check-then-insert raced, with no unique index on contract_id.
- The execution was saved before balances moved, so a failed update
  turned retries into a false 409.
- Unknown tenants were silently created with negative balances.

Exactly-once settlement, without transactions:
- One execution per contract_id, enforced by the store: Mongo's
  unique index and an in-memory lock. Startup fails if the index
  cannot be created.
- Each execution starts PENDING. Its steps are: payment-provider/AP2
  outcome (saved once), consumer debit, provider credit, two ledger
  entries, then SETTLED.
- Every balance change is a single-document atomic update keyed by
  <contract_id>:debit or <contract_id>:credit, so replaying one is a
  no-op. Ledger ids are deterministic. The final change to SETTLED is
  compare-and-set.
- This works on the standalone Mongo used by compose. WithTransaction
  now falls back to running without a transaction on standalone
  deployments instead of failing.

Recovery:
- Settlement work no longer depends on the caller's request: it runs
  on its own context with a 30s limit.
- A resumer drives stranded PENDING executions to SETTLED, safe across
  replicas. It runs at startup and every SETTLEMENT_RESUME_INTERVAL,
  after SETTLEMENT_RESUME_GRACE.
- Executions too old to replay safely (older than half of
  SETTLEMENT_OP_RETENTION) are left out of the scan so they cannot
  starve newer ones, and are logged at ERROR.
- Startup rejects duration settings that contradict each other.
- Applied balance ops are kept for SETTLEMENT_OP_RETENTION (default
  30 days), not the last 1000.

Charging rules enforced here:
- Empty or "unknown" consumer and empty provider or contract get 400.
  A bad price gets 400 instead of 500.
- success=false is recorded with no money moved. A later success=true
  upgrades it and settles once.
- A consumer with no account gets 402 instead of a new negative
  balance.
- IDs are trimmed before use.
- Responses now include charged, execution_id and contract_id.
Work-publisher stored consumer_id "default_consumer" whenever
X-Consumer-ID was missing. The gateway never sends that header, so
every submission was billed to a fake tenant once contracts started
settling.

Work-publisher:
- Reads the consumer from X-Tenant-ID first, then X-Consumer-ID for
  direct service calls. The gateway always sets X-Tenant-ID.
- With neither header, submit and cancel return 401
  CONSUMER_ID_REQUIRED. The placeholder is gone.
- GetWork and CancelWork return 404 only for store.ErrWorkNotFound.
  Other store errors become 500 instead of looking like "not found".
- An invalid work body returns 400 VALIDATION_ERROR instead of 500.

Gateway:
- Strips client-sent X-Consumer-ID, so it cannot be used to name
  another tenant.
- Returns 401 when an authenticated request resolves to no tenant.

Callers:
- hack/integration and hack/tests/e2e_test.sh send a consumer id.
- The demo UI sends X-Consumer-ID and no longer calls a settlement
  route that does not exist; contract-engine settles now.
- hack/integration SettleContract now posts a real ContractCompletedEvent
  to /internal/settlement/complete. TestSettlementContractSettlement
  asserts the balances and that a replay returns 409.
@parlakisik
parlakisik force-pushed the chore/dead-code-cleanup branch from daf197c to 6e5ab09 Compare October 4, 2026 21:10
@cinar
cinar merged commit 59824c3 into main Oct 5, 2026
75 checks passed
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