Skip to content
This repository was archived by the owner on Nov 7, 2026. It is now read-only.

fix: reconcile restored upload target during recovery drill - #95

Closed
camster91 wants to merge 44 commits into
mainfrom
fix/restore-drill-upload-target-sep4
Closed

camster91 wants to merge 44 commits into
mainfrom
fix/restore-drill-upload-target-sep4

Conversation

@camster91

@camster91 camster91 commented Sep 5, 2026 •

Copy link
Copy Markdown
Owner

Purpose

Harden issue #29 backup/restore tooling so a later disposable runtime proof cannot succeed against the wrong upload tree, a contaminated database, unsafe schema metadata, a raced restore target, hidden prepared work, an incomplete upload archive, user-created objects hidden in system-named schemas, or an unrecoverable connection freeze.

Exact reviewed head

1f0877757c79e16ebdc0b91b68f1ecf8059449c3

Final focused recovery contract

  • normal startup still reconciles only default ./uploads;
  • restore-only upload targets require an explicit absolute path and restore confirmation;
  • backup/restore roots reject unsafe or symbolic-link path components;
  • backup rejects symlinks, hard-linked files, and non-regular upload entries;
  • restore fully validates archive members and rejects links/FIFOs/devices/special entries;
  • restore requires a dedicated disposable PostgreSQL database and validates broad user-defined catalogs, default ACLs, prepared transactions, public-schema ownership/ACL, and high-OID user-created objects in system-named schemas;
  • target connections are frozen only after the maintenance cleanup path has proved it can execute the exact re-enable operation;
  • competing sessions are terminated/asserted and preflight + generated restore SQL run in one protected connection;
  • restore never uses pg_restore --clean;
  • restored attachment rows must all have matching regular upload files before reconciliation reports success;
  • normal startup behaviour remains separate from restore-only completeness checks;
  • OPERATIONS.md and contract tests pin these fail-closed boundaries.

Scope

Exact base: b45170409543684a75579a13d7bdd6d8f8ffe8e8.

Effective diff remains exactly five recovery-focused files:

  • OPERATIONS.md
  • scripts/backup.sh
  • scripts/reconcile-uploads.ts
  • scripts/restore-drill.sh
  • tests/backup-restore.test.ts

No deployment, production data, Docker host, DNS, email provider, billing, dependencies, migrations, product features, or CI workflow changes.

Exact-head verification

For 1f0877757c79e16ebdc0b91b68f1ecf8059449c3:

  • Codex manual review completed with no remaining major issues after the final test-contract correction;
  • Ashbi Local CI passed isolated install, Prisma generation, TypeScript, ESLint, focused auth/navigation/security tests, and production Next build;
  • GitGuardian passed with no secrets detected;
  • all inline recovery findings are resolved;
  • GitHub-hosted CI Build / Build failed before executing workflow steps, consistent with the existing hosted-runner incident rather than an observed application/test failure.

Approval boundary

This PR is ready for merge review, but merging remains a separate explicit owner approval. #29 external Docker/restore execution remains another separate action-time approval after merge and must bind to the resulting main SHA/image. This PR is correctness preparation only and is not runtime restore evidence.

Copy link
Copy Markdown
Owner Author

@codex review

Please review exact head bdaff4923d022b576ce039fc32513add939ea55d, focusing on restore safety: the restored upload directory must be the directory reconciled, normal production startup must retain the default ./uploads behavior, and no path handling should make destructive reconciliation less safe.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 5, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-06T06:50:09.980819Z 1f08777 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bdaff4923d

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/reconcile-uploads.ts Resolved
@chatgpt-codex-connector

Copy link
Copy Markdown

💡 Codex Review

if (!path.isAbsolute(configured)) {
throw new Error("SYNAPSE_UPLOAD_DIR must be an absolute path when set");
}
return configured;

P1 Badge Restrict the upload-directory override to safe restore targets

If SYNAPSE_UPLOAD_DIR is inherited by a normal invocation of uploads:reconcile, this accepts any absolute path—including /, $HOME, or a mistyped parent directory—and then deletes every unreferenced regular file directly beneath it. The restore wrapper rejects some unsafe targets and requires emptiness, but the reconciliation command itself is also run during production startup, while the application upload code remains hardcoded to ./uploads; therefore an ambient override can both reconcile the wrong directory and cause unrelated data loss. Gate the override specifically to the validated restore flow or enforce equivalent disposable-target safety before returning it.

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copy link
Copy Markdown
Owner Author

@codex review

Please re-review exact head 4073162c279bfeba8273dfdebbf95f9bd0103e51. Prior safety finding was addressed by restore-only confirmation gating, unsafe-root rejection, symlink-target rejection, and tests pinning both the guarded override and unchanged default startup path.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4073162c27

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/restore-drill.sh Resolved

Copy link
Copy Markdown
Owner Author

@codex review

Please review exact head 72baa3cef65346dfc2000931040945ed32e9fc33. The restore target is now passed only as an explicit command argument, never through ambient SYNAPSE_UPLOAD_DIR; an explicit target still requires restore-only confirmation, absolute-path validation, unsafe-root/home/cwd rejection, and the restore wrapper rejects symlink/non-empty targets.

Copy link
Copy Markdown
Owner Author

@codex review

Please review exact head 4cb300cd7d6934bd23b44c7d483998f065427b7d. Focus on destructive-restore safety: explicit upload target only (no ambient override), restore-only confirmation, unsafe upload-target rejection, actual empty-database preflight before import, and no pg_restore --clean behavior.

Copy link
Copy Markdown
Owner Author

@codex review

Please review exact head 44cc27fcb4fd08d456871edd4d07ab9be2a80162 as the current frozen recovery fix. Focus on: backup upload-root/link safety; archive path/link rejection; verified-empty restore database with no pg_restore --clean; explicit restore upload target only; and preservation of normal ./uploads startup behavior.

Copy link
Copy Markdown
Owner Author

@codex review

Final focused review request for exact head b4edb4ab6acc98e09c07d179965f3845011f20e9. Scope is frozen to five recovery-focused files. Please verify backup root/link safety, archive path/link rejection, independently empty restore DB/no --clean, explicit restore upload target, unchanged normal startup default, and runbook accuracy.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b4edb4ab6a

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/restore-drill.sh Resolved
Comment thread scripts/backup.sh Resolved
Comment thread scripts/restore-drill.sh Resolved
Comment thread scripts/backup.sh Resolved

Copy link
Copy Markdown
Owner Author

@codex review

Please review exact head f8de3ee3180a455634a3261f82020675795e85fe. The two unresolved findings from the prior review are fixed: target-database prepared transactions are rejected, and restore-only upload reconciliation now fails when any restored attachment row lacks its matching regular file. Please verify there are no new recovery-safety findings across the frozen five-file scope.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: f8de3ee318

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@camster91
camster91 marked this pull request as ready for review September 5, 2026 16:07

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f8de3ee318

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/restore-drill.sh Resolved

Copy link
Copy Markdown
Owner Author

@codex address that feedback

Address the one remaining unresolved review finding on scripts/restore-drill.sh: user-created namespace-owned objects can exist inside system-named schemas (for example a high-OID function in pg_catalog) and evade the empty-target gate. Make the narrowest fail-closed fix on this PR branch only, distinguish normal built-in/template objects from user-created entries using OID and/or dependency metadata, cover the relevant namespace-owned catalogs already checked by the preflight, pin the boundary in tests/backup-restore.test.ts, and update the corresponding restore paragraph in OPERATIONS.md. Preserve the current five-file scope. Do not change product runtime behaviour, dependencies, migrations, CI, deployment, or any external environment. After the edit, leave the PR draft for exact-head review.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

Copy link
Copy Markdown
Owner Author

@codex review

Please review exact head 431d7da80c9bac56aa16423ab61a235a176b6fd8. Focus on the one newly fixed finding: the protected restore preflight now rejects user-created high-OID namespace-owned objects in pg_catalog/information_schema across the relevant catalogs while preserving ordinary low-OID built-in/initdb objects. Confirm that this does not introduce false-positive or fail-open recovery behavior and that the five-file scope remains coherent. Do not modify the branch.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 431d7da80c

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@camster91
camster91 marked this pull request as ready for review September 5, 2026 17:17

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 431d7da80c

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/restore-drill.sh Resolved
@camster91
camster91 marked this pull request as draft September 6, 2026 06:32

@camster91 camster91 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh exact-head validation requested for f4dd3c9c1bbc833b314de13a7a76a846fdcd40c3. The latest cleanup-path finding is fixed: the maintenance connection now proves the exact idempotent ALTER DATABASE … ALLOW_CONNECTIONS true operation before any target freeze. Please verify no new recovery-safety findings across the unchanged five-file scope.

Copy link
Copy Markdown
Owner Author

@codex review

Please review exact head f4dd3c9c1bbc833b314de13a7a76a846fdcd40c3. The latest finding is fixed by proving the exact maintenance cleanup operation (ALTER DATABASE <target> ALLOW_CONNECTIONS true) succeeds through the derived postgres maintenance connection before any target freeze. Contract tests pin that preflight before connections_closed=1, and OPERATIONS.md documents it. Please verify no new recovery-safety findings across the unchanged five-file scope.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f4dd3c9c1b

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/restore-drill.sh Resolved

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved: Cursor Security Agent completed successfully with no findings requiring human review, and Cursor Bugbot was not running on this PR. No reviewers were assigned.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

Copy link
Copy Markdown
Owner Author

@codex review

Please review exact head 1f0877757c79e16ebdc0b91b68f1ecf8059449c3. The only change from the last reviewed head is the test-contract correction for the stale restore URL variable-name assertion: it now verifies searchParams.delete('schema') behaviour rather than requiring the old local identifier url. Recovery/runtime logic is unchanged. Please confirm no remaining findings across the five-file scope.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. What shall we delve into next?

Reviewed commit: 1f0877757c

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@camster91
camster91 marked this pull request as ready for review September 6, 2026 06:45

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1f0877757c

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +57 to +59
const missingReferenced = [...referenced].filter(
(filename) => !regularFiles.has(filename)
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Quiesce uploads while creating the backup

When an attachment deletion overlaps npm run backup after pg_dump but before tar, stageFilesForDeletion can move the file into .trash while the dump still retains its attachment row; backup then exits successfully, but this new top-level completeness check rejects the resulting artifact as unrestorable. Fresh evidence beyond the prior comment is that the current code now detects the missing file during restore, exposing that the producer still certifies an archive its own restore workflow cannot accept. Quiesce upload mutations or coordinate/validate the database and filesystem snapshots before reporting backup success.

Useful? React with 👍 / 👎.

@camster91 camster91 closed this Nov 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant