Skip to content

fix(sse): keep one proxy journal row per request actually sent - #14637

Merged
diegosouzapw merged 1 commit into
cheaper-inference:release/v3.8.51from
maxmad64bis:fix/proxy-attempt-log-rows
Sep 24, 2026
Merged

diegosouzapw merged 1 commit into
cheaper-inference:release/v3.8.51from
maxmad64bis:fix/proxy-attempt-log-rows

Conversation

@maxmad64bis

@maxmad64bis maxmad64bis commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Base-red inherited: #14547

Summary

A request saved on its 4th try reads as a first-try success from a healthy outlet: the proxy journal writes one row per request on the final outcome only, so refused outlets leave no trace, and abandoned rows borrow the final outlet's egress address with a zero latency that drags the latency average down. Each send now appends to a per-request journal on the applied-proxy sink (snapshot at entry, status or sanitized error plus send duration at settle), and the log call writes one proxy_logs row per send sharing one correlation id: refused sends first (each with its own outlet's cached egress address and its own duration), the send the request settled on last. The latency strategy ignores abandoned sends. Columns are nullable, added by migration 187 following the 179 pattern.

Related Issues

Related to #14620 (merged: the shared applied-proxy capture this PR appends to is now on the base)
Related to #13580 (prior: kept the provider HTTP status this PR records per send)
Related to #14223 (prior: request correlation this PR groups sends by)

Validation

  • Change type: DB
  • Focused tests: tests/unit/proxy-logs-attempt-rows.test.ts 11/11 GREEN on the rebuilt head 940d0b61 + check-migration-numbering 15/15 + migration runner 3/3
  • npm run lint — ESLint on the touched files is clean (exit 0 with frozen suppressions); the full run is red on the base (🔴 Release branch not green: release/v3.8.51 #14547)
  • Reconciled with the current active release base; focused checks rerun afterward (rev-list HEAD..upstream/release/v3.8.51 = 0 @ 940d0b61, single commit, post-fix(sse): share applied proxy capture across duplicated fetch modules #14620 merge)
  • Production-code changes include a new or updated automated test in this PR

Tests Added Or Updated

  • tests/unit/proxy-logs-attempt-rows.test.ts (11 tests: per-outlet egress address, abandoned-excluded latency average, non-negative send duration, served-row-last order; 11/11 GREEN on the rebuilt head)
  • Migration/runner replay: check-migration-numbering 15/15, db-migration-runner-account-identity 3/3, numbering gate OK (184 migrations, no duplicates)

Coverage Notes

N/A — no coverage gate moved; src/sse/handlers/proxyJournal.ts is covered by the new test (abandoned rows plus final row), proxyLatency.ts exclusion by the latency-average test.

Reviewer Notes

  • Migration 187 is the correct number: the release tip ends at 186_conversation_turn_nodes_last_seen_index.sql (184/185/186 on upstream/release/v3.8.51, no 187), so 187_proxy_logs_attempts.sql plus the case "187" guard is exactly the next free slot. The earlier renumber request predates the tip landing 186.
  • Post-fix(sse): share applied proxy capture across duplicated fetch modules #14620 rebuild: one commit (940d0b61) on the release tip (7d23bcf8); the parent symbols (runWithAppliedProxyCapture / shared-context store in open-sse/utils/proxyFetch.ts, merged via fix(sse): share applied proxy capture across duplicated fetch modules #14620) are on the base, consumed from chatHelpers.ts / upstreamStatusCapture.ts. The stacked-on banner is dropped.
  • Reds qualified as inherited (§12): check-api-typecheck cites only auggie.ts / projectCombo.ts, both outside this PR (diff on them is empty); the chatcore-compression-integration test-size flag cites a file outside this PR; the complexity note on the new proxyJournal.ts is a single function at threshold in a new file with new-code complexity 0. Full lint and mutation coverage stay red on the base (🔴 Release branch not green: release/v3.8.51 #14547).

@maxmad64bis
maxmad64bis force-pushed the fix/proxy-attempt-log-rows branch 2 times, most recently from b04b0b2 to c1e75d2 Compare September 23, 2026 13:07
@maxmad64bis
maxmad64bis marked this pull request as ready for review September 23, 2026 14:59
@maxmad64bis
maxmad64bis marked this pull request as draft September 23, 2026 22:43
@maxmad64bis
maxmad64bis force-pushed the fix/proxy-attempt-log-rows branch from c1e75d2 to fa79657 Compare September 23, 2026 23:01
@maxmad64bis
maxmad64bis marked this pull request as ready for review September 23, 2026 23:31
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Great root-cause fix for the journal-collapses-to-one-row problem — per-send capture on the
applied-proxy sink, correlation_id grouping, and excluding abandoned sends from the latency
average all make sense together, and the 4 RED-before-fix test cases suggest this was built
test-first. One mechanical item before merge: the new migration is numbered 187 but the release
tip's newest is 185 (186 is free) — could you renumber to 186 to avoid an unexplained gap?
Also noting this is stacked on #14620 as you flagged, so it'll need a rebase once that lands.

@maxmad64bis
maxmad64bis force-pushed the fix/proxy-attempt-log-rows branch from a0b75c9 to 940d0b6 Compare September 24, 2026 10:28
@maxmad64bis

Copy link
Copy Markdown
Contributor Author

Thanks for catching the gap — the release tip has since landed 186_conversation_turn_nodes_last_seen_index.sql, so 187 is now exactly the next free number and the file plus the case "187" guard stay as-is. The branch is rebased to a single commit on the current tip now that #14620 has landed, so the stacked-on note is dropped. Migration numbering gate and the attempt-rows suite are green on the rebuilt head.

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