Review group commit in production and record what it exposed

[tonyalaribe]

Docs only.

Group commit works

metric	before (comparable age)	now
journal_hold.avg_us	5,452	3,093
journal_hold duty cycle	18.5%	14.5%
journal_stats_publishes_total	n/a	0.80/s (target ~1/s)

Two wins, not one, and they should not be conflated: #263 removed the full-journal rescan (the 0.80/s confirms it), group commit moved the fsync out from under the global mutex (the duty-cycle drop).

The comparison uses the 18.5%-at-34-min reading, not the 29.6% from the 19 h process in the disk-flood doc — a mature process is not the comparator for a 44-minute one.

The cost became measurable rather than disappearing

performed 30,075 + coalesced 30,506 = 60,581 waiters exactly. 11.4 fsync cycles/s is one per 88 ms, and 11.4 × 88 ms is the whole second — the commit pipeline runs at ~100% duty, so every arrival queues. journal_commit_wait averages 362 ms, max 10.7 s, on the pre-ack path.

The ~2x coalescing is optimal, not disappointing: at 23 arrivals/s a batch can only contain the ~2 that arrive per 88 ms cycle. Batching harder buys nothing — the lever is cost per commit, which is PR #265.

Three new findings

1. The deploy pipeline strands merged code — observed live today. Mint a base rebuild only for refusals the current generation cannot reproduce #262 merged 09:48Z, Build and Deploy green at 10:08:55Z, log ends A newer master commit superseded this rollout; production is unchanged. Superseded by two docs-only pushes. UpdatedAt=09:39:25Z — over an hour stranded, green job, nothing reporting it. Second instance after Weight each backfill project's share by its uncovered backlog #251. Fixed by Do not let a docs-only merge cancel a code deploy #258, merged today after sitting open since 09-10.
2. Journal history grows without bound. tasks_complete 71,399 (09-11) → 78,907 (today); nothing prunes Complete tasks, and compact() rewrites the whole snapshot under the global mutex at a size tracking history rather than load.
3. The no-op rollup skip is defeated by restart frequency. It read 0 on a 31-minute process, because boot-recovered coverage carries content_fp: None — and production restarted three times in two and a half hours today. This makes the content-fingerprint tag follow-up worth considerably more than it looked when written.

🤖 Generated with Claude Code

[claude]

Reviewed. This is a docs-only PR (single new file, docs/plans/2026-09-12-group-commit-in-production.md, 126 lines, no code/tests/config touched), so the usual bug/perf/security/test-coverage checklist doesn't map directly — I focused on verifying the technical claims against the actual code and checking the analysis for internal consistency.

Fact-check against the codebase — all correct:

• TaskJournal::checkpoint really does exactly one sync_all on the task WAL (src/maintenance_coordinator.rs), and persist_rollup_journal → write_atomic_with(durable=true) really does two syncs (temp file + parent dir) — matches the "three fsyncs per commit, two of them the rollup journal" claim.
• The quoted module doc for rollup_journal ("scheduling state, not the read-side correctness boundary…") and for maintenance_tasks ("finer-grained source of truth…") match the source verbatim.
• apply_rollup_hours does increment rollup_source_epochs on every call, supporting the "content hash alone would have shipped inert" argument.
• content_fp: None on boot-recovered coverage paths is real, backing finding Otel #3.
• The arithmetic all checks out: 30,075 + 30,506 = 60,581 exactly; 11.4/s × 88 ms ≈ 1 s; 23 arrivals/s × 88 ms ≈ 2/cycle.
• Both referenced follow-up docs (2026-09-11-noop-skip-production-result.md, 2026-09-11-write-latency-self-inflicted-disk-flood.md) exist, and the PR Mint a base rebuild only for refusals the current generation cannot reproduce #262/Do not let a docs-only merge cancel a code deploy #258/Weight each backfill project's share by its uncovered backlog #251 timeline matches actual merge times.

One gap worth a mention: the summary table reports journal_lock_wait.avg_us going from 1,907 → 4,017 (more than doubling) alongside the hold-time and duty-cycle wins, but the surrounding text only discusses the two improving metrics and never explains the wait-time regression. Since group commit moves fsync out from under the mutex, it's plausible this is contention from more frequent short acquisitions rather than a real problem, but as written a reader is left to guess. Worth a sentence on why that number moved the "wrong" way, or a note that it's out of scope for this pass.

Strengths:

• Careful to avoid conflating two separate wins (PR Stop rescanning the whole journal on every checkpoint #263's rescan removal vs. group commit's mutex change) and explicitly calls out the correct comparator for the duty-cycle numbers (34 min vs. the mature 19 h process) rather than cherry-picking the more flattering delta.
• The pgwire latency table is honestly caveated as directional only, given the process-age mismatch — good instinct not to oversell a comparison you know is confounded.
• Findings are concrete and actionable (unbounded Complete task retention driving compact() cost, deploy pipeline stranding merged code, restart frequency defeating the no-op rollup skip) rather than vague observations.

Suggestion: items 1–3 under "New findings" read like real bugs/gaps (unbounded journal growth, deploy pipeline silently stranding rollouts) rather than just narrative — if they aren't already tracked as issues/follow-up PRs, it'd be worth filing them so they don't get lost inside a dated plan doc.

No blocking issues — good to merge as documentation.

[claude]

Reviewed. This is a docs-only PR (one new file, docs/plans/2026-09-12-group-commit-in-production.md), so I focused the review on factual accuracy of the technical claims and internal consistency of the numbers, rather than code style.

Verification against the codebase

I checked the claims that are falsifiable against source, and all of them hold up:

• commit_journal (src/database/maintain.rs:4263) does call self.journal().checkpoint() then self.persist_rollup_journal() inside journal_group_commit.commit(...), matching "a commit runs TaskJournal::checkpoint ... and then persist_rollup_journal."
• TaskJournal::checkpoint (src/maintenance_coordinator.rs:3289) does exactly one wal.sync_all() — matches "one sync_all on the task WAL."
• persist_rollup_journal -> rollup_journal::store -> write_atomic_with(&path, true, ...) (src/write/wal.rs:1340) does sync_all on the temp file, renames, then sync_all on the parent dir — matches "two of the three [fsyncs] are the rollup journal."
• The rollup_journal.rs module doc is quoted verbatim ("scheduling state, not the read-side correctness boundary ... an absent dirty entry already means full rebuild required to the builder").
• The maintenance_tasks field doc in src/database/mod.rs:2919-2921 is also quoted accurately ("the finer-grained source of truth coordinator workers consume").
• apply_rollup_hours (src/database/maintain.rs:4284) does increment rollup_source_epochs unconditionally on every call (and_modify(|epoch| *epoch = epoch.saturating_add(1)).or_insert(1)), which backs the "a content hash alone would have shipped inert" argument.
• PR cross-references check out: Stop rescanning the whole journal on every checkpoint #263 ("Stop rescanning the whole journal on every checkpoint", merged 2026-09-11) is correctly attributed to the rescan removal; Do not let a docs-only merge cancel a code deploy #258 ("Do not let a docs-only merge cancel a code deploy", merge commit d894483...) matches the "merged today (d8944835)" claim and its timestamp; Stop paying the rollup journal's two fsyncs on every commit #265 is correctly described as open/forthcoming for the two-fsync fix; Mint a base rebuild only for refusals the current generation cannot reproduce #262 merge time (09:48:03Z) matches the doc.
• The internal arithmetic is self-consistent: 30,075 + 30,506 = 60,581 waiters exactly; 60,581 / 2640s ≈ 23/s matches the later "23 arrivals/s"; 11.4/s x 88ms ≈ 1.0s supports the "~100% duty" claim; 361,837 us ≈ 362 ms and 10,687 ms ≈ 10.7 s are correctly converted.

Minor notes (non-blocking)

• The cross-references to 2026-09-11-noop-skip-production-result.md and 2026-09-11-write-latency-self-inflicted-disk-flood.md are plain filenames rather than relative markdown links — but that matches the existing convention in this docs directory, so it is consistent rather than an inconsistency to fix.
• The pgwire latency comparison table is explicitly caveated as a 44-minute vs. 19-hour process comparison and told not to be over-read — good call flagging that inline rather than letting the "directionally large" numbers stand alone.
• One thing not verifiable from the repo (it is runtime data, not code): the raw metric values themselves (journal_hold.avg_us, tasks_complete counts, pgwire percentiles, etc.) come from timefusion_stats snapshots and are not reproducible from source, so I am trusting the sampled numbers as reported.

Overall: the technical claims about code behavior are accurate and well-sourced, the arithmetic is internally consistent, and the PR attributions match actual merge state. No code/security/test-coverage concerns since this changes no code. Nice level of rigor distinguishing the two conflated wins (#263 vs. group commit) and correctly flagging PR #265 as the next lever rather than overselling batching.

🤖 Generated with Claude Code