Conversation
One reading on a channel the descriptor catalog does not describe refused the whole commit and left no database file at all. The described readings in that batch died with the undescribed one, on every acquisition cycle, for as long as the catalog gap lasted. A re-wired sensor, or one added mid-campaign, produces exactly that gap in an ordinary laboratory week, so this is direct data loss on the acquisition path. The write path now admits such a reading against a reserved catalog entry whose whole meaning is that the channel is NOT described. Nothing is inferred from the label, the unit, or anything else about the reading: the reserved entry is a fixed identity that grants nothing. The row keeps the label the instrument emitted, because with no canonical identity that label is the only remaining truth about where the value came from. Two cases, and only one of them changes. A channel with no binding at all has no identity to contradict, so it is admitted against the reserved entry. A channel the catalog DOES describe under another instrument still refuses -- that reading may not be the quantity the descriptor names, and storing it would put a wrong number under a real identity. `bind` itself is untouched and keeps its fail-closed behaviour for every direct caller; only the write path's `admit` differs, and the difference is the cost of a refusal. Reads are bounded. A flood of distinct undescribed labels would turn one query into thousands of rows, which is lag on a week-long run, so discovery materialises at most a fixed number of unbound labels plus an overflow marker, while every further unbound presence is still marked rather than silently dropped. `test_unknown_raw_label_fails_closed_not_legacy_synthesis` is renamed to `test_unknown_raw_label_is_stored_without_canonical_identity` and its expectation reversed deliberately. Its stated purpose -- that duck typing, or matching on a unit or a name, can never grant canonical identity to an unknown channel -- is preserved and asserted directly: the stored row references the reserved entry's hash and not the real descriptor's. What changed is the cost of the refusal, not the rule about identity.
|
@codex review Head The change makes one undescribed channel stop destroying the whole commit batch. Four things I
Control I ran myself at this head: restoring the old strict behaviour in place turned 8 tests red |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4acb36cac9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| descriptor = _reserved_entry_of(self._catalog) | ||
| envelope = PersistedChannelEnvelopeV1.from_descriptor(descriptor) | ||
| selected = decode_persisted_channel_envelope(envelope.canonical_json).descriptor |
There was a problem hiding this comment.
When admit handles an undescribed live label, it keeps the emitted label in the Reading but binds the reserved descriptor here; CommittedReadingReceipt._issue then serializes that descriptor and core/scheduler.py publishes the envelope alongside the differently named reading. qualify_reading_descriptor requires the envelope channel/instrument/unit to equal the reading, so every newly tolerated channel is reported as IDENTITY_MISMATCH and increments the GUI's malformed-descriptor count rather than appearing with an absent descriptor. The reserved entry needs to remain persistence provenance and produce a None live envelope, consistent with the read path.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| if _carries_reserved_entry(snapshot): | ||
| return snapshot | ||
| return snapshot_catalog(ChannelCatalog([*snapshot.descriptors, unbound_channel_descriptor()])) |
There was a problem hiding this comment.
For the supported plain-ChannelCatalog writer path, a caller can supply a real descriptor whose channel_id is cryodaq.unbound but whose other fields differ; this branch accepts it unchanged, while _unbound_descriptor_hash is still set to the canonical reserved hash. The next undescribed reading is therefore assigned a hash absent from the installed catalog and fails the foreign key, destroying the batch—the exact regression this change intends to prevent. The live-catalog constructor already rejects this namespace collision, so this adapter must perform the same exact-descriptor check.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| except ChannelDescriptorStorageError as refusal: | ||
| if "unavailable in the explicit descriptor catalog bindings" not in str(refusal): | ||
| raise |
There was a problem hiding this comment.
The new behavior depends on the exact English text of a base ChannelDescriptorStorageError: rewording bind makes a genuinely absent channel raise again and discard the batch, while any different refusal that later includes this phrase would be admitted. This patch already defines ChannelNotDescribedError specifically to distinguish absence from disagreement, so bind should raise that type and admit should catch only it rather than treating an operator-facing message as an error code.
Useful? React with 👍 / 👎.
| if len(unbound_channels) <= _MAX_MATERIALIZED_UNBOUND_CHANNELS: | ||
| unbound_channels.add(channel) |
There was a problem hiding this comment.
With more than 32 distinct unbound labels, both this SQLite discovery path and its mirrored Parquet path silently stop retaining labels, but the omission is never propagated to the collector. The result consequently reports truncated=False and rows_dropped_by_caps=0 even though stored rows were excluded; complete=False cannot communicate this because even one fully materialized unbound row sets it solely for missing identity. Consumers such as periodic projection use rows_dropped_by_caps for loss accounting, so discovery overflow needs to set the truncation/drop metadata explicitly.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| owned = _own_live_reading(reading) | ||
| descriptor = _reserved_entry_of(self._catalog) |
There was a problem hiding this comment.
With an alias binding such as ('inst', 'probe') -> 'stage_temp', a second reading emitted as stage_temp misses the binding and is admitted here as unbound even though that spelling is already the real canonical ID. If both readings occur in one poll, persistence stores both as (timestamp, 'inst', 'stage_temp') with different descriptor hashes, while _BoundedReadingCollector keys only on timestamp/instrument/channel and collapses them; in the production ordering the reserved row can replace the described measurement. This ambiguous label must be refused rather than sent to the reserved entry.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| admitted_is_unbound = is_reserved_descriptor(item.descriptor) | ||
| if admitted_is_unbound: | ||
| stable = reading |
There was a problem hiding this comment.
The live admission grammar permits instrument, channel, and unit text up to 1024 UTF-8 bytes, and this branch now persists those fields verbatim for unbound readings, but ArchiveReader accepts only 256-byte instruments/channels and 64-byte units. An undescribed channel longer than 256 bytes therefore commits successfully, then discovery raises INVALID_ROW and quarantines the entire SQLite day, including valid described readings; the same mismatch affects cold reads. Validate or normalize the preserved fields against the durable reader grammar before admitting them.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| if not _carries_reserved_entry(self._catalog): | ||
| self._catalog = snapshot_catalog(ChannelCatalog([*self._catalog.descriptors, unbound_channel_descriptor()])) |
There was a problem hiding this comment.
A ChannelCatalog containing the public maximum of 4096 valid descriptors is accepted by the descriptor contract, but this unconditional append constructs a 4097-entry catalog and raises ChannelDescriptorError; catalog_with_reserved_descriptor causes the same failure for the plain writer path. Thus the internal persistence marker silently reduces the usable configured roster to 4095 channels. Reserve capacity outside the caller-visible limit or adjust validation so every catalog accepted at MAX_CATALOG_DESCRIPTORS remains usable.
Useful? React with 👍 / 👎.
| # both kinds in one file, and an all-unbound day rotates with no descriptor | ||
| # sidecar at all. This row carries its own provenance instead. | ||
| assert self._unbound_descriptor_hash is not None | ||
| return self._unbound_descriptor_hash |
There was a problem hiding this comment.
Every unbound row receives the same reserved descriptor hash here, but ArchiveReader.query_rows uses row[6] or row[2] as its deduplication identity. Two different undescribed labels from one instrument at the same poll timestamp therefore produce two durable SQLite rows but the CSV/XLSX/HDF5 read boundary returns only one, because both keys become (timestamp, instrument, UNBOUND_DESCRIPTOR_HASH). The export path must use the emitted channel for reserved references so distinct committed readings survive.
AGENTS.md reference: AGENTS.md:L468-L469
Useful? React with 👍 / 👎.
| # These rows are on disk now. The retry loop reaches this point once, | ||
| # so a retried transaction cannot count the same rows twice. | ||
| self._record_unbound_channels(unbound) |
There was a problem hiding this comment.
For an unbound batch this new call runs after conn.commit() but still inside the transaction's broad try; if an installed logging handler raises while _record_unbound_channels emits its warning, execution enters the exception handler, performs an ineffective rollback, and reports commit failure even though the rows are already durable. The descriptor-authoritative caller then receives no receipt and refuses publication for committed data. Post-commit observational logging must be contained so it cannot invalidate persistence evidence.
AGENTS.md reference: AGENTS.md:L20-L22
Useful? React with 👍 / 👎.
|
Closing as a duplicate of #104, which is the same fix and already existed. I opened this without checking the open queue first. #104 now carries exactly this content: master The branch |
1 participant
The defect
A reading on a channel the descriptor catalog does not describe refused the whole commit and left
no database file at all. Every described reading in that batch died with the undescribed one,
on every acquisition cycle, for as long as the catalog gap lasted.
A re-wired sensor, or one added mid-campaign, produces exactly that gap in an ordinary laboratory
week. This is direct data loss on the acquisition path — the first of the four failure modes
that gate the week.
The change
The write path now admits such a reading against a reserved catalog entry whose whole meaning
is that the channel is not described. Nothing is inferred from the label, the unit, or anything
else about the reading: the reserved entry is a fixed identity that grants nothing. The row keeps
the label the instrument emitted, because with no canonical identity that label is the only
remaining truth about where the value came from.
Two cases, and only one of them changes. A channel with no binding at all has no identity to
contradict, so it is admitted against the reserved entry. A channel the catalog does describe
under another instrument still refuses — that reading may not be the quantity the descriptor
names, and storing it would put a wrong number under a real identity.
binditself is untouchedand keeps its fail-closed behaviour for every direct caller; only the write path's
admitdiffers.
Reads stay bounded. A flood of distinct undescribed labels would turn one query into thousands
of rows, which is lag on a week-long run. Discovery materialises at most a fixed number of unbound
labels plus an overflow marker, and every further unbound presence is still marked rather than
silently dropped.
One test was renamed and its expectation reversed — deliberately
test_unknown_raw_label_fails_closed_not_legacy_synthesisis nowtest_unknown_raw_label_is_stored_without_canonical_identity.Its stated purpose — that duck typing, or matching on a unit or a name, can never grant canonical
identity to an unknown channel — is preserved and asserted directly: the stored row references
the reserved entry's descriptor hash and explicitly not the real descriptor's, and the emitted
label survives. What changed is the cost of the refusal, not the rule about identity. The file's
test count is unchanged at 6.
Reversion control
Restoring the old strict behaviour in place — making
admitcallbinddirectly, so the modulestill imported — turned 8 tests red at pytest exit 1 with the real production error:
Restoring gave a byte-identical file, blob
d1862e9cb0b706941b63fe8cc018fab78ad34dfb. Green side:23 passed.
Evidence
Measured on Windows against this branch, each partition through
python -m tools.ci_candidate_runner, both invocations, with full population receipts:coreagentsremainingtests/docs/test_docs_freshness.pyruff check/ruff format --checkThe two
remainingfailures —tests/test_rest_api.pycollection (ModuleNotFoundError: No module named 'httpx2', a test dependency absent on that host) andtest_candidate_identity_fetches_force_pushed_before_commit— appear identically on the untouchedmerge base. Neither touches the descriptor path. CI is the authority for both.
Relationship to #104
This is #104's work rebuilt directly on
9cd72a91b. The original branch's merge of master wasinterrupted part-way, which left its checkout looking as though it deleted large parts of master;
all of that content is intact, and only the nine files above are real changes. #104 should be
closed in favour of this pull request.
Note for review
admitdecides which refusals to tolerate by matching text inside the exception message(
"unavailable in the explicit descriptor catalog bindings"). If that wording is ever edited thebatch-destroying behaviour returns silently. A typed exception or an error code would bind that
properly, and I would rather it were bound before this merges.
Disclosure
This change was written and verified with AI assistance. Every claim above was measured on the
exact commits in this branch.