fix(deploy): serialize secrets state updates for concurrent deploys#1806
fix(deploy): serialize secrets state updates for concurrent deploys#1806dqn wants to merge 6 commits into
Conversation
Concurrent deploys to the same workspace and application could interleave remote secret writes with local hash-state saves, leaving the state file claiming a value the platform no longer holds; a later deploy whose desired value matched the stale hash silently skipped the update. Secret and auth-connection applies now hold a target-scoped lock (a lock directory next to the scoped state file, with dead-owner detection) across each remote-update/state-save sequence, and state saves go through a temp file and rename so concurrent readers never observe torn JSON.
Reuse p-limit for the in-process lock queue, clean up the lock directory when the owner record cannot be written, and re-verify staleness after the steal rename so a freshly re-acquired lock is handed back instead of removed.
Fall through to the deadline check when a stale lock cannot be stolen so a read-only lock directory times out instead of busy-looping, and treat a lock older than the maximum hold age as stale so a crashed deploy whose pid was reused by a long-lived process cannot block deploys indefinitely.
Judge lock staleness by a single lease rule: holders refresh the lock directory mtime on a heartbeat, and only a lock whose mtime exceeds the lease is stolen. This keeps long-running live deploys protected, makes a crashed deploy self-heal within the lease instead of blocking on a reused pid, and release now verifies an ownership token so an expired holder can never remove the current holder's lock.
🦋 Changeset detectedLatest commit: 0280a55 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
Code Metrics Report (packages/sdk)
Details | | main (4675aa4) | #1806 (6067819) | +/- |
|--------------------|----------------|-----------------|-------|
+ | Coverage | 74.5% | 74.5% | +0.0% |
| Files | 457 | 457 | 0 |
| Lines | 16953 | 17027 | +74 |
+ | Covered | 12636 | 12695 | +59 |
+ | Code to Test Ratio | 1:0.4 | 1:0.4 | +0.0 |
| Code | 113445 | 113831 | +386 |
+ | Test | 52708 | 52946 | +238 |Code coverage of files in pull request scope (95.2% → 91.2%)
SDK Configure Bundle Size
Runtime Performance
Type Performance (instantiations)
Reported by octocov |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📖 Docs Quality & Consistency Check
📝 Reader-Perspective Issues
| File | Issue | Suggested Fix |
|---|---|---|
.changeset/serialize-secrets-state-writes.md |
Mentions "local secrets hash state" — an internal implementation detail users don't need to understand | Rephrase to: "Fix concurrent deploys to the same workspace and application potentially skipping secret updates. Secret and auth-connection updates are now serialized per workspace/application, preventing conflicts." |
packages/sdk/src/cli/commands/deploy/secrets-state.ts:199-201 |
Error message exposes internal lock file path (.tailor-sdk/secrets-state/<hash>.json.lock) and asks users to "remove the lock directory" |
Simplify to: "Another deploy to the same workspace and application is in progress. Wait for it to complete or check for stalled processes if no deploy is running." (Don't expose the lock path unless in verbose/debug mode) |
⚠️ Consistency Issues
| File | Issue | Suggested Fix |
|---|---|---|
packages/sdk/docs/cli/application.md |
Deploy command documentation doesn't mention the new concurrent-deploy serialization behavior or the 5-minute timeout | Add a note in the deploy section explaining that concurrent deploys to the same workspace and application are automatically serialized to prevent state corruption, and that a deploy will timeout after 5 minutes if unable to acquire the necessary lock |
Details
Reader-Perspective Issues
1. Changeset description (.changeset/serialize-secrets-state-writes.md)
The changeset mentions "local secrets hash state," which is an SDK internal implementation detail:
Fix concurrent deploys to the same workspace and application corrupting the local secrets hash state.Why this is internal-only: Users don't interact with or configure "hash state" — it's a behind-the-scenes optimization the SDK uses to skip redundant secret updates. From a user's perspective, the problem is simpler: concurrent deploys could cause secrets to not be updated correctly.
Suggested fix: Rephrase to describe the observable user-facing problem and fix without leaking the hash-tracking mechanism:
Fix concurrent deploys to the same workspace and application potentially skipping secret updates. Secret and auth-connection updates are now serialized per workspace/application, preventing conflicts.2. Error message exposes internal lock file structure (packages/sdk/src/cli/commands/deploy/secrets-state.ts:199-201)
The timeout error message includes the full lock file path:
throw new Error(
`Timed out waiting for the secrets state lock at "${lockPath}". ` +
"Another deploy to the same workspace and application may still be running; " +
"remove the lock directory if no such deploy exists.",
);Where lockPath resolves to something like .tailor-sdk/secrets-state/abc123def456.json.lock.
Why this is internal-only:
- The
.tailor-sdkdirectory is SDK implementation storage - The hash-based filename (
abc123def456.json.lock) is an internal naming scheme - Asking users to "remove the lock directory" requires them to understand SDK internals and manually delete files
Suggested fix: Simplify the error message to describe the situation without exposing the internal file structure:
throw new Error(
"Timed out waiting for another deploy to the same workspace and application to complete. " +
"If no deploy is running, a previous deploy may have been interrupted. " +
"Try running the deploy again or contact support if the issue persists.",
);Optionally, show the full path only in verbose/debug mode for troubleshooting.
Consistency Issue
3. Deploy command documentation doesn't mention serialization behavior
File: packages/sdk/docs/cli/application.md (deploy command section)
Issue: The PR introduces a significant behavioral change: concurrent deploys to the same workspace and application are now automatically serialized (queued), and a deploy will timeout after 5 minutes if it cannot acquire a lock. However, the deploy command documentation doesn't mention this.
Impact on users:
- Users running concurrent deploys might see unexpected waiting/blocking behavior
- Users might encounter the 5-minute timeout error without context
- Users don't know that multiple deploys to different workspaces or applications can still run concurrently
Suggested fix: Add a new section to packages/sdk/docs/cli/application.md after line 147 (after "Migration Handling"):
**Concurrent Deploy Handling:**
When multiple `deploy` commands target the same workspace and application simultaneously, they are automatically serialized to prevent conflicts during secret and auth-connection updates. The first deploy to acquire the lock proceeds immediately, while subsequent deploys wait until the lock is released.
If a deploy cannot acquire the lock within 5 minutes, it will timeout with an error. This typically indicates either:
- Another deploy is still in progress (normal)
- A previous deploy was interrupted and left stale state (check for stray processes)
Deploys to different workspaces or different applications within the same workspace can run concurrently without waiting.This explains the new behavior in user-facing terms without exposing internal implementation details like "hash state", "lock files", or .tailor-sdk directory structure.
Recommended Actions
- Changeset: Rephrase the description to avoid mentioning "hash state" (user docs are published in the CHANGELOG)
- Error message: Simplify to not expose the lock file path or internal directory structure
- Deploy docs: Add a "Concurrent Deploy Handling" section explaining the serialization behavior and 5-minute timeout
Re-run this check by adding the
docs-checklabel to the PR.
| const dir = path.dirname(filePath); | ||
| mkdirSync(dir, { recursive: true }); | ||
| // Write via a temp file and rename so concurrent readers never see torn JSON. | ||
| const tempPath = `${filePath}.tmp-${process.pid}`; |
There was a problem hiding this comment.
nits: the temp file name is based on process.pid.
Harmless since writes are serialized by the lock, but acquireFileLock uses randomUUID() — might be worth aligning for consistency.
| * Run a remote-update/state-save sequence exclusively for one target's secrets state. | ||
| * | ||
| * Serializes concurrent deploys to the same workspace and application (across | ||
| * processes sharing the same output directory) so the persisted hash state |
There was a problem hiding this comment.
q: what's the concrete scenario for concurrent deploys sharing the same output directory?
I first guessed multi-config (--config a.ts,b.ts), but forEachDeployment in apply-phases.ts runs deployments sequentially (for-of, not Promise.all), so that doesn't seem to trigger it.
Is this about double-invocation of the CLI, or duplicate/overlapping CI runs?
2 participants
Summary
Two concurrent deploys to the same workspace and application could interleave
remote secret writes with local hash-state saves, leaving the state claiming a
value the platform no longer holds — a later deploy whose desired value matched
the stale hash then silently skipped its update.
Behavior
deploy(andremove) now hold a per-target lock around each Secret Manager/ auth-connection remote-update + state-save sequence, so deploys sharing a
checkout queue instead of interleaving.
lease is treated as a crashed deploy and reclaimed automatically.
naming the lock directory.
concurrent readers never see torn JSON.
Notes
checkout's
.tailor-sdk. Drift caused by deploys from other checkouts orconsole-side secret edits is a pre-existing gap, deferred to a follow-up
based on platform
updateTimeevidence.mid-apply (e.g. machine suspend) plus precise multi-process timing; the
outcome then degrades to the pre-fix behavior for that one collision.