From 5bc15497b1478223f961c2f03e8325419361b086 Mon Sep 17 00:00:00 2001 From: goodboy Date: Thu, 25 Jun 2026 19:20:30 -0400 Subject: [PATCH] Harden the `.. d2::` directive render path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three robustness fixes surfaced by the PR #460 self-review, - a render that's *attempted and fails* (a `d2` bin is present but errors on the source) now raises a docutils error instead of silently serving the stale committed SVG — so `sphinx-build -W` fails on a broken `.d2` edit. The no-binary case stays a quiet committed-SVG fallback. - render into a sibling temp-file then `os.replace()` so a failed/torn render can never clobber a good committed SVG. - guard against 2 distinct `.d2` sources colliding on a single `_diagrams/.svg` within a build. `render_svg()` now returns a `RenderResult` tristate that `run()` maps to error / raw-source-literal / image. (this patch was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code --- docs/_ext/d2diagrams.py | 140 ++++++++++++++++++++++++++++++++++------ 1 file changed, 120 insertions(+), 20 deletions(-) diff --git a/docs/_ext/d2diagrams.py b/docs/_ext/d2diagrams.py index 02e4d7d5..c57f9028 100644 --- a/docs/_ext/d2diagrams.py +++ b/docs/_ext/d2diagrams.py @@ -16,6 +16,14 @@ Rendering policy, emitted as a literal block so no content is ever silently dropped. +A render that is *attempted and fails* (a ``d2`` bin is +present but errors on the source) is NOT silently +degraded to the stale committed SVG: it raises a +docutils error (so ``sphinx-build -W`` fails the +build). The render is also atomic — a failed/torn +render can never clobber a good committed SVG — so the +last-good diagram survives a bad edit. + Binary discovery order, - the ``D2_BIN`` env var; may contain args which are @@ -37,11 +45,13 @@ Usage, ''' from __future__ import annotations +import enum import os from pathlib import Path import shlex import shutil import subprocess as sp +import tempfile from docutils import nodes from docutils.parsers.rst import directives @@ -55,6 +65,22 @@ log = logging.getLogger(__name__) # git-committed, fallback SVG outputs. _outdir: str = '_diagrams' +# per-build map of {output-svg-name: source-relpath} used +# to detect 2 distinct `.d2` sources colliding on a single +# output stem; reset on each `builder-inited` (see `setup`). +_seen_outputs: dict[str, str] = {} + + +class RenderResult(enum.Enum): + ''' + Outcome of a `render_svg()` call. + + ''' + OK = 'ok' # fresh SVG exists on disk + FELL_BACK = 'fell_back' # no bin; serving committed SVG + NO_OUTPUT = 'no_output' # no bin AND no committed SVG + FAILED = 'failed' # bin present, render errored + def find_d2( app: Sphinx, @@ -77,37 +103,54 @@ def render_svg( app: Sphinx, src: Path, out: Path, -) -> bool: +) -> RenderResult: ''' - Maybe (re)render `src` -> `out`, returning - `True` when an up-to-date SVG exists after the - call. + Maybe (re)render `src` -> `out` and report the + outcome as a `RenderResult`. + + A render is performed only when `out` is missing or + older than `src` AND a `d2` binary is available. The + write is atomic (temp-file + `os.replace`) so a + failed or torn render never clobbers an existing + (committed) SVG. ''' - stale: bool = ( - not out.exists() - or - src.stat().st_mtime > out.stat().st_mtime + fresh: bool = ( + out.exists() + and + src.stat().st_mtime <= out.stat().st_mtime ) - if not stale: - return True + if fresh: + return RenderResult.OK d2cmd: list[str]|None = find_d2(app) if d2cmd is None: + # no binary: fall back to whatever is committed, + # else signal the caller to emit the raw source. if out.exists(): log.info( f'no d2 binary; using committed svg ' f'for {src.name}' ) - return True - return False + return RenderResult.FELL_BACK + return RenderResult.NO_OUTPUT out.parent.mkdir( parents=True, exist_ok=True, ) + # render into a sibling temp-file first so a bad + # `d2` exit (or a crash mid-write) leaves any prior + # committed SVG fully intact. + fd, tmpname = tempfile.mkstemp( + dir=str(out.parent), + prefix=f'.{out.stem}.', + suffix='.svg.tmp', + ) + os.close(fd) + tmp = Path(tmpname) argv: list[str] = ( d2cmd + list(app.config.d2_args) - + [str(src), str(out)] + + [str(src), str(tmp)] ) try: proc = sp.run( @@ -120,18 +163,22 @@ def render_svg( OSError, sp.TimeoutExpired, ) as err: + tmp.unlink(missing_ok=True) log.warning( f'd2 invocation failed for {src.name}: ' f'{err}' ) - return out.exists() + return RenderResult.FAILED if proc.returncode != 0: + tmp.unlink(missing_ok=True) log.warning( f'd2 render error for {src.name}:\n' f'{proc.stderr}' ) - return out.exists() - return True + return RenderResult.FAILED + # atomic swap-in of the freshly rendered SVG. + os.replace(tmp, out) + return RenderResult.OK class D2Diagram(SphinxDirective): @@ -169,18 +216,54 @@ class D2Diagram(SphinxDirective): / _outdir / f'{src.stem}.svg' ) - if not render_svg(self.env.app, src, out): - # last resort: emit the raw d2 source. + # collision guard: two distinct sources must not + # map onto the same output stem within a build. + prior: str|None = _seen_outputs.get(out.name) + if ( + prior is not None + and + prior != relsrc + ): + err = self.state_machine.reporter.error( + f'd2 output collision: {relsrc!r} and ' + f'{prior!r} both render to ' + f'{_outdir}/{out.name}; rename one ' + f'`.d2` stem', + line=self.lineno, + ) + return [err] + _seen_outputs[out.name] = relsrc + + result: RenderResult = render_svg( + self.env.app, + src, + out, + ) + if result is RenderResult.FAILED: + # loud, build-failing (under `-W`) signal; the + # prior committed SVG, if any, is untouched. + err = self.state_machine.reporter.error( + f'd2 render failed for {relsrc} (see ' + f'build log); the committed svg, if any, ' + f'was left untouched', + line=self.lineno, + ) + return [err] + if result is RenderResult.NO_OUTPUT: + # last resort: emit the raw d2 source so no + # content is ever silently dropped. log.warning( f'no svg available for {relsrc}; ' f'emitting d2 source as literal block' ) + src_text: str = src.read_text() literal = nodes.literal_block( - src.read_text(), - src.read_text(), + src_text, + src_text, ) literal['language'] = 'text' return [literal] + # OK | FELL_BACK -> embed the SVG figure. img = nodes.image( uri=f'/{_outdir}/{out.name}', alt=self.options.get( @@ -224,12 +307,29 @@ class D2Diagram(SphinxDirective): return [fig] +def _reset_seen( + app: Sphinx, +) -> None: + ''' + Clear the per-build output-collision map at the + start of each build. + + ''' + _seen_outputs.clear() + + def setup(app: Sphinx) -> dict: app.add_config_value('d2_bin', None, 'env') app.add_config_value('d2_args', [], 'env') app.add_directive('d2', D2Diagram) + app.connect('builder-inited', _reset_seen) return { 'version': '0.1.0', + # NB: run() writes the rendered SVG during the + # READ phase; the temp-file + `os.replace` swap + # keeps that atomic so concurrent renders of the + # same diagram (under `sphinx-build -j`) can't + # tear the output file. 'parallel_read_safe': True, 'parallel_write_safe': True, }