Repository navigation
FORM: form_source serves another data cell's products for single-layer files written out of order #992
Description
storage_reader::get_index maps a single-layer index such as [event:N] straight to row N (https://github.com/Framework-R-D/phlex/blob/fb7fb00/form/storage/storage_reader.cpp#L201-L203, via sequential_row_from_index_id, https://github.com/Framework-R-D/phlex/blob/fb7fb00/form/storage/storage_reader.cpp#L160-L187). It only looks at the index column when the id has more than one component.
The write side doesn't put rows in cell order. Each product goes into the next row in the order stores reach form_module (GetNEntries() at https://github.com/Framework-R-D/phlex/blob/fb7fb00/form/root_storage/root_rfield_write_container.cpp#L82). Under -j N, that is the order in which upstream nodes finish. The index column and the navigation table record the right cell for every row, but the shortcut never consults either.
So a file whose cells sit directly under job reads back wrong whenever it was written by more than one thread. form_source serves row N's products for [event:N], whichever cell row N belongs to. The job exits 0, and if it writes what it read, the new file stores the wrong data under the right label.
This is the "sequential fast path" from #713. #715 fixed its off-by-one, but the fast path is only correct when rows are written in cell order.
Observed (phlex 0.4.3 from conda-forge, ROOT 6.40.04, RNTuple)
A provider writes std::vector<int>{id.number()} for each event cell. A pass-through transform sleeps between 0 and 2 ms at random so that stores reach the output out of order. A second job reads the file with form_source and compares each served value with the cell it was served for.
| Write | Hierarchy | Cells | Read back correctly |
|---|---|---|---|
-j 1 |
job → event | 200 | 200 |
-j 8 |
job → event | 200 | 19 |
-j 8 |
job → run (4) → event | 200 | 200 |
-j 8 |
job → run → event | 20 000 | 20 000 |
-j 1, starting_number: 1000 |
job → event | 50 | throws |
MISMATCH[stamper/stamped] cell [event:0] served value 2
MISMATCH[stamper/stamped] cell [event:4] served value 0
…
CHECK[stamper/stamped] cells=200 match=19 mismatch=181
The last row is the same shortcut. Cells numbered from 1000 map to rows 1000 to 1049 of a 50-row container:
FORM Error: Failed to retrieve product [stamped] for [event:1000]
Multi-layer ids such as [run:1, event:3] take the index-map path and are always correct, which is why the two-layer cases pass.
test/form/form_gaussian_read_test.jsonnet reads back random values without checking which cell they belong to, so CI can't see this.
Suggested fix
Drop the shortcut and always resolve through the index map, which is already built once per container and cached. If the fast path is worth keeping for files that are in order, it could check that row N's index value equals the requested id and fall back to the map otherwise. A read-back test that writes each cell's own number at -j > 1 and checks it per cell would cover both. I haven't built phlex with either change yet; happy to open a PR with the test.
Reproducer
Plugins, configuration and commands
cell_provider.cpp, a provider plus a transform that reorders stores:
#include "phlex/core/product_selector.hpp"
#include "phlex/model/data_cell_index.hpp"
#include "phlex/model/identifier.hpp"
#include "phlex/module.hpp"
#include "phlex/source.hpp"
#include <chrono>
#include <random>
#include <thread>
#include <vector>
using namespace phlex;
PHLEX_REGISTER_PROVIDERS(graph, config)
{
graph
.provide("numbers",
[](data_cell_index const& id) { return std::vector<int>{static_cast<int>(id.number())}; },
concurrency::unlimited)
.output_product("numbers", experimental::identifier("numbers"), experimental::identifier("event"));
graph.provide("ids", [](data_cell_index const& id) { return id; }, concurrency::unlimited)
.output_product("ids", experimental::identifier("id"), experimental::identifier("event"));
}
PHLEX_REGISTER_ALGORITHMS(graph, config)
{
graph
.transform(
"stamp",
[](std::vector<int> const& in) {
thread_local std::mt19937 gen{std::random_device{}()};
std::this_thread::sleep_for(
std::chrono::microseconds(std::uniform_int_distribution<int>(0, 2000)(gen)));
return in;
},
concurrency::unlimited)
.input_family(product_selector{.creator = "numbers", .layer = "event"})
.output_product_suffixes("stamped");
}cell_check.cpp, an observer that compares the served value with its cell:
#include "phlex/core/product_selector.hpp"
#include "phlex/model/data_cell_index.hpp"
#include "phlex/module.hpp"
#include <atomic>
#include <cstdio>
#include <vector>
using namespace phlex;
namespace {
struct checker {
std::atomic<std::size_t> total{0}, bad{0};
~checker() { std::printf("CHECK cells=%zu mismatch=%zu\n", total.load(), bad.load()); }
void check(std::vector<int> const& served, data_cell_index const& id)
{
++total;
if (served.empty() || served.front() != static_cast<int>(id.number())) {
++bad;
}
}
};
}
PHLEX_REGISTER_ALGORITHMS(m, config)
{
m.make<checker>()
.observe("check", &checker::check, concurrency::unlimited)
.input_family(product_selector{.creator = "stamper", .layer = "event", .suffix = "stamped"},
product_selector{.creator = "ids", .layer = "event", .suffix = "id"});
}write.jsonnet:
{
driver: { cpp: 'generate_layers', layers: { event: { parent: 'job', total: 200 } } },
sources: { numbers: { cpp: 'cell_provider' } },
modules: {
stamper: { cpp: 'cell_provider' },
output: { cpp: 'form_module', output_file: 'w.root', technology: 'ROOT_RNTUPLE',
products: ['stamped'] },
},
}read.jsonnet:
{
driver: { cpp: 'generate_layers', layers: { event: { parent: 'job', total: 200 } } },
sources: {
ids: { cpp: 'cell_provider' },
form_in: { cpp: 'form_source', stage: 'write', input_file: 'w.root',
technology: 'ROOT_RNTUPLE', plugin: 'stamper', algorithm: 'stamp',
creator: 'stamper', products: ['stamped'] },
},
modules: { check: { cpp: 'cell_check' } },
}phlex -c write.jsonnet -j 8 --stage write
phlex -c read.jsonnet -j 8 --stage read # CHECK cells=200 mismatch=181 (varies)
phlex -c write.jsonnet -j 1 --stage write
phlex -c read.jsonnet -j 8 --stage read # CHECK cells=200 mismatch=0
Activity
wwuoneway commentedon Oct 21, 2026
@olantwin Thanks for identifying this issue and for the detailed analysis! I agree with your proposed approach. I've been working on a fix for this issue.
FORM: fixed issues when reading single-layer is such as [event:N], re…
olantwin commentedon Oct 22, 2026
Thanks a lot for fixing this!