Conversation
Stage credentials until PostgreSQL accepts the save, then publish the local file. Serialize participating writers through publication and compensation with a session advisory lock based on relation identity. Require direct or session-pooled connections and coordinated writer upgrades. Cover filesystem failures, unchanged-local retries, and concurrent writes with unit and PostgreSQL integration tests.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4264dc445
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| `, s.fullTableName(s.cfg.AuthTable)) | ||
| result, err = conn.ExecContext(ctx, query, relID, json.RawMessage(previous)) | ||
| } else if previousExists { | ||
| query := fmt.Sprintf("UPDATE %s SET content = $2, updated_at = NOW() WHERE id = $1 AND content = $3", s.fullTableName(s.cfg.AuthTable)) |
There was a problem hiding this comment.
When publishing an existing credential fails, this compensation restores the old content but advances updated_at to NOW(), so a failed Save is still exposed by List as a successful modification. The empty-candidate branch similarly reinserts the record with new created_at and updated_at values. Capture and restore the original timestamps together with the content so compensation returns the complete durable record to its previous state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58f93157a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if err != nil { | ||
| return "", err | ||
| } | ||
| tmp := path + ".tmp" |
There was a problem hiding this comment.
When two current instances use the same PGSTORE_LOCAL_PATH and save the same credential concurrently, both operate on this fixed temporary path before acquiring the advisory lock. One save can remove or overwrite the other save's staged output after it has been read, causing the first save to commit one payload to PostgreSQL but rename the other payload locally (or causing cleanup to delete another in-flight save), despite returning success. Create a unique temporary file per save and retain the advisory lock around final publication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 355e8c28b9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| table := s.fullTableName(s.cfg.AuthTable) | ||
| selectQuery := fmt.Sprintf("SELECT content, created_at, updated_at FROM %s WHERE id = $1 FOR UPDATE", table) | ||
| updateQuery := fmt.Sprintf("UPDATE %s SET content = $2, updated_at = NOW() WHERE id = $1", table) |
There was a problem hiding this comment.
When both the local file and PostgreSQL already contain the candidate JSON, replaceAuthRecord still executes this update before Save checks jsonEqual(localPrevious, candidate). As a result, a no-op metadata save now advances updated_at, whereas the previous implementation returned without modifying the record; List and the management API then report a credential modification that never occurred. Compare the durable content before updating, or otherwise retain the existing timestamp when it is unchanged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54f56fc51f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| func (s *PostgresStore) deleteAuthRecord(ctx context.Context, relID string) error { | ||
| return s.withAuthLock(ctx, relID, func(conn *sql.Conn) error { |
There was a problem hiding this comment.
When two instances share PGSTORE_LOCAL_PATH, Delete removes the file before reaching this newly locked database deletion. If another instance is inside Save after committing the database update but before publishing its staged file, the delete removes the old file and waits here; the save then renames its staged file into place, after which the delete acquires the lock, deletes only the database row, and returns success while the credential file remains (and the watcher may re-register or re-persist it). Move the local removal under the same advisory lock, or remove it again while holding that lock.
Useful? React with 👍 / 👎.
|
we didn't want to let CPA use pgsql. |
|
Ah, do you mean you’re planning to remove PostgreSQL support? |
2 participants
A PostgreSQL auth save can currently replace the local credential file before the database write succeeds. A database failure then leaves the local file ahead of durable state, and an unchanged-local retry can skip persistence.
This stages credential output and commits it to PostgreSQL before publishing the local file. If publication fails, it compensates the database write while retaining a per-credential session advisory lock. Save, deletion, and watcher persistence use the same lock, keyed by the resolved table identity, so another participating writer cannot complete a save during compensation. The existing database schema is unchanged.
This requires direct PostgreSQL connections or session pooling; transaction pooling is unsupported. Older instances must be stopped before upgrading writers that share an auth table. The connection and upgrade requirements are documented in
.env.example.The PostgreSQL integration tests exercise concurrent same-content writes, JSONB-equivalent content, deletion, watcher persistence, qualified table names, and cancellation during publication. Those nine cases passed ten consecutive runs with the race detector against PostgreSQL 16.