Skip to content

docs(0110): close the synth split as won't-do — the tail is 29s - #165

Merged
adamkoot merged 6 commits into
developfrom
perf/0110_ci-split-synth-job-drop-node-from-rust
Aug 5, 2026
Merged

docs(0110): close the synth split as won't-do — the tail is 29s#165
adamkoot merged 6 commits into
developfrom
perf/0110_ci-split-synth-job-drop-node-from-rust

Conversation

@adamkoot

@adamkoot adamkoot commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Close 0110 as won't-do — the synth tail is 29s, not 2–3 minutes

This PR started as a throwaway A/B measurement and is now the decision record it produced. ci.yml ships comment-only changes; everything else is lore.

Task 0110 assumed the Node/TypeScript tail on the rust job cost 2–3 minutes. Measured directly, it costs 29 seconds — and splitting synth into its own job would make CI slower. Closed under the task's own AC #2, which permits exactly that with the number recorded.

Method

Two commits pushed 95 seconds apart on this branch, so both runs shared runner class and cache state:

  1. Baseline — comment-only change to ci.yml (run 30904595104, rust job 5m08s)
  2. Experiment — the three tail steps deleted (run 30904701513, rust job 4m17s)

Both were reverted in 0aae4f4. Step-level timings, not job totals: Build Lambda bootstraps alone varied 17s between the two runs on identical content, so the 51s job delta is not the saving.

Result

rust job PR wall-clock
today 5m08s 5m08s
with a separate synth job ~4m39s ~5m29s – 5m49s

The tail is 29s, of which only ~20s actually disappears — synth's own 9s relocates into the new job. That job costs ~50–70s, serialized after rust because it needs its artifact, plus an unmeasured actions/upload-artifact step for ~110 MB.

The real cost centre is Build Lambda bootstraps at 3m24s — 67% of the job. This task optimized the wrong 10%.

What ships

  • ci.yml — a comment block at the setup-node step carrying the number, so the next reader doesn't re-open this. git diff develop -- .github/workflows/ci.yml shows only # lines; every executable step is untouched and the 0077 guards are intact.
  • Task 0110 archived as canceled / obsolete, converted to a directory.
  • notes/G-option-1-synth-split.md — the complete, never-CI-tested implementation of the split, kept in case the arithmetic ever changes.
  • lore/3-wiki/project/ci-pipeline.md — measured cost profile of the rust job.
  • New task 0145cdk synth never runs on infra-only PRs.

Review

Findings 1–6 of the review on this PR are applied. The one that outlived the task: 0110 had ticked "synth still runs on every PR that could break it", and it does not — the rust job's paths filter omits infra/**, so an infra-only PR with a stack-level construct error goes green and fails at deploy. That is the 0070 failure mode, still open. The overclaim is withdrawn here and the gap is now 0145.

Reverts 5bbfbc8 (baseline marker) and a74077b (experiment: Node/TS tail
removed). Both were pushed only to force two comparable `rust` runs on
this branch and were marked do-not-merge in their own bodies.

The measurement they produced is recorded in task 0110. ci.yml is now
byte-identical to blob 91e84b7, its state at 5c37ad7, so the 0077 Lambda
asset guards are provably untouched.
The task assumed the rust job's Node/TS tail cost 2-3 minutes. Measured
via an A/B on this branch (runs 30904595104 / 30904701513), it costs 29s
of a 5m08s job — off by ~5x.

Only ~20s of that is removable; synth's own 9s relocates rather than
disappearing, and the replacement job would cost ~50-70s serialized after
rust because it needs rust's ~110 MB artifact. PR wall-clock would go
5m08s -> ~5m50s and billed minutes would rise. The split also would not
remove the duplicate TypeScript build, which was the other stated
motivation.

The step actually worth attention is `Build Lambda bootstraps` at 3m24s,
67% of the job. Recorded so this is not re-opened on intuition.

Adds lore/3-wiki/project/ci-pipeline.md documenting the pipeline's current
shape and cost profile, and a pointer comment at the setup-node step so
the tail is not mistaken for cruft. Archives 0110.
@adamkoot
adamkoot marked this pull request as ready for review August 4, 2026 13:40
@karczuRF

karczuRF commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Code review — perf/0110_ci-split-synth-job-drop-node-from-rust @ e806236

Scope note: this PR is no longer what its title and body describe

The title says "MEASUREMENT ONLY — do not merge" and the body says "Either way this branch gets reset afterwards" and "The experiment commit deliberately removes a real guard (synth catches CannotFindAsset before deploy — task 0070). That is safe only because this PR is never merged."

As of e806236 that is no longer true: the head is two measurement commits, a revert, and a docs commit — a real deliverable that restores the guard and closes 0110 won't-do. A reviewer acting on the current body either blocks a merge that should land, or believes the head still has the guard removed. Please rewrite the title and body to describe the won't-do closure before merging. This review covers the merged effect of all four commits.

What checks out: the load-bearing measurements are accurate against the Actions API — runs 30904595104 (311s rust job; tail 7+13+9=29s) and 30904701513 (260s; Build Lambda bootstraps 204s→187s), SHAs match the two commits, pushed 95s apart. The ci.yml change is comment-only, parses as valid YAML, and keeps all 13 rust steps. Both changed files are prettier-clean under nx format:check --all. The revert is genuine: the blob at 0aae4f4 is 91e84b7, identical to develop.


1. status: done is not a valid status and fails the lore validator — medium

lore/1-tasks/archive/0110_PERF_ci-split-synth-job-drop-node-from-rust.md:5

lore/1-tasks/archive/CLAUDE.md enumerates completed / superseded / canceled; the repo uses completed 85 times vs done 4. Running lore-framework_validate against an existing status: done file errors:

Invalid enum value. Expected 'active' | 'blocked' | 'completed' | 'superseded' | 'canceled' | 'backlog', received 'done'

This file produces two such errors (frontmatter line 5, history line 28). Further: the archive convention for a won't-do closure is specifically status: canceled with a reason: key — which is exactly what this task is. The ad-hoc wont-do / decision tags on line 8 appear nowhere else in the repo and don't substitute for the status encoding.

2. The "byte-identical" claim is false in the shipped state, and AC #3 rests on it — medium

lore/1-tasks/archive/0110_PERF_ci-split-synth-job-drop-node-from-rust.md:38

The history note asserts "ci.yml is byte-identical to its pre-0110 state (blob 91e84b7), so the 0077 guards are untouched", and line 125 repeats it: "Byte identity with the pre-task blob is the proof."

But this PR ships ci.yml at blob f305ff6a, not 91e84b7 (git rev-parse e806236:.github/workflows/ci.yml). The doc contradicts itself at line 203, Design Decision 5: "the one deliberate deviation from byte-identity".

The change is a comment and is harmless — but the next reader who runs the stated verification gets a mismatch and has no way to tell whether the deviation is the sanctioned comment or a real edit. State the actual proof instead: "identical except an added comment block; git diff develop -- .github/workflows/ci.yml shows only # lines."

3. The documented recovery path for the option-1 implementation does not work — medium

lore/1-tasks/archive/0110_PERF_ci-split-synth-job-drop-node-from-rust.md:158

The doc says the ~82-line split implementation "is preserved in a git stash on this branch" and instructs:

git stash list | grep lore-0110

Git stashes are local to one working copy — they are not on a branch, not pushed, and not part of this PR. That command returns nothing in this clone (the only stash present is stash@{0}: On feat/0038_...: 0063-switch-cargo-lock).

Design Decision 4 (line 199) justifies stashing specifically so the code doesn't have to be rewritten if the arithmetic changes — but the only copy lives on one machine and dies with a git stash clear or a fresh clone. lore/3-wiki/project/ci-pipeline.md:100 repeats the claim. If the code is worth keeping, push it to a branch or paste it into the task file; otherwise drop both claims rather than sending a future reader after something that isn't there.

4. The synth-coverage claim (and AC #1) is wrong for infra-only PRs — medium

lore/3-wiki/project/ci-pipeline.md:45

The wiki presents make -C infra synth-production as "final proof the app the operator deploys actually synthesizes", and the archived task ticks AC #1 "Synth still runs on every PR that could break it."

But synth lives in the rust job, whose paths filter is packages/**, Cargo.toml, Cargo.lock, .github/workflows/ci.yml, tools/scripts/**infra/** is only in the typescript filter.

Concrete scenario: a PR editing only infra/lib/*.ts that introduces a stack-level construct error runs the typescript job (nx run-many -t lint build typecheck plus verify-lambda-assets.sh), none of which invokes cdk synthnx build is a tsc build, per infra/Makefile:4. CI goes green and the failure surfaces at deploy, which is the 0070 failure mode. verify-lambda-assets.sh only closes the asset-name-mapping subset of this.

Related: synth-cicd is never run by CI at all. The "Known gap" section records the develop-push hole but not this larger one, which is the gap most likely to bite.

5. The decision table double-counts synth's 9s, and omits the artifact upload — low

lore/1-tasks/archive/0110_PERF_ci-split-synth-job-drop-node-from-rust.md:113

Line 103 states synth relocates to the new job, and the new job's estimate at line 106 explicitly includes synth 9s. But the "after split" rust job is given as ~4m48s — only 20s below today's 5m08s. Synth's 9s is charged to the new job and left in the rust job. With synth relocated the rust job is ~4m39s, so the wall-clock and billed columns (~5m50s / ~5m48s) are each ~9s high.

Separately, the model omits the actions/upload-artifact step the split would add to the rust job for ~110 MB.

Neither error flips the conclusion, but these exact numbers are now quoted verbatim in .github/workflows/ci.yml:200-202 and lore/3-wiki/project/ci-pipeline.md:90, so the inconsistency propagates to three places.

6. The "billed minutes" column isn't a billing figure — low

lore/1-tasks/archive/0110_PERF_ci-split-synth-job-drop-node-from-rust.md:110

GitHub bills each job rounded up to the whole minute, at different per-minute rates per runner class. Today's single ARM job at 5m08s bills 6 ARM minutes; after a split it would be 5 ARM minutes plus 1–2 minutes on whatever runner the synth job used. Presenting 5m08s → 5m48s as billed minutes implies second-granularity, same-rate billing that doesn't exist.

The conclusion (the split is worse) survives under real billing too, so this is presentation rather than outcome — but a decision record whose entire value is the number should not put a number in the "billed minutes" column that no invoice would ever show.


Summary: findings 1–3 are the ones worth acting on before merge — the status value breaks the validator, and two documented verification paths (byte-identity, git stash) don't work for anyone who follows them. The measurement itself and the won't-do conclusion hold up.

@adamkoot adamkoot changed the title MEASUREMENT ONLY — do not merge (lore-0110): what does the synth tail actually cost? docs(0110): close the synth split as won't-do — the tail is 29s Aug 5, 2026
Six review findings, all in the decision record rather than in ci.yml.

Two documented verification paths did not work for anyone who followed
them. The task claimed ci.yml was "byte-identical" to its pre-0110 state,
but the pointer comment moved the blob to f305ff6 — the check now stated
is `git diff develop -- .github/workflows/ci.yml` showing only comment
lines, which is both true and reproducible. The option-1 implementation
was said to be "preserved in a git stash on this branch"; stashes are
local to one working copy and the reviewer got nothing. It is now
committed as notes/G-option-1-synth-split.md, which is what pushed this
task from a file into a directory.

`status: done` is not in the schema and failed the validator twice. The
archive convention for a won't-do is canceled + reason, now applied.

The decision table charged synth's 9s to both the rust job and its
replacement, and labelled a seconds-granularity column "billed minutes"
when GitHub bills per job rounded up to the whole minute. Corrected in
the task and the wiki; the conclusion is unchanged either way.

The finding that outlived the task: AC #1 asserted synth runs on every
PR that could break it. It does not — the rust job's paths filter omits
infra/**, so an infra-only PR carrying a stack-level construct error is
never synthesized and fails at deploy instead. That is the 0070 failure
mode, still open. Overclaim withdrawn, gap spawned as 0145.

Not fixed, deliberately: the validator also rejects `type: PERF` (6 tasks
use it) and unquoted history dates (237 unquoted, 0 quoted repo-wide).
Both are repo-wide schema drift; fixing them here would leave this file
the only one that differs from its siblings.
The pointer comment asserted the synth split makes "billed minutes"
worse, which is the exact phrasing PR #165 review finding 6 objected
to: GitHub bills each job rounded up to the whole minute at
per-runner-class rates, so no seconds-granularity figure is an invoice
figure. The task file and the wiki both carry that caveat; ci.yml was
the one place left stating it flat.

Now says the split saves nothing on billing, with the real arithmetic
inline (6 ARM minutes today vs 5 ARM plus 1-2 on the synth runner).
The wall-clock claim is unchanged and still the load-bearing one.
Comment-only, as before.
@adamkoot

adamkoot commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

@karczuRF ready for re-review at e54d147. All six findings applied plus the scope note.

# Finding Applied in
Title/body still said "MEASUREMENT ONLY — do not merge" both rewritten to describe the won't-do closure
1 status: done fails the validator canceled + reason: obsolete, frontmatter and both history entries
2 "byte-identical" false in the shipped state replaced everywhere with git diff develop -- .github/workflows/ci.yml shows only # lines — currently 0 non-comment changed lines
3 option-1 impl unrecoverable from a local stash committed as notes/G-option-1-synth-split.md; task + wiki repointed. This is what forced 0110 from a file into a directory
4 synth-coverage claim / AC #1 wrong for infra-only PRs overclaim withdrawn in AC #1; wiki gained a Known gaps entry (incl. synth-cicd never running); spawned as 0145
5 table double-counts synth's 9s, omits the upload corrected to ~4m39s rust / ~5m29s–5m49s wall-clock, double-count called out; the unmeasured ~110 MB upload-artifact noted as making the low end optimistic. Consistent across task, wiki and ci.yml
6 "billed minutes" isn't a billing figure column dropped for an On billing paragraph. e54d147 also fixes the one place left stating it flat — the ci.yml comment now reads "saves nothing on billing", with 6 ARM minutes today vs 5 ARM plus 1–2 on the synth runner

Two things worth your call rather than mine:

  • The validator still errors on this filetype: PERF (not in the enum; 6 tasks use it) and unquoted history dates (542 unquoted, 0 quoted repo-wide). Both are repo-wide schema drift, so fixing them here would leave 0110 as the only conforming file. Left alone deliberately; happy to spawn a task for the schema if you want it owned.
  • The wont-do / decision tags you flagged are still on line 8. The status encoding is now doing the real work, so they are redundant rather than wrong — say the word and they go.

The measurement and the won't-do conclusion are unchanged.

@adamkoot
adamkoot requested a review from karczuRF August 5, 2026 10:12
@adamkoot
adamkoot merged commit 61a1090 into develop Aug 5, 2026
3 checks passed
@adamkoot
adamkoot deleted the perf/0110_ci-split-synth-job-drop-node-from-rust branch August 5, 2026 10:26
karczuRF added a commit that referenced this pull request Aug 5, 2026
The ID was claimed twice. `0145_BUG_synth-not-run-on-infra-only-prs`
landed on develop with PR #165 (the 0110 won't-do closure), while the
unmerged PR #168 branch had already claimed 0145 for the pre-roll
`argMax(close_usd, ...)` guard.

The pre-roll task keeps the number: it is referenced nine times across
four files, gates both the 0088 pass-2 pre-roll and 0136's 07-21 gap
pre-roll, and develop's own generated index already resolves 0145 to it.
This task is referenced three times and moves.

0152 is deliberately left free for the OpenAPI license task on PR #169,
which collides with 0144 the same way and is not ours to renumber.

Nothing about the synth work itself changed. lore/README.md is not
regenerated here — it already points 0145 at the pre-roll task, and
regenerating on this branch would conflict with PR #168's index.
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