A snapshot save in flight across a reset brings the wiped page back, and a restarted computer's snapshots stop landing

[beardthelion]

save in server/src/computer/snapshot-store.ts is an INSERT ... ON CONFLICT DO UPDATE, and its only-ever-forward guard is setWhere: lt(computerSnapshot.snapshotId, values.snapshotId) at line 108. setWhere qualifies the update branch, so it decides nothing when there is no row to conflict with.

resetComputer deletes the row (server/src/computer/gateway.ts:577) precisely because the refs the last snapshot handed out describe a page that no longer exists and a fresh computer counts generations from one again. Between the computer answering /snapshot and the save landing (gateway.ts:313) there is a window, and a reset inside it leaves the delete already done. The in-flight save then inserts unconditionally.

What follows:

1. Session A reaches generation 7. The row holds A's page.
2. A snapshot is in flight.
3. The reset lands. The profile is wiped, the row is deleted.
4. The in-flight save arrives, finds no row, and inserts. The wiped session's page is back at generation 7.
5. The fresh computer counts from one. Its saves take the conflict path, lt(7, 1) is false, and the update is skipped without error.

So refs resolve against a page from a session that was deleted, which is the thing step 3 exists to prevent, and no fresh snapshot lands until the generation climbs past the dead one's.

The larger half

Step 5 does not need the race. agent-computer/src/index.ts:124 keeps the generation in an in-process Map, sessionFor mints snapshotId: 0 for any Bot it is not already holding (:148), and forgetIdleSessions drops sessions whose profile is not live once the map passes 32 (:140-143). Generations therefore restart at one after any container restart, redeploy, crash, or eviction, and none of those clear the server's row. A restarted computer's snapshots are then silently dropped until its counter climbs past whatever the row still holds.

To be clear about what I ran: the server-side half is executed against Postgres, including a generation-1 save being dropped while the row holds 7. The computer restarting its counter I have read at those lines but not executed, since agent-computer/src/index.ts imports Playwright at module scope.

Reproducing it

Two cases against a real database, both failing on be18bc9, with the six existing cases in the file still passing. They drop into server/tests/computer-snapshot-store.integration.test.ts:

test("a save still in flight when the computer was wiped does not bring the old page back", async () => {
  const store = createSnapshotStore(database);
  await store.save("default", snapshot(7, [{ ref: "e9", role: "button", name: "Submit order" }], "https://old.example/order"));

  await store.clear("default");
  // The old session's last snapshot, arriving after the wipe.
  await store.save("default", snapshot(7, [{ ref: "e9", role: "button", name: "Submit order" }], "https://old.example/order"));

  expect(await createSnapshotStore(database).load("default")).toBeUndefined();
});

test("a wiped computer's fresh snapshots are not blocked by the generation the old one reached", async () => {
  const store = createSnapshotStore(database);
  await store.save("default", snapshot(7, [{ ref: "e9", role: "button", name: "Submit order" }], "https://old.example/order"));
  await store.clear("default");
  await store.save("default", snapshot(7, [{ ref: "e9", role: "button", name: "Submit order" }], "https://old.example/order"));

  await store.save("default", snapshot(1, [{ ref: "e1", role: "link", name: "Sign in" }], "https://fresh.example/start"));

  const loaded = await createSnapshotStore(database).load("default");
  expect(loaded?.url).toBe("https://fresh.example/start");
  expect(loaded?.snapshotId).toBe(1);
});

The file's existing "an older snapshot arriving late does not overwrite the newer one" covers the same guard through the conflict path, which is why the insert path read as covered.

Why there is no PR with this

I had a fix ready to write, an epoch owned by the server, bumped on reset and ordered against the generation. Pricing the plumbing is what turned up the restart case, and an epoch bumped on reset does not cover it. Whatever the ordering keys on has to change on any new computer session rather than only on a wipe, which points at the computer stamping a session identity at startup and the server ordering on (session, generation). That spans both services and is your call on where session identity should live, so I stopped rather than guess at it.

One smaller thing found alongside: createInMemorySnapshotStore (snapshot-store.ts:144) has no monotonic guard at all, so the two implementations disagree about the property #46 established.

[NathanTarbert]

Hey @beardthelion,

Thanks for this one, and particularly for stopping where you did. The line references are exact all the way through, and spotting that createInMemorySnapshotStore has no guard at all is the kind of thing that stays invisible until a test quietly proves the wrong thing.

I took the in-memory half in #159. It now applies the same rule the table does, >= to match lt(stored, incoming) exactly, with a test for both the late-arrival and equal-generation cases.

On the insert path, your mechanism is right and I stopped short of the shape, because a premise underneath it does not hold. It is not your premise, to be fair, it is ours: snapshot-store.ts:61 and gateway.ts:576 both say a fresh computer counts generations from one again, and whether that happens depends on which provider is deployed.

agent-computer's /computers/reset handler calls profiles.reset(botId) at src/index.ts:684 and then releases control, and never touches sessions. session.snapshotId is written at :148, :259 and :709 only, none of them the reset path. So on the direct provider path the session outlives the wipe and the fresh browser's first snapshot is the next generation rather than 1. Through the supervisor it is different: supervisor/src/docker.ts:519 removes the container, so that one really does restart at 1.

And your forgetIdleSessions note turns out to matter more than either of us put it. Once the profile is gone the session is evictable at :137-144, so the direct path can restart at 1 too, depending on whether anything pushed the map past 32 in between. That is the strongest form of the argument: both 1 and the next generation are reachable on both paths, so the generation carries no information about which case the store is looking at.

Which means after wiping a row at generation 7, the store can be handed 8 because an old save was in flight, or 8 because the fresh page is genuinely there, and only the page contents tell them apart. Every generation-keyed guard on the insert path picks one to break:

• >= blocks the resurrection and also blocks the fresh browser's every later snapshot on the direct path, forever. No row inserts again and refs stop resolving.
• < only, which is what the two tests you supplied specify, is the same rule with the same effect on a real reset.
• != passes both tests and leaves the realistic case alone, which is just gaming the tests.

So the tests are not wrong about the bug, they are describing the case where the counter did restart. I left them out rather than bank something that has to be undone.

The one shape that fixes it inside server/ is a server-owned epoch per computer, bumped by clear in the same transaction as the delete, captured by gateway.snapshot before the /snapshot call and carried into save, with the write conditional in SQL on the captured epoch still being current. Postgres decides it in one statement, and it does not care what the generation does.

But it would be replaced by the thing you already described. Ordering on (session, generation) subsumes the insert race completely, so landing an epoch first means removing it later. And your instinct that it spans both services is right, with one thing worth adding: the ordinary reset path needs it too, not only restart, redeploy and eviction. The counter surviving a plain /computers/reset on the direct path is the same problem without the container going anywhere.

If you want to write it, it is yours, and I would rather have your version than an epoch. BotSession at agent-computer/src/index.ts:111-124 looks like the natural home for the identity, minted in sessionFor at :148, with the reset handler at :683 re-minting it, which it does not do for anything today. Worth holding both provider paths in mind, since only one of them recreates the container.

Either way the two comments are wrong and want fixing with whatever lands.