Conversation
c1304bd to
d880cda
Compare
437a8fb to
7d8558f
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed 7d8558f17d347646f1f3aef18477157c6f28cd0e. This is the connected user's own PR, so I am submitting COMMENT. One P2 concurrency finding should be addressed before merging; see inline. Upgrades and migrations are outside this review's acceptance scope.
The product direction is useful: a fresh /work start can establish its Session without a throwaway chat turn; the composer/palette gets priority over a large board; executor display labels no longer masquerade as protocol identifiers; rejected work stays attached to the actual Session and tells the user no model/tool ran. Quarantining a corrupt derived CSL only when a canonical journal continuation exists is also the right authority boundary.
I inspected the workspace activity claim lifecycle and transfer checks, retained writer/reservation identities, admission reserve/renew/release and materialized capacity accounting, recovery publication and effect-frontier checks, CSL repair, harness worktree isolation, and the TUI/SDK changes. In particular, the recovery publisher rechecks its canonical anchors transactionally, does not publish caller-authored manifests, and blocks unresolved invocation effects. Its user-facing assessment correctly says workspace/run content is not captured and restore is unavailable. This is a verified Work/conversation boundary, not yet portable workspace recovery or a complete restore experience; keep that distinction in the product wording.
The remaining problem is that the new activity fence is per writer lease, while its clearing API is used as per-turn cleanup. Different reservations under a retained writer share the same cleanup identity, so the advertised stale-cleanup protection does not hold across those turns.
Validation independently executed: 47 Python schema tests passed; git diff --check against the PR base passed. I also extracted the production cleanup UPDATE and checked its predicate in an in-memory SQLite fixture: an old cleanup with the retained writer's lease/epoch clears the row representing a newer reservation under that same writer. This confirms the predicate collision, not an end-to-end Rust/MatrixOne concurrency reproduction. cargo/rustc and local SDK dependencies are unavailable, so I did not run Rust, live DB, SDK, model, or PTY tests. At inspection, GitHub Static Checks passed and Test Suite was still running. Head rechecked unchanged before submission.
| active_execution_expires_at_ms = NULL, updated_at = NOW(6) | ||
| WHERE isolation_domain = ? AND owner_user_id = ? | ||
| AND session_id = ? AND branch_id = ? | ||
| AND active_execution_id = ? AND active_execution_generation = ?", |
There was a problem hiding this comment.
[P2] Fence activity cleanup by the turn reservation, not only the writer lease
The pair in this predicate is not unique to an execution: reserve_turn generates a new reservation_id but copies lease_id and writer_epoch from the retained writer, and installs those latter two values as the activity identity. commit_turn clears the reservation while retaining that writer. Therefore R1 and a later R2 under the same writer L/epoch E both use (L, E) here.
A delayed R1 clear_execution_workspace_activity after R2 reserves will erase R2's live marker. The public cleanup method locks the head but never checks its active reservation, and runtime settlement invokes this cleanup separately after commit_turn and WAL retirement (including the AlreadyApplied result). At minimum, R2's subsequent claim verification can now reject a valid admission; in the pre-Run/slot window this also removes the activity evidence intended to prevent another Session taking the checkout. This conditional UPDATE is safe against a new writer, but not a new turn under the same writer.
Use reservation_id (or another per-execution generation) consistently for activation, verification, renewal, and cleanup, or explicitly reject stale per-turn cleanup against the current reservation. Add a DB regression that reserves/commits R1, reserves R2 using the same live writer, invokes R1 cleanup, and proves R2 still verifies and another Session still cannot take the claim. The current activity test covers distinct controllers but not this retained-writer case.
Problem
A durable Work could still feel unusable at the boundaries: a fresh TUI conversation could not start
/work startwithout a throwaway chat turn, a full Work board could cover the command palette after/work, and Web could reject valid executor labels such asMacBook Proas if they were protocol identities. Multi-session recovery also had a workspace-fence race: delayed cleanup from one turn could clear the activity marker for a later turn under the same retained writer, and that later turn could not renew because the writer still carried its original acquisition cursor.Change
/work start <goal>to bootstrap the durable Session it needs, while reusing an existing Session without extra transport or auth work./…command is immediately discoverable.Validation
make format-checkmake lintmake checkmake test-offline(baseline Rust workspace, runtime e2e, SDK, Web, and build gates passed before the final TUI/SDK delta)make test-contract(16 contract tests passed)npm run typecheck, work-contract tests (65 passed), andnpm run buildkind/display_name/root/source/authorityshape; Edge requests userootplussource.kind=edge_pathand never send event-onlycwdorfallback_policy.deny_unknown_fieldstests, plus SDK consumer typecheck assertions that reject event-only request fields.