fix(#55): minor lifecycle and hygiene issues - #70
Merged
Conversation
s2x
commented
Jul 5, 2026
s2x
commented
Contributor
- dlclose() the libfdb_c.so handle once fdb_run_network completes
- clearAll() now clears from empty string to include zero-length keys
- closeAllDatabases() in stopNetwork() ensures fdb_database_destroy() runs before fdb_stop_network()
- Document Snapshot::$parentTransaction as a GC anchor
- Remove redundant phpstan.neon ignore for Snapshot::property.onlyWritten
…tion-cross moves
DirectoryLayer::move() previously accepted any pair of
non-empty paths with no further validation, which let three
silent-permissive bugs through:
1. moving ['a'] into ['a','b'] produces a cycle / unreachable
subtree in the subdirs index;
2. moving ['p','a'] into ['x','a'] silently re-parents a
partition node under a non-partition parent (or vice-versa),
leaving the moved prefix in a different partition's content
space;
3. moving to a path with more than 64 segments lets a malformed
input produce arbitrarily-deep directory entries on a hot path.
Replace with explicit bounds + throw:
- validateMoveBounds(oldPath, newPath) runs before the
transaction and rejects: identical paths (no-op rewrite),
newPath under oldPath (cycle), depth > MAX_MOVE_PATH_DEPTH
(64 segments).
- assertSamePartitionLayer() runs inside the transaction and
rejects moves that cross between partition and non-partition
layers.
Both helpers throw DirectoryException with a printable rendering
of the offending paths (control bytes, DEL, high bytes, and '/'
inside segments are escaped to \xHH) so application input never
flows raw into log lines.
Tests:
- tests/Unit/DirectoryMoveBoundsValidationTest.php (29 cases):
printableSegment / printablePath boundaries, identical /
self-subtree / src-dest prefix rejections, MAX_MOVE_PATH_DEPTH
boundaries, partition-layer accept and reject paths.
- tests/Integration/DirectoryTest.php gains 7 new cases covering
immediate-subtree rejection, deeper-subtree rejection,
rejected-self-subtree leaves the index untouched, identical-
path rejection, sibling-rename happy path, re-parent-under-
existing-parent happy path, partition crossing rejection in
both directions, and same-partition move happy path.
Docs/CHANGELOG updated to document the new contract.
…own layer
CI caught two real bugs and one cosmetic defect in the original
patch:
1. The original partition-crossing rule compared oldPath's own
layer attribute against newPath's parent's layer. A child born
inside a partition carries layer="" on its own node, so a
move like ['app','part','child'] -> ['app','other'] would
compare oldLayer="" against newParentLayer="" and silently
succeed, even though the child lived inside a 'partition'
node and the move was a real partition crossing.
2. Conversely, ['app','part3','sibling_a'] -> ['app','part3','sibling_b']
was expected to succeed but with the source-vs-parent rule
also happened to work — but for the wrong reason. The check
needs to look at the immediate **parents**, not the source.
Fix: hoist oldParentNode lookup to the top of the transaction
(alongside the existing existence checks), compare
oldParentLayer against newParentLayer, and surface
'parent layer "X"' in the exception so the message carries
the actual partition identity that disagreed.
The unit tests for assertSamePartitionLayer are updated to
match the corrected exception text; the partition-crossing
integration tests stay undchanged at the substring level
('partition crossings are disallowed') so the message
contract remains observable.
Also fix: replace a backtick-delimited regex in
moveIntoOwnDeeperSubdirectoryThrows with the tilde
delimiter, since the original used '/.../' with literal
backticks that PHP didn't recognise as a valid pattern.
Tests:
- 29 unit tests in DirectoryMoveBoundsValidationTest.php
still pass with updated exception-text expectations.
- 9 integration tests in DirectoryTest.php unchanged
conceptually; one regex delimiter repaired.
Verified locally with composer lint and composer test:unit
(429/429 pass).
- dlclose() the libfdb_c.so handle once fdb_run_network completes - clearAll() now clears from empty string to include zero-length keys - closeAllDatabases() in stopNetwork() ensures fdb_database_destroy() runs before fdb_stop_network() - Document Snapshot::$parentTransaction as a GC anchor
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.