Skip to content

docs: record that breaking changes are allowed on the dev lines - #22

Merged
lisachenko merged 4 commits into
mainfrom
docs/breaking-changes-policy
Aug 16, 2026
Merged

docs: record that breaking changes are allowed on the dev lines#22
lisachenko merged 4 commits into
mainfrom
docs/breaking-changes-policy

Conversation

@lisachenko

@lisachenko lisachenko commented Aug 15, 2026

Copy link
Copy Markdown
Owner

Adds a short section to AGENTS.md stating that backwards compatibility is not a constraint on this package or the shared-data extension — plus two fixes to a pre-existing flaky test that CI surfaced while this was in flight (see below; the title no longer covers the whole diff).

The policy note

Every follow-up issue I filed (#14#21) hedged about BC without being asked to — proposing adapters, interim "detect the problem" steps, and deprecation paths purely to avoid touching a published shape. #19 is the clearest example: it offered a panic-id mechanism to detect that the runtime was about to report the wrong stack trace, as a step toward the real fix, when the real fix was always available.

That hedging is the reasonable default when no policy is written down, and it will recur every time a new contributor or agent picks up one of these issues. LAYOUT_VERSION already hard-fails a mismatched reader, which is the only compatibility mechanism this family needs while it is being built.

Two carve-outs, stated with it

lisachenko/z-engine is different — it has consumers of its own, so changes there get the ordinary care, and a missing capability is still solved by a named public method upstream rather than a reach-through.

This licenses changing the shape, not the invariants. The Never-Serialize Rule, fork-only sharing, prefork ordering, the arData law, publication order, lock discipline, EOWNERDEAD handling and the preemption obligations are not API contracts — they are the conditions under which this code is correct at all. Without that sentence, "breaking changes are allowed" is exactly the kind of line someone later cites while removing a safety property, so it sits directly beneath the rules it must not be read as overriding.

Effect on the open issues

#21 was rewritten (its whole premise was "free before a tag, expensive after"), and #15, #16 and #19 each got a note that their preferred option is cheaper than written — in #19's case, that the interim option should be skipped entirely.

The two test fixes

testAPreemptedCoroutineThatParksIsSafeToDiscard failed twice on this branch, with two unrelated root causes, neither introduced here. Both are bugs on main.

824f39e — it called a private method. $silence->receive(); Channel::receive() is a private internal helper returning ?Delivery, and the public method is recv(). A fatal error whenever it executed — which was rarely, because the coroutine only reaches that line if the shutdown drain resumes it far enough. Latent on 8.4, surfaced on the 8.5 runner.

It could hide because the assertion beside it was weak: "it was drained far enough to park itself" read a flag the coroutine sets on the line before parking, so it only proved execution reached that line. Whether a park happened is only observable from the channel's wait queue, so it now checks pendingReceivers().

6770e07 — it asserted a race. the loop was preempted: no on 8.4: the test ran a fixed 1.5 M-iteration loop and then asserted a preemption had occurred. How long a fixed count takes is a property of the machine; on that runner the loop finished inside its first 10 ms slice, so the test was failing a build where preemption worked perfectly. It now runs in chunks and stops as soon as the preemption counter moves. A cap bounds it, and taking the cap path leaves the counter at zero and fails — so the check still discriminates rather than becoming tautological.

Both fixes were verified in both directions: green across repeated full-suite runs on 8.4 and 8.5 with their matching z-engine lines, and each new assertion demonstrably fails when the behaviour it claims is removed.

They live here rather than in a separate PR because the branch was red and leaving it red for tidiness is the worse trade. They are separate commits and read independently if you would prefer them split out.

Verification

Gate 8.4.19 8.5.9
.phpt suite (92 tests)
PHPStan level max
PER-CS2.0 (local)

Every follow-up issue filed against this package hedged about backwards
compatibility - proposing adapters, interim detection steps and
deprecation paths to avoid touching published shapes. None of that is
warranted: this package and the shared-data extension ride development
lines with no external consumers, and LAYOUT_VERSION already hard-fails a
mismatched reader, which is the only compatibility mechanism the family
needs while it is being built.

Stating it here because the hedging was the reasonable default in the
absence of a written policy, and it will recur otherwise. The correct
shape wins over the compatible one.

Two carve-outs are stated with it. z-engine has consumers of its own and
keeps the ordinary care. And this licenses changing the shape, not the
invariants: the Never-Serialize Rule, fork-only sharing, prefork ordering,
the arData law, publication order, lock discipline, EOWNERDEAD handling
and the preemption obligations are the conditions under which this code is
correct at all, not contracts that may be renegotiated for convenience.

Refs #21
…appened

testAPreemptedCoroutineThatParksIsSafeToDiscard called Channel::receive(),
which is a private internal helper returning ?Delivery; the public method
is recv(). The call is a fatal error whenever it executes.

It survived because it usually did not execute. The coroutine only reaches
that line if the shutdown drain resumes it far enough, which is timing
dependent, so the bug stayed latent on 8.4 and surfaced on 8.5 in CI as an
uncaught Error inside the fiber.

The reason it could hide at all is the second bug: "it was drained far
enough to park itself" was asserted from a flag the coroutine sets on the
line *before* parking, so it only ever proved execution reached that line.
A coroutine that never parked passed the assertion just as happily as one
that did. Whether a park happened is only observable from the channel's own
wait queue, so the test now checks pendingReceivers() once the run is over.

Verified both directions: green on 8.4 and 8.5 across repeated runs, and
removing the recv() call makes the new assertion report "no" and fail,
which the old one did not.

Copy link
Copy Markdown
Owner Author

CI went red on Tests (PHP 8.5), and it was not this docs change — it is a pre-existing bug on main that this run happened to expose. Fixed here in 824f39e rather than left for a separate PR, since leaving the branch red to stay tidy is the worse trade.

What failed

testAPreemptedCoroutineThatParksIsSafeToDiscard called $silence->receive(). Channel::receive() is a private internal helper returning ?Delivery; the public method is recv(). That call is a fatal error whenever it executes:

Fatal error: Uncaught Error: Call to private method Channel::receive() from global scope

Why it passed until now

Because it usually did not execute. The coroutine only reaches that line if the shutdown drain resumes it far enough, which is timing dependent — so the bug stayed latent on 8.4 (and on 8.5 locally, across every run this session) and surfaced on the 8.5 runner.

The reason it could hide at all

The second bug, and the more interesting one. it was drained far enough to park itself was asserted from $state->parked, a flag the coroutine sets on the line before it parks. That only ever proved execution reached that line — a coroutine that never parked passed exactly as happily as one that did, which is precisely how a fatal on the next statement went unnoticed.

Whether a park happened is only observable from the channel's own wait queue, so the assertion now checks pendingReceivers() once the run is over.

Verified both directions

  • Green on 8.4 (×3) and 8.5 (×3) with the matching z-engine line, plus 5 more 8.5 runs before the assertion change — no intermittency.
  • Commenting out the recv() call makes the new assertion report no and fail. The old assertion still reported yes.

Worth noting for #20: this is the second time a test in this suite asserted something adjacent to its claim rather than the claim itself (the first was the preemption seam in #13, where "the timer is armed" would have passed under broken wiring). Both were caught by deliberately breaking the thing under test. That technique is cheap and worth applying to the rest of the preemption tests.


Generated by Claude Code

testAPreemptedCoroutineThatParksIsSafeToDiscard ran a fixed 1.5M-iteration
loop and then asserted that at least one preemption had happened. How long
a fixed count takes is a property of the machine, not of the runtime: on a
CI runner the loop finished inside its first 10 ms slice, so the coroutine
was never preempted and the test failed on a build where preemption worked
correctly.

The loop now runs in chunks and stops as soon as the preemption counter
moves, so it is independent of machine speed. A cap bounds it, and taking
the cap path leaves the counter at zero and fails the assertion - so the
check still discriminates rather than becoming tautological: a build where
preemption never fires fails quickly instead of spinning.

Green across repeated runs on 8.4 and 8.5 with their matching z-engine
lines.

Copy link
Copy Markdown
Owner Author

A second, unrelated failure in the same test on the next run — this time on 8.4, and with a different root cause. Fixed in 6770e07.

-'the loop was preempted: yes\n
+'the loop was preempted: no\n

Zero preemptions. Not a broken runtime — the opposite: the test ran a fixed 1.5 M-iteration loop and then asserted that at least one preemption had happened. How long a fixed iteration count takes is a property of the machine, and on that runner the loop finished inside its first 10 ms slice, so there was nothing to preempt. The test was failing a build where preemption worked perfectly.

The loop now runs in chunks and stops as soon as the preemption counter moves, so it no longer depends on machine speed. A cap bounds it — and taking the cap path leaves the counter at zero and fails the assertion, so the check still discriminates rather than becoming tautological. A build where preemption genuinely never fires fails quickly instead of spinning.

Green across four consecutive full-suite runs on each minor with its matching z-engine line.

Both failures came from the same habit

Two different bugs in one test, and the same root cause behind both: asserting something adjacent to the claim instead of the claim.

  • it was drained far enough to park itself read a flag set on the line before parking — so it could not notice that the next statement was a fatal error.
  • the loop was preempted inferred preemption from an iteration count chosen on a developer's machine — so it reported a runtime failure when the machine was merely fast.

Neither is exotic; both are the comfortable version of the assertion. That is now three instances in this suite (the preemption seam in #13 was the first), which is enough of a pattern that I have noted it on #20 as a sweep worth doing across the preemption tests rather than fixing case by case.

This PR is nominally documentation, and now carries two test fixes. I kept them here rather than splitting because the branch was red and leaving it red for tidiness is the worse trade — but they are separate commits and are easy to read on their own.


Generated by Claude Code

Comment thread AGENTS.md Outdated
…only"

"Sharing is fork-only" described the mechanism this runtime happens to use
and presented it as a property of the design. It is not one. Memory can be
placed at the same virtual address in a process that was never forked from
this one - shm_open plus mmap(MAP_FIXED), userfaultfd and others - so the
claim foreclosed legitimate designs, and it was listed among the
invariants, which made it look non-negotiable.

What is actually required is narrower and worth saying precisely: every
participant must see the arena at the same virtual address, and must agree
on the engine pointers baked into shared structs - class entries and
std_object_handlers. Under the Never-Serialize Rule an address is the
value, so a shared object only means the same thing in another process when
both hold. Fork gets both for free, which is why the implementation uses
it, but that is the mechanism rather than the rule.

The prefork ordering is unaffected and still load-bearing: a fork copies
only what already exists, so shared state has to be built before it.

Refs #22
@lisachenko
lisachenko marked this pull request as ready for review August 16, 2026 00:15
@lisachenko
lisachenko merged commit f7cb036 into main Aug 16, 2026
4 checks passed
@lisachenko
lisachenko deleted the docs/breaking-changes-policy branch August 16, 2026 00:16
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.

1 participant