fix(automations): send the compare-and-set active value to the PDS record on PATCH #96

Queued: the loop will work on this version of the issue.

loop:queued cid=bafyreicnfedw6jq2gbepetwar62jlsb2kq72mt6c45dwzpni6fa2tj3hxm

Working on this, attempt 1.

loop:claim run=contribute-20261103-130336-96 branch=loop/96 attempt=1 issue=96

fix(automations): send the compare-and-set active value to the PDS record on PATCH

PATCH /api/automations/:rkey sent active = body.active ?? auto.active to the owner's PDS, then let the database's compare-and-set settle on nextActive, which keeps a background disable that landed in between. When the two differ, the handler now puts the PDS record again with nextActive, so the PDS record and the index row end the PATCH with the same active. If that second put fails, the row keeps its pdsStaleSince marker, and the existing session-reactivated catch-up (lib/automations/pds-rewrite.ts) puts the record again later, built from the row.

What changed #

• app/routes/api/automations/[rkey].ts: the record is built by one pdsRecord(active) helper, used for both puts. The CAS sets pdsStaleSince to null when nextActive === active (as before) and to now otherwise. After the CAS, a different nextActive triggers a second put with that value, which clears the marker once it lands. A failed second put is logged and the PATCH still returns 200, because the database write has already been committed.
• app/routes/api/automations/[rkey].test.ts: new tests for the worker-disable race (two puts, the second with active: false, marker cleared), for a failed second put (marker kept), and for the normal path (one put). The explicit-enable race test now also checks that only one put happens.

Verification #

• pnpm run check: passes.
• pnpm run test: 2775 tests pass.
• With the route change stashed, the two new race tests fail, so they do catch the bug.
• rg "putRecord\(": the only other put next to an active CAS is lib/sync/redundant-automations.ts. It writes active: false to both the PDS and the database, and a concurrent disable leaves both at false, so it does not have this bug.

Acceptance criteria #

• When the CAS's nextActive differs from the active sent to the PDS, the PDS record is written again with nextActive.
• A test simulates a worker disable between the initial read and the database update, and checks that the PDS write ends up with active: false ("puts the PDS record again with the active value a mid-flight auto-disable settled on").

Out of scope, untouched #

Reconciling PDS and index copies that already disagree: nothing on the branch touches existing rows. The marker is only set by a PATCH whose own second put fails.

Decisions the owner can reverse #

• I chose a second put after the CAS over writing the database first. Writing the database first would leave the index changed when the PDS put fails, and the current 502 path promises the row stays untouched in that case.
• pdsStaleSince now also means "the PDS copy's active is behind the row" after a failed second put. This lets the KTD-13 catch-up converge the record. Without it, a failed second put would only be logged.
• A failed second put still returns 200 rather than an error, because the user's edit was saved to the index.

Review: https://tangled.org/exosphere.site/airglow/compare/main...loop/96

loop:pushed run=contribute-20261103-130336-96 sha=7842332010907c694efad3839d2439f93537f864

changes-requested. The route fix is right and meets both acceptance criteria. When the compare-and-set's nextActive differs from the active the first put sent, the record is put again with nextActive, and the test that simulates a worker disable checks that this second put has active: false. One gap is left: the branch gives pdsStaleSince a second meaning, "the PDS copy's active is behind the row after a failed re-put". But the column's own doc comment and the code that reads it still say it means only "the PDS copy is still the pre-steps shape". That column is a migration marker meant to last one release, so the next person to clean it up would remove the fallback this branch relies on.

What I checked:

• Route logic (app/routes/api/automations/[rkey].ts). pdsRecord(active) builds the same record for both puts, with $type on the trigger and the original createdAt. A second put only happens when body.active is unset and the row changed underneath. An explicit enable or disable always gives nextActive === active, so there is only one put. The marker is set in the CAS and cleared only after the re-put lands. If the re-put fails, the PATCH still returns 200 and the row keeps the user's edit.
• Real workers. I wrote a throwaway test (since deleted) that calls the real worker functions from inside the first put, which is where the race happens:
  • disableForRateLimit during a plain rename: the last put has active: false, the same as the row. disabledReason is still rate_limit:hour and the marker is cleared.
  • reactivateSessionLostAutomations during a PATCH on a disabled automation that only sets dryRun: the last put has active: true, the same as the row.
• New tests catch the bug. I put back the base route file, and the two new race tests failed. With the branch's route they pass.
• Other places with this bug. I searched every putRecord( caller and every write to automations.active. redundant-automations.ts writes false to both copies. pds-rewrite.ts writes only the marker after its put. The worker disables (rate-limit, session-loss, dry-run expiry) only write to the database. None of them has this bug.
• Out of scope. Rows that already disagree are left alone. The marker is only set by a PATCH whose own re-put fails.
• Checks and tests. pnpm run check passes and pnpm run test passes (2775 tests). There is no migration.

Changes:

1. lib/db/schema.ts:398-400: rewrite the pdsStaleSince comment to give both meanings. For example: "Non-null while this row's PDS copy is behind the row: still the pre-steps shape (set for every row by the migration), or carrying an active that a PATCH's compare-and-set overrode when its re-put failed. Cleared by any successful full-record put (the U14 rewrite script, a user PATCH, or the session_reactivated catch-up)."
2. lib/automations/pds-rewrite.ts:1-5 (module header) and lib/jetstream/session-loss.ts:133-135 (catch-up comment): make the same change, so neither says the marker only means the pre-steps shape.
3. app/routes/api/automations/[rkey].ts:321-326 and the comment in the test "keeps the PDS-stale marker..." both say the catch-up will put the record again. catchUpStalePdsRecords only runs when reactivateSessionLostAutomations actually reactivates something (session-loss.ts:106). So a re-put that fails after a rate-limit disable converges only on the owner's next save, a later session reactivation, or a run of the rewrite script. Name those three in both comments, instead of saying the catch-up handles it.

loop:verdict run=review-20261103-131041-96 sha=7842332010907c694efad3839d2439f93537f864 verdict=changes-requested

Fix round 1: the review asked for changes.

loop:fix run=contribute-20261103-131459-96 round=1 issue=96

fix(automations): send the compare-and-set active value to the PDS record on PATCH

PATCH /api/automations/:rkey sent active = body.active ?? auto.active to the owner's PDS, then let the database's compare-and-set settle on nextActive, which keeps a background disable that landed in between. When the two differ, the handler now puts the PDS record again with nextActive, so the PDS record and the index row end the PATCH with the same active. If that second put fails, the row keeps its pdsStaleSince marker, and the copies converge on the owner's next save, a later session reactivation (whose catch-up in lib/automations/pds-rewrite.ts only runs when it reactivates something), or a run of the U14 rewrite script.

What changed #

• app/routes/api/automations/[rkey].ts: the record is built by one pdsRecord(active) helper, used for both puts. The CAS sets pdsStaleSince to null when nextActive === active (as before) and to now otherwise. After the CAS, a different nextActive triggers a second put with that value, which clears the marker once it lands. A failed second put is logged and the PATCH still returns 200, because the database write has already been committed.
• lib/db/schema.ts, lib/automations/pds-rewrite.ts, lib/jetstream/session-loss.ts: the comments on pdsStaleSince now give both of its meanings (pre-steps shape, or an active a PATCH's CAS overrode when its re-put failed), so a later cleanup of the migration marker does not drop the fallback this branch relies on.
• app/routes/api/automations/[rkey].test.ts: new tests for the worker-disable race (two puts, the second with active: false, marker cleared), for a failed second put (marker kept), and for the normal path (one put). The explicit-enable race test now also checks that only one put happens.

Fix round 1 #

The review asked for comment changes only: pdsStaleSince's doc comments in lib/db/schema.ts, lib/automations/pds-rewrite.ts and lib/jetstream/session-loss.ts now give both meanings, and the route comment and the failed-re-put test comment name the three ways the copies converge instead of claiming the catch-up handles it. No code changed.

Verification #

• pnpm run check: passes.
• pnpm run test: 2775 tests pass.
• With the route change stashed, the two new race tests fail, so they do catch the bug.
• rg "putRecord\(": the only other put next to an active CAS is lib/sync/redundant-automations.ts. It writes active: false to both the PDS and the database, and a concurrent disable leaves both at false, so it does not have this bug.

Acceptance criteria #

• When the CAS's nextActive differs from the active sent to the PDS, the PDS record is written again with nextActive.
• A test simulates a worker disable between the initial read and the database update, and checks that the PDS write ends up with active: false ("puts the PDS record again with the active value a mid-flight auto-disable settled on").

Out of scope, untouched #

Reconciling PDS and index copies that already disagree: nothing on the branch touches existing rows. The marker is only set by a PATCH whose own second put fails.

Decisions the owner can reverse #

• I chose a second put after the CAS over writing the database first. Writing the database first would leave the index changed when the PDS put fails, and the current 502 path promises the row stays untouched in that case.
• pdsStaleSince now also means "the PDS copy's active is behind the row" after a failed second put, and its doc comments say so. This lets the next save, a session reactivation's catch-up or the rewrite script converge the record. Without it, a failed second put would only be logged.
• A failed second put still returns 200 rather than an error, because the user's edit was saved to the index.

Review: https://tangled.org/exosphere.site/airglow/compare/main...loop/96

loop:pushed run=contribute-20261103-131459-96 sha=6b5bed8a2209c3547fc16c8a1ea3c3c25d71b2cf

merge. The branch does what the issue asks. When the compare-and-set settles on an active that differs from the one the first put sent, the PATCH puts the PDS record again with the settled nextActive and clears pdsStaleSince once that put lands. A test simulates a worker disable between the initial read and the database update and checks that the last PDS write has active: false. Round 1 asked for three comment fixes, and all three are made.

What I checked:

• Round 1's requests. git diff 7842332 6b5bed8 changes only comments:
  • The pdsStaleSince comments in lib/db/schema.ts, lib/automations/pds-rewrite.ts and lib/jetstream/session-loss.ts now give both meanings of the marker.
  • The route comment and the comment in the failed-re-put test now name the three ways the two copies converge (the owner's next save, a later session reactivation, the rewrite script) instead of saying the catch-up handles it.
  • The route logic round 1 checked is unchanged.
• The route (app/routes/api/automations/[rkey].ts):
  • One pdsRecord(active) builder makes both puts. It keeps $type on the trigger and the original createdAt.
  • A second put happens only when body.active is not set and the row changed in between, so an explicit enable or disable still makes exactly one put.
  • The CAS sets the marker only when nextActive !== active, and the marker is cleared only after the re-put succeeds.
  • If the re-put fails, the PATCH returns 200 and the row keeps the edit and the marker. catchUpStalePdsRecords and the rewrite script build the record from the row, so either one would put the right active.
• The new tests catch the bug. With the base route file swapped in, the two new race tests fail (putRecord is called once, not twice). With the branch's route file they pass, and I restored that file afterwards.
• Other places with this bug. I searched every putRecord( caller and every pdsStaleSince reader and writer:
  • redundant-automations.ts writes false to both copies.
  • pds-rewrite.ts builds its record from the row.
  • The migration scripts set only the marker.
  • None of them has the PATCH race.
• Out of scope. Nothing touches rows that already disagree. The marker is set only by a PATCH whose own re-put fails.
• Checks and tests. pnpm run check passes, and pnpm run test passes all 2775 tests. The branch has no migration and no UI change.

loop:verdict run=review-20261103-131906-96 sha=6b5bed8a2209c3547fc16c8a1ea3c3c25d71b2cf verdict=merge

Merged main into the branch: e5ad5b31c90fe919997eddfff04f8cbee78ef0f3.

loop:carry from=6b5bed8a2209c3547fc16c8a1ea3c3c25d71b2cf sha=e5ad5b31c90fe919997eddfff04f8cbee78ef0f3

Merged onto main as 75bd49ec5f57d5287f6f64f14274737ace834aaf.

loop:merged sha=75bd49ec5f57d5287f6f64f14274737ace834aaf head=e5ad5b31c90fe919997eddfff04f8cbee78ef0f3