A cancelled RefreshPeople run discards the stamps it earned #159

New issue

Closed

opened 2026-08-20 19:49:06 +02:00 by Claude · 1 comment

Claude commented

2026-08-20 19:49:06 +02:00

Collaborator

Three things left beside RefreshPeople after #154, none of them the defect
that issue was about. Filed together because they are the same two files.

A run that is cancelled discards every stamp it earned.
crates/ephyra-server/src/libraries.rs::refresh_people collects the people no
database knew and calls mark_people_refreshed once, after the loop. The loop
is cancel.check()? at the top of every iteration, so a cancelled run returns
through ? with the batch unwritten — and every person it asked about and found
nothing for is asked about again on the next run. The people it did file are
kept, because those are written as they go.

A person whose row carries an empty name is asked about forever.
people_wanting_a_refresh answers a Wanting for any Person row whose
Name is not null, empty string included. Write::for_a_name then refuses it,
which is Refreshed::Failed, which writes no stamp by design — so the row comes
back every run. Pre-existing rather than introduced by #154, and it is the one
path Failed can loop on rather than retry. The question is whether such a row
should exist at all: forget_people_nobody_credits already excludes
Name IS NULL, and an empty name is the same kind of row.

mark_people_refreshed is ItemWriter::mark_refreshed copied.
crates/ephyra-db/src/scan/people.rs and
crates/ephyra-db/src/items/writes.rs:272 are the same statement modulo the
Type = ? filter. The copy is forced today — placeholders and
BOUND_AT_ONCE are both pub(super) to items — but the scan copy spells the
batch size 500 as a literal where the other names it.

Where to start

The first is the one worth doing: stamp as the loop goes rather than after it,
or write what is pending on the way out. It is the only one of the three that
loses work.

How to know it is done

A RefreshPeople run cancelled halfway leaves the people it already asked about
stamped, and asks about the rest on the next run rather than all of them.

House rules

CLAUDE.md is the short version and these are the parts that bite: tests run
under cargo nextest; upstream's source is the checkout in upstream/ at the
pin; generated files are regenerated rather than edited; a comment says what
the code does, not how it got there.

A test that cannot fail is worse than none.

Three things left beside `RefreshPeople` after #154, none of them the defect that issue was about. Filed together because they are the same two files. **A run that is cancelled discards every stamp it earned.** `crates/ephyra-server/src/libraries.rs::refresh_people` collects the people no database knew and calls `mark_people_refreshed` once, after the loop. The loop is `cancel.check()?` at the top of every iteration, so a cancelled run returns through `?` with the batch unwritten — and every person it asked about and found nothing for is asked about again on the next run. The people it *did* file are kept, because those are written as they go. **A person whose row carries an empty name is asked about forever.** `people_wanting_a_refresh` answers a `Wanting` for any `Person` row whose `Name` is not null, empty string included. `Write::for_a_name` then refuses it, which is `Refreshed::Failed`, which writes no stamp by design — so the row comes back every run. Pre-existing rather than introduced by #154, and it is the one path `Failed` can loop on rather than retry. The question is whether such a row should exist at all: `forget_people_nobody_credits` already excludes `Name IS NULL`, and an empty name is the same kind of row. **`mark_people_refreshed` is `ItemWriter::mark_refreshed` copied.** `crates/ephyra-db/src/scan/people.rs` and `crates/ephyra-db/src/items/writes.rs:272` are the same statement modulo the `Type = ?` filter. The copy is forced today — `placeholders` and `BOUND_AT_ONCE` are both `pub(super)` to `items` — but the scan copy spells the batch size `500` as a literal where the other names it. ## Where to start The first is the one worth doing: stamp as the loop goes rather than after it, or write what is pending on the way out. It is the only one of the three that loses work. ## How to know it is done A `RefreshPeople` run cancelled halfway leaves the people it already asked about stamped, and asks about the rest on the next run rather than all of them. ### House rules `CLAUDE.md` is the short version and these are the parts that bite: tests run under `cargo nextest`; upstream's source is the checkout in `upstream/` **at the pin**; generated files are regenerated rather than edited; a comment says what the code does, not how it got there. A test that cannot fail is worse than none.

Claude added the

scanner

labels

2026-08-20 19:49:17 +02:00

Claude referenced this issue

2026-08-20 19:49:46 +02:00

A person is asked about on every RefreshPeople run: DateLastRefreshed is never stamped #154

Claude commented

2026-08-21 16:41:24 +02:00

Author

Collaborator

Landed in 79d88c3, with review fixes in 22de82e. All three.

The one that lost work. The loop returns through ? on cancel.check(), so
the batch went with it. It is written on the way out as well as at the end now,
and the loop became ask_about_people — a function of a resolved database
rather than of the configuration — because stopping a run part way is the only
way to reach that path, and a test cannot do it through the configuration.

The test stands a database up that answers "nobody by that name" and cancels the
run on its first fetch: one person is asked about, the second check fires, and
the stamp has to survive it. It fails against the old shape.

The row with no name. people_wanting_a_refresh now gates on
TRIM(Name) <> '', which is what the writer trims — the review caught that
Name <> '' left a row named with a single space still being refused every run
and stamped never. A null name is excluded by the same comparison.

The copy. mark_people_refreshed reads BOUND_AT_ONCE and placeholders
from items, where the statement it was copied from names them, rather than
spelling 500 itself.

One thing the review added: when a stopped run's stamp write also fails, the
loop's error is still the one reported, but the stamp's is logged rather than
dropped silently.

Landed in 79d88c3, with review fixes in 22de82e. All three. **The one that lost work.** The loop returns through `?` on `cancel.check()`, so the batch went with it. It is written on the way out as well as at the end now, and the loop became `ask_about_people` — a function of a resolved database rather than of the configuration — because stopping a run part way is the only way to reach that path, and a test cannot do it through the configuration. The test stands a database up that answers "nobody by that name" and cancels the run on its first fetch: one person is asked about, the second check fires, and the stamp has to survive it. It fails against the old shape. **The row with no name.** `people_wanting_a_refresh` now gates on `TRIM(Name) <> ''`, which is what the writer trims — the review caught that `Name <> ''` left a row named with a single space still being refused every run and stamped never. A null name is excluded by the same comparison. **The copy.** `mark_people_refreshed` reads `BOUND_AT_ONCE` and `placeholders` from `items`, where the statement it was copied from names them, rather than spelling 500 itself. One thing the review added: when a stopped run's stamp write *also* fails, the loop's error is still the one reported, but the stamp's is logged rather than dropped silently.

Claude closed this issue

2026-08-21 16:41:24 +02:00