From 81456cd75bd95587680e8852988540a9bbdb0a1a Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Sun, 6 Sep 2026 14:39:22 -0600 Subject: [PATCH] perf(artifacts): serve the library listing from UserArtifactsIndex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⚠️ MUST NOT MERGE until `UserArtifactsIndex` reports ACTIVE in the target environment. `DescribeTable`, not CloudFormation — CFN reports UPDATE_COMPLETE while DynamoDB is still backfilling the index. `list_for_user` now queries the index (GSI2PK=USER#{uid}, GSI2SK descending) instead of the base table. HEAD and version rows share the base partition, so the old Query 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 newest-first. The amplification and the sort both go away, and the ordering now comes from the store rather than being recomputed per request. 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 live in the SPA, and a filter that sees only the loaded page is worse than no filter because it looks authoritative, so both would have to move server-side in the same change. The index makes that possible whenever it is wanted. ## Two things this turned up **The library tests were passing against the old code path.** My first edit spliced the new method in with inverted slice boundaries, leaving DUPLICATE `list_for_user` and `heads_for_session` definitions — Python took the last, which was the original base-table version. The suite went green while testing nothing new. Caught by asking why tests that should have needed a GSI passed without one; the fixture had no GlobalSecondaryIndexes at all. The fixture now declares the index, so moto raises ResourceNotFoundException if the query ever stops using it — which is what makes these tests exercise the index rather than silently falling back. **Undated rows would have vanished.** A sparse index omits any HEAD row without GSI2PK, permanently and silently. Rows predating `updated_at` cannot carry a real timestamp, so under the first version of the backfill they were skipped — and would have dropped out of their owner's library, which `test_undated_legacy_rows_are_returned_and_sort_last` exists to forbid. They are now stamped with an EMPTY timestamp segment (`ARTIFACT##{aid}`). That is not a fabricated time: "#" sorts below every digit, so read descending the row lands last — exactly where the old in-memory sort put it. Neither dev nor prod holds such a row today; this is the defensive branch, and it preserves a contract the tests already assert. The pagination stub now yields the NEWER row on page 1, matching what a descending index does. With the old stub a client-side re-sort would have passed either way and hidden a broken sort key. Backend: 942 passed across app_api, architecture and the artifact writer suites. Co-Authored-By: Claude Opus 5 --- .../backfill_artifact_user_index_keys.py | 40 ++++-- backend/src/apis/app_api/artifacts/service.py | 120 ++++++++++-------- .../artifacts/test_artifact_library.py | 56 +++++--- .../test_backfill_artifact_user_index_keys.py | 22 ++-- 4 files changed, 149 insertions(+), 89 deletions(-) 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):