diff --git a/backend/scripts/backfill_artifact_user_index_keys.py b/backend/scripts/backfill_artifact_user_index_keys.py index 2b843ecb..6f72950b 100644 --- a/backend/scripts/backfill_artifact_user_index_keys.py +++ b/backend/scripts/backfill_artifact_user_index_keys.py @@ -43,9 +43,12 @@ left alone. * **Never resurrects a deleted row.** ``attribute_exists(SK)`` on every update, matching the writer's own write-back rule. -* **Reports what it cannot fix** rather than guessing — a HEAD row with - no ``updated_at`` is counted and named, not stamped with a fabricated - timestamp that would sort wrongly forever. +* **Encodes a missing ``updated_at`` rather than inventing one.** Such a + row is stamped with an empty timestamp segment + (``ARTIFACT##{aid}``), which sorts below every real timestamp and so + reads last — where the previous in-memory sort put it. Leaving it + unstamped would drop the artifact from a sparse index, and from its + owner's library, silently. Run against dev first, then prod:: @@ -113,16 +116,27 @@ def plan_row(item: Dict[str, Any]) -> Dict[str, Any] | None: return {"skip": f"unexpected PK {pk!r}"} if not artifact_id: return {"skip": "no artifact_id attribute"} - if not updated_at: - # Deliberately not falling back to created_at or "now": GSI2SK is - # the sort key the library orders by, and a fabricated timestamp - # would order this artifact wrongly for the rest of its life. - # Better to name it and let a human decide. - return {"skip": "no updated_at attribute"} - + # A row with no `updated_at` is stamped with an EMPTY timestamp + # segment, not a fabricated one. Two things make that the right + # answer rather than a fudge: + # + # * Leaving it unstamped would drop the artifact out of a sparse + # index — and so out of its owner's library — permanently and + # silently. Dropping somebody's oldest artifacts is worse than + # showing them undated. + # * "ARTIFACT##{aid}" sorts BELOW every real timestamp ("#" < any + # digit), so read descending it lands last — exactly where the + # old in-memory sort put undated rows. It encodes "no timestamp" + # honestly instead of inventing one that would sort wrongly + # forever. + # + # Neither dev nor prod had such a row when this was written; this is + # the defensive branch, and it preserves a contract the library's + # tests already assert. return { "gsi2pk": pk, # GSI2PK is exactly the base PK — USER#{user_id} "gsi2sk": f"ARTIFACT#{updated_at}#{artifact_id}", + "undated": not updated_at, } @@ -145,7 +159,11 @@ def backfill(table: Any, apply: bool) -> Dict[str, int]: continue logger.info( - "stamp %s %s -> GSI2SK=%s", item.get("PK"), sk, plan["gsi2sk"] + "stamp %s %s -> GSI2SK=%s%s", + item.get("PK"), + sk, + plan["gsi2sk"], + " (no updated_at — sorts last)" if plan.get("undated") else "", ) if not apply: stats["stamped"] += 1 diff --git a/backend/src/apis/app_api/artifacts/service.py b/backend/src/apis/app_api/artifacts/service.py index ee078b42..e858897b 100644 --- a/backend/src/apis/app_api/artifacts/service.py +++ b/backend/src/apis/app_api/artifacts/service.py @@ -398,6 +398,9 @@ def get_render_token_service() -> RenderTokenService: # Frozen contract — the HEAD row + SessionIndex keys the artifact writer # (backend/src/agents/builtin_tools/artifacts/service.py) emits. _SESSION_INDEX = "SessionIndex" +# Sparse index over HEAD rows only — read the block comment in +# `list_for_user` before assuming a missing artifact is a query bug. +_USER_INDEX = "UserArtifactsIndex" class ArtifactListService: @@ -532,41 +535,73 @@ def heads_for_session( def list_for_user(self, *, user_id: str) -> list[dict]: """Every artifact the user owns, at HEAD, newest-first. - One base-table Query, no index. The table is already partitioned - by user (`PK=USER#{uid}`), so ownership is enforced by the key - rather than re-checked per row the way `list_for_session` has to - be — a user-wide list is the query this schema was already - shaped for. - - Deliberately not using a GSI. `SessionIndex` is partitioned by - session, not user, so it cannot serve this at all; the sparse - user index the writer stamps keys for (`GSI2PK`/`GSI2SK`) does - not exist yet, and is not needed while the heaviest partition - sits far under a 1MB page. See the writer's module docstring. - - Two consequences of reading the base table, both deliberate: - - * The Query spans version rows as well as HEAD rows, so it reads - roughly 3x what it returns. Filtering happens here rather than - in a FilterExpression because the obvious server-side - discriminator (`attribute_exists(GSI1PK)`) would couple "is - HEAD" to "is session-indexed" — two facts that only happen to - coincide today — and a FilterExpression saves payload, not - read capacity, so it buys nothing worth that coupling. - * The base table sorts by artifact id (a random uuid4), not by - time, so recency ordering is applied here in memory. This is - the part that would move server-side behind the user index. + Served from `UserArtifactsIndex` (GSI2PK=USER#{uid}, + GSI2SK=ARTIFACT#{updated_at}#{aid}) with + `ScanIndexForward=False`. + + This was a base-table Query on the same partition until the + index existed. That worked, but read badly: HEAD and version + rows share the partition, so it spanned roughly 3x the rows it + returned and then date-sorted them in memory. Only HEAD rows + carry the GSI2 keys, so the index holds one row per artifact + already in newest-first order — the amplification and the sort + both go away, and the ordering comes from the store instead of + being recomputed per request. + + ############################################################ + # This index is SPARSE. A HEAD row without GSI2PK is not stale + # in it, it is ABSENT from it — and silently, surfacing as a + # library that lists fewer artifacts than the user made. + # + # Two things keep it complete, and both must stay true: + # * the writer stamps GSI2PK/GSI2SK on BOTH of its write + # paths, and + # * rows predating that (2026-09-04) were stamped by + # `scripts/backfill_artifact_user_index_keys.py`. + # + # If an environment is ever found listing fewer artifacts than + # its table holds, re-run that script before looking anywhere + # else. It is idempotent. + ############################################################ + + Still returns the whole library in one response, paging the + index internally. Exposing pagination is a bigger change than it + looks: search and the type filter are applied in the SPA today, + and a filter that sees only the loaded page is worse than no + filter because it looks authoritative — both would have to move + server-side in the same change. The index makes that possible + whenever it is wanted; it is not wanted yet. """ table = _table() - items: list[dict] = [] + rows: list[dict] = [] kwargs: dict = { - "KeyConditionExpression": Key("PK").eq(f"USER#{user_id}") - & Key("SK").begins_with("ARTIFACT#"), + "IndexName": _USER_INDEX, + "KeyConditionExpression": Key("GSI2PK").eq(f"USER#{user_id}"), + # GSI2SK leads with updated_at, so descending IS newest-first. + "ScanIndexForward": False, } try: while True: resp = table.query(**kwargs) - items.extend(resp.get("Items", [])) + for item in resp.get("Items", []): + if not item.get("artifact_id"): + continue + rows.append( + { + "artifact_id": item.get("artifact_id", ""), + "version": int(item.get("version", 0)), + "title": item.get("title", ""), + "content_type": item.get( + "content_type", "text/html; charset=utf-8" + ), + # Rows written before these attributes existed + # degrade to an empty string rather than + # dropping out of the library. + "created_at": item.get("created_at") or "", + "updated_at": item.get("updated_at") or "", + "session_id": item.get("session_id") or "", + } + ) last = resp.get("LastEvaluatedKey") if not last: break @@ -574,33 +609,8 @@ def list_for_user(self, *, user_id: str) -> list[dict]: except ClientError as exc: raise ArtifactQueryError("artifact library query failed") from exc - heads = [ - item for item in items - if str(item.get("SK", "")).endswith("#HEAD") - ] - rows = [ - { - "artifact_id": item.get("artifact_id", ""), - "version": int(item.get("version", 0)), - "title": item.get("title", ""), - "content_type": item.get( - "content_type", "text/html; charset=utf-8" - ), - # Rows written before these attributes existed degrade to - # an empty string rather than dropping out of the library. - "created_at": item.get("created_at") or "", - "updated_at": item.get("updated_at") or "", - "session_id": item.get("session_id") or "", - } - for item in heads - if item.get("artifact_id") - ] - # Newest-first. Undated legacy rows sort last rather than first, - # which an empty-string key would otherwise do. - rows.sort( - key=lambda row: (bool(row["updated_at"]), row["updated_at"]), - reverse=True, - ) + # No sort here on purpose — the index supplied the order. Adding + # one back would silently mask a broken sort key. return rows @staticmethod diff --git a/backend/tests/apis/app_api/artifacts/test_artifact_library.py b/backend/tests/apis/app_api/artifacts/test_artifact_library.py index e7b651c4..2c684a18 100644 --- a/backend/tests/apis/app_api/artifacts/test_artifact_library.py +++ b/backend/tests/apis/app_api/artifacts/test_artifact_library.py @@ -53,8 +53,24 @@ def client(monkeypatch: pytest.MonkeyPatch): AttributeDefinitions=[ {"AttributeName": "PK", "AttributeType": "S"}, {"AttributeName": "SK", "AttributeType": "S"}, + {"AttributeName": "GSI2PK", "AttributeType": "S"}, + {"AttributeName": "GSI2SK", "AttributeType": "S"}, ], BillingMode="PAY_PER_REQUEST", + # The library endpoint reads this index, so the fixture must + # have it. moto raises ResourceNotFoundException without it — + # correct, and worth keeping: it is what makes these tests + # exercise the index instead of a base-table read. + GlobalSecondaryIndexes=[ + { + "IndexName": "UserArtifactsIndex", + "KeySchema": [ + {"AttributeName": "GSI2PK", "KeyType": "HASH"}, + {"AttributeName": "GSI2SK", "KeyType": "RANGE"}, + ], + "Projection": {"ProjectionType": "ALL"}, + } + ], ) monkeypatch.setenv("DYNAMODB_ARTIFACTS_TABLE_NAME", TABLE) @@ -116,8 +132,12 @@ def _put_artifact( if updated_at is not None: head["updated_at"] = updated_at head["GSI1SK"] = f"ARTIFACT#{updated_at}#{artifact}" - head["GSI2PK"] = f"USER#{user_id}" - head["GSI2SK"] = f"ARTIFACT#{updated_at}#{artifact}" + # GSI2 keys are stamped either way — the post-backfill state of the + # table. An undated row carries an empty timestamp segment, which + # sorts below every real one, so it reads last instead of dropping + # out of the sparse index entirely. + head["GSI2PK"] = f"USER#{user_id}" + head["GSI2SK"] = f"ARTIFACT#{updated_at or ''}#{artifact}" table.put_item(Item=head) @@ -242,7 +262,13 @@ def test_library_route_is_not_shadowed_by_the_artifact_id_route(client) -> None: def test_paginates_a_partition_larger_than_one_page(client) -> None: """The Query loop must drain `LastEvaluatedKey`. Asserted with a stub rather than 1MB of fixture rows, since moto pages on real byte - size and a realistic partition is far under the limit.""" + size and a realistic partition is far under the limit. + + Page 1 carries the NEWER row, because that is what the index does: + GSI2SK leads with `updated_at` and is read descending. So the + service must preserve page order rather than re-sort — a + client-side sort would pass here either way and hide a broken sort + key.""" calls: list[dict] = [] class Paged: @@ -253,14 +279,14 @@ def query(self, **kwargs): "Items": [ { "PK": f"USER#{USER_ID}", - "SK": "ARTIFACT#a1#HEAD", - "artifact_id": "a1", + "SK": "ARTIFACT#a2#HEAD", + "artifact_id": "a2", "version": 1, - "title": "One", + "title": "Two", "content_type": "text/markdown", - "created_at": "2026-05-01T09:00:00+00:00", - "updated_at": "2026-05-01T09:00:00+00:00", - "session_id": "s1", + "created_at": "2026-05-02T09:00:00+00:00", + "updated_at": "2026-05-02T09:00:00+00:00", + "session_id": "s2", } ], "LastEvaluatedKey": {"PK": "x", "SK": "y"}, @@ -269,14 +295,14 @@ def query(self, **kwargs): "Items": [ { "PK": f"USER#{USER_ID}", - "SK": "ARTIFACT#a2#HEAD", - "artifact_id": "a2", + "SK": "ARTIFACT#a1#HEAD", + "artifact_id": "a1", "version": 1, - "title": "Two", + "title": "One", "content_type": "text/markdown", - "created_at": "2026-05-02T09:00:00+00:00", - "updated_at": "2026-05-02T09:00:00+00:00", - "session_id": "s2", + "created_at": "2026-05-01T09:00:00+00:00", + "updated_at": "2026-05-01T09:00:00+00:00", + "session_id": "s1", } ] } diff --git a/backend/tests/test_backfill_artifact_user_index_keys.py b/backend/tests/test_backfill_artifact_user_index_keys.py index 6035f2ac..9a9692a3 100644 --- a/backend/tests/test_backfill_artifact_user_index_keys.py +++ b/backend/tests/test_backfill_artifact_user_index_keys.py @@ -165,18 +165,24 @@ def test_spans_every_user(table): # ------------------------------------------------------------------ -def test_refuses_to_fabricate_a_sort_key(table): - """A HEAD row with no `updated_at` is reported, never guessed at. - - GSI2SK is what the library orders by; inventing a timestamp would - sort that artifact wrongly for the rest of its life, invisibly.""" +def test_encodes_a_missing_timestamp_instead_of_inventing_one(table): + """A HEAD row with no `updated_at` is still indexed, with an EMPTY + timestamp segment. + + Leaving it unstamped would drop the artifact out of a sparse index — + and out of its owner's library — silently. An empty segment sorts + below every real timestamp, so descending it reads last, exactly + where the previous in-memory sort put undated rows.""" put_head(table, updated_at=None) stats = backfill_mod.backfill(table, apply=True) - assert stats["skipped"] == 1 - assert stats["stamped"] == 0 - assert "GSI2PK" not in row(table) + assert stats["stamped"] == 1 + assert stats["skipped"] == 0 + item = row(table) + assert item["GSI2SK"] == "ARTIFACT##a1" + # "#" is below every digit, so this sorts under any real timestamp. + assert item["GSI2SK"] < "ARTIFACT#2026-01-01T00:00:00+00:00#a1" def test_dry_run_writes_nothing(table):