fix(pine-cpp): make_window_view shares parent mu_ to close FlatMap race (#103 #109 #131) - #135
Merged
Merged
Conversation
…ed_mutex> Pure type migration in preparation for #103 fix. Each RowFrame instance still owns its own freshly-allocated shared_mutex via make_shared in both ctors — behavior is unchanged at this commit. The next commit migrates ColumnFrame the same way; the third commit then has make_window_view copy parent's shared_ptr so view and parent alias the same mutex (the actual race fix). Why split it like this: doing the type migration and the semantic change in one commit makes a footgun where reverting the fix also reverts the type and breaks builds. With this split, each commit is independently revertible. Lock acquire sites: all `lk(mu_)` → `lk(*mu_)` (15 sites, mechanical). Validation: cpp test suite passes (211/211 cases, 110k+ assertions). No behavior change expected here. Refs #103.
…hared_mutex> Mirror commit for the column-storage Frame. Same shape as the previous commit's RowFrame migration: type changes from `shared_mutex` to `shared_ptr<shared_mutex>`, both ctors initialize via `make_shared`, all 15 lock-acquire sites flip from `lk(mu_)` to `lk(*mu_)`. No behavior change at this commit — each ColumnFrame still owns its own mutex. The next commit changes make_window_view (Row + Column together) so the shared_ptr is copied from parent into view, restoring the single-mutex-per-storage invariant that #103 / #109 / #131 require. Validation: cpp test suite passes (211/211 cases, 110k+ assertions). Refs #103.
…ce (#103 #109 #131) Core fix. Both `RowFrame::make_window_view` and the static `ColumnFrame::make_window_view(parent, ...)` now copy `parent.mu_` (a shared_ptr<shared_mutex> after the previous two commits) into `v->mu_`. Parent and view alias the same mutex; storage-level locking finally spans both. What this closes: - Writer T7 in #131's TSan report holds parent's mu_ in `RowFrame::apply_output → FlatMap::operator[]` (vector emplace + string move into the freshly grown slot at 0x725400010020). - Reader T21 in the same report holds view's mu_ (a different mutex pre-fix) in `build_operator_input → with_read_lock → item_has_no_lock → FlatMap::find → lower_bound` (string compare reading the same 0x725400010020). - Two distinct mutexes guarding the same memory ⇒ TSan WARNING. - Post-fix: the same shared_mutex serializes them. Concurrent shard reads still parallelize (shared_lock); apply_output across ops blocks on unique_lock until all live views release. The next commit's deterministic reproducer + #131 fixture lockdown verify this empirically. Refresh of obsolete column_frame.hpp comment: The pre-#103 doc claimed "parent is read-only during the view's lifetime; parallel_execute satisfies this". That was true at the time it was written, but the v0.9.2 ready-queue scheduler refactor (`llmdoc/memory/reflections/dag-ready-queue-scheduler.md`) introduced no-field-conflict cross-op parallelism — which assumes field-level DAG edges are sufficient to serialize storage access, but window views had been quietly aliasing storage under separate mutexes the whole time. Comment replaced with the post-#103 reality: storage layer enforces single-mutex serialization, scheduler can keep its field-level parallelism. Performance trade-off: True-parallel cross-op pairs whose storage actually overlapped (predecessor's data_parallel shards still live, successor running concurrently with no field conflict) now serialize via shared_lock contention instead of running unsynchronized. Calibrated benchmark will quantify the impact in the PR body. If > 5 % regression, escalate to Option C from #109's evaluation comment (SharedMutex revival). Validation: cpp test suite passes (211/211 cases, 110k+ assertions). The next commit adds the deterministic race reproducer that needs the fix to pass. Closes #103 (subject to PR-level merge). Refs #109, #131.
…mutex (#103) Adds tests/test_window_view_race.cpp with two doctest cases (RowFrame and ColumnFrame). Each spawns a writer-thread tight-looping parent.apply_output and a reader-thread tight-looping view.item_has(...). The threads run for 500ms; the doctest assertions only verify both made progress. The actual diagnostic comes from running the binary under -fsanitize=thread (the cpp-tsan CI job already does this for the whole test suite). Empirical verification: - Pre-fix (commit 3 reverted): TSan reports `data race in pine::FlatMap::operator[]` and the underlying `vector::_M_realloc_insert` within the first iteration. Two distinct mutexes (parent.mu_, view.mu_) guard the same FlatMap memory — exactly the race #131 captured in production. - Post-fix (commit 3 applied): TSan reports zero races across multiple runs. The shared_ptr<shared_mutex> aliasing causes shared_lock and unique_lock to actually exclude each other. Why not also add #131's unstable_008189 case as a fixture: the case reproduces non-determinism, not a deterministic divergence. Once #103 is fixed, the fuzz round produces stable output, so there's nothing for a fixtures/errors-style negative test to lock. The synthetic race reproducer is the better regression signal — it directly exercises the storage-level lock contract. Validation: cpp test suite passes (211→213 cases, all 4 new test-case expansions clean under both Release and TSan builds). Refs #103, #109, #131.
Contributor
🔍 PR 审查
核心修复正确：
|
…135 review) Address bot-reviewer concern on the previous PR commits: switching mu_ from `std::shared_mutex` to `shared_ptr<std::shared_mutex>` removed the implicit non-copyable/non-movable guarantee that the inline shared_mutex member used to provide. Every other RowFrame member is copyable, so post-migration RowFrame became implicitly copy/move-constructible. A value copy would have aliased the mutex (shared_ptr copy) while deep-copying items_ — two "independent" frames sharing one lock for unrelated data, exactly the kind of subtle hazard #103 was about preventing. ColumnFrame stayed non-copyable through its `unique_ptr<ColumnStore>`, producing the asymmetry the reviewer flagged. Fix: explicitly delete copy/move ctors and assignments on both RowFrame and ColumnFrame. RowFrame restores the lost invariant; ColumnFrame pins the implicit one explicitly so the contract is self-describing rather than depending on a future reader noticing the unique_ptr member. No call site copies or moves either Frame in the current tree; this commit is purely about preventing future misuse. Test suite still passes (213/213, 110k+ assertions) — no value-copy paths existed to break. Refs #103, #135.
Contributor
🔍 PR 增量审查
增量仅新增了 验证结论：删除拷贝/移动不会破坏现有代码。
代码良好，无阻塞或重要问题，已检查 bug 与代码规范。 |
Carries the #103 / #109 / #131 storage-mutex fix: da66bcd refactor(pine-cpp): RowFrame mu_ shared_ptr<shared_mutex> 0dea37e refactor(pine-cpp): ColumnFrame mu_ shared_ptr<shared_mutex> 9ed59b8 fix(pine-cpp): make_window_view shares parent mu_ (#103 #109 #131) bb6c981 test(pine-cpp): deterministic race reproducer for window-view 45fcabe fix(pine-cpp): explicitly delete Row/Column Frame copy & move This is a pine-cpp-only correctness fix — no API surface change, no JSON contract change, no Go/Java behavioral impact. Per-engine audit on #135 confirmed pine-go and pine-java do not have an equivalent hazard (their data_parallel uses single-Frame OperatorInput projection; only pine-cpp's multi-Frame view model needs the shared-mutex aliasing). bump-version.sh ran the full four-runtime test sweep + 12 cross-validate sections inline before this commit. All PASS. Next: merge #135 → master, then `make tag-release` to publish v0.10.9 (release.yml workflow_call gate per #126 will run full CI on the tag SHA before pypi-apple / java jobs publish).
Contributor
🔍 PR 增量审查
增量仅为版本号 bump 验证结论：版本号跨引擎同步一致，符合
|
Liam0205
added a commit
that referenced
this pull request
Jun 21, 2026
…135 review) Address bot-reviewer concern on the previous PR commits: switching mu_ from `std::shared_mutex` to `shared_ptr<std::shared_mutex>` removed the implicit non-copyable/non-movable guarantee that the inline shared_mutex member used to provide. Every other RowFrame member is copyable, so post-migration RowFrame became implicitly copy/move-constructible. A value copy would have aliased the mutex (shared_ptr copy) while deep-copying items_ — two "independent" frames sharing one lock for unrelated data, exactly the kind of subtle hazard #103 was about preventing. ColumnFrame stayed non-copyable through its `unique_ptr<ColumnStore>`, producing the asymmetry the reviewer flagged. Fix: explicitly delete copy/move ctors and assignments on both RowFrame and ColumnFrame. RowFrame restores the lost invariant; ColumnFrame pins the implicit one explicitly so the contract is self-describing rather than depending on a future reader noticing the unique_ptr member. No call site copies or moves either Frame in the current tree; this commit is purely about preventing future misuse. Test suite still passes (213/213, 110k+ assertions) — no value-copy paths existed to break. Refs #103, #135.
This was referenced Jun 21, 2026
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.
Closes #103. Closes #109. Closes #131.
Root cause
#131 (2026-06-19) finally produced a TSan stack trace for the race that #109 / #124 had been failing to reproduce locally for two weeks. #103 (2026-06-14) had already predicted this exact hazard from architecture review:
RowFrame::make_window_viewcreates a new RowFrame instance whoseview_items_aliases parent'sitems_std::shared_mutex mu_unstable_008189: writer (T7) holds parent.mu_ inRowFrame::apply_output → FlatMap::operator[], reader (T21) holds view.mu_ inbuild_operator_input → with_read_lock → item_has_no_lock → FlatMap::find. Different mutexes, same FlatMap memory at0x725400010020→ race.The pre-#103
column_frame.hppcontract comment claimed "parent is read-only during the view's lifetime; parallel_execute satisfies this". That was true when written, but the v0.9.2 ready-queue scheduler refactor (llmdoc/memory/reflections/dag-ready-queue-scheduler.md) introduced no-field-conflict cross-op parallelism — which assumed field-level DAG edges sufficed to serialize storage access, but window views had been quietly aliasing storage under separate mutexes the whole time.Fix: Option A from #109's evaluation
Per #109 evaluation comment and #103 implementation plan comment, Option A: parent and view share the same shared_mutex.
RowFrameandColumnFrameprivate member changes:make_window_view(both Frame impls):+ v->mu_ = mu_; // shared_ptr copy: parent and view alias the same mutexAll lock acquire sites:
lk(mu_)→lk(*mu_)(30 sites total across the two Frame impls, mechanical).4-commit layout
da66bcde*mu_. No behavior change at this commit0dea37ec9ed59b8fv->mu_ = parent.mu_in bothmake_window_viewimpls; refreshcolumn_frame.hppcontract commentbb6c981atests/test_window_view_race.cppwith deterministic Row/Column race reproducers; tight-loop writer (apply_output on parent) vs reader (item_has on view) for 500ms eachEach commit independently builds and passes the cpp test suite. The split lets a future bisect localize regressions to either the type migration or the semantic change without ambiguity.
Empirical verification
The reproducer test was first run pre-fix (commit 3 reverted), produced multiple TSan WARNINGs in
FlatMap::operator[]andvector::_M_realloc_insert(matching #131's stack exactly). Then commit 3 was un-reverted and the same test produced zero TSan reports across multiple Release + TSan invocations. The race is empirically gone.Validation matrix:
scripts/cross-validate.sh(all 12 sections)Cross-engine implementation note
The three engines take different shapes for data_parallel — and that's fine, parity is at the behavior layer, not the implementation layer:
C++ chose the multi-Frame view model because per-instance mutex is the natural C++ idiom; Go/Java single-Frame is the natural managed-runtime idiom. The hazard was specific to C++'s choice — once we share the mutex, the per-instance abstraction stays clean while storage-level locking is correct.
Performance trade-off
True-parallel cross-op pairs whose storage actually overlapped (predecessor's data_parallel shards still live + successor running concurrently with no field-level conflict) now serialize via shared_lock contention instead of running unsynchronized.
Magnitude prediction: < 3 % calibrated regression based on the small fraction of cross-op concurrency in calibrated's 38-op pipeline (most ops chain through fields, DAG edges already serialize). Per #129's plan, daily-sanitized-fuzz post-merge runs will validate.
If > 5 % regression appears: escalate to Option C (SharedMutex revival) per #109's evaluation. Not doing that speculatively.
Test plan
Out of scope
cc: #109, #131