Skip to content

fix(compilers/openapi): detect without decoding the root mapping - #448

Merged
fuad-daoud merged 1 commit into
mainfrom
fix/detect-duplicate-key-blowup
Sep 10, 2026
Merged

fix(compilers/openapi): detect without decoding the root mapping#448
fuad-daoud merged 1 commit into
mainfrom
fix/detect-duplicate-key-blowup

Conversation

@fuad-daoud

Copy link
Copy Markdown
Collaborator

Closes #443.

Summary

Format detection decoded a document’s whole root mapping into a two-field struct
to read its openapi / swagger key. yaml.v3 compares every pair of a mapping’s
keys before it reads any of them, so a mapping repeating one key n times raises
n(n-1)/2 errors and then abandons the mapping — meaning the probe came back
empty as well as expensive, and the document was reported unreadable. The
parser the compiler goes on to use reads those same bytes and reports the repeats
itself, once each and sited.

Detection now parses the document and reads the two keys off the tree. It never
hands a mapping back to the decoder, which is the property the fix rests on.

Fixture Before After
32 KB, 6,553 repeats 16.7 s, 1.2 GB on stderr, refused 0.048 s, 1.9 MB, exit 0
128 KB, 26,214 repeats no result in 150 s 0.147 s, exit 0

The output after the fix is one sited validation-duplicate-key warning per
repeat, none longer than 288 bytes — which is what the compiler’s own parser
already produced for duplicates nested anywhere below the root. That contrast is
what places the defect in Detect and nowhere else.

Separately, diag.OneLine now bounds what a foreign error contributes to a
diagnostic message, and stops scanning at the bound rather than trimming
afterwards. That is the general form of the same defect — a message a library can
make arbitrarily large — and it reaches the two overlay callers as well:
soaoverlay.ParseReader has the same quadratic decode, and while its 6.5 s is
inside the library, its complaint no longer reaches the terminal whole. The cut
lands on a rune boundary, so a diagnostic never carries half a rune to a reader.

Two rules the walk now has

Neither was available before, because the decoder refused any mapping that
repeated a key at all:

Deliberately out of scope

  • The merge chain is bounded at maxMergeDepth, where the decoder followed one as
    far as yaml’s own alias limits. Recorded in internal/archtest as a bounded
    recursion, with the bound named.
  • An unreadable version key is still reported only where declaresProbeKey sees
    it declared at column 0; reached through a << it is indented, so the source is
    declined in silence. Widening that guard would claim documents of formats that
    nest a key of the same name, which its own doc comment argues against. Pinned as
    it stands in TestDetect_ReportsAnUnreadableVersionKeyOnlyWhereItIsDeclared.

Test plan

  • make gate exits 0. Changed files at 100% statement coverage.
  • Every new test reddens on a reverted decodeYAML, checked by putting the
    old body back: the outcome tests, the parser-agreement test in both orders, the
    merge-depth bound, and the cost bound. The two that stay green are non-regression
    pins for behaviour the old decoder also had.
  • Three mutations planted on OneLine — no cut, a cut ignoring rune
    boundaries, and an unbounded scan. The third initially passed: the test compared
    outputs rather than work, so it was rewritten to measure allocations, and now
    fails on it.
  • The cost bound measures growth rather than paying for it. The fixture repeats a
    key 512 times, not the report’s 6,553: at that size a revert exhausts memory and
    is killed instead of failing an assertion. Measured at 512→1024, the linear
    reading grows ×1.97 and the quadratic one ×4.64; the bound is ×3.
  • TestCodes_MatchTheDeclaredSet counted every exported constant as a diagnostic
    code, which the new MaxQuotedErrorBytes is not. It now counts exported string
    constants, reading the kind off the declaration rather than the name — verified
    still to redden on a code added without being listed.

Note for whoever merges second

This conflicts with #441 in compilers/openapi/detect.go, which rewrites ~190
lines of the same file. Verified by attempting the rebase. Both are correct
against main; the second to land needs the resolution.

🤖 Generated with Claude Code

https://claude.ai/code/session_016EHKV7ZYQJJXCPyynTWq4P

Format detection decoded a document's whole root mapping into a two-field
struct to read the `openapi` / `swagger` key. yaml.v3 compares every pair of a
mapping's keys before it reads any of them, so a mapping repeating one key n
times raises n(n-1)/2 errors and then abandons the mapping — the probe came
back empty as well as expensive. A 32 KB source repeating one key 6,553 times
produced 21,467,628 errors and a 1.2 GB diagnostic in 16.7 s; a 128 KB one did
not finish in 150 s. Both were reported as unreadable, though the parser the
compiler goes on to use reads them and reports the repeats itself, once each
and sited.

Detection now parses the document and reads the two keys off the tree, which is
linear and answers the same for a mapping whose keys repeat as for one whose
keys do not. The 32 KB case takes 0.048 s and prints 6,553 sited warnings; the
128 KB case takes 0.147 s.

Separately, diag.OneLine now bounds what a foreign error contributes to a
diagnostic message. That is the general form of the same defect — a message a
library can make arbitrarily large — and it covers the two overlay callers as
well, where the library's own decode is still slow but its complaint no longer
reaches the terminal whole. The cut lands on a rune boundary, so a message
never carries half a rune to a reader.

Two rules the walk now has and the decoder could not, since it refused any
mapping that repeated a key at all: a key written twice takes its last
spelling, matching the parser that later records the dialect on ir.SourceInfo,
so one document cannot get two answers; and a key written directly beats one
merged in through `<<`.

Deliberately out of scope: the merge chain is bounded at maxMergeDepth, where
the decoder followed one as far as yaml's own alias limits, and detection still
reports an unreadable version key only where declaresProbeKey sees it declared
at column 0 — widening that guard would claim documents of formats that nest a
key of the same name.

Closes #443

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016EHKV7ZYQJJXCPyynTWq4P
@fuad-daoud
fuad-daoud merged commit d0e25a0 into main Sep 10, 2026
1 check passed
@fuad-daoud
fuad-daoud deleted the fix/detect-duplicate-key-blowup branch September 10, 2026 10:44
@fuad-daoud
fuad-daoud restored the fix/detect-duplicate-key-blowup branch September 10, 2026 18:17
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.

Duplicate top-level keys blow up format detection: 32 KB file emits a 1.2 GB diagnostic

2 participants