Feat: show which work runs inside a database transaction

[webard]

The graph knew what calls what and nothing about what is atomic. A service that writes three tables inside one transaction looked exactly like one that writes them in three — which is the difference between a bug and not a bug.

Nodes now carry inTransaction, inRollback and a transactionId, and the canvas draws a boundary around the work that shares one.

the region, where it can be drawn	and where it cannot

A span, not a step — the decision everything else follows from

The obvious shape is a TRANSACTION START / TRANSACTION END pair of nodes. It is wrong three times over:

• A transaction does not do anything, it contains what others do. A node beside a service reads as the service calling it.
• It adds two hops to every path through it, quietly changing every fan-out, shortest path and blast radius the graph reports.
• beginTransaction() and commit() are frequently in different methods, so there is often no single place to put either end. Eight of the spans measured here are that shape.

An attribute plus a region says the same thing and changes no topology at all: this PR adds no node and no edge.

Both forms, because both are written

Measured on the application this was built against: 103 closure transactions, 8 hand-rolled pairs, 7 rollbacks.

DB::transaction(fn () => ...)             the body is the span
DB::beginTransaction() ... DB::commit()   the statements between are the span

The closure form is a subtree and nests for free. The hand-rolled form is a range within a statement list — beginTransaction() and commit() are siblings — which cannot be read off the tree shape at all, only off statement order. Two passes, for that reason.

The method names are Laravel's: transaction, beginTransaction, commit, rollBack. There is no startTransaction or endTransaction in the framework, and a detector keyed on those would find nothing and say nothing.

The rollback path is tracked separately, because it is a different claim. A catch block that calls rollBack() runs with the transaction already gone: a write there survives, and an event fired there announces a failure rather than a fact.

One region per span, and only when it is true

A boolean would have been enough to tint a node. It is not enough to draw a region, because a region has to know which transaction — and the ordinary case is several things in one. The median span holds 8 calls, the largest 33, and only 9 of 92 hold a single call.

The layout arranges by call structure and knows nothing about transactions, so a shape around a span's members can enclose a node that was never in it. Every region is checked against every non-member, using the cards' corners rather than their centres — a wide node can sit with its centre outside a boundary and half its body inside, which passes a centre test and still reads as enclosed by anyone looking.

Then the layouts were taught to make the answer usually yes: layoutDagre builds a compound graph and gives a span's members a shared parent, so dagre places them as a unit; the breadth-first layout orders each layer so members sit together, changing only the order within a layer and never a node's level.

                     hierarchical   breadth-first   force   circle   grid
before clustering         ✗               ✗           ✓        ✓       ✓
after                     ✓               ✗           ✓        ✓       ✓

Breadth-first still falls back where a span's members sit on different levels — one calling the other puts a whole layer between them, and ordering within a layer cannot close that. The fallback is correct there rather than a defect being hidden.

Exactly one mark is drawn at a time. The hull where it can be, member outlines where it cannot, and the name goes with whichever is showing: once on the hull, or once on each member when there is no hull to hang it on. Regions are numbered so two on a canvas can be told apart, numbered by span id rather than discovery order so the number survives a relayout or a drag.

It can be switched off, in the places the canvas already lists what it draws

Show on graph gains a Transactions row — same click, same behaviour as every node type beside it, and on by default. It is not a node type, so its count is a number of spans rather than of nodes.

The canvas legend names them when the canvas holds any, after a separator rather than an arrow: the chain there is a sequence a request passes through, and a transaction is not a step in it. Its dot is hollow and dashed rather than filled, because every other entry is a kind of node and this is a boundary drawn around them.

Coverage, plainly

Five nodes are marked across nine tabs on the application measured, against 92 transaction spans in its source. The limit is the call chain's reach — it follows what a route or command reaches, and transactions live mostly in listeners and jobs it does not. The detector itself is not the limit: run directly over a file the chain never reaches, it finds the span and places 4 of 27 calls inside it. Nothing here changes how far the tracer goes.

Things the tests caught before they shipped

• A span ran past the block that opened it. if ($flag) { begin; …; commit; } was read as an opener at the outer level, so everything after the if was swallowed. Only an unbalanced opener extends into its siblings.
• transaction() is not a reserved word. A domain service with a method of that name would have put half of it inside a span that never existed. The receiver is checked.
• Regions were computed from the laid-out positions while the cards were drawn from the dragged ones, so moving a node left its boundary behind, still claiming a membership no longer on screen.
• Gating regions on the footer toggle made them vanish entirely, because the default visible set is built from node types and a transaction is not one.

Nine unit tests cover both forms, the rollback path, nesting, and the first two above.

414 passed (1153 assertions), PHPStan clean, Pint clean, tsc -b and eslint clean. One entry chunk and one stylesheet ship; the superseded ones are deleted.

[github-actions]

Benchmark

base 4bdba90 → head 6f812e9 · PHP 8.4.25 · 10 repetitions per arm, interleaved in one job

What the scan detects

Deterministic: both arms scan the same generated application on the same PHP version, and every repetition is checked to produce the same figures. A difference here is a change in what Brain detects, not machine noise — which is worth looking at rather than automatically worth fixing, since detecting more legitimately moves these.

Scenario	Nodes	Edges	Tabs	Routes	Security issues	parse() calls
Full scan — 398 files	721	1,961	196	160	80	219
Full scan — 1,188 files	2,133	5,785	584	480	240	639
Method tracing — every entry-point method	—	6,661	—	—	—	325

No count changed on either corpus, including the per-node-type breakdown.

Timing

Median wall clock on a shared CI runner. Noise is how far the base arm's own repetitions sat from its median, discounting the single worst on each side, with their full spread beside it; a delta inside the noise is not a measurable effect.

Scenario	Base	Head	Δ	Noise	Base spread
Full scan — 398 files	175 ms	191 ms	+9.1%	±8.4%	152–195 ms
Full scan — 1,188 files	530 ms	578 ms	+9.1%	±6.8%	511–567 ms
Method tracing — every entry-point method	150 ms	200 ms	+33.0%	±5.8%	137–180 ms

Phase split — Full scan — 1,188 files

A phase carries at least as much runner noise as the scan it is part of, so each one gets its own noise floor from the base arm's repetitions of that phase. Useful for locating a large move; a phase that is a few percent of the scan cannot be read at a few percent of accuracy.

Phase	Base	Head	Δ	Noise
lifecycle	121.1 ms	159.9 ms	+32.0%	±4.6%
graph	107.3 ms	113.7 ms	+6.0% (within noise)	±15.3%
queries	70.5 ms	72.3 ms	+2.6% (within noise)	±8.6%
controllers	63.4 ms	64.7 ms	+2.1% (within noise)	±3.2%
security	44.3 ms	45.5 ms	+2.9% (within noise)	±4.4%
commands	22.2 ms	22.6 ms	+1.3%	±1.3%
models	17.4 ms	17.8 ms	+2.6% (within noise)	±2.6%
routes	15.3 ms	15.2 ms	-0.2% (within noise)	±9.5%
facades	13.9 ms	13.9 ms	-0.2% (within noise)	±2.5%
split	13.7 ms	12.0 ms	-12.1% (within noise)	±13.3%
cmd_chains	5.9 ms	10.7 ms	+81.5%	±10.5%
observers	1.8 ms	1.8 ms	-1.1% (within noise)	±9.6%
policies	1.7 ms	1.6 ms	-0.6% (within noise)	±1.8%

_{Scans a synthetic Laravel application generated by benchmark/generate-corpus.php, not a real one — the shapes are chosen to stress the scan, so treat the absolute times as a workload, not as a user's scan. Reproduce locally with composer benchmark.}

[SanderMuller]

Reviewed at dcfa706b. The design argument is right and I checked the part it all rests on: this really does add no node and no edge. One thing blocks, and it is one missing argument at one of three call sites.

The blocking one: most marked nodes carry no span identity

CallChainEdge is constructed in three places in MethodTracer. Two pass transactionId. The third, in the loop over $discovered hops at line 243, passes the two flags and not the id:

$edges[] = new CallChainEdge(
    …
    inTransaction: $hop['inTransaction'] ?? false,
    inRollback: $hop['inRollback'] ?? false,
    // transactionId is not passed here, so it defaults to null
);

recordSpanState() binds with ??=, first write wins, so a node bound through that path keeps a null span for the rest of the build even when a later edge carries a real one. It is marked as inside a transaction and can never be drawn inside a region, which is the feature.

Measured on three applications, marked nodes against nodes that know which span they are in:

	inTransaction	with a transactionId	after adding the argument
A	148	28	148
B	216	151	216
C	93	49	93

I added transactionId: $hop['transactionId'] ?? null, at that site and re-ran the builds: every marked node gains an id, node and edge counts do not move on any of the three, and the suite stays at 417 passed. That is the whole change I am asking for.

Nothing in the suite would have caught it, which is the part worth fixing alongside: delete the same argument from either of the two sites that do pass it and all 417 tests still pass. The stamping path from edge to node data has no test on it, and this is the second time that shape of bug has cost this branch. The commit message on dcfa706b describes finding it from the other side.

The claim everything rests on holds

No node and no edge. Node and edge id counts are identical on all three applications: 49,292, 21,054 and 31,225 on both sides. The output differs only by added keys, and the classification is exact: inTransaction and transactionId appear, nothing is removed, and every other key balances added-against-removed. A null arm on each application came out identical, so those are the change's numbers.

inRollback appears on none of the three, which fits what you say about the tracer's reach rather than contradicting it.

The detector, against shapes your tests do not cover

try { begin; writeA; commit } catch { rollBack; notifyFailure }   writeA in span, notifyFailure in span + rollback, code after: clean
if ($flag) { begin; inside; commit }  … afterIf                   inside in span, afterIf clean
begin with no commit in the method                                span runs to the end, as documented
commit with no begin                                              nothing marked
foreach { begin; perRow; commit } … afterLoop                     perRow in span, afterLoop clean
begin; DB::transaction(fn () => inner); outer; commit             both inside
$this->db->transaction(...)                                       caught
DB::connection('tenant')->transaction(...)                        caught
$this->orders->transaction(...)                                   correctly ignored
begin; work; rollBack; afterRollback                              work in span, afterRollback outside

Ten shapes, ten right answers, including the two you say the tests caught before shipping. The receiver check earns its place: a domain service with a transaction() method is the shape that would quietly swallow half a method, and it does not.

Everything else I looked at

The committed bundle reproduces: npm ci && npm run build leaves git status empty, one entry chunk and one stylesheet are added with their predecessors deleted, and the blade names files that exist. 417 passed (1159 assertions), PHPStan clean, Pint clean. CI is green with per-job results across 15 jobs.

Small thing: the body says 414 passed (1153 assertions), which was true before dcfa706b added three tests. Worth refreshing when you push the fix.

Verdict

Requesting changes for the one argument. Everything else here is sound, and the parts I most expected to find wrong (the topology claim, the receiver guard, the unbalanced-opener case) are all right. Add the id at that third site and I will approve on the re-review; I have the counts to check it against.

[webard]

Fixed in 949ae73 — you were right on both halves, and the second one turned out to be the larger job.

The argument

transactionId: $hop['transactionId'] ?? null, added at the trace() site. Reproduced on a 60-module application before touching it — smaller than your numbers, because that application traces few transactions, but the same shape:

	marked	with a span id
before	6	5
after	6	6

Node and edge counts unchanged either side (4033 / 10295).

"Nothing in the suite would have caught it"

This is the part worth the time. The seam had unit tests on both ends and nothing across the middle: TransactionScopesTest proves the detector knows which statements are in the span, and TransactionScopeStampingTest feeds edges it constructs by hand. trace() could drop the identity between them and stay green — which is exactly what happened.

TransactionSpanReachesTheNodeTest now runs a fixture through the real pipeline (RouteAnalyzer → ControllerAnalyzer → MethodTracer → GraphBuilder) and asserts the identity arrives at both the edge and the node data. The fixture is deliberately shaped to reach all three construction sites: a controller method (trace()), a closure route wired exactly as ProjectAnalyzer wires it (traceClosure()), and a service that opens a span of its own around a deeper hop (traceDeep()).

Deleting the argument from any one of the three is now red:

no mutation              3 passed
remove at traceClosure   1 failed, 2 passed
remove at trace          2 failed, 1 passed
remove at traceDeep      1 failed, 2 passed

There is a negative control alongside it — a hop after the transaction closes must have neither the flag nor an id — so the rule cannot be satisfied by stamping everything.

Building that fixture turned up one thing worth recording, since it cost a few attempts: traceClosure() passes an empty dependency map to scanMethod(), so a closure route resolves nothing from a variable. function (Ledger $ledger) { … $ledger->reconcile(); } yields zero edges, and so does a plain Reconciler::run(). (new Reconciler)->handle() is what produces one. Not a defect for this PR, but it does mean closure routes see less than a reader would expect, and it is why the fixture is written the way it is.

Gate: pint clean, phpstan no errors, 420 passing.

Thanks for measuring it across three applications — the marked-versus-identified ratio is the framing that makes the bug obvious, and it is going straight into how I check this feature from now on.

[SanderMuller]

Re-reviewed at 949ae73, against dcfa706 — the head my last review described.

The blocking finding is fixed, and fixed at the line it was at: transactionId now travels with the other two flags out of the controller-method construction site in MethodTracer.

The probe that produced the finding, re-run. A controller whose method wraps a service call in DB::transaction(...), and the same shape on an invokable controller:

before   OrderService::createOrder    inTransaction=yes   transactionId=null
after    OrderService::createOrder    inTransaction=yes   transactionId='App\Http\Controllers\OrderController::store#0'

Both callees now carry the id of the span their controller opened, which is the same id the opener carries — so they are in one span rather than each in none.

The fix commit measured on three applications, null arm first (the previous head against itself, identical):

	nodes with inTransaction	had no span id	after	distinct spans	spans holding one node
A	40	27	0	12 → 12	11 → 2
B	73	19	0	30 → 30	16 → 9
C	17	6	0	8 → 8	6 → 3

The only field that changes on any node across all three is data.transactionId — 27, 19 and 6 nodes. No node, edge, subgraph or tab is added, removed or otherwise touched, and the inTransaction count does not move either, so nothing newly counts as being inside a transaction. The number of distinct spans is identical on all three: the fix joins nodes to spans that already existed rather than inventing any. That is what I would have asked for as proof it does not over-correct, and it is what the gate says.

The test bites. Removing the one line again fails two tests — it carries the span identity from a controller hop onto its edge and it stamps that identity onto the node the builder makes — one at the edge level and one at the node level, which is the right pair. The third test in the file is the negative control and correctly stays green.

Suite green on this head: 420 passed (1167 assertions), PHPStan no errors, Pint clean, 15/15 CI.

Worth knowing for the branch that sits on this one: #130 replaces this single transactionId field with a list of regions, and its version of this call site inherits the fix. I raised the same finding there against its older base; that review's numbers were measured before this commit existed.

Approving.

[SanderMuller]

Head moved to e657389 while I was writing the review above, so this re-points the approval at the commit it actually describes.

The delta is one commit touching one file: .gitignore goes from docs/.vitepress/dist/.DS_Store back to docs/.vitepress/dist/ with .DS_Store on its own line. That is the fix I asked for on #130, landed here at the base of the stack, which is the right place for it. Checked in a worktree at this head:

.gitignore:10:docs/.vitepress/dist/   docs/.vitepress/dist/index.html
.gitignore:10:docs/.vitepress/dist/   docs/.vitepress/dist/.DS_Store

Both ignored again. No code changed, so everything measured in the review above still describes this head.

[SanderMuller]

Following up on my approval rather than reversing it — the analysis is right and the tests bite. This is about cost, and it is one line of config rather than a change to the work.

The number

The Benchmark comment on this branch, at the current head:

	base	head	Δ	noise
Full scan — 395 files	289 ms	315 ms	+8.7%	±1.9%
Full scan — 1,185 files	887 ms	960 ms	+8.2%	±1.0%

Well outside the floor, and the phase split says where it goes: lifecycle 209.1 → 284.6 ms (+36.1%, ±2.7%) and cmd_chains 10.5 → 20.1 ms (+91.6%, ±2.4%). Every other phase is flat or slightly down. That is TransactionScopes::in($ast), which scanMethod() builds for every method it scans.

What makes it worth raising

The generated corpus contains no transactions at all. I grepped benchmark/generate-corpus.php: no DB::transaction, no beginTransaction, no commit, no rollBack. It emits controllers, models, services, repositories, jobs, events, listeners, observers, middleware, Livewire components, commands and routes, and nothing that opens a span.

So +8.2% is not what the feature costs to do its job. It is what an application pays to be told it has none — a full extra traversal of every method body, on every scan, and again on every watch-mode poll, to discover there is nothing to mark. An application that does use transactions pays that plus the real work.

And it is the only expensive feature on the board with no switch. Comparing the open branches:

	switch
#130 chains and batches	laravel-brain.job_groups.enabled
#131 reachability	laravel-brain.reachability.enabled
#126 outgoing HTTP	laravel-brain.outgoing_http.enabled
#125 AI agents	laravel-brain.ai.enabled
#129 action classes	laravel-brain.actions.enabled
this branch	none

That is not a criticism of the design — it is the one place the pattern the rest of your branches follow was not applied.

Why it matters more than eight percent sounds

Every open branch measures against the same merge base c2bf14b, so these deltas compound at merge and no single comment shows the total. Adding the ones that clear their noise floors, and not double-counting #130 (which is stacked on this one, so its +11.2% already contains this +8.2%):

• the features that ship on and that the corpus never exercises: +23.5%
• with Feat: show what exists, and what nothing reaches #131's reachability pass as well: +89.9% — a 980 ms large scan becoming roughly 1,861 ms

That is an estimate assuming the passes are independent; the shared parse cache means they overlap, so the real figure needs composer benchmark on the merged tree. Either way this branch is a meaningful slice of it, and it is the slice nobody can turn off.

The ask

A laravel-brain.transactions.enabled flag next to the others, gating the construction of TransactionScopes the way job_groups.enabled gates JobGroups::at() on #130 — off before the detector runs, not around the result it would have produced. Whether it defaults on or off is the maintainer's call, not mine.

If the answer is "8% is worth it unconditionally", that is a legitimate answer and I would just ask for the number and the phase split to appear in the description, so the next person reading it knows what the tab costs.

[webard]

Done in fc74154 — laravel-brain.transactions.enabled, gating the construction of TransactionScopes rather than its result. Off means TransactionScopes::none() and no traversal at all, the way job_groups.enabled works on #130.

You are right that this was the one place the pattern the rest of the branches follow was not applied, and the framing is the part I want to acknowledge: +8.2% on a corpus containing no DB::transaction is the price of being told there is nothing to draw. That is a fair way to put it and it is now in the config comment, along with the phase split, so the next person deciding sees what the tab costs before they pay for it.

Default is on, matching the other feature flags. A test pins that turning it off skips the detection without taking the chain with it — the hops are still traced, only the boundary is gone. Removing the gate turns it red.

Gate: pint clean, phpstan no errors, 421 passing.

Note for whoever merges: #130 is stacked on this branch and will need a rebase to pick this up, as it did for the span-identity fix.

[SanderMuller]

Re-reviewed at fc74154, against e657389. One commit, and it is the switch I asked for.

Off costs nothing, and I mean that literally rather than approximately. Three interleaved repetitions on one of the applications I measure with, base against this head with the flag on and with it off:

	wall (median of 3)	nodes	edges	nodes flagged inTransaction
merge base	1.46 s	2,889	5,218	0
head, transactions.enabled on	1.57 s	2,889	5,218	73
head, transactions.enabled off	1.47 s	2,889	5,218	0

The 0 → 73 → 0 column is the control: the switch really does move what is measured, so the timing either side of it means something.

And the off path is byte-identical to the merge base. I dumped the full graph of a real application on both and compared:

base  vs  head with transactions off    BYTE-IDENTICAL
base  vs  head with transactions on     73 nodes changed, and only data.inTransaction and data.transactionId

No node, edge or tab added or removed on either side. So the flag skips the traversal rather than discarding its result, exactly as the config block says, and turning it on adds the two fields and nothing else.

The suite is green on this head — 421 passed (1169 assertions), PHPStan no errors, Pint clean — and the new test covers the off path.

On the default

The benchmark comment at this head still reads +9.1% (±0.7%) on the large scan with lifecycle at +34.2%, because the default is still on. That is your call and I said so; what I asked for in that case was the number and the phase split written down where a reader will find them, and the config block now carries both, with the honest sentence that the corpus contains no transaction at all. That is better than putting it in the description, since the config is what someone reads when they wonder why a scan got slower.

One thing for whoever merges

This block lands at the same anchor as #132's, and both leave the same comment stranded. In each branch the Model Observers heading and its whole explanation now sit above the new section, and 'observers' => [ follows with nothing above it — every other section in that file reads heading, comment, array. git merge-tree on the two heads reports a conflict in config/laravel-brain.php, so whichever merges second has to resolve it by hand; worth putting the observers comment back next to its array while doing so.

Approving, and re-pointing the approval at this head.

[mrmarchone]

@webard conflicts. see also the other opened PRs.