[NancyKwongPerplexity]

Summary

Replaying a native session create (same method, path, body and Idempotency-Key within the key's 24 hours) after the session was revoked now returns the existing conflict code instead of 201 with a token for a dead session. The response is 409 with retryable: false and the detail {"key": "reason", "value": "session_ended"}.

At the moment the replay path refreshes the session's attach token without checking its status. After DELETE /v1/sandboxes/{id}/sessions/{session_id}, a replay of the original command create returns 201 with state: revoked and a fresh token that cannot be used.

The refusal also has to outlive the session row. Data-plane session retention deletes a revoked row one hour after the revoke, but the key lives 24 hours and the create operation 7 days. Without handling that, a replay after the sweep finds the operation but no row and answers a retryable 500, so a client keeps retrying for most of the key's lifetime.

What changes:

• The replay's token refresh (refresh_attach_token_tx) only matches a row with revocation_status = 'active' and expires_at in the future. It reads revocation_status rather than state, like the data plane's readers of these rows: nothing writes closing, closed or failed yet, so a separate list of ended states would be a rule nothing else applies.
• When that refresh matches nothing, the store returns SessionEnded. This covers a revoked or revoking session, an expired one, and one whose row retention has deleted. A missing row is safe to read as ended: the create writes the session, the operation and the key record in one transaction under the key's advisory lock, so a replay never sees a partial create. An expired row can only outlive its key when the API host's clock runs behind the database's, because the session's expiry comes from the host and the key's from the database. Treating the missing row as a fresh create would run the command a second time.
• An operation with no target session, or a refreshed row that the org- and sandbox-scoped read cannot find, still returns MissingIdempotencyTarget.
• The public API maps SessionEnded to conflict through a new conflict_with_reason constructor, with two new constants in pplx-space-public-api: CONFLICT_REASON_DETAIL_KEY (reason) and CONFLICT_REASON_SESSION_ENDED (session_ended). No new error code is added.
• The refresh's row lock orders a replay against a concurrent revoke. A replay waiting on the lock re-checks the guard after the revoke commits and is refused.
• OpenAPI: createCommand's 409 examples list conflict as well as idempotency_conflict. The 409 descriptions of createCommand and the terminal, process, file-watch, port and protocol session creates mention the replay of a revoked session. The intro qualifies "returns the original result", and PublicErrorEnvelope.details documents the reason detail.
• docs/api/index.md gains one sentence on this 409 in "Retry safely". It says "revoked" because revocation is the only path that ends a session today.

Nothing else changes. Creating a session, a replay of a live session (including rotating its token), validation and revocation all behave as before. This is the status guard from the larger proposal #809, split out so it does not depend on the credential-overlap decision deferred in the review of #79 and the review of #91. native_contract.command_replay_credentials stays ignored.

Validation

At this head (7769d52, on main at 4324915):

• cargo fmt --all --check: pass.
• cargo clippy -p pplx-space-api -p pplx-space-public-api --all-features --all-targets -- -D warnings: pass.
• cargo nextest run --config 'profile.test.package.pplx-space-api.opt-level=0' --all-features -p pplx-space-api -p pplx-space-public-api -E 'not group(privileged)' with nextest's Postgres and Redis (SPACE_CI_REQUIRE_TEST_DEPS=1, so a missing dependency fails instead of skipping): 1952 passed, 1 skipped, no dependency skips.

Store tests, against a real Postgres:

• replay_of_a_revoked_session_is_refused_without_a_token: revoke, then replay, gives SessionEnded, and the stored token stays cleared.
• replay_is_refused_once_revocation_starts_while_still_pending: with state left pending, revocation_status set to revoking and then to revoked are each refused, and the stored token is unchanged.
• replay_of_an_expired_session_is_refused: with expires_at in the past, the replay gives SessionEnded, and the stored token is unchanged.
• replay_of_a_revoked_session_is_refused_after_its_row_is_swept: revoke, delete the row as retention does, then replay, gives SessionEnded.

HTTP and contract tests:

• ended_session_replay_is_a_non_retryable_conflict_with_a_reason: the envelope is 409 conflict, retryable: false, with the session_ended detail.
• supported_error_examples_match_envelopes_status_and_retryability now expects conflict and idempotency_conflict among createCommand's 409 examples.

Mutation check, one mutation at a time, restoring the file before the next. Each mutation made the named tests fail:

• revocation_status predicate removed: the revoked test and the pending-state test.
• Predicate loosened to revocation_status <> 'revoked': the pending-state test, on its revoking case.
• Expiry predicate removed from the refresh: the expired test (it gets MissingIdempotencyTarget).
• A missing row answers MissingIdempotencyTarget again: the swept and expired tests.
• SessionEnded mapped to idempotency_conflict: the HTTP test.
• The reason detail's value changed: the HTTP test.
• createCommand's 409 examples reverted to idempotency_conflict only: the examples test.

Kind, on a local cluster set up from docs/installation.md (install-fc-vendor.sh, bootstrap.sh), driven only through the public API. I created two commands with their own keys, revoked both sessions, and waited for the controller's retention job to delete the rows (GET on each session went from 200 revoked to 404):

• Previous head of this PR (d5fefdc): a replay right after the revoke gave 409 with session_revoked. After the sweep, the replay gave 500 {"code":"internal","retryable":true}, and the API logged session idempotency operation has no target session.
• This head (7769d52), deployed over the same database: both swept replays gave 409 {"code":"conflict","retryable":false,"details":[{"key":"reason","value":"session_ended"}]}. A fresh create, revoke and replay gave the same 409. A live create and its replay both gave 201 with the same session id.
• The served /openapi-m1.json lists conflict and idempotency_conflict under createCommand's 409, and the intro carries the qualified sentence.

Typed smoke on that cluster at this head: bash scripts/tests/smoke-kind-typed.sh native ran 26 scenarios, all passed, including native.m1_generated_sdk_conformance. native_contract.command_replay_credentials was reported IGNORED, as on main.

Not validated: an expired session through the API, because it needs the API host's clock to run behind the database's; the store test covers it. docs/book/test.py was not run, because it needs a built guide. The docs change is one sentence in an existing paragraph.

Severity and risk

Severity: low. A replay currently returns 201 and a token that cannot attach, so a client wastes a round trip and sees a confusing failure later. After retention sweeps the row, it returns a retryable 500 instead. No access is granted: in an earlier Kind run on main, attaching with the returned token failed.

Risk: a client that replays a revoked session's create and treats any 201 as success now gets a 409. A replay after the sweep or after expiry previously returned a 500, so no client could have relied on it succeeding. The guard runs in the existing replay UPDATE, and the replay of a live session is unchanged. Roll back by reverting this PR. No migration is involved.

Not covered

Follow-ups:

• When something starts writing closing, closed or failed, give SessionState an ended (or attachable) predicate and apply it here, in the data plane and in retention, which today omits closing. Until then, this guard follows revocation_status.

AI assistance: Claude Code drafted the code, tests, and this description; the author reviewed them.

[ac-pplx]

Thanks, the guard is in the right place and nothing here regresses main. The items below are small. Most of them concern the public contract this PR introduces (the new 409 and its reason values), which is harder to change after release.

Before merge

1. The 409 holds for about an hour after a revoke, and then the replay returns a retryable 500 again. crates/libs/api/src/public_sessions.rs:115-132

   • Cause. The new answer depends on the session row still existing:
     • The data-plane session retention job deletes rows with state IN ('closed', 'revoked', 'failed') once updated_at is older than its 1-hour grace (crates/libs/cluster/controller/server/src/data_plane_session_retention.rs:9,70-77).
     • The idempotency record lives 24 hours (public_sessions.rs:762) and the create operation 7 days (public_sessions.rs:709).
     • After the sweep, the operation still yields the session id, the refresh matches nothing, fetch_session_tx returns None, and session_expired_tx returns false. The result is MissingIdempotencyTarget, which falls into the other => arm, logs at error, and answers internal(): a 500 with retryable: true (crates/libs/api/src/routes/public/error.rs:75,397-400).
   • Failure scenario. Create a command with key K, DELETE its session, wait a little over an hour, then replay with K. The client gets a retryable 500 instead of the documented non-retryable 409, and keeps retrying. That covers most of the 24-hour window that the new sentence in docs/api/index.md:39 describes.
   • Suggested fix. Return the same 409 when the create operation exists but its session row does not. For example, make the expiry probe a present / expired / absent lookup and map "absent" to the 409. Keep MissingIdempotencyTarget for an operation that has no target session, which is genuine inconsistency. Add a store test that deletes the row after a revoke and expects the 409. Also update the comment at public_sessions.rs:129-130 ("still exists after a revoke or close").
   • Why an absent row is safe to read as "ended". By this point, the only realistic way to get here is that retention swept an ended session:
     • The create writes the session, the operation and the key record in one transaction under the key's advisory lock, so a replay never sees a partial create.
     • The route has already rejected a missing sandbox, and the fingerprint pins the sandbox id.
     • An expired session is swept only an hour after it expires, by which time its key has expired too.
   • Alternatives I would avoid.
     • Lengthening the retention grace to cover the key's 24 hours keeps ended rows much longer, and makes this contract depend on a constant in another component staying above the API's key lifetime.
     • Treating the swept row as a fresh create would run the command a second time.

2. The generated OpenAPI contract still describes the old behaviour. crates/libs/api/src/routes/public_docs/error_examples.rs:37

   • What is stale:
     • The examples table lists only idempotency_conflict for createCommand's 409, and crates/libs/api/src/routes/public_docs/tests.rs:1174 pins that list.
     • createCommand's 409 is described as "Idempotency conflict" (crates/libs/api/src/routes/public_docs/path_contract.rs:122), and so are the terminal, process, file-watch, port and protocol session creates (crates/libs/api/src/routes/public_docs.rs:136,146,156,166,176).
     • The spec intro still says "Within 24 hours, this returns the original result" (public_docs.rs:82).
     • The PublicErrorEnvelope.details doc lists only the not_found detail (crates/libs/public/api/src/lib.rs:2008-2012).
   • Failure scenario. A client written against the published spec reads a 409 from createCommand as "you changed the body", when the body is identical and the session is gone.
   • Suggested fix. Add Conflict to the createCommand 409 examples and update the test. Widen the six 409 descriptions. Qualify the "original result" sentence. Mention the reason detail in the details doc.
   • Related wording in docs/api/index.md:39. "A command whose session was revoked or has ended" reads as "the command finished". But nothing writes closing, closed or failed today: only the create INSERT (pending) and revoke (revoked) write state, and a command finishing changes neither state nor revocation_status. So the sentence covers only revoked sessions in practice. Saying "revoked" there is accurate until something writes an ended state.

3. No test fails if AND revocation_status = 'active' is removed. crates/libs/api/src/public_sessions/tests.rs:1182-1240

   • Why. The guard has two predicates, and the tests only ever vary the second. The revoked test goes through revoke_session, which sets both revocation_status and state (public_sessions.rs:338,341), and the ended-state test sets only state. So the state predicate alone satisfies all three store tests, and deleting the revocation_status predicate (public_sessions.rs:619) is not caught. The schema also allows revocation_status = 'revoking' (db/postgres/20261002070625_baseline.up.sql:777), which no test uses.
   • Failure scenario. A later cleanup drops the revocation predicate as redundant with the state list, and the guard quietly narrows to the state check for any future path that changes only revocation_status, without any test failing.
   • Suggested fix. Add cases with state = 'pending' and revocation_status set to revoking and to revoked, asserting the same outcome as the existing cases.

4. Settle the reason values before they ship (your call). crates/libs/public/api/src/lib.rs:2024-2034

   • session_revoked is broader than its name. It is also the answer for closing, closed and failed sessions (public_sessions.rs:131-132), and it would cover the swept rows from item 1. Only revocation reaches it today, but the value is permanent once published.
   • session_expired is reachable only under clock skew:
     • The session's expires_at is the API host's now_utc() plus 24 hours, read after the transaction began (public_sessions.rs:146-149).
     • The record's expires_at is the database NOW(), the transaction start, plus 24 hours (public_sessions.rs:762).
     • Nothing else writes data_plane_sessions.expires_at.
     • So with aligned clocks the key expires first, and the replay creates a new session (pinned by tests.rs:442-469).
     • The description's note that an API-level test "would need the session's 24-hour TTL to elapse" does not hold for the same reason.
   • Suggested fix. Either use one neutral value (for example session_ended) for revoked, ended, swept and expired sessions, and drop CONFLICT_REASON_SESSION_EXPIRED before it ships. Or keep both values, but say in a comment that the expired branch guards against clock skew, so it is not later removed as dead code. Either choice changes the follow-up about documenting session_expired.

Follow-ups

• The state list is a separate "ended" rule that nothing else enforces. crates/libs/api/src/public_sessions.rs:620
  • Why. The list closing, closed, revoked, failed is a new definition of an ended session. Retention uses a different list that omits closing (data_plane_session_retention.rs:76), SessionState has no predicate for it (crates/libs/public/api/src/lib.rs:737-746), and the other readers of these rows decide on revocation_status rather than state. Nothing writes these states today, so the description's statement about closing sessions describes intent rather than current behaviour.
  • Failure scenario. Not reachable today, because nothing writes these states. Once something writes these states, the replay path, the data plane and retention would each apply a different rule to the same session.
  • Suggested fix. Either add an ended (or attachable) predicate to SessionState and bind it in both SQL sites, or rely on revocation_status alone until a state writer exists.

Nits

• Duplicated detail-building. crates/libs/api/src/routes/public/error.rs:376-395. The two new arms repeat the same conflict-plus-detail block, so a third reason would copy it again. A conflict_with_reason constructor, like the existing path_not_found (error.rs:177-184), would keep it in one place.
• The refresh UPDATE has no expiry predicate. crates/libs/api/src/public_sessions.rs:612-621. It lacks the (expires_at IS NULL OR expires_at > NOW()) predicate that revoke, process binding and reconnect apply (public_sessions.rs:349, crates/libs/internal/api/server/src/data_plane_sessions/process_binding.rs:42,121), so an expired live row is updated and then rolled back when the error drops the transaction. That is correct today, and the expired test pins the rollback. Adding the predicate would make the guard self-contained, so it does not depend on the error path never committing.

[ac-pplx]

The problem. A session or command create takes an Idempotency-Key, and a same-key retry within 24 hours returns the original session with a newly rotated attach token. On main, that retry does not check the session first. After the session is revoked, the retry reports success with a token that cannot be used. Once retention deletes the revoked row, about an hour later, the retry fails with a retryable 500.

Example.

POST /v1/sandboxes/{id}/commands   Idempotency-Key: K   -> 201, session S
DELETE /v1/sandboxes/{id}/sessions/S
POST ... (same body, key K)
   main:     201, state: revoked, unusable token; after retention: 500, retryable: true
   this PR:  409 conflict, retryable: false, details [{"key":"reason","value":"session_ended"}], before and after retention

Verdict: approved. Every item from the last review is fixed, and nothing here blocks. Please refresh the PR description before merging.

Status of the previous items

1. A swept row gave a retryable 500: fixed. A refresh that matches no row, including an absent one, is SessionEnded (crates/libs/api/src/public_sessions.rs:118-120), and tests.rs:1288 pins it.
2. The OpenAPI contract was stale: fixed. This covers the examples and their test, the six 409 descriptions, the intro, the details doc and docs/api/index.md:39.
3. The revocation predicate was not pinned: fixed (crates/libs/api/src/public_sessions/tests.rs:1223).
4. The reason values: fixed, with one session_ended (crates/libs/public/api/src/lib.rs:2032).
5. The separate state list: fixed. The guard is revocation_status plus expiry (public_sessions.rs:606-610).
6. Both nits: fixed (crates/libs/api/src/routes/public/error.rs:187, public_sessions.rs:610).

Before merge

• The PR description still describes the previous design.
  • What is stale. It names session_revoked and session_expired, the closing/closed/revoked/failed state list, and SessionRevoked/SessionExpired. It says "any other missing target still returns MissingIdempotencyTarget". It lists test names that no longer exist and reports validation at d5fefdc. It also has a follow-up to document session_expired, and it does not mention the swept-row test or the OpenAPI changes.
  • Why it matters. A reader of the merged PR would expect session_revoked, and a 500 after the sweep. The rule is in CONTRIBUTING.md:111-113 and :119-120.
  • Suggested fix. Describe one session_ended reason, from a refresh guarded by revocation_status = 'active' and expiry, where zero rows (including an absent row) is the 409. List the current tests and validation head, and drop the session_expired follow-up. Restate the Kind evidence with the new value, or label it as from the earlier commit.
  • Optional. The squash commit is built from the commit messages, and the first two still name the removed values, so you may want to reword or squash them.

Nits

• The comment's explanation of a missing row is incomplete. crates/libs/api/src/public_sessions.rs:113-117 says a missing row "was swept by retention after it ended". A sandbox deleted between the route's owner check and this transaction also removes it, through the sessions' cascade (db/postgres/20261002070625_baseline.up.sql:785). The 409 is still the right answer there. Mentioning both causes would keep the comment accurate.

[NancyKwongPerplexity]

thanks for the review. all addressed in 92b523a, 0295930, 7769d52:

1. swept rows: the token refresh now matches only an active, unexpired row, and no match (revoked, revoking, expired or swept) gives the same non-retryable 409. an operation with no target session still returns MissingIdempotencyTarget. since there's one reason value now (see 4), i dropped the probe instead of extending it. new store test replay_of_a_revoked_session_is_refused_after_its_row_is_swept. on kind, i revoked two sessions and waited out retention: the old head gave a retryable 500 after the sweep, the new head gives 409 session_ended
2. openapi: conflict is in createCommand's 409 examples (and the examples test), the six session-create 409 descriptions mention replaying a revoked session, "returns the original result" is qualified, the details doc mentions the reason, and docs/api/index.md says "revoked"
3. added revoking and revoked cases with state left pending, so dropping the revocation predicate now fails a test
4. went with one value, session_ended, and dropped session_revoked / session_expired before they ship. a comment notes the expired case only happens under clock skew

follow-up: dropped the separate state list, so the guard relies on revocation_status like the data plane. nothing writes closing/closed/failed today. an attachable/ended predicate on SessionState is listed under not covered

nits: added conflict_with_reason, and the refresh UPDATE has the expiry predicate now, so an expired row is never touched

[ac-pplx]

The problem. A session or command create takes an Idempotency-Key, and a same-key retry within 24 hours returns the original session with a newly rotated attach token. On main, the retry does not check the session first. After a revoke, it reports success with a token that cannot be used. After retention deletes the revoked row, about an hour later, it fails with a retryable 500.

Example.

POST /v1/sandboxes/{id}/commands   Idempotency-Key: K   -> 201, session S
DELETE /v1/sandboxes/{id}/sessions/S
POST ... (same body, key K)
   main:     201, state: revoked, unusable token; after retention: 500, retryable: true
   this PR:  409 conflict, retryable: false, reason session_ended, before and after retention

Verdict: approved. The rewritten description and the test cleanup look good. Dropping the row count is fine, because SessionEnded is returned before any insert and an error rolls back the transaction. What is left is description polish and one comment.

Status of the previous items

• The stale PR description: fixed.
• The comment naming only retention (crates/libs/api/src/public_sessions.rs:113-117): not changed yet. A sandbox deleted after the route's owner check also removes the row, through the cascade at db/postgres/20261002070625_baseline.up.sql:785. The 409 is still right; mentioning both causes would keep the comment accurate.
• Optional rewording of the first two commit messages: not done, which is fine. They still name session_revoked and session_expired.

Before merge (small)

• Link an issue for the follow-up, and compare the Kind evidence against main.
  • The follow-up. The SessionState ended or attachable predicate, including retention's missing closing (crates/libs/cluster/controller/server/src/data_plane_session_retention.rs:76), is recorded only in "Not covered". CONTRIBUTING.md:116-117 asks for an issue so it is tracked after merge.
  • The Kind "before" bullet. It compares against d5fefdc and shows a session_revoked 409 that never ships. main takes the same path after the sweep (zero-row refresh, no row, MissingIdempotencyTarget, a retryable 500), and right after a revoke it returns 201 with an unusable token. Stating main's behaviour keeps the description to what merging does (CONTRIBUTING.md:111-115).

Nits

• Two sentences in the description misstate expiry.
  • "An expired row can only outlive its key when the API host's clock runs behind the database's" has it backwards (the same wording is in the 92b523a message). It is the key that has to outlive the session's expiry for a replay to see an expired row, and that needs the host clock to run behind. The code comment at public_sessions.rs:116-117 has it right, and could be reused.
  • "Revocation is the only path that ends a session today" leaves out expiry. "The only ended state anything writes today" would be accurate.