fix(telemetry): the reading project survives a backup (#4196)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 10s
CI & Build / TypeScript typecheck (push) Successful in 54s
CI & Build / integration (push) Successful in 53s
CI & Build / Python tests (push) Successful in 1m38s
CI & Build / Build & push image (push) Successful in 27s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 10s
CI & Build / TypeScript typecheck (push) Successful in 54s
CI & Build / integration (push) Successful in 53s
CI & Build / Python tests (push) Successful in 1m38s
CI & Build / Build & push image (push) Successful in 27s
Run 7068 failed two tests on a4aae97, and the second was a real bug the
guard caught before it shipped.
THE COLUMN GUARD (#3182) was right. `note_usage_events` excludes only
`id` from backup, so telemetry IS exported — and a new column that the
serialiser never learned about would have restored as null on every row.
That is the exact failure #3182 was built from, where nine columns
vanished from `notes` the same way: added to the model and the migration,
which fail loudly, and never to the serialiser, which fails silently.
So `project_id` is exported, not excluded, via the one `_usage_event_rows`
builder both exporters share.
The importer re-maps it through `project_id_map` and DEGRADES rather than
skipping. `code_shape_events` drops a row whose project will not map,
because a shape event without its project says nothing; a usage event is
not like that. The project is optional by design and null already means
"not reported", so an unmappable one restores as unreported. Dropping it
would lose a real pull and deflate the pull-through this table exists to
report.
BACKUP_VERSION deliberately unchanged: the suite states that column
additions do not bump it, only new sections do, and a third test asserts
the current number.
The second failure was mine and smaller — test_record_pulled_writes_a_
single_row compares exact dicts, so it now expects the null project. I
had claimed these assertions were key-based; they are not, and I had
read a summary line rather than the assertion.
Added an import-side guard while here. The column family checks the
EXPORT side only, so a column can be exported faithfully and dropped on
the way back in — a restore that reports success and has quietly lost a
dimension. No general guard for that direction exists yet; this covers
the column added here and names the gap for whoever builds the general
one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -333,7 +333,7 @@ def _usage_event_rows(rows) -> list[dict]:
|
|||||||
return [
|
return [
|
||||||
{
|
{
|
||||||
"user_id": r.user_id, "note_id": r.note_id, "event": r.event,
|
"user_id": r.user_id, "note_id": r.note_id, "event": r.event,
|
||||||
"source": r.source,
|
"source": r.source, "project_id": r.project_id,
|
||||||
"created_at": r.created_at.isoformat() if r.created_at else None,
|
"created_at": r.created_at.isoformat() if r.created_at else None,
|
||||||
}
|
}
|
||||||
for r in rows
|
for r in rows
|
||||||
@@ -1548,6 +1548,12 @@ async def _restore_v2(data: dict) -> dict:
|
|||||||
note_id=mapped_nid,
|
note_id=mapped_nid,
|
||||||
event=ev.get("event", ""),
|
event=ev.get("event", ""),
|
||||||
source=ev.get("source", ""),
|
source=ev.get("source", ""),
|
||||||
|
# Degrades to None rather than dropping the row: unlike a shape
|
||||||
|
# event, a usage event without a project is still a real pull,
|
||||||
|
# and discarding it would deflate the pull-through this table
|
||||||
|
# exists to report. Null already means "not reported".
|
||||||
|
project_id=(project_id_map.get(ev["project_id"])
|
||||||
|
if ev.get("project_id") else None),
|
||||||
created_at=_dt(ev.get("created_at")),
|
created_at=_dt(ev.get("created_at")),
|
||||||
))
|
))
|
||||||
stats["note_usage_events"] += 1
|
stats["note_usage_events"] += 1
|
||||||
|
|||||||
@@ -41,7 +41,8 @@ async def test_record_pulled_writes_a_single_row():
|
|||||||
record_pulled(user_id=7, note_id=11, source="mcp_get_snippet")
|
record_pulled(user_id=7, note_id=11, source="mcp_get_snippet")
|
||||||
rows = sched.call_args[0][0]
|
rows = sched.call_args[0][0]
|
||||||
assert rows == [
|
assert rows == [
|
||||||
{"user_id": 7, "note_id": 11, "event": "pulled", "source": "mcp_get_snippet"}
|
{"user_id": 7, "note_id": 11, "event": "pulled",
|
||||||
|
"source": "mcp_get_snippet", "project_id": None}
|
||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -245,6 +245,42 @@ def test_every_column_is_exported_or_declared_excluded(table):
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_the_usage_importer_restores_the_reading_project():
|
||||||
|
"""The column guard above checks the EXPORT side only.
|
||||||
|
|
||||||
|
A column can be exported faithfully and then dropped on the way back in,
|
||||||
|
which restores a backup that reports success and has quietly lost a
|
||||||
|
dimension — #3182's failure mode, one direction over. There is no general
|
||||||
|
import-side guard yet; this covers the column #4196 added, by source
|
||||||
|
inspection, because the behavioural path needs Postgres.
|
||||||
|
|
||||||
|
It also pins the DEGRADE. `code_shape_events` skips a row whose project
|
||||||
|
will not map, because a shape event without its project says nothing. A
|
||||||
|
usage event is not like that: the project is optional by design and null
|
||||||
|
already means "not reported", so an unmappable one must restore as
|
||||||
|
unreported rather than vanish — dropping it would lose a real pull and
|
||||||
|
deflate the very pull-through this table exists to report.
|
||||||
|
"""
|
||||||
|
import inspect
|
||||||
|
|
||||||
|
src = inspect.getsource(backup._restore_v2)
|
||||||
|
marker = 'for ev in data.get("note_usage_events", []):'
|
||||||
|
assert marker in src, "the usage import loop moved; this guard is blind"
|
||||||
|
block = src[src.index(marker):][:1200]
|
||||||
|
|
||||||
|
assert 'ev.get("project_id")' in block, (
|
||||||
|
"the usage importer drops project_id — a restore would report success "
|
||||||
|
"and come back without the reading project"
|
||||||
|
)
|
||||||
|
assert "project_id_map" in block, (
|
||||||
|
"project_id must be re-mapped; a raw id points at whatever project "
|
||||||
|
"happens to hold that number in the destination install"
|
||||||
|
)
|
||||||
|
assert "continue" not in block.split('project_id=')[1][:200], (
|
||||||
|
"an unmappable project must degrade to None, not skip the row"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
def test_the_column_guard_covers_every_table_with_a_row_helper():
|
def test_the_column_guard_covers_every_table_with_a_row_helper():
|
||||||
"""The guard is only as good as its registry — a table added to _BACKED_UP
|
"""The guard is only as good as its registry — a table added to _BACKED_UP
|
||||||
with a new helper, and not to the registry, would be unguarded and look
|
with a new helper, and not to the registry, would be unguarded and look
|
||||||
|
|||||||
Reference in New Issue
Block a user