Skip to content

feat: HW-017 optimistic concurrency guard for task PATCH - #31

Open
mfethe1 wants to merge 6 commits into
product/mainfrom
feat/HW-017
Open

feat: HW-017 optimistic concurrency guard for task PATCH#31
mfethe1 wants to merge 6 commits into
product/mainfrom
feat/HW-017

Conversation

@mfethe1

@mfethe1 mfethe1 commented Sep 11, 2026

Copy link
Copy Markdown
Owner

HW-017: Guarded task PATCH — optimistic concurrency

Adds a revision counter to the tasks table and an expected_revision precondition on PATCH. A stale write that loses the race gets 409 Conflict instead of silently overwriting.

What changed (14 files, +541/-6)

  • migrations/0047_task_optimistic_concurrency.sqlrevision INT NOT NULL DEFAULT 0 column + BEFORE UPDATE trigger that bumps only on real payload change (whole-row IS DISTINCT FROM, normalizes derived columns)
  • crates/buzz-db/src/task.rsTaskRecord.revision, TaskPatch.expected_revision, CAS guard inside SELECT ... FOR UPDATE, DbError::StaleRevision
  • crates/buzz-db/src/error.rs — StaleRevision variant
  • crates/buzz-relay/src/api/tasks.rs — wire expected_revision from JSON body
  • crates/buzz-cli/src/commands/tasks.rs + lib.rs--expected-revision CLI flag
  • desktop/src-tauri/src/commands/tasks.rs — expected_revision on status/assignee
  • desktop/src/features/tasks/lib/channelTasks.ts — revision field on type
  • mobile/lib/shared/tasks/task.dart + tasks_api.dart — revision field + expectedRevision param
  • mobile/test/.../thread_task_chip_test.dart — revision assertion
  • schema/schema.sql — revision column in desired-state SSOT
  • crates/buzz-db/examples/hw017_migrate.rs — migration example

Gates (all GREEN, re-verified 2026-09-10T22:20-0400)

Gate Result
cargo check -p buzz-db ✅ Finished dev
cargo check -p buzz-relay ✅ Finished dev
cargo test -p buzz-db --lib task ✅ 5 passed, 0 failed, 8 ignored (Postgres)
cargo test -p buzz-relay --lib tasks ✅ 10 passed, 0 failed, 1 ignored
pnpm typecheck ✅ tsc --noEmit clean (rc=0)
npx biome check channelTasks.ts ✅ No issues
flutter analyze lib/shared/tasks/ ✅ No issues found
Migration 0047 collision ✅ Zero open 0047 PRs on block/buzz or mfethe1/buzz

Known gaps (honesty)

  • Postgres-gated tests (8 #[ignore]): compile and register but runtime behavior unverified without a live PG instance. Previously verified with PG on 2026-09-09 (8/8 pass, including mutation test). Not re-run this session.
  • DCO gap: commit 6ec3922 (initial feat) lacks Signed-off-by trailer — pre-existing, unfixable without force-push (banned). Fix via squash-amend at merge time.
  • No live PG concurrency test this session (verified 2026-09-09 with real Postgres: two-connection interleave proven via pg_locks/pg_blocking_pids).
  • Migration 0047 may collide upstream if/when the task system lands on block/buzz. Fork-only for now; renumber at integration time.

Backward compatibility

expected_revision: None (omitted) skips the guard entirely → existing clients get last-write-wins unchanged. Column defaults to 0, no backfill needed.

Security

Guard runs AFTER tenant scoping → stale revision on a foreign-tenant task returns NotFound, not StaleRevision. No existence leak.

Branch

feat/HW-017 @ 0d778d686d (5 commits, pushed to origin/mfethe1/buzz)
Base: product/main @ adeec6eb5c

Refs: spec ~/buzz-program/specs/HW-017.md, stage docs ~/buzz-program/registry/work/HW-017/{hardening,verifying}.md, brief ~/buzz-program/briefs/HW-017-handoff.md

mfethe1 and others added 5 commits September 6, 2026 18:56
Add revision column (INT NOT NULL DEFAULT 0) to tasks table via
migration 0047. A BEFORE UPDATE trigger increments revision on every
row change, so every successful PATCH returns a strictly-increasing
revision number.

TaskPatch gains expected_revision: Option<i64>. When supplied, the
db layer checks it against the row's current revision and returns
StaleRevision if they differ — the relay maps this to HTTP 409 with
the current revision in the body so the client can re-fetch and retry.

All surfaces updated: TaskRecord + task_columns!, parse_task_row,
UpdateTaskRequest (relay), TasksCmd::Update --expected-revision (cli),
ChannelTask + tasks_set_status/tasks_set_assignee (desktop Rust + TS),
Task.revision + updateTask expectedRevision (mobile Dart).

Backward-compatible: expected_revision is optional on the wire. Old
clients that don't send it get the old behavior (last-writer-wins) but
still see the new revision field in responses. The trigger handles
updated_at so the UPDATE no longer sets it manually.

Gates: cargo check + clippy clean (buzz-db, buzz-relay, buzz-cli,
desktop). 504 unit tests pass across all crates. Migration count
test updated 45 -> 46. Pre-existing git-sign-nostr test failure
unrelated.
The BEFORE UPDATE trigger bumped `revision` unconditionally, so an
idempotent restate (a client retry, or a PATCH setting status to the
status it already holds) advanced the counter without changing the
row. That invalidates every other client's `expected_revision` for a
write that changed nothing, manufacturing spurious 409s and
contradicting the existing task-event logic, which already suppresses
same-value events.

The trigger now normalises the two derived columns to their OLD values
and bumps only when the whole row `IS DISTINCT FROM` OLD. Comparing the
entire row instead of an enumerated column list means a column added to
`tasks` later is guarded automatically.

Also stop counting `expected_revision` in `TaskPatch::is_empty`. It is
a precondition on a change, not a change, so a guard-only PATCH body
was passing the relay's "patch must change at least one field" check
and performing a no-op write.

Adds the two Postgres regression tests the feature shipped without:
revision advances on real change / holds on a semantic no-op, and a
stale `expected_revision` is rejected while changing nothing. Both were
verified to fail against the previous trigger.

Signed-off-by: Michael Feth <michael@jira-flow.com>
… (info lint)

Signed-off-by: Michael Fethe <mfethe1@gmail.com>
The r89 handoff left the mandatory live-PostgreSQL two-writer race test
unimplemented; its stale-revision test only exercised the sequential
case. This adds the interleaving the guard exists for: writer A holds
the FOR UPDATE row lock, writer B's guarded update_task (second pool)
blocks on that lock, A commits revision+1, and B must then be rejected
with StaleRevision naming the row's actual revision.

Also fixes thread_task_chip_test.dart, which constructed Task without
the required revision field HW-017 added — a real analyze error the
prior verification missed (8 issues vs baseline).

Adds a tiny migration-runner example (hw017_migrate) so the ignored
Postgres tests can run against a fresh database without the Docker
harness.

Verification (all under set -o pipefail, fresh DB migrated to 0047):
- cargo test -p buzz-db --lib task -- --ignored : 8/8 pass incl. the
  new race test
- cargo test -p buzz-db --lib : 127 pass
- cargo test -p buzz-relay tasks : 10 pass, 0 fail
- cargo clippy -p buzz-db --all-targets -- -D warnings : clean
- cargo fmt -p buzz-db --check : clean
- cd desktop && pnpm typecheck : rc=0
- desktop task tests (repo loader): 25/25 pass
- mobile: 94 tests pass across 7 task-lane files
- flutter analyze : 7 issues == run-88 measured baseline; the one
  error (missing revision) is gone

Signed-off-by: Michael Fethe <michael@jira-flow.com>
The final rewrite to compile-time CARGO_MANIFEST_DIR resolution passed
&PathBuf to Migrator::new, which takes impl MigrationSource on &Path.
Every gate that ran before the commit exercised --lib, which never
builds examples, so the break shipped in c352474. Caught by ad-hoc
post-commit verification (cargo build --example), fixed with .as_path().

Verified: cargo build --example hw017_migrate OK; fresh DB migrates to
0047 (MIGRATED_TO=47); clippy -p buzz-db --all-targets -D warnings
clean (examples now included); fmt --check clean.

Signed-off-by: Michael Fethe <michael@jira-flow.com>
@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 7de27023-0019-410b-bf5e-db710547d1b7

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mfethe1

mfethe1 commented Sep 11, 2026

Copy link
Copy Markdown
Owner Author

Independent validation — Loop 4 run 102 (re-ran every gate, did not inherit the handoff)

Validated feat/HW-017@0d778d686d against product/main@adeec6eb5c
(merge-base --is-ancestor true, so the tested tree is the combined tree).
Feature diff is a focused 14 files / +541 -6.

Integrity — no conflict markers, git diff --check rc=0, duplicate migration-prefix scan
empty (0047 unique), no AI attribution trailers.

Code — buzz-db 127P/0F, buzz-core 262P/0F, buzz-acp 968P/0F, buzz-relay 1061P/6F;
desktop typecheck + all 5 check:* guards rc=0; flutter analyze 7 = baseline 7 with
identical signatures; full mobile suite 2249 tracked / 15 failures.

Baseline diff (freshly measured on untouched product/main, not inherited)

  • relay: comm -23 empty, comm -13 empty. All 6 failures are api::media::tests::*, and this
    branch touches zero media files (diff on those paths = 0 lines).
  • mobile: comm -23 empty, comm -13 empty.
    zero regressions attributable to HW-017.

The guard itself is actually proven. The CAS tests are #[ignore = "requires Postgres"], so
a --lib run never exercises them. Against a real Postgres (migrations applied, MIGRATED_TO=47):

task::postgres_tests ... 8 passed; 0 failed
  a_grounded_writer_loses_against_an_interleaved_commit ... ok
  a_stale_expected_revision_is_rejected_and_changes_nothing ... ok
  revision_advances_on_real_change_and_holds_on_a_semantic_noop ... ok

Relay api::tasks 10/10 green.

The trigger design holds up on review: normalising revision/updated_at to their OLD values
before a whole-row IS DISTINCT FROM means an idempotent restate cannot manufacture a spurious
409, and comparing the whole row rather than an enumerated column list keeps future columns
covered automatically. Omitting expected_revision skips the guard, so pre-HW-017 clients are
unaffected. Excluding it from is_empty() is also right — it is a precondition, not a change.

Disclosed gaps

  1. DCO6ec3922a08 has no Signed-off-by; the other 4 commits do.
  2. Visual evidence NOT produced — data-layer change with no rendered-state delta. Stating
    this explicitly rather than implying it was verified.

Not self-merging

This adds a migration and a schema column, which is the schema class I am not permitted to
self-merge. Handing to @mfethe1 for review-merge.

No overlapping upstream work: the open upstream PRs touching migrations/ all take the 0045
prefix (block#7565, block#7543, block#7506, block#7492, block#7483) — none touch buzz-db/src/task.rs or collide with 0047.

Detail: digests/2026-09-11-validation-run102.md.

@mfethe1

mfethe1 commented Sep 11, 2026

Copy link
Copy Markdown
Owner Author

⚠️ Migration version collision with #15 (detected by CI-independent audit)

This PR adds migrations/0047_task_optimistic_concurrency.sql.
#15 (AGENT-HOMES-001 PR-3) adds migrations/0047_agent_machine_homes.sql — the same version number.

Both branches also contain the same hardcoded count assertion in crates/buzz-db/src/runtime/migration.rs (embedded_migrator_contains_consolidated_initial_schema, ~L707):

assert_eq!(migrations.len(), 46);

product/main currently ends at 0046_task_system.sql, so each PR is individually consistent. Whichever merges second will break trunk in two ways:

  1. Two distinct migrations at version 47 in migrations/sqlx::migrate! rejects duplicate versions.
  2. migrations.len() becomes 47, failing the assert_eq!(..., 46) in whichever branch did not update it.

Resolution needed (owner call, not done here): one lane renumbers to 0048_* and bumps the assertion to 47. Ordering is the owners' choice; agent-homes has no dependency on the HW-017 guard, and vice versa.

Filed as coordination evidence only — no changes pushed to either branch (one-owner-per-task).

@mfethe1

mfethe1 commented Sep 11, 2026

Copy link
Copy Markdown
Owner Author

✅ Verified: the two 0047s are compatible once renumbered — proof attached

Following up on the version collision reported above. I applied the full chain against a real PostgreSQL instance with HW-017 renumbered to 0048, to establish that the collision is purely a numbering problem and not a semantic conflict.

Setup: clean cluster, product/main migrations + 0047_agent_machine_homes.sql (#15) + 0048_task_optimistic_concurrency.sql (#31, renumbered), applied in order, each in a single transaction with ON_ERROR_STOP=1.

Result: CHAIN_FAIL=0 — all 47 migrations applied clean. Both feature schemas are present and functional:

Check Result
users.machine_id / machine_label / machine_runtime present
idx_users_one_home_per_machine present
tasks.revision (integer) present
trg_tasks_revision trigger present

Integration behaviour verified (a task assigned to a machine-homed agent):

  • Task inserted with assignee_pubkey → machine-homed user: OK, revision starts at 0
  • UPDATE → revision auto-bumps to 1 via the trigger: OK
  • Join tasks → users resolves assignee to machine + runtime: deploy relay | in_progress | rev 1 | Rosie | openclaw
  • Stale CAS WHERE revision=0 after the bump → UPDATE 0 (conflict correctly rejected)
  • Correct CAS WHERE revision=1UPDATE 1 (revision → 2)
  • Fleet rollup: airy/hermes 0 tasks, rosie/openclaw 1 task, winnie/hermes 0 tasks

The two migrations touch disjoint tables (users vs tasks) — there is no ordering dependency in either direction, so whichever lane renumbers, the result is the same.

Remaining action is still just the rename + bumping assert_eq!(migrations.len(), 46)47 in the second-to-merge branch. No changes pushed to either branch.

Note for fixture authors: tasks uses created_by_pubkey (not creator_pubkey), and chk_tasks_done_at_matches_status requires done_at to be set whenever status='done'.

Trunk landed 0047_agent_machine_homes while this branch carried its own
0047_task_optimistic_concurrency. PR #30 owns 0048 and PR #18 owns 0049,
so this takes 0050 (matching the numbering PR #27 already expects).
sqlx::migrate! globs migrations/ by filename, so the rename is the whole
renumber. Inventory now pins 47 migrations with 50 last.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant